From e9a76808df4cc3eaa629969b1a2eb3d7448698dc Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Feb 2018 20:43:21 -0500 Subject: [PATCH 1/3] create clistate.Locker interface Simplify the use of clistate.Lock by creating a clistate.Locker instance, which stores the context of locking a state, to allow unlock to be called without knowledge of how the state was locked. This alows the backend code to bring the needed UI methods to the point where the state is locked, and still unlock the state from an outer scope. Provide a NoopLocker as well, so that callers can always call Unlock without verifying the status of the lock. Add the StateLocker field to the backend.Operation, so that the state lock can be carried between the different function scopes of the backend code. This will allow the backend context to lock the state before it's read, while allowing the different operations to unlock the state when they complete. --- backend/backend.go | 5 ++ backend/local/backend.go | 13 ++++ backend/local/backend_apply.go | 20 ------ backend/local/backend_local.go | 12 ++++ backend/local/backend_plan.go | 22 ------ backend/local/backend_refresh.go | 22 ------ command/clistate/state.go | 114 +++++++++++++++++++++++++------ command/meta_backend.go | 32 +++------ command/meta_backend_migrate.go | 16 ++--- command/taint.go | 7 +- command/untaint.go | 8 +-- command/workspace_delete.go | 8 +-- command/workspace_new.go | 8 +-- 13 files changed, 144 insertions(+), 143 deletions(-) diff --git a/backend/backend.go b/backend/backend.go index f67d60ab60cb..dfeb80ef6f87 100644 --- a/backend/backend.go +++ b/backend/backend.go @@ -9,6 +9,7 @@ import ( "errors" "time" + "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/config/module" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" @@ -135,6 +136,10 @@ type Operation struct { // state.Lockers for its duration, and Unlock when complete. LockState bool + // StateLocker is used to lock the state while providing UI feedback to the + // user. This will be supplied by the Backend itself. + StateLocker clistate.Locker + // The duration to retry obtaining a State lock. StateLockTimeout time.Duration diff --git a/backend/local/backend.go b/backend/local/backend.go index b42aedb17cc7..eb4eff4b00ea 100644 --- a/backend/local/backend.go +++ b/backend/local/backend.go @@ -13,6 +13,7 @@ import ( "sync" "github.com/hashicorp/terraform/backend" + "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/helper/schema" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" @@ -260,12 +261,24 @@ func (b *Local) Operation(ctx context.Context, op *backend.Operation) (*backend. cancelCtx, cancel := context.WithCancel(context.Background()) runningOp.Cancel = cancel + if op.LockState { + op.StateLocker = clistate.NewLocker(stopCtx, op.StateLockTimeout, b.CLI, b.Colorize()) + } else { + op.StateLocker = clistate.NewNoopLocker() + } + // Do it go func() { defer done() defer stop() defer cancel() + // the state was locked during context creation, unlock the state when + // the operation completes + defer func() { + runningOp.Err = op.StateLocker.Unlock(runningOp.Err) + }() + defer b.opLock.Unlock() f(stopCtx, cancelCtx, op, runningOp) }() diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index da8c684bc3b5..102d40c39e35 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -11,7 +11,6 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" - "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/command/format" "github.com/hashicorp/terraform/config/module" "github.com/hashicorp/terraform/state" @@ -55,25 +54,6 @@ func (b *Local) opApply( return } - if op.LockState { - lockCtx, cancel := context.WithTimeout(stopCtx, op.StateLockTimeout) - defer cancel() - - lockInfo := state.NewLockInfo() - lockInfo.Operation = op.Type.String() - lockID, err := clistate.Lock(lockCtx, opState, lockInfo, b.CLI, b.Colorize()) - if err != nil { - runningOp.Err = errwrap.Wrapf("Error locking state: {{err}}", err) - return - } - - defer func() { - if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { - runningOp.Err = multierror.Append(runningOp.Err, err) - } - }() - } - // Setup the state runningOp.State = tfCtx.State() diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index aa056a1a16df..d26fefa57d60 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -1,9 +1,11 @@ package local import ( + "context" "errors" "log" + "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/command/format" "github.com/hashicorp/terraform/tfdiags" @@ -20,6 +22,12 @@ func (b *Local) Context(op *backend.Operation) (*terraform.Context, state.State, // to ask for input/validate. op.Type = backend.OperationTypeInvalid + if op.LockState { + op.StateLocker = clistate.NewLocker(context.Background(), op.StateLockTimeout, b.CLI, b.Colorize()) + } else { + op.StateLocker = clistate.NewNoopLocker() + } + return b.context(op) } @@ -30,6 +38,10 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, state.State, return nil, nil, errwrap.Wrapf("Error loading state: {{err}}", err) } + if err := op.StateLocker.Lock(s, op.Type.String()); err != nil { + return nil, nil, errwrap.Wrapf("Error locking state: {{err}}", err) + } + if err := s.RefreshState(); err != nil { return nil, nil, errwrap.Wrapf("Error loading state: {{err}}", err) } diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index ebf053192265..00c0a859d563 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -9,12 +9,9 @@ import ( "strings" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" - "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/command/format" "github.com/hashicorp/terraform/config/module" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -62,25 +59,6 @@ func (b *Local) opPlan( return } - if op.LockState { - lockCtx, cancel := context.WithTimeout(stopCtx, op.StateLockTimeout) - defer cancel() - - lockInfo := state.NewLockInfo() - lockInfo.Operation = op.Type.String() - lockID, err := clistate.Lock(lockCtx, opState, lockInfo, b.CLI, b.Colorize()) - if err != nil { - runningOp.Err = errwrap.Wrapf("Error locking state: {{err}}", err) - return - } - - defer func() { - if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { - runningOp.Err = multierror.Append(runningOp.Err, err) - } - }() - } - // Setup the state runningOp.State = tfCtx.State() diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index 959f969a3220..b5ec9aa6d4ce 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -8,11 +8,8 @@ import ( "strings" "github.com/hashicorp/errwrap" - "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" - "github.com/hashicorp/terraform/command/clistate" "github.com/hashicorp/terraform/config/module" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -53,25 +50,6 @@ func (b *Local) opRefresh( return } - if op.LockState { - lockCtx, cancel := context.WithTimeout(stopCtx, op.StateLockTimeout) - defer cancel() - - lockInfo := state.NewLockInfo() - lockInfo.Operation = op.Type.String() - lockID, err := clistate.Lock(lockCtx, opState, lockInfo, b.CLI, b.Colorize()) - if err != nil { - runningOp.Err = errwrap.Wrapf("Error locking state: {{err}}", err) - return - } - - defer func() { - if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { - runningOp.Err = multierror.Append(runningOp.Err, err) - } - }() - } - // Set our state runningOp.State = opState.State() if runningOp.State.Empty() || !runningOp.State.HasResources() { diff --git a/command/clistate/state.go b/command/clistate/state.go index f02f053be93c..9bd9fe940e8e 100644 --- a/command/clistate/state.go +++ b/command/clistate/state.go @@ -8,9 +8,11 @@ import ( "context" "fmt" "strings" + "sync" "time" "github.com/hashicorp/errwrap" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/helper/slowmessage" "github.com/hashicorp/terraform/state" "github.com/mitchellh/cli" @@ -48,47 +50,119 @@ that no one else is holding a lock. ` ) -// Lock locks the given state and outputs to the user if locking -// is taking longer than the threshold. The lock is retried until the context -// is cancelled. -func Lock(ctx context.Context, s state.State, info *state.LockInfo, ui cli.Ui, color *colorstring.Colorize) (string, error) { - var lockID string +// Locker allows for more convenient locking, by creating the timeout context +// and LockInfo for the caller, while storing any related data required for +// Unlock. +type Locker interface { + // Lock the provided state, storing the reason string in the LockInfo. + Lock(s state.State, reason string) error + // Unlock the previously locked state. + // An optional error can be passed in, and will be combined with any error + // from the Unlock operation. + Unlock(error) error +} + +type locker struct { + mu sync.Mutex + ctx context.Context + timeout time.Duration + state state.State + ui cli.Ui + color *colorstring.Colorize + lockID string +} + +// Create a new Locker. +// The provided context will be used for lock cancellation, and combined with +// the timeout duration. Lock progress will be be reported to the user through +// the provided UI. +func NewLocker( + ctx context.Context, + timeout time.Duration, + ui cli.Ui, + color *colorstring.Colorize) Locker { + + l := &locker{ + ctx: ctx, + timeout: timeout, + ui: ui, + color: color, + } + return l +} + +// Locker locks the given state and outputs to the user if locking is taking +// longer than the threshold. The lock is retried until the context is +// cancelled. +func (l *locker) Lock(s state.State, reason string) error { + l.mu.Lock() + defer l.mu.Unlock() + + l.state = s + + ctx, cancel := context.WithTimeout(l.ctx, l.timeout) + defer cancel() + + lockInfo := state.NewLockInfo() + lockInfo.Operation = reason err := slowmessage.Do(LockThreshold, func() error { - id, err := state.LockWithContext(ctx, s, info) - lockID = id + id, err := state.LockWithContext(ctx, s, lockInfo) + l.lockID = id return err }, func() { - if ui != nil { - ui.Output(color.Color(LockMessage)) + if l.ui != nil { + l.ui.Output(l.color.Color(LockMessage)) } }) if err != nil { - err = errwrap.Wrapf(strings.TrimSpace(LockErrorMessage), err) + return errwrap.Wrapf(strings.TrimSpace(LockErrorMessage), err) } - return lockID, err + return nil } -// Unlock unlocks the given state and outputs to the user if the -// unlock fails what can be done. -func Unlock(s state.State, id string, ui cli.Ui, color *colorstring.Colorize) error { +func (l *locker) Unlock(parentErr error) error { + l.mu.Lock() + defer l.mu.Unlock() + + if l.lockID == "" { + return parentErr + } + err := slowmessage.Do(LockThreshold, func() error { - return s.Unlock(id) + return l.state.Unlock(l.lockID) }, func() { - if ui != nil { - ui.Output(color.Color(UnlockMessage)) + if l.ui != nil { + l.ui.Output(l.color.Color(UnlockMessage)) } }) if err != nil { - ui.Output(color.Color(fmt.Sprintf( + l.ui.Output(l.color.Color(fmt.Sprintf( "\n"+strings.TrimSpace(UnlockErrorMessage)+"\n", err))) - err = fmt.Errorf( - "Error releasing the state lock. Please see the longer error message above.") + if parentErr != nil { + parentErr = multierror.Append(parentErr, err) + } } + return parentErr + +} + +type noopLocker struct{} + +// NewNoopLocker returns a valid Locker that does nothing. +func NewNoopLocker() Locker { + return noopLocker{} +} + +func (l noopLocker) Lock(state.State, string) error { + return nil +} + +func (l noopLocker) Unlock(err error) error { return err } diff --git a/command/meta_backend.go b/command/meta_backend.go index 2fa4ff542ef6..726f3b70ccec 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -586,15 +586,11 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) { lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "backend from plan" - - lockID, err := clistate.Lock(lockCtx, realMgr, lockInfo, m.Ui, m.Colorize()) + unlock, err := clistate.Lock(lockCtx, realMgr, "backend from plan", "", m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(realMgr, lockID, m.Ui, m.Colorize()) + defer unlock(nil) } if err := realMgr.RefreshState(); err != nil { @@ -967,15 +963,11 @@ func (m *Meta) backend_C_r_s( lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "backend from config" - - lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, m.Ui, m.Colorize()) + unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) + defer unlock(nil) } // Store the metadata in our saved state location @@ -1050,15 +1042,11 @@ func (m *Meta) backend_C_r_S_changed( lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "backend from config" - - lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, m.Ui, m.Colorize()) + unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) + defer unlock(nil) } // Update the backend state @@ -1193,15 +1181,11 @@ func (m *Meta) backend_C_R_S_unchanged( lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "backend from config" - - lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, m.Ui, m.Colorize()) + unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) + defer unlock(nil) } // Unset the remote state diff --git a/command/meta_backend_migrate.go b/command/meta_backend_migrate.go index 7f60a6605d9c..be20a44768f8 100644 --- a/command/meta_backend_migrate.go +++ b/command/meta_backend_migrate.go @@ -236,25 +236,17 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error { lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) defer cancel() - lockInfoOne := state.NewLockInfo() - lockInfoOne.Operation = "migration" - lockInfoOne.Info = "source state" - - lockIDOne, err := clistate.Lock(lockCtx, stateOne, lockInfoOne, m.Ui, m.Colorize()) + unlockOne, err := clistate.Lock(lockCtx, stateOne, "migration", "source state", m.Ui, m.Colorize()) if err != nil { return fmt.Errorf("Error locking source state: %s", err) } - defer clistate.Unlock(stateOne, lockIDOne, m.Ui, m.Colorize()) - - lockInfoTwo := state.NewLockInfo() - lockInfoTwo.Operation = "migration" - lockInfoTwo.Info = "destination state" + defer unlockOne(nil) - lockIDTwo, err := clistate.Lock(lockCtx, stateTwo, lockInfoTwo, m.Ui, m.Colorize()) + unlockTwo, err := clistate.Lock(lockCtx, stateTwo, "migration", "destination state", m.Ui, m.Colorize()) if err != nil { return fmt.Errorf("Error locking destination state: %s", err) } - defer clistate.Unlock(stateTwo, lockIDTwo, m.Ui, m.Colorize()) + defer unlockTwo(nil) // We now own a lock, so double check that we have the version // corresponding to the lock. diff --git a/command/taint.go b/command/taint.go index 626f88a46f38..c3d6c4cc9981 100644 --- a/command/taint.go +++ b/command/taint.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/hashicorp/terraform/command/clistate" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -87,15 +86,13 @@ func (c *TaintCommand) Run(args []string) int { lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) defer cancel() - lockInfo := state.NewLockInfo() - lockInfo.Operation = "taint" - lockID, err := clistate.Lock(lockCtx, st, lockInfo, c.Ui, c.Colorize()) + unlock, err := clistate.Lock(lockCtx, st, "taint", "", c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer clistate.Unlock(st, lockID, c.Ui, c.Colorize()) + defer unlock(nil) } // Get the actual state structure diff --git a/command/untaint.go b/command/untaint.go index 1eca202779bc..9a2e43b24694 100644 --- a/command/untaint.go +++ b/command/untaint.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/hashicorp/terraform/command/clistate" - "github.com/hashicorp/terraform/state" ) // UntaintCommand is a cli.Command implementation that manually untaints @@ -75,15 +74,12 @@ func (c *UntaintCommand) Run(args []string) int { lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) defer cancel() - lockInfo := state.NewLockInfo() - lockInfo.Operation = "untaint" - lockID, err := clistate.Lock(lockCtx, st, lockInfo, c.Ui, c.Colorize()) + unlock, err := clistate.Lock(lockCtx, st, "untaint", "", c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - - defer clistate.Unlock(st, lockID, c.Ui, c.Colorize()) + defer unlock(nil) } // Get the actual state structure diff --git a/command/workspace_delete.go b/command/workspace_delete.go index 99dac86d3b2d..8a9f792e1fda 100644 --- a/command/workspace_delete.go +++ b/command/workspace_delete.go @@ -6,7 +6,6 @@ import ( "strings" "github.com/hashicorp/terraform/command/clistate" - "github.com/hashicorp/terraform/state" "github.com/mitchellh/cli" "github.com/posener/complete" ) @@ -114,10 +113,7 @@ func (c *WorkspaceDeleteCommand) Run(args []string) int { lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "workspace delete" - lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, c.Ui, c.Colorize()) + unlock, err := clistate.Lock(lockCtx, sMgr, "workspace delete", "", c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 @@ -132,7 +128,7 @@ func (c *WorkspaceDeleteCommand) Run(args []string) int { // state deletion, i.e. in a CI environment. Adding Delete() as a // required method of States would allow the removal of the resource to // be delegated from the Backend to the State itself. - clistate.Unlock(sMgr, lockID, c.Ui, c.Colorize()) + unlock(nil) } err = b.DeleteState(delEnv) diff --git a/command/workspace_new.go b/command/workspace_new.go index 71c9fdc1fcc2..430ecc58b5b9 100644 --- a/command/workspace_new.go +++ b/command/workspace_new.go @@ -7,7 +7,6 @@ import ( "strings" "github.com/hashicorp/terraform/command/clistate" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" "github.com/posener/complete" @@ -118,15 +117,12 @@ func (c *WorkspaceNewCommand) Run(args []string) int { lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) defer cancel() - // Lock the state if we can - lockInfo := state.NewLockInfo() - lockInfo.Operation = "workspace new" - lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, c.Ui, c.Colorize()) + unlock, err := clistate.Lock(lockCtx, sMgr, "workspace_new", "", c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer clistate.Unlock(sMgr, lockID, c.Ui, c.Colorize()) + defer unlock(nil) } // read the existing state file From bdd475e14919f0c56f95c1a155464e25733c35e7 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 23 Feb 2018 11:28:47 -0500 Subject: [PATCH 2/3] use the new clistate.Locker in commands Use the new StateLocker field to provide a wrapper for locking the state during terraform.Context creation in the commands that directly manipulate the state. --- command/meta_backend.go | 36 +++++++++------------------ command/meta_backend_migrate.go | 15 ++++++------ command/taint.go | 10 +++----- command/untaint.go | 9 +++---- command/workspace_delete.go | 43 ++++++++++++++++----------------- command/workspace_new.go | 9 +++---- 6 files changed, 49 insertions(+), 73 deletions(-) diff --git a/command/meta_backend.go b/command/meta_backend.go index 726f3b70ccec..1bee95339c1d 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -583,14 +583,11 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) { } if m.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, realMgr, "backend from plan", "", m.Ui, m.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), m.stateLockTimeout, m.Ui, m.Colorize()) + if err := stateLocker.Lock(realMgr, "backend from plan"); err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer unlock(nil) + defer stateLocker.Unlock(nil) } if err := realMgr.RefreshState(); err != nil { @@ -960,14 +957,11 @@ func (m *Meta) backend_C_r_s( } if m.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), m.stateLockTimeout, m.Ui, m.Colorize()) + if err := stateLocker.Lock(sMgr, "backend from plan"); err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer unlock(nil) + defer stateLocker.Unlock(nil) } // Store the metadata in our saved state location @@ -1039,14 +1033,11 @@ func (m *Meta) backend_C_r_S_changed( } if m.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), m.stateLockTimeout, m.Ui, m.Colorize()) + if err := stateLocker.Lock(sMgr, "backend from plan"); err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer unlock(nil) + defer stateLocker.Unlock(nil) } // Update the backend state @@ -1178,14 +1169,11 @@ func (m *Meta) backend_C_R_S_unchanged( } if m.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, sMgr, "backend from config", "", m.Ui, m.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), m.stateLockTimeout, m.Ui, m.Colorize()) + if err := stateLocker.Lock(sMgr, "backend from plan"); err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer unlock(nil) + defer stateLocker.Unlock(nil) } // Unset the remote state diff --git a/command/meta_backend_migrate.go b/command/meta_backend_migrate.go index be20a44768f8..0c3610a8a13d 100644 --- a/command/meta_backend_migrate.go +++ b/command/meta_backend_migrate.go @@ -233,20 +233,19 @@ func (m *Meta) backendMigrateState_s_s(opts *backendMigrateOpts) error { } if m.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), m.stateLockTimeout) - defer cancel() + lockCtx := context.Background() - unlockOne, err := clistate.Lock(lockCtx, stateOne, "migration", "source state", m.Ui, m.Colorize()) - if err != nil { + lockerOne := clistate.NewLocker(lockCtx, m.stateLockTimeout, m.Ui, m.Colorize()) + if err := lockerOne.Lock(stateOne, "migration source state"); err != nil { return fmt.Errorf("Error locking source state: %s", err) } - defer unlockOne(nil) + defer lockerOne.Unlock(nil) - unlockTwo, err := clistate.Lock(lockCtx, stateTwo, "migration", "destination state", m.Ui, m.Colorize()) - if err != nil { + lockerTwo := clistate.NewLocker(lockCtx, m.stateLockTimeout, m.Ui, m.Colorize()) + if err := lockerTwo.Lock(stateTwo, "migration destination state"); err != nil { return fmt.Errorf("Error locking destination state: %s", err) } - defer unlockTwo(nil) + defer lockerTwo.Unlock(nil) // We now own a lock, so double check that we have the version // corresponding to the lock. diff --git a/command/taint.go b/command/taint.go index c3d6c4cc9981..4dc16e60cd5a 100644 --- a/command/taint.go +++ b/command/taint.go @@ -83,16 +83,12 @@ func (c *TaintCommand) Run(args []string) int { } if c.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, st, "taint", "", c.Ui, c.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize()) + if err := stateLocker.Lock(st, "taint"); err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - - defer unlock(nil) + defer stateLocker.Unlock(nil) } // Get the actual state structure diff --git a/command/untaint.go b/command/untaint.go index 9a2e43b24694..39d047fd677d 100644 --- a/command/untaint.go +++ b/command/untaint.go @@ -71,15 +71,12 @@ func (c *UntaintCommand) Run(args []string) int { } if c.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, st, "untaint", "", c.Ui, c.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize()) + if err := stateLocker.Lock(st, "untaint"); err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer unlock(nil) + defer stateLocker.Unlock(nil) } // Get the actual state structure diff --git a/command/workspace_delete.go b/command/workspace_delete.go index 8a9f792e1fda..1e1eb1182f7f 100644 --- a/command/workspace_delete.go +++ b/command/workspace_delete.go @@ -96,6 +96,17 @@ func (c *WorkspaceDeleteCommand) Run(args []string) int { return 1 } + var stateLocker clistate.Locker + if c.stateLock { + stateLocker = clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize()) + if err := stateLocker.Lock(sMgr, "workspace_delete"); err != nil { + c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) + return 1 + } + } else { + stateLocker = clistate.NewNoopLocker() + } + if err := sMgr.RefreshState(); err != nil { c.Ui.Error(err.Error()) return 1 @@ -108,28 +119,16 @@ func (c *WorkspaceDeleteCommand) Run(args []string) int { return 1 } - // Honor the lock request, for consistency and one final safety check. - if c.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, sMgr, "workspace delete", "", c.Ui, c.Colorize()) - if err != nil { - c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) - return 1 - } - - // We need to release the lock just before deleting the state, in case - // the backend can't remove the resource while holding the lock. This - // is currently true for Windows local files. - // - // TODO: While there is little safety in locking while deleting the - // state, it might be nice to be able to coordinate processes around - // state deletion, i.e. in a CI environment. Adding Delete() as a - // required method of States would allow the removal of the resource to - // be delegated from the Backend to the State itself. - unlock(nil) - } + // We need to release the lock just before deleting the state, in case + // the backend can't remove the resource while holding the lock. This + // is currently true for Windows local files. + // + // TODO: While there is little safety in locking while deleting the + // state, it might be nice to be able to coordinate processes around + // state deletion, i.e. in a CI environment. Adding Delete() as a + // required method of States would allow the removal of the resource to + // be delegated from the Backend to the State itself. + stateLocker.Unlock(nil) err = b.DeleteState(delEnv) if err != nil { diff --git a/command/workspace_new.go b/command/workspace_new.go index 430ecc58b5b9..810a776fc45a 100644 --- a/command/workspace_new.go +++ b/command/workspace_new.go @@ -114,15 +114,12 @@ func (c *WorkspaceNewCommand) Run(args []string) int { } if c.stateLock { - lockCtx, cancel := context.WithTimeout(context.Background(), c.stateLockTimeout) - defer cancel() - - unlock, err := clistate.Lock(lockCtx, sMgr, "workspace_new", "", c.Ui, c.Colorize()) - if err != nil { + stateLocker := clistate.NewLocker(context.Background(), c.stateLockTimeout, c.Ui, c.Colorize()) + if err := stateLocker.Lock(sMgr, "workspace_delete"); err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer unlock(nil) + defer stateLocker.Unlock(nil) } // read the existing state file From 2b97585d460da65c1eaaa3e8280a454a0d4d1636 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 27 Feb 2018 10:49:06 -0500 Subject: [PATCH 3/3] improve clistate.Locker docs --- command/clistate/state.go | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/command/clistate/state.go b/command/clistate/state.go index 9bd9fe940e8e..a17f96c342af 100644 --- a/command/clistate/state.go +++ b/command/clistate/state.go @@ -50,9 +50,15 @@ that no one else is holding a lock. ` ) -// Locker allows for more convenient locking, by creating the timeout context -// and LockInfo for the caller, while storing any related data required for -// Unlock. +// Locker allows for more convenient usage of the lower-level state.Locker +// implementations. +// The state.Locker API requires passing in a state.LockInfo struct. Locker +// implementations are expected to create the required LockInfo struct when +// Lock is called, populate the Operation field with the "reason" string +// provided, and pass that on to the underlying state.Locker. +// Locker implementations are also expected to store any state required to call +// Unlock, which is at a minimum the LockID string returned by the +// state.Locker. type Locker interface { // Lock the provided state, storing the reason string in the LockInfo. Lock(s state.State, reason string) error @@ -73,9 +79,9 @@ type locker struct { } // Create a new Locker. -// The provided context will be used for lock cancellation, and combined with -// the timeout duration. Lock progress will be be reported to the user through -// the provided UI. +// This Locker uses state.LockWithContext to retry the lock until the provided +// timeout is reached, or the context is canceled. Lock progress will be be +// reported to the user through the provided UI. func NewLocker( ctx context.Context, timeout time.Duration,