[JENKINS-72841] - Pass flow node ID when skipping stage #700

timja · 2024-03-11T19:33:47Z

JENKINS issue(s):
Description:
- Previously skipping stages would only be tagged for the first occurrence of the stage. This now passes the flow node id rather than guessing by stage name
Documentation changes:
- Link to related jenkins.io PR or explanation for why doc change not needed
Users/aliases to notify:
- ...

Before:

After:

see jenkinsci/pipeline-graph-view-plugin#220

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

...model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy

timja · 2024-03-13T14:55:39Z

@dwnusbaum any chance of another review please?

timja · 2024-03-24T14:32:08Z

@jenkinsci/pipeline-model-definition-plugin-developers could someone take a look please?

dwnusbaum · 2024-04-09T13:45:27Z

...el-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ParallelTest.java

-        - 11: second stage start
-        - 12: second stage body
-        - 14: inner-first stage start (probably)
+        - 6:  parallel start


Hmm, I guess adding the call to getContext will break other tests that use Declarative and expect specific IDs as well. Probably worth a PCT run just to understand if the impact is wide enough that we should prep downstream PRs in advance. I filed jenkinsci/bom#3102 to check.

...model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy

...odel-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/MatrixTest.java

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

…s/pipeline/modeldefinition/Utils.groovy Co-authored-by: Devin Nusbaum <[email protected]>

Co-authored-by: Devin Nusbaum <[email protected]>

dwnusbaum · 2024-04-09T15:59:55Z

@timja Yes sorry I'm just running the OSS BOM and another proprietary PCT suite to see if it's worth trying to adjust getContext to only be called when a stage is going to be skipped to avoid having to update a bunch of tests.

dwnusbaum · 2024-04-09T16:43:41Z

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

@@ -238,15 +244,16 @@ class ModelInterpreter implements Serializable {
                      SkippedStageReason skippedReason) {
        return {
            script.stage(thisStage.name) {
+                def flowNodeId = getFlowNodeId()


Still waiting on the full results, but so far this breaks some tests in blueocean-pipeline-api-impl and another ~10 spread across some proprietary plugins I have access to:

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.sequentialParallelStagesWithPost

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.submitInputPostBlock

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.declarativeSyntheticSteps

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.encodedStepDescription

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.submitInputPostBlockWithParallelStages

blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.declarativeSyntheticSkippedStage

Maybe we could move the call to getFlowNodeId inside of skipStage so that the changes only affects tests that are directly related to skipped stages?

EDIT: Those are all the failures in the two PCT runs I did.

bd14425 will hopefully help with this, but tests like PipelineNodeTest.declarativeSyntheticSkippedStag will probably still need to be adjusted.

created a test run in jenkinsci/blueocean-plugin#2555

I tried to run locally but I can't get the plugin to build due to a like 6 year old npm / node version and unmaintained dependencies 😢

I tried to run locally but I can't get the plugin to build due to a like 6 year old npm / node version and unmaintained dependencies 😢

Yep, I ran into the same issue in jenkinsci/blueocean-plugin#2540.

With the latest changes, the total affected tests went from 15 to 6, so that's nice. The only remaining affected test in Blue Ocean appears to be blueocean-pipeline-api-impl / io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.submitInputPostBlockWithParallelStages. Here's the output:

org.junit.ComparisonFailure: expected:<[47]> but was:<[50]> at org.junit.Assert.assertEquals(Assert.java:117) at org.junit.Assert.assertEquals(Assert.java:146) at io.jenkins.blueocean.rest.impl.pipeline.PipelineNodeTest.submitInputPostBlockWithParallelStages(PipelineNodeTest.java:2314)

Let's try to update your Blue Ocean PR to fix the test. There are other tests that will need to be updated internally in CloudBees, but I will take care of that. I'll work on all of this later today.

Thanks, I've pushed a guess in timja/blueocean-plugin@5184941

I tried to run locally but I can't get the plugin to build due to a like 6 year old npm / node version and unmaintained dependencies 😢

Yep, I ran into the same issue in jenkinsci/blueocean-plugin#2540.

Based on a suggestion by @batmat, I tried building this plugin in the environment created by https://github.com/jenkinsci/blueocean-plugin/blob/1ce0c18265882733f3251dcad0b4af616da5a7c9/Dockerfile.build and it built just fine on my AMD64 system. So while definitely annoying, building this plugin locally is not insurmountable on AMD64. Presumably there is some way to run AMD64 code on ARM64 via emulation/virtualization, where the same Dockerfile.build could be used.

It doesn’t even work on amd64 macOS for me. Sure you can make it work on docker.

I’ve got a branch locally that has made a fair bit of progress but now need to update the JS builder and figure out who has access to release there

(jenkinsci/blueocean-plugin#2557 more work to go though)

I got it working in jenkinsci/blueocean-plugin#2558

This should help reduce the number of tests that need to be adjusted to update FlowNode IDs.

dwnusbaum · 2024-04-09T21:05:43Z

I pushed bd14425 to try to reduce the number of impacted tests and will rerun the PCT once a new incremental build is available. If nothing else it at least prevents us from having to update ParallelTest here.

dwnusbaum · 2024-04-09T22:44:01Z

I triggered another PCT run (just a CloudBees internal one this time) and will check the results tomorrow.

dwnusbaum

It's a bit awkward to use getContext like this, and it's annoying that it affects some tests, but even so, this seems preferable to the kind of approach that was used before and in #698. I've prepped some CloudBees-internal PRs to fix affected tests and will keep an eye on the build of jenkinsci/blueocean-plugin#2555. Once it looks like we've got things working there I'll go ahead and merge this to get a release out.

jglick · 2024-04-11T13:17:20Z

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy

@@ -222,6 +223,11 @@ class ModelInterpreter implements Serializable {

    }

+    @NonCPS
+    private String getFlowNodeId() {
+        return script.getContext(FlowNode.class).getId()


Can we not use the withContext step? It is intended for use in (really) power user trusted libraries, and it is just going to add overhead to the flow node graph for every Declarative build (as well as breaking tons of tests).

Would be better to add some helper method to Utils that is run silently, without recording a flow node.

Can we not use the withContext step?

getContext

It is intended for use in (really) power user trusted libraries

What is ModelInterpreter.groovy if not an incredibly complicated trusted library written by power users?

and it is just going to add overhead to the flow node graph for every Declarative build

One extra FlowNode per skipped stage. I do not think this is a big problem.

as well as breaking tons of tests

It breaks 6 known tests across OSS plugins and CloudBees-internal plugins, and PRs have already been filed to fix all of them.

Would be better to add some helper method to Utils that is run silently, without recording a flow node.

Yes, but here is the problem: Declarative has only a stage name, and wants to add/update tags on the corresponding FlowNode objects. Without an initial FlowNode to guide the scan, we have to scan the whole FlowGraph, and I am suspicious that there are some unfixable bugs with that approach given duplicate stage names in complex Pipelines. #698 shows that kind of approach. I think it is much clearer to just get a contextual ID to guide the search and the tradeoffs of having to use getContext do not seem that severe.

Perhaps the code could be adjusted to check the current heads and guess which one is the right one, but I think that is still problematic. For example if you use matrix and have two identically named stages/parallel branches, but then only one of them gets skipped, it does not seem trivial to figure out which is the "right" one.

Perhaps the code could be adjusted to check the current heads and guess which one is the right one, but I think that is still problematic. For example if you use matrix and have two identically named stages/parallel branches, but then only one of them gets skipped, it does not seem trivial to figure out which is the "right" one.

Maybe CpsThread.head could be used to avoid this problem, but I am not sure, particularly given its Javadoc (and it is package private, but that would not stop Groovy here).

Without an initial FlowNode to guide the scan

I see, this is the crux of the problem: there actually is no obvious Java/Groovy to call that would return usable information at this point. CpsThread.current().etc() would be tricky.

timja added the bug label Mar 11, 2024

timja requested a review from dwnusbaum March 11, 2024 19:34

timja marked this pull request as draft March 11, 2024 19:35

timja commented Mar 11, 2024

View reviewed changes

...on/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/ModelInterpreter.groovy Show resolved Hide resolved

JENKINS-72841 - Pass flow node ID when skipping stage

f16fa77

timja force-pushed the JENKINS-72841-use-step-context branch from f6c915b to f16fa77 Compare March 11, 2024 20:18

timja commented Mar 11, 2024

View reviewed changes

...model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy Show resolved Hide resolved

timja marked this pull request as ready for review March 11, 2024 20:23

timja requested review from dwnusbaum and removed request for dwnusbaum March 11, 2024 20:23

timja mentioned this pull request Mar 11, 2024

[JENKINS-72841] Add TagsAction on all relevant stages #698

Closed

timja marked this pull request as draft March 11, 2024 20:57

Handle parallel enclosing block

a97ad08

timja mentioned this pull request Mar 11, 2024

Parallel skipped stage from declarative not rendered quite right (blue circle is added) jenkinsci/pipeline-graph-view-plugin#358

Closed

dwnusbaum reviewed Mar 11, 2024

View reviewed changes

...model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/Utils.groovy Outdated Show resolved Hide resolved

timja added 2 commits March 11, 2024 22:44

Adjust test for new flow node

68d2cbb

Simplify

5027164

timja requested a review from dwnusbaum March 11, 2024 22:52

timja marked this pull request as ready for review March 11, 2024 22:53

timja requested a review from a team March 12, 2024 08:42

timja mentioned this pull request Mar 13, 2024

Pipeline overview shows wrong times and wrong color jenkinsci/pipeline-graph-view-plugin#362

Closed

timja requested a review from joseblas March 15, 2024 11:10

timja requested review from a team and removed request for a team March 24, 2024 14:31

Merge branch 'master' into JENKINS-72841-use-step-context

c53f774

timja mentioned this pull request Mar 24, 2024

Some skipped steps are wrongly displayed as in progress jenkinsci/pipeline-graph-view-plugin#220

Closed

dwnusbaum mentioned this pull request Apr 9, 2024

Test jenkinsci/pipeline-model-definition#700 jenkinsci/bom#3102

Closed

dwnusbaum reviewed Apr 9, 2024

View reviewed changes

dwnusbaum and others added 3 commits April 9, 2024 10:15

Merge branch 'master' into JENKINS-72841-use-step-context

13c481a

Update pipeline-model-definition/src/main/groovy/org/jenkinsci/plugin…

0ed4438

…s/pipeline/modeldefinition/Utils.groovy Co-authored-by: Devin Nusbaum <[email protected]>

Make test assertion stronger

5eb4ac3

Co-authored-by: Devin Nusbaum <[email protected]>

timja requested a review from dwnusbaum April 9, 2024 15:57

dwnusbaum reviewed Apr 9, 2024

View reviewed changes

Only use getContext step when actually skipping a stage

bd14425

This should help reduce the number of tests that need to be adjusted to update FlowNode IDs.

timja mentioned this pull request Apr 10, 2024

Update pipeline-model-definition and fix related test jenkinsci/blueocean-plugin#2555

Merged

7 tasks

dwnusbaum approved these changes Apr 10, 2024

View reviewed changes

dwnusbaum merged commit 41dd8ef into jenkinsci:master Apr 10, 2024
15 checks passed

jglick reviewed Apr 11, 2024

View reviewed changes

timja deleted the JENKINS-72841-use-step-context branch April 11, 2024 14:34

alecharp mentioned this pull request Apr 25, 2024

Add Pipeline Graph View to the managed set jenkinsci/bom#3137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-72841] - Pass flow node ID when skipping stage #700

[JENKINS-72841] - Pass flow node ID when skipping stage #700

timja commented Mar 11, 2024

timja commented Mar 13, 2024

timja commented Mar 24, 2024

dwnusbaum Apr 9, 2024

dwnusbaum commented Apr 9, 2024

dwnusbaum Apr 9, 2024 •

edited

Loading

dwnusbaum Apr 9, 2024

timja Apr 10, 2024 •

edited

Loading

dwnusbaum Apr 10, 2024

timja Apr 10, 2024

basil Apr 11, 2024

timja Apr 11, 2024

timja Apr 11, 2024

timja Apr 12, 2024

dwnusbaum commented Apr 9, 2024

dwnusbaum commented Apr 9, 2024 •

edited

Loading

dwnusbaum left a comment

jglick Apr 11, 2024

dwnusbaum Apr 11, 2024

dwnusbaum Apr 11, 2024 •

edited

Loading

jglick Apr 11, 2024

[JENKINS-72841] - Pass flow node ID when skipping stage #700

[JENKINS-72841] - Pass flow node ID when skipping stage #700

Conversation

timja commented Mar 11, 2024

timja commented Mar 13, 2024

timja commented Mar 24, 2024

Choose a reason for hiding this comment

dwnusbaum commented Apr 9, 2024

dwnusbaum Apr 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timja Apr 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum commented Apr 9, 2024

dwnusbaum commented Apr 9, 2024 • edited Loading

dwnusbaum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Apr 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dwnusbaum Apr 9, 2024 •

edited

Loading

timja Apr 10, 2024 •

edited

Loading

dwnusbaum commented Apr 9, 2024 •

edited

Loading

dwnusbaum Apr 11, 2024 •

edited

Loading