Merge pull request #17422 from hashicorp/jbardin/state-locking

Add StateLocker interface to backend operations

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
 

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)
 	}()

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()
 

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)
 	}

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()
 

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() {

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,125 @@ 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 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
+	// 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.
+// 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,
+	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
 }

command/meta_backend.go

@@ -583,18 +583,11 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) {
 	}
 
 	if m.stateLock {
-		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())
-		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 clistate.Unlock(realMgr, lockID, m.Ui, m.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	if err := realMgr.RefreshState(); err != nil {
@@ -964,18 +957,11 @@ func (m *Meta) backend_C_r_s(
 	}
 
 	if m.stateLock {
-		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())
-		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 clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// Store the metadata in our saved state location
@@ -1047,18 +1033,11 @@ func (m *Meta) backend_C_r_S_changed(
 	}
 
 	if m.stateLock {
-		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())
-		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 clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// Update the backend state
@@ -1190,18 +1169,11 @@ func (m *Meta) backend_C_R_S_unchanged(
 	}
 
 	if m.stateLock {
-		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())
-		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 clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// Unset the remote state

command/meta_backend_migrate.go

@@ -233,28 +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()
 
-		lockInfoOne := state.NewLockInfo()
-		lockInfoOne.Operation = "migration"
-		lockInfoOne.Info = "source state"
-
-		lockIDOne, err := clistate.Lock(lockCtx, stateOne, lockInfoOne, 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 clistate.Unlock(stateOne, lockIDOne, m.Ui, m.Colorize())
+		defer lockerOne.Unlock(nil)
 
-		lockInfoTwo := state.NewLockInfo()
-		lockInfoTwo.Operation = "migration"
-		lockInfoTwo.Info = "destination state"
-
-		lockIDTwo, err := clistate.Lock(lockCtx, stateTwo, lockInfoTwo, 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 clistate.Unlock(stateTwo, lockIDTwo, m.Ui, m.Colorize())
+		defer lockerTwo.Unlock(nil)
 
 		// We now own a lock, so double check that we have the version
 		// corresponding to the lock.

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"
 )
 
@@ -84,18 +83,12 @@ func (c *TaintCommand) Run(args []string) int {
 	}
 
 	if c.stateLock {
-		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())
-		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 clistate.Unlock(st, lockID, c.Ui, c.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// Get the actual state structure

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
@@ -72,18 +71,12 @@ func (c *UntaintCommand) Run(args []string) int {
 	}
 
 	if c.stateLock {
-		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())
-		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 clistate.Unlock(st, lockID, c.Ui, c.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// Get the actual state structure

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"
 )
@@ -97,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
@@ -109,31 +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()
-
-		// Lock the state if we can
-		lockInfo := state.NewLockInfo()
-		lockInfo.Operation = "workspace delete"
-		lockID, err := clistate.Lock(lockCtx, sMgr, lockInfo, 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.
-		clistate.Unlock(sMgr, lockID, c.Ui, c.Colorize())
-	}
+	// 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 {

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"
@@ -115,18 +114,12 @@ func (c *WorkspaceNewCommand) Run(args []string) int {
 	}
 
 	if c.stateLock {
-		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())
-		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 clistate.Unlock(sMgr, lockID, c.Ui, c.Colorize())
+		defer stateLocker.Unlock(nil)
 	}
 
 	// read the existing state file