From ec00564be6c94ae503b72d01841b2a52e37470e6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 15 Feb 2017 14:01:18 -0500 Subject: [PATCH] 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 8b4ed6e7ef..8a26165e85 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 e71072616c..376c5e29e3 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 86028d52fb..7f21aa1c2f 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 b2f628fc62..6fd7d20cf2 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 52aa33112d..c196db394d 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 9aa9b2664e..c658ff0a3a 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 80bf7bd72a..0cc27ca0d0 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 43bb6c66f4..3a9e40c43a 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 94c93d81a2..d9799e4373 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 dd74f94971..9491958a39 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 a70fb52b4c..e93f5680ae 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) + } }