Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[JENKINS-27039] Option for stage step to cancel older executions #177

Closed
wants to merge 4 commits into from

Conversation

@kreinloo
Copy link

commented Aug 12, 2015

JENKINS-27039

Added eager flag to stage step. When a new build enters this stage, it will cancel any running build so that it could proceed on its own. Inspired by @jglick's comment.

Example:

stage name: 'build', eager: true
input message: 'Proceed?'
Add 'eager' flag to stage step. Interrupts any build which is current…
…ly in this stage and takes its place. Inspired by JENKINS-27039.
@jglick

This comment has been minimized.

Copy link
Member

commented Aug 12, 2015

This definitely needs coverage in StageStepTest.

@@ -41,6 +41,7 @@

public final String name;
@DataBoundSetter public @CheckForNull Integer concurrency;
@DataBoundSetter public @CheckForNull Boolean eager;

This comment has been minimized.

Copy link
@jglick

jglick Aug 12, 2015

Member

boolean suffices (especially as the default remains false).

if (ex != null) {
ex.interrupt(Result.ABORTED);
}
stage.holding.remove(i);

This comment has been minimized.

Copy link
@jglick

jglick Aug 12, 2015

Member

Need to use Iterator.remove to avoid ConcurrentModificationException I think.

try {
Executor ex = job.getBuildByNumber(i).getExecutor();
if (ex != null) {
ex.interrupt(Result.ABORTED);

This comment has been minimized.

Copy link
@jglick

jglick Aug 12, 2015

Member

Better to use NOT_BUILT with a CanceledCause, as in the cancel method.

if (eager != null && eager) {
for (Integer i : stage.holding) {
try {
Executor ex = job.getBuildByNumber(i).getExecutor();

This comment has been minimized.

Copy link
@jglick

jglick Aug 12, 2015

Member

This probably works well enough, though it is rather indirect; nicer to use FlowExecution.interrupt if you can get access to the FlowExecution, perhaps by keeping a FlowExectionOwner in Stage (taking care to deal gracefully with null fields from serialized instances from older plugin versions). I am not sure offhand if there is a concrete downside to going through Executor like this; the test would need to verify that the interrupted build’s result and CauseOfInterruption are being set properly.

@kreinloo kreinloo force-pushed the kreinloo:master branch from b439916 to 6296765 Aug 13, 2015

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 18, 2015

Looks good so far. Marking WiP pending test coverage (whether written by you or me or someone else).

@kreinloo

This comment has been minimized.

Copy link
Author

commented Aug 21, 2015

As of now, it still cancels jobs with the ABORTED message.

Running: End of Workflow
Rejected by SYSTEM
Finished: ABORTED

I tried using the FlowExecution.interrupt as well, but the results were the same. Probably did something wrong. I'll try to improve it and write tests as well.

@jglick

This comment has been minimized.

Copy link
Member

commented Aug 21, 2015

StageTest IIRC.

Logic for handling aborts is a little subtle. Might need to check with a debugger why it is getting ABORTED. I suspect stop is called on the Execution and this overrides the cancellation cause you tried to set.

@witokondoria

This comment has been minimized.

Copy link
Member

commented Oct 22, 2015

Wouldnt it be nice to merge this feature and handle the exit status on a different issue (IMHO ABORTED fits well)

@jglick

This comment has been minimized.

Copy link
Member

commented Oct 23, 2015

Exit status is a minor issue (mainly consistency with the current behavior), but tests are vital.

@jglick

This comment has been minimized.

Copy link
Member

commented Jan 20, 2016

support/src/test/java/org/jenkinsci/plugins/workflow/support/steps/StageStepTest.java only tests configuration. This tests runtime behavior, the interesting stuff.

@witokondoria

This comment has been minimized.

Copy link
Member

commented Feb 4, 2016

What about another parameter to flag a stage as uncancellable? It could serve to define complex-one level semaphores

@amuniz

This comment has been minimized.

Copy link
Member

commented Feb 25, 2016

Tested locally, it works as advertised.
@kreinloo I'm going to add some tests and will file a new PR based on this one (keeping your commits).

@amuniz

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

Changed my mind. I'm working on a new step (milestone) that supersedes this PR. See JENKINS-27039 comments for more context.

@amuniz

This comment has been minimized.

Copy link
Member

commented Mar 4, 2016

See #355

@jglick jglick referenced this pull request Mar 7, 2016
3 of 3 tasks complete
@jenkinsadmin

This comment has been minimized.

Copy link
Member

commented Apr 8, 2016

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@jglick

This comment has been minimized.

Copy link
Member

commented May 3, 2016

I think this has become obsolete; please try out jenkinsci/pipeline-milestone-step-plugin#1 and let us know if it works for you.

@jglick jglick closed this May 3, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.