Orca 679 global atlantis lock new release branch #49

msarvar · 2021-03-04T19:46:28Z

Added support to create a command specific lock entries to the db/bolt_db, LockCommand, UnlockCommand, and CheckCommandLock.

Implemented a new ApplyLocker interface for locking.go#Client that manages apply lock creation/deletion and lock retrieval.

LocksController has 2 handlers to create and delete apply locks.
Server#Index renders the UI to create/delete the locks.
ApplyCommandRunner expects an object that implements ApplyCommandLocker which boils down to object having IsDisabled function that returns boolean value.
DefaultApplyCommandLocker implements the interface.

In UI it is just a AJAX request that will create/delete the locks for apply. To create a new lock or delete existing one do to atlantis main page.

Recreated #48 against new release branch.

msarvar · 2021-03-04T19:47:01Z

/ptal @nishkrishnan

nishkrishnan

Nice work, have a few comments

nishkrishnan · 2021-03-04T19:55:08Z

server/events/db/boltdb.go

+	transactionErr := b.db.Update(func(tx *bolt.Tx) error {
+		bucket := tx.Bucket(b.globalLocksBucketName)
+
+		currLockSerialized := bucket.Get([]byte(b.commandLockKey(cmdName)))


Do we want to make this idempotent? Personally, would rather error out if the lock exists. Think about TFE returning a 409 in a similar situation.

nishkrishnan · 2021-03-04T19:56:23Z

server/events/db/boltdb.go

+func (b *BoltDB) LockCommand(cmdName models.CommandName, lockTime time.Time) (*models.CommandLock, error) {
+	lock := models.CommandLock{
+		CommandName: cmdName,
+		Time:        lockTime,


I'd lean to having like a LockMetadata struct which contains the time in addition to a description. Eventually we might want to add user there for example and just makes it easy to add without changing the interface.

nishkrishnan · 2021-03-04T20:18:39Z

server/events/apply_command_runner.go

+//go:generate pegomock generate -m --use-experimental-model-gen --package mocks -o mocks/mock_apply_command_locker.go ApplyCommandLocker
+
+type ApplyCommandLocker interface {
+	IsDisabled(ctx *CommandContext) bool


ie. This struct isn't responsible for performing the locking, it's responsible for checking whether a lock exists. Confusing naming imo.

Id suggest renaming this to GlobalApplyLockChecker with the method isLocked (disabled also doesn't fit well if the name of the struct doesn't include that)

nishkrishnan · 2021-03-04T20:19:44Z

server/events/apply_command_runner.go

+
+func NewApplyCommandLocker(
+	applyLockChecker locking.ApplyLockChecker,
+	disableApply bool,


can we rename this to disableApplyFlag or something that hints at this being a global server flag?

nishkrishnan · 2021-03-04T20:26:28Z

server/events/apply_command_runner.go

+// IsDisabled returns true if there is a global apply command lock or
+// DisableApply flag is set to true
+func (a *DefaultApplyCommandLocker) IsDisabled(ctx *CommandContext) bool {
+	lock, err := a.ApplyLockChecker.CheckApplyLock()


Looks like command context is barely used here. The logger embedded in there is useful if we want to debug per-pull issues. However, the global apply lock fetching error doesn't seem necessary to log at that level, you can just keep that logger embedded in the struct.

If you do the above, you can just use a single interface delegation pattern for:

fetching the global lock

comparing the result of that to the global server config flag.

nishkrishnan · 2021-03-04T20:27:37Z

server/events/apply_command_runner.go

@@ -53,7 +88,7 @@ func (a *ApplyCommandRunner) Run(ctx *CommandContext, cmd *CommentCommand) {
 	baseRepo := ctx.Pull.BaseRepo
 	pull := ctx.Pull

-	if a.DisableApply {
+	if a.locker.IsDisabled(ctx) {
 		ctx.Log.Info("ignoring apply command since apply disabled globally")
 		if err := a.vcsClient.CreateComment(baseRepo, pull.Num, applyDisabledComment, models.ApplyCommand.String()); err != nil {


The applyDisabledComment is very generic, would be good to think about how to make that more useful depending on how the apply command lock is configured. Not in scope for this PR though.

nishkrishnan · 2021-03-04T20:33:46Z

server/events/locking/locking.go

+}
+
+// CheckApplyLock retrieves an apply command lock if present.
+func (c *Client) CheckApplyLock() (ApplyCommandLockResponse, error) {


Sounds to me like this should also follow the same logic of checking the server configuration as well. We should also probably not allow unlocking of atlantis if it's configured to disable applies through server configuration both.

Right now the implementation doesn't do that which is good, however, it'll say it's locked and unlocked it, but applies still won't work if the server configuration has it defined.

nishkrishnan · 2021-03-04T21:41:52Z

server/events/locking/locking.go

+func (c *Client) LockApply() (ApplyCommandLockResponse, error) {
+	response := ApplyCommandLockResponse{}
+
+	applyCmdLock, err := c.backend.LockCommand(models.ApplyCommand, time.Now().Local())


Best practice is to use unix timestamps if we are storing data.

nishkrishnan · 2021-03-10T16:36:46Z

server/events/apply_command_runner_test.go

+	"github.com/runatlantis/atlantis/server/events/models/fixtures"
+)
+
+// func setupApplyCmd(t *testing.T) *events.ApplyCommandRunner {


nishkrishnan · 2021-03-10T16:37:21Z

server/events/command_runner_test.go

 	When(logger.GetLevel()).ThenReturn(logging.Info)
 	When(logger.NewLogger("runatlantis/atlantis#1", true, logging.Info)).
 		ThenReturn(pullLogger)

+	scope := stats.NewStore(stats.NewNullSink(), false)


Was this added for fixing the tests you thought were broken? lol do we still need it?

No, this is to remove the logging warnings about statsd failing to reach the localhost. Just to reduce the noise :)

nishkrishnan · 2021-03-10T16:38:20Z

server/events/db/boltdb.go

-	pullKeySeparator = "::"
+	locksBucketName       = "runLocks"
+	pullsBucketName       = "pulls"
+	globalLocksBucketName = "global"


missed this earlier but let's name it globalLocks

nishkrishnan · 2021-03-10T16:39:04Z

server/events/db/boltdb.go

+// If the lock already exists, it will just return pointer to the existing lock
+// If lock creation fails it will return nil


Probably should update this to reflect the new changes.

Address PR comments

msarvar · 2021-03-10T21:57:38Z

💨

* Orca 679 global atlantis lock new release branch (#49) * Adding CommandLocker to boltDB and exposing it through locker interface * Apply lock ui and apply command lock controller * Minor comments * Adding more tests and refactorinng * Linting fixes * creating applyLockingClient variable to fix interface error * nullsink for stats * Addressing PR comments * fixing e2e tests * linting fix fml * Update outdated function descriptions Address PR comments * revert stats sink changes * remove unnecessary dependencies

* Adding CommandLocker to boltDB and exposing it through locker interface * Apply lock ui and apply command lock controller * Minor comments * Adding more tests and refactorinng * Linting fixes * creating applyLockingClient variable to fix interface error * nullsink for stats * Addressing PR comments * fixing e2e tests * linting fix fml * Update outdated function descriptions Address PR comments * revert stats sink changes

msarvar added 5 commits March 4, 2021 11:43

Adding CommandLocker to boltDB and exposing it through locker interface

892b52a

Apply lock ui and apply command lock controller

3e2dff3

Minor comments

20b53c4

Adding more tests and refactorinng

f4b6268

Linting fixes

8206184

msarvar added 2 commits March 4, 2021 12:02

creating applyLockingClient variable to fix interface error

888a2b7

nullsink for stats

02f375c

nishkrishnan reviewed Mar 4, 2021

View reviewed changes

msarvar added 3 commits March 9, 2021 13:35

Addressing PR comments

e225a63

fixing e2e tests

6ab3f62

linting fix fml

9e6473d

nishkrishnan previously approved these changes Mar 10, 2021

View reviewed changes

Update outdated function descriptions

edb49da

Address PR comments

msarvar dismissed nishkrishnan’s stale review via edb49da March 10, 2021 20:26

revert stats sink changes

fac637e

nishkrishnan approved these changes Mar 10, 2021

View reviewed changes

msarvar merged commit 5db4734 into release-v0.17.0-beta-lyft.1 Mar 12, 2021

mikecutalo deleted the ORCA-679-global-atlantis-lock-new-release-branch branch June 8, 2023 18:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Orca 679 global atlantis lock new release branch #49

Orca 679 global atlantis lock new release branch #49

msarvar commented Mar 4, 2021 •

edited

Loading

msarvar commented Mar 4, 2021

nishkrishnan left a comment

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 4, 2021

nishkrishnan Mar 10, 2021

nishkrishnan Mar 10, 2021

msarvar Mar 10, 2021

nishkrishnan Mar 10, 2021

nishkrishnan Mar 10, 2021

msarvar commented Mar 10, 2021

		// If the lock already exists, it will just return pointer to the existing lock
		// If lock creation fails it will return nil

Orca 679 global atlantis lock new release branch #49

Orca 679 global atlantis lock new release branch #49

Conversation

msarvar commented Mar 4, 2021 • edited Loading

msarvar commented Mar 4, 2021

nishkrishnan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

msarvar commented Mar 10, 2021

msarvar commented Mar 4, 2021 •

edited

Loading