Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-55582] Converting bundled modules to detached plugins #3988

Closed
wants to merge 13 commits into from

Conversation

jglick
Copy link
Member

@jglick jglick commented Apr 17, 2019

See JENKINS-55582. While the jenkins-module packaging remains an option for hypothetical use cases such as a custom ConfidentialStore implementation, it turns out that none of the existing modules had any compelling reason to be modules: they were built and maintained like plugins with some @Extensions that could be loaded from a regular plugin class loader during the regular initialization sequence. Distributing them as modules offered no benefit, and had several drawbacks: complexity and inconsistency; difficulty updating functionality quickly (modules had to be delivered on the core LTS cycle); and a confusing mess in plugin POMs needing access to these classes, since you needed to declare a provided dependency on the exact version that happened to be bundled in your currently selected jenkins.version.

The existing “split plugin” system, for retaining (binary) compatibility of other plugins that might be depending on these classes, sufficed to mark the split point just like any classes moved out of core. The only real difference is that the handful of consuming plugins (e.g., callers of InstanceIdentity) were explicitly including the dependency before. I have not changed groupId/artifactId so source updates on the caller side should just look like a plain update, except now you can switch to the regular compile scope (i.e., the default) and you will get an explicit entry in Plugin-Dependencies as you should.

The JNLP 4 protocol still needs the instance-identity plugin loaded in order to work (though in practice you would have a hard time disabling any of these new plugins in the near future, due to automatic dependencies). Resurrecting #2875 would clean this up. Other than that, there is very little in the way of code changes here; it is mostly rearranging some metadata.

Subsumes #3987 to aid testing. Downstream of

Manual testing

This page describes how to see one the newly detached agent installer plugins being used in a realistic class loader environment. If you first run, say, 2.164.2 and then upgrade to the WAR from this branch, the detached plugins are all installed automatically. The same is true if any other plugin (built against Jenkins 2.173 or earlier) is installed.

SSH functionality was also tested. With the new sshd plugin enabled, you can configure a port, attach your public key to your user configuration, and run commands like

ssh -p 2222 admin@localhost who-am-i

You can also install workflow-cps-global-lib and use the (now deprecated) in-master Git server, workflowLibs.git, via SSH protocol.

Proposed changelog entries

  • Some components of Jenkins which were previously packaged as “modules” are now regular plugins, and can be managed normally including updating from the update center. These components handle an identity for inbound (“JNLP”) agent connections; an SSH server; and hooks to offer service installation for JNLP agents.
  • “Detached” plugins—those split out of Jenkins core at some point, including the former “modules”—will now be installed upon Jenkins startup when needed as implied dependencies of other plugins which were already present. This simplifies compatibility for specialized installation scenarios not using the update center, such as when Jenkins is run from a Docker image prepackaged with some plugins.

Desired reviewers

@jenkinsci/code-reviewers

jglick added 4 commits April 16, 2019 14:08
This happens when locally testing split plugins (despite their presence in WEB-INF/detached-plugins/*.hpi).
…tity is missing, as it may be after JENKINS-55582.

Cleaner would be to implement the JENKINS-44100 split so that JnlpSlaveAgentProtocol4 is in a plugin depending on instance-identity.
@daniel-beck
Copy link
Member

Choosing the recommended plugin set in the wizard today installs 70 plugins. This adds another 8 through implied dependencies for the next few years probably. All while we have a plugin management UI written to expose a plugin ecosystem that has 100 plugins total.

I'm not sure the benefits here outweigh the drawbacks for Jenkins admins.

@jglick jglick changed the title Converting bundled modules to detached plugins [JENKINS-55582] Converting bundled modules to detached plugins Apr 17, 2019
@jglick
Copy link
Member Author

jglick commented Apr 17, 2019

we have a plugin management UI written to expose a plugin ecosystem that has 100 plugins total

Not sure what you mean by that. The plugin count in the UC is pushing 2k. Our plugin management UI could certainly be improved in various ways, but we need to make those improvements anyway.

Consider the reverse move. We have a working module system that allows you to add features to a core application. Now say someone proposes to take a somewhat arbitrary selection of ~0.5% of those modules and, for no particular technical reason, pull them out of the visible list of managed modules and instead load them in a special-cased manifest, complicating the overall system architecture. Silly.

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to firstly fix known issues with plugin management. E.g. Docker install-plugins does not install detached plugins automatically. So such change would nuke pretty much every Docker instance due to instance-identity.

My strong preference is to have a JEP for such change so that it can be discussed properly in the community. Otherwise the change may produce a lot of maintenance burden on Jenkins users and maintainers

Copy link
Member

@oleg-nenashev oleg-nenashev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feedback needs to be addressed before merging

@jglick
Copy link
Member Author

jglick commented Apr 17, 2019

Docker install-plugins does not install detached plugins automatically.

Can you give me a reference please? There are a lot of systems for running Jenkins from Docker containers and managing the plugins therein, and I am not sure which specific tool you are referring to here.

@daniel-beck
Copy link
Member

@jglick
Copy link
Member Author

jglick commented Apr 17, 2019

Thanks, I will check interaction with that. It is unlikely the situation is different from any other plugin split (most recently jdk-tool), but if there is indeed a problem it may be possible to ameliorate it either in core or in that script.

@jglick
Copy link
Member Author

jglick commented Apr 19, 2019

Confirmed that the Jenkins plugin manager was failing to install detached plugins in the Docker scenario, where you start Jenkins with some stuff in $JENKINS_HOME/plugins/*.jpi but there is no jenkins.install.InstallUtil.lastExecVersion. This would actually have been more of a problem for previous plugin splits in which some of the split classes were likely to have been linked to from loaded plugins, as compared to the relatively uncommon uses of InstanceIdentity and SshCommandFactory. At any rate, I have fixed it here with a functional test, and done some interactive sanity checks using examples such as

FROM jenkins/jenkins:2.173
RUN /usr/local/bin/install-plugins.sh build-token-root aws-device-farm workflow-aggregator
COPY jenkins.war /usr/share/jenkins/jenkins.war
#!/bin/sh
set -e
mvn -f …/jenkins -pl core,war -DskipTests clean install
cp -v …/jenkins/war/target/jenkins.war jenkins.war
docker build -t docker-modules-jenkins-55582 .
docker run --rm -p 127.0.0.1:8080:8080 -e JAVA_OPTS=-Dhudson.Main.development=true docker-modules-jenkins-55582

@oleg-nenashev oleg-nenashev added needs-more-reviews Complex change, which would benefit from more eyes on-hold This pull request depends on another event/release, and it cannot be merged right now labels Apr 26, 2019
@oleg-nenashev
Copy link
Member

@jglick would it be possible to detach #3988 (comment) to a separate pull request? It is a really important fix for Jenkins users, and we may want to land it ahead of the modules split

@jglick
Copy link
Member Author

jglick commented May 7, 2019

I already did that.

@varyvol
Copy link

varyvol commented Nov 28, 2019

@jglick do you intend to keep working on this? Otherwise we could propose it for close and it can be reopen when necessary.

@jglick
Copy link
Member Author

jglick commented Nov 28, 2019

There has been some recent discussion around agent.jar signing, relevant here in that if it were not signed, javaws would not work, GUI mode would be unavailable, and five of these modules would be useless. I am inclined to detach them without bundling. instance-identity could perhaps just be made part of core. The two SSH-related modules/plugins would still make sense as detached, I think.

@jglick
Copy link
Member Author

jglick commented Dec 2, 2019

Can be closed for now I guess, and as time permits I can propose incremental patches.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-more-reviews Complex change, which would benefit from more eyes on-hold This pull request depends on another event/release, and it cannot be merged right now
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants