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

New commit or "plan comment" to cancel the currently running plan and rerun #305

Open
majormoses opened this issue Oct 2, 2018 · 16 comments
Labels
feature New functionality/enhancement Stale

Comments

@majormoses
Copy link
Contributor

majormoses commented Oct 2, 2018

Current Behavior:

  1. user opens PR
  2. plan runs and user waits for CR
  3. user needs to rebase/merge master into their branch to pick up already merged changes
  4. user commits again (terraform fmt for example)
  5. Atlantis comments back "the default workspace is currently locked by another command that is running for this pull request–wait until the previous command is complete and try again" and marks the pull request status checks as failed
  6. running plan finishes and outputs success but does not update the status check

Desired Behavior:

  1. cancels the current plan and comments back such
  2. new plan is run with the latest commit

Related

@jolexa
Copy link
Contributor

jolexa commented Oct 8, 2018

I had the same experience and logged it in #303 including a few options that I thought of. 👍

@majormoses
Copy link
Contributor Author

We do need to be careful to send/trap the signal to allow terraform to shut down gracefully or it could lead to a corrupted/incomplete state.

@smiller171
Copy link

@majormoses Is that true even when only running plan?

@majormoses
Copy link
Contributor Author

majormoses commented Apr 3, 2019

@smiller171 the short answer it depends, the longer answer is yes 😉. If you run terraform with a plan -refresh=true it can absolutely cause state corruption without allowing it to gracefully shut down. While this is not the default (anymore) we need to be careful about this as users are free to specify their own options. Additionally in #187 we also want to be able to abort an apply in the case of something missed during code review and to provide some damage control. I think we should solve the problem of how to trap the terraform process properly once which allows reuse rather than trying to take shortcuts and focus on delivering potentially dangerous quick wins.

@lkysow lkysow added the feature New functionality/enhancement label Apr 4, 2019
@joshk0
Copy link

joshk0 commented Aug 14, 2019

I'm seeing this issue primarily in the context of this sequence of events:

  • A user submits a pull request
  • The plan is ongoing (plan takes several minutes)
  • The user pushes a commit to the pull request
  • Plan is set to failed because a plan is already running
  • The ongoing plan finishes, but it's useless because it's a) related to an old commit and b) doesn't update the plan status to green.

In my mind, the way it should work is that after the second commit is pushed, a new plan should be 'queued' for execution directly after the first plan. Then the first plan can pass, set the check status to whatever that plan ended up as, then immediately begin working on the second plan.

The way things are now, I have to wait for all the plans to quiesce then issue atlantis plan again. It's a fairly annoying papercut for a lot of the amateur users of atlantis in my team because they're not aware of the state management (or lack thereof) inside atlantis.

It seems like the progression of this thread is focused on locking or aborting existing plans rather than enqueuing a new one. It seems like you can get correctness easiest by not trying to abort anything, but instead just getting a queueing logic setup properly.

@smiller171
Copy link

Personally I'd prefer an abort so that I don't have to wait on the irrelevant plan

@jolexa
Copy link
Contributor

jolexa commented Aug 14, 2019

Personally I'd prefer an abort so that other tooling waiting on ✅doesn't start up (race condition).

@smiller171
Copy link

Aren't those checks bound to specific commits?

@majormoses
Copy link
Contributor Author

The short answer is that it should be user configurable, in many cases all it does is clog up your pipeline to queue them. I think abort is a sane default and allow those that want to queue to do that.

@andrewring
Copy link
Contributor

I would suggest the implementation of this not depend on new commits, but rather a new plan/apply starting. This will broaden scope to include manually re-running plan or apply via comments.

@majormoses
Copy link
Contributor Author

majormoses commented Jul 18, 2020

I see your point but I think that we need to limit that only to plans and not applies. Canceling an apply can lead to state corruption even in some cases when they are properly trapped. On the matter of canceling a running apply that is a much larger issue and that needs to be tackled separately with a great deal of care.

@andrewring
Copy link
Contributor

That's a great point. Perhaps, instead, applies can block, or otherwise be stopped from running in parallel, but without interruption.

@majormoses
Copy link
Contributor Author

majormoses commented Jul 18, 2020

That's a great point. Perhaps, instead, applies can block, or otherwise be stopped from running in parallel, but without interruption.

If an apply is running you can't just run an apply again without running a plan. In addition to atlantis locks there are terraform locks and hopefully something dynamodb as a locking table. What is the use case of doing anything other than commenting back that there is an apply in progress and they will need to wait until it is finished before plans can be run again? Terraform plan and applies must be done serially if they are to be trusted. I might be missing something but the problem set here IMO is specific to plans. I think that even if there were a use case I would want to tackle the general problem of safe killing an apply which is already being tracked on #187 I would not do it in the same change set as this.

@majormoses majormoses changed the title A new commit on the same pull request should cancel the running plan Ability for a new commit or "plan comment" on the same pull request to cancel the currently running plan and rerun Jul 18, 2020
@andrewring
Copy link
Contributor

andrewring commented Jul 18, 2020 via email

@majormoses
Copy link
Contributor Author

Not a correctness problem, then, so much as a clarity problem. It two people comment apply about the same time, it becomes unclear what's happening, as you see errors but no indication one of the runs is still running. I have seen that happen before.

💯 % agree and I think that's actually not that hard to solve, but is a separate issue. An apply (at least if you use it sanely) is always based on a previously saved plan so we just need to detect when an apply comment is passed and to have a better comment message. This is focused specifically on having the ability to cancel a running plan when a new comment (love the addition) or commit essentially invalidates if configured to. In the context of apply there is no such thing as it being invalid because you add new things, you wait for the current one to finish and then do another plan / apply cycle. There is a use case for canceling applies and I linked to that already.

@nitrocode nitrocode changed the title Ability for a new commit or "plan comment" on the same pull request to cancel the currently running plan and rerun New commit or "plan comment" to cancel the currently running plan and rerun Jan 18, 2023
@igorjanevski
Copy link

Is this in some roadmap to be worked on?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement Stale
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants