From e88bd74bb70801564ba1ffa6e52ecbab78ce8f15 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 22 Feb 2018 20:43:21 -0500 Subject: [PATCH] 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. --- backend/local/backend_apply.go | 9 +- backend/local/backend_plan.go | 11 +-- backend/local/backend_refresh.go | 11 +-- backend/remote-state/consul/client.go | 5 ++ 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 +- 11 files changed, 125 insertions(+), 104 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index da8c684bc3..8a97fc8299 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -59,18 +59,13 @@ func (b *Local) opApply( 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()) + unlock, err := clistate.Lock(lockCtx, opState, op.Type.String(), "", 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) - } + runningOp.Err = unlock(runningOp.Err) }() } diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index ebf0531922..d2956e341d 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -9,12 +9,10 @@ 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" ) @@ -66,18 +64,13 @@ func (b *Local) opPlan( 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()) + unlock, err := clistate.Lock(lockCtx, opState, op.Type.String(), "", 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) - } + runningOp.Err = unlock(runningOp.Err) }() } diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index 959f969a32..a19aa9e5e5 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -8,11 +8,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/config/module" - "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -57,18 +55,13 @@ func (b *Local) opRefresh( 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()) + unlock, err := clistate.Lock(lockCtx, opState, op.Type.String(), "", 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) - } + runningOp.Err = unlock(runningOp.Err) }() } diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index bd37712f3f..8ccb1814e0 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -9,6 +9,7 @@ import ( "errors" "fmt" "log" + "runtime/debug" "sync" "time" @@ -81,6 +82,9 @@ func (c *RemoteClient) Get() (*remote.Payload, error) { c.modifyIndex = pair.ModifyIndex + log.Println("[XXXX] GOT STATE WITH INDEX:", c.modifyIndex) + log.Println(string(debug.Stack())) + payload := pair.Value // If the payload starts with 0x1f, it's gzip, not json if len(pair.Value) >= 1 && pair.Value[0] == '\x1f' { @@ -280,6 +284,7 @@ func (c *RemoteClient) lock() (string, error) { return "", lockErr } + defer log.Println("[XXXX] OBTAINED CONSUL LOCK") c.lockCh = lockCh err = c.putLockInfo(c.info) diff --git a/command/clistate/state.go b/command/clistate/state.go index f02f053be9..9bd9fe940e 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 2fa4ff542e..726f3b70cc 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 7f60a6605d..be20a44768 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()) + defer unlockOne(nil) - lockInfoTwo := state.NewLockInfo() - lockInfoTwo.Operation = "migration" - lockInfoTwo.Info = "destination state" - - 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 626f88a46f..c3d6c4cc99 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 1eca202779..9a2e43b246 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 99dac86d3b..8a9f792e1f 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 71c9fdc1fc..430ecc58b5 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