From c2bf60060359b0570a16f30686a4bea63083bf11 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 23 Feb 2015 21:26:33 -0800 Subject: [PATCH 1/5] state: only change serial if changed --- state/cache.go | 2 ++ state/inmem.go | 1 + state/local.go | 10 ++++++++-- state/testing.go | 44 +++++++++++++++++++++++++++++++++++++---- terraform/state.go | 16 ++++++++++++--- terraform/state_test.go | 13 ------------ 6 files changed, 64 insertions(+), 22 deletions(-) diff --git a/state/cache.go b/state/cache.go index 3322ab5ac..0f0f306a0 100644 --- a/state/cache.go +++ b/state/cache.go @@ -104,6 +104,8 @@ func (s *CacheState) RefreshState() error { s.refreshResult = CacheRefreshNoop return err } + + cached = durable } s.state = cached diff --git a/state/inmem.go b/state/inmem.go index 82385a6df..68c2ad0c3 100644 --- a/state/inmem.go +++ b/state/inmem.go @@ -18,6 +18,7 @@ func (s *InmemState) RefreshState() error { } func (s *InmemState) WriteState(state *terraform.State) error { + state.IncrementSerialMaybe(s.state) s.state = state return nil } diff --git a/state/local.go b/state/local.go index 1840cae1a..30c3093aa 100644 --- a/state/local.go +++ b/state/local.go @@ -15,13 +15,15 @@ type LocalState struct { Path string PathOut string - state *terraform.State - written bool + state *terraform.State + readState *terraform.State + written bool } // SetState will force a specific state in-memory for this local state. func (s *LocalState) SetState(state *terraform.State) { s.state = state + s.readState = state } // StateReader impl. @@ -61,6 +63,9 @@ func (s *LocalState) WriteState(state *terraform.State) error { } defer f.Close() + s.state.IncrementSerialMaybe(s.readState) + s.readState = s.state + if err := terraform.WriteState(s.state, f); err != nil { return err } @@ -105,5 +110,6 @@ func (s *LocalState) RefreshState() error { } s.state = state + s.readState = state return nil } diff --git a/state/testing.go b/state/testing.go index 3ada81e40..7efd782d8 100644 --- a/state/testing.go +++ b/state/testing.go @@ -28,9 +28,7 @@ func TestState(t *testing.T, s interface{}) { current := TestStateInitial() // Check that the initial state is correct - state := reader.State() - current.Serial = state.Serial - if !reflect.DeepEqual(state, current) { + if state := reader.State(); !reflect.DeepEqual(state, current) { t.Fatalf("not initial: %#v\n\n%#v", state, current) } @@ -67,11 +65,49 @@ func TestState(t *testing.T, s interface{}) { // Just set the serials the same... Then compare. actual := reader.State() - actual.Serial = current.Serial if !reflect.DeepEqual(actual, current) { t.Fatalf("bad: %#v\n\n%#v", actual, current) } } + + // If we can write and persist then verify that the serial + // is only implemented on change. + writer, writeOk := s.(StateWriter) + persister, persistOk := s.(StatePersister) + if writeOk && persistOk { + // Same serial + serial := current.Serial + if err := writer.WriteState(current); err != nil { + t.Fatalf("err: %s", err) + } + if err := persister.PersistState(); err != nil { + t.Fatalf("err: %s", err) + } + + if reader.State().Serial != serial { + t.Fatalf("bad: expected %d, got %d", serial, reader.State().Serial) + } + + // Change the serial + currentCopy := *current + current = ¤tCopy + current.Modules = []*terraform.ModuleState{ + &terraform.ModuleState{ + Path: []string{"root", "somewhere"}, + Outputs: map[string]string{"serialCheck": "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 reader.State().Serial <= serial { + t.Fatalf("bad: expected %d, got %d", serial, reader.State().Serial) + } + } } // TestStateInitial is the initial state that a State should have diff --git a/terraform/state.go b/terraform/state.go index 3ef3f3afe..68b5c11c6 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -161,6 +161,11 @@ func (s *State) RootModule() *ModuleState { // Equal tests if one state is equal to another. func (s *State) Equal(other *State) bool { + // If one is nil, we do a direct check + if s == nil || other == nil { + return s == other + } + // If the versions are different, they're certainly not equal if s.Version != other.Version { return false @@ -183,6 +188,14 @@ func (s *State) Equal(other *State) bool { return true } +// IncrementSerialMaybe increments the serial number of this state +// if it different from the other state. +func (s *State) IncrementSerialMaybe(other *State) { + if !s.Equal(other) { + s.Serial++ + } +} + func (s *State) init() { if s.Version == 0 { s.Version = StateVersion @@ -951,9 +964,6 @@ func WriteState(d *State, dst io.Writer) error { // Ensure the version is set d.Version = StateVersion - // Always increment the serial number - d.Serial++ - // Encode the data in a human-friendly way data, err := json.MarshalIndent(d, "", " ") if err != nil { diff --git a/terraform/state_test.go b/terraform/state_test.go index fc969e973..aaa049f4b 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -537,15 +537,6 @@ func TestReadWriteState(t *testing.T) { t.Fatalf("bad version number: %d", state.Version) } - // Verify the serial number is incremented - if state.Serial != 10 { - t.Fatalf("bad serial: %d", state.Serial) - } - - // Remove the changes or the checksum will fail - state.Version = 0 - state.Serial = 9 - // Checksum after the write chksumAfter := checksumStruct(t, state) if chksumAfter != chksum { @@ -557,10 +548,6 @@ func TestReadWriteState(t *testing.T) { t.Fatalf("err: %s", err) } - // Verify the changes came through - state.Version = StateVersion - state.Serial = 10 - // ReadState should not restore sensitive information! mod := state.RootModule() mod.Resources["foo"].Primary.Ephemeral = EphemeralState{} From ed6128aa6ecdc6122641db745c1117bb6c9abfc8 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 23 Feb 2015 21:30:59 -0800 Subject: [PATCH 2/5] state/remote: increment serial properly --- state/remote/state.go | 5 ++++- state/remote/state_test.go | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/state/remote/state.go b/state/remote/state.go index 5137744ec..c0abca40e 100644 --- a/state/remote/state.go +++ b/state/remote/state.go @@ -13,7 +13,7 @@ import ( type State struct { Client Client - state *terraform.State + state, readState *terraform.State } // StateReader impl. @@ -43,11 +43,14 @@ func (s *State) RefreshState() error { } s.state = state + s.readState = state return nil } // StatePersister impl. func (s *State) PersistState() error { + s.state.IncrementSerialMaybe(s.readState) + var buf bytes.Buffer if err := terraform.WriteState(s.state, &buf); err != nil { return err diff --git a/state/remote/state_test.go b/state/remote/state_test.go index 08b51439b..487891667 100644 --- a/state/remote/state_test.go +++ b/state/remote/state_test.go @@ -7,8 +7,11 @@ import ( ) func TestState(t *testing.T) { - s := &State{Client: new(InmemClient)} - s.WriteState(state.TestStateInitial()) + s := &State{ + Client: new(InmemClient), + state: state.TestStateInitial(), + readState: state.TestStateInitial(), + } if err := s.PersistState(); err != nil { t.Fatalf("err: %s", err) } From f3af221866785450257edc7c117f3069adf4eb20 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 23 Feb 2015 21:32:27 -0800 Subject: [PATCH 3/5] terraform: make DeepCopy public --- terraform/context.go | 6 +++--- terraform/state.go | 38 ++++++++++++++++++++------------------ 2 files changed, 23 insertions(+), 21 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 97abe573c..e2db6e6f9 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -224,7 +224,7 @@ func (c *Context) Apply() (*State, error) { defer c.releaseRun(v) // Copy our own state - c.state = c.state.deepcopy() + c.state = c.state.DeepCopy() // Do the walk _, err := c.walk(walkApply) @@ -264,7 +264,7 @@ func (c *Context) Plan(opts *PlanOpts) (*Plan, error) { c.state = &State{} c.state.init() } else { - c.state = old.deepcopy() + c.state = old.DeepCopy() } defer func() { c.state = old @@ -299,7 +299,7 @@ func (c *Context) Refresh() (*State, error) { defer c.releaseRun(v) // Copy our own state - c.state = c.state.deepcopy() + c.state = c.state.DeepCopy() // Do the walk if _, err := c.walk(walkRefresh); err != nil { diff --git a/terraform/state.go b/terraform/state.go index 68b5c11c6..f94d9bedc 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -188,6 +188,26 @@ func (s *State) Equal(other *State) bool { return true } +// DeepCopy performs a deep copy of the state structure and returns +// a new structure. +func (s *State) DeepCopy() *State { + if s == nil { + return nil + } + n := &State{ + Version: s.Version, + Serial: s.Serial, + Modules: make([]*ModuleState, 0, len(s.Modules)), + } + for _, mod := range s.Modules { + n.Modules = append(n.Modules, mod.deepcopy()) + } + if s.Remote != nil { + n.Remote = s.Remote.deepcopy() + } + return n +} + // IncrementSerialMaybe increments the serial number of this state // if it different from the other state. func (s *State) IncrementSerialMaybe(other *State) { @@ -209,24 +229,6 @@ func (s *State) init() { } } -func (s *State) deepcopy() *State { - if s == nil { - return nil - } - n := &State{ - Version: s.Version, - Serial: s.Serial, - Modules: make([]*ModuleState, 0, len(s.Modules)), - } - for _, mod := range s.Modules { - n.Modules = append(n.Modules, mod.deepcopy()) - } - if s.Remote != nil { - n.Remote = s.Remote.deepcopy() - } - return n -} - // prune is used to remove any resources that are no longer required func (s *State) prune() { if s == nil { From cc8e6b6331cc1626db029d3dfba15c8f102d28fe Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 23 Feb 2015 21:36:35 -0800 Subject: [PATCH 4/5] state: deep copies are required --- state/cache.go | 2 +- state/inmem.go | 2 +- state/local.go | 2 +- state/remote/state.go | 2 +- state/testing.go | 12 +++++++++--- 5 files changed, 13 insertions(+), 7 deletions(-) diff --git a/state/cache.go b/state/cache.go index 0f0f306a0..a20eb4a06 100644 --- a/state/cache.go +++ b/state/cache.go @@ -19,7 +19,7 @@ type CacheState struct { // StateReader impl. func (s *CacheState) State() *terraform.State { - return s.state + return s.state.DeepCopy() } // WriteState will write and persist the state to the cache. diff --git a/state/inmem.go b/state/inmem.go index 68c2ad0c3..ff8daab8f 100644 --- a/state/inmem.go +++ b/state/inmem.go @@ -10,7 +10,7 @@ type InmemState struct { } func (s *InmemState) State() *terraform.State { - return s.state + return s.state.DeepCopy() } func (s *InmemState) RefreshState() error { diff --git a/state/local.go b/state/local.go index 30c3093aa..02afb1ed7 100644 --- a/state/local.go +++ b/state/local.go @@ -28,7 +28,7 @@ func (s *LocalState) SetState(state *terraform.State) { // StateReader impl. func (s *LocalState) State() *terraform.State { - return s.state + return s.state.DeepCopy() } // WriteState for LocalState always persists the state as well. diff --git a/state/remote/state.go b/state/remote/state.go index c0abca40e..e679b5d73 100644 --- a/state/remote/state.go +++ b/state/remote/state.go @@ -18,7 +18,7 @@ type State struct { // StateReader impl. func (s *State) State() *terraform.State { - return s.state + return s.state.DeepCopy() } // StateWriter impl. diff --git a/state/testing.go b/state/testing.go index 7efd782d8..6a4a88ad0 100644 --- a/state/testing.go +++ b/state/testing.go @@ -28,7 +28,7 @@ func TestState(t *testing.T, s interface{}) { current := TestStateInitial() // Check that the initial state is correct - if state := reader.State(); !reflect.DeepEqual(state, current) { + if state := reader.State(); !current.Equal(state) { t.Fatalf("not initial: %#v\n\n%#v", state, current) } @@ -45,7 +45,7 @@ func TestState(t *testing.T, s interface{}) { t.Fatalf("err: %s", err) } - if actual := reader.State(); !reflect.DeepEqual(actual, current) { + if actual := reader.State(); !actual.Equal(current) { t.Fatalf("bad: %#v\n\n%#v", actual, current) } } @@ -65,7 +65,7 @@ func TestState(t *testing.T, s interface{}) { // Just set the serials the same... Then compare. actual := reader.State() - if !reflect.DeepEqual(actual, current) { + if !actual.Equal(current) { t.Fatalf("bad: %#v\n\n%#v", actual, current) } } @@ -107,6 +107,12 @@ func TestState(t *testing.T, s interface{}) { if reader.State().Serial <= serial { t.Fatalf("bad: expected %d, got %d", serial, reader.State().Serial) } + + // Check that State() returns a copy + reader.State().Serial++ + if reflect.DeepEqual(reader.State(), current) { + t.Fatal("State() should return a copy") + } } } From 57f7507ebd7161a12559673024cba345d5fc66cc Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Mon, 23 Feb 2015 21:43:54 -0800 Subject: [PATCH 5/5] terraform: more state tests, fix a bug --- terraform/state.go | 3 +++ terraform/state_test.go | 16 ++++++++++++++++ 2 files changed, 19 insertions(+) diff --git a/terraform/state.go b/terraform/state.go index f94d9bedc..5b695c33c 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -172,6 +172,9 @@ func (s *State) Equal(other *State) bool { } // If any of the modules are not equal, then this state isn't equal + if len(s.Modules) != len(other.Modules) { + return false + } for _, m := range s.Modules { // This isn't very optimal currently but works. otherM := other.ModuleByPath(m.Path) diff --git a/terraform/state_test.go b/terraform/state_test.go index aaa049f4b..ef2aa0b59 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -116,6 +116,19 @@ func TestStateEqual(t *testing.T) { Result bool One, Two *State }{ + // Nils + { + false, + nil, + &State{Version: 2}, + }, + + { + true, + nil, + nil, + }, + // Different versions { false, @@ -159,6 +172,9 @@ func TestStateEqual(t *testing.T) { if tc.One.Equal(tc.Two) != tc.Result { t.Fatalf("Bad: %d\n\n%s\n\n%s", i, tc.One.String(), tc.Two.String()) } + if tc.Two.Equal(tc.One) != tc.Result { + t.Fatalf("Bad: %d\n\n%s\n\n%s", i, tc.One.String(), tc.Two.String()) + } } }