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

checkout-strategy merge : checking if it's at the right commit not working #804

Closed
quidio opened this issue Oct 14, 2019 · 4 comments · Fixed by #867 or #3187
Closed

checkout-strategy merge : checking if it's at the right commit not working #804

quidio opened this issue Oct 14, 2019 · 4 comments · Fixed by #867 or #3187
Labels
bug Something isn't working

Comments

@quidio
Copy link

quidio commented Oct 14, 2019

checkout-strategy merge : checking if it's at the right commit not working

Hi,

I'm using atlantis (0.9.0) and I find an issue with the checkout-strategy: merge.

I love the strategy merge, it fit my use case, but when I have multiple PR open they are not updated with modification on master.

I have reproduce a use case only with git command.

Initial setup

git init demo
cd demo
echo "UserDev1" > user-dev.txt
echo "UserAdmin1" > user-admin.txt
git add .
git commit -m "Initial file"

Simulate first PR

git checkout -b first-pr
echo "UserDev2" >> user-dev.txt
git add .
git commit -m "Add user dev 2"
cd ..

atlantis checkout first PR

git clone --branch master --single-branch demo atlantis-first-pr
cd atlantis-first-pr
git remote add head ../demo
git fetch head +refs/heads/first-pr
git merge -q --no-ff -m atlantis-merge FETCH_HEAD

Simulate second PR

cd ../demo
git checkout master
git checkout -b second-pr
echo "UserAdmin2" >> user-admin.txt
git add .
git commit -m "Add user admin 2"
cd ..

atlantis checkout second PR

git clone --branch master --single-branch demo atlantis-second-pr
cd atlantis-second-pr
git remote add head ../demo
git fetch head +refs/heads/second-pr
git merge -q --no-ff -m atlantis-merge FETCH_HEAD
cd ..

merge first PR

cd demo
git checkout master
git merge first-pr
cd ..

In second PR simulate atlantis plan

cd atlantis-second-pr
git log
 commit 4e2d4da0334e674aa2bffc2b31588d9ebe612224 (HEAD -> master)
 Merge: 7039e1f 4f21f02
 Author: Stephane Cusin <stephane.cusin@wayoos.com>
 Date:   Mon Oct 14 21:57:49 2019 +0200

    atlantis-merge

 commit 4f21f0265ef8d910a105a147537f32e8ca129be3 (head/second-pr)
 Author: Stephane Cusin <stephane.cusin@wayoos.com>
 Date:   Mon Oct 14 21:52:18 2019 +0200

    Add user admin 2

 commit 7039e1f877a475a03821fe62aeaf3ff1553e5886 (origin/master)
 Author: Stephane Cusin <stephane.cusin@wayoos.com>
 Date:   Mon Oct 14 21:15:11 2019 +0200

    Initial file

git rev-parse HEAD^2
4f21f0265ef8d910a105a147537f32e8ca129be3

The repository is at the same commit but master branch has changed, my first PR merged in master is not present.
The checkout mode merge is very interresting but in this case if it was terraform code it will remove my user-dev2.

@lkysow
Copy link
Member

lkysow commented Oct 15, 2019

Hi Stéphane,
Are you testing this with Atlantis or just locally simulating the git commands?
If you haven't tested with Atlantis can you please try and replicate this issue there first. If you have tested with Atlantis, please show screenshots of both PRs and the Atlantis logs.

There are a couple of things to consider when running with Atlantis:

  1. If the first PR is already open, then when you open up the second PR Atlantis will not run plan because the workspace will be locked.
  2. When you've applied and merged the first PR the workspace will be unlocked
  3. At this point, you'll have to run atlantis plan on the second PR which should pick up the changes from master.

@lkysow lkysow added the question Further information is requested label Oct 15, 2019
@quidio
Copy link
Author

quidio commented Oct 16, 2019

Hi Luke,

We had the issue in production with gitlab. The git commands was the way to reproduce the issue with reading the code.

I put here the atlantis log of the same scenario as I describe with git command.

I agree with you about atlantis workflow but I will add that:

  1. If the first PR is already open, then when you open up the second PR Atlantis will not run plan because the workspace will be locked. But Atlantis will clone the repository before check the lock
  2. ...
  3. At this point, you'll have to run atlantis plan on the second PR which should pick up the changes from master. The second PR will not recloned while it was already cloned at 1.

What I see to fix that is:

  • add an option in server config like --force-clone
  • in check if repo is at correct commit also validate that master is up-to-date.
  • execute force clone when the PR acquires the lock

@lkysow
Copy link
Member

lkysow commented Oct 16, 2019

Okay I see the issue now.
It's not that simple though because we don't want to be recloning in the middle of running commands for different workspaces/dirs.

For example if I run atlantis plan -d dir1 and then master changes and I run atlantis plan -d dir2 we don't want the second command to be running on a different version of the code.

The other issue is that locks aren't per pull request but per workspace/dir. So you could have dir1 unlocked and dir2 locked. Then when dir2 becomes unlocked, again we don't want to change the code version out from under dir1.

We'll need to think about this further.

@lkysow lkysow added bug Something isn't working and removed question Further information is requested labels Oct 16, 2019
@MRinalducci
Copy link
Contributor

Hi Luke,

As I understand, first point is about concurrent computing issue, when atlantis plan -d dir1 runs concurrent with atlantis plan -d dir2 in the same PR. I'm right?

I'm not sure to understand second and third point.
I think it is always better to have the code up to date, because you can have issues when the states doesn't match you current PR code as it is outdated (deleting ressources created in the last merged PR).

I agree that we need to think this further and make some testing.
But in the meantime, would it be possible to implement a warning in the comment if master has changed?

@lkysow lkysow mentioned this issue Nov 27, 2019
finnag added a commit to finnag/atlantis that referenced this issue Mar 3, 2023
The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979
finnag added a commit to finnag/atlantis that referenced this issue Mar 21, 2023
The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979
nitrocode pushed a commit that referenced this issue Mar 25, 2023
…n base update (#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects #804, #867, #979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
nitrocode pushed a commit that referenced this issue May 5, 2023
…n base update (#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects #804, #867, #979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
ijames-gc pushed a commit to gocardless/atlantis that referenced this issue Feb 13, 2024
…n base update (runatlantis#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
ijames-gc pushed a commit to gocardless/atlantis that referenced this issue Feb 13, 2024
…n base update (runatlantis#3187)

* merge again if base branch has been updated

The current "merge" checkout strategy is unsafe. It merges the PR and the base
branch without holding the directory lock(s), so there is a potentially
very long window where another PR can be applied and be unexpectedly reverted
later.

This happens occasionally if a PR causes plans in multiple directories, but
is almost _guaranteed_ to happen if the initial plan has to wait until a
lock is freed up and a manual "atlantis plan" command is given.

Instead of printing a warning when this happens, we now merge again
if necessary while holding the lock. Plans are then guaranteed to only
be made when merged with the base branch for each directory being planned,
and applying later should be safe even if the base branch sees further
updates.

This fixes/affects runatlantis#804, runatlantis#867, runatlantis#979

* Remove diverged test that no longer applies

* Reinstate TestClone_MasterHasDiverged test and make it pass

* Extend TestClone_MasterHasDiverged to test new merging functionality

We now verify that the first Clone with CheckoutMerge=true with
a diverged base branch atually clones and merges again.

---------

Co-authored-by: PePe Amengual <jose.amengual@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
3 participants