-
Notifications
You must be signed in to change notification settings - Fork 6
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
Orca 679 global atlantis lock new release branch #49
Changes from 5 commits
892b52a
3e2dff3
20b53c4
f4b6268
8206184
888a2b7
02f375c
e225a63
6ab3f62
9e6473d
edb49da
fac637e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,14 +2,49 @@ package events | |
|
||
import ( | ||
"github.com/runatlantis/atlantis/server/events/db" | ||
"github.com/runatlantis/atlantis/server/events/locking" | ||
"github.com/runatlantis/atlantis/server/events/models" | ||
"github.com/runatlantis/atlantis/server/events/vcs" | ||
) | ||
|
||
//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 | ||
} | ||
|
||
func NewApplyCommandLocker( | ||
applyLockChecker locking.ApplyLockChecker, | ||
disableApply bool, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can we rename this to disableApplyFlag or something that hints at this being a global server flag? |
||
) *DefaultApplyCommandLocker { | ||
return &DefaultApplyCommandLocker{ | ||
ApplyLockChecker: applyLockChecker, | ||
DisableApply: disableApply, | ||
} | ||
} | ||
|
||
type DefaultApplyCommandLocker struct { | ||
ApplyLockChecker locking.ApplyLockChecker | ||
DisableApply bool | ||
} | ||
|
||
// 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() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
|
||
if err != nil { | ||
ctx.Log.Err("failed to retrieve globalApplyCmdLock: %s", err) | ||
return a.DisableApply | ||
} | ||
|
||
disableApply := lock.Present || a.DisableApply | ||
return disableApply | ||
} | ||
|
||
func NewApplyCommandRunner( | ||
vcsClient vcs.Client, | ||
disableApplyAll bool, | ||
disableApply bool, | ||
applyCommandLocker ApplyCommandLocker, | ||
commitStatusUpdater CommitStatusUpdater, | ||
prjCommandBuilder ProjectApplyCommandBuilder, | ||
prjCmdRunner ProjectApplyCommandRunner, | ||
|
@@ -22,7 +57,7 @@ func NewApplyCommandRunner( | |
return &ApplyCommandRunner{ | ||
vcsClient: vcsClient, | ||
DisableApplyAll: disableApplyAll, | ||
DisableApply: disableApply, | ||
locker: applyCommandLocker, | ||
commitStatusUpdater: commitStatusUpdater, | ||
prjCmdBuilder: prjCommandBuilder, | ||
prjCmdRunner: prjCmdRunner, | ||
|
@@ -36,8 +71,8 @@ func NewApplyCommandRunner( | |
|
||
type ApplyCommandRunner struct { | ||
DisableApplyAll bool | ||
DisableApply bool | ||
DB *db.BoltDB | ||
locker ApplyCommandLocker | ||
vcsClient vcs.Client | ||
commitStatusUpdater CommitStatusUpdater | ||
prjCmdBuilder ProjectApplyCommandBuilder | ||
|
@@ -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 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
ctx.Log.Err("unable to comment on pull request: %s", err) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,68 @@ | ||
package events_test | ||
|
||
import ( | ||
"errors" | ||
"testing" | ||
|
||
. "github.com/petergtz/pegomock" | ||
"github.com/runatlantis/atlantis/server/events" | ||
"github.com/runatlantis/atlantis/server/events/locking" | ||
lockingmocks "github.com/runatlantis/atlantis/server/events/locking/mocks" | ||
"github.com/runatlantis/atlantis/server/logging" | ||
. "github.com/runatlantis/atlantis/testing" | ||
) | ||
|
||
func TestApplyCommandLocker_IsDisabled(t *testing.T) { | ||
ctx := &events.CommandContext{ | ||
Log: logging.NewNoopLogger(), | ||
} | ||
|
||
cases := []struct { | ||
Description string | ||
DisableApply bool | ||
ApplyLockPresent bool | ||
ApplyLockError error | ||
ExpIsDisabled bool | ||
}{ | ||
{ | ||
Description: "When global apply lock is present IsDisabled returns true", | ||
DisableApply: false, | ||
ApplyLockPresent: true, | ||
ApplyLockError: nil, | ||
ExpIsDisabled: true, | ||
}, | ||
{ | ||
Description: "When no global apply lock is present and DisableApply flag is false IsDisabled returns false", | ||
DisableApply: false, | ||
ApplyLockPresent: false, | ||
ApplyLockError: nil, | ||
ExpIsDisabled: false, | ||
}, | ||
{ | ||
Description: "When no global apply lock is present and DisableApply flag is true IsDisabled returns true", | ||
ApplyLockPresent: false, | ||
DisableApply: true, | ||
ApplyLockError: nil, | ||
ExpIsDisabled: true, | ||
}, | ||
{ | ||
Description: "If ApplyLockChecker returns an error IsDisabled return value of DisableApply flag", | ||
ApplyLockError: errors.New("error"), | ||
ApplyLockPresent: false, | ||
DisableApply: true, | ||
ExpIsDisabled: true, | ||
}, | ||
} | ||
|
||
for _, c := range cases { | ||
t.Run(c.Description, func(t *testing.T) { | ||
applyLockChecker := lockingmocks.NewMockApplyLockChecker() | ||
When(applyLockChecker.CheckApplyLock()).ThenReturn(locking.ApplyCommandLockResponse{Present: c.ApplyLockPresent}, nil) | ||
|
||
applyCommandLocker := events.NewApplyCommandLocker(applyLockChecker, c.DisableApply) | ||
|
||
Equals(t, c.ExpIsDisabled, applyCommandLocker.IsDisabled(ctx)) | ||
}) | ||
|
||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -60,6 +60,7 @@ var autoMerger *events.AutoMerger | |
var policyCheckCommandRunner *events.PolicyCheckCommandRunner | ||
var approvePoliciesCommandRunner *events.ApprovePoliciesCommandRunner | ||
var planCommandRunner *events.PlanCommandRunner | ||
var applyCommandLocker *mocks.MockApplyCommandLocker | ||
var applyCommandRunner *events.ApplyCommandRunner | ||
var unlockCommandRunner *events.UnlockCommandRunner | ||
var preWorkflowHooksCommandRunner events.PreWorkflowHooksCommandRunner | ||
|
@@ -86,10 +87,12 @@ func setup(t *testing.T) *vcsmocks.MockClient { | |
|
||
drainer = &events.Drainer{} | ||
deleteLockCommand = eventmocks.NewMockDeleteLockCommand() | ||
applyCommandLocker = eventmocks.NewMockApplyCommandLocker() | ||
When(logger.GetLevel()).ThenReturn(logging.Info) | ||
When(logger.NewLogger("runatlantis/atlantis#1", true, logging.Info)). | ||
ThenReturn(pullLogger) | ||
|
||
scope := stats.NewStore(stats.NewNullSink(), false) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this added for fixing the tests you thought were broken? lol do we still need it? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this is to remove the logging warnings about statsd failing to reach the localhost. Just to reduce the noise :) |
||
dbUpdater = &events.DBUpdater{ | ||
DB: defaultBoltDB, | ||
} | ||
|
@@ -132,7 +135,7 @@ func setup(t *testing.T) *vcsmocks.MockClient { | |
applyCommandRunner = events.NewApplyCommandRunner( | ||
vcsClient, | ||
false, | ||
false, | ||
applyCommandLocker, | ||
commitUpdater, | ||
projectCommandBuilder, | ||
projectCommandRunner, | ||
|
@@ -296,21 +299,37 @@ func TestRunCommentCommand_DisableApplyAllDisabled(t *testing.T) { | |
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "**Error:** Running `atlantis apply` without flags is disabled. You must specify which project to apply via the `-d <dir>`, `-w <workspace>` or `-p <project name>` flags.", "apply") | ||
} | ||
|
||
func TestRunCommentCommand_ApplyDisabled(t *testing.T) { | ||
t.Log("if \"atlantis apply\" is run and this is disabled globally atlantis should" + | ||
" comment saying that this is not allowed") | ||
vcsClient := setup(t) | ||
applyCommandRunner.DisableApply = true | ||
defer func() { applyCommandRunner.DisableApply = false }() | ||
pull := &github.PullRequest{ | ||
State: github.String("open"), | ||
} | ||
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState, Num: fixtures.Pull.Num} | ||
When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil) | ||
When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil) | ||
func TestRunCommentCommand_IsApplyDisabled(t *testing.T) { | ||
t.Run("if \"atlantis apply\" is disabled globally atlantis should"+ | ||
" comment saying that this is not allowed", func(t *testing.T) { | ||
vcsClient := setup(t) | ||
|
||
ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, modelPull.Num, &events.CommentCommand{Name: models.ApplyCommand}) | ||
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "**Error:** Running `atlantis apply` is disabled.", "apply") | ||
pull := &github.PullRequest{ | ||
State: github.String("open"), | ||
} | ||
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState, Num: fixtures.Pull.Num} | ||
When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil) | ||
When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil) | ||
When(applyCommandLocker.IsDisabled(matchers.AnyPtrToModelsCommandContext())).ThenReturn(true) | ||
|
||
ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, modelPull.Num, &events.CommentCommand{Name: models.ApplyCommand}) | ||
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "**Error:** Running `atlantis apply` is disabled.", "apply") | ||
}) | ||
|
||
t.Run("if \"atlantis apply\" is enabled globally atlantis should perform apply", func(t *testing.T) { | ||
vcsClient := setup(t) | ||
|
||
pull := &github.PullRequest{ | ||
State: github.String("open"), | ||
} | ||
modelPull := models.PullRequest{BaseRepo: fixtures.GithubRepo, State: models.OpenPullState, Num: fixtures.Pull.Num} | ||
When(githubGetter.GetPullRequest(fixtures.GithubRepo, fixtures.Pull.Num)).ThenReturn(pull, nil) | ||
When(eventParsing.ParseGithubPull(pull)).ThenReturn(modelPull, modelPull.BaseRepo, fixtures.GithubRepo, nil) | ||
When(applyCommandLocker.IsDisabled(matchers.AnyPtrToModelsCommandContext())).ThenReturn(false) | ||
|
||
ch.RunCommentCommand(fixtures.GithubRepo, nil, nil, fixtures.User, modelPull.Num, &events.CommentCommand{Name: models.ApplyCommand}) | ||
vcsClient.VerifyWasCalledOnce().CreateComment(fixtures.GithubRepo, modelPull.Num, "Ran Apply for 0 projects:\n\n\n\n", "apply") | ||
}) | ||
} | ||
|
||
func TestRunCommentCommand_DisableDisableAutoplan(t *testing.T) { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -17,15 +17,17 @@ import ( | |
|
||
// BoltDB is a database using BoltDB | ||
type BoltDB struct { | ||
db *bolt.DB | ||
locksBucketName []byte | ||
pullsBucketName []byte | ||
db *bolt.DB | ||
locksBucketName []byte | ||
pullsBucketName []byte | ||
globalLocksBucketName []byte | ||
} | ||
|
||
const ( | ||
locksBucketName = "runLocks" | ||
pullsBucketName = "pulls" | ||
pullKeySeparator = "::" | ||
locksBucketName = "runLocks" | ||
pullsBucketName = "pulls" | ||
globalLocksBucketName = "global" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. missed this earlier but let's name it |
||
pullKeySeparator = "::" | ||
) | ||
|
||
// New returns a valid locker. We need to be able to write to dataDir | ||
|
@@ -50,18 +52,31 @@ func New(dataDir string) (*BoltDB, error) { | |
if _, err = tx.CreateBucketIfNotExists([]byte(pullsBucketName)); err != nil { | ||
return errors.Wrapf(err, "creating bucket %q", pullsBucketName) | ||
} | ||
if _, err = tx.CreateBucketIfNotExists([]byte(globalLocksBucketName)); err != nil { | ||
return errors.Wrapf(err, "creating bucket %q", globalLocksBucketName) | ||
} | ||
return nil | ||
}) | ||
if err != nil { | ||
return nil, errors.Wrap(err, "starting BoltDB") | ||
} | ||
// todo: close BoltDB when server is sigtermed | ||
return &BoltDB{db: db, locksBucketName: []byte(locksBucketName), pullsBucketName: []byte(pullsBucketName)}, nil | ||
return &BoltDB{ | ||
db: db, | ||
locksBucketName: []byte(locksBucketName), | ||
pullsBucketName: []byte(pullsBucketName), | ||
globalLocksBucketName: []byte(globalLocksBucketName), | ||
}, nil | ||
} | ||
|
||
// NewWithDB is used for testing. | ||
func NewWithDB(db *bolt.DB, bucket string) (*BoltDB, error) { | ||
return &BoltDB{db: db, locksBucketName: []byte(bucket), pullsBucketName: []byte(pullsBucketName)}, nil | ||
func NewWithDB(db *bolt.DB, bucket string, globalBucket string) (*BoltDB, error) { | ||
return &BoltDB{ | ||
db: db, | ||
locksBucketName: []byte(bucket), | ||
pullsBucketName: []byte(pullsBucketName), | ||
globalLocksBucketName: []byte(globalBucket), | ||
}, nil | ||
} | ||
|
||
// TryLock attempts to create a new lock. If the lock is | ||
|
@@ -155,6 +170,87 @@ func (b *BoltDB) List() ([]models.ProjectLock, error) { | |
return locks, nil | ||
} | ||
|
||
// LockCommand attempts to create a new lock for a CommandName. If the lock is | ||
// not present, it will create a lock and return a pointer to it. | ||
// If the lock already exists, it will just return pointer to the existing lock | ||
// If lock creation fails it will return nil | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Probably should update this to reflect the new changes. |
||
func (b *BoltDB) LockCommand(cmdName models.CommandName, lockTime time.Time) (*models.CommandLock, error) { | ||
lock := models.CommandLock{ | ||
CommandName: cmdName, | ||
Time: lockTime, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
newLockSerialized, _ := json.Marshal(lock) | ||
transactionErr := b.db.Update(func(tx *bolt.Tx) error { | ||
bucket := tx.Bucket(b.globalLocksBucketName) | ||
|
||
currLockSerialized := bucket.Get([]byte(b.commandLockKey(cmdName))) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
if currLockSerialized == nil { | ||
|
||
// This will only error on readonly buckets, it's okay to ignore. | ||
bucket.Put([]byte(b.commandLockKey(cmdName)), newLockSerialized) // nolint: errcheck | ||
return nil | ||
} | ||
|
||
// otherwise the lock fails, return to caller the run that's holding the lock | ||
if err := json.Unmarshal(currLockSerialized, &lock); err != nil { | ||
return errors.Wrap(err, "failed to deserialize current lock") | ||
} | ||
return nil | ||
}) | ||
|
||
if transactionErr != nil { | ||
return nil, errors.Wrap(transactionErr, "DB transaction failed") | ||
} | ||
|
||
return &lock, nil | ||
} | ||
|
||
// UnlockApplyCmd removes CommandName lock if present | ||
func (b *BoltDB) UnlockCommand(cmdName models.CommandName) error { | ||
transactionErr := b.db.Update(func(tx *bolt.Tx) error { | ||
bucket := tx.Bucket(b.globalLocksBucketName) | ||
return bucket.Delete([]byte(b.commandLockKey(cmdName))) | ||
}) | ||
|
||
if transactionErr != nil { | ||
return errors.Wrap(transactionErr, "DB transaction failed") | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// CheckCommandLock checks if CommandName lock was set. | ||
// If the lock exists return the pointer to the lock object, otherwise return nil | ||
func (b *BoltDB) CheckCommandLock(cmdName models.CommandName) (*models.CommandLock, error) { | ||
cmdLock := models.CommandLock{ | ||
CommandName: cmdName, | ||
} | ||
|
||
found := false | ||
|
||
err := b.db.View(func(tx *bolt.Tx) error { | ||
bucket := tx.Bucket(b.globalLocksBucketName) | ||
|
||
serializedLock := bucket.Get([]byte(b.commandLockKey(cmdName))) | ||
|
||
if serializedLock != nil { | ||
if err := json.Unmarshal(serializedLock, &cmdLock); err != nil { | ||
return errors.Wrap(err, "failed to deserialize UserConfig") | ||
} | ||
found = true | ||
} | ||
|
||
return nil | ||
}) | ||
|
||
if found { | ||
return &cmdLock, err | ||
} | ||
|
||
return nil, err | ||
} | ||
|
||
// UnlockByPull deletes all locks associated with that pull request and returns them. | ||
func (b *BoltDB) UnlockByPull(repoFullName string, pullNum int) ([]models.ProjectLock, error) { | ||
var locks []models.ProjectLock | ||
|
@@ -355,6 +451,10 @@ func (b *BoltDB) pullKey(pull models.PullRequest) ([]byte, error) { | |
nil | ||
} | ||
|
||
func (b *BoltDB) commandLockKey(cmdName models.CommandName) string { | ||
return fmt.Sprintf("%s/lock", cmdName) | ||
} | ||
|
||
func (b *BoltDB) lockKey(p models.Project, workspace string) string { | ||
return fmt.Sprintf("%s/%s/%s", p.RepoFullName, p.Path, workspace) | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 methodisLocked
(disabled also doesn't fit well if the name of the struct doesn't include that)