atlantis icon indicating copy to clipboard operation
atlantis copied to clipboard

feat: plan queue functionality

Open ghaiszaher opened this issue 2 years ago • 9 comments

Relates to: https://github.com/runatlantis/atlantis/discussions/1722

Hey @nishkrishnan, based on above discussion, we are submitting this PR that provides plan queue functionality.

The implementation of the queue functionality is complete and the code is almost production-ready. We are seeking your assistance and would appreciate your feedback if this is the right direction and if this functionality could be considered.

We we have done so far:

  • Automatically entering the queue upon a plan request if another PR has the lock.
  • Granting the lock to the next PR in queue when the previous PR is unlocked (either via explicit atlantis unlock command, from Atlantis UI or by merging/closing the PR).
  • Notifying the author that the PR is ready to be planned for the unlocked environments/workspaces.
    • We avoided automatic planning because: 1) A PR can have more than one lock and not all of them might be released yet. 2) It was cumbersome to store the initial command in case extra arguments were passed.
  • Showing the queue in the main UI and in the locks view. image image

What is missing:

  • Feature flag to enable/disable the queue.
  • Handling plan errors in respect to the queue: currently the lock is released on plan error, which means we should either:
    • Grant the lock to the next person in the queue
    • Or change that behavior and not release the lock on plan errors
  • More commands to handle the queue: e.g. jump in front of the queue, or get out of the queue.
  • More test coverage.

Contributors

@monikma @amir-elgayed @ghaiszaher

ghaiszaher avatar Nov 29 '21 16:11 ghaiszaher

how this behaves on 1 PR, 5 different projects changes all w/default workspace ? would it be able to use ProjectName to produce parallel plans ? Mostly because the existing locking mechanism is tied to $workDir and how it is formulated repo+pr+workspace+path

xavipanda avatar Dec 02 '21 03:12 xavipanda

how this behaves on 1 PR, 5 different projects changes all w/default workspace ? would it be able to use ProjectName to produce parallel plans ? Mostly because the existing locking mechanism is tied to $workDir and how it is formulated repo+pr+workspace+path

@xavipanda The key for the queue is the same as the key for the lock (see here).

As soon as one of those projects gets unlocked, a comment will be posted on the PR stating the projects for which locks are free. The PR owner has to then replan manually.

Same happens with 1 PR and 1 project & workspace. We were not sure how and if we want to plan automatically because of that multi-project/workspace situation (as mentioned in the PR description). Suggestions are welcome.

monikma avatar Dec 03 '21 13:12 monikma

This also relates to #305 but is technically different.

majormoses avatar Feb 12 '22 01:02 majormoses

@ghaiszaher @monikma Great job

GabrielBB avatar May 16 '22 19:05 GabrielBB

This also relates to #305 but is technically different.

@majormoses As far as I understood, it is quiet different: This is about providing a queueing mechanism for plans cross different PRs that affect the same workspace (they compete for the same lock), if a lock is acquired by PR#1, plans from other PRs will enter a queue and will be notified when the lock is released by PR#1, whereas #305 is about plans on the same PR that is already holding the lock.

amir-elgayed avatar May 17 '22 14:05 amir-elgayed

@ghaiszaher is there still interest in getting this merged?

jamengual avatar Aug 18 '22 16:08 jamengual

@ghaiszaher is there still interest in getting this merged?

@jamengual yes we are still interested, we'd appreciate any feedback on the current PR.

ghaiszaher avatar Aug 19 '22 15:08 ghaiszaher

@ghaiszaher do you have any time to address the comments?

jamengual avatar Sep 21 '22 04:09 jamengual

@ghaiszaher do you have any time to address the comments?

@jamengual I tried to answer the comments, since Ghais is currently on vacation. First of all, @lilincmu thanks a lot for the review! Hopefully we can shorten the feedback cycle now, and make each next interaction exponentially quicker ;)

One more nit from me is, it might be more readable if we define []models.ProjectLock to be a new type, such as ProjectLockQueue. When reading the code, sometimes I had to figure out whether []models.ProjectLock refers to a queue, or simply a list of locks, for example, that are just being unlocked by a PR.

Makes a lot of sense to me.

About your questions on the tests - yes we have not implemented all of them, as we first wanted to check with you if this is some functionality that sounds like a good idea, and if how we go about implementing it would make sense to you (as mentioned in the PR description). So my understanding is "yes it does" ? :)

We could continue by applying the suggestions and extending tests. Would that make sense to you? What do you expect from us to call our PR "production ready", and get closer to merging it?

One topic we circled around but never properly got to, is feature flag to enable/disable the feature. If you had any suggestions there, it would be great (meaning how feature flags are usually approached in this codebase, we would not want to break some convention).

monikma avatar Sep 21 '22 21:09 monikma