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

External Locking DB: Redis #2491

Merged
merged 11 commits into from
Sep 7, 2022
Merged

Conversation

SudoSpartanDan
Copy link
Contributor

This is hopefully the start of many implementations of external locking databases.

This addresses #894 and has the ability to make Atlantis a stateless application within Kubernetes, allowing scaling up and down of an Atlantis instance. Additionally, this abstracts out the backend DB, making it easier for others to add other locking backend database types as time goes on.

@SudoSpartanDan SudoSpartanDan requested a review from a team as a code owner September 6, 2022 13:20
@SudoSpartanDan

This comment was marked as resolved.

@NotLikeThisCHR
Copy link

Absolutely Cool @SudoSpartanDan!

What a step up for Atlantis.

Thank you for the efforts!

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.

🎉 amazing work!

@jamengual jamengual added feature New functionality/enhancement waiting-on-review Waiting for a review from a maintainer labels Sep 7, 2022
Copy link
Contributor

@lilincmu lilincmu left a comment

Choose a reason for hiding this comment

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

Except for small nits this looks good to me! Thanks for the contribution 🎉

cmd/server.go Outdated Show resolved Hide resolved
docker-compose.yml Show resolved Hide resolved
@lilincmu
Copy link
Contributor

lilincmu commented Sep 7, 2022

Another nit. There are many rearrangements on the import statement of "reflect", which could be removed from this PR for the sake of code hygiene.

@SudoSpartanDan
Copy link
Contributor Author

Another nit. There are many rearrangements on the import statement of "reflect", which could be removed from this PR for the sake of code hygiene.

Do you have a link to a specific instance? Not seeing it in the diff on GitHub's side, but I know GitHub has been off today, so not sure if it's me or GitHub.

@lilincmu
Copy link
Contributor

lilincmu commented Sep 7, 2022

Another nit. There are many rearrangements on the import statement of "reflect", which could be removed from this PR for the sake of code hygiene.

Do you have a link to a specific instance? Not seeing it in the diff on GitHub's side, but I know GitHub has been off today, so not sure if it's me or GitHub.

Hmm I just realized those rearrangements were probably generated by pegomock. I didn't notice it because the comments in the first line were hidden when I first reviewed it.
Screen Shot 2022-09-07 at 3 31 00 PM

@SudoSpartanDan If they were indeed generated by pegomock then I can go ahead and merge this PR. What do you think?

@SudoSpartanDan
Copy link
Contributor Author

Yup, I didn't generate those files, so any rearranging was done by pegomock.

@lilincmu
Copy link
Contributor

lilincmu commented Sep 7, 2022

@SudoSpartanDan The golangci-lint linter is failing. Maybe add // nolint: revive to the type RedisDB definition?

If I understand the error message correctly, it's complaining exported: type name will be used as redis.RedisDB by other packages, and that stutters; consider calling this DB (revive). But I can see why you named it RedisDB, since we name the other backend BoltDB in boltdb.go.

@SudoSpartanDan
Copy link
Contributor Author

Thanks, @lilincmu! Looks like that fixed the lint test!

@lilincmu lilincmu merged commit ab2cdb8 into runatlantis:master Sep 7, 2022
@jamengual
Copy link
Contributor

This is an awesome addition to the project thanks @SudoSpartanDan

@Omicron7
Copy link
Contributor

@SudoSpartanDan Thanks for adding this. We've been waiting for some sort of stateless locking mechanism for a while. Would it be possible to add Redis DB index configuration option that would issue a SELECT $db_index on connection so that all subsequent redis commands use a different database index. See https://redis.io/commands/select/
Thanks again for this.

@SudoSpartanDan
Copy link
Contributor Author

@Omicron7 Im not sure what that gets us for performance, also we haven’t released this yet so I want to wait til testing by others is done before we start throwing more options at this. Feel free to contribute on your side if you feel this is necessary, but in my testing, this is more of a nice to have.

krrrr38 pushed a commit to krrrr38/atlantis that referenced this pull request Dec 16, 2022
* Initial build out of redis backend

* Regenerate locking mocks and matchers

* More test cleanup

* More test fixes

* Added tests for redis

* Test fix

* Linting fix

* Dcos update

* Update redis.go

* Update server.go

* Adding nolint to RedisDB struct
@nitrocode nitrocode added this to the v0.19.9 milestone Jan 13, 2023
@nitrocode
Copy link
Member

Thank you so much for adding this feature @SudoSpartanDan !

@lilincmu @SudoSpartanDan how does this work with storing plans? From what I recall, Atlantis stores the plans locally on each server.

Is it required for all the Atlantis instances to use the same volume so it has access to the same plan output?

Have you folks been able to test this?

Reason I'm asking is because i was requesting this enablement in the anton/bryant fargare Atlantis module to get more users to take advantage of this feature. The hope is that we can raise the number of Atlantis instances to more than one

See request in upstream module terraform-aws-modules/terraform-aws-atlantis#322

@JonGilmore
Copy link
Contributor

@nitrocode we have been running this in production since it was released and have had zero problems, it works flawlessly. We don't run multiple atlantis nodes, but that I'm aware of, there's no reason why we couldn't. We perform red/black deploys using immutable ec2 infra and the atlantis user home directory is stored on EFS, so plans are persisted during deployments.

@nitrocode
Copy link
Member

nitrocode commented Feb 27, 2023

That's good to hear.

I'd love to know if you can increase your atlantis so it's HA.

I'd also like to know if the locking db for redis also stores the plan, then we do not have to use EFS since EFS is very slow when it comes to git clone.

Please let us know if you (or someone else) gets a chance to test this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New functionality/enhancement waiting-on-review Waiting for a review from a maintainer
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants