Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407] #93

svanoort · 2018-03-28T15:56:09Z

Aims to resolve (in conjunction with workflow-cps changes as well that help harden the logic):

Includes lazy-load for FlowExecutions (enhancement): JENKINS-45585 -- which if merged will replace 49086 because it will defer a ton of the load-time logic for completed Pipelines.

TODOs:

Cross-test with workflow-cps durability tests
Fix issue with unitialized listener field for unable-to-resume flows (may mandate changes in wf-cps too)
Hardening: consider forcing atomic write of XML file when re-saving with the completed field persisted -- even when normally non-atomic (guarantees we won't mangle original build record if an error occurs upon save).

…instead

…ting permits

…ome minor cleanups

…ur, such as removing a Run from active execution

abayer

Need to come back and do a second pass on the logic around completed, but feels good so far.

abayer · 2018-03-30T15:04:43Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -171,6 +171,9 @@ public String url() {

    private transient boolean allowKill;

+    /** Controls whether or not our execution has been initialized via its {@link FlowExecution#onLoad(FlowExecutionOwner)} method yet if.*/
+    transient boolean executionLoaded = false;  // NonPrivate for tests


I actually wonder if that might be something we want to expose outside of just tests - I don't have a specific scenario in mind, but I know I've stumbled across cases where this could be useful information. I think. I may be getting confused with something else, though.

Can always expose it with a getter.

…if and when there is a downstream PR demonstrating the need yada yada.

abayer · 2018-03-30T15:05:33Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java


    /** map from node IDs to log positions from which we should copy text */
-    private Map<String,Long> logsToCopy;
+    Map<String,Long> logsToCopy;


Probably worth a // Non-private for testing comment, but not strictly necessary since that's basically the only reason anything we do ever is package-private.

abayer · 2018-03-30T15:07:00Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

+        return logCopyGuard;
+    }
+
+    /** Avoids creating new instances, analogous to {@link TaskListener} but as full StreamBuildListener. */


Think you meant TaskListener.NULL here?

jglick

Serial format of completed should be changed.

As to the implementation, sounds OK, though the changes are too extensive for me to really review seriously (in part due to reindentation perhaps?), so I hope tests will catch any mistakes.

jglick · 2018-04-03T21:44:03Z

pom.xml

@@ -68,6 +68,7 @@
        <workflow-support-plugin.version>2.17</workflow-support-plugin.version>
        <scm-api-plugin.version>2.1.1</scm-api-plugin.version>
        <git-plugin.version>3.2.0</git-plugin.version>
+        <jenkins-test-harness.version>2.33</jenkins-test-harness.version>


Just update parent POM to 3.4 or newer.

jglick · 2018-04-03T21:44:37Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -171,6 +171,9 @@ public String url() {

    private transient boolean allowKill;

+    /** Controls whether or not our execution has been initialized via its {@link FlowExecution#onLoad(FlowExecutionOwner)} method yet if.*/


stray if?

jglick · 2018-04-03T21:45:06Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -171,6 +171,9 @@ public String url() {

    private transient boolean allowKill;

+    /** Controls whether or not our execution has been initialized via its {@link FlowExecution#onLoad(FlowExecutionOwner)} method yet if.*/
+    transient boolean executionLoaded = false;  // NonPrivate for tests


…if and when there is a downstream PR demonstrating the need yada yada.

jglick · 2018-04-03T21:49:53Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

     */
-    private transient AtomicBoolean completed;
+    AtomicBoolean completed;  // Non-private for testing


No, you really do not want this to be a serialized field of the build.xml:

<completed> <value>1</value> </completed>

Ugh! Use boolean completed (or Boolean if null is a meaningful state, or an enum) and if you need to synchronize access to it against something, I guess use logCopyGuard.

jglick · 2018-04-03T21:51:07Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-    private transient AtomicBoolean completed;
+    AtomicBoolean completed;  // Non-private for testing
+
+    private transient Object logCopyGuard = new Object();


Given getLogCopyGuard, could drop the field initializer.

jglick · 2018-04-03T22:00:19Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

@@ -506,9 +535,9 @@ private void copyLogs() {

                    String prefix = getLogPrefix(node);
                    if (prefix != null) {
-                        logger = new LogLinePrefixOutputFilter(listener.getLogger(), "[" + prefix + "] ");
+                        logger = new LogLinePrefixOutputFilter(getListener().getLogger(), "[" + prefix + "] ");


Tons of merge conflicts with #27 coming up, sigh. Was the introduction of getListener() necessary to solve one of these bugs?

Yep, I didn't want to introduce any more conflicts than needed but unfortunately this was the cleanest way to guard against NPEs with the listener that were seen in tests. FWIW I'll be happy to deal with the conflict resolution as part of the log rewrite integration since I'm introducing most of the conflicts -- seems only fair.

Nah, person creating branch resolves merge conflicts. I will deal with it soon.

jglick · 2018-04-03T22:02:50Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-        Timer.get().submit(() -> {
+            LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[]{toString(), getResult()});
+            if (listener == null) {
+                // Can we actually rely on this, now that we use an initializing getter?


Is this a TODO?

Something like that, more of a "I wonder what @jglick will say in review"

jglick · 2018-04-03T22:03:39Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

+                            FlowExecutionListener.fireResumed(fetchedExecution);
+
+                            getListener().getLogger().println("Resuming build at " + new Date() + " after Jenkins restart");
+                            Timer.get().submit(new Runnable() { // JENKINS-31614


As long as you are shuffling code around, might as well:

Timer.get().submit(() -> Queue.getInstance().schedule(new AfterRestartTask(this), 0)); // JENKINS-31614

jglick · 2018-04-03T22:05:55Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-                });
+                }
+            }
+        } finally {  // Ensure the run is ALWAYS removed from loading even if something failed, so threads awaken.


/me runs away in fear

jglick · 2018-04-03T22:07:42Z

src/test/java/org/jenkinsci/plugins/workflow/job/WorkflowRunRestartTest.java

+    @Test public void lazyLoadExecution() {
+        story.thenWithHardShutdown(r -> {
+            WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+            p.setDefinition(new CpsFlowDefinition("echo 'dosomething'", true));


Do you need to set the durability mode too? Best to not rely on defaults if it is significant here.

svanoort · 2018-04-03T22:33:19Z

@jglick Does this help with Whitespace? https://github.com/jenkinsci/workflow-job-plugin/pull/93/files?w=1

…round of robustness fixes

rsandell · 2018-04-06T13:48:29Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-    /** null until started, or after serious failures or hard kill */
-    private @CheckForNull FlowExecution execution;
+    /** Null until started, or after serious failures or hard kill. */
+    @CheckForNull FlowExecution execution; // Not private for test use only


then I guess should be @Restricted?

Nah, it's package-private, so it's already more restricted than if I used @Restricted on it

rsandell · 2018-04-06T13:51:08Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

+    static final StreamBuildListener NULL_LISTENER = new StreamBuildListener(new NullStream());
+
+    /** Used internally to ensure listener has been initialized correctly. */
+    StreamBuildListener getListener() {


synchronized?

Good catch. I believe there was a specific reason why it wasn't based on testing but... darned if I can remember the issue/exact chain of reasoning since it was rather complex.

Give me a few, I'll either synchronize or provide a detailed comment to explain the rationale why not to.

rsandell · 2018-04-06T14:19:38Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-            LOGGER.log(Level.WARNING, "failed to save " + this, x);
-        }
-        Timer.get().submit(() -> {
+            LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[]{toString(), getResult()});


Is Level.INFO really wise here?

Well, that's what it was at before -- it should only run once per build (or at most twice in rare circumstances if something fails catastrophically right in the middle of the finalization).

rsandell · 2018-04-06T14:20:53Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

-        }
-        Timer.get().submit(() -> {
+            LOGGER.log(Level.INFO, "{0} completed: {1}", new Object[]{toString(), getResult()});
+            if (listener == null) {


what about getListener()?

Similar to the comment on synchronizing getListener() -- give me a bit and I'll either have it changed or better documented in code comments.

jglick

From a shallow inspection looks OK.

jglick · 2018-04-06T17:24:07Z

src/main/java/org/jenkinsci/plugins/workflow/job/WorkflowRun.java

                    FlowExecution fetchedExecution = getExecution();  // Triggers execution.onLoad so we can resume running if not done

                    if (fetchedExecution != null) {
                        fetchedExecution.addListener(new GraphL());
                        executionPromise.set(fetchedExecution);

                        if (completed == null) {
-                            completed = new AtomicBoolean(fetchedExecution.isComplete());
+                            completed = Boolean.valueOf(fetchedExecution.isComplete());


Note: can just drop the valueOf call as this is done by autoboxing anyway.

svanoort · 2018-04-08T13:58:59Z

Merging now that I've done one more round of integration testing locally using the related workflow-cps PR and an updated snapshot.

…ob-plugin#27.

svanoort added 8 commits March 22, 2018 15:18

Make completed persistent and use a guard object for synchronization …

b749460

…instead

Remove redundant save operations from graphListener if durability set…

960c5cc

…ting permits

Set completed sooner when finishing run, limit scope of locking and s…

4107ee4

…ome minor cleanups

Ensure some key execution state transitions happen even if errors occ…

ab4455d

…ur, such as removing a Run from active execution

Safely lazy-load the execution state

472e5f0

More hardening of FlowExecution lazyLoad

841eaea

Simplify conditionals

1d7e873

Test cases for lazy-load FlowExecution

b3d13b4

svanoort added the work-in-progress label Mar 28, 2018

Prevent NPEs around listener use by switching to an intializing getter

2039dc0

svanoort mentioned this pull request Mar 30, 2018

Fix a whole wad of Resume issues and add a bunch of diagnostic logging for FlowHead problems etc jenkinsci/workflow-cps-plugin#216

Merged

Typo

3cc104b

abayer requested review from abayer and jglick March 30, 2018 15:03

abayer reviewed Mar 30, 2018

View reviewed changes

A few review changes

bb334e1

svanoort removed the work-in-progress label Apr 3, 2018

svanoort changed the title ~~[WIP] Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407]~~ [WIP] Fix resume issues [JENKINS-46986] and [JENKINS-50199] and [JENKINS-50407] Apr 3, 2018

svanoort changed the title ~~[WIP] Fix resume issues [JENKINS-46986] and [JENKINS-50199] and [JENKINS-50407]~~ [WIP] Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407] Apr 3, 2018

jglick changed the title ~~[WIP] Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407]~~ Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407] Apr 3, 2018

jglick requested changes Apr 3, 2018

View reviewed changes

svanoort added 3 commits April 3, 2018 19:30

Additional hardening based on workflow-cps FlowDurabilityTest fuzzing

75d05c5

Fix another bug that can cause completed to be unset and not persisted

a535449

Implement the review changes suggested by @jglick on top of the last …

d2e5b3c

…round of robustness fixes

svanoort requested a review from jglick April 6, 2018 13:22

abayer approved these changes Apr 6, 2018

View reviewed changes

rsandell reviewed Apr 6, 2018

View reviewed changes

svanoort added 2 commits April 6, 2018 13:12

Clarify some of listener use and synchronization

eb258f8

More explanatory comment

3e12266

rsandell approved these changes Apr 6, 2018

View reviewed changes

jglick approved these changes Apr 6, 2018

View reviewed changes

Merge branch 'master' into fix-resume-issues

db23202

svanoort merged commit f0c2605 into jenkinsci:master Apr 8, 2018

svanoort deleted the fix-resume-issues branch April 8, 2018 13:59

svanoort mentioned this pull request Apr 19, 2018

Fix [JENKINS-50784] by using a blocking load of execution where needed jenkinsci/workflow-multibranch-plugin#73

Merged

jglick mentioned this pull request May 18, 2018

[JENKINS-51390] Use ProxyException when necessary for program.dat jenkinsci/workflow-support-plugin#61

Merged

jglick mentioned this pull request Sep 28, 2018

[JEP-210] Log handling rewrite #27

Merged

11 tasks

jglick added a commit to jglick/dump-live-pipeline-programs that referenced this pull request Aug 27, 2019

Adapting to jenkinsci/workflow-job-plugin#93 and jenkinsci/workflow-j…

50d0f3b

…ob-plugin#27.

jglick mentioned this pull request Apr 9, 2020

WorkflowRunRestartTest.resumeDisabled should not assume that the execution is not loaded #160

Merged

jglick mentioned this pull request May 23, 2023

Switch WorkflowRunRestartTest to JenkinsSessionRule #356

Merged

jglick mentioned this pull request Nov 7, 2024

FlowExecution.getOwner left null after WorkflowRun.reload #472

Merged

		@@ -171,6 +171,9 @@ public String url() {

		private transient boolean allowKill;

		/** Controls whether or not our execution has been initialized via its {@link FlowExecution#onLoad(FlowExecutionOwner)} method yet if.*/

Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407] #93

Fix resume issues [JENKINS-49686] and [JENKINS-50199] and [JENKINS-50407] #93

Uh oh!

Conversation

svanoort commented Mar 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

abayer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svanoort commented Apr 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jglick left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

svanoort commented Apr 8, 2018

Uh oh!

Uh oh!

svanoort commented Mar 28, 2018 •

edited

Loading