[JENKINS-49707] Agent missing after controller restart to fail resumption of node step, not kill whole build

[jglick]

JENKINS-49707

Upstream of jenkinsci/workflow-basic-steps-plugin#195 and jenkinsci/kubernetes-plugin#1211. Subsumes #231. Subsumes #249.

Relevant prior work: #104, jenkinsci/kubernetes-plugin#461, #47, #48, #115, #101, jenkinsci/workflow-basic-steps-plugin#86, #175, #176, #179.

Plan of record:

• Introduce ExecutorStepDynamicContext and associated machinery as a replacement for ExecutorPickle and other pickles (see below).
• Check PCT for any downstream regressions associated with this switch.
• Introduce an extension point allowing the retry step to be given configurable behavior.
• Create an implementation which matches generic “agent failure” conditions.
• Offer a refined version in kubernetes which takes into account pod status.
• Offer a retry option for Declarative agent types any and label (i.e., generic) and kubernetes
• Decide what to do about resuming inside node but outside sh.
• Revalidate behavior in a K8s disaster recovery scenario.
• Port ExecutorPickle should not reschedule anything until after Queue.load #184 to the new code, as Warn in Queue.init if items scheduled during startup will be clobbered jenkins#5934 can be reproduced again.
• Some error saving queue.xml: [JENKINS-49707] Agent missing after controller restart to fail resumption of node step, not kill whole build #180 (comment)
• 5m timeout restoring node should count from when build resumed; otherwise it takes 10m to resume a build with two parallel nodes (both missing) and retry: jenkinsci/workflow-api-plugin@b1778a9
• Extend [JENKINS-49707] AgentErrorCondition improvements #249 to ExecutorStepDynamicContext

The idea is that users updating plugins would see a somewhat altered behavior for resumption inside node blocks by default, which I hope would go mostly unnoticed; and could adjust pipelines to look something like

podTemplate(…) {
  retry(count: 3, conditions: [kubernetesAgent(), nonresumable()]) {
    node(POD_LABEL) {
      checkout scm
      sh 'make world'
    }
  }
}

or

pipeline {
  agent {
    kubernetes {
      yaml: '…'
      retries: 3
    }
  }
  stages {
    stage('main') {
      steps {
        sh 'make world'
      }
    }
  }
}

---

The key is to avoid relying on Pickles for ExecutorStepExecution to resume properly. (More precisely, pickles using TryRepeatedly; SecretPickle and the like are harmless in this context.) Instead we take explicit control over how the step is resumed and what happens when. The problem with Pickle here is that it is too magical; we would like to be able to detect after resumption that an agent is gone and then throw an exception at the Groovy level, which is not possible if the build is aborted at the CPS VM level due to an inability to rehydrate.

---

I had planned to offer a system property to revert to ExecutorPickle in case of catastrophic problems (along with a couple sanity tests of that mode) but after reviewing the changes to ExecutorStepExecution this does not seem straightforward: the major simplifications and fixes to it rely on moving away from ExecutorPickle.

[dwnusbaum]

Can the factories for these pickles be deleted or replaced with an implementation that just throws an exception if it encounters an object of the specified type? Or do you want to keep them around in case some other step or wild Groovy code is relying on them?

[jglick]

I plan to retain them for compatibility. Weird Pipeline script is one possibility. They could also be used in other Pipeline steps; I know PushdStep binds FilePathDynamicContext, so that is not affected, but there may be others. Seems harmless enough to leave them here.

[jglick]

Note that

workflow-durable-task-step-plugin/src/main/java/org/jenkinsci/plugins/workflow/support/steps/AgentErrorCondition.java

Lines 60 to 62 in 97365d4

	if (t instanceof FlowInterruptedException && ((FlowInterruptedException) t).getCauses().stream().anyMatch(
	c -> c instanceof ExecutorStepExecution.RemovedNodeCause || c instanceof ExecutorStepExecution.QueueTaskCancelled)) {
	return true;

bypasses the actualInterruption check in https://github.com/jenkinsci/workflow-basic-steps-plugin/blob/d57e3ca46d24bb91b8410467284176b0f319c64e/src/main/java/org/jenkinsci/plugins/workflow/steps/RetryStepExecution.java#L93 so the distinction would only matter for other catching steps like https://github.com/jenkinsci/workflow-basic-steps-plugin/blob/d57e3ca46d24bb91b8410467284176b0f319c64e/src/main/java/org/jenkinsci/plugins/workflow/steps/CatchErrorStep.java#L229

[dwnusbaum]

I took a brief look at the PR and added some questions, but in general it looks fine. I am planning on reviewing it more thoroughly tomorrow.

[dwnusbaum]

Is ordinal = 100 needed for PushdStep to keep working, or something else?

[jglick]

Yes exactly that.

[jglick]

Does not suffice: https://issues.jenkins.io/browse/JENKINS-69936

[basil]

Breaks io.jenkins.docker.pipeline.DockerNodeStepTest#withinNode and io.jenkins.docker.pipeline.DockerNodeStepTest#nodeWithinDockerNodeWithinNode as discovered in jenkinsci/docker-plugin#943.