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

Add support of throttling of the entire build in Declarative Pipeline

    XMLWordPrintable

    Details

    • Similar Issues:

      Description

      So, I'm having this problem that I described in a similar bug for the lockable-resource plugin (JENKINS-45138). I said to myself, "oh, hey, I remember being able to throttle executions on a per-agent basis!"

      Imagine my surprise when I hit the documentation and find that throttle is only applicable inside a step.

      I need to acquire, use, and cleanup exclusive access to a resource on each agent. Will throttle work how I expect?

      step('foo') {
          throttle(['foo-label'])
          bat '... acquire the resource...'
          bat '... use the resource...'
      }
      post {
          always {
              bat '... cleanup the resource...'
          }
      }
      

        Attachments

          Activity

          Hide
          gpaciga Gregory Paciga added a comment -

          I can confirm that when running a (Scripted or Declarative) Pipeline job whose ThrottleJobProperty has throttleOption set to category and whose corresponding ThrottleCategory has a positive value of maxConcurrentPerNodeThrottleCategory#maxConcurrentPerNode is not respected (as demonstrated in ThrottleJobPropertyPipelineTest#onePerNode). Note that ThrottleCategory#maxConcurrentPerNode is respected for Freestyle jobs (as demonstrated in ThrottleJobPropertyFreestyleTest#onePerNode) and for (Scripted) Pipeline jobs that use ThrottleStep rather than ThrottleJobProperty (as demonstrated in ThrottleStepTest#onePerNode).

          There seem to be a two things at play preventing this:

          1. the method to get the job's throttle settings only works on instances of hudson.model.Job, but for pipelines (at least Scripted pipelines) the task is actually a 
          ExecutorStepExecution$PlaceholderTask. It is easy to add access to the throttle property by checking for this type and getting the property from task.getOwnerTask()
           
          2. the logic to count whether a running task is equal to a queued task needs to be expanded to include pipelines. The task passed to ThrottleQueueTaskDispatcher#buildsOnExecutor is a PlaceholderTask when checking the job's properties and equality can be checked with

          task.getOwnerTask().equals(currentExecutable.getParent().getOwnerTask()) 

          but when checking based on category, the task passed is the actual WorkflowJob, and then this equality check works:

          task.equals(currentExecutable.getParent().getOwnerTask()) 

          but, then pipelines get counted twice because they're counted both on a flyweight executor and regular executors, even before the job starts. I don't know what flyweight executors are or, more importantly, why they're being counted. They don't appear to come into play for freestyle jobs. On my local fork where I've been experimenting, I'm tempted to just skip counting flyweights for these two cases, and then the per node limits seem to work as you'd expect them too. Obviously the flyweight counts are there for a reason, though. Those equalities above are also from trial and error so they may not be comparing the right objects conceptually.

          I could open a PR that does all this, but since I only use scripted pipelines, it's very likely what I'm doing is not type safe and will break for either other job types or other uses of this plugin.

           

          Show
          gpaciga Gregory Paciga added a comment - I can confirm that when running a (Scripted or Declarative) Pipeline job whose  ThrottleJobProperty  has  throttleOption  set to  category  and whose corresponding  ThrottleCategory  has a positive value of  maxConcurrentPerNode ,  ThrottleCategory#maxConcurrentPerNode  is not respected (as demonstrated in  ThrottleJobPropertyPipelineTest#onePerNode ). Note that  ThrottleCategory#maxConcurrentPerNode  is respected for Freestyle jobs (as demonstrated in  ThrottleJobPropertyFreestyleTest#onePerNode ) and for (Scripted) Pipeline jobs that use  ThrottleStep  rather than  ThrottleJobProperty  (as demonstrated in  ThrottleStepTest#onePerNode ). There seem to be a two things at play preventing this: 1. the method to get the job's throttle settings only works on instances of hudson.model.Job, but for pipelines (at least Scripted pipelines) the task is actually a  ExecutorStepExecution$PlaceholderTask. It is easy to add access to the throttle property by checking for this type and getting the property from  task.getOwnerTask()   2. the logic to count whether a running task is equal to a queued task needs to be expanded to include pipelines. The task passed to ThrottleQueueTaskDispatcher#buildsOnExecutor is a PlaceholderTask when checking the job's properties and equality can be checked with task.getOwnerTask().equals(currentExecutable.getParent().getOwnerTask()) but when checking based on category, the task passed is the actual WorkflowJob, and then this equality check works: task.equals(currentExecutable.getParent().getOwnerTask()) but, then pipelines get counted twice because they're counted both on a flyweight executor and regular executors, even before the job starts. I don't know what flyweight executors are or, more importantly, why they're being counted. They don't appear to come into play for freestyle jobs. On my local fork where I've been experimenting, I'm tempted to just skip counting flyweights for these two cases, and then the per node limits seem to work as you'd expect them too. Obviously the flyweight counts are there for a reason, though. Those equalities above are also from trial and error so they may not be comparing the right objects conceptually. I could open a PR that does all this, but since I only use scripted pipelines, it's very likely what I'm doing is not type safe and will break for either other job types or other uses of this plugin.  
          Hide
          basil Basil Crow added a comment -

          Gregory Paciga that sounds right at a high level. I only use scripted Pipelines myself as well. I don't know too much about flyweight tasks, but I know they are used in Matrix Projects as well as in the portions of Pipeline jobs that run outside of a node block (typically on the controller's built-in node). The current behavior for flyweight executors was added well before my time in jenkinsci/throttle-concurrent-builds-plugin#23 and appears to be a workaround for JENKINS-24748, which relates to the Build Flow plugin (which itself is no longer supported). It's hard for me to answer definitively whether that flyweight task logic is still needed for Pipeline, though maybe someone else on the jenkins-dev mailing list might have more context. The most important thing for a change of this nature is to get plenty of real-world testing done prior to release. There are 39 people watching this issue, so I'm sure we could convince some of them to install and test an incremental build if you filed a PR. Also note that the documentation regarding what is and isn't supported in this plugin is woefully out-of-date and would need to be updated as well.

          Show
          basil Basil Crow added a comment - Gregory Paciga that sounds right at a high level. I only use scripted Pipelines myself as well. I don't know too much about flyweight tasks, but I know they are used in Matrix Projects as well as in the portions of Pipeline jobs that run outside of a node block (typically on the controller's built-in node). The current behavior for flyweight executors was added well before my time in jenkinsci/throttle-concurrent-builds-plugin#23 and appears to be a workaround for JENKINS-24748 , which relates to the Build Flow plugin (which itself is no longer supported). It's hard for me to answer definitively whether that flyweight task logic is still needed for Pipeline, though maybe someone else on the jenkins-dev mailing list might have more context. The most important thing for a change of this nature is to get plenty of real-world testing done prior to release. There are 39 people watching this issue, so I'm sure we could convince some of them to install and test an incremental build if you filed a PR. Also note that the documentation regarding what is and isn't supported in this plugin is woefully out-of-date and would need to be updated as well.
          Hide
          thehosh Hosh added a comment -

          We're using declarative pipelines, I'd be happy to test it out.

          Show
          thehosh Hosh added a comment - We're using declarative pipelines, I'd be happy to test it out.
          Hide
          gpaciga Gregory Paciga added a comment -

          I think there is a case where a pipeline job is occupying a flyweight but not a regular executor, then, and it won't be counted towards the limit. 

          This is what I have so far: https://github.com/gpaciga/throttle-concurrent-builds-plugin/tree/jenkins-45140-throttle-pipeline-per-node

          All the changes are confined to ThrottleQueueTaskDispatcher.java. Tests break, but I can't easily identify why (I don't have any experience with writing plugin tests).

          Show
          gpaciga Gregory Paciga added a comment - I think there is a case where a pipeline job is occupying a flyweight but not a regular executor, then, and it won't be counted towards the limit.  This is what I have so far: https://github.com/gpaciga/throttle-concurrent-builds-plugin/tree/jenkins-45140-throttle-pipeline-per-node All the changes are confined to ThrottleQueueTaskDispatcher.java. Tests break, but I can't easily identify why (I don't have any experience with writing plugin tests).
          Hide
          basil Basil Crow added a comment -

          A promising start. I think you're starting to see just how hard fixing this issue really is. The fundamental problem is, as you wrote, differentiating between cases where a Pipeline job is executing logic outside of a node block (i.e., on a flyweight executor) vs when it is executing logic inside of a node block (i.e., on a regular executor). In Freestyle jobs, this problem does not exist. Note that the throttle step solves this problem in ThrottleJobProperty.getThrottledPipelineRunsForCategory by doing accounting to keep track of whether the Pipeline is or isn't in a (throttled) node block. It's not entirely clear to me how such accounting could be implemented outside of a custom step, which is why I wrote earlier that going down this route "would likely require significant knowledge of Pipeline internals".

          Feel free to open a PR if you want to keep discussing the technical details of this issue without spamming everyone who is watching this Jira ticket.

          Show
          basil Basil Crow added a comment - A promising start. I think you're starting to see just how hard fixing this issue really is. The fundamental problem is, as you wrote, differentiating between cases where a Pipeline job is executing logic outside of a node block (i.e., on a flyweight executor) vs when it is executing logic inside of a node block (i.e., on a regular executor). In Freestyle jobs, this problem does not exist. Note that the throttle step solves this problem in ThrottleJobProperty.getThrottledPipelineRunsForCategory by doing accounting to keep track of whether the Pipeline is or isn't in a (throttled) node block. It's not entirely clear to me how such accounting could be implemented outside of a custom step, which is why I wrote earlier that going down this route "would likely require significant knowledge of Pipeline internals". Feel free to open a PR if you want to keep discussing the technical details of this issue without spamming everyone who is watching this Jira ticket.

            People

            Assignee:
            Unassigned Unassigned
            Reporter:
            anthonymastrean Anthony Mastrean
            Votes:
            30 Vote for this issue
            Watchers:
            39 Start watching this issue

              Dates

              Created:
              Updated: