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
This commit is contained in:
James Bardin 2016-07-29 13:17:48 -04:00
parent 9dec28bccf
commit 5802f76eaa
13 changed files with 212 additions and 54 deletions

View File

@ -47,7 +47,7 @@ func (h *CountHook) PreApply(
}
action := countHookActionChange
if d.Destroy {
if d.GetDestroy() {
action = countHookActionRemove
} else if s.ID == "" {
action = countHookActionAdd

View File

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

View File

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

View File

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

View File

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

View File

@ -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())
}

View File

@ -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 {
// "<computed>". 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)
}
}

View File

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

View File

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

View File

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

View File

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

View File

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

View File

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