From 9eb32c45360d7ab15c7eaa6d7e02fcd84594c8c4 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Fri, 14 Sep 2018 15:13:58 -0700 Subject: [PATCH] core: Reinstaint instance tainting, but without mutating objects Our previous mechanism for dealing with tainting relied on directly mutating the InstanceState object to mark it as such. In our new state models we consider the instance objects to be immutable by convention, and so we frequently copy them. As a result, the taint flagging was no longer making it all the way through the apply evaluation process. Here we now implement tainting as a separate step in the evaluation process, creating a copy of the object with a tainted status if there were any errors during creation. This introduces a new behavior where any provider-level errors during creation will also cause an instance to be marked as tainted if any object is returned at all. Create-time errors _normally_ result in no object at all, but the provider might return an object if the failure occurred at a subsequent step of a multi-step creation process and so left behind a remote object that needs to be cleaned up on a future run. --- states/instance_object.go | 14 ++++ terraform/eval_apply.go | 87 ++++++++++++++++++----- terraform/node_resource_apply_instance.go | 16 ++++- 3 files changed, 98 insertions(+), 19 deletions(-) diff --git a/states/instance_object.go b/states/instance_object.go index be836053f..b45bfa6ac 100644 --- a/states/instance_object.go +++ b/states/instance_object.go @@ -104,3 +104,17 @@ func (o *ResourceInstanceObject) Encode(ty cty.Type, schemaVersion uint64) (*Res Dependencies: o.Dependencies, }, nil } + +// AsTainted returns a deep copy of the receiver with the status updated to +// ObjectTainted. +func (o *ResourceInstanceObject) AsTainted() *ResourceInstanceObject { + if o == nil { + // A nil object can't be tainted, but we'll allow this anyway to + // avoid a crash, since we presumably intend to eventually record + // the object has having been deleted anyway. + return nil + } + ret := o.DeepCopy() + ret.Status = ObjectTainted + return ret +} diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index edb3a7e74..77b3d1911 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -171,7 +171,7 @@ func (n *EvalApply) Eval(ctx EvalContext) (interface{}, error) { var newState *states.ResourceInstanceObject if !newVal.IsNull() { // null value indicates that the object is deleted, so we won't set a new state in that case newState = &states.ResourceInstanceObject{ - Status: states.ObjectReady, // TODO: Consider marking as tainted if the provider returned errors? + Status: states.ObjectReady, Value: newVal, Private: resp.Private, Dependencies: n.Dependencies, // Should be populated by the caller from the StateDependencies method on the resource instance node @@ -228,10 +228,10 @@ func (n *EvalApplyPre) Eval(ctx EvalContext) (interface{}, error) { // EvalApplyPost is an EvalNode implementation that does the post-Apply work type EvalApplyPost struct { - Addr addrs.ResourceInstance - Gen states.Generation - State **states.ResourceInstanceObject - Error *error + Addr addrs.ResourceInstance + Gen states.Generation + State **states.ResourceInstanceObject + Error *error } // TODO: test @@ -262,6 +262,57 @@ func (n *EvalApplyPost) Eval(ctx EvalContext) (interface{}, error) { return nil, *n.Error } +// EvalMaybeTainted is an EvalNode that takes the planned change, new value, +// and possible error from an apply operation and produces a new instance +// object marked as tainted if it appears that a create operation has failed. +// +// This EvalNode never returns an error, to ensure that a subsequent EvalNode +// can still record the possibly-tainted object in the state. +type EvalMaybeTainted struct { + Addr addrs.ResourceInstance + Gen states.Generation + Change **plans.ResourceInstanceChange + State **states.ResourceInstanceObject + Error *error + + // If StateOutput is not nil, its referent will be assigned either the same + // pointer as State or a new object with its status set as Tainted, + // depending on whether an error is given and if this was a create action. + StateOutput **states.ResourceInstanceObject +} + +// TODO: test +func (n *EvalMaybeTainted) Eval(ctx EvalContext) (interface{}, error) { + state := *n.State + change := *n.Change + err := *n.Error + + if state != nil && state.Status == states.ObjectTainted { + log.Printf("[TRACE] EvalMaybeTainted: %s was already tainted, so nothing to do", n.Addr.Absolute(ctx.Path())) + return nil, nil + } + + if n.StateOutput != nil { + if err != nil && change.Action == plans.Create { + // If there are errors during a _create_ then the object is + // in an undefined state, and so we'll mark it as tainted so + // we can try again on the next run. + // + // We don't do this for other change actions because errors + // during updates will often not change the remote object at all. + // If there _were_ changes prior to the error, it's the provider's + // responsibility to record the effect of those changes in the + // object value it returned. + log.Printf("[TRACE] EvalMaybeTainted: %s encountered an error during creation, so it is now marked as tainted", n.Addr.Absolute(ctx.Path())) + *n.StateOutput = state.AsTainted() + } else { + *n.StateOutput = state + } + } + + return nil, nil +} + // resourceHasUserVisibleApply returns true if the given resource is one where // apply actions should be exposed to the user. // @@ -300,9 +351,15 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) { log.Printf("[TRACE] EvalApplyProvisioners: %s has no state, so skipping provisioners", n.Addr) return nil, nil } - if n.CreateNew != nil && !*n.CreateNew { // If we're not creating a new resource, then don't run provisioners + log.Printf("[TRACE] EvalApplyProvisioners: %s is not freshly-created, so no provisioning is required", n.Addr) + return nil, nil + } + if state.Status == states.ObjectTainted { + // No point in provisioning an object that is already tainted, since + // it's going to get recreated on the next apply anyway. + log.Printf("[TRACE] EvalApplyProvisioners: %s is tainted, so skipping provisioning", n.Addr) return nil, nil } @@ -312,14 +369,7 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } - // taint tells us whether to enable tainting. - taint := n.When == configs.ProvisionerWhenCreate - if n.Error != nil && *n.Error != nil { - if taint { - state.Status = states.ObjectTainted - } - // We're already tainted, so just return out return nil, nil } @@ -338,12 +388,13 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) { // if we have one, otherwise we just output it. err := n.apply(ctx, provs) if err != nil { - if taint { - state.Status = states.ObjectTainted - } - *n.Error = multierror.Append(*n.Error, err) - return nil, err + if n.Error == nil { + return nil, err + } else { + log.Printf("[TRACE] EvalApplyProvisioners: %s provisioning failed, but we will continue anyway at the caller's request", absAddr) + return nil, nil + } } { diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index af77eee41..e5604405d 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -342,6 +342,13 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe Error: &err, CreateNew: &createNew, }, + &EvalMaybeTainted{ + Addr: addr.Resource, + State: &state, + Change: &diffApply, + Error: &err, + StateOutput: &state, + }, &EvalWriteState{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider, @@ -350,12 +357,19 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe }, &EvalApplyProvisioners{ Addr: addr.Resource, - State: &state, + State: &state, // EvalApplyProvisioners will skip if already tainted ResourceConfig: n.Config, CreateNew: &createNew, Error: &err, When: configs.ProvisionerWhenCreate, }, + &EvalMaybeTainted{ + Addr: addr.Resource, + State: &state, + Change: &diffApply, + Error: &err, + StateOutput: &state, + }, &EvalWriteState{ Addr: addr.Resource, ProviderAddr: n.ResolvedProvider,