Skip to content

Conversation

@Vlatombe
Copy link
Member

@Vlatombe Vlatombe commented Aug 25, 2023

cf. jenkinsci/kubernetes-plugin#1342 (comment)

Testing done

AFAIU to reproduce the issue would require to set up a cloud agent. On its termination, it calls indirectly computeNext() which unregisters the FlowExecutionOwner if it is the last node in the pipeline. Then the regular unregister call when the WorkflowRun completes now issues a warning because it has already been unregistered.

### Submitter checklist
- [X] Make sure you are opening from a **topic/feature/bugfix branch** (right side) and not your main branch!
- [X] Ensure that the pull request title represents the desired changelog entry
- [X] Please describe what you did
- [ ] Link to relevant issues in GitHub or Jira
- [x] Link to relevant pull requests, esp. upstream and downstream changes
- [ ] Ensure you have provided tests - that demonstrates feature works or fixes the issue

Copy link
Member

@jglick jglick left a comment

Choose a reason for hiding this comment

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

Idea seems right, but we want to keep the unregister call from

/**
* When Jenkins starts up and everything is loaded, be sure to proactively resurrect
* all the ongoing {@link FlowExecution}s so that they start running again.
*/
@Extension
public static class ItemListenerImpl extends ItemListener {
@Override
public void onLoaded() {
FlowExecutionList list = FlowExecutionList.get();
for (final FlowExecution e : list) {
// The call to FlowExecutionOwner.get in the implementation of iterator() is sufficent to load the Pipeline.
LOGGER.log(Level.FINE, "Eagerly loaded {0}", e);
}
list.resumptionComplete = true;
}
}
(in some form) as this cleans up stale entries.

@jglick jglick added the bug label Aug 25, 2023
@jglick jglick requested a review from dwnusbaum August 25, 2023 13:21
@Vlatombe Vlatombe requested a review from jglick August 25, 2023 14:12
jglick
jglick previously requested changes Aug 25, 2023
@Vlatombe Vlatombe requested a review from jglick August 25, 2023 14:59
@Vlatombe
Copy link
Member Author

@dwnusbaum can you help with review/merge of this fix? Currently holds up another series of PRs (e.g. jenkinsci/kubernetes-plugin#1342)

@dwnusbaum
Copy link
Member

@Vlatombe Sure I will look at this today

@dwnusbaum
Copy link
Member

@Vlatombe Did you check this against tests in workflow-cps and workflow-job since the new code seems to be not well covered by existing tests in this plugin? I can check it in a minute if you haven't already.

@Vlatombe
Copy link
Member Author

Vlatombe commented Aug 28, 2023

@dwnusbaum no.

To cover the new lines would require a test pipeline involving a cloud agent (so that the agent gets removed at the end of the node block).

@dwnusbaum
Copy link
Member

To cover the new lines would require a test pipeline involving cloud agents

It should be possible to set up a relevant test with @LocalData and/or a custom Pipeline step, but either way I mainly just wanted to make sure it didn't affect existing tests in those plugins, and from a quick local check things seem ok.

@dwnusbaum dwnusbaum changed the title computeNext() should not cause a side-effect in the normal case Do not unregister completed executions when iterating over FlowExecutionList to avoid unnecessary log warnings Aug 28, 2023
@dwnusbaum dwnusbaum merged commit d9baddd into jenkinsci:master Aug 28, 2023
@Vlatombe Vlatombe deleted the computeNext-remove-side-effect branch August 28, 2023 15:10
Vlatombe added a commit to Vlatombe/kubernetes-plugin that referenced this pull request Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants