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

[JENKINS-8618] Support maximum builds on slave #323

Merged
merged 12 commits into from Jan 28, 2019

Conversation

@autogun
Copy link
Contributor

autogun commented Nov 11, 2018

This is a rebased and conflict-free PR based on @ariaviran's original PR to support maximum builds on slave.

This code is running in our environment for more than a year (based on ver. 1.37 of the plugin) and it's doing very good job.

ariaviran and others added 3 commits Nov 29, 2016
Added support to set a maximum number of builds on a slave before it is
terminated in a setting called 'Maximum Total Uses'.

Resolves JENKINS-8618
@@ -96,6 +96,7 @@
public List<EC2Tag> tags;
public final String cloudName;
public AMITypeData amiType;
public String maxTotalUses;

This comment has been minimized.

Copy link
@jvz

jvz Nov 13, 2018

Member

Shouldn't this be an int?

Danny-Rehelis Danny-Rehelis
Danny-Rehelis added 2 commits Nov 17, 2018
* Exposed maxTotalUses configuration to already provisioned computer
* EC2AbstractSlaveTest to use old constructor (for testing purposes)
* Fixing some newlines and whitespaces
Copy link
Member

jvz left a comment

Looking pretty good! Just need the SlaveTemplate constructor added, but otherwise everything looks good to me.

…ard compatibility
…ckward compatibility
@jvz jvz requested review from thoulen and julienduchesne Nov 22, 2018
…rd compatibility

Hope this is the last one :-D
@jvz
jvz approved these changes Nov 26, 2018
Copy link
Member

jvz left a comment

Looks good to me, thanks!

@awiddersheim

This comment has been minimized.

Copy link

awiddersheim commented Dec 16, 2018

I'm interested in making use of this feature but looking at the implementation I have some concerns. I guess the biggest of which is nothing immediately pops out at me that would prevent an instance from terminating or stopping when there are still active builds running. Perhaps I'm missing something though.

There is this call computer.setAcceptingTasks(false); but the instance is immediately terminated after that without waiting any amount of time for any currently running jobs to complete.

Also, since the max number of builds is determined after jobs have completed, it seems possible that more than the configured number of builds may run.

Take for example the following scenario. I've configured 100 max builds for this executor and I'm at build 99. The job Foo runs and takes 20 seconds followed immediately by job Bar running which only takes 10 seconds. Presumably, the job Foo would just be killed off when the instance is terminated.

Maybe it would make more sense to do the counting of max builds in taskAccepted(). Once the max has been reached, computer.setAcceptingTasks(false) can be called to no longer accept any new jobs. Then in the postTask* stages, if max has been reached, wait for the last build to complete and terminate. Perhaps with something like this.

@autogun

This comment has been minimized.

Copy link
Contributor Author

autogun commented Dec 17, 2018

@awiddersheim You're totally right. This is exactly what happens.
This never bitten us for the sole reason we set our agents with # of executors set to 1 and therefore, overlooked.

See my last commit, following your suggestion, which fixes this.

computer.setAcceptingTasks(false);
slaveNode.terminate();
// At this point, if agent is in suspended state and has 1 last executer running, it is safe to terminate.
if (computer.countBusy() <= 1 && !computer.isAcceptingTasks()) {

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

nit: Indentation looks wrong here.

} else {
slaveNode.maxTotalUses = slaveNode.maxTotalUses - 1;
LOGGER.info("Agent " + slaveNode.instanceId + " is still in use by more than one (" + computer.countBusy() + ") executers");

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

This will log after every task. May need to guard it better so it only emits when applicable.

This comment has been minimized.

Copy link
@autogun

autogun Dec 18, 2018

Author Contributor

Something like this?

if (maxTotalUses <= 1) {
    LOGGER.info("Agent " + slaveNode.instanceId + " is still in use by more than one (" + computer.countBusy() + ") executers");
}

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

Yeah, I would only want to see this message when an instance is slated to be removed and the maxTotalUses feature is in use.

EC2AbstractSlave slaveNode = computer.getNode();
int maxTotalUses = slaveNode.maxTotalUses;
if (maxTotalUses <= -1) {
LOGGER.info("maxTotalUses set to unlimited (" + slaveNode.maxTotalUses + ") for agent " + slaveNode.instanceId);

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

I think it's debatable that this log message is of any use. At least at the INFO level.

This comment has been minimized.

Copy link
@autogun

autogun Dec 18, 2018

Author Contributor

Adding bunch of those loggers basically helps during the tests... I don't mind removing it tho.

0.361 [id=111]       INFO    jenkins.InitReactorRunner$1#onAttained: Completed initialization
   0.369 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUsesInstance has 3 builds left
   0.369 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUsesInstance has 2 builds left
   0.370 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUsesInstance has 1 builds left
   0.370 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUses drained - suspending agent maxTotalUsesInstance
   0.370 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#postJobAction: Agent maxTotalUsesInstance is terminated due to maxTotalUses (1)
   0.370 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUses drained - suspending agent maxTotalUsesInstance
   0.370 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#postJobAction: Agent maxTotalUsesInstance is terminated due to maxTotalUses (0)
   0.371 [id=94]        INFO    h.p.ec2.EC2RetentionStrategy#taskAccepted: maxTotalUses set to unlimited (-1) for agent maxTotalUsesInstance
   0.372 [id=94]        INFO    jenkins.model.Jenkins#cleanUp: Stopping Jenkins

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

Right, I'd just adjust to LOGGER.debug.

computer.setAcceptingTasks(false);
} else {
slaveNode.maxTotalUses = slaveNode.maxTotalUses - 1;
LOGGER.info("maxTotalUses set to " + slaveNode.maxTotalUses + " for agent " + slaveNode.instanceId);

This comment has been minimized.

Copy link
@awiddersheim

awiddersheim Dec 18, 2018

This message is slight misleading in my opinion. Instead of maxTotalUses set to, maybe something like slaveNode.instanceId + " has " + slaveNode.maxTotalUses + " builds left"

@stefanverhoeff

This comment has been minimized.

Copy link

stefanverhoeff commented Jan 28, 2019

@awiddersheim @thoulen this change would be very useful. What is stopping it from getting merged?

@awiddersheim

This comment has been minimized.

Copy link

awiddersheim commented Jan 28, 2019

I'm just some guy without commit access to this repo. 🤷‍♂️

@jvz jvz changed the title Support maximum builds on slave [JENKINS-8618] Support maximum builds on slave Jan 28, 2019
@jvz jvz merged commit 753894c into jenkinsci:master Jan 28, 2019
1 check passed
1 check passed
continuous-integration/jenkins/pr-merge This commit looks good
Details
@jvz

This comment has been minimized.

Copy link
Member

jvz commented Jan 28, 2019

No negative feedback over time, so I'm going to merge this. See related issue: https://issues.jenkins-ci.org/browse/JENKINS-8618

@stefanverhoeff

This comment has been minimized.

Copy link

stefanverhoeff commented Jan 28, 2019

Thanks @jvz + @awiddersheim :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.