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

Add support for parallel plans #926

Merged
merged 11 commits into from
May 25, 2020

Conversation

Fauzyy
Copy link
Contributor

@Fauzyy Fauzyy commented Feb 7, 2020

Issue: #260

We've been running a simple repo config implementation of parallel plans for a couple weeks in our fork and it's working well

Caveat: because Atlantis has internal locks on the workspace name, this only works if all projects in the repo use Terraform Workspaces.

This PR also adds parallel apply as well.

Example config:

version: 3
automerge: true
parallel_plan: true (default false)
parallel_apply: true (default false)
...

Copy link
Member

@lkysow lkysow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey sorry it's taken so long to get to this. I've left a couple of comments, but I know it's been a while so no worries if you can't get back to me.

I was thinking about having this be on automatically if the plans are in separate workspaces and skip the config option.

cmd/server.go Outdated Show resolved Hide resolved
server/events/command_runner.go Show resolved Hide resolved
server/events/yaml/raw/repo_cfg.go Outdated Show resolved Hide resolved
@Fauzyy
Copy link
Contributor Author

Fauzyy commented Apr 6, 2020

@lkysow thanks! I'll address the comments this week

@lkysow
Copy link
Member

lkysow commented Apr 6, 2020

@Fauzyy FYI I was looking at enabling this for all projects, not just workspaces. If we add a queuing system to the working dir locker then we could lock it while it's doing the clone but otherwise we'd be fine to run plan in separate directories. What do you think?

You wouldn't have to do it in this PR, I could pick it up once you've got the per-workspace code ready.

@Fauzyy
Copy link
Contributor Author

Fauzyy commented Apr 6, 2020

@lkysow that sounds great 👍. The only case I'd be concerned about is the one where multiple atlantis projects point to the same directory (through some kind of .tfvars setup with custom workflows)

@adnankobir
Copy link

@Fauzyy @lkysow we would love to have the ability to run apply's in parallel as well. Given that there's a risk of unintended side effects, could we perhaps have it default to disable and expose a flag to enable?

@Fauzyy
Copy link
Contributor Author

Fauzyy commented May 6, 2020

@adnankobir that sounds reasonable yeah, I can make plans default parallel but add a flag for applies. Let me know what you think, @lkysow

Fauzyy added 2 commits May 5, 2020 18:15
+ Default parallel plans breaks in a lot of autoplan scenarios i.e. with shared workspaces
+ Added a set of e2e tests to check parallel plans and applies
+ Also added an option to the Terraform client to turn off the plugin cache. This was breaking the e2e test suite (never seen this error in practice)
@Fauzyy Fauzyy requested a review from lkysow May 8, 2020 05:47
@Fauzyy
Copy link
Contributor Author

Fauzyy commented May 8, 2020

For some reason the e2e test is running when it shouldn't be:

- run: if [ -z "${CIRCLE_PR_REPONAME}" ]; then ./scripts/e2e.sh; fi

I also added back the flag to disable parallel plans (off by default), for now. It's breaking a lot of e2e tests when you're not using workspaces. Feel free to change this after!

@lkysow
Copy link
Member

lkysow commented May 8, 2020

It looks like your segmentio circleci instance is running on this PR and somehow reporting checks back.

@Fauzyy
Copy link
Contributor Author

Fauzyy commented May 8, 2020

Oops, should be fixed now 👍

@codecov
Copy link

codecov bot commented May 8, 2020

Codecov Report

Merging #926 into master will decrease coverage by 0.45%.
The diff coverage is 52.74%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #926      +/-   ##
==========================================
- Coverage   71.96%   71.50%   -0.46%     
==========================================
  Files          65       65              
  Lines        5429     5489      +60     
==========================================
+ Hits         3907     3925      +18     
- Misses       1215     1254      +39     
- Partials      307      310       +3     
Impacted Files Coverage Δ
server/events/models/models.go 75.00% <ø> (ø)
server/events/yaml/valid/repo_cfg.go 19.04% <ø> (ø)
server/events/command_runner.go 42.95% <8.88%> (-6.42%) ⬇️
server/events/yaml/raw/repo_cfg.go 91.66% <83.33%> (-4.77%) ⬇️
server/events/project_command_builder.go 82.57% <100.00%> (+0.44%) ⬆️
server/events/terraform/terraform_client.go 78.36% <100.00%> (+0.21%) ⬆️
server/server.go 63.88% <100.00%> (+0.10%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 541d25d...67ef5fc. Read the comment docs.

@rverma-jm
Copy link

How do we enable parallel plan using repos.yaml

@@ -65,6 +65,9 @@ type DefaultClient struct {

// versionsLock is used to ensure versions isn't being concurrently written to.
versionsLock *sync.Mutex

// usePluginCache determines whether or not to set the TF_PLUGIN_CACHE_DIR env var
usePluginCache bool
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you going to expose this via a config file flag? Curious as to your use case for why you want to be able to turn this off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I could definitely expose this as a config file flag, this was just to fix the e2e test suite for parallel plans. They would transiently fail with the plugin cache enabled, we never saw this error in production however. The error mentions that the null_resource provider is missing when a plan is run, I wonder if the nature of these tests exacerbates the problem

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ahh okay

@lkysow
Copy link
Member

lkysow commented May 25, 2020

Amazing work, sorry this took so long. I'm planning to take steps to avoid this long a delay in the future. We will need to add docs for this.

@lkysow lkysow merged commit 6418b57 into runatlantis:master May 25, 2020
@ghostsquad
Copy link

Has the documentation not been updated for this feature? I don't see it here: https://www.runatlantis.io/docs/repo-level-atlantis-yaml.html#example-using-all-keys

@lkysow
Copy link
Member

lkysow commented Oct 23, 2020

Has the documentation not been updated for this feature? I don't see it here: https://www.runatlantis.io/docs/repo-level-atlantis-yaml.html#example-using-all-keys

No it has not. PRs welcome!

@nwsparks
Copy link

nwsparks commented May 4, 2021

Is this only valid if using terraform workspaces?

@mwarkentin
Copy link
Contributor

@nwsparks yes this is terraform workspaces only I believe.

@dmattia
Copy link
Contributor

dmattia commented May 4, 2021

We do not use terraform workspaces, but use parallelization. There is a difference between Atlantis workspaces and terraform workspaces

@Fauzyy Fauzyy deleted the parallel-plans-upstream branch May 4, 2021 23:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants