[FIXED JENKINS-41196] allow plugins to override core components #2754

kohsuke · 2017-02-17T20:05:36Z

See this example for the actual example of a plugin that overrides core component.

war file now contains very small bootstrap in WEB-INF/lib
bootstrap builds "core classloader" from WEB-INF/jars which is where most of the jar files now live
plugins who want to override core components have to have a manifest entry.
bootstrap considers that in building the classpath.
version consistency check is performed on every jar that's getting overridden. Any attempt to "downgrade" a component from what's in core will result in the plugin getting rejected.

Still more to do:

NAH: If a core override is rejected, that plugin should be marked as failed. Currently, the error message just gets logged and the plugin will load like a normal plugin. This is necessary for some plugins (like BO) to require a newer version of Stapler.
do not double-load jar files in plugin classloader so as not to have two copies of the same jar (which trips up META-INF/services lookup for example)

Otherwise we end up using that class before it gets initialized

Implemented the bootstrap logic and necessary packaging change to load the core from a separate classloader.

Jelly's <invokeStatic> tries to load a class from context classloader, so we need it to be core loader, not bootstrap loader.

Conflicts: core/src/main/java/hudson/WebAppMain.java war/src/main/webapp/WEB-INF/web.xml

kohsuke · 2017-02-17T20:24:37Z

If a core override is rejected, that plugin should be marked as failed. Currently, the error message just gets logged and the plugin will load like a normal plugin. This is necessary for some plugins (like BO) to require a newer version of Stapler.

On further thought I think this is wrong, because if BO specifies a specific version of stapler-plugin to be a dependency and if core is newer than that, then we want BO to run normally, instead of failing. So I think the current behavior is actually sufficient.

kohsuke · 2017-02-17T20:25:07Z

@reviewbybees

If someone is enumerating resources, it can end up twice

If the bootstrap fails, we want "java -jar jenkins.war" to terminate.

oleg-nenashev · 2017-02-17T20:38:46Z

bootstrap/pom.xml

+<!--
+The MIT License
+
+Copyright (c) 2004-2010, Sun Microsystems, Inc., Kohsuke Kawaguchi,


Smells like an outdated header

This (and the one below you commented) is because I copied this file from core/pom.xml. I don't know how much of pom is copyrightable, but I think I'll just leave it in.

If you have modified the Pom you should at least update the date range (unclear if this was personal time or work time project so adding employer is your personal due diligence)

oleg-nenashev · 2017-02-17T20:43:06Z

bootstrap/pom.xml

+  <description>Hands off the execution to jenkins-core</description>
+
+  <properties>
+    <!-- TODO: Actually many issues are being filtered by src/findbugs/findbugs-excludes.xml -->


Yes, FindBugs takes only high-severity issues. IIRC we have about 400 of the medium-priority. I have started working on cleaning up/filtering them in #2519 , but have not finished this PR yet. Hope to revisit once I have some time for it.

FTR https://issues.jenkins-ci.org/browse/JENKINS-36716

oleg-nenashev · 2017-02-17T20:43:36Z

bootstrap/pom.xml

+  <dependencies>
+    <dependency>
+      <groupId>org.jenkins-ci</groupId>
+      <artifactId>version-number</artifactId>


Maybe it could be moved to the parent POM

See 3377523. I think it's error prone to have this dependency in multiple places.

oleg-nenashev · 2017-02-17T20:44:46Z

bootstrap/src/main/java/jenkins/bootstrap/BootLogic.java

+ * Bootstrap code uses {@link ServiceLoader} to look for subtypes of this marker interface
+ * and invokes {@link #contextInitialized(ServletContextEvent)} and {@link #contextDestroyed(ServletContextEvent)}
+ *
+ * @author Kohsuke Kawaguchi


since Javadocs for new API classes would be useful (since we really want to implement them). Or @Restricted otherwise

Has to wait until the code gets merged since we don't know which version this will go live.

We commonly use @since TODO in such case

Okay, 8bdeb05

oleg-nenashev · 2017-02-17T20:46:25Z

bootstrap/src/main/java/jenkins/bootstrap/Bootstrap.java

+    /**
+     * JENKINS_HOME
+     */
+    private File home;


NIT: Maybe makes sense to use Path and the java.io.Files API with exceptions for the new code. Otherwise error handling may be painful (and FindBugs will grumble)

Unrelated change that should be handled separately.

oleg-nenashev · 2017-02-17T20:57:51Z

bootstrap/src/main/java/jenkins/bootstrap/Bootstrap.java

+    }
+
+    private String getString(String key) {
+        String value = System.getProperty(key); // keep passing on any exceptions


SystemProperties (?) Just to support web.xml overrides & Co

Yes. IMHO this method duplicates SystemProperties logic. Likely we should move it somewhere to a shared lib (required for the stuff like remoting anyway)

I didn't want to bring a large class like that into bootstrap, and it's not like there's a lot of duplicate here, so I think this is OK.

oleg-nenashev · 2017-02-17T21:00:04Z

bootstrap/src/main/java/jenkins/bootstrap/Bootstrap.java

+
+    private static final Logger LOGGER = Logger.getLogger(Bootstrap.class.getName());
+
+    private static final String[] HOME_NAMES = {"JENKINS_HOME","HUDSON_HOME"};


Should we follow the Oracle codestyle and keep these definitions in the beginning of the file? I do not care much, but we declare this codestyle somewhere

That's funny. I've worked for Sun Microsystems for close to 10 years and our coding convention has always been to put static fields & methods in the end. I just checked the coding convention PDF and it does say static fields before instance fields and no mention of static variables.

oleg-nenashev · 2017-02-17T21:01:08Z

bootstrap/src/main/java/jenkins/bootstrap/DependenciesTxt.java

+    public DependenciesTxt(InputStream i) throws IOException {
+        List<Dependency> list = new ArrayList<>();
+
+        try (Reader r = new InputStreamReader(i,"UTF-8");


Are we fine with hardcoding UTF? IIRC @jtnord has bugged one of my FindBugs PRs due to it

In practice this file contents is within the ASCII charset, so frankly it's not worth our time to discuss this one further.

oleg-nenashev · 2017-02-17T21:02:33Z

bootstrap/src/main/java/jenkins/bootstrap/DependenciesTxt.java

+
+            // line that we care about has 5 components: groupId:artifactId:packaging[:classifier]:version:scope
+            String line;
+            while ((line=br.readLine())!=null) {


readLine() may cause hanging of the logic. I am not sure we want to have it for external files. Would be better to define a timeout

There must be misunderstanding. This is just to read dependencies.txt from a jar file.

oleg-nenashev · 2017-02-17T21:03:20Z

bootstrap/src/main/java/jenkins/bootstrap/DependenciesTxt.java

+    /**
+     * Attempts to reverse map a jar file name back to {@link Dependency}
+     */
+    public Dependency fromFileName(String jarFileName) {


I know what @stephenc may say, but annotations would not hurt here. NIT

well when I see no annotations I assume that means Nullable. Iow the caller is responsible for knowing when null is appropriate.

If the caller is abdicated of that responsible then CheckForNull is useful to indicate the method is taking the responsibility for the caller.

NonNull is saying never give me a null.

As the default is Nullable, I do not see adding annotations as being ever a blocking issue with a PR review.

That said, there are times where adding annotations is very useful, and gentle reminders are ok. For example when defining an API / SPI I would see annotations as almost mandatory. But when creating an implementation of the api/spi or when writing code that is not intended to be used elsewhere I see them as very optional.

That's my €0.02 anyway

jars in the bootstrap override jars in the core, so duplicating it is not only verbose but also error prone.

daniel-beck · 2017-02-17T23:18:15Z

@jenkinsci/code-reviewers

... but I guess I've never actually bothered to read it!

jglick · 2017-09-05T15:19:39Z

Besides https://github.com/kohsuke/remoting-plugin there also seems to be https://github.com/ndeloof/stapler-plugin as example implementations. Do we expect these to become reactor modules of https://github.com/jenkinsci/remoting and https://github.com/stapler/stapler resp., so that we automatically get plugin releases every time we cut a component release?

jglick · 2017-09-05T15:23:22Z

Unclear what CI failures are about, but needs to be cleared up.

daniel-beck · 2017-09-05T17:39:30Z

that we automatically get plugin releases every time we cut a component release?

Please, no. We should at least wait the LTS soaking period, preferably longer, before such a plugin is released.

kohsuke · 2017-09-05T18:28:23Z

It looks like CLIActionTest gets into an infinite loop, puts stress to the test machine, and eats all the available disk space. I suspect this resulted in strange CI failures.

kohsuke · 2017-09-05T20:51:33Z

After investigation and conversation with Jesse, we believe we hit the same test problem that blocked #2959 from getting integrated.

The problem is that CLIActionTest.interleavedStdio() runs into a tight loop in PlainCLIProtocol.Reader.run() when it catches TimeoutException wrapped in IOException. Newer version of Jetty used in newer JTH makes this condition fatal and not retryable.

The result is that the test produce ungodly amount of log output, eating up all the disk space and preventing the successful completion of the tests.

Jesse said he'll incorporate a fix to this problem in #2959 and merge that first, which should unblock this PR.

jglick · 2017-09-06T00:48:06Z

We should at least wait the LTS soaking period, preferably longer, before such a plugin is released.

Why? Consider that if I cut a release of, say, script-security-plugin, that goes to the public update center in a matter of an hour or so. A mistake in that plugin is just as likely to cause catastrophic harm as a mistake in Stapler or Remoting, and I see no reason to think it is less likely either. Why are these core components any different? If you want a stable installation, do not update any plugins, or only update plugins to versions that have been pretested by some third party.

daniel-beck · 2017-09-06T00:56:36Z

A mistake in that plugin is just as likely to cause catastrophic harm as a mistake in Stapler

The history of terrible Stapler releases over the last year indicates that the likelihood of such a bug is greater. Notably, release of the plugin would precede the core PR to update the component (and the history of process irregularities surrounding merges without code review for Stapler also gives me no confidence that a "snapshot PR first" process would actually be followed). Which is when we repeatedly caught regressions.

Last review was about test results.

stephenc · 2017-09-06T08:21:30Z

Notably, release of the plugin would precede the core PR to update the component (and the history of process irregularities surrounding merges without code review for Stapler also gives me no confidence that a "snapshot PR first" process would actually be followed). Which is when we repeatedly caught regressions.

Though having the CI for stapler automatically produce the plugin with every PR would enable easier testing of the side-effects before release. Whereas at the moment you need to make a custom build of jenkins to upgrade these.

I think a better approach would be to set the update center to blacklist new versions of these plugins from the non-experimental update centres until they have been vetted, but have remoting and stapler produce their plugins with every release... thus easier testing of upgrades and less risk to users

jglick · 2017-09-06T13:56:45Z

at the moment you need to make a custom build of jenkins to upgrade these

You would anyway, because those components have only unit tests, and by far the most extensive tests of their functionality are in jenkinsci/jenkins/test/. Having them be plugins would only be convenient for manual sanity testing. To run the automated test suites, you need to have jenkinsci/jenkins/core/pom.xml specify the snapshot dependency.

Of course we can and should have a daily CI build which picks up master branches of components like these, so that integration test failures would be apparent before anyone considered cutting a release.

The history of terrible Stapler releases over the last year indicates that the likelihood of such a bug is greater.

Have you not been following script-security changelogs? :-)

…e hates maven-release-plugin

jglick · 2017-09-06T14:28:59Z

stapler-plugin seems to be bundling only org.kohsuke.stapler:stapler, i.e., Stapler core. But jenkins-core also includes stapler-groovy, stapler-jrebel, and stapler-jelly. (I am not sure what picks up stapler-jruby—ruby plugin does not appear to—and I think stapler-jsp is unused.) We need a ready-to-release proposed repository or submodule for this component to consider the core component fully up for review, as this was a critical use case.

jglick · 2017-09-06T14:39:05Z

set the update center to blacklist new versions of these plugins from the non-experimental update centres until they have been vetted, but have remoting and stapler produce their plugins with every release

Looks like this should be possible with minor changes to update-center2. I am going to play with it. Certainly much easier to deal with than having override plugins living in separate repositories.

…uts.

jglick · 2017-09-06T19:43:07Z

Finally have a clean CI build, at least. A fair amount of manual testing remains to be done.

ghost · 2017-09-15T15:30:34Z

This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation.

jglick

I think where this stands is that excepting some minor technical details, the change itself was fine, but the advent of Essentials and general changes in workflow and policy mean we do not need this much any more and it is perhaps not worth the added complexity. TBD.

batmat · 2019-01-10T10:22:58Z

Given this is conflicted and stalled, I'm proposing to close this.

I'll do it in the next days in nobody objects.

Thanks!

jglick · 2019-01-11T21:46:31Z

I'm proposing to close this.

I was just about to do that myself. Please close the PR and JENKINS-41196 as well. There are also downstream PRs that should be closed: jenkinsci/jenkins-test-harness#48, jenkinsci/maven-hpi-plugin#73, and jenkinsci/acceptance-test-harness#356.

batmat · 2019-01-17T22:10:14Z

@jglick done.

Also closing here as per my last comment.

Thanks everyone!

kohsuke added 8 commits February 16, 2017 16:39

Do not statically depend on SystemProperties class.

3cd1c36

Otherwise we end up using that class before it gets initialized

[JENKINS-41196]

d13004c

Implemented the bootstrap logic and necessary packaging change to load the core from a separate classloader.

Fix up context classloader.

77f0c87

Jelly's <invokeStatic> tries to load a class from context classloader, so we need it to be core loader, not bootstrap loader.

Improving robustness against d==null

dfbae6e

Handle classifier correctly

18e8e42

Leave the record of what core override plugin was activated

4b996bc

Honor disabled flag

7820660

Ant needs to be a part of the bootstrap

7a3915a

kohsuke added the work-in-progress The PR is under active development, not ready to the final review label Feb 17, 2017

Merge remote-tracking branch 'origin/master' into JENKINS-41196

654cc7d

Conflicts: core/src/main/java/hudson/WebAppMain.java war/src/main/webapp/WEB-INF/web.xml

kohsuke closed this Feb 17, 2017

kohsuke reopened this Feb 17, 2017

Forgot to add POM

0249f47

kohsuke added needs-more-reviews Complex change, which would benefit from more eyes and removed work-in-progress The PR is under active development, not ready to the final review labels Feb 17, 2017

Test case needed a modification

9e403e9

oleg-nenashev self-requested a review February 17, 2017 20:37

jglick self-assigned this Feb 17, 2017

kohsuke added 2 commits February 17, 2017 12:54

Avoid double-loading jar files into classpath

d9ec0e3

If someone is enumerating resources, it can end up twice

Using the latest version

46c2ec9

If the bootstrap fails, we want "java -jar jenkins.war" to terminate.

oleg-nenashev reviewed Feb 17, 2017

View reviewed changes

kohsuke added 3 commits February 17, 2017 15:01

removed unused dependency

a375fcb

This dependency now comes through bootstrap

3377523

jars in the bootstrap override jars in the core, so duplicating it is not only verbose but also error prone.

Correct ordering of the exception handler

8a5ce62

I thought I've been using Sun Coding Convention for 10+ years

d91db82

... but I guess I've never actually bothered to read it!

jglick mentioned this pull request Feb 17, 2017

It's OK not to have classes dir. jenkinsci/maven-hpi-plugin#51

Closed

In prep for when it gets merged.

8bdeb05

jglick mentioned this pull request Sep 6, 2017

Using RestartableJenkinsRule.then #2959

Merged

Merge branch 'master' into JENKINS-41196

1a5bbfd

jglick self-requested a review September 6, 2017 02:25

Also have to keep bootstrap/pom.xml up to date with existing POMs. /m…

c8abb79

…e hates maven-release-plugin

Pulling in jenkinsci/jenkins-test-harness#67 to better diagnose timeo…

a168a24

…uts.

daniel-beck added the unresolved-merge-conflict There is a merge conflict with the target branch. label Sep 15, 2017

olivergondza mentioned this pull request Sep 19, 2017

[JENKINS-41196] demonstrate the capability of loading core component jar jenkinsci/acceptance-test-harness#356

Closed

oleg-nenashev removed their assignment Jan 28, 2018

jglick reviewed Jun 26, 2018

View reviewed changes

batmat added the proposed-for-close There is no consensus about feasibility of this PR, and maintainers do not see a way forward for it label Jan 10, 2019

batmat closed this Jan 17, 2019

batmat deleted the JENKINS-41196 branch January 17, 2019 22:10

basil mentioned this pull request Feb 3, 2022

Support for systemd jenkinsci/packaging#266

Merged


		private static final Logger LOGGER = Logger.getLogger(Bootstrap.class.getName());

		private static final String[] HOME_NAMES = {"JENKINS_HOME","HUDSON_HOME"};

[FIXED JENKINS-41196] allow plugins to override core components #2754

[FIXED JENKINS-41196] allow plugins to override core components #2754

Conversation

kohsuke commented Feb 17, 2017 • edited Loading

kohsuke commented Feb 17, 2017

kohsuke commented Feb 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daniel-beck commented Feb 17, 2017

jglick commented Sep 5, 2017

jglick commented Sep 5, 2017

daniel-beck commented Sep 5, 2017

kohsuke commented Sep 5, 2017

kohsuke commented Sep 5, 2017

jglick commented Sep 6, 2017

daniel-beck commented Sep 6, 2017 • edited Loading

stephenc commented Sep 6, 2017

jglick commented Sep 6, 2017

jglick commented Sep 6, 2017

jglick commented Sep 6, 2017

jglick commented Sep 6, 2017

ghost commented Sep 15, 2017

jglick left a comment

Choose a reason for hiding this comment

batmat commented Jan 10, 2019

jglick commented Jan 11, 2019

batmat commented Jan 17, 2019

kohsuke commented Feb 17, 2017 •

edited

Loading

daniel-beck commented Sep 6, 2017 •

edited

Loading