From b3795e2d297842de77f1e3ef22aa7ba1375fd145 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 24 May 2017 16:16:35 -0400 Subject: [PATCH 1/5] remove old state hook code --- command/hook_state.go | 33 --------------------------------- command/hook_state_test.go | 29 ----------------------------- 2 files changed, 62 deletions(-) delete mode 100644 command/hook_state.go delete mode 100644 command/hook_state_test.go diff --git a/command/hook_state.go b/command/hook_state.go deleted file mode 100644 index ab5c47a11..000000000 --- a/command/hook_state.go +++ /dev/null @@ -1,33 +0,0 @@ -package command - -import ( - "sync" - - "github.com/hashicorp/terraform/state" - "github.com/hashicorp/terraform/terraform" -) - -// StateHook is a hook that continuously updates the state by calling -// WriteState on a state.State. -type StateHook struct { - terraform.NilHook - sync.Mutex - - State state.State -} - -func (h *StateHook) PostStateUpdate( - s *terraform.State) (terraform.HookAction, error) { - h.Lock() - defer h.Unlock() - - if h.State != nil { - // Write the new state - if err := h.State.WriteState(s); err != nil { - return terraform.HookActionHalt, err - } - } - - // Continue forth - return terraform.HookActionContinue, nil -} diff --git a/command/hook_state_test.go b/command/hook_state_test.go deleted file mode 100644 index 0d0fd7927..000000000 --- a/command/hook_state_test.go +++ /dev/null @@ -1,29 +0,0 @@ -package command - -import ( - "testing" - - "github.com/hashicorp/terraform/state" - "github.com/hashicorp/terraform/terraform" -) - -func TestStateHook_impl(t *testing.T) { - var _ terraform.Hook = new(StateHook) -} - -func TestStateHook(t *testing.T) { - is := &state.InmemState{} - var hook terraform.Hook = &StateHook{State: is} - - s := state.TestStateInitial() - action, err := hook.PostStateUpdate(s) - if err != nil { - t.Fatalf("err: %s", err) - } - if action != terraform.HookActionContinue { - t.Fatalf("bad: %v", action) - } - if !is.State().Equal(s) { - t.Fatalf("bad state: %#v", is.State()) - } -} From b73d03776163466e6ed9a11d08670d78f408dd46 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 25 May 2017 09:18:42 -0400 Subject: [PATCH 2/5] have StateHook periodically PersistState Have StateHook periodically call PersistState to flush any cached state to permanent storage. This uses a minimal 10 second interval between calls to PersistState. --- backend/local/hook_state.go | 26 +++++++++ backend/local/hook_state_test.go | 97 ++++++++++++++++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/backend/local/hook_state.go b/backend/local/hook_state.go index 5483c4344..62e33416e 100644 --- a/backend/local/hook_state.go +++ b/backend/local/hook_state.go @@ -2,17 +2,27 @@ package local import ( "sync" + "time" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" ) +// interval between forced PersistState calls by StateHook +const persistStateHookInterval = 10 * time.Second + // StateHook is a hook that continuously updates the state by calling // WriteState on a state.State. type StateHook struct { terraform.NilHook sync.Mutex + // lastPersist is the time of the last call to PersistState, for periodic + // updates to remote state. PostStateUpdate will force a call PersistState + // if it has been more that persistStateHookInterval since the last call to + // PersistState. + lastPersist time.Time + State state.State } @@ -26,8 +36,24 @@ func (h *StateHook) PostStateUpdate( if err := h.State.WriteState(s); err != nil { return terraform.HookActionHalt, err } + + // periodically persist the state + if time.Since(h.lastPersist) > persistStateHookInterval { + if err := h.persistState(); err != nil { + return terraform.HookActionHalt, err + } + } } // Continue forth return terraform.HookActionContinue, nil } + +func (h *StateHook) persistState() error { + if h.State != nil { + err := h.State.PersistState() + h.lastPersist = time.Now() + return err + } + return nil +} diff --git a/backend/local/hook_state_test.go b/backend/local/hook_state_test.go index 7f4d88770..1614942d6 100644 --- a/backend/local/hook_state_test.go +++ b/backend/local/hook_state_test.go @@ -1,7 +1,9 @@ package local import ( + "sync" "testing" + "time" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" @@ -27,3 +29,98 @@ func TestStateHook(t *testing.T) { t.Fatalf("bad state: %#v", is.State()) } } + +// testPersistState stores the state on WriteState, and +type testPersistState struct { + *state.InmemState + + mu sync.Mutex + persisted bool +} + +func (s *testPersistState) WriteState(state *terraform.State) error { + s.mu.Lock() + defer s.mu.Unlock() + + s.persisted = false + return s.InmemState.WriteState(state) +} + +func (s *testPersistState) PersistState() error { + s.mu.Lock() + defer s.mu.Unlock() + + s.persisted = true + return nil +} + +// verify that StateHook calls PersistState if the last call was more than +// persistStateHookInterval +func TestStateHookPersist(t *testing.T) { + is := &testPersistState{ + InmemState: &state.InmemState{}, + } + hook := &StateHook{State: is} + + s := state.TestStateInitial() + hook.PostStateUpdate(s) + + // the first call should persist, since the last time was zero + if !is.persisted { + t.Fatal("PersistState not called") + } + + s.Serial++ + hook.PostStateUpdate(s) + + // this call should not have persisted + if is.persisted { + t.Fatal("PostStateUpdate called PersistState early") + } + + if !is.State().Equal(s) { + t.Fatalf("bad state: %#v", is.State()) + } + + // set the last call back to before our interval + hook.lastPersist = time.Now().Add(-2 * persistStateHookInterval) + + s.Serial++ + hook.PostStateUpdate(s) + + if !is.persisted { + t.Fatal("PersistState not called") + } + + if !is.State().Equal(s) { + t.Fatalf("bad state: %#v", is.State()) + } +} + +// verify that the satet hook is safe for concurrent use +func TestStateHookRace(t *testing.T) { + is := &state.InmemState{} + var hook terraform.Hook = &StateHook{State: is} + + s := state.TestStateInitial() + + var wg sync.WaitGroup + + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + action, err := hook.PostStateUpdate(s) + if err != nil { + t.Fatalf("err: %s", err) + } + if action != terraform.HookActionContinue { + t.Fatalf("bad: %v", action) + } + if !is.State().Equal(s) { + t.Fatalf("bad state: %#v", is.State()) + } + }() + } + wg.Wait() +} From 9e4c0ff2adb2af2d590fc4d954d98d89346b16f3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 25 May 2017 09:29:51 -0400 Subject: [PATCH 3/5] call PersistState immediately when cancelling When the backend operation is cancelled, immediately call PersistState. The is a high likelihood that the user is going to terminate the process early if the provider doesn't return in a timely manner, so persist as much state as possible. --- backend/local/backend_apply.go | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/backend/local/backend_apply.go b/backend/local/backend_apply.go index 27883d5b2..269095940 100644 --- a/backend/local/backend_apply.go +++ b/backend/local/backend_apply.go @@ -126,6 +126,17 @@ func (b *Local) opApply( b.CLI.Output("stopping apply operation...") } + // try to force a PersistState just in case the process is terminated + // before we can complete. + if err := opState.PersistState(); err != nil { + // We can't error out from here, but warn the user if there was an error. + // If this isn't transient, we will catch it again below, and + // attempt to save the state another way. + if b.CLI != nil { + b.CLI.Error(fmt.Sprintf(earlyStateWriteErrorFmt, err)) + } + } + // Stop execution go tfCtx.Stop() @@ -270,3 +281,10 @@ importing each resource using its id from the target system. This is a serious bug in Terraform and should be reported. ` + +const earlyStateWriteErrorFmt = `Error saving current state: %s + +Terraform encountered an error attempting to save the state before canceling +the current operation. Once the operation is complete another attempt will be +made to save the final state. +` From f0f2220abb592d46a50b4f3d1010bf9a1e4739d2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 25 May 2017 11:01:25 -0400 Subject: [PATCH 4/5] add mutexes to remote.State --- state/remote/state.go | 21 +++++++++++++++++++++ state/remote/state_test.go | 22 ++++++++++++++++++++++ 2 files changed, 43 insertions(+) diff --git a/state/remote/state.go b/state/remote/state.go index c87416705..dccbab18a 100644 --- a/state/remote/state.go +++ b/state/remote/state.go @@ -2,6 +2,7 @@ package remote import ( "bytes" + "sync" "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" @@ -12,6 +13,8 @@ import ( // local caching so every persist will go to the remote storage and local // writes will go to memory. type State struct { + mu sync.Mutex + Client Client state, readState *terraform.State @@ -19,17 +22,26 @@ type State struct { // StateReader impl. func (s *State) State() *terraform.State { + s.mu.Lock() + defer s.mu.Unlock() + return s.state.DeepCopy() } // StateWriter impl. func (s *State) WriteState(state *terraform.State) error { + s.mu.Lock() + defer s.mu.Unlock() + s.state = state return nil } // StateRefresher impl. func (s *State) RefreshState() error { + s.mu.Lock() + defer s.mu.Unlock() + payload, err := s.Client.Get() if err != nil { return err @@ -52,6 +64,9 @@ func (s *State) RefreshState() error { // StatePersister impl. func (s *State) PersistState() error { + s.mu.Lock() + defer s.mu.Unlock() + s.state.IncrementSerialMaybe(s.readState) var buf bytes.Buffer @@ -64,6 +79,9 @@ func (s *State) PersistState() error { // Lock calls the Client's Lock method if it's implemented. func (s *State) Lock(info *state.LockInfo) (string, error) { + s.mu.Lock() + defer s.mu.Unlock() + if c, ok := s.Client.(ClientLocker); ok { return c.Lock(info) } @@ -72,6 +90,9 @@ func (s *State) Lock(info *state.LockInfo) (string, error) { // Unlock calls the Client's Unlock method if it's implemented. func (s *State) Unlock(id string) error { + s.mu.Lock() + defer s.mu.Unlock() + if c, ok := s.Client.(ClientLocker); ok { return c.Unlock(id) } diff --git a/state/remote/state_test.go b/state/remote/state_test.go index 90a60e9c4..26a031b7e 100644 --- a/state/remote/state_test.go +++ b/state/remote/state_test.go @@ -1,6 +1,7 @@ package remote import ( + "sync" "testing" "github.com/hashicorp/terraform/state" @@ -13,3 +14,24 @@ func TestState_impl(t *testing.T) { var _ state.StateRefresher = new(State) var _ state.Locker = new(State) } + +func TestStateRace(t *testing.T) { + s := &State{ + Client: nilClient{}, + } + + current := state.TestStateInitial() + + var wg sync.WaitGroup + + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + s.WriteState(current) + s.PersistState() + s.RefreshState() + }() + } + wg.Wait() +} From 4866f35645ddeedbd84f82180f024b38842f2373 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Thu, 25 May 2017 11:05:48 -0400 Subject: [PATCH 5/5] add mutexes to Local, Backup, and InmemState --- state/backup.go | 13 ++++++++++++- state/backup_test.go | 32 ++++++++++++++++++++++++++++++++ state/inmem.go | 10 ++++++++++ state/local.go | 18 ++++++++++++++++++ state/local_test.go | 17 +++++++++++++++++ 5 files changed, 89 insertions(+), 1 deletion(-) diff --git a/state/backup.go b/state/backup.go index c357bba49..047258f4d 100644 --- a/state/backup.go +++ b/state/backup.go @@ -1,12 +1,17 @@ package state -import "github.com/hashicorp/terraform/terraform" +import ( + "sync" + + "github.com/hashicorp/terraform/terraform" +) // BackupState wraps a State that backs up the state on the first time that // a WriteState or PersistState is called. // // If Path exists, it will be overwritten. type BackupState struct { + mu sync.Mutex Real State Path string @@ -22,6 +27,9 @@ func (s *BackupState) RefreshState() error { } func (s *BackupState) WriteState(state *terraform.State) error { + s.mu.Lock() + defer s.mu.Unlock() + if !s.done { if err := s.backup(); err != nil { return err @@ -32,6 +40,9 @@ func (s *BackupState) WriteState(state *terraform.State) error { } func (s *BackupState) PersistState() error { + s.mu.Lock() + defer s.mu.Unlock() + if !s.done { if err := s.backup(); err != nil { return err diff --git a/state/backup_test.go b/state/backup_test.go index 85f722863..8ef0afec6 100644 --- a/state/backup_test.go +++ b/state/backup_test.go @@ -3,6 +3,7 @@ package state import ( "io/ioutil" "os" + "sync" "testing" ) @@ -31,3 +32,34 @@ func TestBackupState(t *testing.T) { t.Fatalf("bad: %d", fi.Size()) } } + +func TestBackupStateRace(t *testing.T) { + f, err := ioutil.TempFile("", "tf") + if err != nil { + t.Fatalf("err: %s", err) + } + f.Close() + defer os.Remove(f.Name()) + + ls := testLocalState(t) + defer os.Remove(ls.Path) + bs := &BackupState{ + Real: ls, + Path: f.Name(), + } + + current := TestStateInitial() + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + bs.WriteState(current) + bs.PersistState() + bs.RefreshState() + }() + } + + wg.Wait() +} diff --git a/state/inmem.go b/state/inmem.go index 2bbfb3d44..4e031896c 100644 --- a/state/inmem.go +++ b/state/inmem.go @@ -10,18 +10,28 @@ import ( // InmemState is an in-memory state storage. type InmemState struct { + mu sync.Mutex state *terraform.State } func (s *InmemState) State() *terraform.State { + s.mu.Lock() + defer s.mu.Unlock() + return s.state.DeepCopy() } func (s *InmemState) RefreshState() error { + s.mu.Lock() + defer s.mu.Unlock() + return nil } func (s *InmemState) WriteState(state *terraform.State) error { + s.mu.Lock() + defer s.mu.Unlock() + state.IncrementSerialMaybe(s.state) s.state = state return nil diff --git a/state/local.go b/state/local.go index b4029267e..5ce02ce57 100644 --- a/state/local.go +++ b/state/local.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "os" "path/filepath" + "sync" "time" multierror "github.com/hashicorp/go-multierror" @@ -16,6 +17,8 @@ import ( // LocalState manages a state storage that is local to the filesystem. type LocalState struct { + mu sync.Mutex + // Path is the path to read the state from. PathOut is the path to // write the state to. If PathOut is not specified, Path will be used. // If PathOut already exists, it will be overwritten. @@ -42,6 +45,9 @@ type LocalState struct { // SetState will force a specific state in-memory for this local state. func (s *LocalState) SetState(state *terraform.State) { + s.mu.Lock() + defer s.mu.Unlock() + s.state = state s.readState = state } @@ -58,6 +64,9 @@ func (s *LocalState) State() *terraform.State { // // StateWriter impl. func (s *LocalState) WriteState(state *terraform.State) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.stateFileOut == nil { if err := s.createStateFiles(); err != nil { return nil @@ -99,6 +108,9 @@ func (s *LocalState) PersistState() error { // StateRefresher impl. func (s *LocalState) RefreshState() error { + s.mu.Lock() + defer s.mu.Unlock() + var reader io.Reader if !s.written { // we haven't written a state file yet, so load from Path @@ -141,6 +153,9 @@ func (s *LocalState) RefreshState() error { // Lock implements a local filesystem state.Locker. func (s *LocalState) Lock(info *LockInfo) (string, error) { + s.mu.Lock() + defer s.mu.Unlock() + if s.stateFileOut == nil { if err := s.createStateFiles(); err != nil { return "", err @@ -170,6 +185,9 @@ func (s *LocalState) Lock(info *LockInfo) (string, error) { } func (s *LocalState) Unlock(id string) error { + s.mu.Lock() + defer s.mu.Unlock() + if s.lockID == "" { return fmt.Errorf("LocalState not locked") } diff --git a/state/local_test.go b/state/local_test.go index 76abde1ce..633356028 100644 --- a/state/local_test.go +++ b/state/local_test.go @@ -4,6 +4,7 @@ import ( "io/ioutil" "os" "os/exec" + "sync" "testing" "github.com/hashicorp/terraform/terraform" @@ -15,6 +16,22 @@ func TestLocalState(t *testing.T) { TestState(t, ls) } +func TestLocalStateRace(t *testing.T) { + ls := testLocalState(t) + defer os.Remove(ls.Path) + + current := TestStateInitial() + + var wg sync.WaitGroup + for i := 0; i < 100; i++ { + wg.Add(1) + go func() { + defer wg.Done() + ls.WriteState(current) + }() + } +} + func TestLocalStateLocks(t *testing.T) { s := testLocalState(t) defer os.Remove(s.Path)