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

Revisit use of $JENKINS_SERVER_COOKIE and Launcher.kill

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      PlaceholderExecutable needs an environment variable to add to the context to ensure that Launcher.kill, called when the block exits, will clean up any stray external processes run on this node.

      Originally the name of this variable was chosen to be JENKINS_SERVER_COOKIE, which is set by Job (to a confidential hex key) and normally used by AbstractBuild for this purpose. Unlike in that case, for Workflow the value is overridden (to a random UUID) for each node block, since there may be several. I think the intent was to use the same variable name to suppress any unwanted kills coming from Jenkins core code, but there seem to be none. Anyway after the many refactorings of environment variable handling in Workflow, it turns out this is dead code!

      • FileMonitoringTask sets its own value for JENKINS_SERVER_COOKIE, to durable-<workspaceHash>, which overrides any other value, and is used by stop. So within a shell step, $JENKINS_SERVER_COOKIE is the one from durable-task, not PlaceholderExecutable.
      • env.JENKINS_SERVER_COOKIE picks up the value from Job, due to the precedence order in getEffectiveEnvironment.

      So it seems that while the intention is still good (we would like a final check that any processes started inside node are killed when the block exits), the implementation is not right. Probably PlaceholderExecutable just needs to pick an unrelated environment variable for this purpose. And this logic needs to be tested; for example:

      def file = new File(...);
      node {
        sh "(sleep 5; touch $file) &"
      }
      sleep 10
      assert !file.exists()
      

        Attachments

          Issue Links

            Activity

            jglick Jesse Glick created issue -
            jglick Jesse Glick made changes -
            Field Original Value New Value
            Link This issue is related to JENKINS-28131 [ JENKINS-28131 ]
            jglick Jesse Glick made changes -
            Link This issue depends on JENKINS-25938 [ JENKINS-25938 ]
            jglick Jesse Glick made changes -
            Epic Link JENKINS-35399 [ 171192 ]
            rtyler R. Tyler Croy made changes -
            Workflow JNJira [ 162900 ] JNJira + In-Review [ 181065 ]
            abayer Andrew Bayer made changes -
            Component/s pipeline-general [ 21692 ]
            abayer Andrew Bayer made changes -
            Component/s workflow-plugin [ 18820 ]
            jglick Jesse Glick made changes -
            Component/s workflow-durable-task-step-plugin [ 21715 ]
            Component/s pipeline [ 21692 ]
            jglick Jesse Glick made changes -
            Labels envinronment-variables process-killer envinronment-variables environment-variables process-killer
            jglick Jesse Glick made changes -
            Labels envinronment-variables environment-variables process-killer environment-variables process-killer
            mmitche Matthew Mitchell made changes -
            Description {{PlaceholderExecutable}} needs an environment variable to add to the context to ensure that {{Launcher.kill}}, called when the block exits, will clean up any stray external processes run on this node.

            Originally the name of this variable was chosen to be {{JENKINS_SERVER_COOKIE}}, which is set by {{Job}} (to a confidential hex key) and normally used by {{AbstractBuild}} for this purpose. Unlike in that case, for Workflow the value is overridden (to a random UUID) for each {{node}} block, since there may be several. I think the intent was to use the same variable name to suppress any unwanted kills coming from Jenkins core code, but there seem to be none. Anyway after the many refactorings of environment variable handling in Workflow, it turns out this is dead code!

            * {{FileMonitoringTask}} sets its own value for {{JENKINS_SERVER_COOKIE}}, to {{durable-<workspaceHash>}}, which overrides any other value, and is used by {{stop}}. So within a shell step, {{$JENKINS_SERVER_COOKIE}} is the one from {{durable-task}}, not {{PlaceholderExecutable}}.
            * {{env.JENKINS_SERVER_COOKIE}} picks up the value from {{Job}}, due to the precedence order in {{getEffectiveEnvironment}}.

            So it seems that while the intention is still good (we would like a final check that any processes started inside {{node}} are killed when the block exits), the implementation is not right. Probably {{PlaceholderExecutable}} just needs to pick an unrelated environment variable for this purpose. And this logic needs to be tested; for example:

            {code}
            def file = new File(...);
            node {
              sh "(sleep 5; touch $file) &"
            }
            sleep 10
            assert !file.exists()
            {code}
            {{PlaceholderExecutable}} needs an environment variable to add to the context to ensure that {{Launcher.kill}}, called when the block exits, will clean up any stray external processes run on this node.

            Originally the name of this variable was chosen to be {{JENKINS_SERVER_COOKIE}}, which is set by {{Job}} (to a confidential hex key) and normally used by {{AbstractBuild}} for this purpose. Unlike in that case, for Workflow the value is overridden (to a random UUID) for each {{node}} block, since there may be several. I think the intent was to use the same variable name to suppress any unwanted kills coming from Jenkins core code, but there seem to be none. Anyway after the many refactorings of environment variable handling in Workflow, it turns out this is dead code!
             * {{FileMonitoringTask}} sets its own value for {{JENKINS_SERVER_COOKIE}}, to {{durable-<workspaceHash>}}, which overrides any other value, and is used by {{stop}}. So within a shell step, {{$JENKINS_SERVER_COOKIE}} is the one from {{durable-task}}, not {{PlaceholderExecutable}}.
             * {{env.JENKINS_SERVER_COOKIE}} picks up the value from {{Job}}, due to the precedence order in {{getEffectiveEnvironment}}.

            So it seems that while the intention is still good (we would like a final check that any processes started inside {{node}} are killed when the block exits), the implementation is not right. Probably {{PlaceholderExecutable}} just needs to pick an unrelated environment variable for this purpose. And this logic needs to be tested; for example:
            {code:java}
            def file = new File(...);
            node {
              sh "(sleep 5; touch $file) &"
            }
            sleep 10
            assert !file.exists()
            {code}
            mmitche Matthew Mitchell made changes -
            Assignee Jesse Glick [ jglick ] Matthew Mitchell [ mmitche ]
            mmitche Matthew Mitchell made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            jglick Jesse Glick made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]
            jglick Jesse Glick made changes -
            Status In Review [ 10005 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            mykola Mykola Marzhan made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            jglick Jesse Glick made changes -
            Status Reopened [ 4 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            mykola Mykola Marzhan made changes -
            Resolution Fixed [ 1 ]
            Status Resolved [ 5 ] Reopened [ 4 ]
            jglick Jesse Glick made changes -
            Status Reopened [ 4 ] Resolved [ 5 ]
            Resolution Fixed [ 1 ]
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-46089 [ JENKINS-46089 ]

              People

              • Assignee:
                mmitche Matthew Mitchell
                Reporter:
                jglick Jesse Glick
              • Votes:
                1 Vote for this issue
                Watchers:
                9 Start watching this issue

                Dates

                • Created:
                  Updated:
                  Resolved: