From 200c8de4e9eadba52620e3ea9aef9e40c25fb086 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 17:43:42 -0500 Subject: [PATCH 01/15] Update the state.Locker interface Remove CacheState rather than update it, since it's no longer used. --- state/backup.go | 10 +- state/cache.go | 290 -------------------------------------------- state/cache_test.go | 118 ------------------ state/local.go | 21 ---- state/state.go | 60 ++++++++- 5 files changed, 61 insertions(+), 438 deletions(-) delete mode 100644 state/cache.go delete mode 100644 state/cache_test.go diff --git a/state/backup.go b/state/backup.go index bb935eb09..15d8f6f3e 100644 --- a/state/backup.go +++ b/state/backup.go @@ -42,16 +42,16 @@ func (s *BackupState) PersistState() error { } // all states get wrapped by BackupState, so it has to be a Locker -func (s *BackupState) Lock(reason string) error { +func (s *BackupState) Lock(info *LockInfo) (string, error) { if s, ok := s.Real.(Locker); ok { - return s.Lock(reason) + return s.Lock(info) } - return nil + return "", nil } -func (s *BackupState) Unlock() error { +func (s *BackupState) Unlock(id string) error { if s, ok := s.Real.(Locker); ok { - return s.Unlock() + return s.Unlock(id) } return nil } diff --git a/state/cache.go b/state/cache.go deleted file mode 100644 index 3b2b9739c..000000000 --- a/state/cache.go +++ /dev/null @@ -1,290 +0,0 @@ -package state - -import ( - "fmt" - - multierror "github.com/hashicorp/go-multierror" - "github.com/hashicorp/terraform/terraform" -) - -// CacheState is an implementation of the state interfaces that uses -// a StateReadWriter for a local cache. -type CacheState struct { - Cache CacheStateCache - Durable CacheStateDurable - - refreshResult CacheRefreshResult - state *terraform.State -} - -// Locker implementation. -// Since remote states are wrapped in a CacheState, we need to implement the -// Lock/Unlock methods here to delegate them to the remote client. -func (s *CacheState) Lock(reason string) error { - durable, durableIsLocker := s.Durable.(Locker) - cache, cacheIsLocker := s.Cache.(Locker) - - if durableIsLocker { - if err := durable.Lock(reason); err != nil { - return err - } - } - - // We try to lock the Cache too, which is usually a local file. This also - // protects against multiple local processes if the remote state doesn't - // support locking. - if cacheIsLocker { - if err := cache.Lock(reason); err != nil { - // try to unlock Durable if this failed - if unlockErr := durable.Unlock(); unlockErr != nil { - err = multierror.Append(err, unlockErr) - } - return err - } - } - - return nil -} - -// Unlock unlocks both the Durable and Cache states. -func (s *CacheState) Unlock() error { - durable, durableIsLocker := s.Durable.(Locker) - cache, cacheIsLocker := s.Cache.(Locker) - - var err error - if durableIsLocker { - if unlockErr := durable.Unlock(); unlockErr != nil { - err = multierror.Append(err, unlockErr) - } - } - - if cacheIsLocker { - if unlockErr := cache.Unlock(); unlockErr != nil { - err = multierror.Append(err, unlockErr) - } - } - - return err -} - -// StateReader impl. -func (s *CacheState) State() *terraform.State { - return s.state.DeepCopy() -} - -// WriteState will write and persist the state to the cache. -// -// StateWriter impl. -func (s *CacheState) WriteState(state *terraform.State) error { - if err := s.Cache.WriteState(state); err != nil { - return err - } - - s.state = state - return s.Cache.PersistState() -} - -// RefreshState will refresh both the cache and the durable states. It -// can return a myriad of errors (defined at the top of this file) depending -// on potential conflicts that can occur while doing this. -// -// If the durable state is newer than the local cache, then the local cache -// will be replaced with the durable. -// -// StateRefresher impl. -func (s *CacheState) RefreshState() error { - // Refresh the durable state - if err := s.Durable.RefreshState(); err != nil { - return err - } - - // Refresh the cached state - if err := s.Cache.RefreshState(); err != nil { - return err - } - - // Handle the matrix of cases that can happen when comparing these - // two states. - cached := s.Cache.State() - durable := s.Durable.State() - switch { - case cached == nil && durable == nil: - // Initialized - s.refreshResult = CacheRefreshInit - case cached != nil && durable == nil: - // Cache is newer than remote. Not a big deal, user can just - // persist to get correct state. - s.refreshResult = CacheRefreshLocalNewer - case !cached.HasResources() && durable != nil: - // Cache should be updated since the remote is set but cache isn't - // - // If local is empty then we'll treat it as missing so that - // it can be overwritten by an already-existing remote. This - // allows the user to activate remote state for the first time - // against an already-existing remote state. - s.refreshResult = CacheRefreshUpdateLocal - case durable.Serial < cached.Serial: - // Cache is newer than remote. Not a big deal, user can just - // persist to get correct state. - s.refreshResult = CacheRefreshLocalNewer - case durable.Serial > cached.Serial: - // Cache should be updated since the remote is newer - s.refreshResult = CacheRefreshUpdateLocal - case durable.Serial == cached.Serial: - // They're supposedly equal, verify. - if cached.Equal(durable) { - // Hashes are the same, everything is great - s.refreshResult = CacheRefreshNoop - break - } - - // This is very bad. This means we have two state files that - // have the same serial but have a different hash. We can't - // reconcile this. The most likely cause is parallel apply - // operations. - s.refreshResult = CacheRefreshConflict - - // Return early so we don't updtae the state - return nil - default: - panic("unhandled cache refresh state") - } - - if s.refreshResult == CacheRefreshUpdateLocal { - if err := s.Cache.WriteState(durable); err != nil { - s.refreshResult = CacheRefreshNoop - return err - } - if err := s.Cache.PersistState(); err != nil { - s.refreshResult = CacheRefreshNoop - return err - } - - cached = durable - } - - s.state = cached - - return nil -} - -// RefreshResult returns the result of the last refresh. -func (s *CacheState) RefreshResult() CacheRefreshResult { - return s.refreshResult -} - -// PersistState takes the local cache, assuming it is newer than the remote -// state, and persists it to the durable storage. If you want to challenge the -// assumption that the local state is the latest, call a RefreshState prior -// to this. -// -// StatePersister impl. -func (s *CacheState) PersistState() error { - if err := s.Durable.WriteState(s.state); err != nil { - return err - } - - return s.Durable.PersistState() -} - -// CacheStateCache is the meta-interface that must be implemented for -// the cache for the CacheState. -type CacheStateCache interface { - StateReader - StateWriter - StatePersister - StateRefresher -} - -// CacheStateDurable is the meta-interface that must be implemented for -// the durable storage for CacheState. -type CacheStateDurable interface { - StateReader - StateWriter - StatePersister - StateRefresher -} - -// CacheRefreshResult is used to explain the result of the previous -// RefreshState for a CacheState. -type CacheRefreshResult int - -const ( - // CacheRefreshNoop indicates nothing has happened, - // but that does not indicate an error. Everything is - // just up to date. (Push/Pull) - CacheRefreshNoop CacheRefreshResult = iota - - // CacheRefreshInit indicates that there is no local or - // remote state, and that the state was initialized - CacheRefreshInit - - // CacheRefreshUpdateLocal indicates the local state - // was updated. (Pull) - CacheRefreshUpdateLocal - - // CacheRefreshUpdateRemote indicates the remote state - // was updated. (Push) - CacheRefreshUpdateRemote - - // CacheRefreshLocalNewer means the pull was a no-op - // because the local state is newer than that of the - // server. This means a Push should take place. (Pull) - CacheRefreshLocalNewer - - // CacheRefreshRemoteNewer means the push was a no-op - // because the remote state is newer than that of the - // local state. This means a Pull should take place. - // (Push) - CacheRefreshRemoteNewer - - // CacheRefreshConflict means that the push or pull - // was a no-op because there is a conflict. This means - // there are multiple state definitions at the same - // serial number with different contents. This requires - // an operator to intervene and resolve the conflict. - // Shame on the user for doing concurrent apply. - // (Push/Pull) - CacheRefreshConflict -) - -func (sc CacheRefreshResult) String() string { - switch sc { - case CacheRefreshNoop: - return "Local and remote state in sync" - case CacheRefreshInit: - return "Local state initialized" - case CacheRefreshUpdateLocal: - return "Local state updated" - case CacheRefreshUpdateRemote: - return "Remote state updated" - case CacheRefreshLocalNewer: - return "Local state is newer than remote state, push required" - case CacheRefreshRemoteNewer: - return "Remote state is newer than local state, pull required" - case CacheRefreshConflict: - return "Local and remote state conflict, manual resolution required" - default: - return fmt.Sprintf("Unknown state change type: %d", sc) - } -} - -// SuccessfulPull is used to clasify the CacheRefreshResult for -// a refresh operation. This is different by operation, but can be used -// to determine a proper exit code. -func (sc CacheRefreshResult) SuccessfulPull() bool { - switch sc { - case CacheRefreshNoop: - return true - case CacheRefreshInit: - return true - case CacheRefreshUpdateLocal: - return true - case CacheRefreshLocalNewer: - return false - case CacheRefreshConflict: - return false - default: - return false - } -} diff --git a/state/cache_test.go b/state/cache_test.go deleted file mode 100644 index 7252637e0..000000000 --- a/state/cache_test.go +++ /dev/null @@ -1,118 +0,0 @@ -package state - -import ( - "fmt" - "os" - "reflect" - "testing" - - "github.com/hashicorp/terraform/terraform" -) - -func TestCacheState(t *testing.T) { - cache := testLocalState(t) - durable := testLocalState(t) - defer os.Remove(cache.Path) - defer os.Remove(durable.Path) - - TestState(t, &CacheState{ - Cache: cache, - Durable: durable, - }) -} - -func TestCacheState_persistDurable(t *testing.T) { - cache := testLocalState(t) - durable := testLocalState(t) - defer os.Remove(cache.Path) - defer os.Remove(durable.Path) - - cs := &CacheState{ - Cache: cache, - Durable: durable, - } - - state := cache.State() - state.Modules = nil - if err := cs.WriteState(state); err != nil { - t.Fatalf("err: %s", err) - } - - if reflect.DeepEqual(cache.State(), durable.State()) { - t.Fatal("cache and durable should not be the same") - } - - if err := cs.PersistState(); err != nil { - t.Fatalf("err: %s", err) - } - - if !reflect.DeepEqual(cache.State(), durable.State()) { - t.Fatalf( - "cache and durable should be the same\n\n%#v\n\n%#v", - cache.State(), durable.State()) - } -} - -func TestCacheState_RefreshState(t *testing.T) { - for i, test := range []struct { - cacheModules []*terraform.ModuleState - expected CacheRefreshResult - }{ - { - cacheModules: nil, - expected: CacheRefreshUpdateLocal, - }, - { - cacheModules: []*terraform.ModuleState{}, - expected: CacheRefreshUpdateLocal, - }, - { - cacheModules: []*terraform.ModuleState{ - &terraform.ModuleState{ - Path: terraform.RootModulePath, - Resources: map[string]*terraform.ResourceState{ - "foo.foo": &terraform.ResourceState{ - Primary: &terraform.InstanceState{ - ID: "ID", - }, - }, - }, - }, - }, - expected: CacheRefreshLocalNewer, - }, - } { - t.Run(fmt.Sprintf("%d", i), func(t *testing.T) { - cache := testLocalState(t) - durable := testLocalState(t) - defer os.Remove(cache.Path) - defer os.Remove(durable.Path) - - cs := &CacheState{ - Cache: cache, - Durable: durable, - } - - state := cache.State() - state.Modules = test.cacheModules - if err := cs.WriteState(state); err != nil { - t.Fatalf("err: %s", err) - } - - if err := cs.RefreshState(); err != nil { - t.Fatalf("err: %s", err) - } - - if cs.RefreshResult() != test.expected { - t.Fatalf("bad %d: %v", i, cs.RefreshResult()) - } - }) - } -} - -func TestCacheState_impl(t *testing.T) { - var _ StateReader = new(CacheState) - var _ StateWriter = new(CacheState) - var _ StatePersister = new(CacheState) - var _ StateRefresher = new(CacheState) -} diff --git a/state/local.go b/state/local.go index 0aae22bb3..01708af6f 100644 --- a/state/local.go +++ b/state/local.go @@ -34,27 +34,6 @@ type LocalState struct { written bool } -// LockInfo stores metadata for locks taken. -type LockInfo struct { - Path string // Path to the state file - Created time.Time // The time the lock was taken - Info string // Extra info passed to State.Lock -} - -// Err returns the lock info formatted in an error -func (l *LockInfo) Err() error { - return fmt.Errorf("state locked. path:%q, created:%s, info:%q", - l.Path, l.Created, l.Info) -} - -func (l *LockInfo) String() string { - js, err := json.Marshal(l) - if err != nil { - panic(err) - } - return string(js) -} - // SetState will force a specific state in-memory for this local state. func (s *LocalState) SetState(state *terraform.State) { s.state = state diff --git a/state/state.go b/state/state.go index 532e14872..6f394f46d 100644 --- a/state/state.go +++ b/state/state.go @@ -1,6 +1,11 @@ package state import ( + "encoding/json" + "fmt" + "strings" + "time" + "github.com/hashicorp/terraform/terraform" ) @@ -42,9 +47,56 @@ type StatePersister interface { } // Locker is implemented to lock state during command execution. -// The optional info parameter can be recorded with the lock, but the -// implementation should not depend in its value. +// The info parameter can be recorded with the lock, but the +// implementation should not depend in its value. The string returned by Lock +// is an ID corresponding to the lock acquired, and must be passed to Unlock to +// ensure that the correct lock is being released. +// +// Lock and Unlock may return an error value of type LockError which in turn +// can contain the LockInfo of a conflicting lock. type Locker interface { - Lock(info string) error - Unlock() error + Lock(info *LockInfo) (string, error) + Unlock(id string) error +} + +// LockInfo stores metadata for locks taken. +type LockInfo struct { + ID string // unique ID + Path string // Path to the state file + Created time.Time // The time the lock was taken + Version string // Terraform version + Operation string // Terraform operation + Who string // user@hostname when available + Info string // Extra info field +} + +// Err returns the lock info formatted in an error +func (l *LockInfo) Err() error { + return fmt.Errorf("state locked. path:%q, created:%s, info:%q", + l.Path, l.Created, l.Info) +} + +func (l *LockInfo) String() string { + js, err := json.Marshal(l) + if err != nil { + panic(err) + } + return string(js) +} + +type LockError struct { + Info *LockInfo + Err error +} + +func (e *LockError) Error() string { + var out []string + if e.Err != nil { + out = append(out, e.Err.Error()) + } + + if e.Info != nil { + out = append(out, e.Info.Err().Error()) + } + return strings.Join(out, "\n") } From 0ad6f08204a1001dfd3583687197c904af00357d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 17:48:57 -0500 Subject: [PATCH 02/15] Make remote state test run Make them compile against the new interface. The tests will be updated later to check new behavior. --- state/remote/remote_test.go | 74 ------------------------------------- state/remote/testing.go | 23 ++++++++---- 2 files changed, 16 insertions(+), 81 deletions(-) diff --git a/state/remote/remote_test.go b/state/remote/remote_test.go index db3c795c8..a9c55b9db 100644 --- a/state/remote/remote_test.go +++ b/state/remote/remote_test.go @@ -2,8 +2,6 @@ package remote import ( "bytes" - "io/ioutil" - "os" "testing" "github.com/hashicorp/terraform/state" @@ -61,75 +59,3 @@ func (nilClient) Get() (*Payload, error) { return nil, nil } func (c nilClient) Put([]byte) error { return nil } func (c nilClient) Delete() error { return nil } - -// ensure that remote state can be properly initialized -func TestRemoteClient_stateInit(t *testing.T) { - localStateFile, err := ioutil.TempFile("", "tf") - if err != nil { - t.Fatal(err) - } - - // we need to remove the temp files so we recognize there's no local or - // remote state. - localStateFile.Close() - os.Remove(localStateFile.Name()) - defer os.Remove(localStateFile.Name()) - - remoteStateFile, err := ioutil.TempFile("", "tf") - if err != nil { - t.Fatal(err) - } - remoteStateFile.Close() - os.Remove(remoteStateFile.Name()) - defer os.Remove(remoteStateFile.Name()) - - // Now we need an empty state to initialize the state files. - newState := terraform.NewState() - newState.Remote = &terraform.RemoteState{ - Type: "_local", - Config: map[string]string{"path": remoteStateFile.Name()}, - } - - remoteClient := &FileClient{ - Path: remoteStateFile.Name(), - } - - cache := &state.CacheState{ - Cache: &state.LocalState{ - Path: localStateFile.Name(), - }, - Durable: &State{ - Client: remoteClient, - }, - } - - // This will write the local state file, and set the state field in the CacheState - err = cache.WriteState(newState) - if err != nil { - t.Fatal(err) - } - - // This will persist the local state we just wrote to the remote state file - err = cache.PersistState() - if err != nil { - t.Fatal(err) - } - - // now compare the two state files just to be sure - localData, err := ioutil.ReadFile(localStateFile.Name()) - if err != nil { - t.Fatal(err) - } - - remoteData, err := ioutil.ReadFile(remoteStateFile.Name()) - if err != nil { - t.Fatal(err) - } - - if !bytes.Equal(localData, remoteData) { - t.Log("state files don't match") - t.Log("Local:\n", string(localData)) - t.Log("Remote:\n", string(remoteData)) - t.Fatal("failed to initialize remote state") - } -} diff --git a/state/remote/testing.go b/state/remote/testing.go index d7de10b16..17faa26d3 100644 --- a/state/remote/testing.go +++ b/state/remote/testing.go @@ -57,31 +57,40 @@ func TestRemoteLocks(t *testing.T, a, b Client) { t.Fatal("client B not a state.Locker") } - if err := lockerA.Lock("test client A"); err != nil { + infoA := &state.LockInfo{ + Operation: "test", + Who: "client A", + } + infoB := &state.LockInfo{ + Operation: "test", + Who: "client B", + } + + if _, err := lockerA.Lock(infoA); err != nil { t.Fatal("unable to get initial lock:", err) } - if err := lockerB.Lock("test client B"); err == nil { - lockerA.Unlock() + if _, err := lockerB.Lock(infoB); err == nil { + lockerA.Unlock("") t.Fatal("client B obtained lock while held by client A") } else { t.Log("lock info error:", err) } - if err := lockerA.Unlock(); err != nil { + if err := lockerA.Unlock(""); err != nil { t.Fatal("error unlocking client A", err) } - if err := lockerB.Lock("test client B"); err != nil { + if _, err := lockerB.Lock(infoB); err != nil { t.Fatal("unable to obtain lock from client B") } - if err := lockerB.Unlock(); err != nil { + if err := lockerB.Unlock(""); err != nil { t.Fatal("error unlocking client B:", err) } // unlock should be repeatable - if err := lockerA.Unlock(); err != nil { + if err := lockerA.Unlock(""); err != nil { t.Fatal("Unlock error from client A when state was not locked:", err) } } From 6d32b70637a7557d7cd68d93d8e589a169058652 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 18:00:59 -0500 Subject: [PATCH 03/15] Make S3 remote state pass tests TODO: update S3Client to make full use of the state.Locker interface --- state/remote/s3.go | 33 +++++++++++++++++++-------------- 1 file changed, 19 insertions(+), 14 deletions(-) diff --git a/state/remote/s3.go b/state/remote/s3.go index ab1045b80..7f4040f2e 100644 --- a/state/remote/s3.go +++ b/state/remote/s3.go @@ -17,6 +17,7 @@ import ( "github.com/aws/aws-sdk-go/service/s3" "github.com/hashicorp/go-cleanhttp" "github.com/hashicorp/go-multierror" + uuid "github.com/hashicorp/go-uuid" terraformAws "github.com/hashicorp/terraform/builtin/providers/aws" "github.com/hashicorp/terraform/state" ) @@ -203,27 +204,31 @@ func (c *S3Client) Delete() error { return err } -func (c *S3Client) Lock(info string) error { +func (c *S3Client) Lock(info *state.LockInfo) (string, error) { if c.lockTable == "" { - return nil + return "", nil } stateName := fmt.Sprintf("%s/%s", c.bucketName, c.keyName) - lockInfo := &state.LockInfo{ - Path: stateName, - Created: time.Now().UTC(), - Info: info, + + lockID, err := uuid.GenerateUUID() + if err != nil { + return "", err } + info.ID = lockID + info.Path = stateName + info.Created = time.Now().UTC() + putParams := &dynamodb.PutItemInput{ Item: map[string]*dynamodb.AttributeValue{ "LockID": {S: aws.String(stateName)}, - "Info": {S: aws.String(lockInfo.String())}, + "Info": {S: aws.String(info.String())}, }, TableName: aws.String(c.lockTable), ConditionExpression: aws.String("attribute_not_exists(LockID)"), } - _, err := c.dynClient.PutItem(putParams) + _, err = c.dynClient.PutItem(putParams) if err != nil { getParams := &dynamodb.GetItemInput{ @@ -236,7 +241,7 @@ func (c *S3Client) Lock(info string) error { resp, err := c.dynClient.GetItem(getParams) if err != nil { - return fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) + return "", fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) } var infoData string @@ -244,18 +249,18 @@ func (c *S3Client) Lock(info string) error { infoData = *v.S } - lockInfo = &state.LockInfo{} + lockInfo := &state.LockInfo{} err = json.Unmarshal([]byte(infoData), lockInfo) if err != nil { - return fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) + return "", fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) } - return lockInfo.Err() + return "", lockInfo.Err() } - return nil + return "", nil } -func (c *S3Client) Unlock() error { +func (c *S3Client) Unlock(string) error { if c.lockTable == "" { return nil } From 67dc16c9cac663019d155dd83e5b52237457adef Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 18:11:55 -0500 Subject: [PATCH 04/15] Make backend/local test pass --- backend/local/backend_apply.go | 2 +- backend/local/backend_local.go | 6 +++++- backend/local/backend_plan.go | 2 +- backend/local/backend_refresh.go | 2 +- command/state/state.go | 18 ++++++++++++------ 5 files changed, 20 insertions(+), 10 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 62c4fcd1f..caffaa983 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -38,7 +38,7 @@ func (b *Local) opApply( // If we're locking state, unlock when we're done if op.LockState { defer func() { - if err := clistate.Unlock(opState, b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, "", b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index cdcaac1d1..265075675 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -30,7 +30,11 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, state.State, } if op.LockState { - err := clistate.Lock(s, op.Type.String(), b.CLI, b.Colorize()) + lockInfo := &state.LockInfo{ + Info: op.Type.String(), + } + + _, err := clistate.Lock(s, lockInfo, b.CLI, b.Colorize()) if err != nil { return nil, nil, errwrap.Wrapf("Error locking state: {{err}}", err) } diff --git a/backend/local/backend_plan.go b/backend/local/backend_plan.go index 2e218c79e..70587cb36 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -62,7 +62,7 @@ func (b *Local) opPlan( // If we're locking state, unlock when we're done if op.LockState { defer func() { - if err := clistate.Unlock(opState, b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, "", b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index dc5c9a6e4..6a939d491 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -51,7 +51,7 @@ func (b *Local) opRefresh( // If we're locking state, unlock when we're done if op.LockState { defer func() { - if err := clistate.Unlock(opState, b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, "", b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/command/state/state.go b/command/state/state.go index 32a5c8619..3915e8ca2 100644 --- a/command/state/state.go +++ b/command/state/state.go @@ -49,14 +49,18 @@ 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. -func Lock(s state.State, info string, ui cli.Ui, color *colorstring.Colorize) error { +func Lock(s state.State, info *state.LockInfo, ui cli.Ui, color *colorstring.Colorize) (string, error) { sl, ok := s.(state.Locker) if !ok { - return nil + return "", nil } + var lockID string + err := slowmessage.Do(LockThreshold, func() error { - return sl.Lock(info) + id, err := sl.Lock(info) + lockID = id + return err }, func() { if ui != nil { ui.Output(color.Color(LockMessage)) @@ -67,18 +71,20 @@ func Lock(s state.State, info string, ui cli.Ui, color *colorstring.Colorize) er err = errwrap.Wrapf(strings.TrimSpace(LockErrorMessage), err) } - return err + return lockID, err } // Unlock unlocks the given state and outputs to the user if the // unlock fails what can be done. -func Unlock(s state.State, ui cli.Ui, color *colorstring.Colorize) error { +func Unlock(s state.State, id string, ui cli.Ui, color *colorstring.Colorize) error { sl, ok := s.(state.Locker) if !ok { return nil } - err := slowmessage.Do(LockThreshold, sl.Unlock, func() { + err := slowmessage.Do(LockThreshold, func() error { + return sl.Unlock(id) + }, func() { if ui != nil { ui.Output(color.Color(UnlockMessage)) } From cd233fef6ab460643e787ec909b2be1eae96a98d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 18:19:47 -0500 Subject: [PATCH 05/15] make consul client pass state.Locker tests --- backend/remote-state/consul/client.go | 33 ++++++++++++--------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index 580789522..a5578a3c1 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -58,14 +58,11 @@ func (c *RemoteClient) Delete() error { return err } -func (c *RemoteClient) putLockInfo(info string) error { - li := &state.LockInfo{ - Path: c.Path, - Created: time.Now().UTC(), - Info: info, - } +func (c *RemoteClient) putLockInfo(info *state.LockInfo) error { + info.Path = c.Path + info.Created = time.Now().UTC() - js, err := json.Marshal(li) + js, err := json.Marshal(info) if err != nil { return err } @@ -98,16 +95,16 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) { return li, nil } -func (c *RemoteClient) Lock(info string) error { +func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { select { case <-c.lockCh: // We had a lock, but lost it. // Since we typically only call lock once, we shouldn't ever see this. - return errors.New("lost consul lock") + return "", errors.New("lost consul lock") default: if c.lockCh != nil { // we have an active lock already - return nil + return "", nil } } @@ -123,7 +120,7 @@ func (c *RemoteClient) Lock(info string) error { lock, err := c.Client.LockOpts(opts) if err != nil { - return nil + return "", nil } c.consulLock = lock @@ -131,29 +128,29 @@ func (c *RemoteClient) Lock(info string) error { lockCh, err := c.consulLock.Lock(make(chan struct{})) if err != nil { - return err + return "", err } if lockCh == nil { lockInfo, e := c.getLockInfo() if e != nil { - return e + return "", e } - return lockInfo.Err() + return "", lockInfo.Err() } c.lockCh = lockCh err = c.putLockInfo(info) if err != nil { - err = multierror.Append(err, c.Unlock()) - return err + err = multierror.Append(err, c.Unlock("")) + return "", err } - return nil + return "", nil } -func (c *RemoteClient) Unlock() error { +func (c *RemoteClient) Unlock(id string) error { if c.consulLock == nil || c.lockCh == nil { return nil } From 4f0c465187fe13e2bb2e874e594e35ac1d6675d0 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 18:51:32 -0500 Subject: [PATCH 06/15] make command tests pass with new state.Locker --- command/meta_backend.go | 32 ++++++++++++++++++++++++-------- command/meta_backend_migrate.go | 16 ++++++++++++---- command/taint.go | 8 ++++++-- command/testdata/statelocker.go | 4 ++-- command/unlock.go | 3 ++- command/untaint.go | 8 ++++++-- state/local.go | 26 +++++++++++--------------- 7 files changed, 63 insertions(+), 34 deletions(-) diff --git a/command/meta_backend.go b/command/meta_backend.go index 85277bae3..7a53c8685 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -533,11 +533,16 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) { } // Lock the state if we can - err = clistate.Lock(realMgr, "backend from plan", m.Ui, m.Colorize()) + lockInfo := &state.LockInfo{ + Operation: "plan", + Info: "backend from plan", + } + + lockID, err := clistate.Lock(realMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(realMgr, m.Ui, m.Colorize()) + defer clistate.Unlock(realMgr, lockID, m.Ui, m.Colorize()) if err := realMgr.RefreshState(); err != nil { return nil, fmt.Errorf("Error reading state: %s", err) @@ -986,11 +991,15 @@ func (m *Meta) backend_C_r_s( } // Lock the state if we can - err = clistate.Lock(sMgr, "backend from config", m.Ui, m.Colorize()) + lockInfo := &state.LockInfo{ + Info: "backend from config", + } + + lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, m.Ui, m.Colorize()) + defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) // Store the metadata in our saved state location s := sMgr.State() @@ -1091,11 +1100,14 @@ func (m *Meta) backend_C_r_S_changed( } // Lock the state if we can - err = clistate.Lock(sMgr, "backend from config", m.Ui, m.Colorize()) + lockInfo := &state.LockInfo{ + Info: "backend from config", + } + lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, m.Ui, m.Colorize()) + defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) // Update the backend state s = sMgr.State() @@ -1249,11 +1261,15 @@ func (m *Meta) backend_C_R_S_unchanged( } // Lock the state if we can - err = clistate.Lock(sMgr, "backend from config", m.Ui, m.Colorize()) + lockInfo := &state.LockInfo{ + Info: "backend from config", + } + + lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) } - defer clistate.Unlock(sMgr, m.Ui, m.Colorize()) + defer clistate.Unlock(sMgr, lockID, m.Ui, m.Colorize()) // Unset the remote state s = sMgr.State() diff --git a/command/meta_backend_migrate.go b/command/meta_backend_migrate.go index 4df4463f4..a51e4a57a 100644 --- a/command/meta_backend_migrate.go +++ b/command/meta_backend_migrate.go @@ -24,17 +24,25 @@ import ( // // This will attempt to lock both states for the migration. func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error { - err := clistate.Lock(opts.One, "migration source state", m.Ui, m.Colorize()) + lockInfoOne := &state.LockInfo{ + Info: "migration source state", + } + + lockIDOne, err := clistate.Lock(opts.One, lockInfoOne, m.Ui, m.Colorize()) if err != nil { return fmt.Errorf("Error locking source state: %s", err) } - defer clistate.Unlock(opts.One, m.Ui, m.Colorize()) + defer clistate.Unlock(opts.One, lockIDOne, m.Ui, m.Colorize()) - err = clistate.Lock(opts.Two, "migration destination state", m.Ui, m.Colorize()) + lockInfoTwo := &state.LockInfo{ + Info: "migration source state", + } + + lockIDTwo, err := clistate.Lock(opts.Two, lockInfoTwo, m.Ui, m.Colorize()) if err != nil { return fmt.Errorf("Error locking destination state: %s", err) } - defer clistate.Unlock(opts.Two, m.Ui, m.Colorize()) + defer clistate.Unlock(opts.Two, lockIDTwo, m.Ui, m.Colorize()) one := opts.One.State() two := opts.Two.State() diff --git a/command/taint.go b/command/taint.go index c2321c813..95f4a5249 100644 --- a/command/taint.go +++ b/command/taint.go @@ -6,6 +6,7 @@ import ( "strings" clistate "github.com/hashicorp/terraform/command/state" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -73,13 +74,16 @@ func (c *TaintCommand) Run(args []string) int { } if c.Meta.stateLock { - err := clistate.Lock(st, "taint", c.Ui, c.Colorize()) + lockInfo := &state.LockInfo{ + Operation: "taint", + } + lockID, err := clistate.Lock(st, lockInfo, c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer clistate.Unlock(st, c.Ui, c.Colorize()) + defer clistate.Unlock(st, lockID, c.Ui, c.Colorize()) } // Get the actual state structure diff --git a/command/testdata/statelocker.go b/command/testdata/statelocker.go index 8f25f9d33..3128d59f7 100644 --- a/command/testdata/statelocker.go +++ b/command/testdata/statelocker.go @@ -23,7 +23,7 @@ func main() { Path: os.Args[1], } - err := s.Lock("command test") + lockID, err := s.Lock(&state.LockInfo{Operation: "test", Info: "state locker"}) if err != nil { io.WriteString(os.Stderr, err.Error()) return @@ -33,7 +33,7 @@ func main() { io.WriteString(os.Stdout, "LOCKED") defer func() { - if err := s.Unlock(); err != nil { + if err := s.Unlock(lockID); err != nil { io.WriteString(os.Stderr, err.Error()) } }() diff --git a/command/unlock.go b/command/unlock.go index fb1d22c4e..952d9bacf 100644 --- a/command/unlock.go +++ b/command/unlock.go @@ -92,7 +92,8 @@ func (c *UnlockCommand) Run(args []string) int { } } - if err := s.Unlock(); err != nil { + // FIXME: unlock should require the lock ID + if err := s.Unlock(""); err != nil { c.Ui.Error(fmt.Sprintf("Failed to unlock state: %s", err)) return 1 } diff --git a/command/untaint.go b/command/untaint.go index 453a79ac8..3bd160d28 100644 --- a/command/untaint.go +++ b/command/untaint.go @@ -6,6 +6,7 @@ import ( "strings" clistate "github.com/hashicorp/terraform/command/state" + "github.com/hashicorp/terraform/state" ) // UntaintCommand is a cli.Command implementation that manually untaints @@ -61,13 +62,16 @@ func (c *UntaintCommand) Run(args []string) int { } if c.Meta.stateLock { - err := clistate.Lock(st, "untaint", c.Ui, c.Colorize()) + lockInfo := &state.LockInfo{ + Operation: "untaint", + } + lockID, err := clistate.Lock(st, lockInfo, c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) return 1 } - defer clistate.Unlock(st, c.Ui, c.Colorize()) + defer clistate.Unlock(st, lockID, c.Ui, c.Colorize()) } // Get the actual state structure diff --git a/state/local.go b/state/local.go index 01708af6f..c92cc8e48 100644 --- a/state/local.go +++ b/state/local.go @@ -134,25 +134,25 @@ func (s *LocalState) RefreshState() error { } // Lock implements a local filesystem state.Locker. -func (s *LocalState) Lock(reason string) error { +func (s *LocalState) Lock(info *LockInfo) (string, error) { if s.stateFileOut == nil { if err := s.createStateFiles(); err != nil { - return err + return "", err } } if err := s.lock(); err != nil { if info, err := s.lockInfo(); err == nil { - return info.Err() + return "", info.Err() } - return fmt.Errorf("state file %q locked: %s", s.Path, err) + return "", fmt.Errorf("state file %q locked: %s", s.Path, err) } - return s.writeLockInfo(reason) + return "", s.writeLockInfo(info) } -func (s *LocalState) Unlock() error { +func (s *LocalState) Unlock(id string) error { // we can't be locked if we don't have a file if s.stateFileOut == nil { return nil @@ -232,18 +232,14 @@ func (s *LocalState) lockInfo() (*LockInfo, error) { } // write a new lock info file -func (s *LocalState) writeLockInfo(info string) error { +func (s *LocalState) writeLockInfo(info *LockInfo) error { path := s.lockInfoPath() + info.Path = s.Path + info.Created = time.Now().UTC() - lockInfo := &LockInfo{ - Path: s.Path, - Created: time.Now().UTC(), - Info: info, - } - - infoData, err := json.Marshal(lockInfo) + infoData, err := json.Marshal(info) if err != nil { - panic(fmt.Sprintf("could not marshal lock info: %#v", lockInfo)) + panic(fmt.Sprintf("could not marshal lock info: %#v", info)) } err = ioutil.WriteFile(path, infoData, 0600) From 52b234367281f6450cf2847689ed8b355f222b82 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 14 Feb 2017 19:00:36 -0500 Subject: [PATCH 07/15] make state test pass with new state.Locker --- state/local_test.go | 21 ++++++++++++--------- state/testdata/lockstate.go | 3 ++- 2 files changed, 14 insertions(+), 10 deletions(-) diff --git a/state/local_test.go b/state/local_test.go index cfd327f8d..2210aa949 100644 --- a/state/local_test.go +++ b/state/local_test.go @@ -20,14 +20,17 @@ func TestLocalStateLocks(t *testing.T) { defer os.Remove(s.Path) // lock first - if err := s.Lock("test"); err != nil { + info := &LockInfo{ + Operation: "test", + } + lockID, err := s.Lock(info) + if err != nil { t.Fatal(err) } out, err := exec.Command("go", "run", "testdata/lockstate.go", s.Path).CombinedOutput() - if err != nil { - t.Fatal("unexpected lock failure", err) + t.Fatal("unexpected lock failure", err, string(out)) } if string(out) != "lock failed" { @@ -40,25 +43,26 @@ func TestLocalStateLocks(t *testing.T) { t.Fatal(err) } - if lockInfo.Info != "test" { + if lockInfo.Operation != "test" { t.Fatalf("invalid lock info %#v\n", lockInfo) } // a noop, since we unlock on exit - if err := s.Unlock(); err != nil { + if err := s.Unlock(lockID); err != nil { t.Fatal(err) } // local locks can re-lock - if err := s.Lock("test"); err != nil { + lockID, err = s.Lock(info) + if err != nil { t.Fatal(err) } // Unlock should be repeatable - if err := s.Unlock(); err != nil { + if err := s.Unlock(lockID); err != nil { t.Fatal(err) } - if err := s.Unlock(); err != nil { + if err := s.Unlock(lockID); err != nil { t.Fatal(err) } @@ -67,7 +71,6 @@ func TestLocalStateLocks(t *testing.T) { if _, err := os.Stat(lockInfoPath); !os.IsNotExist(err) { t.Fatal("lock info not removed") } - } func TestLocalState_pathOut(t *testing.T) { diff --git a/state/testdata/lockstate.go b/state/testdata/lockstate.go index ca8ff5af5..a23b758ef 100644 --- a/state/testdata/lockstate.go +++ b/state/testdata/lockstate.go @@ -19,10 +19,11 @@ func main() { Path: os.Args[1], } - err := s.Lock("test") + _, err := s.Lock(&state.LockInfo{Operation: "test", Info: "state locker"}) if err != nil { io.WriteString(os.Stderr, "lock failed") } + return } From f5ed8cd28891903c1a30b8de1ce81727ccc76a33 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 10:25:04 -0500 Subject: [PATCH 08/15] Use NewLockInfo to get a pre-populated value Using NewLockInfo ensure we start with all required fields filled. --- backend/local/backend_local.go | 6 +-- command/meta_backend.go | 22 +++++------ command/meta_backend_migrate.go | 10 ++--- command/taint.go | 5 +-- command/testdata/statelocker.go | 6 ++- command/untaint.go | 5 +-- state/local_test.go | 5 +-- state/remote/s3.go | 28 +++++++------- state/remote/testing.go | 15 ++++---- state/state.go | 67 +++++++++++++++++++++++++++++---- state/state_test.go | 21 +++++++++++ state/testdata/lockstate.go | 6 ++- 12 files changed, 132 insertions(+), 64 deletions(-) diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index 265075675..e7a98b6e4 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -30,10 +30,8 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, state.State, } if op.LockState { - lockInfo := &state.LockInfo{ - Info: op.Type.String(), - } - + lockInfo := state.NewLockInfo() + lockInfo.Operation = op.Type.String() _, err := clistate.Lock(s, lockInfo, b.CLI, b.Colorize()) if err != nil { return nil, nil, errwrap.Wrapf("Error locking state: {{err}}", err) diff --git a/command/meta_backend.go b/command/meta_backend.go index 7a53c8685..8cc95fd0c 100644 --- a/command/meta_backend.go +++ b/command/meta_backend.go @@ -533,10 +533,8 @@ func (m *Meta) backendFromPlan(opts *BackendOpts) (backend.Backend, error) { } // Lock the state if we can - lockInfo := &state.LockInfo{ - Operation: "plan", - Info: "backend from plan", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "backend from plan" lockID, err := clistate.Lock(realMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { @@ -991,9 +989,8 @@ func (m *Meta) backend_C_r_s( } // Lock the state if we can - lockInfo := &state.LockInfo{ - Info: "backend from config", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "backend from config" lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { @@ -1100,9 +1097,9 @@ func (m *Meta) backend_C_r_S_changed( } // Lock the state if we can - lockInfo := &state.LockInfo{ - Info: "backend from config", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "backend from config" + lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { return nil, fmt.Errorf("Error locking state: %s", err) @@ -1261,9 +1258,8 @@ func (m *Meta) backend_C_R_S_unchanged( } // Lock the state if we can - lockInfo := &state.LockInfo{ - Info: "backend from config", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "backend from config" lockID, err := clistate.Lock(sMgr, lockInfo, m.Ui, m.Colorize()) if err != nil { diff --git a/command/meta_backend_migrate.go b/command/meta_backend_migrate.go index a51e4a57a..f1225e9d2 100644 --- a/command/meta_backend_migrate.go +++ b/command/meta_backend_migrate.go @@ -24,9 +24,8 @@ import ( // // This will attempt to lock both states for the migration. func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error { - lockInfoOne := &state.LockInfo{ - Info: "migration source state", - } + lockInfoOne := state.NewLockInfo() + lockInfoOne.Operation = "migration source state" lockIDOne, err := clistate.Lock(opts.One, lockInfoOne, m.Ui, m.Colorize()) if err != nil { @@ -34,9 +33,8 @@ func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error { } defer clistate.Unlock(opts.One, lockIDOne, m.Ui, m.Colorize()) - lockInfoTwo := &state.LockInfo{ - Info: "migration source state", - } + lockInfoTwo := state.NewLockInfo() + lockInfoTwo.Operation = "migration source state" lockIDTwo, err := clistate.Lock(opts.Two, lockInfoTwo, m.Ui, m.Colorize()) if err != nil { diff --git a/command/taint.go b/command/taint.go index 95f4a5249..9b184e27a 100644 --- a/command/taint.go +++ b/command/taint.go @@ -74,9 +74,8 @@ func (c *TaintCommand) Run(args []string) int { } if c.Meta.stateLock { - lockInfo := &state.LockInfo{ - Operation: "taint", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "taint" lockID, err := clistate.Lock(st, lockInfo, c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) diff --git a/command/testdata/statelocker.go b/command/testdata/statelocker.go index 3128d59f7..7bdce2112 100644 --- a/command/testdata/statelocker.go +++ b/command/testdata/statelocker.go @@ -23,7 +23,11 @@ func main() { Path: os.Args[1], } - lockID, err := s.Lock(&state.LockInfo{Operation: "test", Info: "state locker"}) + info := state.NewLockInfo() + info.Operation = "test" + info.Info = "state locker" + + lockID, err := s.Lock(info) if err != nil { io.WriteString(os.Stderr, err.Error()) return diff --git a/command/untaint.go b/command/untaint.go index 3bd160d28..dca2cd9be 100644 --- a/command/untaint.go +++ b/command/untaint.go @@ -62,9 +62,8 @@ func (c *UntaintCommand) Run(args []string) int { } if c.Meta.stateLock { - lockInfo := &state.LockInfo{ - Operation: "untaint", - } + lockInfo := state.NewLockInfo() + lockInfo.Operation = "untaint" lockID, err := clistate.Lock(st, lockInfo, c.Ui, c.Colorize()) if err != nil { c.Ui.Error(fmt.Sprintf("Error locking state: %s", err)) diff --git a/state/local_test.go b/state/local_test.go index 2210aa949..b0472aaa6 100644 --- a/state/local_test.go +++ b/state/local_test.go @@ -20,9 +20,8 @@ func TestLocalStateLocks(t *testing.T) { defer os.Remove(s.Path) // lock first - info := &LockInfo{ - Operation: "test", - } + info := NewLockInfo() + info.Operation = "test" lockID, err := s.Lock(info) if err != nil { t.Fatal(err) diff --git a/state/remote/s3.go b/state/remote/s3.go index 7f4040f2e..b5c245ad2 100644 --- a/state/remote/s3.go +++ b/state/remote/s3.go @@ -8,7 +8,6 @@ import ( "log" "os" "strconv" - "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/aws/awserr" @@ -210,15 +209,16 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { } stateName := fmt.Sprintf("%s/%s", c.bucketName, c.keyName) - - lockID, err := uuid.GenerateUUID() - if err != nil { - return "", err - } - - info.ID = lockID info.Path = stateName - info.Created = time.Now().UTC() + + if info.ID == "" { + lockID, err := uuid.GenerateUUID() + if err != nil { + return "", err + } + + info.ID = lockID + } putParams := &dynamodb.PutItemInput{ Item: map[string]*dynamodb.AttributeValue{ @@ -228,7 +228,7 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { TableName: aws.String(c.lockTable), ConditionExpression: aws.String("attribute_not_exists(LockID)"), } - _, err = c.dynClient.PutItem(putParams) + _, err := c.dynClient.PutItem(putParams) if err != nil { getParams := &dynamodb.GetItemInput{ @@ -241,7 +241,7 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { resp, err := c.dynClient.GetItem(getParams) if err != nil { - return "", fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) + return info.ID, fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) } var infoData string @@ -252,12 +252,12 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { lockInfo := &state.LockInfo{} err = json.Unmarshal([]byte(infoData), lockInfo) if err != nil { - return "", fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) + return info.ID, fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) } - return "", lockInfo.Err() + return info.ID, lockInfo.Err() } - return "", nil + return info.ID, nil } func (c *S3Client) Unlock(string) error { diff --git a/state/remote/testing.go b/state/remote/testing.go index 17faa26d3..0f7a2d15e 100644 --- a/state/remote/testing.go +++ b/state/remote/testing.go @@ -57,14 +57,13 @@ func TestRemoteLocks(t *testing.T, a, b Client) { t.Fatal("client B not a state.Locker") } - infoA := &state.LockInfo{ - Operation: "test", - Who: "client A", - } - infoB := &state.LockInfo{ - Operation: "test", - Who: "client B", - } + infoA := state.NewLockInfo() + infoA.Operation = "test" + infoA.Who = "clientA" + + infoB := state.NewLockInfo() + infoB.Operation = "test" + infoB.Who = "clientB" if _, err := lockerA.Lock(infoA); err != nil { t.Fatal("unable to get initial lock:", err) diff --git a/state/state.go b/state/state.go index 6f394f46d..dd74f9497 100644 --- a/state/state.go +++ b/state/state.go @@ -3,12 +3,22 @@ package state import ( "encoding/json" "fmt" + "math/rand" + "os" + "os/user" "strings" "time" + uuid "github.com/hashicorp/go-uuid" "github.com/hashicorp/terraform/terraform" ) +var rngSource *rand.Rand + +func init() { + rngSource = rand.New(rand.NewSource(time.Now().UnixNano())) +} + // State is the collection of all state interfaces. type State interface { StateReader @@ -59,15 +69,56 @@ type Locker interface { Unlock(id string) error } -// LockInfo stores metadata for locks taken. +// Generate a LockInfo structure, populating the required fields. +func NewLockInfo() *LockInfo { + // this doesn't need to be cryptographically secure, just unique. + // Using math/rand alleviates the need to check handle the read error. + // Use a uuid format to match other IDs used throughout Terraform. + buf := make([]byte, 16) + rngSource.Read(buf) + + id, err := uuid.FormatUUID(buf) + if err != nil { + // this of course shouldn't happen + panic(err) + } + + // don't error out on user and hostname, as we don't require them + username, _ := user.Current() + host, _ := os.Hostname() + + info := &LockInfo{ + ID: id, + Who: fmt.Sprintf("%s@%s", username, host), + Version: terraform.Version, + Created: time.Now().UTC(), + } + return info +} + +// LockInfo stores lock metadata. +// +// Only Operation and Info are required to be set by the caller of Lock. type LockInfo struct { - ID string // unique ID - Path string // Path to the state file - Created time.Time // The time the lock was taken - Version string // Terraform version - Operation string // Terraform operation - Who string // user@hostname when available - Info string // Extra info field + // Unique ID for the lock. NewLockInfo provides a random ID, but this may + // be overridden by the lock implementation. The final value if ID will be + // returned by the call to Lock. + ID string + + // Terraform operation, provided by the caller. + Operation string + // Extra information to store with the lock, provided by the caller. + Info string + + // user@hostname when available + Who string + // Terraform version + Version string + // Time that the lock was taken. + Created time.Time + + // Path to the state file when applicable. Set by the Lock implementation. + Path string } // Err returns the lock info formatted in an error diff --git a/state/state_test.go b/state/state_test.go index 27cfa4103..a70fb52b4 100644 --- a/state/state_test.go +++ b/state/state_test.go @@ -21,3 +21,24 @@ func TestMain(m *testing.M) { } os.Exit(m.Run()) } + +func TestNewLockInfo(t *testing.T) { + info1 := NewLockInfo() + info2 := NewLockInfo() + + if info1.ID == "" { + t.Fatal("LockInfo missing ID") + } + + if info1.Version == "" { + t.Fatal("LockInfo missing version") + } + + if info1.Created.IsZero() { + t.Fatal("LockInfo missing Created") + } + + if info1.ID == info2.ID { + t.Fatal("multiple LockInfo with identical IDs") + } +} diff --git a/state/testdata/lockstate.go b/state/testdata/lockstate.go index a23b758ef..ced62c232 100644 --- a/state/testdata/lockstate.go +++ b/state/testdata/lockstate.go @@ -19,7 +19,11 @@ func main() { Path: os.Args[1], } - _, err := s.Lock(&state.LockInfo{Operation: "test", Info: "state locker"}) + info := state.NewLockInfo() + info.Operation = "test" + info.Info = "state locker" + + _, err := s.Lock(info) if err != nil { io.WriteString(os.Stderr, "lock failed") From 6aa1066f7c124934e132ee8a7c52095302470d58 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 10:39:35 -0500 Subject: [PATCH 09/15] Store and use the correct IDs in TestRemoteLocks --- state/remote/testing.go | 22 ++++++++++------------ 1 file changed, 10 insertions(+), 12 deletions(-) diff --git a/state/remote/testing.go b/state/remote/testing.go index 0f7a2d15e..bd71aefcb 100644 --- a/state/remote/testing.go +++ b/state/remote/testing.go @@ -65,31 +65,29 @@ func TestRemoteLocks(t *testing.T, a, b Client) { infoB.Operation = "test" infoB.Who = "clientB" - if _, err := lockerA.Lock(infoA); err != nil { + lockIDA, err := lockerA.Lock(infoA) + if err != nil { t.Fatal("unable to get initial lock:", err) } - if _, err := lockerB.Lock(infoB); err == nil { - lockerA.Unlock("") + _, err = lockerB.Lock(infoB) + if err == nil { + lockerA.Unlock(lockIDA) t.Fatal("client B obtained lock while held by client A") - } else { - t.Log("lock info error:", err) } - if err := lockerA.Unlock(""); err != nil { + if err := lockerA.Unlock(lockIDA); err != nil { t.Fatal("error unlocking client A", err) } - if _, err := lockerB.Lock(infoB); err != nil { + lockIDB, err := lockerB.Lock(infoB) + if err != nil { t.Fatal("unable to obtain lock from client B") } - if err := lockerB.Unlock(""); err != nil { + if err = lockerB.Unlock(lockIDB); err != nil { t.Fatal("error unlocking client B:", err) } - // unlock should be repeatable - if err := lockerA.Unlock(""); err != nil { - t.Fatal("Unlock error from client A when state was not locked:", err) - } + // TODO: Should we enforce that Unlock requires the correct ID? } From 08cff7cc13f260cd0c4ed5df67f1541e845eefda Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 10:53:04 -0500 Subject: [PATCH 10/15] have LocalState check Lock ID on Unlock Have LocalState store and check the lock ID, and strictly enforce unlocking with the correct ID. This isn't required for local lock correctness, as we track the file descriptor to unlock, but it does provide a varification that locking and unlocking is done correctly throughout terraform. --- state/local.go | 28 ++++++++++++++++++++++++---- state/local_test.go | 7 ++++--- 2 files changed, 28 insertions(+), 7 deletions(-) diff --git a/state/local.go b/state/local.go index c92cc8e48..43bb6c66f 100644 --- a/state/local.go +++ b/state/local.go @@ -10,6 +10,7 @@ import ( "path/filepath" "time" + "github.com/hashicorp/errwrap" "github.com/hashicorp/terraform/terraform" ) @@ -24,6 +25,11 @@ type LocalState struct { // the file handle corresponding to PathOut stateFileOut *os.File + // While the stateFileOut will correspond to the lock directly, + // store and check the lock ID to maintain a strict state.Locker + // implementation. + lockID string + // created is set to true if stateFileOut didn't exist before we created it. // This is mostly so we can clean up emtpy files during tests, but doesn't // hurt to remove file we never wrote to. @@ -149,13 +155,26 @@ func (s *LocalState) Lock(info *LockInfo) (string, error) { return "", fmt.Errorf("state file %q locked: %s", s.Path, err) } - return "", s.writeLockInfo(info) + s.lockID = info.ID + return s.lockID, s.writeLockInfo(info) } func (s *LocalState) Unlock(id string) error { - // we can't be locked if we don't have a file - if s.stateFileOut == nil { - return nil + if s.lockID == "" { + return fmt.Errorf("LocalState not locked") + } + + if id != s.lockID { + idErr := fmt.Errorf("invalid lock id: %q. current id: %q", id, s.lockID) + info, err := s.lockInfo() + if err == nil { + return errwrap.Wrap(idErr, err) + } + + return &LockError{ + Err: idErr, + Info: info, + } } os.Remove(s.lockInfoPath()) @@ -166,6 +185,7 @@ func (s *LocalState) Unlock(id string) error { s.stateFileOut.Close() s.stateFileOut = nil + s.lockID = "" // clean up the state file if we created it an never wrote to it stat, err := os.Stat(fileName) diff --git a/state/local_test.go b/state/local_test.go index b0472aaa6..27d176384 100644 --- a/state/local_test.go +++ b/state/local_test.go @@ -57,12 +57,13 @@ func TestLocalStateLocks(t *testing.T) { t.Fatal(err) } - // Unlock should be repeatable if err := s.Unlock(lockID); err != nil { t.Fatal(err) } - if err := s.Unlock(lockID); err != nil { - t.Fatal(err) + + // we should not be able to unlock the same lock twice + if err := s.Unlock(lockID); err == nil { + t.Fatal("unlocking an unlocked state should fail") } // make sure lock info is gone From f2e496a14cdfd7eeb071f8231774cfea09f74f61 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 11:53:19 -0500 Subject: [PATCH 11/15] Have backend operations properly unlock state Make sure unlock is called with the correct LockID during operations --- backend/local/backend_apply.go | 12 ++++++++++-- backend/local/backend_local.go | 10 ---------- backend/local/backend_plan.go | 12 ++++++++++-- backend/local/backend_refresh.go | 12 ++++++++++-- command/apply_destroy_test.go | 1 + command/command_test.go | 3 ++- command/testdata/statelocker.go | 2 +- command/unlock.go | 19 ++++++++++++++++--- command/unlock_test.go | 9 +++++++-- 9 files changed, 57 insertions(+), 23 deletions(-) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index caffaa983..466bce468 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" clistate "github.com/hashicorp/terraform/command/state" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -35,10 +36,17 @@ func (b *Local) opApply( return } - // If we're locking state, unlock when we're done if op.LockState { + lockInfo := state.NewLockInfo() + lockInfo.Operation = op.Type.String() + lockID, err := clistate.Lock(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, "", b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/backend/local/backend_local.go b/backend/local/backend_local.go index e7a98b6e4..0ecba44ce 100644 --- a/backend/local/backend_local.go +++ b/backend/local/backend_local.go @@ -8,7 +8,6 @@ import ( "github.com/hashicorp/errwrap" "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" - clistate "github.com/hashicorp/terraform/command/state" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -29,15 +28,6 @@ func (b *Local) context(op *backend.Operation) (*terraform.Context, state.State, return nil, nil, errwrap.Wrapf("Error loading state: {{err}}", err) } - if op.LockState { - lockInfo := state.NewLockInfo() - lockInfo.Operation = op.Type.String() - _, err := clistate.Lock(s, lockInfo, b.CLI, b.Colorize()) - if 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 70587cb36..afb483dad 100644 --- a/backend/local/backend_plan.go +++ b/backend/local/backend_plan.go @@ -13,6 +13,7 @@ import ( "github.com/hashicorp/terraform/command/format" clistate "github.com/hashicorp/terraform/command/state" "github.com/hashicorp/terraform/config/module" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) @@ -59,10 +60,17 @@ func (b *Local) opPlan( return } - // If we're locking state, unlock when we're done if op.LockState { + lockInfo := state.NewLockInfo() + lockInfo.Operation = op.Type.String() + lockID, err := clistate.Lock(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, "", b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/backend/local/backend_refresh.go b/backend/local/backend_refresh.go index 6a939d491..5c09597e8 100644 --- a/backend/local/backend_refresh.go +++ b/backend/local/backend_refresh.go @@ -9,6 +9,7 @@ import ( "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/backend" clistate "github.com/hashicorp/terraform/command/state" + "github.com/hashicorp/terraform/state" ) func (b *Local) opRefresh( @@ -48,10 +49,17 @@ func (b *Local) opRefresh( return } - // If we're locking state, unlock when we're done if op.LockState { + lockInfo := state.NewLockInfo() + lockInfo.Operation = op.Type.String() + lockID, err := clistate.Lock(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, "", b.CLI, b.Colorize()); err != nil { + if err := clistate.Unlock(opState, lockID, b.CLI, b.Colorize()); err != nil { runningOp.Err = multierror.Append(runningOp.Err, err) } }() diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index f8e37f8cb..e71072616 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -45,6 +45,7 @@ func TestApply_destroy(t *testing.T) { testFixturePath("apply"), } if code := c.Run(args); code != 0 { + t.Log(ui.OutputWriter.String()) t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) } diff --git a/command/command_test.go b/command/command_test.go index abf134331..cb89f5e75 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -583,7 +583,8 @@ func testLockState(sourceDir, path string) (func(), error) { return deferFunc, fmt.Errorf("read from statelocker returned: %s", err) } - if string(buf[:n]) != "LOCKED" { + output := string(buf[:n]) + if !strings.HasPrefix(output, "LOCKID") { return deferFunc, fmt.Errorf("statelocker wrote: %s", string(buf[:n])) } return deferFunc, nil diff --git a/command/testdata/statelocker.go b/command/testdata/statelocker.go index 7bdce2112..e3a7f678f 100644 --- a/command/testdata/statelocker.go +++ b/command/testdata/statelocker.go @@ -34,7 +34,7 @@ func main() { } // signal to the caller that we're locked - io.WriteString(os.Stdout, "LOCKED") + io.WriteString(os.Stdout, "LOCKID "+lockID) defer func() { if err := s.Unlock(lockID); err != nil { diff --git a/command/unlock.go b/command/unlock.go index 952d9bacf..2e197e775 100644 --- a/command/unlock.go +++ b/command/unlock.go @@ -6,6 +6,7 @@ import ( "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" + "github.com/mitchellh/cli" ) // UnlockCommand is a cli.Command implementation that manually unlocks @@ -25,9 +26,21 @@ func (c *UnlockCommand) Run(args []string) int { return 1 } + args = cmdFlags.Args() + if len(args) == 0 { + c.Ui.Error("unlock requires a lock id argument") + return cli.RunResultHelp + } + + lockID := args[0] + + if len(args) > 1 { + args = args[1:] + } + // assume everything is initialized. The user can manually init if this is // required. - configPath, err := ModulePath(cmdFlags.Args()) + configPath, err := ModulePath(args) if err != nil { c.Ui.Error(err.Error()) return 1 @@ -93,7 +106,7 @@ func (c *UnlockCommand) Run(args []string) int { } // FIXME: unlock should require the lock ID - if err := s.Unlock(""); err != nil { + if err := s.Unlock(lockID); err != nil { c.Ui.Error(fmt.Sprintf("Failed to unlock state: %s", err)) return 1 } @@ -104,7 +117,7 @@ func (c *UnlockCommand) Run(args []string) int { func (c *UnlockCommand) Help() string { helpText := ` -Usage: terraform force-unlock [DIR] +Usage: terraform force-unlock LOCK_ID [DIR] Manually unlock the state for the defined configuration. diff --git a/command/unlock_test.go b/command/unlock_test.go index 2496f3fe6..761701b2f 100644 --- a/command/unlock_test.go +++ b/command/unlock_test.go @@ -40,7 +40,12 @@ func TestUnlock(t *testing.T) { }, } - if code := c.Run([]string{"-force"}); code != 0 { - t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + args := []string{ + "-force", + "LOCK_ID", + } + + if code := c.Run(args); code != 1 { + t.Fatalf("bad: %d\n%s\n%s", code, ui.OutputWriter.String(), ui.ErrorWriter.String()) } } From 67bbebce08ec168fc2aa9d024cbcce12f1c4bf9b Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 12:02:37 -0500 Subject: [PATCH 12/15] Have consul state reutrn the lock ID The lock ID isn't used because the lock is tied to the client, but return the lock ID to match the behavior of other locks. --- backend/remote-state/consul/client.go | 7 ++++--- state/remote/testing.go | 4 ++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index a5578a3c1..8b4ed6e7e 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -120,7 +120,7 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { lock, err := c.Client.LockOpts(opts) if err != nil { - return "", nil + return "", err } c.consulLock = lock @@ -143,14 +143,15 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { err = c.putLockInfo(info) if err != nil { - err = multierror.Append(err, c.Unlock("")) + err = multierror.Append(err, c.Unlock(info.ID)) return "", err } - return "", nil + return info.ID, nil } func (c *RemoteClient) Unlock(id string) error { + // this doesn't use the lock id, because the lock is tied to the consul client. if c.consulLock == nil || c.lockCh == nil { return nil } diff --git a/state/remote/testing.go b/state/remote/testing.go index bd71aefcb..b379b509d 100644 --- a/state/remote/testing.go +++ b/state/remote/testing.go @@ -85,6 +85,10 @@ func TestRemoteLocks(t *testing.T, a, b Client) { t.Fatal("unable to obtain lock from client B") } + if lockIDB == lockIDA { + t.Fatalf("duplicate lock IDs: %q", lockIDB) + } + if err = lockerB.Unlock(lockIDB); err != nil { t.Fatal("error unlocking client B:", err) } From 888af93356fcbf59bd6a86dbaae94e9cdde8a073 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 12:51:57 -0500 Subject: [PATCH 13/15] Have S3 check the lockID on Unlock This needs to be improved to happen in a transaction, but it gets the implementation passing the new tests. --- state/remote/s3.go | 67 ++++++++++++++++++++++++++++++---------------- 1 file changed, 44 insertions(+), 23 deletions(-) diff --git a/state/remote/s3.go b/state/remote/s3.go index b5c245ad2..94c93d81a 100644 --- a/state/remote/s3.go +++ b/state/remote/s3.go @@ -231,28 +231,9 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { _, err := c.dynClient.PutItem(putParams) if err != nil { - getParams := &dynamodb.GetItemInput{ - Key: map[string]*dynamodb.AttributeValue{ - "LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.keyName))}, - }, - ProjectionExpression: aws.String("LockID, Created, Info"), - TableName: aws.String(c.lockTable), - } - - resp, err := c.dynClient.GetItem(getParams) + lockInfo, err := c.getLockInfo() if err != nil { - return info.ID, fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) - } - - var infoData string - if v, ok := resp.Item["Info"]; ok && v.S != nil { - infoData = *v.S - } - - lockInfo := &state.LockInfo{} - err = json.Unmarshal([]byte(infoData), lockInfo) - if err != nil { - return info.ID, fmt.Errorf("s3 state file %q locked, failed get lock info: %s", stateName, err) + return "", fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) } return info.ID, lockInfo.Err() @@ -260,18 +241,58 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { return info.ID, nil } -func (c *S3Client) Unlock(string) error { +func (c *S3Client) getLockInfo() (*state.LockInfo, error) { + getParams := &dynamodb.GetItemInput{ + Key: map[string]*dynamodb.AttributeValue{ + "LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.keyName))}, + }, + ProjectionExpression: aws.String("LockID, Info"), + TableName: aws.String(c.lockTable), + } + + resp, err := c.dynClient.GetItem(getParams) + if err != nil { + return nil, err + } + + var infoData string + if v, ok := resp.Item["Info"]; ok && v.S != nil { + infoData = *v.S + } + + lockInfo := &state.LockInfo{} + err = json.Unmarshal([]byte(infoData), lockInfo) + if err != nil { + return nil, err + } + + return lockInfo, nil +} + +func (c *S3Client) Unlock(id string) error { if c.lockTable == "" { return nil } + // TODO: store the path and lock ID in separate fields, and have proper + // projection expression only delete the lock if both match, rather than + // checking the ID from the info field first. + lockInfo, err := c.getLockInfo() + if err != nil { + return fmt.Errorf("failed to retrieve lock info: %s", err) + } + + if lockInfo.ID != id { + return fmt.Errorf("lock id %q does not match existing lock", id) + } + params := &dynamodb.DeleteItemInput{ Key: map[string]*dynamodb.AttributeValue{ "LockID": {S: aws.String(fmt.Sprintf("%s/%s", c.bucketName, c.keyName))}, }, TableName: aws.String(c.lockTable), } - _, err := c.dynClient.DeleteItem(params) + _, err = c.dynClient.DeleteItem(params) if err != nil { return err From ec00564be6c94ae503b72d01841b2a52e37470e6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 14:01:18 -0500 Subject: [PATCH 14/15] Clean up LockInfo and LockError and use them Gove LockInfo a Marshal method for easy serialization, and a String method for more readable output. Have the state.Locker implementations use LockError when possible to return LockInfo and an error. --- backend/remote-state/consul/client.go | 30 +++++++++++--------- command/apply_destroy_test.go | 2 +- command/apply_test.go | 2 +- command/plan_test.go | 2 +- command/refresh_test.go | 2 +- command/taint_test.go | 2 +- command/untaint_test.go | 2 +- state/local.go | 25 +++++++++-------- state/remote/s3.go | 26 +++++++++++------ state/state.go | 40 ++++++++++++++++++++++----- state/state_test.go | 8 ++++++ 11 files changed, 95 insertions(+), 46 deletions(-) diff --git a/backend/remote-state/consul/client.go b/backend/remote-state/consul/client.go index 8b4ed6e7e..8a26165e8 100644 --- a/backend/remote-state/consul/client.go +++ b/backend/remote-state/consul/client.go @@ -4,10 +4,10 @@ import ( "crypto/md5" "encoding/json" "errors" + "fmt" "time" consulapi "github.com/hashicorp/consul/api" - "github.com/hashicorp/errwrap" multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/state/remote" @@ -62,15 +62,10 @@ func (c *RemoteClient) putLockInfo(info *state.LockInfo) error { info.Path = c.Path info.Created = time.Now().UTC() - js, err := json.Marshal(info) - if err != nil { - return err - } - kv := c.Client.KV() - _, err = kv.Put(&consulapi.KVPair{ + _, err := kv.Put(&consulapi.KVPair{ Key: c.Path + lockInfoSuffix, - Value: js, + Value: info.Marshal(), }, nil) return err @@ -89,7 +84,7 @@ func (c *RemoteClient) getLockInfo() (*state.LockInfo, error) { li := &state.LockInfo{} err = json.Unmarshal(pair.Value, li) if err != nil { - return nil, errwrap.Wrapf("error unmarshaling lock info: {{err}}", err) + return nil, fmt.Errorf("error unmarshaling lock info: %s", err) } return li, nil @@ -126,24 +121,33 @@ func (c *RemoteClient) Lock(info *state.LockInfo) (string, error) { c.consulLock = lock } + lockErr := &state.LockError{} + lockCh, err := c.consulLock.Lock(make(chan struct{})) if err != nil { - return "", err + lockErr.Err = err + return "", lockErr } if lockCh == nil { lockInfo, e := c.getLockInfo() if e != nil { - return "", e + lockErr.Err = e + return "", lockErr } - return "", lockInfo.Err() + + lockErr.Info = lockInfo + return "", lockErr } c.lockCh = lockCh err = c.putLockInfo(info) if err != nil { - err = multierror.Append(err, c.Unlock(info.ID)) + if unlockErr := c.Unlock(info.ID); unlockErr != nil { + err = multierror.Append(err, unlockErr) + } + return "", err } diff --git a/command/apply_destroy_test.go b/command/apply_destroy_test.go index e71072616..376c5e29e 100644 --- a/command/apply_destroy_test.go +++ b/command/apply_destroy_test.go @@ -140,7 +140,7 @@ func TestApply_destroyLockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/command/apply_test.go b/command/apply_test.go index 86028d52f..7f21aa1c2 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -87,7 +87,7 @@ func TestApply_lockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/command/plan_test.go b/command/plan_test.go index b2f628fc6..6fd7d20cf 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -72,7 +72,7 @@ func TestPlan_lockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/command/refresh_test.go b/command/refresh_test.go index 52aa33112..c196db394 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -91,7 +91,7 @@ func TestRefresh_lockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/command/taint_test.go b/command/taint_test.go index 9aa9b2664..c658ff0a3 100644 --- a/command/taint_test.go +++ b/command/taint_test.go @@ -84,7 +84,7 @@ func TestTaint_lockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/command/untaint_test.go b/command/untaint_test.go index 80bf7bd72..0cc27ca0d 100644 --- a/command/untaint_test.go +++ b/command/untaint_test.go @@ -90,7 +90,7 @@ func TestUntaint_lockedState(t *testing.T) { } output := ui.ErrorWriter.String() - if !strings.Contains(output, "locked") { + if !strings.Contains(output, "lock") { t.Fatal("command output does not look like a lock error:", output) } } diff --git a/state/local.go b/state/local.go index 43bb6c66f..3a9e40c43 100644 --- a/state/local.go +++ b/state/local.go @@ -10,7 +10,7 @@ import ( "path/filepath" "time" - "github.com/hashicorp/errwrap" + multierror "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/terraform" ) @@ -148,11 +148,17 @@ func (s *LocalState) Lock(info *LockInfo) (string, error) { } if err := s.lock(); err != nil { - if info, err := s.lockInfo(); err == nil { - return "", info.Err() + info, infoErr := s.lockInfo() + if infoErr != nil { + err = multierror.Append(err, infoErr) } - return "", fmt.Errorf("state file %q locked: %s", s.Path, err) + lockErr := &LockError{ + Info: info, + Err: err, + } + + return "", lockErr } s.lockID = info.ID @@ -167,8 +173,8 @@ func (s *LocalState) Unlock(id string) error { if id != s.lockID { idErr := fmt.Errorf("invalid lock id: %q. current id: %q", id, s.lockID) info, err := s.lockInfo() - if err == nil { - return errwrap.Wrap(idErr, err) + if err != nil { + err = multierror.Append(idErr, err) } return &LockError{ @@ -257,12 +263,7 @@ func (s *LocalState) writeLockInfo(info *LockInfo) error { info.Path = s.Path info.Created = time.Now().UTC() - infoData, err := json.Marshal(info) - if err != nil { - panic(fmt.Sprintf("could not marshal lock info: %#v", info)) - } - - err = ioutil.WriteFile(path, infoData, 0600) + err := ioutil.WriteFile(path, info.Marshal(), 0600) if err != nil { return fmt.Errorf("could not write lock info for %q: %s", s.Path, err) } diff --git a/state/remote/s3.go b/state/remote/s3.go index 94c93d81a..d9799e437 100644 --- a/state/remote/s3.go +++ b/state/remote/s3.go @@ -223,7 +223,7 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { putParams := &dynamodb.PutItemInput{ Item: map[string]*dynamodb.AttributeValue{ "LockID": {S: aws.String(stateName)}, - "Info": {S: aws.String(info.String())}, + "Info": {S: aws.String(string(info.Marshal()))}, }, TableName: aws.String(c.lockTable), ConditionExpression: aws.String("attribute_not_exists(LockID)"), @@ -231,12 +231,16 @@ func (c *S3Client) Lock(info *state.LockInfo) (string, error) { _, err := c.dynClient.PutItem(putParams) if err != nil { - lockInfo, err := c.getLockInfo() - if err != nil { - return "", fmt.Errorf("s3 state file %q locked, failed to retrieve info: %s", stateName, err) + lockInfo, infoErr := c.getLockInfo() + if infoErr != nil { + err = multierror.Append(err, infoErr) } - return info.ID, lockInfo.Err() + lockErr := &state.LockError{ + Err: err, + Info: lockInfo, + } + return "", lockErr } return info.ID, nil } @@ -274,16 +278,21 @@ func (c *S3Client) Unlock(id string) error { return nil } + lockErr := &state.LockError{} + // TODO: store the path and lock ID in separate fields, and have proper // projection expression only delete the lock if both match, rather than // checking the ID from the info field first. lockInfo, err := c.getLockInfo() if err != nil { - return fmt.Errorf("failed to retrieve lock info: %s", err) + lockErr.Err = fmt.Errorf("failed to retrieve lock info: %s", err) + return lockErr } + lockErr.Info = lockInfo if lockInfo.ID != id { - return fmt.Errorf("lock id %q does not match existing lock", id) + lockErr.Err = fmt.Errorf("lock id %q does not match existing lock", id) + return lockErr } params := &dynamodb.DeleteItemInput{ @@ -295,7 +304,8 @@ func (c *S3Client) Unlock(id string) error { _, err = c.dynClient.DeleteItem(params) if err != nil { - return err + lockErr.Err = err + return lockErr } return nil } diff --git a/state/state.go b/state/state.go index dd74f9497..9491958a3 100644 --- a/state/state.go +++ b/state/state.go @@ -1,12 +1,15 @@ package state import ( + "bytes" "encoding/json" + "errors" "fmt" "math/rand" "os" "os/user" "strings" + "text/template" "time" uuid "github.com/hashicorp/go-uuid" @@ -84,12 +87,15 @@ func NewLockInfo() *LockInfo { } // don't error out on user and hostname, as we don't require them - username, _ := user.Current() + userName := "" + if userInfo, err := user.Current(); err == nil { + userName = userInfo.Username + } host, _ := os.Hostname() info := &LockInfo{ ID: id, - Who: fmt.Sprintf("%s@%s", username, host), + Who: fmt.Sprintf("%s@%s", userName, host), Version: terraform.Version, Created: time.Now().UTC(), } @@ -123,16 +129,36 @@ type LockInfo struct { // Err returns the lock info formatted in an error func (l *LockInfo) Err() error { - return fmt.Errorf("state locked. path:%q, created:%s, info:%q", - l.Path, l.Created, l.Info) + return errors.New(l.String()) } -func (l *LockInfo) String() string { +// Marshal returns a string json representation of the LockInfo +func (l *LockInfo) Marshal() []byte { js, err := json.Marshal(l) if err != nil { panic(err) } - return string(js) + return js +} + +// String return a multi-line string representation of LockInfo +func (l *LockInfo) String() string { + tmpl := `Lock Info: + ID: {{.ID}} + Path: {{.Path}} + Operation: {{.Operation}} + Who: {{.Who}} + Version: {{.Version}} + Created: {{.Created}} + Info: {{.Info}} +` + + t := template.Must(template.New("LockInfo").Parse(tmpl)) + var out bytes.Buffer + if err := t.Execute(&out, l); err != nil { + panic(err) + } + return out.String() } type LockError struct { @@ -147,7 +173,7 @@ func (e *LockError) Error() string { } if e.Info != nil { - out = append(out, e.Info.Err().Error()) + out = append(out, e.Info.String()) } return strings.Join(out, "\n") } diff --git a/state/state_test.go b/state/state_test.go index a70fb52b4..e93f5680a 100644 --- a/state/state_test.go +++ b/state/state_test.go @@ -1,6 +1,7 @@ package state import ( + "encoding/json" "flag" "io/ioutil" "log" @@ -41,4 +42,11 @@ func TestNewLockInfo(t *testing.T) { if info1.ID == info2.ID { t.Fatal("multiple LockInfo with identical IDs") } + + // test the JSON output is valid + newInfo := &LockInfo{} + err := json.Unmarshal(info1.Marshal(), newInfo) + if err != nil { + t.Fatal(err) + } } From a372e9c54b95a9ea4e803adcc81800e6d25c9fc4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 17:00:54 -0500 Subject: [PATCH 15/15] fix state migration lock info --- command/meta_backend_migrate.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/command/meta_backend_migrate.go b/command/meta_backend_migrate.go index f1225e9d2..03a73e2b6 100644 --- a/command/meta_backend_migrate.go +++ b/command/meta_backend_migrate.go @@ -25,7 +25,8 @@ import ( // This will attempt to lock both states for the migration. func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error { lockInfoOne := state.NewLockInfo() - lockInfoOne.Operation = "migration source state" + lockInfoOne.Operation = "migration" + lockInfoOne.Info = "source state" lockIDOne, err := clistate.Lock(opts.One, lockInfoOne, m.Ui, m.Colorize()) if err != nil { @@ -34,7 +35,8 @@ func (m *Meta) backendMigrateState(opts *backendMigrateOpts) error { defer clistate.Unlock(opts.One, lockIDOne, m.Ui, m.Colorize()) lockInfoTwo := state.NewLockInfo() - lockInfoTwo.Operation = "migration source state" + lockInfoTwo.Operation = "migration" + lockInfoTwo.Info = "destination state" lockIDTwo, err := clistate.Lock(opts.Two, lockInfoTwo, m.Ui, m.Colorize()) if err != nil {