-
Notifications
You must be signed in to change notification settings - Fork 96
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
Make ExecutorPickle wait on EphemeralNodes when trying to resume, restoring original behavior of fix to JENKINS-36013 #48
Conversation
…toring original behavior of fix to JENKINS-36013
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does not make sense to me. Why not just fix the Swarm plugin?
Queue.getInstance().cancel(item); | ||
if (isEphemeral()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then you might as well delete the ephemeral
flag.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to keep that info handy for future use, plus it avoids grumping from XStream because the field no longer exists.
@jglick The exact required behavior of EphemeralNodes is pretty poorly defined - the node itself may not be persistent with regard to the master but there's nothing to prevent it from being recreated once removed (as Swarm plugin does when nodes reconnect). The safest thing to do is to ensure we're not prematurely terminating builds and just use the original grace period -- there's no guarantee there are no other oddball implementations. |
Suggest developing automated tests in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🐝
Amends #47 FTR. |
Per comment here, immediately failing builds with an ExecutorPickle that depends on an EphemeralNode is not safe to do, due to the Swarm Plugin. Documented in the JIRA comments.
So, I am restoring my original behavior (apply a timeout in all cases). Can't remove the new field, though.
@reviewbybees CC @jglick