Uploaded image for project: 'Jenkins'
  1. Jenkins
  2. JENKINS-38223

FlowNode.isRunning is not very useful

    XMLWordPrintable

Details

    Description

      Give following pipeline script, b1 branch finishes immediately and b2 is not completed yet (waiting for input). I determine branch's completion from a branch's end FlowNode and calling FlowNode.isRunning() returns true instead false as its already completed. This makes BlueOcean API returns b1 as running whereas its already been completed.

      node {
          stage("hey") {
              echo "hello from hey"
          }
          
          stage("par") {
              parallel (
                  "b1" : {
                      echo 'hello from b1'
                  },
      
                  "b2" : {
                      def branchInput = input message: 'Please input branch to test against', parameters: [[$class: 'StringParameterDefinition', defaultValue: 'master', description: '', name: 'branch']]
                      echo "BRANCH NAME: ${branchInput}"
                  }             
              )            
          }
             
          stage("ho") {
              echo "hello from ho"
          }
          
      }
      

      Attachments

        Issue Links

          Activity

            vivek Vivek Pandey created issue -
            jglick Jesse Glick added a comment -

            Javadoc does claim

            be false for something that has finished but is pending child node creation, such as a completed fork branch which is waiting for the join node to be created

            but I can see no way that could ever have been implemented, since the BlockEndNode for parallel is not constructed until after all branches have completed. So I guess the Javadoc needs to be edited to stop making that claim.

            The use case you cite makes no sense. If the branch has a BlockEndNode, then by definition it has completed.

            jglick Jesse Glick added a comment - Javadoc does claim be false for something that has finished but is pending child node creation, such as a completed fork branch which is waiting for the join node to be created but I can see no way that could ever have been implemented, since the BlockEndNode for parallel is not constructed until after all branches have completed. So I guess the Javadoc needs to be edited to stop making that claim. The use case you cite makes no sense. If the branch has a BlockEndNode , then by definition it has completed.
            vivek Vivek Pandey added a comment -

            jglick

            >but I can see no way that could ever have been implemented, since the BlockEndNode for parallel is not constructed until after all branches have completed. So I guess the Javadoc needs to be edited to stop making that claim.

            Hmm, thats the bug, I have BlockEndNode for that parallel branch, very fact that its been already constructed then it should be returning false for isRunning(), no? I am not calling isRunning() on a step inside parallel branch, its on the BlockEndNode of that branch.

            >The use case you cite makes no sense. If the branch has a BlockEndNode, then by definition it has completed.

            Rather than asking caller to infer that all has ended since you have this end block, which is true but 'isRunning() API' makes no sense to return true in this case as well.

            vivek Vivek Pandey added a comment - jglick >but I can see no way that could ever have been implemented, since the BlockEndNode for parallel is not constructed until after all branches have completed. So I guess the Javadoc needs to be edited to stop making that claim. Hmm, thats the bug, I have BlockEndNode for that parallel branch, very fact that its been already constructed then it should be returning false for isRunning(), no? I am not calling isRunning() on a step inside parallel branch, its on the BlockEndNode of that branch. >The use case you cite makes no sense. If the branch has a BlockEndNode, then by definition it has completed. Rather than asking caller to infer that all has ended since you have this end block, which is true but 'isRunning() API' makes no sense to return true in this case as well.
            svanoort Sam Van Oort added a comment -

            > Rather than asking caller to infer that all has ended since you have this end block, which is true but 'isRunning() API' makes no sense to return true in this case as well.

            Yeah, it's a clear bug in workflow-cps here. I think fixable, by looking more closely at cases with > 1 currentHead (and then seeing if it's the end of one of the currently executing branch). May be something I can fix as a small one-off fix (probably needs a change in workflow-api and workflow-cps though, due to spec).

            In pipeline-graph-analysis I use a bit of a deeper inspection to work around this, by looking to see if the node is the BlockEndNode for a BlockStartNode which is a parallel branch start.

            svanoort Sam Van Oort added a comment - > Rather than asking caller to infer that all has ended since you have this end block, which is true but 'isRunning() API' makes no sense to return true in this case as well. Yeah, it's a clear bug in workflow-cps here. I think fixable, by looking more closely at cases with > 1 currentHead (and then seeing if it's the end of one of the currently executing branch). May be something I can fix as a small one-off fix (probably needs a change in workflow-api and workflow-cps though, due to spec). In pipeline-graph-analysis I use a bit of a deeper inspection to work around this, by looking to see if the node is the BlockEndNode for a BlockStartNode which is a parallel branch start.
            michaelneale Michael Neale added a comment -

            vivek so is this fix needed or is it more a temporary thing for the current blue ocean code and should go away when bismuth is used instead?

            michaelneale Michael Neale added a comment - vivek so is this fix needed or is it more a temporary thing for the current blue ocean code and should go away when bismuth is used instead?
            svanoort Sam Van Oort added a comment -

            michaelneale Bismuth doesn't suffer this issue, but it has a kind of filthy hack in place for handling parallels unfortunately (I'd like to be able to drop that).

            svanoort Sam Van Oort added a comment - michaelneale Bismuth doesn't suffer this issue, but it has a kind of filthy hack in place for handling parallels unfortunately (I'd like to be able to drop that).
            michaelneale Michael Neale made changes -
            Field Original Value New Value
            Link This issue blocks JENKINS-38398 [ JENKINS-38398 ]
            jamesdumay James Dumay made changes -
            Link This issue blocks JENKINS-38398 [ JENKINS-38398 ]
            jamesdumay James Dumay made changes -
            Link This issue relates to JENKINS-38398 [ JENKINS-38398 ]
            jglick Jesse Glick added a comment -

            I still think the method is behaving according to spec—it is just not a useful spec. When the branch has completed, a BlockEndNode is created and becomes one of the heads of the flow graph. Thus it isRunning() according to the definition.

            The spec will not be changed (that would not be compatible), so the most that would be done is to deprecate the method, and possibly introduce a new method with a different semantics: example

            jglick Jesse Glick added a comment - I still think the method is behaving according to spec—it is just not a useful spec. When the branch has completed, a BlockEndNode is created and becomes one of the heads of the flow graph. Thus it isRunning() according to the definition. The spec will not be changed (that would not be compatible), so the most that would be done is to deprecate the method, and possibly introduce a new method with a different semantics: example
            jglick Jesse Glick made changes -
            Summary Incorrect state of completed parallel branch FlowNode.isRunning is not very useful
            jglick Jesse Glick made changes -
            Link This issue is related to JENKINS-26139 [ JENKINS-26139 ]
            michaelneale Michael Neale added a comment -

            Agree Jesse - if that is the spec, that is the spec. Perhaps a clearer name would help, but this is much needed.

            michaelneale Michael Neale added a comment - Agree Jesse - if that is the spec, that is the spec. Perhaps a clearer name would help, but this is much needed.
            jglick Jesse Glick made changes -
            Link This issue is blocking JENKINS-40934 [ JENKINS-40934 ]
            jglick Jesse Glick made changes -
            Status Open [ 1 ] In Progress [ 3 ]
            jglick Jesse Glick made changes -
            Link This issue relates to JENKINS-45553 [ JENKINS-45553 ]
            jglick Jesse Glick made changes -
            Remote Link This issue links to "PR 45 (Web Link)" [ 17348 ]
            jglick Jesse Glick made changes -
            Status In Progress [ 3 ] In Review [ 10005 ]

            Code changed in jenkins
            User: Jesse Glick
            Path:
            src/main/java/org/jenkinsci/plugins/workflow/graph/FlowNode.java
            http://jenkins-ci.org/commit/workflow-api-plugin/5deffd7f6f73638576ddcacc2890e8dfd3438408
            Log:
            JENKINS-38223 Introduced FlowNode.isActive.

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: src/main/java/org/jenkinsci/plugins/workflow/graph/FlowNode.java http://jenkins-ci.org/commit/workflow-api-plugin/5deffd7f6f73638576ddcacc2890e8dfd3438408 Log: JENKINS-38223 Introduced FlowNode.isActive.

            Code changed in jenkins
            User: Sam Van Oort
            Path:
            Jenkinsfile
            pom.xml
            src/main/java/org/jenkinsci/plugins/workflow/graph/FlowNode.java
            src/test/java/org/jenkinsci/plugins/workflow/graph/FlowNodeTest.java
            http://jenkins-ci.org/commit/workflow-api-plugin/63e8ad0c271573f4bebc57fb0776b3fac4fccea9
            Log:
            Merge pull request #45 from jglick/FlowNode.isActive-JENKINS-38223

            JENKINS-38223 Introduced FlowNode.isActive

            Compare: https://github.com/jenkinsci/workflow-api-plugin/compare/55979f5e128b...63e8ad0c2715

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Sam Van Oort Path: Jenkinsfile pom.xml src/main/java/org/jenkinsci/plugins/workflow/graph/FlowNode.java src/test/java/org/jenkinsci/plugins/workflow/graph/FlowNodeTest.java http://jenkins-ci.org/commit/workflow-api-plugin/63e8ad0c271573f4bebc57fb0776b3fac4fccea9 Log: Merge pull request #45 from jglick/FlowNode.isActive- JENKINS-38223 JENKINS-38223 Introduced FlowNode.isActive Compare: https://github.com/jenkinsci/workflow-api-plugin/compare/55979f5e128b...63e8ad0c2715

            Code changed in jenkins
            User: Jesse Glick
            Path:
            pom.xml
            src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java
            http://jenkins-ci.org/commit/workflow-support-plugin/d78f301c4630ad3d9cbd408494ed1ad36e97b132
            Log:
            JENKINS-38223 Using FlowNode.isActive to eliminate the main overhead in JENKINS-45553.

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Jesse Glick Path: pom.xml src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java http://jenkins-ci.org/commit/workflow-support-plugin/d78f301c4630ad3d9cbd408494ed1ad36e97b132 Log: JENKINS-38223 Using FlowNode.isActive to eliminate the main overhead in JENKINS-45553 .

            Code changed in jenkins
            User: Sam Van Oort
            Path:
            pom.xml
            src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java
            src/main/java/org/jenkinsci/plugins/workflow/support/visualization/table/FlowGraphTable.java
            src/test/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImplTest.java
            http://jenkins-ci.org/commit/workflow-support-plugin/236f06ca40fc019ca4da2cadb4d0804971faa9db
            Log:
            Merge pull request #38 from jglick/FlowNode.isActive-JENKINS-38223

            JENKINS-38223 Using FlowNode.isActive to improve JENKINS-45553

            Compare: https://github.com/jenkinsci/workflow-support-plugin/compare/01f9538af15c...236f06ca40fc

            scm_issue_link SCM/JIRA link daemon added a comment - Code changed in jenkins User: Sam Van Oort Path: pom.xml src/main/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImpl.java src/main/java/org/jenkinsci/plugins/workflow/support/visualization/table/FlowGraphTable.java src/test/java/org/jenkinsci/plugins/workflow/support/actions/LogActionImplTest.java http://jenkins-ci.org/commit/workflow-support-plugin/236f06ca40fc019ca4da2cadb4d0804971faa9db Log: Merge pull request #38 from jglick/FlowNode.isActive- JENKINS-38223 JENKINS-38223 Using FlowNode.isActive to improve JENKINS-45553 Compare: https://github.com/jenkinsci/workflow-support-plugin/compare/01f9538af15c...236f06ca40fc
            svanoort Sam Van Oort added a comment -

            Review finished and released with workflow-api 2.22

            svanoort Sam Van Oort added a comment - Review finished and released with workflow-api 2.22
            svanoort Sam Van Oort made changes -
            Resolution Fixed [ 1 ]
            Status In Review [ 10005 ] Closed [ 6 ]

            People

              jglick Jesse Glick
              vivek Vivek Pandey
              Votes:
              0 Vote for this issue
              Watchers:
              7 Start watching this issue

              Dates

                Created:
                Updated:
                Resolved: