Merge pull request #1036 from hashicorp/b-serial-on-change

Only increment serial on state change

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.
@@ -104,6 +104,8 @@ func (s *CacheState) RefreshState() error {
 			s.refreshResult = CacheRefreshNoop
 			return err
 		}
+
+		cached = durable
 	}
 
 	s.state = cached

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 {
@@ -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
 }

state/local.go

@@ -15,18 +15,20 @@ type LocalState struct {
 	Path    string
 	PathOut string
 
-	state   *terraform.State
+	state     *terraform.State
-	written bool
+	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.
 func (s *LocalState) State() *terraform.State {
-	return s.state
+	return s.state.DeepCopy()
 }
 
 // WriteState for LocalState always persists the state as well.
@@ -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
 }

state/remote/state.go

@@ -13,12 +13,12 @@ import (
 type State struct {
 	Client Client
 
-	state *terraform.State
+	state, readState *terraform.State
 }
 
 // StateReader impl.
 func (s *State) State() *terraform.State {
-	return s.state
+	return s.state.DeepCopy()
 }
 
 // StateWriter 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

state/remote/state_test.go

@@ -7,8 +7,11 @@ import (
 )
 
 func TestState(t *testing.T) {
-	s := &State{Client: new(InmemClient)}
+	s := &State{
-	s.WriteState(state.TestStateInitial())
+		Client:    new(InmemClient),
+		state:     state.TestStateInitial(),
+		readState: state.TestStateInitial(),
+	}
 	if err := s.PersistState(); err != nil {
 		t.Fatalf("err: %s", err)
 	}

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()
+	if state := reader.State(); !current.Equal(state) {
-	current.Serial = state.Serial
-	if !reflect.DeepEqual(state, current) {
 		t.Fatalf("not initial: %#v\n\n%#v", state, current)
 	}
 
@@ -47,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)
 		}
 	}
@@ -67,11 +65,55 @@ func TestState(t *testing.T, s interface{}) {
 
 		// Just set the serials the same... Then compare.
 		actual := reader.State()
-		actual.Serial = current.Serial
+		if !actual.Equal(current) {
-		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 = &currentCopy
+		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)
+		}
+
+		// Check that State() returns a copy
+		reader.State().Serial++
+		if reflect.DeepEqual(reader.State(), current) {
+			t.Fatal("State() should return a copy")
+		}
+	}
 }
 
 // TestStateInitial is the initial state that a State should have

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 {

terraform/state.go

@@ -161,12 +161,20 @@ 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
 	}
 
 	// 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)
@@ -183,20 +191,9 @@ func (s *State) Equal(other *State) bool {
 	return true
 }
 
-func (s *State) init() {
+// DeepCopy performs a deep copy of the state structure and returns
-	if s.Version == 0 {
+// a new structure.
-		s.Version = StateVersion
+func (s *State) DeepCopy() *State {
-	}
-	if len(s.Modules) == 0 {
-		root := &ModuleState{
-			Path: rootModulePath,
-		}
-		root.init()
-		s.Modules = []*ModuleState{root}
-	}
-}
-
-func (s *State) deepcopy() *State {
 	if s == nil {
 		return nil
 	}
@@ -214,6 +211,27 @@ func (s *State) deepcopy() *State {
 	return n
 }
 
+// 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
+	}
+	if len(s.Modules) == 0 {
+		root := &ModuleState{
+			Path: rootModulePath,
+		}
+		root.init()
+		s.Modules = []*ModuleState{root}
+	}
+}
+
 // prune is used to remove any resources that are no longer required
 func (s *State) prune() {
 	if s == nil {
@@ -951,9 +969,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 {

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())
+		}
 	}
 }
 
@@ -537,15 +553,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 +564,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{}