Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FIXED JENKINS-46809] Add sequential groups of parallel stages #227

Merged
merged 8 commits into from Jun 13, 2018

Conversation

@abayer
Copy link
Member

commented Nov 29, 2017

SYNTAX

stages {
    stage('first-solo') { ... }
    stage('parent') {
        parallel {
            // Order in the parallel block is not significant.
            stage('single-stage') { ... }
            
            stage('multiple-stages') {
                stages {
                    // Order inside a group's stages is significant - we will execute the stages in the order 
                    // they're specified here.
                    stage('first-sequential-stage') { ... }
                    stage('second-sequential-stage') { ... }
                }
            }

            stage('other-single-stage') { ... }

            // etc - arbitrarily long list of either "stage" or "group"
        }
    }
    stage('second-solo') { ... }
}

RUNTIME "DIAGRAM"
Based on the above:

  • stage 'first-solo' runs alone
  • stage 'parent' starts after 'first-solo' finishes ->
    • runs the following branches in parallel ->
      • branch 'single-stage' ->
        • stage 'single-stage'
      • branch 'multiple-stages' ->
        • stage 'multiple-stages' (yes, there's a stage step here now - runs its multiple stages sequentially) ->
          • stage 'first-sequential-stage'
          • stage 'second-sequential-stage' runs after 'first-sequential-stage' finishes
      • branch 'other-single-stage' ->
        • stage 'other-single-stage'
  • stage 'second-solo' runs after 'parent' (and all its nested stages) finishes

TESTS TODO

  • Compatibility tests for JSON containing parallel from before this change - should still translate and work the same. (get for free with pre-existing JSON/parallel tests!)
  • Compatibility tests for Groovy containing parallel from before this change - get for free as well. Hooray!
  • JSON<->Groovy conversion of new mixed parallel (i.e., stage and group) blocks.
  • Upgrade testing - what happens to a running build with old-school parallel definition, since that gets migrated behind the scenes? (EDIT: This will be fine - we haven't actually changed parallel, we've changed stage by adding more fields/functionality. So nothing needs to be tested here, and if it does go weird on upgrade while a build is running, well, we don't guarantee that for 1.x versions.)
  • Validate that nested stages within group can't have parallel within them.
  • Testing in Blue Ocean to make sure this doesn't Break All The Things visually in versions of Blue Ocean before visualization support for nested stage groups is added. (EDIT: It's not great, but it doesn't blow up horribly, so I'll roll with it.)
  • Verify that the new JSON representation is still valid (without any groups in the parallel) in the Pipeline Editor before it also is updated for nested stage groups. EDIT: This turns out to be a no-op. The JSON syntax if you don't include stages with nested stages is the same, so...yay?
  • post behavior for both parent stages containing parallel groups and for the sequential stages within the groups.
  • group level agent
  • group level tools
  • group level post
  • group level environment
  • group level when

OTHER TODO

  • Docs PR (jenkins-infra/jenkins.io#1274)
  • HTML help for group directive.
  • Do we want to add when for groups, so you can conditionally execute/not execute the whole group of stages with one when declaration while still executing any other groups or stages within the same parallel?
  • And that brings up the question of whether we should have an actual stage step execution for a group. I think that if we do when on groups, we should also nest a stage step directly within the parallel branch and run the sequential stages within that group stage, so that things like marking as skipped and the like actually work right.
  • Make group basically a special case of stage where steps and parallel aren't allowed, a list of stages is, and agent, tools, when, post, environment are all allowed. This will mean having an actual stage step execution for the group.
  • Move the list of stages inside group into a nested stages block now that we actually have other possible fields.
  • Validation (and testing thereof) for rejecting empty groups (i.e., without stages or with empty stages). (EDIT: Realized we get this for free in ModelASTStages validation. woo!)
  • Switch stage to be able to contain one and only one of steps, parallel, or stages - i.e., merge together stage and group.

@abayer abayer added this to the 1.3 milestone Nov 29, 2017

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Adding @bitwiseman for feedback as well.

@rsandell
Copy link
Member

left a comment

I also think it makes sense for a group to have when conditions, so You don't have to repeat the same conditions in every group's stage.

// If there's already a set of parallel stages defined, add that to the parallel content instead.
if (this.parallel != null) {
this.parallelContent.addAll(this.parallel.getStages());
this.parallel = null;

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 29, 2017

Member

So parallel should now be transient and @Deprecated?

This comment has been minimized.

Copy link
@abayer

abayer Nov 29, 2017

Author Member

Good point. getParallel() is deprecated already, and I've re-added setParallel(ModelASTStages) solely so I can deprecate it. =) Adding transient and @Deprecated on the field too.

This comment has been minimized.

Copy link
@rsandell

rsandell Dec 1, 2017

Member

It is very strange to add a method just to deprecate it :/

this.name = name
this.steps = steps
this.agent = agent
this.post = post
this.when = when
this.tools = tools
this.environment = environment
this.parallel = parallel

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 29, 2017

Member

I think the same goes for this one.

This comment has been minimized.

Copy link
@abayer

abayer Nov 29, 2017

Author Member

Gotcha.

* cannot be transformed.
*/
Expression transformParallelContent(@CheckForNull ModelASTStage original) {
if (isGroovyAST(original) && !original.parallelContent?.isEmpty()) {

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 29, 2017

Member

#210 was merged only 7 days ago :)
!original.parallelContent?.isEmpty() can be shortened to just !original.parallelContent iirc "Groovy truthiness" checks for both null and non empty for lists.
But I could be remembering wrongly ;)

This comment has been minimized.

Copy link
@abayer

abayer Nov 29, 2017

Author Member

boo, this was actually older code that I copy-pasted into an older iteration of this. Cleaning up!

* cannot be transformed.
*/
Expression transformParallelGroup(@CheckForNull ModelASTParallelStageGroup original) {
if (isGroovyAST(original) && !original.stages?.stages?.isEmpty()) {

This comment has been minimized.

Copy link
@rsandell

rsandell Nov 29, 2017

Member

same here

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

I also think it makes sense for a group to have when conditions, so You don't have to repeat the same conditions in every group's stage.

Interesting. Do we really want when as a possibility on all of the parent stage, the group, and the individual sequential stages? I'm not ruling it out, I'm just not sure it's worth the additional parsing/model pain. Hrm. I'll add a note to the TODO to decide that.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

cc @rpocase and @stephendonner - this all may be of interest to you as well. And I'm looking for any feedback I can get, so hey. =)

@rpocase

This comment has been minimized.

Copy link

commented Nov 29, 2017

👍 From a current syntax perspective. I don't love the name group, but I'm struggling with coming up with anything more fitting that isn't overly verbose.

Another possible TODO decision (that might depend on whens on group TODO) is if group should support post. I can think of a use case where I want something along the lines of

group('cluster1') {
    ...
    post { failure { rollBack() } }
}
group('cluster2') {
    ...
    post { failure { rollBack() } }
}

This could be handled at the sequential stage level, but I could imagine scenarios where having at the group level reduces repetition.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Upon further reflection, yeah. We should make group a special case of stage - no steps or parallel allowed in it, 1..n stages required, but agent, tools, environment, when, and post are all available.

Here's how I got there: first, I realized we needed to be able to have one agent for all the stages in a group - otherwise, you can't be sure you'll get the same actual agent and workspace, which could be relevant. And then I started thinking about my eventual dream of a matrix directive - and that wants not just agent but when and post too. So if we're going that far, hey, might as well just make stage and group inherit from a common ancestor that provides agent, tools, environment, when, post, and a name field. So that'll take some refactoring and fiddling with syntax, but I think it's the right thing to do.

@rpocase

This comment has been minimized.

Copy link

commented Nov 29, 2017

Sounds great! I was leaning towards group really feeling like a stage, but wasn't sure if the rest of the directives were too much.

There is still room for each stage contained within a group to require specific agents, though. Could group have an implied agent none if not present? It's a small enough amount of noise to not be terribly obstructive when I don't need a group level agent, but would be a nice convenience.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

Yeah, none of agent et al would be required on group.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

ping @cliffmeyers too from the Blue Ocean perspective.

.logContains(Messages.ModelValidatorImpl_NoNestedWithinNestedStages())
.go();
}

This comment has been minimized.

Copy link
@kshultzCB

kshultzCB Nov 29, 2017

Contributor

I'd love to see a similar test, which checks the ability to override/inherit agent, tools, environment, when, and post inside of a group.

This comment has been minimized.

Copy link
@abayer

abayer Nov 29, 2017

Author Member

See above in the TEST TODOs =)

This comment has been minimized.

Copy link
@kshultzCB

kshultzCB Nov 29, 2017

Contributor

Oh, right. That. :)

@rtyler

This comment has been minimized.

Copy link
Member

commented Nov 29, 2017

@rpocase @abayer how does the step name sequence strike you both instead of group? I think group is too ambiguous to accurately convey what's happening within the block.

@bitwiseman

This comment has been minimized.

Copy link
Contributor

commented Nov 29, 2017

@abayer
Would this syntax (with group directly under stages also be allowed?

stages {
    stage('Build') { ... }
    group('Release Train') {
        when { branch 'master' }
        stage("Confirm deploy to Staging") { ... }
        stage("Deploy to Staging") { ... }
        stage("Confirm deploy to Production") { ... }
        stage("Deploy to Production") { ... }
    }
}

@rtyler I mostly agree with sequence, especially when under parallel but it makes less sense here.

@abayer It sounds like stage and group are basically identical.
What about having each stage allow only one of the following: steps, parallel, or stages? (A reasonable alternative to stages would be sequence.)

The the above would look like:

stages {
    stage('Build') { steps { ... }  }
    stage('Release Train') {
        when { branch 'master' }
        stages {
            stage("Confirm deploy to Staging") { ... }
            stage("Deploy to Staging") { ... }
            stage("Confirm deploy to Production") { ... }
            stage("Deploy to Production") { ... }
        }
    }
}

This makes sequential stage groups consistent with the top level stages. This addresses the "Make group basically a special case of stage where steps and parallel aren't allowed" item in the list above.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

The way I'm coding it up is that Stage and ParallelGroup (the runtime model classes) both inherit from AbstractParallelContent, with AbstractParallelContent having agent et al, Stage having steps, parallel, and failFast, and ParallelGroup having a list of stages.

I'm not sure what the point of group directly under stages (or elided into stage) would be - the whole point is that you can have 2..N groups or stages within a stage's parallel. It's not meant to do "nesting" of a sequential list of stages within a stage without parallelism involved.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 29, 2017

@rtyler I don't hate sequence. I don't love it either, but I don't hate it. I almost wonder if stages would be right, but that'd create some confusion since parallel->stages could have agent, post, etc, while stages at the root can't...

@bitwiseman

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

@abayer
I'm not sure what the point of group directly under stages

In this case, so that we can do a when clause once on the group instead of on multiple stages.

I don't understand what you said about the runtime classes. I'm only talking about the user facing syntax. A stage can currently have either steps or parallel. If you allow a stage to have either steps, parallel, or stages, that (IMO) has is the least disruptive path. It also keeps the syntax consistent as it nests - pipeline -> stages is serial execution of stage items, stage -> stages is the same.

@bitwiseman

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

@abayer I get what you're saying about how you meant to use it, and I don't want to punish you for implementing this feature (which is my personal number 1 desired feature for declarative).

Maybe what I'm saying is if you open the box of allowing when, etc. on group, go all the way. Otherwise, it's going to cause more confusion and frustration.

Also if you allow group to have when, etc., then group is no long a list like parallel or stages and there needs to be a group -> stages.

@bitwiseman

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

@abayer
but that'd create some confusion since parallel->stages could have agent, post, etc, while stages at the root can't...

Huh? Example please.

@bitwiseman
Copy link
Contributor

left a comment

@abayer Maybe we could get on a hangout to discuss this?

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2017

@i386

This comment has been minimized.

Copy link

commented Nov 30, 2017

Liking where this is going but we need to get a designer on how this will work in the Editor before committing to this.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Nov 30, 2017

Got on the same page with @bitwiseman - the idea is to possibly make stage able to contain one-and-only-one-of steps, parallel, or stages. I just moved the stage list in group to a nested stages block now that we have other fields that could be used in group (agent etc), so what's being proposed is basically this:

  • At the top level, a stage could contain steps, parallel, or stages - in the last case, that would mean a parent stage step containing a sequentially executed list of individual stages, no parallelism involved.
  • Inside a parallel, a stage could contain steps or stages - in the last case, that would mean a parent stage step containing a sequentially executed list of individual stages, with that parent stage step run in parallel with any other stages defined inside the parallel.
  • Inside a stages (that is, a stage(...) { stages { ... } }), a stage could only contain steps. No arbitrarily deep nesting of parallelization or nested sequential lists.

The upsides to this approach are that it's simpler syntactically (albeit a bit confusing due to how many different ways you can use stage, and particularly for the double usage of stages, which we might want to rename in the "sequential list inside a stage" scenario), a sequential stage list within a parallel has the same syntax as a sequential stage list at the top level, and, here's where things get interesting, it'd let you do things like say "run this list of stages on a different agent than the top level one, but with all of this list of stages running in the same agent+workspace", since the node block for the parent would have all of the stage->stages list run inside it. That opens up some very interesting possibilities. I'll write more on that tomorrow when I haven't been at the computer for ~11 hours already. =)

@bitwiseman

This comment has been minimized.

Copy link
Contributor

commented Nov 30, 2017

Here's sample code that I think conveys what @abayer said above.

pipeline {
    agent none
    // list of stages executed sequentially
    stages {
        stage('first-solo') { 
            // list of steps executed in sequentially
            steps { ... } 
        }
        stage('parent') {
            // list of stages executed in parallel
            parallel {
                // Order in the parallel block is not significant.
                stage('single-stage') { steps { ... } }

                // It does need to take a string, though, so we have a name for the parallel branch 
                // it creates to contain its sequential stages. 

                stage('sequential-stages') {
                    // list of stages executed sequentially
                    stages { 
                        // Order inside a stages is significant - we will execute the stages in the order 
                        // they're specified here.
                        stage('first-sequential-stage') { steps { ... } }
                        stage('second-sequential-stage') { steps { ... } }
                    }
                }

                stage('other-single-stage') { steps { ... } }

                // etc - arbitrarily long list of either "stage" or "group"
            }
        }
        stage('grouped-stages') {
            // list of stages executed sequentially
            stages {
                stage('first-grouped') { steps { ... } }
                stage('second-grouped') { steps { ... } }
            }
        }
    }
}

@abayer abayer force-pushed the abayer:jenkins-46809 branch 2 times, most recently from 9511756 to 19a6837 Apr 24, 2018

@abayer abayer force-pushed the abayer:jenkins-46809 branch from 19a6837 to 79cad4e May 3, 2018

@Hokwang

This comment has been minimized.

Copy link

commented May 24, 2018

I am looking forward committing this PR. Please consider 1.2.10 not 1.3 milestone.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2018

@rsandell @svanoort @batmat @bitwiseman @kshultzCB - if any of you would like to do a review (or re-review) here, it'd be appreciated. Looking to get this and #228 rolling towards release in the next week or two.

@abayer abayer requested review from rsandell and bitwiseman Jun 4, 2018

/**
* Legacy version to pass the root tools in, rather than directly passing in a tools. Only relevant for in-progress
* runs.
* TODO: Delete in 1.4? Or maybe just nuke now.

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 7, 2018

Member

Don't know either. I don't remember how it behaves when restoring an old running pipeline with this new code. If the old code is what is run or not.

@@ -446,6 +512,7 @@ class ModelInterpreter implements Serializable {
} else {
toolEnv = actualToolsInstall(toolsList)
}
System.err.println("TOOL ENV: ${toolEnv}")

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 7, 2018

Member

remove

This comment has been minimized.

Copy link
@abayer

abayer Jun 8, 2018

Author Member

Done.

toolsList = root.tools.mergeToolEntries(null)
toolsList = tools.mergeToolEntries(rootTools)
} else if (rootTools != null) {
toolsList = rootTools.mergeToolEntries(null)

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 7, 2018

Member

Seems to be missing the actual root tools? i.e. merging all the way up?

This comment has been minimized.

Copy link
@abayer

abayer Jun 8, 2018

Author Member

Nope - if there are stage-level tools defined, we merge the root tools into them. Otherwise, we merge null into the root tools. It works. =)

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 12, 2018

Member

Hmm, from what I can see rootTools is either the tools definition from the parent or from root not both, and I don't see any of the merge results being "written" anywhere. 😕

"Second in group: FOO is BAH")
.go();
}

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 7, 2018

Member

credentials in group?

This comment has been minimized.

Copy link
@abayer

abayer Jun 8, 2018

Author Member

Good idea.

/*
* The MIT License
*
* Copyright (c) 2017, CloudBees, Inc.

This comment has been minimized.

Copy link
@rsandell

rsandell Jun 7, 2018

Member

It's a new year, a new life, a new dawn...

This comment has been minimized.

Copy link
@abayer

abayer Jun 8, 2018

Author Member

Ah, but it wasn't when I created that file. =)

}
}
}
}

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 11, 2018

Contributor

Can you add a newline at the end to avoid Ø?

This comment has been minimized.

Copy link
@abayer

abayer Jun 11, 2018

Author Member

Done!

@@ -26,3 +26,4 @@ DirectiveGenerator.DeclarativeDirectivesLink.displayName=Declarative Directive G
DirectiveGenerator.DeclarativeOnlineDocsLink.displayName=Declarative Online Documentation
StageDirective.Steps.name=Steps
StageDirective.Parallel.name=Parallel Stages
StageDirective.Stages.name=Sequential Stages

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

Can you add a newline at the end to avoid Ø?

for (ModelASTBranch branch : branches) {
branch.validate(validator);
}
for (ModelASTStage content: parallelContent) {

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

probably add a space before : to match the previous block

@@ -575,11 +575,24 @@ class ModelParser implements Parser {
stage.environment = parseEnvironment(s)
break
case 'parallel':
stage.parallel = parseStages(s)
def parallelStmt = matchBlockStatement(s)
if (parallelStmt==null) {

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

probably spaces around ==

@@ -123,7 +123,7 @@ ModelValidatorImpl.InvalidUnnamedParameterType=Expecting "{0}" but got "{1}" of
ModelValidatorImpl.NoSteps=No steps specified for branch
ModelValidatorImpl.RequiredSection=Missing required section "{0}"
ModelValidatorImpl.NoStageName=Stage does not have a name
ModelValidatorImpl.NothingForStage=No "steps" or "parallel" to execute within stage "{0}"
ModelValidatorImpl.NothingForStage=No "steps", "stages", or "parallel" to execute within stage "{0}"

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

Expected one of "steps", "stages", or "parallel" for stage "{0}"

(No item series + or item... is a mess in English grammar)

@@ -142,7 +142,7 @@ ModelValidatorImpl.NestedWhenNoArgs=No additional arguments are allowed for chil
ModelValidatorImpl.NoNestedWhenAllowed=No children when conditions are allowed for when condition "{0}".
ModelValidatorImpl.NestedWhenWithoutChildren=Nested when condition "{0}" requires at least one child condition.
ModelValidatorImpl.NestedWhenWrongChildrenCount=Nested when condition "{0}" requires exactly {1} child condition(s).
ModelValidatorImpl.BothStagesAndSteps=Only one of "parallel" or "steps" allowed for stage "{0}"
ModelValidatorImpl.TwoOfStepsStagesParallel=Only one of "parallel", "stages", or "steps" allowed for stage "{0}"

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

This is better (compare above)

Note to self: The periods in the nearby lines are probably bugs -- or indications of bugs...

* @param root The root of the Declarative model
* @param parentAgent The parent agent definition, which can be null
* @param thisStage The current stage we'll look in for parallel stages
* @param firstError An error that's already occurred earlier in the build. Can be null.

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

The way this is written contrasts with parentAgent two lines earlier

Utils.markStageSkippedForConditional(parallelStage.name)
parallelStages.put(content.name, {
script.stage(content.name) {
Utils.logToTaskListener("Stage '${content.name}' skipped due to when conditional")

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

could when be marked up in some way?

This comment has been minimized.

Copy link
@abayer

abayer Jun 13, 2018

Author Member

I try to avoid markup in the console log.

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 13, 2018

Contributor

Single quotes (') as around the name of the stage was what I had in mind...

No worries.

"value": "some-label"
}
}
}}

This comment has been minimized.

Copy link
@jsoref

jsoref Jun 12, 2018

Contributor

Can you add a newline at the end to avoid Ø?

-- There may be others, best if your editor always includes them...

This comment has been minimized.

Copy link
@abayer

abayer Jun 13, 2018

Author Member

Yeah, this was a copy-paste case - my editor catches them normally.

@abayer

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2018

Alrightie, I am finally merging this. =)

@abayer abayer merged commit cf8afac into jenkinsci:master Jun 13, 2018

2 checks passed

continuous-integration/jenkins/branch This commit looks good
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
@ader1990

This comment has been minimized.

Copy link

commented Jul 4, 2018

Hello,

Is there a working example of this feature?
I see more than one version of this feature in the above comments.

Thanks.

@steven-foster

This comment has been minimized.

Copy link

commented Jul 4, 2018

there's a good blogpost about it here
https://jenkins.io/blog/2018/07/02/whats-new-declarative-piepline-13x-sequential-stages/

note that blue ocean does not support visualization of the stages separately yet, but it will still work properly. I'm personally waiting for that before adopting it.

@ader1990

This comment has been minimized.

Copy link

commented Jul 4, 2018

Thanks, I was also interested in the Blue Ocean integration (tracked here: https://issues.jenkins-ci.org/browse/JENKINS-384420

@nabertrand

This comment has been minimized.

Copy link

commented Jul 25, 2018

There's an extra number in the JIRA link above. Should be https://issues.jenkins-ci.org/browse/JENKINS-38442

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.