Skip to content
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

Throttling flyweight tasks #34

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

Throttling flyweight tasks #34

wants to merge 16 commits into from

Conversation

dsavenko
Copy link
Contributor

Hi,

This pull request enables throttling flyweights tasks. No new UI's added, but now if a user turns on throttling for a flyweight job, it works.

Some bits of explanation:

  1. Throttling logic is not changed. It's only moved to the Throttler class. This is done because I need it in two places.
  2. Since QueueTaskDispatcher is not called for flyweights, I use the NodeProperty's canTake() instead. There is a gap between returning from canTake() and the build being assigned to some node (leave the queue). Another throttle subject may come to this node in this gap, which can result in throttling condition being violated. This is why I needed a static lock. With this lock, I make sure we don't make any actual decisions about throttling until the previous build actually leaves the queue.
  3. I need ThrottleNodeProperty to be present in all nodes, so automatically add it to each node on every computer change.
  4. Behaviour is not changed for non-flyweight tasks.
  5. I changed the Jenkins target version to 1.609.3, since earlier we agreed it's OK (see Reduce applicability requirements #32).

I realise the implementation's somewhat unusual. But I spent some time studying the Jenkins code and didn't found any other options to throttle flyweight tasks. Probably throttling flyweights usually doesn't make much sense. But we have real cases when it does.

Please, share your thoughts.

@jenkinsadmin
Copy link
Member

Thank you for this pull request! Please check this document for how the Jenkins project handles pull requests.

@oleg-nenashev oleg-nenashev self-assigned this Apr 10, 2016
@oleg-nenashev
Copy link
Member

needs a review

@damphyr
Copy link

damphyr commented Jul 14, 2016

Hi all.
I've been trying to use this to throttle jobs executed from a pipeline job and the throttling does not work.
Would this PR solve the problem?

The problem I ran into in detail:

I have a job (batch) which starts several parametrized instances of another job (compile).

I want to limit the number of nodes the compile job runs on, so I configured it with throttle-concurrent:

pipelineJob("Compile")
{
  concurrentBuild(true);
  throttleConcurrentBuilds {
    maxPerNode(1)
    maxTotal(3)
  }
  parameters {
    stringParam('TARGET', 'TGT', 'The target to compile')
  }
  environmentVariables {
    keepSystemVariables(true);
    keepBuildVariables(true);
  }
  definition {
    cps {
      script(readFileFromWorkspace('pipelines/compile.groovy'))
    }
  }

If I start them manually, the throttling works. The batch job is another pipeline definition:

def targets=["TGT1","TGT2","TGT3","TGT4","TGT5"]
def jobs=[:]
for(int i = 0; i < targets.size(); i++)
{
  def target= targets.get(i)
  println "triggering compilation for ${targets}"
  jobs[region]={
    build job: 'Compile', parameters: [[$class: 'StringParameterValue', name: 'TARGET', value: region]]
  }
}
parallel jobs

When this is triggered then I get all 5 compile jobs running immediately where I would expect execution to be throttled down to 3 nodes

@sshelomentsev
Copy link

Hi @damphyr
Yes, this PR solve the problem with throttling concurrent jobs in the pipeline.

@oleg-nenashev
Copy link
Member

I doubt it completely solves the throttling topic, at least for parallel() pipeline invocations. In general, this PR definitely deserves some attention from maintainers and integration. Node throttling approach rework is also reasonable.

Sorry about dropping a ball on it.

@iaroslav42
Copy link

iaroslav42 commented May 30, 2017

Hi, I've merged master into our branch and made some minor improvements. Could you please review it?
Just to clarify, #46 doesn't supersede this one

@mamh2021
Copy link

mamh2021 commented Jun 4, 2021

any update???

@ggkochanski
Copy link

@damphyr can you check if adding quietPeriod to build() doesn't help accidently?
e.g.
build job: 'Compile', quietPeriod: 3, parameters: [[$class: 'StringParameterValue', name: 'TARGET', value: region]]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
8 participants