[JENKINS-31801] Add Pipeline throttle(category) step#46
Conversation
| <properties> | ||
| <jenkins.version>1.609.3</jenkins.version> | ||
| <jenkins.version>1.642.3</jenkins.version> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> |
There was a problem hiding this comment.
Yup yup - at one point, I was experimenting with stuff that did need newer versions of other dependencies and bumped things, but I'll roll this back now.
| <compileSource>1.6</compileSource> | ||
| <compileTarget>1.6</compileTarget> | ||
| <compileSource>1.7</compileSource> | ||
| <compileTarget>1.7</compileTarget> |
There was a problem hiding this comment.
java.level should be used instead
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <compileSource>1.6</compileSource> | ||
| <compileTarget>1.6</compileTarget> | ||
| <compileSource>1.7</compileSource> |
There was a problem hiding this comment.
Brought these back now that we're depending on 1.642.3 again.
| List<Queue.Task> categoryTasks = new ArrayList<Queue.Task>(); | ||
| Collection<ThrottleJobProperty> properties; | ||
| DescriptorImpl descriptor = Jenkins.getActiveInstance().getDescriptorByType(DescriptorImpl.class); | ||
| DescriptorImpl descriptor = Jenkins.getActiveInstance().getDescriptorByType(DescriptorImpl.class); |
There was a problem hiding this comment.
Please avoid gratuitous whitespace changes. Configure your editor to not make them.
| // No run found, so remove the throttle. | ||
| descriptor.removeThrottledPipelineForCategory(currentPipeline.getKey(), category, null); | ||
| } else if (!(flowNodeRun instanceof FlowExecutionOwner.Executable)) { | ||
| // If for some reason we've somehow ended up with a non-pipeline job, remove the throttle. |
There was a problem hiding this comment.
Why not just index by FlowExecutionOwner to begin with?
There was a problem hiding this comment.
Is there an easy way to go from some easily serializable identifier for FlowExecutionOwner to an actual instance of FlowExecutionOwner, like with Run.fromExternalizableId?
There was a problem hiding this comment.
Ah. Still not sure I want to actually save that large a chunk of data when I can just do the round-tripping...
| return new ThrottleStepExecution(this, context); | ||
| } | ||
|
|
||
| } |
| if (parentInfo != null) { | ||
| // Make sure we record the node used for the parent throttle step if it exists. | ||
| Computer computer = getContext().get(Computer.class); | ||
| if (computer != null && computer.getNode() != null) { |
There was a problem hiding this comment.
Return Node from getRequiredContext and skip both the null check and the getNode conversion.
There was a problem hiding this comment.
Aaaaah, for some reason I didn't think that was possible. Win!
|
|
||
| @Override | ||
| public void stop(Throwable cause) throws Exception { | ||
| if (body != null) |
There was a problem hiding this comment.
This method can be left blank—will not be called for a block-scoped step anyway, unless it is doing something between returning from start and the BodyInvoker.start.
|
|
||
| body = getContext().newBodyInvoker() | ||
| .withContext(info) | ||
| .withCallback(BodyExecutionCallback.wrap(getContext())) |
There was a problem hiding this comment.
I think you meant to use TailCall and call removeThrottledPipelineForCategory.
There was a problem hiding this comment.
Aaaah, you're right. Thanks.
| ThrottleMatrixProjectOptions.DisplayName=Additional options for Matrix projects No newline at end of file | ||
| ThrottleMatrixProjectOptions.DisplayName=Additional options for Matrix projects | ||
|
|
||
| ThrottleJobProperty.DescriptorImpl.NoSuchCategory=Requested category "{0}" does not exist, so cannot throttle. No newline at end of file |
|
@jglick Did a fairly drastic rewrite here - I think I got out of the blocking-CPS-VM issues, and am tracking |
|
I would be happy to test this! 👍 |
|
Would this be a correct usage, see test stage? Sorry for sick indentation but such is life 🍎 pipeline {
agent none
options {
buildDiscarder(logRotator(numToKeepStr:'20'))
}
stages {
stage('Build') {
agent { label 'ios' }
steps {
withCredentials([string(credentialsId: 'keychain', variable: 'PASSWORD')]) {
sh "security list-keychains -s ~/Library/Keychains/login.keychain"
sh "security unlock-keychain -p ${PASSWORD} ~/Library/Keychains/login.keychain"
}
wrap([$class: 'AnsiColorBuildWrapper', 'colorMapName': 'XTerm', 'defaultFg': 1, 'defaultBg': 2]) {
withCredentials([
string(credentialsId: 'fastlaneMatch', variable: 'MATCH_PASSWORD'),
string(credentialsId: 'fastlanePassword', variable: 'FASTLANE_PASSWORD')])
{
sh 'fastlane build'
}
}
}
}
stage('Test') {
agent none
steps {
throttle('ios-simulator') {
node('ios') {
lock("ios-simulator-${env.NODE_NAME}") { // Do I even need lock?
withCredentials([string(credentialsId: 'keychain', variable: 'PASSWORD')]) {
sh "security list-keychains -s ~/Library/Keychains/login.keychain"
sh "security unlock-keychain -p ${PASSWORD} ~/Library/Keychains/login.keychain"
}
retry(5) {
wrap([$class: 'AnsiColorBuildWrapper', 'colorMapName': 'XTerm', 'defaultFg': 1, 'defaultBg': 2]) {
withCredentials([
string(credentialsId: 'fastlaneMatch', variable: 'MATCH_PASSWORD'),
string(credentialsId: 'fastlanePassword', variable: 'FASTLANE_PASSWORD')])
{
sh 'fastlane test'
}
}
}
}
}
}
}
}
}
} |
Needed to bump to newer dependency versions, most notably to get PlaceholderTask.getNode(). Still a work in progress, mind you.
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>plugin</artifactId> | ||
| <version>2.6</version> | ||
| <version>2.22</version> |
There was a problem hiding this comment.
2.23 led to errors due to a TestExtension usage in workflow-support's tests, so I bumped back down to 2.22 for now at least.
There was a problem hiding this comment.
Harmless, just stack traces. workflow-support itself should be bumped up.
|
|
||
| <properties> | ||
| <jenkins.version>1.609.3</jenkins.version> | ||
| <jenkins.version>1.642.3</jenkins.version> |
There was a problem hiding this comment.
As mentioned in the commit message, we needed to bump dependency versions to get the latest durable-task-step for PlaceholderTask.getNode().
|
|
||
| // Handle multi-configuration filters | ||
| if (!shouldBeThrottled(task, tjp)) { | ||
| if (!shouldBeThrottled(task, tjp) && pipelineCategories.isEmpty()) { |
There was a problem hiding this comment.
So re: this and later similar ugliness - I plan to rewrite ThrottleQueueTaskDispatcher pretty much completely before I'm done here, or at least rationalize it a lot better. This was me wedging things together until they worked, with clean up planned for afterwards.
| " node('first-agent') {\n" + | ||
| " semaphore 'wait-first-job'\n" + | ||
| " }\n" + | ||
| "}\n", false)); |
There was a problem hiding this comment.
Soooo - as I mentioned a few lines up, I'm getting script security exceptions on this when the sandbox is enabled and I can't figure out why, so for the moment, disabling script security until someone (probably @jglick!) tells me what I'm doing wrong to cause those errors to show up. =)
Also discovered that Run<?,?> is a very bad Map key.
| " node('" + label + "') {\n" + | ||
| " semaphore 'wait-" + jobName + "-job'\n" + | ||
| " }\n" + | ||
| "}\n", false); |
There was a problem hiding this comment.
Soooo - as I mentioned a few lines up, I'm getting script security exceptions on this when the sandbox is enabled and I can't figure out why, so for the moment, disabling script security until someone (probably @jglick!) tells me what I'm doing wrong to cause those errors to show up. =)
|
This pull request originates from a CloudBees employee. At CloudBees, we require that all pull requests be reviewed by other CloudBees employees before we seek to have the change accepted. If you want to learn more about our process please see this explanation. |
|
This supersedes #34 ? |
|
Just checking if agent { label } + throttle with work same as agent none + throttle + node(label) Are these two pipeline equal since throttle might run before reserving the executor? |
|
Would it be possible to have agent label and throttle work so that we can have post steps that have file path context. I would perhaps suggest this syntax but I know it requires work on the pipeline model definition instead. This one below does not work: Neither does this: |
oleg-nenashev
left a comment
There was a problem hiding this comment.
Looks good so far, but I do not feel this is a final implementation
| <compileSource>1.6</compileSource> | ||
| <compileTarget>1.6</compileTarget> | ||
| <compileSource>1.7</compileSource> | ||
| <compileTarget>1.7</compileTarget> |
There was a problem hiding this comment.
java.level should be used instead
| <packaging>hpi</packaging> | ||
| <name>Jenkins Throttle Concurrent Builds Plug-in</name> | ||
| <version>1.9.1-SNAPSHOT</version> | ||
| <version>2.0-SNAPSHOT</version> |
There was a problem hiding this comment.
I would advice going through the beta- or -rc for this change. The potential impact is too high.
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <compileSource>1.6</compileSource> | ||
| <compileTarget>1.6</compileTarget> | ||
| <compileSource>1.7</compileSource> |
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
| <artifactId>workflow-durable-task-step</artifactId> | ||
| <version>2.8</version> |
There was a problem hiding this comment.
Is there a need to have it as required dependency? It pulls a significant number of Pipeline plugins into (like Pipeline Support & Co)
There was a problem hiding this comment.
Yes - we need ExecutorStepExecution.class for checking against, etc.
There was a problem hiding this comment.
You do not use this class from what I see
There was a problem hiding this comment.
Ah, that may have gone away. Fixing.
There was a problem hiding this comment.
Nope, it's still there - specifically ExecutorStepExecution.PlaceholderTask in ThrottleQueueTaskDispatcher.
| this.step = step; | ||
| } | ||
|
|
||
| public String getCategory() { |
| } | ||
|
|
||
| @Nonnull | ||
| public String getCategory() { |
There was a problem hiding this comment.
Only one category? I would expect an opportunity to define multiple categories like I can do for Job Properties
There was a problem hiding this comment.
I'd rather keep it simple and wait to see what the demand is like for more complex.
There was a problem hiding this comment.
First of all thanks for this Pullrequest I was waiting for it for quite some time :)
I would really like to be able to throttle multiple categories as this is our usual scenario in our freestyle jobs.
|
|
||
| @Override | ||
| public void stop(Throwable cause) throws Exception { | ||
|
|
There was a problem hiding this comment.
Should it also call removeThrottledPipelineForCategory ?
There was a problem hiding this comment.
Apparently, from Jesse's earlier comments, no - stop doesn't have relevance here.
| <?jelly escape-by-default='true'?> | ||
| <j:jelly xmlns:j="jelly:core" xmlns:st="jelly:stapler" xmlns:d="jelly:define" | ||
| xmlns:l="/lib/layout" xmlns:t="/lib/hudson" xmlns:f="/lib/form"> | ||
| <f:entry title="${%Category}" field="category"> |
There was a problem hiding this comment.
I would expect multiple categories
|
|
||
| import static org.assertj.core.api.Assertions.assertThat; | ||
|
|
||
| @Ignore("Depends on a newer version of Guava than can be used with Pipeline") |
There was a problem hiding this comment.
It is a pretty important test suite. I do not feel that it is really required to have such declarative syntax here, so maybe it worth just rewriting it
There was a problem hiding this comment.
I agree. I'll tackle that either here or in a follow-up PR.
There was a problem hiding this comment.
Actually, on second thought, this is basically covered by ThrottleStepTest, isn't it?
There was a problem hiding this comment.
I take that back - did some experimenting and there is something weird once you've got enough Pipeline tasks in queue. Working on it.
There was a problem hiding this comment.
Aaaah, queue.getPendingItems(). Fixed.
| } | ||
|
|
||
| @Override | ||
| public boolean start() throws Exception { |
There was a problem hiding this comment.
Maybe makes sense to contribute some information to the build log using the BuildListener from the context
jglick
left a comment
There was a problem hiding this comment.
Only have a fuzzy idea what this is doing, so pobably not going to be a good reviewer.
| <groupId>org.jenkins-ci.plugins</groupId> | ||
| <artifactId>plugin</artifactId> | ||
| <version>2.6</version> | ||
| <version>2.22</version> |
There was a problem hiding this comment.
Harmless, just stack traces. workflow-support itself should be bumped up.
| private boolean isTaskThrottledPipeline(Task origTask, List<FlowNode> flowNodes) { | ||
| if (origTask instanceof PlaceholderTask) { | ||
| PlaceholderTask task = (PlaceholderTask)origTask; | ||
| try { |
There was a problem hiding this comment.
This really deserves a proper API so you can get rid of the workflow-durable-task-step dependency. It was never intended as an API plugin.
| enclosing.getAction(BodyInvocationAction.class) == null) { | ||
| // Check if this is a *different* throttling node. | ||
| StepDescriptor desc = ((StepNode) enclosing).getDescriptor(); | ||
| if (desc != null && desc.getClass().equals(ThrottleStep.DescriptorImpl.class)) { |
| import java.util.Set; | ||
| import java.util.StringTokenizer; | ||
|
|
||
| public class ThrottleStep extends Step implements Serializable { |
There was a problem hiding this comment.
If you just have the one field, probably better to inline that into the execution state.
|
@oleg-nenashev So are we good to go here? I defer to you for merging and releasing. |
oleg-nenashev
left a comment
There was a problem hiding this comment.
Just minor comments so far. Will test it manually this week
| <dependency> | ||
| <groupId>org.jenkins-ci.plugins.workflow</groupId> | ||
| <artifactId>workflow-durable-task-step</artifactId> | ||
| <version>2.8</version> |
There was a problem hiding this comment.
You do not use this class from what I see
|
|
||
| StringTokenizer tokenizer = new StringTokenizer(categories, ","); | ||
| while (tokenizer.hasMoreTokens()) { | ||
| catList.add(tokenizer.nextToken().trim()); |
There was a problem hiding this comment.
If I understand correctly, it is going to inject empty strings. Which may become a real problem if somebody uses something like cat_a,${myvar},cat_c expression
There was a problem hiding this comment.
I mean strings like throttle("cat_a,,cat_b"), which may come to this method
There was a problem hiding this comment.
Aaaaaah. Changing to discard empty.
| return Collections.singleton(TaskListener.class); | ||
| } | ||
|
|
||
| public FormValidation doCheckCategoryName(@QueryParameter String value) { |
There was a problem hiding this comment.
Restricted ? not sure if RequirePost is mandatory
There was a problem hiding this comment.
Good question. This doesn't actually expose any meaningful information - all you can do is tell whether there's a category with a given name, not anything about the category, etc. I think it's harmless, but defer to others.
|
|
||
| @Override | ||
| public String getDisplayName() { | ||
| return "Throttle execution of node blocks within this body"; |
There was a problem hiding this comment.
Nice2have: make it localizable
| firstThrottle.getId()); | ||
| } | ||
| } catch (IOException | InterruptedException e) { | ||
| return new ArrayList<>(); |
| return new ArrayList<>(); | ||
| } | ||
| } | ||
| return new ArrayList<>(); |
There was a problem hiding this comment.
Nice2have: assert : false
There was a problem hiding this comment.
? We're calling this method for all tasks, so an assert would cause problems.
oleg-nenashev
left a comment
There was a problem hiding this comment.
The success path behavior looks good. Though 🐛 from me according to the UX testing
Documentation:
- The feature is not documented. I am working on Wiki => GitHub migration in a parallel PR to make it possible
Configuration:
- No validation of the categories field in the snippet generator, no context help about available categories
- No built-n Documentation for the "Categories" field in the Snipper generator. 🐛 according to the criteria we have IIRC
- Multiple categories are being passed as string, not as a list/array. It complicates management of categories in the code
Behavior:
- If the declared category does not exist, it is just ignored. It is not safe if a user plans to throttle a special resource (like my favorite FPGAs) and makes a typo in Pipeline. I would expect the script to fail if there is a non-existent category. Smells like a 🐛 to me
- Behavior for duplicated definitions is not clear. One may expect
throttle('A,A,A')to acquire 3 items from the category poolAto run. I would add an explicit disclaimer that it is not how it is supposed to work - "Throttle Concurrent Builds" Job Property appears in the UI, but does nothing for both modes from what I see. It may confuse plugin users, hence 🐛. The JobProperty should be hidden IMHO, it is not a part of MVP to have its support for Pipeline
|
@oleg-nenashev Thanks for testing it out! Snippet generator stuff definitely needs to be updated - I'll get that done.
So this is an interesting question. I've found surprisingly few examples of steps taking lists, so I went conservative. I'll play around with this to see if I can get a good list or array approach.
I wasn't sure what the best behavior would be in this case. I opted for a "first, do no harm" approach, but am fine with switching to fail.
Huh. I literally never thought about that. So you're thinking logging something when there are duplicate categories?
This isn't a change introduced by this PR - I think I did that a while back. It would, I think, apply to the full Pipeline execution, but yeah, it's pretty useless. I just worry that someone out there may be using it and would get screwed if we dropped it. |
Yeah, this is my approach as well. I just expect less harm if the job fails than if it gets executed incorrectly. Better fail than
Or I can probably just add note to README in #47.
I will check if it actually works. If yes, likely we need to polish UI a bit to prevent confusion of users. Not sure how exactly |
|
@oleg-nenashev Ok, I think I've responded to everything but the job property thing (which is a separate matter entirely). |
|
@abayer After some consideration, I still feel bad about the TCB JobProperty UX. Would it be possible to add a disclaimer to its built-in help at least? |
|
@oleg-nenashev I...guess? I'd almost rather just make it not show up and say "screw it" to anyone who's for some reason somehow using the job property with a Pipeline currently. |
|
Also, I'm not the one who added that support in the first place, as far as I can tell. =) |
It is 2.0, so probably it is a valid option. The possible confusion is too high since the behavior is not documented at all. And the plugin explicitly says Pipeline is not supported. Just add incompatibleSince to pom.xml in such case @Casz @jenkinsci/code-reviewers If you use Global config for TCB, please let us know |
|
@oleg-nenashev Then frankly, I think documenting it is a separate issue, isn't it? |
think some basic built-in docs should be included in this PR, because the new functionality leads to confusion with the existing one. A couple of sentences in JobProperty is enough. Regarding the full documentation, I will add it in #47 . Does it sound like a deal? |
|
Deal. |
oleg-nenashev
left a comment
There was a problem hiding this comment.
Needs some manual testing of UI, but the code looks good to me. 🐝
| } | ||
|
|
||
| public List<ThrottleJobProperty.ThrottleCategory> getCategories() { | ||
| return ThrottleJobProperty.fetchDescriptor().getCategories(); |
There was a problem hiding this comment.
Wrap to the unmodifyableList?
|
@reviewbybees done |
|
All my comments have been addressed. I have created https://issues.jenkins-ci.org/browse/JENKINS-44373 for better diagnosability, but it is no a blocker for the merge. @abayer should I squash it? |
JENKINS-31801
Note that yes, this depends on
workflow-api- I need that forFlowExecutionand friends. And I had to remove the existingguava:18.0test dependency due to that violently conflicting withStepContext. I haven't actually run tests yet, let alone written new ones, but I wanted to throw this up for initial feedback ASAP.cc @reviewbybees esp @jglick for Pipeline sanity review and @oleg-nenashev for Throttle sanity review