Skip to content

Change NODE_NAME for the built-in node based on its self label #168

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

Merged
merged 4 commits into from
Sep 21, 2021

Conversation

daniel-beck
Copy link
Member

@daniel-beck daniel-beck commented Sep 7, 2021

This implements the same terminology cleanup as jenkinsci/jenkins#5425 did in core for for NODE_NAME variable which is set here independently of core.

This is independent of #167 but without that PR, this one will timeout on 2.307+ (unchanged from before).

This is not expected to result in a behavior difference on Jenkins 2.306 and earlier.

On Jenkins 2.307 and later, this PR will change the behavior and make it consistent with core's (freestyle jobs etc.). This can easily be tested by using the script console to provide a custom name and label for the built-in node:

Jenkins.nodeNameAndSelfLabelOverride = 'mast-in'

Changelog suggestion:

  • The NODE_NAME environment variable value set for the built-in node inside node blocks now matches the behavior of other job types: On Jenkins 2.306 and earlier, the behavior is unchanged (master). On Jenkins 2.307 and later, the variable will be built-in or master, depending on whether the instance was upgraded from a previous release or newly installed, and whether the migration was applied. See https://www.jenkins.io/redirect/built-in-node-migration/

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.

Does

// Switches the label to a self-label, so if the executable is killed and restarted via ExecutorPickle, it will run on the same node:
label = computer.getName();
still work?

@daniel-beck
Copy link
Member Author

Does

// Switches the label to a self-label, so if the executable is killed and restarted via ExecutorPickle, it will run on the same node:
label = computer.getName();

still work?

That's the "agent" branch, computer.getName() is weird for the built-in executor, always has been: https://github.com/jenkinsci/jenkins/blob/89d334145d2755f74f82aad07b5df4119d7fa6ce/core/src/main/java/jenkins/model/Jenkins.java#L5092-L5095

@@ -71,6 +71,7 @@
<java.level>8</java.level>
<useBeta>true</useBeta>
<gitHubRepo>jenkinsci/${project.artifactId}-plugin</gitHubRepo>
<hpi.compatibleSinceVersion>2.40</hpi.compatibleSinceVersion>
Copy link
Member Author

@daniel-beck daniel-beck Sep 7, 2021

Choose a reason for hiding this comment

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

Not strictly correct but the best we can do to encourage admins to read the changelog and understand the impact of upgrading.

While there's no dedicated system property for this plugin's behavior, the same jenkins.model.Jenkins.nodeNameAndSelfLabelOverride would change the behavior here.

What's not possible: To get the new behavior as it is in 2.307+ while retaining the existing behavior in this plugin only.

@jglick

This comment has been minimized.

@daniel-beck

This comment has been minimized.

@jglick

This comment has been minimized.

@daniel-beck

This comment has been minimized.

@car-roll
Copy link
Collaborator

@jglick do you think your comments fully addressed?

@jglick
Copy link
Member

jglick commented Sep 20, 2021

do you think your comments fully addressed?

Other than #168 (comment) yes.

Copy link
Member

@timja timja left a comment

Choose a reason for hiding this comment

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

@daniel-beck
Copy link
Member Author

@car-roll FYI I updated https://github.com/jenkinsci/workflow-durable-task-step-plugin/releases/tag/workflow-durable-task-step-2.40 in response to https://groups.google.com/g/jenkinsci-users/c/R7AUrb6nq5k/m/j_Omo1f3BQAJ; feel free to update/revert.

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.

4 participants