From 5802f76eaa44d168e4438a5f354811912fa7da8c Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Jul 2016 13:17:48 -0400 Subject: [PATCH 1/2] Make all terraform package tests pass under -race This isn't a pretty refactor, but fixes the race issues in this package for now. Fix race on RawConfig.Config() fix command package races --- command/hook_count.go | 2 +- command/hook_ui.go | 8 +- config/raw_config.go | 2 + terraform/diff.go | 133 +++++++++++++++++++++--- terraform/eval_apply.go | 6 +- terraform/eval_check_prevent_destroy.go | 2 +- terraform/eval_diff.go | 36 +++---- terraform/eval_read_data.go | 8 +- terraform/graph_config_node_resource.go | 2 +- terraform/hook_mock.go | 46 ++++++++ terraform/resource_provisioner_mock.go | 9 ++ terraform/state.go | 2 +- terraform/transform_resource.go | 10 +- 13 files changed, 212 insertions(+), 54 deletions(-) diff --git a/command/hook_count.go b/command/hook_count.go index 3e642b254..150ae438e 100644 --- a/command/hook_count.go +++ b/command/hook_count.go @@ -47,7 +47,7 @@ func (h *CountHook) PreApply( } action := countHookActionChange - if d.Destroy { + if d.GetDestroy() { action = countHookActionRemove } else if s.ID == "" { action = countHookActionAdd diff --git a/command/hook_ui.go b/command/hook_ui.go index 2270a1dea..a41f40c98 100644 --- a/command/hook_ui.go +++ b/command/hook_ui.go @@ -84,8 +84,10 @@ func (h *UiHook) PreApply( // Get all the attributes that are changing, and sort them. Also // determine the longest key so that we can align them all. keyLen := 0 - keys := make([]string, 0, len(d.Attributes)) - for key, _ := range d.Attributes { + + dAttrs := d.CopyAttributes() + keys := make([]string, 0, len(dAttrs)) + for key, _ := range dAttrs { // Skip the ID since we do that specially if key == "id" { continue @@ -100,7 +102,7 @@ func (h *UiHook) PreApply( // Go through and output each attribute for _, attrK := range keys { - attrDiff := d.Attributes[attrK] + attrDiff, _ := d.GetAttribute(attrK) v := attrDiff.New u := attrDiff.Old diff --git a/config/raw_config.go b/config/raw_config.go index 18b9dcaf2..b4cb71450 100644 --- a/config/raw_config.go +++ b/config/raw_config.go @@ -93,6 +93,8 @@ func (r *RawConfig) Value() interface{} { // structure will always successfully decode into its ultimate // structure using something like mapstructure. func (r *RawConfig) Config() map[string]interface{} { + r.lock.Lock() + defer r.lock.Unlock() return r.config } diff --git a/terraform/diff.go b/terraform/diff.go index d5008c6c8..f3e7a092f 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -8,6 +8,7 @@ import ( "regexp" "sort" "strings" + "sync" ) // DiffChangeType is an enum with the kind of changes a diff has planned. @@ -216,9 +217,9 @@ func (d *ModuleDiff) String() string { crud := "UPDATE" switch { - case rdiff.RequiresNew() && (rdiff.Destroy || rdiff.DestroyTainted): + case rdiff.RequiresNew() && (rdiff.GetDestroy() || rdiff.GetDestroyTainted()): crud = "DESTROY/CREATE" - case rdiff.Destroy: + case rdiff.GetDestroy(): crud = "DESTROY" case rdiff.RequiresNew(): crud = "CREATE" @@ -230,8 +231,9 @@ func (d *ModuleDiff) String() string { name)) keyLen := 0 - keys := make([]string, 0, len(rdiff.Attributes)) - for key, _ := range rdiff.Attributes { + rdiffAttrs := rdiff.CopyAttributes() + keys := make([]string, 0, len(rdiffAttrs)) + for key, _ := range rdiffAttrs { if key == "id" { continue } @@ -244,7 +246,7 @@ func (d *ModuleDiff) String() string { sort.Strings(keys) for _, attrK := range keys { - attrDiff := rdiff.Attributes[attrK] + attrDiff, _ := rdiff.GetAttribute(attrK) v := attrDiff.New u := attrDiff.Old @@ -279,6 +281,7 @@ func (d *ModuleDiff) String() string { // InstanceDiff is the diff of a resource from some state to another. type InstanceDiff struct { + mu sync.Mutex Attributes map[string]*ResourceAttrDiff Destroy bool DestroyTainted bool @@ -324,6 +327,10 @@ func (d *InstanceDiff) init() { } } +func NewInstanceDiff() *InstanceDiff { + return &InstanceDiff{Attributes: make(map[string]*ResourceAttrDiff)} +} + // ChangeType returns the DiffChangeType represented by the diff // for this single instance. func (d *InstanceDiff) ChangeType() DiffChangeType { @@ -331,11 +338,11 @@ func (d *InstanceDiff) ChangeType() DiffChangeType { return DiffNone } - if d.RequiresNew() && (d.Destroy || d.DestroyTainted) { + if d.RequiresNew() && (d.GetDestroy() || d.GetDestroyTainted()) { return DiffDestroyCreate } - if d.Destroy { + if d.GetDestroy() { return DiffDestroy } @@ -352,6 +359,8 @@ func (d *InstanceDiff) Empty() bool { return true } + d.mu.Lock() + defer d.mu.Unlock() return !d.Destroy && len(d.Attributes) == 0 } @@ -366,6 +375,17 @@ func (d *InstanceDiff) RequiresNew() bool { return false } + d.mu.Lock() + defer d.mu.Unlock() + + return d.requiresNew() +} + +func (d *InstanceDiff) requiresNew() bool { + if d == nil { + return false + } + if d.DestroyTainted { return true } @@ -379,24 +399,103 @@ func (d *InstanceDiff) RequiresNew() bool { return false } +// These methods are properly locked, for use outside other InstanceDiff +// methods but everywhere else within in the terraform package. +// TODO refactor the locking scheme +func (d *InstanceDiff) SetTainted(b bool) { + d.mu.Lock() + defer d.mu.Unlock() + + d.DestroyTainted = b +} + +func (d *InstanceDiff) GetDestroyTainted() bool { + d.mu.Lock() + defer d.mu.Unlock() + + return d.DestroyTainted +} + +func (d *InstanceDiff) SetDestroy(b bool) { + d.mu.Lock() + defer d.mu.Unlock() + + d.Destroy = b +} + +func (d *InstanceDiff) GetDestroy() bool { + d.mu.Lock() + defer d.mu.Unlock() + + return d.Destroy +} + +func (d *InstanceDiff) SetAttribute(key string, attr *ResourceAttrDiff) { + d.mu.Lock() + defer d.mu.Unlock() + + d.Attributes[key] = attr +} + +func (d *InstanceDiff) DelAttribute(key string) { + d.mu.Lock() + defer d.mu.Unlock() + + delete(d.Attributes, key) +} + +func (d *InstanceDiff) GetAttribute(key string) (*ResourceAttrDiff, bool) { + d.mu.Lock() + defer d.mu.Unlock() + + attr, ok := d.Attributes[key] + return attr, ok +} +func (d *InstanceDiff) GetAttributesLen() int { + d.mu.Lock() + defer d.mu.Unlock() + + return len(d.Attributes) +} + +// Safely copies the Attributes map +func (d *InstanceDiff) CopyAttributes() map[string]*ResourceAttrDiff { + d.mu.Lock() + defer d.mu.Unlock() + + attrs := make(map[string]*ResourceAttrDiff) + for k, v := range d.Attributes { + attrs[k] = v + } + + return attrs +} + // Same checks whether or not two InstanceDiff's are the "same". When // we say "same", it is not necessarily exactly equal. Instead, it is // just checking that the same attributes are changing, a destroy // isn't suddenly happening, etc. func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) { - if d == nil && d2 == nil { + // we can safely compare the pointers without a lock + switch { + case d == nil && d2 == nil: + return true, "" + case d == nil || d2 == nil: + return false, "one nil" + case d == d2: return true, "" - } else if d == nil || d2 == nil { - return false, "both nil" } - if d.Destroy != d2.Destroy { + d.mu.Lock() + defer d.mu.Unlock() + + if d.Destroy != d2.GetDestroy() { return false, fmt.Sprintf( - "diff: Destroy; old: %t, new: %t", d.Destroy, d2.Destroy) + "diff: Destroy; old: %t, new: %t", d.Destroy, d2.GetDestroy()) } - if d.RequiresNew() != d2.RequiresNew() { + if d.requiresNew() != d2.RequiresNew() { return false, fmt.Sprintf( - "diff RequiresNew; old: %t, new: %t", d.RequiresNew(), d2.RequiresNew()) + "diff RequiresNew; old: %t, new: %t", d.requiresNew(), d2.RequiresNew()) } // Go through the old diff and make sure the new diff has all the @@ -406,7 +505,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) { for k, _ := range d.Attributes { checkOld[k] = struct{}{} } - for k, _ := range d2.Attributes { + for k, _ := range d2.CopyAttributes() { checkNew[k] = struct{}{} } @@ -431,7 +530,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) { delete(checkOld, k) delete(checkNew, k) - _, ok := d2.Attributes[k] + _, ok := d2.GetAttribute(k) if !ok { // If there's no new attribute, and the old diff expected the attribute // to be removed, that's just fine. @@ -483,7 +582,7 @@ func (d *InstanceDiff) Same(d2 *InstanceDiff) (bool, string) { // Similarly, in a RequiresNew scenario, a list that shows up in the plan // diff can disappear from the apply diff, which is calculated from an // empty state. - if d.RequiresNew() && (strings.HasSuffix(k, ".#") || strings.HasSuffix(k, ".%")) { + if d.requiresNew() && (strings.HasSuffix(k, ".#") || strings.HasSuffix(k, ".%")) { ok = true } diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index fd687c5a1..5dced0136 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -35,9 +35,9 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { } // Remove any output values from the diff - for k, ad := range diff.Attributes { + for k, ad := range diff.CopyAttributes() { if ad.Type == DiffAttrOutput { - delete(diff.Attributes, k) + diff.DelAttribute(k) } } @@ -49,7 +49,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { // Flag if we're creating a new instance if n.CreateNew != nil { - *n.CreateNew = state.ID == "" && !diff.Destroy || diff.RequiresNew() + *n.CreateNew = state.ID == "" && !diff.GetDestroy() || diff.RequiresNew() } { diff --git a/terraform/eval_check_prevent_destroy.go b/terraform/eval_check_prevent_destroy.go index ae9dc4f82..aec0ae134 100644 --- a/terraform/eval_check_prevent_destroy.go +++ b/terraform/eval_check_prevent_destroy.go @@ -22,7 +22,7 @@ func (n *EvalCheckPreventDestroy) Eval(ctx EvalContext) (interface{}, error) { diff := *n.Diff preventDestroy := n.Resource.Lifecycle.PreventDestroy - if diff.Destroy && preventDestroy { + if diff.GetDestroy() && preventDestroy { return nil, fmt.Errorf(preventDestroyErrStr, n.Resource.Id()) } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 4a5027d60..8fe476688 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -28,16 +28,16 @@ func (n *EvalCompareDiff) Eval(ctx EvalContext) (interface{}, error) { two = new(InstanceDiff) two.init() } - oneId := one.Attributes["id"] - twoId := two.Attributes["id"] - delete(one.Attributes, "id") - delete(two.Attributes, "id") + oneId, _ := one.GetAttribute("id") + twoId, _ := two.GetAttribute("id") + one.DelAttribute("id") + two.DelAttribute("id") defer func() { if oneId != nil { - one.Attributes["id"] = oneId + one.SetAttribute("id", oneId) } if twoId != nil { - two.Attributes["id"] = twoId + two.SetAttribute("id", twoId) } }() @@ -114,12 +114,12 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // Preserve the DestroyTainted flag if n.Diff != nil { - diff.DestroyTainted = (*n.Diff).DestroyTainted + diff.SetTainted((*n.Diff).GetDestroyTainted()) } // Require a destroy if there is an ID and it requires new. if diff.RequiresNew() && state != nil && state.ID != "" { - diff.Destroy = true + diff.SetDestroy(true) } // If we're creating a new resource, compute its ID @@ -131,12 +131,12 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { // Add diff to compute new ID diff.init() - diff.Attributes["id"] = &ResourceAttrDiff{ + diff.SetAttribute("id", &ResourceAttrDiff{ Old: oldID, NewComputed: true, RequiresNew: true, Type: DiffAttrOutput, - } + }) } if err := n.processIgnoreChanges(diff); err != nil { @@ -187,7 +187,7 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { ignorableAttrKeys := make(map[string]bool) for _, ignoredKey := range ignoreChanges { - for k := range diff.Attributes { + for k := range diff.CopyAttributes() { if strings.HasPrefix(k, ignoredKey) { ignorableAttrKeys[k] = true } @@ -200,7 +200,7 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { // "". Filtering these out allows us to see if we might be able to // skip this diff altogether. if changeType == DiffDestroyCreate { - for k, v := range diff.Attributes { + for k, v := range diff.CopyAttributes() { if v.Empty() || v.NewComputed { ignorableAttrKeys[k] = true } @@ -210,7 +210,7 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { // tweak, we ignore the "id" attribute diff that gets added by EvalDiff, // since that was added in reaction to RequiresNew being true. requiresNewAfterIgnores := false - for k, v := range diff.Attributes { + for k, v := range diff.CopyAttributes() { if k == "id" { continue } @@ -233,15 +233,15 @@ func (n *EvalDiff) processIgnoreChanges(diff *InstanceDiff) error { // attribute diff and the Destroy boolean field log.Printf("[DEBUG] Removing 'id' diff and setting Destroy to false " + "because after ignore_changes, this diff no longer requires replacement") - delete(diff.Attributes, "id") - diff.Destroy = false + diff.DelAttribute("id") + diff.SetDestroy(false) } // If we didn't hit any of our early exit conditions, we can filter the diff. for k := range ignorableAttrKeys { log.Printf("[DEBUG] [EvalIgnoreChanges] %s - Ignoring diff attribute: %s", n.Resource.Id(), k) - delete(diff.Attributes, k) + diff.DelAttribute(k) } return nil @@ -333,8 +333,8 @@ func (n *EvalFilterDiff) Eval(ctx EvalContext) (interface{}, error) { result := new(InstanceDiff) if n.Destroy { - if input.Destroy || input.RequiresNew() { - result.Destroy = true + if input.GetDestroy() || input.RequiresNew() { + result.SetDestroy(true) } } diff --git a/terraform/eval_read_data.go b/terraform/eval_read_data.go index 4fd843123..aeb2ebaef 100644 --- a/terraform/eval_read_data.go +++ b/terraform/eval_read_data.go @@ -30,7 +30,7 @@ func (n *EvalReadDataDiff) Eval(ctx EvalContext) (interface{}, error) { var diff *InstanceDiff - if n.Previous != nil && *n.Previous != nil && (*n.Previous).Destroy { + if n.Previous != nil && *n.Previous != nil && (*n.Previous).GetDestroy() { // If we're re-diffing for a diff that was already planning to // destroy, then we'll just continue with that plan. diff = &InstanceDiff{Destroy: true} @@ -49,12 +49,12 @@ func (n *EvalReadDataDiff) Eval(ctx EvalContext) (interface{}, error) { // id is always computed, because we're always "creating a new resource" diff.init() - diff.Attributes["id"] = &ResourceAttrDiff{ + diff.SetAttribute("id", &ResourceAttrDiff{ Old: "", NewComputed: true, RequiresNew: true, Type: DiffAttrOutput, - } + }) } err = ctx.Hook(func(h Hook) (HookAction, error) { @@ -97,7 +97,7 @@ func (n *EvalReadDataApply) Eval(ctx EvalContext) (interface{}, error) { // If the diff is for *destroying* this resource then we'll // just drop its state and move on, since data resources don't // support an actual "destroy" action. - if diff != nil && diff.Destroy { + if diff != nil && diff.GetDestroy() { if n.Output != nil { *n.Output = nil } diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index 36e1d9112..1c45289a9 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -485,7 +485,7 @@ func (n *graphNodeResourceDestroy) destroyInclude( if d != nil { for k, v := range d.Resources { match := k == prefix || strings.HasPrefix(k, prefix+".") - if match && v.Destroy { + if match && v.GetDestroy() { return true } } diff --git a/terraform/hook_mock.go b/terraform/hook_mock.go index d6c5fcb3f..3797a1e15 100644 --- a/terraform/hook_mock.go +++ b/terraform/hook_mock.go @@ -1,8 +1,12 @@ package terraform +import "sync" + // MockHook is an implementation of Hook that can be used for tests. // It records all of its function calls. type MockHook struct { + sync.Mutex + PreApplyCalled bool PreApplyInfo *InstanceInfo PreApplyDiff *InstanceDiff @@ -89,6 +93,9 @@ type MockHook struct { } func (h *MockHook) PreApply(n *InstanceInfo, s *InstanceState, d *InstanceDiff) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreApplyCalled = true h.PreApplyInfo = n h.PreApplyDiff = d @@ -97,6 +104,9 @@ func (h *MockHook) PreApply(n *InstanceInfo, s *InstanceState, d *InstanceDiff) } func (h *MockHook) PostApply(n *InstanceInfo, s *InstanceState, e error) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostApplyCalled = true h.PostApplyInfo = n h.PostApplyState = s @@ -105,6 +115,9 @@ func (h *MockHook) PostApply(n *InstanceInfo, s *InstanceState, e error) (HookAc } func (h *MockHook) PreDiff(n *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreDiffCalled = true h.PreDiffInfo = n h.PreDiffState = s @@ -112,6 +125,9 @@ func (h *MockHook) PreDiff(n *InstanceInfo, s *InstanceState) (HookAction, error } func (h *MockHook) PostDiff(n *InstanceInfo, d *InstanceDiff) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostDiffCalled = true h.PostDiffInfo = n h.PostDiffDiff = d @@ -119,6 +135,9 @@ func (h *MockHook) PostDiff(n *InstanceInfo, d *InstanceDiff) (HookAction, error } func (h *MockHook) PreProvisionResource(n *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreProvisionResourceCalled = true h.PreProvisionResourceInfo = n h.PreProvisionInstanceState = s @@ -126,6 +145,9 @@ func (h *MockHook) PreProvisionResource(n *InstanceInfo, s *InstanceState) (Hook } func (h *MockHook) PostProvisionResource(n *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostProvisionResourceCalled = true h.PostProvisionResourceInfo = n h.PostProvisionInstanceState = s @@ -133,6 +155,9 @@ func (h *MockHook) PostProvisionResource(n *InstanceInfo, s *InstanceState) (Hoo } func (h *MockHook) PreProvision(n *InstanceInfo, provId string) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreProvisionCalled = true h.PreProvisionInfo = n h.PreProvisionProvisionerId = provId @@ -140,6 +165,9 @@ func (h *MockHook) PreProvision(n *InstanceInfo, provId string) (HookAction, err } func (h *MockHook) PostProvision(n *InstanceInfo, provId string) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostProvisionCalled = true h.PostProvisionInfo = n h.PostProvisionProvisionerId = provId @@ -150,6 +178,9 @@ func (h *MockHook) ProvisionOutput( n *InstanceInfo, provId string, msg string) { + h.Lock() + defer h.Unlock() + h.ProvisionOutputCalled = true h.ProvisionOutputInfo = n h.ProvisionOutputProvisionerId = provId @@ -157,6 +188,9 @@ func (h *MockHook) ProvisionOutput( } func (h *MockHook) PreRefresh(n *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreRefreshCalled = true h.PreRefreshInfo = n h.PreRefreshState = s @@ -164,6 +198,9 @@ func (h *MockHook) PreRefresh(n *InstanceInfo, s *InstanceState) (HookAction, er } func (h *MockHook) PostRefresh(n *InstanceInfo, s *InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostRefreshCalled = true h.PostRefreshInfo = n h.PostRefreshState = s @@ -171,6 +208,9 @@ func (h *MockHook) PostRefresh(n *InstanceInfo, s *InstanceState) (HookAction, e } func (h *MockHook) PreImportState(info *InstanceInfo, id string) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PreImportStateCalled = true h.PreImportStateInfo = info h.PreImportStateId = id @@ -178,6 +218,9 @@ func (h *MockHook) PreImportState(info *InstanceInfo, id string) (HookAction, er } func (h *MockHook) PostImportState(info *InstanceInfo, s []*InstanceState) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostImportStateCalled = true h.PostImportStateInfo = info h.PostImportStateState = s @@ -185,6 +228,9 @@ func (h *MockHook) PostImportState(info *InstanceInfo, s []*InstanceState) (Hook } func (h *MockHook) PostStateUpdate(s *State) (HookAction, error) { + h.Lock() + defer h.Unlock() + h.PostStateUpdateCalled = true h.PostStateUpdateState = s return h.PostStateUpdateReturn, h.PostStateUpdateError diff --git a/terraform/resource_provisioner_mock.go b/terraform/resource_provisioner_mock.go index 2ba7220cd..be04e9814 100644 --- a/terraform/resource_provisioner_mock.go +++ b/terraform/resource_provisioner_mock.go @@ -1,8 +1,11 @@ package terraform +import "sync" + // MockResourceProvisioner implements ResourceProvisioner but mocks out all the // calls for testing purposes. type MockResourceProvisioner struct { + sync.Mutex // Anything you want, in case you need to store extra data with the mock. Meta interface{} @@ -21,6 +24,9 @@ type MockResourceProvisioner struct { } func (p *MockResourceProvisioner) Validate(c *ResourceConfig) ([]string, []error) { + p.Lock() + defer p.Unlock() + p.ValidateCalled = true p.ValidateConfig = c if p.ValidateFn != nil { @@ -33,6 +39,9 @@ func (p *MockResourceProvisioner) Apply( output UIOutput, state *InstanceState, c *ResourceConfig) error { + p.Lock() + defer p.Unlock() + p.ApplyCalled = true p.ApplyOutput = output p.ApplyState = state diff --git a/terraform/state.go b/terraform/state.go index a8d3dac89..5d7524c80 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -1331,7 +1331,7 @@ func (s *InstanceState) MergeDiff(d *InstanceDiff) *InstanceState { } } if d != nil { - for k, diff := range d.Attributes { + for k, diff := range d.CopyAttributes() { if diff.NewRemoved { delete(result.Attributes, k) continue diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index 2ab485cde..b877a6051 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -418,11 +418,11 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, return true, EvalEarlyExitError{} } - if diffApply.Destroy && len(diffApply.Attributes) == 0 { + if diffApply.GetDestroy() && diffApply.GetAttributesLen() == 0 { return true, EvalEarlyExitError{} } - diffApply.Destroy = false + diffApply.SetDestroy(false) return true, nil }, Then: EvalNoop{}, @@ -432,7 +432,7 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, If: func(ctx EvalContext) (bool, error) { destroy := false if diffApply != nil { - destroy = diffApply.Destroy || diffApply.RequiresNew() + destroy = diffApply.GetDestroy() || diffApply.RequiresNew() } createBeforeDestroyEnabled = @@ -762,7 +762,7 @@ func (n *graphNodeExpandedResource) dataResourceEvalNodes(resource *Resource, in return true, EvalEarlyExitError{} } - if len(diff.Attributes) == 0 { + if diff.GetAttributesLen() == 0 { return true, EvalEarlyExitError{} } @@ -887,7 +887,7 @@ func (n *graphNodeExpandedResourceDestroy) EvalTree() EvalNode { // If we're not destroying, then compare diffs &EvalIf{ If: func(ctx EvalContext) (bool, error) { - if diffApply != nil && diffApply.Destroy { + if diffApply != nil && diffApply.GetDestroy() { return true, nil } From 074be9ae5658bad51d6fc2c77ae0c0be28783081 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 29 Jul 2016 18:35:54 -0400 Subject: [PATCH 2/2] Another race in resource.Retry --- helper/resource/wait.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/helper/resource/wait.go b/helper/resource/wait.go index 77a7a01b0..ca50e292f 100644 --- a/helper/resource/wait.go +++ b/helper/resource/wait.go @@ -20,13 +20,15 @@ func Retry(timeout time.Duration, f RetryFunc) error { MinTimeout: 500 * time.Millisecond, Refresh: func() (interface{}, string, error) { rerr := f() + + resultErrMu.Lock() + defer resultErrMu.Unlock() + if rerr == nil { resultErr = nil return 42, "success", nil } - resultErrMu.Lock() - defer resultErrMu.Unlock() resultErr = rerr.Err if rerr.Retryable {