Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-29547

"master" value is silently ignored and empty value is set for "Restrict where..." in Tied Label Slicer section

    Details

    • Similar Issues:

      Description

      Configuration Slicer page -> "Tied Label Slicer".

      I move some of my jenkins jobs to a new row and set "master" value on the left.
      I expect these jobs to get this "restricted to run on master" setting.

      actual:
      when I open individual configuration screen for those jobs, they still have "restrict where it can be run" checkbox selected,
      but the label value is empty. there is NO "master" text in it.
      if I go back to Configuration Slicer page -> Tied Label Slicer, I can see those jobs in a separate row with "master" value on the left, which is good.
      but it is simply not shown on individual Job Config pages (when you click "Configure" for a particular job in Jenkins UI).

      note: setting labels other than "master" seems to work fine in my environment.

      also, I have 0 executors available on the "master", which may or may not be relevant to this. I think it's not, but decided to share just in case.

      maybe "master" is considered to be a "default" label and not always shown in some UI pages... but that's weird because I can open "Configure Job" page and type "master" value as the node restriction and it stays there...

        Attachments

          Issue Links

            Activity

            Hide
            mdonohue mdonohue added a comment - - edited

            I've been looking at the code, comparing the configuration of a freestyle project manually, and via the label slicer. I think this is a bug in Jenkins, since using the configuration screen sets the 'assignedNode' private field directly based on what was typed in the HTML form. In this case the 'master' string goes directly into the field.

            But any other plugin that wants to modify the 'assignedNode' field needs to call 'setAssignedNode', and that function does extra checking. If you set the assigned node to "master" it switches that to 'null'. There are other comments indicating that 'null' is the preferred representation for the master node, but the AbstractProject configuration code doesn't maintain that invariant.

            Show
            mdonohue mdonohue added a comment - - edited I've been looking at the code, comparing the configuration of a freestyle project manually, and via the label slicer. I think this is a bug in Jenkins, since using the configuration screen sets the 'assignedNode' private field directly based on what was typed in the HTML form. In this case the 'master' string goes directly into the field. But any other plugin that wants to modify the 'assignedNode' field needs to call 'setAssignedNode', and that function does extra checking. If you set the assigned node to "master" it switches that to 'null'. There are other comments indicating that 'null' is the preferred representation for the master node, but the AbstractProject configuration code doesn't maintain that invariant.
            Hide
            danielbeck Daniel Beck added a comment -

            mdonohue getAssignedLabel() returns the master node's self-label ("master") when canRoam is false but the label is null, so that seems consistent with the setter.

            Show
            danielbeck Daniel Beck added a comment - mdonohue getAssignedLabel() returns the master node's self-label ("master") when canRoam is false but the label is null , so that seems consistent with the setter.
            Hide
            mdonohue mdonohue added a comment -

            Yes, getAssignedNode and setAssignedNode are consistent about converting 'master' to/from a null value in the field. The problem is that the configuration section for AbstractProject does not follow that convention.

            see getAssignedLabelString and AbstractProject.submit. Here are the relevant lines of code, where you can see no attempt is made to convert 'master' into a null value for assignedNode. You can also verify this by looking at config.xml for a project after setting the label to 'master', where you will see <assignedNode>master</assignedNode> in the file.

                    if(req.getParameter("hasSlaveAffinity")!=null) {
                        assignedNode = Util.fixEmptyAndTrim(req.getParameter("_.assignedLabelString"));
                    } else {
                        assignedNode = null;
                    }
                    canRoam = assignedNode==null;
            
            
            Show
            mdonohue mdonohue added a comment - Yes, getAssignedNode and setAssignedNode are consistent about converting 'master' to/from a null value in the field. The problem is that the configuration section for AbstractProject does not follow that convention. see getAssignedLabelString and AbstractProject.submit. Here are the relevant lines of code, where you can see no attempt is made to convert 'master' into a null value for assignedNode. You can also verify this by looking at config.xml for a project after setting the label to 'master', where you will see <assignedNode>master</assignedNode> in the file. if (req.getParameter( "hasSlaveAffinity" )!= null ) { assignedNode = Util.fixEmptyAndTrim(req.getParameter( "_.assignedLabelString" )); } else { assignedNode = null ; } canRoam = assignedNode== null ;
            Hide
            mdonohue mdonohue added a comment -

            My proposed fix to Jenkins core is here: https://github.com/mdonohue/jenkins/commit/2cb89d3e269318644fabd1acb4861690e662aef9

            I am having difficulty testing this code right now - JDK8 on Windows 10 results in several unit test failures doing a 'mvn install' on jenkins. I think JDK7 will do better, but I couldn't find any documentation of that on the jenkins wiki. JDK7 testing will happen another day.

            Show
            mdonohue mdonohue added a comment - My proposed fix to Jenkins core is here: https://github.com/mdonohue/jenkins/commit/2cb89d3e269318644fabd1acb4861690e662aef9 I am having difficulty testing this code right now - JDK8 on Windows 10 results in several unit test failures doing a 'mvn install' on jenkins. I think JDK7 will do better, but I couldn't find any documentation of that on the jenkins wiki. JDK7 testing will happen another day.
            Hide
            mdonohue mdonohue added a comment -

            JDK7 didn't fare any better. Does Jenkins-core build on windows cleanly?

            Show
            mdonohue mdonohue added a comment - JDK7 didn't fare any better. Does Jenkins-core build on windows cleanly?
            Hide
            mdonohue mdonohue added a comment -

            And here is the pull request: https://github.com/jenkinsci/jenkins/pull/1881

            I'm still working on getting it accepted.

            Show
            mdonohue mdonohue added a comment - And here is the pull request: https://github.com/jenkinsci/jenkins/pull/1881 I'm still working on getting it accepted.
            Hide
            mdonohue mdonohue added a comment -

            I don't know how to get any more attention for this one. I just added a comment to the pull request.

            Show
            mdonohue mdonohue added a comment - I don't know how to get any more attention for this one. I just added a comment to the pull request.
            Hide
            danielbeck Daniel Beck added a comment -

            mdonohue With ~550 PRs this year so far, the time of the handful of people able to review PRs is stretched thin.

            Pinging people often helps (place an at character before their GitHub user name, e.g. @jtnord, not jtnord), as does having tests that show your change is safe (if it's not obvious).

            Show
            danielbeck Daniel Beck added a comment - mdonohue With ~550 PRs this year so far, the time of the handful of people able to review PRs is stretched thin. Pinging people often helps (place an at character before their GitHub user name, e.g. @jtnord , not jtnord ), as does having tests that show your change is safe (if it's not obvious).
            Hide
            jcg44 JC G added a comment -

            Hi,

            Can someone tell me whether a fix is available to avoid this behaviour, as I use a Jenkins 1.580 ?

            The Label Expression parameter is just set to null for jobs I move to the master label, but when launched, they run on the master, that's exactly what I want.

            How can I be sure it will work like that any time ?

            Can I consider this is just a matter of display ?

             

            Thanks in advance

            Show
            jcg44 JC G added a comment - Hi, Can someone tell me whether a fix is available to avoid this behaviour, as I use a Jenkins 1.580 ? The Label Expression parameter is just set to null for jobs I move to the master label, but when launched, they run on the master, that's exactly what I want. How can I be sure it will work like that any time ? Can I consider this is just a matter of display ?   Thanks in advance
            Hide
            dhs Dirk Heinrichs added a comment - - edited

            Can this be fixed, please? I don't understand why the label field being "null" means "master". This is highly confusing. Just write "master" into it. This is especially true since the help of the text input field states (under Notes): "An empty expression will always evaluate to true, matching all agents." and then explicitely lists master under Examples.

            Show
            dhs Dirk Heinrichs added a comment - - edited Can this be fixed, please? I don't understand why the label field being "null" means "master". This is highly confusing. Just write "master" into it. This is especially true since the help of the text input field states (under Notes ): "An empty expression will always evaluate to true , matching all agents." and then explicitely lists master under Examples .

              People

              • Assignee:
                mdonohue mdonohue
                Reporter:
                alskor Alex Java
              • Votes:
                3 Vote for this issue
                Watchers:
                5 Start watching this issue

                Dates

                • Created:
                  Updated: