From faddb83a9292ecb5030f5691bb753ed1a86752f1 Mon Sep 17 00:00:00 2001 From: Martin Atkins Date: Thu, 20 Sep 2018 18:28:27 -0700 Subject: [PATCH] core: If create leg of create_before_destroy fails, restore deposed I misunderstood the logic here on the first pass of porting to the new provider and state types: EvalUndeposeState is supposed to return the deposed object back to being current again, so we can undo the deposing in the case where the create leg fails. If we don't do this, we end up leaving the instance with no current object at all and with its prior object deposed, and then the later destroy node deletes that deposed object, leaving the user with no object at all. For safety we skip this restoration if there _is_ a new current object, since a failed create can still produce a partial result which we need to keep to avoid losing track of any remote objects that were successfully created. --- states/module.go | 22 +++++++++++++++ states/sync.go | 24 ++++++++++++++++ terraform/context_apply_test.go | 2 +- terraform/eval_state.go | 34 +++++++++++++++++------ terraform/node_resource_apply_instance.go | 2 +- terraform/provider_mock.go | 24 ++++++++-------- 6 files changed, 86 insertions(+), 22 deletions(-) diff --git a/states/module.go b/states/module.go index ea5093754..2938a087f 100644 --- a/states/module.go +++ b/states/module.go @@ -181,6 +181,28 @@ func (ms *Module) deposeResourceInstanceObject(addr addrs.ResourceInstance, forc return is.deposeCurrentObject(forceKey) } +// maybeRestoreResourceInstanceDeposed is the real implementation of +// SyncState.MaybeRestoreResourceInstanceDeposed. +func (ms *Module) maybeRestoreResourceInstanceDeposed(addr addrs.ResourceInstance, key DeposedKey) bool { + rs := ms.Resource(addr.Resource) + if rs == nil { + return false + } + is := rs.Instance(addr.Key) + if is == nil { + return false + } + if is.Current != nil { + return false + } + if len(is.Deposed) == 0 { + return false + } + is.Current = is.Deposed[key] + delete(is.Deposed, key) + return true +} + // SetOutputValue writes an output value into the state, overwriting any // existing value of the same name. func (ms *Module) SetOutputValue(name string, value cty.Value, sensitive bool) *OutputValue { diff --git a/states/sync.go b/states/sync.go index 00f2fd317..20ef6d123 100644 --- a/states/sync.go +++ b/states/sync.go @@ -361,6 +361,30 @@ func (s *SyncState) ForgetResourceInstanceDeposed(addr addrs.AbsResourceInstance s.maybePruneModule(addr.Module) } +// MaybeRestoreResourceInstanceDeposed will restore the deposed object with the +// given key on the specified resource as the current object for that instance +// if and only if that would not cause us to forget an existing current +// object for that instance. +// +// Returns true if the object was restored to current, or false if no change +// was made at all. +func (s *SyncState) MaybeRestoreResourceInstanceDeposed(addr addrs.AbsResourceInstance, key DeposedKey) bool { + s.lock.Lock() + defer s.lock.Unlock() + + if key == NotDeposed { + panic("MaybeRestoreResourceInstanceDeposed called without DeposedKey") + } + + ms := s.state.Module(addr.Module) + if ms == nil { + // Nothing to do, since the specified deposed object cannot exist. + return false + } + + return ms.maybeRestoreResourceInstanceDeposed(addr.Resource, key) +} + // RemovePlannedResourceInstanceObjects removes from the state any resource // instance objects that have the status ObjectPlanned, indiciating that they // are just transient placeholders created during planning. diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 7e957f2a2..334c285b0 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -4600,7 +4600,7 @@ func TestContext2Apply_error_createBeforeDestroy(t *testing.T) { actual := strings.TrimSpace(state.String()) expected := strings.TrimSpace(testTerraformApplyErrorCreateBeforeDestroyStr) if actual != expected { - t.Fatalf("bad: \n%s\n\nExpected:\n\n%s", actual, expected) + t.Fatalf("wrong final state\ngot:\n%s\n\nwant:\n%s", actual, expected) } } diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 1573f3d38..5c08fb16a 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -343,13 +343,24 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } -// EvalUndeposeState is an EvalNode implementation that forgets a particular -// deposed object from the state, causing Terraform to no longer track it. +// EvalMaybeRestoreDeposedObject is an EvalNode implementation that will +// restore a particular deposed object of the specified resource instance +// to be the "current" object if and only if the instance doesn't currently +// have a current object. // -// Users of this must ensure that the upstream object that the object was -// tracking has been deleted in the remote system before this node is -// evaluated. -type EvalUndeposeState struct { +// This is intended for use when the create leg of a create before destroy +// fails with no partial new object: if we didn't take any action, the user +// would be left in the unfortunate situation of having no current object +// and the previously-workign object now deposed. This EvalNode causes a +// better outcome by restoring things to how they were before the replace +// operation began. +// +// The create operation may have produced a partial result even though it +// failed and it's important that we don't "forget" that state, so in that +// situation the prior object remains deposed and the partial new object +// remains the current object, allowing the situation to hopefully be +// improved in a subsequent run. +type EvalMaybeRestoreDeposedObject struct { Addr addrs.ResourceInstance // Key is a pointer to the deposed object key that should be forgotten @@ -358,12 +369,17 @@ type EvalUndeposeState struct { } // TODO: test -func (n *EvalUndeposeState) Eval(ctx EvalContext) (interface{}, error) { +func (n *EvalMaybeRestoreDeposedObject) Eval(ctx EvalContext) (interface{}, error) { absAddr := n.Addr.Absolute(ctx.Path()) + dk := *n.Key state := ctx.State() - state.ForgetResourceInstanceDeposed(absAddr, *n.Key) - log.Printf("[TRACE] EvalDeposeState: %s deposed object %s is forgotten", absAddr, *n.Key) + restored := state.MaybeRestoreResourceInstanceDeposed(absAddr, dk) + if restored { + log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s was restored as the current object", absAddr, dk) + } else { + log.Printf("[TRACE] EvalMaybeRestoreDeposedObject: %s deposed object %s remains deposed", absAddr, dk) + } return nil, nil } diff --git a/terraform/node_resource_apply_instance.go b/terraform/node_resource_apply_instance.go index 7b4123c23..2a66eef99 100644 --- a/terraform/node_resource_apply_instance.go +++ b/terraform/node_resource_apply_instance.go @@ -384,7 +384,7 @@ func (n *NodeApplyableResourceInstance) evalTreeManagedResource(addr addrs.AbsRe If: func(ctx EvalContext) (bool, error) { return createBeforeDestroyEnabled && err != nil, nil }, - Then: &EvalUndeposeState{ + Then: &EvalMaybeRestoreDeposedObject{ Addr: addr.Resource, Key: &deposedKey, }, diff --git a/terraform/provider_mock.go b/terraform/provider_mock.go index 13a6be484..b38478281 100644 --- a/terraform/provider_mock.go +++ b/terraform/provider_mock.go @@ -388,20 +388,22 @@ func (p *MockProvider) ApplyResourceChange(r providers.ApplyResourceChangeReques if err != nil { resp.Diagnostics = resp.Diagnostics.Append(err) } - var newVal cty.Value if newState != nil { - var err error - newVal, err = newState.AttrsAsObjectValue(schema.Block.ImpliedType()) - if err != nil { - resp.Diagnostics = resp.Diagnostics.Append(err) + var newVal cty.Value + if newState != nil { + var err error + newVal, err = newState.AttrsAsObjectValue(schema.Block.ImpliedType()) + if err != nil { + resp.Diagnostics = resp.Diagnostics.Append(err) + } + } else { + // If apply returned a nil new state then that's the old way to + // indicate that the object was destroyed. Our new interface calls + // for that to be signalled as a null value. + newVal = cty.NullVal(schema.Block.ImpliedType()) } - } else { - // If apply returned a nil new state then that's the old way to - // indicate that the object was destroyed. Our new interface calls - // for that to be signalled as a null value. - newVal = cty.NullVal(schema.Block.ImpliedType()) + resp.NewState = newVal } - resp.NewState = newVal return resp }