Skip to content

Conversation

@jglick
Copy link
Member

@jglick jglick commented Jan 3, 2025

Allows WorkflowRuns to load in a background thread without blocking Jenkins startup. (StepExecution.onResume was already called in the background.) This could be beneficial when restarting a controller running a large number of builds; for example, in Kubernetes a startup probe might otherwise time out if the server does not indicate it is up and running earlier.

Note that this makes the clause in docs for isResumptionComplete

* <p>This takes place slightly after {@link InitMilestone#COMPLETED} is reached during Jenkins startup.
actually be true, which apparently it was not when introduced in #221. I looked around for callers that actually relied on it being false (i.e., that assumed that all builds are resumed prior to Jenkins initialization being complete), but did not find anyway. Generally, it is StepExecution.onResumed being called that really matters. The subtlest case was added in jenkinsci/workflow-durable-task-step-plugin#323 but there one of three things can happen: all builds resume, then the agent comes online; the agent comes online, then the build using it resumes; or, at worst, the build using an agent resumes, then the agent comes online (so the listener does nothing), then some unrelated builds resume, in which case we may need to wait up to 5m for check to be called. Even that could I think happen without the change in this PR, in case an agent comes online during Jenkins startup (likely possible for outbound agents, or inbound TCP agents in direct mode); or even if the agent comes online after startup, since isResumptionComplete just checks that builds have been loaded, not that an individual step’s onResume has been called, which will happen later.

@jglick jglick mentioned this pull request Jan 7, 2025
@jglick jglick marked this pull request as ready for review January 14, 2025 19:37
@jglick jglick requested a review from a team as a code owner January 14, 2025 19:37
@jglick jglick requested review from Vlatombe and dwnusbaum January 14, 2025 19:37
Copy link
Member

@dwnusbaum dwnusbaum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems fine. I tried looking through the history of #178 and #188 which got reverted due to problems before being modified and then reintroduced in #221 because I thought we might have already considered this, but I did not find anything directly relevant.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants