[JENKINS-55174] Update JBoss marshalling and unignore fixed tests

[dwnusbaum]

See JENKINS-55174. Based against #84.

It looks like the root cause of the NPE we were seeing was JBMAR-221, which was fixed in JBoss Marshalling 2.0.6.Final, so I've updated to that version and unignored the previously-failing tests.

CC @jenkinsci/java11-support

[dwnusbaum]

mvn dependency:tree shows that we already pick up org.jenkins-ci:annotation-indexer:jar:1.12:provided through core, so better to remove this.

[alecharp]

I do understand the point, but I disagree. If you use a library, you should say so in your pom.xml. See mvn dependency:analyze, you will have "undeclared" libraries. The point is, if you don't list the dependency, and the other one which was providing it by transitivity stop using it (won't happen with core, yes), then you might have issues during an upgrade.

[dwnusbaum]

Yes, my opinion would be different if the library was not in core, but depending on a specific version of the library here just opens us up to accidental version mismatches with core which could be problematic.

[MRamonLeon]

What a simple solution, after all 😮

[batmat]

Thanks a lot Devin!

[MarkEWaite]

I've deployed the incrementals build of this pull request in my JDK 11 test installation. It no longer reports a null pointer exception when Jenkins restarts (using the "/safeRestart" page) with a Pipeline job in progress.

Unfortunately, one or more of the Pipeline jobs that were in progress seem to fail. I tried with Pipeline jobs that were building the git client plugin, the git plugin, and the platform labeler plugin. Builds of the git client plugin and the platform labeler plugin failed when safeRestart was used to restart the server.

The Pipeline persistence setting is "Performance optimized: much faster - (requires clean shutdown to save running pipelines)". I believe the "/safeRestart" technique qualifies as a clean shutdown, since it blocks until all running Freestyle jobs have completed.

This change is better than the current state. I agree with the approvers that this should be merged. It doesn't seem to have solved all the issues, but it has resolved the NPE that I was seeing.

[dwnusbaum]

@MarkEWaite The wire format of JBoss marshalling is not guaranteed to be backwards compatible across version changes, so that might just have been an issue across the restart where you upgraded the plugin. Do you still see that issue on restarts for in-progress builds even when using only the same version of the plugin across restarts?

Also, if you have stack traces or logs for those earlier failures, I would love to take a look at them to try to understand what happened.

[MarkEWaite]

Yes @dwnusbaum I see the same issue when restarting with no change to plugin version. I've been through several restart cycles to see if the failures are consistent.

I've now changed the consistency setting for maximum consistency (worst performance) and will try a restart with jobs in progress. I'll check the status after the restart and let you know the results

[MarkEWaite]

It appears that buildPlugin sets the consistency to performance optimized, which overrides my attempt to set it for the entire Jenkins instance. I'll need to find a long-running Pipeline job that is not building a plugin before I can correctly test with maximum consistency.

In the interim, shall I open a Jira ticket for the new behavior so that we can track it in a Jira ticket rather than in comments on this pull request?

[dwnusbaum]

@MarkEWaite Yeah I'd go ahead and open a ticket, and please include any logs/error messages that occur at the time of the failure. FWIW, I have not been able to reproduce any issues across restarts under any durability setting and the latest Pipeline-related plugins, although my Pipelines are relatively simple and I am running Jenkins using Java 8.

[dwnusbaum]

I ran workflow-job and workflow-cps tests with this version of workflow-support just in case any interesting failures turned up. I saw 2 failures:

• WorkflowRunTest#loadMigratedBuildRecord
• WorkflowRunTest#stepRunningAcrossUpgrade

These failures are expected due to changes in the binary format of program.dat, and we'll need to recreate the data used in @LocalData in those tests using the Java 11-compatible version of workflow-support for them to pass. They do not represent a problem for real-world usage of the plugin.

[svanoort]

I can confirm that this resolves the issues, and in cross-testing with workflow-cps tests it does not introduce regressions. There are failures with the fuzzer tests in workflow-cps but those failures are also present independently of JBoss marshalling changes, with modern workflow-support version. Edit: and those failures are not necessarily critical -- they often represent rare and timing-sensitive issues, and should be addressed independently via other in-flight fixes.

The failures noted in #77 are absent.

Merging into the java11 branch now, would suggest we cut a release of this tomorrow if there are no objections -- we can remove the requirement for modern Java versions as long as the core dependency is high enough.

[batmat]

Merging into the java11 branch now, would suggest we cut a release of this tomorrow

Yes, please! :-) Fully agree. Having a release for the plugin that fixes issue and moves the ball forward does make perfect sense.