[JENKINS-67164] Call StepExecution.onResume in response to WorkflowRun.onLoad not FlowExecutionList.ItemListenerImpl

[jglick]

JENKINS-67164: Reapplies #178 and its amendment #188, reverting the reversion #198.

Fixes the problem identified in SSHAgentStepWorkflowTest.sshAgentAvailableAfterRestart whereby a sh step began running while an sshagent step was still in the middle of an onResume, by introducing a FlowExecution.afterStepExecutionsResumed hook which CpsFlowExecution will implement to unpause the program near the end of WorkflowRun.onLoad, rather than unpausing immediately upon deserializing program.dat.

Seems to offer a nicer way to implement the ExecutorPickle replacement portion of jenkinsci/workflow-durable-task-step-plugin#180, by letting ExecutorStepExecution.onResume wait for an agent to reconnect before resuming progran execution. Compared to ExecutorPickle the advantage is that if there is in fact a timeout, rather than marking the entire program as a failure to load, the node step can throw a proper Groovy-level exception which can be handled gracefully.

A matching PR to workflow-cps-plugin is needed to avoid the class of regression seen with sshagent. I would like to run some integration tests in advance. jenkinsci/bom#1122 (comment)

[dwnusbaum]

Seems fine to me. For what it's worth, I would have preferred to see the old commits cherry-picked or a revert-the-revert commit with your changes applied as a new commit to make it easier to tell what the new changes are, although I guess the followup commits in #198 would have created a bunch of conflicts.

[jglick]

a revert-the-revert commit with your changes applied as a new commit

Agreed, though it would have been tricky, since I had to play with various uncommitted edits before I found something that worked.

[dwnusbaum]

Is this required for correctness or is it an optimization?

[jglick]

It is an optimization. Previously all steps were resumed serially in a topological order. (Or I hope getCurrentExecutions enforced a topological order; the sorting by CpsThread is a bit opaque to me.) The problem in jenkinsci/workflow-durable-task-step-plugin#180 arises when you have a bunch of parallel branches all running node and all of them are going to fail due to missing agents. Previously, Jenkins would wait 5m for the first agent, report that it was gone, then wait 5m for the next agent, etc. Now all the node blocks are resumed in parallel, followed by the sh steps inside them, etc. Have a test demonstrating this in workflow-durable-task-step but only now got an incremental build from jenkinsci/workflow-cps-plugin#534 that I needed.

[dwnusbaum]

Ok, I guess it's kind of unusual for ExecutorStepExecution.onResume to block if no other steps do so, but looking through jenkinsci/workflow-durable-task-step-plugin#180 it seems like you already tried various alternatives, so this seems fine.

[jglick]

Basically this is the replacement for the TryRepeatedly pickle. I played with a lot of alternatives indeed. I am not claiming this approach is ideal but it seems to be straightforward enough and do the job.

Earlier, before hitting on the idea of having onResume block, I had hoped to arrange it so that the program would resume right away, and then a sh step running on the dead agent would eventually figure out there was no hope and abort. I ran into problems in functional tests, though, things like

node('agent') {
  input 'Proceed?' // restart here
  sh 'true'
}

The program would run past input to the sh step, which would then either fail with a MissingContextVariableException (confusing) or block the CPS VM thread inside DSL (which will throw an error if it blocks >5s). Admittedly this is an artificial case (you should not hold an executor like that) but a lot of tests were failing that I had to work around in artificial ways and it seemed uncomfortable. It would have been possible to fix all these cases but only in pretty intrusive ways: by defining a new StepContext object in lieu of FilePath (perhaps DynamicFilePathContext.Representation) which could be deserialized without pickles, but every step currently expecting FilePath would need to be adjusted to expect this instead, and then the step would need to have complicated logic to wait for the new contextual object to be ready (translatable to a live FilePath) before doing anything useful.

The compromise I settled on is closer to the original pickle behavior in that CPS code and steps do not run until an attempt is made to restore the original context: until all open node blocks have gotten their agent to reconnect, or timed out trying. The difference is only that in case this fails (with a timeout), the error is thrown out of StepExecution.onResume and thus StepContext.onFailure so it becomes a properly modeled Throwable with a CPS VM stack trace that we can catch and handle—unlike the original situation where the entire build would be effectively hard-killed with no possible cleanup.

[jglick]

I believe this is ready to release if jenkinsci/workflow-cps-plugin#534 can be released at the same time (to avoid regressions in some corner cases). jenkinsci/workflow-durable-task-step-plugin#226 can then also be released (but there is no rush in doing so).

[dwnusbaum]

Ok, I guess it's kind of unusual for ExecutorStepExecution.onResume to block if no other steps do so, but looking through jenkinsci/workflow-durable-task-step-plugin#180 it seems like you already tried various alternatives, so this seems fine.