From 4d53eaa6df2f2f597e18c352ae0ef8baa3e4ed32 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Wed, 5 Jul 2017 12:34:30 -0700 Subject: [PATCH 1/5] state: more robust handling of state Serial Previously we relied on a constellation of coincidences for everything to work out correctly with state serials. In particular, callers needed to be very careful about mutating states (or not) because many different bits of code shared pointers to the same objects. Here we move to a model where all of the state managers always use distinct instances of state, copied when WriteState is called. This means that they are truly a snapshot of the state as it was at that call, even if the caller goes on mutating the state that was passed. We also adjust the handling of serials so that the state managers ignore any serials in incoming states and instead just treat each Persist as the next version after what was most recently Refreshed. (An exception exists for when nothing has been refreshed, e.g. because we are writing a state to a location for the first time. In that case we _do_ trust the caller, since the given state is either a new state or it's a copy of something we're migrating from elsewhere with its state and lineage intact.) The intent here is to allow the rest of Terraform to not worry about serials and state identity, and instead just treat the state as a mutable structure. We'll just snapshot it occasionally, when WriteState is called, and deal with serials _only_ at persist time. This is intended as a more robust version of #15423, which was a quick hotfix to an issue that resulted from our previous slopping handling of state serials but arguably makes the problem worse by depending on an additional coincidental behavior of the local backend's apply implementation. --- command/workspace_command_test.go | 1 + state/inmem.go | 12 ++- state/local.go | 20 +++-- state/remote/state.go | 44 +++++++++- state/state.go | 18 ++++ state/testing.go | 14 +++- terraform/state.go | 65 +++++++++------ terraform/state_test.go | 132 +++++++++++++++++++----------- 8 files changed, 218 insertions(+), 88 deletions(-) diff --git a/command/workspace_command_test.go b/command/workspace_command_test.go index 3329e8565..576dca43b 100644 --- a/command/workspace_command_test.go +++ b/command/workspace_command_test.go @@ -186,6 +186,7 @@ func TestWorkspace_createWithState(t *testing.T) { } newState := envState.State() + originalState.Version = newState.Version // the round-trip through the state manager implicitly populates version if !originalState.Equal(newState) { t.Fatalf("states not equal\norig: %s\nnew: %s", originalState, newState) } diff --git a/state/inmem.go b/state/inmem.go index 4e031896c..36fa34147 100644 --- a/state/inmem.go +++ b/state/inmem.go @@ -32,8 +32,18 @@ func (s *InmemState) WriteState(state *terraform.State) error { s.mu.Lock() defer s.mu.Unlock() - state.IncrementSerialMaybe(s.state) + state = state.DeepCopy() + + if s.state != nil { + state.Serial = s.state.Serial + + if !s.state.MarshalEqual(state) { + state.Serial++ + } + } + s.state = state + return nil } diff --git a/state/local.go b/state/local.go index 5ce02ce57..a6d17653b 100644 --- a/state/local.go +++ b/state/local.go @@ -48,8 +48,8 @@ func (s *LocalState) SetState(state *terraform.State) { s.mu.Lock() defer s.mu.Unlock() - s.state = state - s.readState = state + s.state = state.DeepCopy() + s.readState = state.DeepCopy() } // StateReader impl. @@ -74,7 +74,14 @@ func (s *LocalState) WriteState(state *terraform.State) error { } defer s.stateFileOut.Sync() - s.state = state + s.state = state.DeepCopy() // don't want mutations before we actually get this written to disk + + if s.readState != nil && s.state != nil { + // We don't trust callers to properly manage serials. Instead, we assume + // that a WriteState is always for the next version after what was + // most recently read. + s.state.Serial = s.readState.Serial + } if _, err := s.stateFileOut.Seek(0, os.SEEK_SET); err != nil { return err @@ -88,8 +95,9 @@ func (s *LocalState) WriteState(state *terraform.State) error { return nil } - s.state.IncrementSerialMaybe(s.readState) - s.readState = s.state + if !s.state.MarshalEqual(s.readState) { + s.state.Serial++ + } if err := terraform.WriteState(s.state, s.stateFileOut); err != nil { return err @@ -147,7 +155,7 @@ func (s *LocalState) RefreshState() error { } s.state = state - s.readState = state + s.readState = s.state.DeepCopy() return nil } diff --git a/state/remote/state.go b/state/remote/state.go index dccbab18a..8e157101d 100644 --- a/state/remote/state.go +++ b/state/remote/state.go @@ -2,6 +2,7 @@ package remote import ( "bytes" + "fmt" "sync" "github.com/hashicorp/terraform/state" @@ -33,7 +34,28 @@ func (s *State) WriteState(state *terraform.State) error { s.mu.Lock() defer s.mu.Unlock() - s.state = state + if s.readState != nil && !state.SameLineage(s.readState) { + return fmt.Errorf("incompatible state lineage; given %s but want %s", state.Lineage, s.readState.Lineage) + } + + // We create a deep copy of the state here, because the caller also has + // a reference to the given object and can potentially go on to mutate + // it after we return, but we want the snapshot at this point in time. + s.state = state.DeepCopy() + + // Force our new state to have the same serial as our read state. We'll + // update this if PersistState is called later. (We don't require nor trust + // the caller to properly maintain serial for transient state objects since + // the rest of Terraform treats state as an openly mutable object.) + // + // If we have no read state then we assume we're either writing a new + // state for the first time or we're migrating a state from elsewhere, + // and in both cases we wish to retain the lineage and serial from + // the given state. + if s.readState != nil { + s.state.Serial = s.readState.Serial + } + return nil } @@ -58,7 +80,7 @@ func (s *State) RefreshState() error { } s.state = state - s.readState = state + s.readState = s.state.DeepCopy() // our states must be separate instances so we can track changes return nil } @@ -67,14 +89,28 @@ func (s *State) PersistState() error { s.mu.Lock() defer s.mu.Unlock() - s.state.IncrementSerialMaybe(s.readState) + if !s.state.MarshalEqual(s.readState) { + // Our new state does not marshal as byte-for-byte identical to + // the old, so we need to increment the serial. + // Note that in WriteState we force the serial to match that of + // s.readState, if we have a readState. + s.state.Serial++ + } var buf bytes.Buffer if err := terraform.WriteState(s.state, &buf); err != nil { return err } - return s.Client.Put(buf.Bytes()) + err := s.Client.Put(buf.Bytes()) + if err != nil { + return err + } + + // After we've successfully persisted, what we just wrote is our new + // reference state until someone calls RefreshState again. + s.readState = s.state.DeepCopy() + return nil } // Lock calls the Client's Lock method if it's implemented. diff --git a/state/state.go b/state/state.go index 45163852b..293fc0daa 100644 --- a/state/state.go +++ b/state/state.go @@ -36,6 +36,11 @@ type State interface { // the state here must not error. Loading the state fresh (an operation that // can likely error) should be implemented by RefreshState. If a state hasn't // been loaded yet, it is okay for State to return nil. +// +// Each caller of this function must get a distinct copy of the state, and +// it must also be distinct from any instance cached inside the reader, to +// ensure that mutations of the returned state will not affect the values +// returned to other callers. type StateReader interface { State() *terraform.State } @@ -43,6 +48,15 @@ type StateReader interface { // StateWriter is the interface that must be implemented by something that // can write a state. Writing the state can be cached or in-memory, as // full persistence should be implemented by StatePersister. +// +// Implementors that cache the state in memory _must_ take a copy of it +// before returning, since the caller may continue to modify it once +// control returns. The caller must ensure that the state instance is not +// concurrently modified _during_ the call, or behavior is undefined. +// +// If an object implements StatePersister in conjunction with StateReader +// then these methods must coordinate such that a subsequent read returns +// a copy of the most recent write, even if it has not yet been persisted. type StateWriter interface { WriteState(*terraform.State) error } @@ -57,6 +71,10 @@ type StateRefresher interface { // StatePersister is implemented to truly persist a state. Whereas StateWriter // is allowed to perhaps be caching in memory, PersistState must write the // state to some durable storage. +// +// If an object implements StatePersister in conjunction with StateReader +// and/or StateRefresher then these methods must coordinate such that +// subsequent reads after a persist return an updated value. type StatePersister interface { PersistState() error } diff --git a/state/testing.go b/state/testing.go index 61f972ca1..be948d409 100644 --- a/state/testing.go +++ b/state/testing.go @@ -31,6 +31,12 @@ func TestState(t *testing.T, s interface{}) { t.Fatalf("not initial:\n%#v\n\n%#v", state, current) } + // Now we've proven that the state we're starting with is an initial + // state, we'll complete our work here with that state, since otherwise + // further writes would violate the invariant that we only try to write + // states that share the same lineage as what was initially written. + current = reader.State() + // Write a new state and verify that we have it if ws, ok := s.(StateWriter); ok { current.AddModuleState(&terraform.ModuleState{ @@ -74,12 +80,12 @@ func TestState(t *testing.T, s interface{}) { } // If we can write and persist then verify that the serial - // is only implemented on change. + // is only incremented on change. writer, writeOk := s.(StateWriter) persister, persistOk := s.(StatePersister) if writeOk && persistOk { // Same serial - serial := current.Serial + serial := reader.State().Serial if err := writer.WriteState(current); err != nil { t.Fatalf("err: %s", err) } @@ -88,7 +94,7 @@ func TestState(t *testing.T, s interface{}) { } if reader.State().Serial != serial { - t.Fatalf("bad: expected %d, got %d", serial, reader.State().Serial) + t.Fatalf("serial changed after persisting with no changes: got %d, want %d", reader.State().Serial, serial) } // Change the serial @@ -113,7 +119,7 @@ func TestState(t *testing.T, s interface{}) { } if reader.State().Serial <= serial { - t.Fatalf("bad: expected %d, got %d", serial, reader.State().Serial) + t.Fatalf("serial incorrect after persisting with changes: got %d, want > %d", reader.State().Serial, serial) } // Check that State() returns a copy by modifying the copy and comparing diff --git a/terraform/state.go b/terraform/state.go index 074b68245..a8fc5277b 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -533,6 +533,43 @@ func (s *State) equal(other *State) bool { return true } +// MarshalEqual is similar to Equal but provides a stronger definition of +// "equal", where two states are equal if and only if their serialized form +// is byte-for-byte identical. +// +// This is primarily useful for callers that are trying to save snapshots +// of state to persistent storage, allowing them to detect when a new +// snapshot must be taken. +// +// Note that the serial number and lineage are included in the serialized form, +// so it's the caller's responsibility to properly manage these attributes +// so that this method is only called on two states that have the same +// serial and lineage, unless detecting such differences is desired. +func (s *State) MarshalEqual(other *State) bool { + if s == nil && other == nil { + return true + } else if s == nil || other == nil { + return false + } + + recvBuf := &bytes.Buffer{} + otherBuf := &bytes.Buffer{} + + err := WriteState(s, recvBuf) + if err != nil { + // should never happen, since we're writing to a buffer + panic(err) + } + + err = WriteState(other, otherBuf) + if err != nil { + // should never happen, since we're writing to a buffer + panic(err) + } + + return bytes.Equal(recvBuf.Bytes(), otherBuf.Bytes()) +} + type StateAgeComparison int const ( @@ -603,6 +640,10 @@ func (s *State) SameLineage(other *State) bool { // DeepCopy performs a deep copy of the state structure and returns // a new structure. func (s *State) DeepCopy() *State { + if s == nil { + return nil + } + copy, err := copystructure.Config{Lock: true}.Copy(s) if err != nil { panic(err) @@ -611,30 +652,6 @@ func (s *State) DeepCopy() *State { return copy.(*State) } -// IncrementSerialMaybe increments the serial number of this state -// if it different from the other state. -func (s *State) IncrementSerialMaybe(other *State) { - if s == nil { - return - } - if other == nil { - return - } - s.Lock() - defer s.Unlock() - - if s.Serial > other.Serial { - return - } - if other.TFVersion != s.TFVersion || !s.equal(other) { - if other.Serial > s.Serial { - s.Serial = other.Serial - } - - s.Serial++ - } -} - // FromFutureTerraform checks if this state was written by a Terraform // version from the future. func (s *State) FromFutureTerraform() bool { diff --git a/terraform/state_test.go b/terraform/state_test.go index 5578f89c9..e56e04d44 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -631,87 +631,121 @@ func TestStateSameLineage(t *testing.T) { } } -func TestStateIncrementSerialMaybe(t *testing.T) { - cases := map[string]struct { +func TestStateMarshalEqual(t *testing.T) { + tests := map[string]struct { S1, S2 *State - Serial int64 + Want bool }{ - "S2 is nil": { + "both nil": { + nil, + nil, + true, + }, + "first zero, second nil": { &State{}, nil, - 0, + false, }, - "S2 is identical": { + "first nil, second zero": { + nil, &State{}, - &State{}, - 0, + false, }, - "S2 is different": { + "both zero": { + // These are not equal because they both implicitly init with + // different lineage. &State{}, + &State{}, + false, + }, + "both set, same lineage": { &State{ - Modules: []*ModuleState{ - &ModuleState{Path: rootModulePath}, - }, + Lineage: "abc123", }, - 1, - }, - "S2 is different, but only via Instance Metadata": { &State{ - Serial: 3, + Lineage: "abc123", + }, + true, + }, + "both set, same lineage, different serial": { + &State{ + Lineage: "abc123", + Serial: 1, + }, + &State{ + Lineage: "abc123", + Serial: 2, + }, + false, + }, + "both set, same lineage, same serial, same resources": { + &State{ + Lineage: "abc123", + Serial: 1, Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, + { + Path: []string{"root"}, Resources: map[string]*ResourceState{ - "test_instance.foo": &ResourceState{ - Primary: &InstanceState{ - Meta: map[string]interface{}{}, - }, - }, + "foo_bar.baz": {}, }, }, }, }, &State{ - Serial: 3, + Lineage: "abc123", + Serial: 1, Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, + { + Path: []string{"root"}, Resources: map[string]*ResourceState{ - "test_instance.foo": &ResourceState{ - Primary: &InstanceState{ - Meta: map[string]interface{}{ - "schema_version": "1", - }, - }, - }, + "foo_bar.baz": {}, }, }, }, }, - 4, + true, }, - "S1 serial is higher": { - &State{Serial: 5}, + "both set, same lineage, same serial, different resources": { &State{ - Serial: 3, + Lineage: "abc123", + Serial: 1, Modules: []*ModuleState{ - &ModuleState{Path: rootModulePath}, + { + Path: []string{"root"}, + Resources: map[string]*ResourceState{ + "foo_bar.baz": {}, + }, + }, }, }, - 5, - }, - "S2 has a different TFVersion": { - &State{TFVersion: "0.1"}, - &State{TFVersion: "0.2"}, - 1, + &State{ + Lineage: "abc123", + Serial: 1, + Modules: []*ModuleState{ + { + Path: []string{"root"}, + Resources: map[string]*ResourceState{ + "pizza_crust.tasty": {}, + }, + }, + }, + }, + false, }, } - for name, tc := range cases { - tc.S1.IncrementSerialMaybe(tc.S2) - if tc.S1.Serial != tc.Serial { - t.Fatalf("Bad: %s\nGot: %d", name, tc.S1.Serial) - } + for name, test := range tests { + t.Run(name, func(t *testing.T) { + got := test.S1.MarshalEqual(test.S2) + if got != test.Want { + t.Errorf("wrong result %#v; want %#v", got, test.Want) + s1Buf := &bytes.Buffer{} + s2Buf := &bytes.Buffer{} + _ = WriteState(test.S1, s1Buf) + _ = WriteState(test.S2, s2Buf) + t.Logf("\nState 1: %s\nState 2: %s", s1Buf.Bytes(), s2Buf.Bytes()) + } + }) } } From fba5decae591de1854c002d8b056d9bcd48a7986 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 5 Jul 2017 17:10:19 -0400 Subject: [PATCH 2/5] update TestState helper In practice, States must all implement the full interface, so checking for each method set only leaves gaps where tests could be skipped. Change the helper to only accept a full state.State implementation. Add some Lineage, Version, and TFVersion checks to TestState to avoid regressions. Compare the copy test against the immediate State returnedm rather than our previous "current" state. Check that the states round-trip and still marhsal identically via MarshalEqual. --- state/testing.go | 191 ++++++++++++++++++++++++----------------------- 1 file changed, 96 insertions(+), 95 deletions(-) diff --git a/state/testing.go b/state/testing.go index be948d409..482615991 100644 --- a/state/testing.go +++ b/state/testing.go @@ -10,125 +10,126 @@ import ( // TestState is a helper for testing state implementations. It is expected // that the given implementation is pre-loaded with the TestStateInitial // state. -func TestState(t *testing.T, s interface{}) { - reader, ok := s.(StateReader) - if !ok { - t.Fatalf("must at least be a StateReader") +func TestState(t *testing.T, s State) { + if err := s.RefreshState(); err != nil { + t.Fatalf("err: %s", err) } - // If it implements refresh, refresh - if rs, ok := s.(StateRefresher); ok { - if err := rs.RefreshState(); err != nil { - t.Fatalf("err: %s", err) - } - } - - // current will track our current state - current := TestStateInitial() - - // Check that the initial state is correct - if state := reader.State(); !current.Equal(state) { - t.Fatalf("not initial:\n%#v\n\n%#v", state, current) + // Check that the initial state is correct. + // These do have different Lineages, but we will replace current below. + initial := TestStateInitial() + if state := s.State(); !state.Equal(initial) { + t.Fatalf("state does not match expected initial state:\n%#v\n\n%#v", state, initial) } // Now we've proven that the state we're starting with is an initial // state, we'll complete our work here with that state, since otherwise // further writes would violate the invariant that we only try to write // states that share the same lineage as what was initially written. - current = reader.State() + current := s.State() // Write a new state and verify that we have it - if ws, ok := s.(StateWriter); ok { - current.AddModuleState(&terraform.ModuleState{ - Path: []string{"root"}, - Outputs: map[string]*terraform.OutputState{ - "bar": &terraform.OutputState{ - Type: "string", - Sensitive: false, - Value: "baz", - }, + current.AddModuleState(&terraform.ModuleState{ + Path: []string{"root"}, + Outputs: map[string]*terraform.OutputState{ + "bar": &terraform.OutputState{ + Type: "string", + Sensitive: false, + Value: "baz", }, - }) + }, + }) - if err := ws.WriteState(current); err != nil { - t.Fatalf("err: %s", err) - } + if err := s.WriteState(current); err != nil { + t.Fatalf("err: %s", err) + } - if actual := reader.State(); !actual.Equal(current) { - t.Fatalf("bad:\n%#v\n\n%#v", actual, current) - } + if actual := s.State(); !actual.Equal(current) { + t.Fatalf("bad:\n%#v\n\n%#v", actual, current) } // Test persistence - if ps, ok := s.(StatePersister); ok { - if err := ps.PersistState(); err != nil { - t.Fatalf("err: %s", err) - } - - // Refresh if we got it - if rs, ok := s.(StateRefresher); ok { - if err := rs.RefreshState(); err != nil { - t.Fatalf("err: %s", err) - } - } - - // Just set the serials the same... Then compare. - actual := reader.State() - if !actual.Equal(current) { - t.Fatalf("bad: %#v\n\n%#v", actual, current) - } + if err := s.PersistState(); err != nil { + t.Fatalf("err: %s", err) } - // If we can write and persist then verify that the serial - // is only incremented on change. - writer, writeOk := s.(StateWriter) - persister, persistOk := s.(StatePersister) - if writeOk && persistOk { - // Same serial - serial := reader.State().Serial - if err := writer.WriteState(current); err != nil { - t.Fatalf("err: %s", err) - } - if err := persister.PersistState(); err != nil { - t.Fatalf("err: %s", err) - } + // Refresh if we got it + if err := s.RefreshState(); err != nil { + t.Fatalf("err: %s", err) + } - if reader.State().Serial != serial { - t.Fatalf("serial changed after persisting with no changes: got %d, want %d", reader.State().Serial, serial) - } + if s.State().Lineage != current.Lineage { + t.Fatalf("Lineage changed from %s to %s", s.State().Lineage, current.Lineage) + } - // Change the serial - current = current.DeepCopy() - current.Modules = []*terraform.ModuleState{ - &terraform.ModuleState{ - Path: []string{"root", "somewhere"}, - Outputs: map[string]*terraform.OutputState{ - "serialCheck": &terraform.OutputState{ - Type: "string", - Sensitive: false, - Value: "true", - }, + // Just set the serials the same... Then compare. + actual := s.State() + if !actual.Equal(current) { + t.Fatalf("bad: %#v\n\n%#v", actual, current) + } + + // Same serial + serial := s.State().Serial + if err := s.WriteState(current); err != nil { + t.Fatalf("err: %s", err) + } + if err := s.PersistState(); err != nil { + t.Fatalf("err: %s", err) + } + + if s.State().Serial != serial { + t.Fatalf("serial changed after persisting with no changes: got %d, want %d", s.State().Serial, serial) + } + + // Change the serial + current = current.DeepCopy() + current.Modules = []*terraform.ModuleState{ + &terraform.ModuleState{ + Path: []string{"root", "somewhere"}, + Outputs: map[string]*terraform.OutputState{ + "serialCheck": &terraform.OutputState{ + Type: "string", + Sensitive: false, + Value: "true", }, }, - } - if err := writer.WriteState(current); err != nil { - t.Fatalf("err: %s", err) - } - if err := persister.PersistState(); err != nil { - t.Fatalf("err: %s", err) - } + }, + } + if err := s.WriteState(current); err != nil { + t.Fatalf("err: %s", err) + } + if err := s.PersistState(); err != nil { + t.Fatalf("err: %s", err) + } - if reader.State().Serial <= serial { - t.Fatalf("serial incorrect after persisting with changes: got %d, want > %d", reader.State().Serial, serial) - } + if s.State().Serial <= serial { + t.Fatalf("serial incorrect after persisting with changes: got %d, want > %d", s.State().Serial, serial) + } - // Check that State() returns a copy by modifying the copy and comparing - // to the current state. - stateCopy := reader.State() - stateCopy.Serial++ - if reflect.DeepEqual(stateCopy, current) { - t.Fatal("State() should return a copy") - } + if s.State().Version != current.Version { + t.Fatalf("Version changed from %d to %d", s.State().Version, current.Version) + } + + if s.State().TFVersion != current.TFVersion { + t.Fatalf("TFVersion changed from %s to %s", s.State().TFVersion, current.TFVersion) + } + + // verify that Lineage doesn't change along with Serial, or during copying. + if s.State().Lineage != current.Lineage { + t.Fatalf("Lineage changed from %s to %s", s.State().Lineage, current.Lineage) + } + + // Check that State() returns a copy by modifying the copy and comparing + // to the current state. + stateCopy := s.State() + stateCopy.Serial++ + if reflect.DeepEqual(stateCopy, s.State()) { + t.Fatal("State() should return a copy") + } + + // our current expected state should also marhsal identically to the persisted state + if current.MarshalEqual(s.State()) { + t.Fatalf("Persisted state altered unexpectedly. Expected: %#v\b Got: %#v", current, s.State()) } } From 501cbeaffe905eb5a5ec83864b89b21dff25f9f6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 5 Jul 2017 17:47:05 -0400 Subject: [PATCH 3/5] testState shouldn't rely on mods from WriteState The state returned from the testState helper shouldn't rely on any mutations caused by WriteState. The Init function (which is analogous to NewState) shoudl set any required fields. --- command/command_test.go | 16 +--------------- terraform/state.go | 1 + 2 files changed, 2 insertions(+), 15 deletions(-) diff --git a/command/command_test.go b/command/command_test.go index a416256f8..1507b5b96 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -160,7 +160,6 @@ func testReadPlan(t *testing.T, path string) *terraform.Plan { // testState returns a test State structure that we use for a lot of tests. func testState() *terraform.State { state := &terraform.State{ - Version: 2, Modules: []*terraform.ModuleState{ &terraform.ModuleState{ Path: []string{"root"}, @@ -177,20 +176,7 @@ func testState() *terraform.State { }, } state.Init() - - // Write and read the state so that it is properly initialized. We - // do this since we didn't call the normal NewState constructor. - var buf bytes.Buffer - if err := terraform.WriteState(state, &buf); err != nil { - panic(err) - } - - result, err := terraform.ReadState(&buf) - if err != nil { - panic(err) - } - - return result + return state } func testStateFile(t *testing.T, s *terraform.State) string { diff --git a/terraform/state.go b/terraform/state.go index a8fc5277b..0c46194d6 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -677,6 +677,7 @@ func (s *State) init() { if s.Version == 0 { s.Version = StateVersion } + if s.moduleByPath(rootModulePath) == nil { s.addModule(rootModulePath) } From 054716c397340f56aa365a1231fca4b53017d725 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 5 Jul 2017 17:59:42 -0400 Subject: [PATCH 4/5] use testStateRead helper in apply tests We have a helper function that we hardly ever use. TODO: convert the rest of the manual ReadState calls eventually. --- command/apply_test.go | 178 ++++------------------------------------ command/command_test.go | 2 +- 2 files changed, 18 insertions(+), 162 deletions(-) diff --git a/command/apply_test.go b/command/apply_test.go index 2065fe134..ad6e5a9a0 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -45,16 +45,7 @@ func TestApply(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -292,16 +283,7 @@ func TestApply_defaultState(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -360,16 +342,7 @@ func TestApply_error(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -484,13 +457,7 @@ func TestApply_noArgs(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) + state := testStateRead(t, statePath) if err != nil { t.Fatalf("err: %s", err) } @@ -538,16 +505,7 @@ func TestApply_plan(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -584,16 +542,7 @@ func TestApply_plan_backup(t *testing.T) { { // Should have a backup file - f, err := os.Open(backupPath) - if err != nil { - t.Fatalf("err: %s", err) - } - - _, err = terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + testStateRead(t, backupPath) } } @@ -732,16 +681,7 @@ func TestApply_planWithVarFile(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -847,31 +787,13 @@ func TestApply_refresh(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } // Should have a backup file - f, err = os.Open(statePath + DefaultBackupExtension) - if err != nil { - t.Fatalf("err: %s", err) - } - - backupState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + backupState := testStateRead(t, statePath+DefaultBackupExtension) actualStr := strings.TrimSpace(backupState.String()) expectedStr := strings.TrimSpace(originalState.String()) @@ -953,16 +875,7 @@ func TestApply_shutdown(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } @@ -1035,31 +948,12 @@ func TestApply_state(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } - // Should have a backup file - f, err = os.Open(statePath + DefaultBackupExtension) - if err != nil { - t.Fatalf("err: %s", err) - } - - backupState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + backupState := testStateRead(t, statePath+DefaultBackupExtension) // nil out the ConnInfo since that should not be restored originalState.RootModule().Resources["test_instance.foo"].Primary.Ephemeral.ConnInfo = nil @@ -1142,17 +1036,7 @@ func TestApply_stateFuture(t *testing.T) { t.Fatal("should fail") } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - - newState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } - + newState := testStateRead(t, statePath) if !newState.Equal(originalState) { t.Fatalf("bad: %#v", newState) } @@ -1422,31 +1306,12 @@ func TestApply_backup(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } - // Should have a backup file - f, err = os.Open(backupPath) - if err != nil { - t.Fatalf("err: %s", err) - } - - backupState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + backupState := testStateRead(t, backupPath) actual := backupState.RootModule().Resources["test_instance.foo"] expected := originalState.RootModule().Resources["test_instance.foo"] @@ -1504,22 +1369,13 @@ func TestApply_disableBackup(t *testing.T) { t.Fatalf("err: %s", err) } - f, err := os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - defer f.Close() - - state, err := terraform.ReadState(f) - if err != nil { - t.Fatalf("err: %s", err) - } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") } // Ensure there is no backup - _, err = os.Stat(statePath + DefaultBackupExtension) + _, err := os.Stat(statePath + DefaultBackupExtension) if err == nil || !os.IsNotExist(err) { t.Fatalf("backup should not exist") } diff --git a/command/command_test.go b/command/command_test.go index 1507b5b96..55bea29df 100644 --- a/command/command_test.go +++ b/command/command_test.go @@ -238,9 +238,9 @@ func testStateRead(t *testing.T, path string) *terraform.State { if err != nil { t.Fatalf("err: %s", err) } + defer f.Close() newState, err := terraform.ReadState(f) - f.Close() if err != nil { t.Fatalf("err: %s", err) } From fb397060eb2b90198a2d808d068a7c69b160f6a4 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 5 Jul 2017 18:17:07 -0400 Subject: [PATCH 5/5] add some Serial checks to apply and refresh tests --- command/apply_test.go | 18 ++++++++++++++---- command/refresh_test.go | 35 +++++++++-------------------------- 2 files changed, 23 insertions(+), 30 deletions(-) diff --git a/command/apply_test.go b/command/apply_test.go index ad6e5a9a0..bab8e0955 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -272,6 +272,14 @@ func TestApply_defaultState(t *testing.T) { }, } + // create an existing state file + localState := &state.LocalState{Path: statePath} + if err := localState.WriteState(terraform.NewState()); err != nil { + t.Fatal(err) + } + + serial := localState.State().Serial + args := []string{ testFixturePath("apply"), } @@ -287,6 +295,10 @@ func TestApply_defaultState(t *testing.T) { if state == nil { t.Fatal("state should not be nil") } + + if state.Serial <= serial { + t.Fatalf("serial was not incremented. previous:%d, current%d", serial, state.Serial) + } } func TestApply_error(t *testing.T) { @@ -540,10 +552,8 @@ func TestApply_plan_backup(t *testing.T) { t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) } - { - // Should have a backup file - testStateRead(t, backupPath) - } + // Should have a backup file + testStateRead(t, backupPath) } func TestApply_plan_noBackup(t *testing.T) { diff --git a/command/refresh_test.go b/command/refresh_test.go index c53c84bd3..5a0cfd357 100644 --- a/command/refresh_test.go +++ b/command/refresh_test.go @@ -10,6 +10,7 @@ import ( "testing" "github.com/hashicorp/terraform/helper/copy" + "github.com/hashicorp/terraform/state" "github.com/hashicorp/terraform/terraform" "github.com/mitchellh/cli" ) @@ -193,15 +194,11 @@ func TestRefresh_defaultState(t *testing.T) { } statePath := filepath.Join(td, DefaultStateFilename) - f, err := os.Create(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - err = terraform.WriteState(originalState, f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) + localState := &state.LocalState{Path: statePath} + if err := localState.WriteState(originalState); err != nil { + t.Fatal(err) } + serial := localState.State().Serial // Change to that directory cwd, err := os.Getwd() @@ -236,16 +233,7 @@ func TestRefresh_defaultState(t *testing.T) { t.Fatal("refresh should be called") } - f, err = os.Open(statePath) - if err != nil { - t.Fatalf("err: %s", err) - } - - newState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + newState := testStateRead(t, statePath) actual := newState.RootModule().Resources["test_instance.foo"].Primary expected := p.RefreshReturn @@ -254,16 +242,11 @@ func TestRefresh_defaultState(t *testing.T) { t.Fatalf("bad:\n%#v", actual) } - f, err = os.Open(statePath + DefaultBackupExtension) - if err != nil { - t.Fatalf("err: %s", err) + if newState.Serial <= serial { + t.Fatalf("serial not incremented during refresh. previous:%d, current:%d", serial, newState.Serial) } - backupState, err := terraform.ReadState(f) - f.Close() - if err != nil { - t.Fatalf("err: %s", err) - } + backupState := testStateRead(t, statePath+DefaultBackupExtension) actual = backupState.RootModule().Resources["test_instance.foo"].Primary expected = originalState.RootModule().Resources["test_instance.foo"].Primary