From bd299b9a225a37ddb58099fe863ed63787e99522 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Mon, 27 Aug 2018 14:33:08 -0700 Subject: [PATCH] core: Re-implement EvalWriteDiff to work with new plan types --- plans/changes.go | 9 ++++ plans/changes_src.go | 32 +++++++++++++ plans/changes_sync.go | 17 +++++++ plans/dynamic_value.go | 11 +++++ terraform/context.go | 1 + terraform/eval_diff.go | 57 ++++++++++++------------ terraform/node_resource_apply.go | 10 +++-- terraform/node_resource_plan_destroy.go | 5 ++- terraform/node_resource_plan_instance.go | 10 +++-- terraform/node_resource_plan_orphan.go | 5 ++- 10 files changed, 116 insertions(+), 41 deletions(-) diff --git a/plans/changes.go b/plans/changes.go index 4f7c6f676..bb4ca43bd 100644 --- a/plans/changes.go +++ b/plans/changes.go @@ -55,6 +55,15 @@ func (c *Changes) ResourceInstanceDeposed(addr addrs.AbsResourceInstance, key st return nil } +// SyncWrapper returns a wrapper object around the receiver that can be used +// to make certain changes to the receiver in a concurrency-safe way, as long +// as all callers share the same wrapper object. +func (c *Changes) SyncWrapper() *ChangesSync { + return &ChangesSync{ + changes: c, + } +} + // ResourceInstanceChange describes a change to a particular resource instance // object. type ResourceInstanceChange struct { diff --git a/plans/changes_src.go b/plans/changes_src.go index f97e5fbd4..7d195d2ae 100644 --- a/plans/changes_src.go +++ b/plans/changes_src.go @@ -66,6 +66,38 @@ func (rcs *ResourceInstanceChangeSrc) Decode(ty cty.Type) (*ResourceInstanceChan }, nil } +// DeepCopy creates a copy of the receiver where any pointers to nested mutable +// values are also copied, thus ensuring that future mutations of the receiver +// will not affect the copy. +// +// Some types used within a resource change are immutable by convention even +// though the Go language allows them to be mutated, such as the types from +// the addrs package. These are _not_ copied by this method, under the +// assumption that callers will behave themselves. +func (rcs *ResourceInstanceChangeSrc) DeepCopy() *ResourceInstanceChangeSrc { + if rcs == nil { + return nil + } + ret := *rcs + + if len(ret.RequiredReplace) != 0 { + rr := make([]cty.Path, len(ret.RequiredReplace)) + copy(rr, ret.RequiredReplace) + ret.RequiredReplace = rr + } + + if len(ret.Private) != 0 { + private := make([]byte, len(ret.Private)) + copy(private, ret.Private) + ret.Private = private + } + + ret.ChangeSrc.Before = ret.ChangeSrc.Before.Copy() + ret.ChangeSrc.After = ret.ChangeSrc.After.Copy() + + return &ret +} + // OutputChange describes a change to an output value. type OutputChangeSrc struct { // ChangeSrc is an embedded description of the not-yet-decoded change. diff --git a/plans/changes_sync.go b/plans/changes_sync.go index eec8756e7..32716b3d0 100644 --- a/plans/changes_sync.go +++ b/plans/changes_sync.go @@ -16,3 +16,20 @@ type ChangesSync struct { lock sync.Mutex changes *Changes } + +// AppendResourceInstanceChange records the given resource instance change in +// the set of planned resource changes. +// +// The caller must ensure that there are no concurrent writes to the given +// change while this method is running, but it is safe to resume mutating +// it after this method returns without affecting the saved change. +func (cs *ChangesSync) AppendResourceInstanceChange(changeSrc *ResourceInstanceChangeSrc) { + if cs == nil { + panic("AppendResourceInstanceChange on nil ChangesSync") + } + cs.lock.Lock() + defer cs.lock.Unlock() + + s := changeSrc.DeepCopy() + cs.changes.Resources = append(cs.changes.Resources, s) +} diff --git a/plans/dynamic_value.go b/plans/dynamic_value.go index fadf24312..91c1215b8 100644 --- a/plans/dynamic_value.go +++ b/plans/dynamic_value.go @@ -83,3 +83,14 @@ func (v DynamicValue) Decode(ty cty.Type) (cty.Value, error) { func (v DynamicValue) ImpliedType() (cty.Type, error) { return ctymsgpack.ImpliedType([]byte(v)) } + +// Copy produces a copy of the receiver with a distinct backing array. +func (v DynamicValue) Copy() DynamicValue { + if len(v) == 0 { + return nil + } + + ret := make(DynamicValue, len(v)) + copy(ret, v) + return ret +} diff --git a/terraform/context.go b/terraform/context.go index be4625551..a3df33c7c 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -718,6 +718,7 @@ func (c *Context) graphWalker(operation walkOperation) *ContextGraphWalker { return &ContextGraphWalker{ Context: c, State: c.state.SyncWrapper(), + Changes: c.changes.SyncWrapper(), Operation: operation, StopContext: c.runContext, RootVariableValues: c.variables, diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 95f93410b..80b194faf 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -732,41 +732,40 @@ func (n *EvalReadDiff) Eval(ctx EvalContext) (interface{}, error) { // EvalWriteDiff is an EvalNode implementation that saves a planned change // for an instance object into the set of global planned changes. type EvalWriteDiff struct { - Addr addrs.ResourceInstance - DeposedKey states.DeposedKey - Change **plans.ResourceInstanceChange + Addr addrs.ResourceInstance + DeposedKey states.DeposedKey + ProviderSchema **ProviderSchema + Change **plans.ResourceInstanceChange } // TODO: test func (n *EvalWriteDiff) Eval(ctx EvalContext) (interface{}, error) { - return nil, fmt.Errorf("EvalWriteDiff not yet updated for new plan types") - /* - diff, lock := ctx.Diff() + providerSchema := *n.ProviderSchema + change := *n.Change - // The diff to write, if its empty it should write nil - var diffVal *InstanceDiff - if n.Diff != nil { - diffVal = *n.Diff - } - if diffVal.Empty() { - diffVal = nil - } + if change.Addr.String() != n.Addr.String() || change.DeposedKey != n.DeposedKey { + // Should never happen, and indicates a bug in the caller. + panic("inconsistent address and/or deposed key in EvalWriteDiff") + } - // Acquire the lock so that we can do this safely concurrently - lock.Lock() - defer lock.Unlock() + schema := providerSchema.ResourceTypes[n.Addr.Resource.Type] + if schema == nil { + // Should be caught during validation, so we don't bother with a pretty error here + return nil, fmt.Errorf("provider does not support resource type %q", n.Addr.Resource.Type) + } - // Write the diff - modDiff := diff.ModuleByPath(ctx.Path()) - if modDiff == nil { - modDiff = diff.AddModule(ctx.Path()) - } - if diffVal != nil { - modDiff.Resources[n.Name] = diffVal - } else { - delete(modDiff.Resources, n.Name) - } + csrc, err := change.Encode(schema.ImpliedType()) + if err != nil { + return nil, fmt.Errorf("failed to encode planned changes for %s: %s", n.Addr, err) + } - return nil, nil - */ + changes := ctx.Changes() + changes.AppendResourceInstanceChange(csrc) + if n.DeposedKey == states.NotDeposed { + log.Printf("[TRACE] EvalWriteDiff: recorded %s change for %s", change.Action, n.Addr) + } else { + log.Printf("[TRACE] EvalWriteDiff: recorded %s change for %s deposed object %s", change.Action, n.Addr, n.DeposedKey) + } + + return nil, nil } diff --git a/terraform/node_resource_apply.go b/terraform/node_resource_apply.go index 80a0bce33..4ca75ae0b 100644 --- a/terraform/node_resource_apply.go +++ b/terraform/node_resource_apply.go @@ -181,8 +181,9 @@ func (n *NodeApplyableResourceInstance) evalTreeDataResource(addr addrs.AbsResou // Clear the diff now that we've applied it, so // later nodes won't see a diff that's now a no-op. &EvalWriteDiff{ - Addr: addr.Resource, - Change: nil, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: nil, }, &EvalUpdateStateHook{}, @@ -347,8 +348,9 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe // don't see a diff that is already complete. There // is no longer a diff! &EvalWriteDiff{ - Addr: addr.Resource, - Change: nil, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: nil, }, &EvalApplyPost{ diff --git a/terraform/node_resource_plan_destroy.go b/terraform/node_resource_plan_destroy.go index 22191d1e7..d978b6505 100644 --- a/terraform/node_resource_plan_destroy.go +++ b/terraform/node_resource_plan_destroy.go @@ -78,8 +78,9 @@ func (n *NodePlanDestroyableResourceInstance) EvalTree() EvalNode { Change: &change, }, &EvalWriteDiff{ - Addr: addr.Resource, - Change: &change, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: &change, }, }, } diff --git a/terraform/node_resource_plan_instance.go b/terraform/node_resource_plan_instance.go index 217a4eaa7..31d35a333 100644 --- a/terraform/node_resource_plan_instance.go +++ b/terraform/node_resource_plan_instance.go @@ -99,8 +99,9 @@ func (n *NodePlannableResourceInstance) evalTreeDataResource(addr addrs.AbsResou }, &EvalWriteDiff{ - Addr: addr.Resource, - Change: &change, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: &change, }, }, } @@ -157,8 +158,9 @@ func (n *NodePlannableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe ProviderSchema: &providerSchema, }, &EvalWriteDiff{ - Addr: addr.Resource, - Change: &change, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: &change, }, }, } diff --git a/terraform/node_resource_plan_orphan.go b/terraform/node_resource_plan_orphan.go index 8f5328375..fddd465e6 100644 --- a/terraform/node_resource_plan_orphan.go +++ b/terraform/node_resource_plan_orphan.go @@ -68,8 +68,9 @@ func (n *NodePlannableResourceInstanceOrphan) EvalTree() EvalNode { Change: &change, }, &EvalWriteDiff{ - Addr: addr.Resource, - Change: &change, + Addr: addr.Resource, + ProviderSchema: &providerSchema, + Change: &change, }, &EvalWriteState{ Addr: addr.Resource,