-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Set nodeUsageMode to EXCLUSIVE as default #386
Set nodeUsageMode to EXCLUSIVE as default #386
Conversation
can you split the PR for nodeUsageMode and cleanup, will make it easier to review and merge |
@carlossg I'll try. The changes are not quite independent, since the changed lines in each commit are right next to each other... (OTOH, reviewing each commit on its own should be possible) |
Rebased after clean-up stuff moved into its own pull request. |
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 don't know if this makes sense, if EXCLUSIVE
solves anything then it should be the default, but I don't think it really changes the execution for kubernetes agents. Can you elaborate?
} | ||
|
||
@DataBoundSetter | ||
public void setNodeUsageMode(String nodeUsageMode) { | ||
this.nodeUsageMode = Node.Mode.valueOf(nodeUsageMode); | ||
public void setNodeUsageMode(Node.Mode nodeUsageMode) { |
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 think if @DataBoundSetter
is removed you wouldn't have the ordering issue
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 changed it and it seems you are right.
Sure: FreeStyle jobs without node specification (can run an "any" node) are scheduled on Kubernetes nodes with |
I agree with @carlossg, it should be the default as long as there is a label defined for the pod template |
yes, the defaulting to exclusive should be in this PR |
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.
EXCLUSIVE
should be the default
This changes the behavior for existing podTemplate() pipeline steps (for the better IMHO). This will (obviously) not update existing persisted pod templates, users will need to fix them manually.
I'm not really happy with specifying the default both in the code and in the Jelly template, but it seems the Jelly template is rendered without any object (not even an empty object) first and only when submitting the form an object is created... The behaviour is now like this: Both Jelly forms (the snippet generator for pipelines and the pod template form in Jenkins' system settings) now preselect "EXCLUSIVE", to give the unsuspecting user the strong hint that this is the correct setting. The default in |
This is an attempt to give "nodeUsageMode" more visibility, due to a "race-condition" discovered while this was set to its default value "NORMAL" (other jobs "stealing" the pod from a job).
PS: Having
@DataBoundSetter
on two overloads of the same method seems to confuse the snippet generator... Switching the order of those cases fixed it for me... (Maybe removing the non-String version is the "correct" fix, but might break backwards compatibility)