Allow defining node properties on template #382

mildsunrise · 2019-07-25T12:14:36Z

Proposal to allow defining node properties (environment variables, tool locations, etc.) that will be applied to created slaves.

EC2AbstractSlave and children already take a nodeProperties parameter, so we just need to add the field to SlaveTemplate.

jvz

Seems like a reasonable feature. Some small code change suggestions. I don't think it's a great idea to make any public fields that aren't trivial properties, and since you've made a lazily initialized field, this should be private for sure.

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

mildsunrise · 2019-07-25T20:09:40Z

Thanks for the quick review! I swear I had made that field private... Both changes committed and tested.

jvz

Changes look good, though I haven't tested this manually.

res0nance · 2019-09-10T08:57:37Z

Retrigger-CI

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

mildsunrise · 2019-10-30T14:43:35Z

Code re-rebased; conflicts solved again; tested.

src/main/java/hudson/plugins/ec2/SlaveTemplate.java

jhansche · 2020-02-10T19:38:44Z

@mildsunrise @res0nance Does this resolve https://issues.jenkins-ci.org/browse/JENKINS-36544?

The merge commit shows it is included in tags: ec2-1.49.1 ec2-1.49 ec2-1.48 ec2-1.47. And according to the config.jelly change, this should be present between the "Maximum Total Uses" field, and the repeatable-delete button.

I'm running v1.49.1, but I do not see the Node Properties option where it looks like it should be. Am I missing something?

jhansche · 2020-02-10T20:08:52Z

I see the Node Properties listed if I click Configure on a slave instance that was already launched. But I assumed this should appear in the "Configure Clouds" template -- and I don't see it in that template.

mildsunrise · 2020-02-11T03:39:13Z

That's strange, I'm running v1.49.1 and I do see the node properties section at the bottom of the AMI definition in the advanced properties 🤔

jhansche · 2020-02-11T06:31:59Z

OK, that worries me then 😶
I was poking around with the Script Console, and it looks to me like everything should be right. But Jenkins.instance.getCloud("ec2-$name").templates.each { it.nodeProperties } is just an empty list. nodePropertyDescriptors contains the 4 NodeProperty describables that are available via core/plugins (including Env vars, which is what I'm trying to get working).

I also tried editing config.xml to add an env var to the template. When I then reload from disk, the same groovy script above did show the Env vars NodeProperty on the template's nodeProperties list. But then when I go back to Configure Clouds, there's still nothing where the <descriptorList> is supposed to be. If I click save at that point, it simply strips the nodeProperties from the template.

I don't think it should matter, but I am using a spot instance template.

I'm at the extent of my knowledge here, but it seems to be something to do with how stapler is generating the form, but I don't know why or how to figure out why.

Jenkins v2.219
Plugin v1.49.1
Amazon EC2 cloud
Spot instance slave template
Nothing that seemed out of the ordinary in jenkins.log
Anything else I can look for? 🤷‍♂️

jhansche · 2020-03-09T15:32:28Z

@mildsunrise Any other suggestions that we can look for, to figure out why the Node Properties isn't appearing on my cloud configuration? I added a screenshot to the JENKINS-36544 ticket to show what I see:

mildsunrise · 2020-03-09T15:56:45Z

@jhansche Nope, sorry :( I don't know how to debug those problems... the only thing I could suggest is to build this plugin from source and install it. You could make changes to the template and verify that they appear in the UI.

jhansche · 2020-03-09T18:46:23Z

Thanks for the suggestion.

I ran the plugin locally, with the existing POM configuration, which uses jenkins version 2.150.1 by default. On that version of Jenkins, the "Configure Clouds" is still in the System Configuration ($JENKINS/configure) page, instead of the newer separate page. On that old page, I do see the Node Properties descriptor list.

Updating the jenkins version to 2.223 (and fixing a dependency version error with slf4jVersion), it is now moved into the newer $JENKINS/configureClouds URL, and on this screen, I no longer see the Node Properties descriptor. It looks like maybe there was a change in Jenkins that caused this to stop working?

@mildsunrise What version of Jenkins are you running, such that you see it working properly?

jhansche · 2020-03-09T18:57:50Z

The Configure Clouds section got moved to its own page as of jenkinsci/jenkins#4339 with commit 77f1242 on Nov 16.

I confirmed that if I run the plugin against version 2.204, it works correctly and I see the Node Properties list. Running against version 2.205 breaks it, and the Node Properties do not appear in the separate Configure Clouds screen.

Looks like this PR got approved on Nov 2, but didn't merge until a month later on Dec 1, after the Configure Clouds change happened. Is it possible that this change assumed it was going to be in the /configure Configure System screen, and after that moves to its own screen, it no longer has access to those Node Properties for some reason?

jhansche · 2020-03-09T20:01:01Z

Tracked this down further by debugging the config.jelly file:

<f:descriptorList title="${%Node Properties}" field="nodeProperties" descriptors="${it.getNodePropertyDescriptors()}" />

In version 2.204 and older, $it is an instance of hudson.model.Hudson. On 2.205 and later, $it refers to jenkins.model.GlobalCloudConfiguration.

Looking into https://wiki.jenkins.io/display/JENKINS/Basic+guide+to+Jelly+usage+in+Jenkins#BasicguidetoJellyusageinJenkins-Otherpredefinedobjects I see that ${app} is another way to access the Hudson instance, so changing that to:

<f:descriptorList title="${%Node Properties}" field="nodeProperties" descriptors="${app.getNodePropertyDescriptors()}" />

causes it to work properly on both older and newer Jenkins versions.

I will open a new PR with this fix.

mildsunrise · 2020-03-09T20:05:43Z

Wow, great work :o Yeah, that explains it... Thanks a lot!

This is a follow-up fix for PR jenkinsci#382. In 2.204, the "Configure Clouds" screen was on the main /configure system configuration page. As a result, `${it}` referred to the global system object, aka the Hudson instance. In 2.205, PR jenkinsci/jenkins#4339 merged, which moves the Configure Clouds section to a new page: /configureClouds. As a result of that change, `${it}` no longer refers to the Hudson instance, but to a new GlobalCloudConfiguration object. That object does not have the `getNodePropertyDescriptors()` method, so it is not able to populate the "Node Properties" block. Switching that to `${app}` gives it a correct reference to the Hudson object, and that allows it to show the Node Properties block on all Jenkins versions, both before and after v2.205.

jhansche · 2020-03-09T20:11:08Z

New PR opened: #440

* [JENKINS-36544] Fix Node Properties on Jenkins 2.205+ This is a follow-up fix for PR #382. In 2.204, the "Configure Clouds" screen was on the main /configure system configuration page. As a result, `${it}` referred to the global system object, aka the Hudson instance. In 2.205, PR jenkinsci/jenkins#4339 merged, which moves the Configure Clouds section to a new page: /configureClouds. As a result of that change, `${it}` no longer refers to the Hudson instance, but to a new GlobalCloudConfiguration object. That object does not have the `getNodePropertyDescriptors()` method, so it is not able to populate the "Node Properties" block. Switching that to `${app}` gives it a correct reference to the Hudson object, and that allows it to show the Node Properties block on all Jenkins versions, both before and after v2.205. * Use the correct form of getNodePropertyDescriptors() Current master is calling `${it.getNodePropertyDescriptors()}`, but on Jenkins versions <= 2.204, that is calling the method on the global Jenkins node (via Node class). It should instead be using the version of the method that was added in SlaveTemplate, and to do that from a Jelly script, it needs to be moved into the SlaveTemplate.DescriptorImpl class.

jvz requested changes Jul 25, 2019

View reviewed changes

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Outdated Show resolved Hide resolved

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Outdated Show resolved Hide resolved

jvz approved these changes Jul 25, 2019

View reviewed changes

res0nance added the enhancement Feature additions or enhancements label Sep 4, 2019

res0nance approved these changes Sep 10, 2019

View reviewed changes

res0nance closed this Sep 10, 2019

res0nance reopened this Sep 10, 2019

res0nance reviewed Sep 10, 2019

View reviewed changes

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Show resolved Hide resolved

ro-kr reviewed Oct 30, 2019

View reviewed changes

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Outdated Show resolved Hide resolved

mildsunrise added 4 commits October 30, 2019 15:30

Allow defining node properties on template

471467f

Fixes

ccaa4c4

Rebase and fix

d842e67

Apply suggestions

e5a59e6

ro-kr reviewed Oct 30, 2019

View reviewed changes

src/main/java/hudson/plugins/ec2/SlaveTemplate.java Outdated Show resolved Hide resolved

Apply suggestions

da04051

ro-kr approved these changes Nov 2, 2019

View reviewed changes

res0nance merged commit 6bf5a84 into jenkinsci:master Dec 2, 2019

mildsunrise deleted the feature-node-props branch December 2, 2019 11:20

jhansche mentioned this pull request Mar 9, 2020

[JENKINS-36544] Fix Node Properties on Jenkins 2.205+ #440

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow defining node properties on template #382

Allow defining node properties on template #382

mildsunrise commented Jul 25, 2019

jvz left a comment

mildsunrise commented Jul 25, 2019

jvz left a comment

res0nance commented Sep 10, 2019

mildsunrise commented Oct 30, 2019

jhansche commented Feb 10, 2020

jhansche commented Feb 10, 2020

mildsunrise commented Feb 11, 2020

jhansche commented Feb 11, 2020 •

edited

Loading

jhansche commented Mar 9, 2020

mildsunrise commented Mar 9, 2020

jhansche commented Mar 9, 2020

jhansche commented Mar 9, 2020

jhansche commented Mar 9, 2020 •

edited

Loading

mildsunrise commented Mar 9, 2020

jhansche commented Mar 9, 2020

Allow defining node properties on template #382

Allow defining node properties on template #382

Conversation

mildsunrise commented Jul 25, 2019

jvz left a comment

Choose a reason for hiding this comment

mildsunrise commented Jul 25, 2019

jvz left a comment

Choose a reason for hiding this comment

res0nance commented Sep 10, 2019

mildsunrise commented Oct 30, 2019

jhansche commented Feb 10, 2020

jhansche commented Feb 10, 2020

mildsunrise commented Feb 11, 2020

jhansche commented Feb 11, 2020 • edited Loading

jhansche commented Mar 9, 2020

mildsunrise commented Mar 9, 2020

jhansche commented Mar 9, 2020

jhansche commented Mar 9, 2020

jhansche commented Mar 9, 2020 • edited Loading

mildsunrise commented Mar 9, 2020

jhansche commented Mar 9, 2020

jhansche commented Feb 11, 2020 •

edited

Loading

jhansche commented Mar 9, 2020 •

edited

Loading