From 0be90c8994aaf072ea4ef71307a3c447b5a2ebb3 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:11:37 -0400 Subject: [PATCH 1/7] Add locks to all exported State methods move implementation into private methods when needed --- terraform/state.go | 76 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 7a23d4c26e..7428c6d9d8 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -98,8 +98,14 @@ func NewState() *State { // the given path. If the path is "root", for example, then children // returned might be "root.child", but not "root.child.grandchild". func (s *State) Children(path []string) []*ModuleState { + s.Lock() + defer s.Unlock() // TODO: test + return s.children(path) +} + +func (s *State) children(path []string) []*ModuleState { result := make([]*ModuleState, 0) for _, m := range s.Modules { if len(m.Path) != len(path)+1 { @@ -120,8 +126,10 @@ func (s *State) Children(path []string) []*ModuleState { // This should be the preferred method to add module states since it // allows us to optimize lookups later as well as control sorting. func (s *State) AddModule(path []string) *ModuleState { + s.Lock() + defer s.Unlock() // check if the module exists first - m := s.ModuleByPath(path) + m := s.moduleByPath(path) if m != nil { return m } @@ -140,6 +148,13 @@ func (s *State) ModuleByPath(path []string) *ModuleState { if s == nil { return nil } + s.Lock() + defer s.Unlock() + + return s.moduleByPath(path) +} + +func (s *State) moduleByPath(path []string) *ModuleState { for _, mod := range s.Modules { if mod.Path == nil { panic("missing module path") @@ -155,6 +170,14 @@ func (s *State) ModuleByPath(path []string) *ModuleState { // returning their full paths. These paths can be used with ModuleByPath // to return the actual state. func (s *State) ModuleOrphans(path []string, c *config.Config) [][]string { + s.Lock() + defer s.Unlock() + + return s.moduleOrphans(path, c) + +} + +func (s *State) moduleOrphans(path []string, c *config.Config) [][]string { // direct keeps track of what direct children we have both in our config // and in our state. childrenKeys keeps track of what isn't an orphan. direct := make(map[string]struct{}) @@ -168,7 +191,7 @@ func (s *State) ModuleOrphans(path []string, c *config.Config) [][]string { // Go over the direct children and find any that aren't in our keys. var orphans [][]string - for _, m := range s.Children(path) { + for _, m := range s.children(path) { key := m.Path[len(m.Path)-1] // Record that we found this key as a direct child. We use this @@ -228,6 +251,8 @@ func (s *State) Empty() bool { if s == nil { return true } + s.Lock() + defer s.Unlock() return len(s.Modules) == 0 } @@ -238,6 +263,9 @@ func (s *State) IsRemote() bool { if s == nil { return false } + s.Lock() + defer s.Unlock() + if s.Remote == nil { return false } @@ -258,6 +286,9 @@ func (s *State) IsRemote() bool { // If this returns an error, then the user should be notified. The error // response will include detailed information on the nature of the error. func (s *State) Validate() error { + s.Lock() + defer s.Unlock() + var result error // !!!! FOR DEVELOPERS !!!! @@ -295,6 +326,9 @@ func (s *State) Validate() error { // all children as well. To check what will be deleted, use a StateFilter // first. func (s *State) Remove(addr ...string) error { + s.Lock() + defer s.Unlock() + // Filter out what we need to delete filter := &StateFilter{State: s} results, err := filter.Filter(addr...) @@ -362,7 +396,7 @@ func (s *State) removeModule(path []string, v *ModuleState) { func (s *State) removeResource(path []string, v *ResourceState) { // Get the module this resource lives in. If it doesn't exist, we're done. - mod := s.ModuleByPath(path) + mod := s.moduleByPath(path) if mod == nil { return } @@ -420,6 +454,16 @@ func (s *State) Equal(other *State) bool { return s == other } + s.Lock() + defer s.Unlock() + return s.equal(other) +} + +func (s *State) equal(other *State) bool { + 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 @@ -431,7 +475,7 @@ func (s *State) Equal(other *State) bool { } for _, m := range s.Modules { // This isn't very optimal currently but works. - otherM := other.ModuleByPath(m.Path) + otherM := other.moduleByPath(m.Path) if otherM == nil { return false } @@ -466,7 +510,6 @@ const ( // An error is returned if the two states are not of the same lineage, // in which case the integer returned has no meaning. func (s *State) CompareAges(other *State) (StateAgeComparison, error) { - // nil states are "older" than actual states switch { case s != nil && other == nil: @@ -483,6 +526,9 @@ func (s *State) CompareAges(other *State) (StateAgeComparison, error) { ) } + s.Lock() + defer s.Unlock() + switch { case s.Serial < other.Serial: return StateAgeReceiverOlder, nil @@ -496,6 +542,9 @@ func (s *State) CompareAges(other *State) (StateAgeComparison, error) { // SameLineage returns true only if the state given in argument belongs // to the same "lineage" of states as the reciever. func (s *State) SameLineage(other *State) bool { + s.Lock() + defer s.Unlock() + // If one of the states has no lineage then it is assumed to predate // this concept, and so we'll accept it as belonging to any lineage // so that a lineage string can be assigned to newer versions @@ -527,10 +576,13 @@ func (s *State) IncrementSerialMaybe(other *State) { 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.TFVersion != s.TFVersion || !s.equal(other) { if other.Serial > s.Serial { s.Serial = other.Serial } @@ -542,6 +594,9 @@ func (s *State) IncrementSerialMaybe(other *State) { // FromFutureTerraform checks if this state was written by a Terraform // version from the future. func (s *State) FromFutureTerraform() bool { + s.Lock() + defer s.Unlock() + // No TF version means it is certainly from the past if s.TFVersion == "" { return false @@ -552,6 +607,8 @@ func (s *State) FromFutureTerraform() bool { } func (s *State) Init() { + s.Lock() + defer s.Unlock() s.init() } @@ -574,6 +631,9 @@ func (s *State) init() { } func (s *State) EnsureHasLineage() { + s.Lock() + defer s.Unlock() + if s.Lineage == "" { s.Lineage = uuid.NewV4().String() log.Printf("[DEBUG] New state was assigned lineage %q\n", s.Lineage) @@ -585,6 +645,8 @@ func (s *State) EnsureHasLineage() { // AddModuleState insert this module state and override any existing ModuleState func (s *State) AddModuleState(mod *ModuleState) { mod.init() + s.Lock() + defer s.Unlock() for i, m := range s.Modules { if reflect.DeepEqual(m.Path, mod.Path) { @@ -624,6 +686,8 @@ func (s *State) String() string { if s == nil { return "" } + s.Lock() + defer s.Unlock() var buf bytes.Buffer for _, m := range s.Modules { From 1235623fada5bc2df19464f9eb03220192421afb Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:13:21 -0400 Subject: [PATCH 2/7] Add locks to exported RemoteState methods --- terraform/state.go | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/terraform/state.go b/terraform/state.go index 7428c6d9d8..b0a6858be6 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -755,10 +755,19 @@ func (r *RemoteState) deepcopy() *RemoteState { } func (r *RemoteState) Empty() bool { - return r == nil || r.Type == "" + if r == nil { + return true + } + r.Lock() + defer r.Unlock() + + return r.Type == "" } func (r *RemoteState) Equals(other *RemoteState) bool { + r.Lock() + defer r.Unlock() + if r.Type != other.Type { return false } From f1b44e36b41d3977e00db278459a4e3c6d95c099 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:14:37 -0400 Subject: [PATCH 3/7] add lock to OutputState.Equal --- terraform/state.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/terraform/state.go b/terraform/state.go index b0a6858be6..c6282b80ea 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -814,6 +814,8 @@ func (s *OutputState) Equal(other *OutputState) bool { if s == nil || other == nil { return false } + s.Lock() + defer s.Unlock() if s.Type != other.Type { return false From 469e71a48860b0e09440229051d3dcb50020e9ba Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:22:27 -0400 Subject: [PATCH 4/7] add locks to all exported ModuleState methods --- terraform/state.go | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/terraform/state.go b/terraform/state.go index c6282b80ea..dfa387ffaa 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -886,6 +886,9 @@ func (s *ModuleState) Unlock() { s.mu.Unlock() } // Equal tests whether one module state is equal to another. func (m *ModuleState) Equal(other *ModuleState) bool { + m.Lock() + defer m.Unlock() + // Paths must be equal if !reflect.DeepEqual(m.Path, other.Path) { return false @@ -934,11 +937,16 @@ func (m *ModuleState) Equal(other *ModuleState) bool { // IsRoot says whether or not this module diff is for the root module. func (m *ModuleState) IsRoot() bool { + m.Lock() + defer m.Unlock() return reflect.DeepEqual(m.Path, rootModulePath) } // IsDescendent returns true if other is a descendent of this module. func (m *ModuleState) IsDescendent(other *ModuleState) bool { + m.Lock() + defer m.Unlock() + i := len(m.Path) return len(other.Path) > i && reflect.DeepEqual(other.Path[:i], m.Path) } @@ -947,6 +955,9 @@ func (m *ModuleState) IsDescendent(other *ModuleState) bool { // but aren't present in the configuration itself. Hence, these keys // represent the state of resources that are orphans. func (m *ModuleState) Orphans(c *config.Config) []string { + m.Lock() + defer m.Unlock() + keys := make(map[string]struct{}) for k, _ := range m.Resources { keys[k] = struct{}{} @@ -1028,6 +1039,9 @@ func (m *ModuleState) deepcopy() *ModuleState { // prune is used to remove any resources that are no longer required func (m *ModuleState) prune() { + m.Lock() + defer m.Unlock() + for k, v := range m.Resources { v.prune() @@ -1050,6 +1064,9 @@ func (m *ModuleState) sort() { } func (m *ModuleState) String() string { + m.Lock() + defer m.Unlock() + var buf bytes.Buffer if len(m.Resources) == 0 { From 8d5080fe72e0ac278d62f730df454e1f9d0f6b56 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:26:11 -0400 Subject: [PATCH 5/7] Lock all ResourceState methods Also rename receivers so they are consistent. --- terraform/state.go | 72 +++++++++++++++++++++++++++++----------------- 1 file changed, 45 insertions(+), 27 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index dfa387ffaa..7ed8491221 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -1322,6 +1322,9 @@ func (s *ResourceState) Unlock() { s.mu.Unlock() } // Equal tests whether two ResourceStates are equal. func (s *ResourceState) Equal(other *ResourceState) bool { + s.Lock() + defer s.Unlock() + if s.Type != other.Type { return false } @@ -1351,43 +1354,49 @@ func (s *ResourceState) Equal(other *ResourceState) bool { } // Taint marks a resource as tainted. -func (r *ResourceState) Taint() { - if r.Primary != nil { - r.Primary.Tainted = true +func (s *ResourceState) Taint() { + s.Lock() + defer s.Unlock() + + if s.Primary != nil { + s.Primary.Tainted = true } } // Untaint unmarks a resource as tainted. -func (r *ResourceState) Untaint() { - if r.Primary != nil { - r.Primary.Tainted = false +func (s *ResourceState) Untaint() { + s.Lock() + defer s.Unlock() + + if s.Primary != nil { + s.Primary.Tainted = false } } -func (r *ResourceState) init() { - r.Lock() - defer r.Unlock() +func (s *ResourceState) init() { + s.Lock() + defer s.Unlock() - if r.Primary == nil { - r.Primary = &InstanceState{} + if s.Primary == nil { + s.Primary = &InstanceState{} } - r.Primary.init() + s.Primary.init() - if r.Dependencies == nil { - r.Dependencies = []string{} + if s.Dependencies == nil { + s.Dependencies = []string{} } - if r.Deposed == nil { - r.Deposed = make([]*InstanceState, 0) + if s.Deposed == nil { + s.Deposed = make([]*InstanceState, 0) } - for _, dep := range r.Deposed { + for _, dep := range s.Deposed { dep.init() } } -func (r *ResourceState) deepcopy() *ResourceState { - copy, err := copystructure.Config{Lock: true}.Copy(r) +func (s *ResourceState) deepcopy() *ResourceState { + copy, err := copystructure.Config{Lock: true}.Copy(s) if err != nil { panic(err) } @@ -1396,26 +1405,35 @@ func (r *ResourceState) deepcopy() *ResourceState { } // prune is used to remove any instances that are no longer required -func (r *ResourceState) prune() { - n := len(r.Deposed) +func (s *ResourceState) prune() { + s.Lock() + defer s.Unlock() + + n := len(s.Deposed) for i := 0; i < n; i++ { - inst := r.Deposed[i] + inst := s.Deposed[i] if inst == nil || inst.ID == "" { - copy(r.Deposed[i:], r.Deposed[i+1:]) - r.Deposed[n-1] = nil + copy(s.Deposed[i:], s.Deposed[i+1:]) + s.Deposed[n-1] = nil n-- i-- } } - r.Deposed = r.Deposed[:n] + s.Deposed = s.Deposed[:n] } -func (r *ResourceState) sort() { - sort.Strings(r.Dependencies) +func (s *ResourceState) sort() { + s.Lock() + defer s.Unlock() + + sort.Strings(s.Dependencies) } func (s *ResourceState) String() string { + s.Lock() + defer s.Unlock() + var buf bytes.Buffer buf.WriteString(fmt.Sprintf("Type = %s", s.Type)) return buf.String() From 90aab0105d5089c6d37a224cc8479f7ca852311d Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:34:40 -0400 Subject: [PATCH 6/7] Add locks to InstanceState Rename receivers for consistency --- terraform/state.go | 61 ++++++++++++++++++++++++++++------------------ 1 file changed, 37 insertions(+), 24 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 7ed8491221..884ac94366 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -1470,36 +1470,36 @@ type InstanceState struct { func (s *InstanceState) Lock() { s.mu.Lock() } func (s *InstanceState) Unlock() { s.mu.Unlock() } -func (i *InstanceState) init() { - i.Lock() - defer i.Unlock() +func (s *InstanceState) init() { + s.Lock() + defer s.Unlock() - if i.Attributes == nil { - i.Attributes = make(map[string]string) + if s.Attributes == nil { + s.Attributes = make(map[string]string) } - if i.Meta == nil { - i.Meta = make(map[string]string) + if s.Meta == nil { + s.Meta = make(map[string]string) } - i.Ephemeral.init() + s.Ephemeral.init() } // Copy all the Fields from another InstanceState -func (i *InstanceState) Set(from *InstanceState) { - i.Lock() - defer i.Unlock() +func (s *InstanceState) Set(from *InstanceState) { + s.Lock() + defer s.Unlock() from.Lock() defer from.Unlock() - i.ID = from.ID - i.Attributes = from.Attributes - i.Ephemeral = from.Ephemeral - i.Meta = from.Meta - i.Tainted = from.Tainted + s.ID = from.ID + s.Attributes = from.Attributes + s.Ephemeral = from.Ephemeral + s.Meta = from.Meta + s.Tainted = from.Tainted } -func (i *InstanceState) DeepCopy() *InstanceState { - copy, err := copystructure.Config{Lock: true}.Copy(i) +func (s *InstanceState) DeepCopy() *InstanceState { + copy, err := copystructure.Config{Lock: true}.Copy(s) if err != nil { panic(err) } @@ -1508,7 +1508,13 @@ func (i *InstanceState) DeepCopy() *InstanceState { } func (s *InstanceState) Empty() bool { - return s == nil || s.ID == "" + if s == nil { + return true + } + s.Lock() + defer s.Unlock() + + return s.ID == "" } func (s *InstanceState) Equal(other *InstanceState) bool { @@ -1516,6 +1522,8 @@ func (s *InstanceState) Equal(other *InstanceState) bool { if s == nil || other == nil { return s == other } + s.Lock() + defer s.Unlock() // IDs must be equal if s.ID != other.ID { @@ -1575,6 +1583,8 @@ func (s *InstanceState) MergeDiff(d *InstanceDiff) *InstanceState { result.init() if s != nil { + s.Lock() + defer s.Unlock() for k, v := range s.Attributes { result.Attributes[k] = v } @@ -1597,16 +1607,19 @@ func (s *InstanceState) MergeDiff(d *InstanceDiff) *InstanceState { return result } -func (i *InstanceState) String() string { +func (s *InstanceState) String() string { + s.Lock() + defer s.Unlock() + var buf bytes.Buffer - if i == nil || i.ID == "" { + if s == nil || s.ID == "" { return "" } - buf.WriteString(fmt.Sprintf("ID = %s\n", i.ID)) + buf.WriteString(fmt.Sprintf("ID = %s\n", s.ID)) - attributes := i.Attributes + attributes := s.Attributes attrKeys := make([]string, 0, len(attributes)) for ak, _ := range attributes { if ak == "id" { @@ -1622,7 +1635,7 @@ func (i *InstanceState) String() string { buf.WriteString(fmt.Sprintf("%s = %s\n", ak, av)) } - buf.WriteString(fmt.Sprintf("Tainted = %t\n", i.Tainted)) + buf.WriteString(fmt.Sprintf("Tainted = %t\n", s.Tainted)) return buf.String() } From e5ff4d5bd4a97113b53fb69a9a06327ee266bd51 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 31 Aug 2016 16:37:45 -0400 Subject: [PATCH 7/7] create some unlocked methods for State State.Init() needs to be called externally, so make sure it only calls unlocked internal methods once a lock is acquired. --- terraform/state.go | 19 ++++++++++++++++--- 1 file changed, 16 insertions(+), 3 deletions(-) diff --git a/terraform/state.go b/terraform/state.go index 884ac94366..2ab56ba6ce 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -128,6 +128,11 @@ func (s *State) children(path []string) []*ModuleState { func (s *State) AddModule(path []string) *ModuleState { s.Lock() defer s.Unlock() + + return s.addModule(path) +} + +func (s *State) addModule(path []string) *ModuleState { // check if the module exists first m := s.moduleByPath(path) if m != nil { @@ -616,10 +621,10 @@ func (s *State) init() { if s.Version == 0 { s.Version = StateVersion } - if s.ModuleByPath(rootModulePath) == nil { - s.AddModule(rootModulePath) + if s.moduleByPath(rootModulePath) == nil { + s.addModule(rootModulePath) } - s.EnsureHasLineage() + s.ensureHasLineage() for _, mod := range s.Modules { mod.init() @@ -634,6 +639,10 @@ func (s *State) EnsureHasLineage() { s.Lock() defer s.Unlock() + s.ensureHasLineage() +} + +func (s *State) ensureHasLineage() { if s.Lineage == "" { s.Lineage = uuid.NewV4().String() log.Printf("[DEBUG] New state was assigned lineage %q\n", s.Lineage) @@ -648,6 +657,10 @@ func (s *State) AddModuleState(mod *ModuleState) { s.Lock() defer s.Unlock() + s.addModuleState(mod) +} + +func (s *State) addModuleState(mod *ModuleState) { for i, m := range s.Modules { if reflect.DeepEqual(m.Path, mod.Path) { s.Modules[i] = mod