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

Error during always block skips execution of remaining steps

XMLWordPrintable

      Here's a trivial example of a general problem:

      pipeline {
    agent any
    stages {
        stage ("Example") {
            steps {
                // Install dependencies
                sh 'npm install'
                echo 'Execute build that creates artifacts'
                echo 'Execute step that fails to create the expected junit test file'
            }
        }
    }
    post {
        always {
            junit 'reports/**'
            echo 'This step and everything after it won't run.'
            archive '**'
            echo "send email to people with final build status"
        }
    }
}

      I have an always block, but I can't actually guarantee that all steps in that always block will all always attempt to run.

      In Scripted Pipeline I could wrap each of these in a try-catch, but I can't do that in Declarative.

      I can reorder these steps to put the ones that I know won't fail earlier, but what about the email step? I want that to be last, so it gets the proper build status in the email.

      abayer suggested a catchError block. But depending on which step fails, that doesn't get the right behavior either. I want each step in the always to run exactly once no matter the build status. If the second step fails the catchError can't tell which one failed and not to run the first step again.

      Logically what I would expect to work:

      pipeline {
    agent any
    stages {
        stage ("Example") {
            steps {
                // Install dependencies
                sh 'npm install'
                echo 'Execute build that creates artifacts'
                echo 'Execute step that fails to create the expected junit test file'
            }
        }
    }
    post {
        always {
            junit 'reports/**'
        }
        always {
            echo 'This step and everything after it won't run.'
        }
        always {
            archive '**
        }
        always {
            echo "send email to people with final build status"
        }
    }
}

      However, I'm not allowed to have more than one always block.

        1. Screen Shot 2017-01-25 at 12.32.41 PM.png
          Screen Shot 2017-01-25 at 12.32.41 PM.png
          121 kB

          [JENKINS-41239] Error during always block skips execution of remaining steps

          Andrew Bayer added a comment -

          So this is an interesting question. The current architecture is such that we can't actually break down each step and execute them individually, so we can't guarantee execution of all the steps. Changing this would be nontrivial at best. I don't think there's a plausible chance of addressing this in the 1.0 timeframe, and even post-1.0, no guarantees - I can go into detail on the problems with that if you'd like.

          Andrew Bayer added a comment - So this is an interesting question. The current architecture is such that we can't actually break down each step and execute them individually, so we can't guarantee execution of all the steps. Changing this would be nontrivial at best. I don't think there's a plausible chance of addressing this in the 1.0 timeframe, and even post-1.0, no guarantees - I can go into detail on the problems with that if you'd like.

          Ryan Campbell added a comment -

          The idea of multiple always blocks makes sense, but this definitely feels like an RFE.

          Ryan Campbell added a comment - The idea of multiple always blocks makes sense, but this definitely feels like an RFE.

          Andrew Bayer added a comment -

          Adding a bit more detail - we capture whole closure blocks of steps at the beginning of the build, when we process the Jenkinsfile's pipeline block, so that we can defer execution of those blocks 'til later in the run. If we wanted to capture each step individually (as would be needed to guarantee each of the steps in a block were executed), we'd have to somehow record each step, its arguments, any nested steps, any steps or functions that get passed as arguments, etc without actually evaluating any of it until later. That's theoretically possible, but unless I'm missing something obvious, glaringly non-trivial. Closures represent far and away the easiest way to defer execution of a chunk of steps until later.

          So that brings up the question of multiple always (or any other condition type) blocks - that would also require some fairly significant changes, since post is actually represented as a map behind the scenes - we'd have to change that to be a list of condition name/step block pairs, and then iterate over them in the condition execution order, i.e., first all the always in the order they're specified in the Jenkinsfile, then all the changed, etc...it could be done, but it would take some work and I'm not entirely comfortable with that change in the first place.

          Either way - this is definitely not going to be addressed in 1.0.

          Andrew Bayer added a comment - Adding a bit more detail - we capture whole closure blocks of steps at the beginning of the build, when we process the Jenkinsfile's pipeline block, so that we can defer execution of those blocks 'til later in the run. If we wanted to capture each step individually (as would be needed to guarantee each of the steps in a block were executed), we'd have to somehow record each step, its arguments, any nested steps, any steps or functions that get passed as arguments, etc without actually evaluating any of it until later. That's theoretically possible, but unless I'm missing something obvious, glaringly non-trivial. Closures represent far and away the easiest way to defer execution of a chunk of steps until later. So that brings up the question of multiple always (or any other condition type) blocks - that would also require some fairly significant changes, since post is actually represented as a map behind the scenes - we'd have to change that to be a list of condition name/step block pairs, and then iterate over them in the condition execution order, i.e., first all the always in the order they're specified in the Jenkinsfile, then all the changed , etc...it could be done, but it would take some work and I'm not entirely comfortable with that change in the first place. Either way - this is definitely not going to be addressed in 1.0.

          Sam Van Oort added a comment -

          I think this sort of RFE is a good litmus test for when a user needs to shift from declarative to full pipeline, because this level of complexity breaks from the goals of declarative. We want simplicity, straightforward use that applies best practices, and are aiming to cover the 80% case rather than 100% of possibilities.

          This is equivalent to doing a try-catch within a finally clause. Even in Java or native pipeline groovy code, this is an iffy practice at best and should be avoided whenever possible.

          On these grounds, would suggest we classify this as "won't fix" or maybe even "can't fix."

          Sam Van Oort added a comment - I think this sort of RFE is a good litmus test for when a user needs to shift from declarative to full pipeline, because this level of complexity breaks from the goals of declarative. We want simplicity, straightforward use that applies best practices, and are aiming to cover the 80% case rather than 100% of possibilities. This is equivalent to doing a try-catch within a finally clause. Even in Java or native pipeline groovy code, this is an iffy practice at best and should be avoided whenever possible. On these grounds, would suggest we classify this as "won't fix" or maybe even "can't fix."

          Sam Van Oort added a comment -

          Not trying to say it's a ridiculous idea, just that we have to draw a line in the sand somewhere, or declarative will end up just being native pipeline with an extra layer of abstraction added.

          Sam Van Oort added a comment - Not trying to say it's a ridiculous idea, just that we have to draw a line in the sand somewhere, or declarative will end up just being native pipeline with an extra layer of abstraction added.

          Liam Newman added a comment -

          abayer
          I disagree. This is exactly kind of thing that we do not want going into 1.0 because it will be hard to change later. Also, it will encourage users to build their own hacks to work around this. We should do the right thing now.

          I understand (basically) why this problematic but it a major issue. It first appeared in Scripted but could be worked around. I'm not sure how we address it in Declarative, but we absolutely need to.

          At core, this is a regression from even Freestyle behavior.

          Here's a couple publishers at the end of a Freestyle job:

          When this runs, both of these fail (I have no step in that job, there's nothing to work with). But they both run, even tho the archive step fails. 

          Building remotely on osx_mbp (highmem) in workspace /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux
Archiving artifacts
ERROR: No artifacts found that match the file pattern "test/**". Configuration error?
ERROR: ‘test/**’ doesn’t match anything, but ‘**’ does. Perhaps that’s what you mean?
Build step 'Archive the artifacts' changed build result to FAILURE
[htmlpublisher] Archiving HTML reports...
[htmlpublisher] Archiving at PROJECT level /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux to /var/jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report
ERROR: Directory '/Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux' exists but failed copying to '/var/jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report'.
Finished: FAILURE

          My assertion is that the the

          {post} block in Declarative is an improved version of publishers.

          Users are going to take their Freestyle jobs, try to turn them into Declarative. They see the {always} block and treat it like their beloved publishers (which Scripted didn't do well - {try-catch} hell). However, they will eventually encounter a rude gotcha - when one step in their lone {always} block fails the rest don't run.

          From a Groovy execution standpoint, I understand why the later steps don't run. There are cases where I would want this behavior, some set of steps that I want to always do at the end of the job but if one of those fails don't do the others. I'm not sure which is the more common, but we need to support both or people will do stupid things trying to do the one we don't support.

          Figuring out how to wrap all each method call in the post sections with error handlers sounds like a little slice of hell, likely a bug farm, and also confusing since it doesn't be have at all like script.

          Allowing multiple blocks of the same in in {post}

          seems the cleanest way to do this. It's logical from a scripting standpoint, but also clear from a declarative standpoint. The downside is we're basically going end up with the second example in the description being the normal way to do things for most users, and that's not great.

          On the other hand, really what most users would probably want would be more like: 

              post {
        always {
            junit 'reports/**'
            echo 'This step and everything after it in this block won't run.'
            archive '**
        }

        always { echo "send email to people with final build status" }
        always { echo "send hipchat to people with final build status" }
    }

          Some parts they want always-always, some they're okay if they do bail.

          Liam Newman added a comment - abayer I disagree. This is exactly kind of thing that we do not want going into 1.0 because it will be hard to change later. Also, it will encourage users to build their own hacks to work around this. We should do the right thing now. I understand (basically) why this problematic but it a major issue. It first appeared in Scripted but could be worked around. I'm not sure how we address it in Declarative, but we absolutely need to. At core, this is a regression from even Freestyle behavior. Here's a couple publishers at the end of a Freestyle job: When this runs, both of these fail (I have no step in that job, there's nothing to work with). But they both run , even tho the archive step fails. Building remotely on osx_mbp (highmem) in workspace /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux Archiving artifacts ERROR: No artifacts found that match the file pattern "test/**" . Configuration error? ERROR: ‘test/**’ doesn’t match anything, but ‘**’ does. Perhaps that’s what you mean? Build step 'Archive the artifacts' changed build result to FAILURE [htmlpublisher] Archiving HTML reports... [htmlpublisher] Archiving at PROJECT level /Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux to / var /jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report ERROR: Directory '/Users/bitwiseman/jenkins/agents/osx_mbp/workspace/full-build-linux' exists but failed copying to '/ var /jenkins_home/jobs/full-build-linux/htmlreports/HTML_Report' . Finished: FAILURE My assertion is that the the {post} block in Declarative is an improved version of publishers. Users are going to take their Freestyle jobs, try to turn them into Declarative. They see the {always} block and treat it like their beloved publishers (which Scripted didn't do well - {try-catch} hell). However, they will eventually encounter a rude gotcha - when one step in their lone {always} block fails the rest don't run. From a Groovy execution standpoint, I understand why the later steps don't run. There are cases where I would want this behavior, some set of steps that I want to always do at the end of the job but if one of those fails don't do the others. I'm not sure which is the more common, but we need to support both or people will do stupid things trying to do the one we don't support. Figuring out how to wrap all each method call in the post sections with error handlers sounds like a little slice of hell, likely a bug farm, and also confusing since it doesn't be have at all like script. Allowing multiple blocks of the same in in {post} seems the cleanest way to do this. It's logical from a scripting standpoint, but also clear from a declarative standpoint. The downside is we're basically going end up with the second example in the description being the normal way to do things for most users, and that's not great. On the other hand, really what most users would probably want would be more like: post { always { junit 'reports/**' echo 'This step and everything after it in this block won' t run.' archive '** } always { echo "send email to people with final build status" } always { echo "send hipchat to people with final build status" } } Some parts they want always-always, some they're okay if they do bail.

          Andrew Bayer added a comment - - edited

          1.0 is closed for anything but showstopper bugs - so regardless, this isn't going to be addressed in 1.0. And I still disagree on the premise - I don't think there's anything unreasonable about a user discovering that steps in a block after a failed step didn't execute and therefore changing their ordering around in the future. And yes, this is a difference from freestyle behavior - I'm fine with that.

          Andrew Bayer added a comment - - edited 1.0 is closed for anything but showstopper bugs - so regardless, this isn't going to be addressed in 1.0. And I still disagree on the premise - I don't think there's anything unreasonable about a user discovering that steps in a block after a failed step didn't execute and therefore changing their ordering around in the future. And yes, this is a difference from freestyle behavior - I'm fine with that.

          Patrick Wolf added a comment -

          We can look at this after 1.0. IMO, this is more of an RFE than a bug and not a showstopper.

          I'm not a big fan of multiple always blocks but that might be the easiest way to let users decide what should happen. Otherwise, if I put several dependent things in an always block and the first one fails I could cause more damage but running the remaining steps.

          The other big advantage here is the ability to these steps on the post block of individual stages. You can do archive '**' on the success block of a specific stage (why archive if the build fails) and also capture the final failure if the archive or something else downstream fails.

          Patrick Wolf added a comment - We can look at this after 1.0. IMO, this is more of an RFE than a bug and not a showstopper. I'm not a big fan of multiple always blocks but that might be the easiest way to let users decide what should happen. Otherwise, if I put several dependent things in an always block and the first one fails I could cause more damage but running the remaining steps. The other big advantage here is the ability to these steps on the post block of individual stages. You can do archive '**' on the success block of a specific stage (why archive if the build fails) and also capture the final failure if the archive or something else downstream fails.

          Liam Newman added a comment - - edited

          Oh, stages can have post steps. I forgot about that. That gives a workaround and also some guidance about structure.

          It might go something like this:

          pipeline {
    agent any
    stages {
        stage ("Prepare Dependencies") {
            steps {
                // Install dependencies
                sh 'npm install'
            }
        }
        stage ("Build") {
            steps {
                echo 'Execute build that creates artifacts'
            }
            post {
               success {
                    archive 'output/**'
                }
               always {
                    archive 'logs/**'
                }
            }
        }
        stage ("Test") {
            steps {
                echo 'Execute build that creates artifacts'
            }
            post {
               always {
                    junit 'reports/**'
                }
            }
        }
    }
    post {
        always {
            echo "send email to people with final build status"
        }
    }
}

          Cool, I can live with this.

          Liam Newman added a comment - - edited Oh, stages can have post steps. I forgot about that. That gives a workaround and also some guidance about structure. It might go something like this: pipeline { agent any stages { stage ( "Prepare Dependencies" ) { steps { // Install dependencies sh 'npm install' } } stage ( "Build" ) { steps { echo 'Execute build that creates artifacts' } post { success { archive 'output/**' } always { archive 'logs/**' } } } stage ( "Test" ) { steps { echo 'Execute build that creates artifacts' } post { always { junit 'reports/**' } } } } post { always { echo "send email to people with final build status" } } } Cool, I can live with this.

          Moritz Baumann added a comment -

          The workaround mentioned here doesn't work for me. Are there any plans to address this issue?

          Currently, I have to use 

          post { always { catchError { ... } cleanWs() } }

          everywhere to make sure I always clean up my workspaces. This workaround introduces another bug, though: Incorrect archiveArtifacts configurations in the catchError block will be completely ignored.

          Moritz Baumann added a comment - The workaround mentioned here doesn't work for me. Are there any plans to address this issue? Currently, I have to use post { always { catchError { ... } cleanWs() } } everywhere to make sure I always clean up my workspaces. This workaround introduces another bug, though: Incorrect archiveArtifacts configurations in the catchError block will be completely ignored.

          Moritz Baumann added a comment - - edited

          When I discovered this problem, my first instinct was to add a second always closure, but Jenkins didn't accept that. Maybe allowing this would be a good solution to the underlying problem?

          Edit: Sorry, completely missed that this is already mentioned above.

          Moritz Baumann added a comment - - edited When I discovered this problem, my first instinct was to add a second always closure, but Jenkins didn't accept that. Maybe allowing this would be a good solution to the underlying problem? Edit : Sorry, completely missed that this is already mentioned above.

          Andrew Bayer added a comment -

          I've got a PR up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/258 that adds a new post condition, currently called cleanup, which runs regardless of build status but after all the other conditions have resolved.

          Andrew Bayer added a comment - I've got a PR up at https://github.com/jenkinsci/pipeline-model-definition-plugin/pull/258 that adds a new post condition, currently called cleanup , which runs regardless of build status but after all the other conditions have resolved.

          Moritz Baumann added a comment -

          abayer thank you very much! While it doesn't fix the original issue, it very much fixes my use case. Especially the fact that `always` ran before `success` was giving me headaches.

          Moritz Baumann added a comment - abayer thank you very much! While it doesn't fix the original issue, it very much fixes my use case. Especially the fact that `always` ran before `success` was giving me headaches.

          SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Andrew Bayer
          Path:
          pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Cleanup.groovy
          pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Messages.properties
          pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BuildConditionResponderTest.java
          pipeline-model-definition/src/test/resources/postChecksAllConditions.groovy
          http://jenkins-ci.org/commit/pipeline-model-definition-plugin/83abd0ec35960c1f2a37b6a66b2d26385b2962e2
          Log:
          [FIXED JENKINS-41239] Add new cleanup post condition

          The new `cleanup` condition will always run regardless of build
          status, like `always`, but runs after all other `post` conditions
          have been evaluated, rather than before.

          I wanted to call this `finally` but that actually breaks Groovy
          parsing because `finally` is a reserved word. Dang. So for now, I'm
          calling it `cleanup`, but am open to suggestions.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: pipeline-model-definition/src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Cleanup.groovy pipeline-model-definition/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/model/conditions/Messages.properties pipeline-model-definition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/BuildConditionResponderTest.java pipeline-model-definition/src/test/resources/postChecksAllConditions.groovy http://jenkins-ci.org/commit/pipeline-model-definition-plugin/83abd0ec35960c1f2a37b6a66b2d26385b2962e2 Log: [FIXED JENKINS-41239] Add new cleanup post condition The new `cleanup` condition will always run regardless of build status, like `always`, but runs after all other `post` conditions have been evaluated, rather than before. I wanted to call this `finally` but that actually breaks Groovy parsing because `finally` is a reserved word. Dang. So for now, I'm calling it `cleanup`, but am open to suggestions.

          Andrew Bayer added a comment -

          Merged, releasing later today in Declarative 1.2.9

          Andrew Bayer added a comment - Merged, releasing later today in Declarative 1.2.9

          SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: Andrew Bayer
          Path:
          content/doc/book/pipeline/syntax.adoc
          http://jenkins-ci.org/commit/jenkins.io/c8c7bce2e7c5955d5176f0791800156cf0c087a2
          Log:
          JENKINS-41239 Add doc for new cleanup post condition.

          This was just released in Declarative 1.2.9 so this should be ready to
          go whenever works for you.

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: Andrew Bayer Path: content/doc/book/pipeline/syntax.adoc http://jenkins-ci.org/commit/jenkins.io/c8c7bce2e7c5955d5176f0791800156cf0c087a2 Log: JENKINS-41239 Add doc for new cleanup post condition. This was just released in Declarative 1.2.9 so this should be ready to go whenever works for you.

          SCM/JIRA link daemon added a comment -

          Code changed in jenkins
          User: R. Tyler Croy
          Path:
          content/doc/book/pipeline/syntax.adoc
          http://jenkins-ci.org/commit/jenkins.io/5c434e826c79dc06f837a6d244c9ddf5df1fa382
          Log:
          Merge pull request #1504 from abayer/jenkins-41239

          JENKINS-41239 Add doc for new cleanup post condition.

          Compare: https://github.com/jenkins-infra/jenkins.io/compare/92e468e7f66b...5c434e826c79

          SCM/JIRA link daemon added a comment - Code changed in jenkins User: R. Tyler Croy Path: content/doc/book/pipeline/syntax.adoc http://jenkins-ci.org/commit/jenkins.io/5c434e826c79dc06f837a6d244c9ddf5df1fa382 Log: Merge pull request #1504 from abayer/jenkins-41239 JENKINS-41239 Add doc for new cleanup post condition. Compare: https://github.com/jenkins-infra/jenkins.io/compare/92e468e7f66b...5c434e826c79

          Liam Newman added a comment -

          Bulk closing resolved issues.

          Liam Newman added a comment - Bulk closing resolved issues.

            abayer Andrew Bayer
            bitwiseman Liam Newman
            Votes:
            1 Vote for this issue
            Watchers:
            8 Start watching this issue

              Created:
              Updated:
              Resolved: