Add PauseAction when throttling pipelines #134
Conversation
nickb-carallon
changed the title
nickb-carallon
changed the title
|
Not sure what went wrong with Jenkins, but the test ran fine locally. |
There was a problem hiding this comment.
Can you start by describing the high-level problem you are trying to solve? What is the current behavior, and what is the desired behavior? I placed a breakpoint after your new call to TestUtil.hasPauseActionForItem(queuedItem), but I don't see any difference in functionality in the UI. I need to understand the high-level problem before I can evaluate the design of the solution and its implementation.
src/main/java/hudson/plugins/throttleconcurrents/ThrottleQueueTaskDispatcher.java
Show resolved
Hide resolved
| flowNode.addAction(new PauseAction(cause.getShortDescription())); | ||
| } | ||
| } else { | ||
| PauseAction.endCurrentPause(flowNode); |
There was a problem hiding this comment.
PauseAction#endCurrentPause has the following code:
LOGGER.log(Level.FINE, "‘endCurrentPause’ was called for a FlowNode (‘{0}’) that does not have an active pause. ‘endCurrentPause’ may have already been called.", node.getDisplayName());That code should never get called; it indicates a programming error. Putting a breakpoint here, I see this is getting called a lot with your PR.
| @@ -682,5 +689,26 @@ private int getMaxConcurrentPerNodeBasedOnMatchingLabels( | |||
| return maxConcurrentPerNodeLabeledIfMatch; | |||
| } | |||
|
|
|||
| private void updatePauseAction(Task task, CauseOfBlockage cause) { | |||
There was a problem hiding this comment.
How does this relate to throttling of Pipeline jobs with a throttle job property rather than a throttle step? Every other usage of new PauseAction that I can find in the jenkinsci organization (e.g. in Lockable Resource, SonarQube, and Pipeline: Input Step) is from a StepExecution. Should this be specific to a throttle step (see ThrottleStep and ThrottleStepExecution)?
There was a problem hiding this comment.
The pause action only applies to stages within a pipeline, so doesn't apply for pipelines using the throttle job property.
| @@ -682,5 +689,26 @@ private int getMaxConcurrentPerNodeBasedOnMatchingLabels( | |||
| return maxConcurrentPerNodeLabeledIfMatch; | |||
| } | |||
|
|
|||
| private void updatePauseAction(Task task, CauseOfBlockage cause) { | |||
| if (task instanceof PlaceholderTask) { | |||
There was a problem hiding this comment.
task can also be of type WorkflowJob. Do we need to handle this as well?
There was a problem hiding this comment.
No, see above.
| @@ -98,6 +98,7 @@ public void onePerNode() throws Exception { | |||
| assertThat( | |||
| blockageReasons, | |||
| hasItem(Messages._ThrottleQueueTaskDispatcher_MaxCapacityOnNode(1).toString())); | |||
| TestUtil.hasPauseActionForItem(queuedItem); | |||
There was a problem hiding this comment.
This assertion needs to be added to other tests as well, at least in ThrottleStepTest (if not also the other Pipeline tests).
Thanks for the thorough review! I realise now that PauseAction is only used by stage visualiser plugins (such as pipeline-stage-view-plugin) and it also requires the use of stages in the pipeline. Neither of these are used in the tests which is why you didn't see a change in the UI. I'll rework this PR to resolve your issues and implement a better test for showing the effect of my changes. |
|
I hope I've resolved your questions now. I've also adjusted the throttle step tests so that, if you place a breakpoint, you should see the pipeline stage view indicate that the pipeline is paused. |
|
I've had a look at the failing tests, and they seem to be due to a deadlock between the maintain() call on line 149 of ThrottleJobPropertyPipelineRestartTest.java, and the getNode() call on line 696 of ThrottleQueueTaskDispatcher.java. I'm not sure how to resolve this, is there anyone/anywhere that could help with this issue? |
The PauseAction is used by pipeline visualisers (such as pipeline-stage-view) to indicate when a stage has been paused, and for how long is was staged for. This can improve the display of stages timings, as the paused time is removed from the overall stage time. The PauseAction only applies to stages, so only applies when the throttle step is used in conjunction with a pipeline stage.
Huh, tests seem to be passing now 🙃 |
This change will allow the pipeline UI to show when a stage is throttled, and how long for.