From 8560f50cbc5c6de09e019a3c9e49b6d74495a619 Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 21 Apr 2016 21:55:29 +0200 Subject: [PATCH 1/3] Change taint behaviour to act as a normal resource MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This means it’s shown correctly in a plan and takes into account any actions that are dependant on the tainted resource and, vice verse, any actions that the tainted resource depends on. So this changes the behaviour from saying this resource is tainted so just forget about it and make sure it gets deleted in the background, to saying I want that resource to be recreated (taking into account the existing resource and it’s place in the graph). --- command/format_plan.go | 9 +- command/format_state.go | 2 +- command/untaint.go | 15 +-- helper/schema/resource_data.go | 4 + helper/schema/schema.go | 8 ++ terraform/diff.go | 11 +- terraform/eval_diff.go | 37 +----- terraform/eval_state.go | 56 +-------- terraform/graph_config_node_resource.go | 128 ++++++-------------- terraform/state.go | 121 +++---------------- terraform/transform_destroy.go | 48 ++------ terraform/transform_orphan.go | 12 +- terraform/transform_resource.go | 41 +------ terraform/transform_tainted.go | 154 ------------------------ terraform/transform_tainted_test.go | 71 ----------- 15 files changed, 106 insertions(+), 611 deletions(-) delete mode 100644 terraform/transform_tainted.go delete mode 100644 terraform/transform_tainted_test.go diff --git a/command/format_plan.go b/command/format_plan.go index 7846f0e8c..9ef62b4bf 100644 --- a/command/format_plan.go +++ b/command/format_plan.go @@ -112,9 +112,14 @@ func formatPlanModuleExpand( symbol = "-" } + taintStr := "" + if rdiff.DestroyTainted { + taintStr = " (tainted)" + } + buf.WriteString(opts.Color.Color(fmt.Sprintf( - "[%s]%s %s\n", - color, symbol, name))) + "[%s]%s %s%s\n", + color, symbol, name, taintStr))) // Get all the attributes that are changing, and sort them. Also // determine the longest key so that we can align them all. diff --git a/command/format_state.go b/command/format_state.go index 765fb9d7a..ccfd6573d 100644 --- a/command/format_state.go +++ b/command/format_state.go @@ -101,7 +101,7 @@ func formatStateModuleExpand( } taintStr := "" - if len(rs.Tainted) > 0 { + if rs.Primary.Tainted { taintStr = " (tainted)" } diff --git a/command/untaint.go b/command/untaint.go index b34b9a2fd..099c94859 100644 --- a/command/untaint.go +++ b/command/untaint.go @@ -17,14 +17,12 @@ func (c *UntaintCommand) Run(args []string) int { var allowMissing bool var module string - var index int cmdFlags := c.Meta.flagSet("untaint") cmdFlags.BoolVar(&allowMissing, "allow-missing", false, "module") cmdFlags.StringVar(&module, "module", "", "module") cmdFlags.StringVar(&c.Meta.statePath, "state", DefaultStateFilename, "path") cmdFlags.StringVar(&c.Meta.stateOutPath, "state-out", "", "path") cmdFlags.StringVar(&c.Meta.backupPath, "backup", "", "path") - cmdFlags.IntVar(&index, "index", -1, "index") cmdFlags.Usage = func() { c.Ui.Error(c.Help()) } if err := cmdFlags.Parse(args); err != nil { return 1 @@ -108,11 +106,7 @@ func (c *UntaintCommand) Run(args []string) int { } // Untaint the resource - if err := rs.Untaint(index); err != nil { - c.Ui.Error(fmt.Sprintf("Error untainting %s: %s", name, err)) - c.Ui.Error("You can use `terraform show` to inspect the current state.") - return 1 - } + rs.Untaint() log.Printf("[INFO] Writing state output to: %s", c.Meta.StateOutPath()) if err := c.Meta.PersistState(s); err != nil { @@ -148,13 +142,6 @@ Options: modifying. Defaults to the "-state-out" path with ".backup" extension. Set to "-" to disable backup. - -index=n Selects a single tainted instance when there are more - than one tainted instances present in the state for a - given resource. This flag is required when multiple - tainted instances are present. The vast majority of the - time, there is a maxiumum of one tainted instance per - resource, so this flag can be safely omitted. - -module=path The module path where the resource lives. By default this will be root. Child modules can be specified by names. Ex. "consul" or "consul.vpc" (nested modules). diff --git a/helper/schema/resource_data.go b/helper/schema/resource_data.go index 36599aebb..500de0a3d 100644 --- a/helper/schema/resource_data.go +++ b/helper/schema/resource_data.go @@ -290,6 +290,10 @@ func (d *ResourceData) State() *terraform.InstanceState { result.Attributes["id"] = d.Id() } + if d.state != nil { + result.Tainted = d.state.Tainted + } + return &result } diff --git a/helper/schema/schema.go b/helper/schema/schema.go index deed582c1..a113273f3 100644 --- a/helper/schema/schema.go +++ b/helper/schema/schema.go @@ -308,6 +308,11 @@ func (m schemaMap) Diff( result := new(terraform.InstanceDiff) result.Attributes = make(map[string]*terraform.ResourceAttrDiff) + // Make sure to mark if the resource is tainted + if s != nil { + result.DestroyTainted = s.Tainted + } + d := &ResourceData{ schema: m, state: s, @@ -330,6 +335,9 @@ func (m schemaMap) Diff( result2 := new(terraform.InstanceDiff) result2.Attributes = make(map[string]*terraform.ResourceAttrDiff) + // Preserve the DestroyTainted flag + result2.DestroyTainted = result.DestroyTainted + // Reset the data to not contain state. We have to call init() // again in order to reset the FieldReaders. d.state = nil diff --git a/terraform/diff.go b/terraform/diff.go index c44c06c18..73d56f823 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -215,11 +215,12 @@ func (d *ModuleDiff) String() string { rdiff := d.Resources[name] crud := "UPDATE" - if rdiff.RequiresNew() && (rdiff.Destroy || rdiff.DestroyTainted) { + switch { + case rdiff.RequiresNew() && (rdiff.Destroy || rdiff.DestroyTainted): crud = "DESTROY/CREATE" - } else if rdiff.Destroy { + case rdiff.Destroy: crud = "DESTROY" - } else if rdiff.RequiresNew() { + case rdiff.RequiresNew(): crud = "CREATE" } @@ -356,6 +357,10 @@ func (d *InstanceDiff) RequiresNew() bool { return false } + if d.DestroyTainted { + return true + } + for _, rd := range d.Attributes { if rd != nil && rd.RequiresNew { return true diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index ea6b124c3..10deacb26 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -104,7 +104,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { diff = new(InstanceDiff) } - // Require a destroy if there is no ID and it requires new. + // Require a destroy if there is an ID and it requires new. if diff.RequiresNew() && state != nil && state.ID != "" { diff.Destroy = true } @@ -216,41 +216,6 @@ func (n *EvalDiffDestroyModule) Eval(ctx EvalContext) (interface{}, error) { return nil, nil } -// EvalDiffTainted is an EvalNode implementation that writes the diff to -// the full diff. -type EvalDiffTainted struct { - Name string - Diff **InstanceDiff -} - -// TODO: test -func (n *EvalDiffTainted) Eval(ctx EvalContext) (interface{}, error) { - state, lock := ctx.State() - - // Get a read lock so we can access this instance - lock.RLock() - defer lock.RUnlock() - - // Look for the module state. If we don't have one, then it doesn't matter. - mod := state.ModuleByPath(ctx.Path()) - if mod == nil { - return nil, nil - } - - // Look for the resource state. If we don't have one, then it is okay. - rs := mod.Resources[n.Name] - if rs == nil { - return nil, nil - } - - // If we have tainted, then mark it on the diff - if len(rs.Tainted) > 0 { - (*n.Diff).DestroyTainted = true - } - - return nil, nil -} - // EvalFilterDiff is an EvalNode implementation that filters the diff // according to some filter. type EvalFilterDiff struct { diff --git a/terraform/eval_state.go b/terraform/eval_state.go index 3a3e9efd6..aa1d1e28e 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -1,8 +1,6 @@ package terraform -import ( - "fmt" -) +import "fmt" // EvalReadState is an EvalNode implementation that reads the // primary InstanceState for a specific resource out of the state. @@ -17,31 +15,6 @@ func (n *EvalReadState) Eval(ctx EvalContext) (interface{}, error) { }) } -// EvalReadStateTainted is an EvalNode implementation that reads a -// tainted InstanceState for a specific resource out of the state -type EvalReadStateTainted struct { - Name string - Output **InstanceState - // Index indicates which instance in the Tainted list to target, or -1 for - // the last item. - Index int -} - -func (n *EvalReadStateTainted) Eval(ctx EvalContext) (interface{}, error) { - return readInstanceFromState(ctx, n.Name, n.Output, func(rs *ResourceState) (*InstanceState, error) { - // Get the index. If it is negative, then we get the last one - idx := n.Index - if idx < 0 { - idx = len(rs.Tainted) - 1 - } - if idx >= 0 && idx < len(rs.Tainted) { - return rs.Tainted[idx], nil - } else { - return nil, fmt.Errorf("bad tainted index: %d, for resource: %#v", idx, rs) - } - }) -} - // EvalReadStateDeposed is an EvalNode implementation that reads the // deposed InstanceState for a specific resource out of the state type EvalReadStateDeposed struct { @@ -168,33 +141,6 @@ func (n *EvalWriteState) Eval(ctx EvalContext) (interface{}, error) { ) } -// EvalWriteStateTainted is an EvalNode implementation that writes -// an InstanceState out to the Tainted list of a resource in the state. -type EvalWriteStateTainted struct { - Name string - ResourceType string - Provider string - Dependencies []string - State **InstanceState - // Index indicates which instance in the Tainted list to target, or -1 to append. - Index int -} - -// EvalWriteStateTainted is an EvalNode implementation that writes the -// one of the tainted InstanceStates for a specific resource out of the state. -func (n *EvalWriteStateTainted) Eval(ctx EvalContext) (interface{}, error) { - return writeInstanceToState(ctx, n.Name, n.ResourceType, n.Provider, n.Dependencies, - func(rs *ResourceState) error { - if n.Index == -1 { - rs.Tainted = append(rs.Tainted, *n.State) - } else { - rs.Tainted[n.Index] = *n.State - } - return nil - }, - ) -} - // EvalWriteStateDeposed is an EvalNode implementation that writes // an InstanceState out to the Deposed list of a resource in the state. type EvalWriteStateDeposed struct { diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index a2c688da4..dbe5a0ecd 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -20,9 +20,9 @@ type GraphNodeCountDependent interface { type GraphNodeConfigResource struct { Resource *config.Resource - // If this is set to anything other than destroyModeNone, then this - // resource represents a resource that will be destroyed in some way. - DestroyMode GraphNodeDestroyMode + // If set to true, this resource represents a resource + // that will be destroyed in some way. + Destroy bool // Used during DynamicExpand to target indexes Targets []ResourceAddress @@ -32,10 +32,10 @@ type GraphNodeConfigResource struct { func (n *GraphNodeConfigResource) Copy() *GraphNodeConfigResource { ncr := &GraphNodeConfigResource{ - Resource: n.Resource.Copy(), - DestroyMode: n.DestroyMode, - Targets: make([]ResourceAddress, 0, len(n.Targets)), - Path: make([]string, 0, len(n.Path)), + Resource: n.Resource.Copy(), + Destroy: n.Destroy, + Targets: make([]ResourceAddress, 0, len(n.Targets)), + Path: make([]string, 0, len(n.Path)), } for _, t := range n.Targets { ncr.Targets = append(ncr.Targets, *t.Copy()) @@ -120,23 +120,12 @@ func (n *GraphNodeConfigResource) VarWalk(fn func(config.InterpolatedVariable)) } func (n *GraphNodeConfigResource) Name() string { - result := n.Resource.Id() - switch n.DestroyMode { - case DestroyNone: - case DestroyPrimary: - result += " (destroy)" - case DestroyTainted: - result += " (destroy tainted)" - default: - result += " (unknown destroy type)" - } - - return result + return n.Resource.Id() + " (destroy)" } // GraphNodeDotter impl. func (n *GraphNodeConfigResource) DotNode(name string, opts *GraphDotOpts) *dot.Node { - if n.DestroyMode != DestroyNone && !opts.Verbose { + if n.Destroy && !opts.Verbose { return nil } return dot.NewNode(name, map[string]string{ @@ -162,23 +151,18 @@ func (n *GraphNodeConfigResource) DynamicExpand(ctx EvalContext) (*Graph, error) // Start creating the steps steps := make([]GraphTransformer, 0, 5) - // Primary and non-destroy modes are responsible for creating/destroying - // all the nodes, expanding counts. - switch n.DestroyMode { - case DestroyNone, DestroyPrimary: - steps = append(steps, &ResourceCountTransformer{ - Resource: n.Resource, - Destroy: n.DestroyMode != DestroyNone, - Targets: n.Targets, - }) - } + // Expand counts. + steps = append(steps, &ResourceCountTransformer{ + Resource: n.Resource, + Destroy: n.Destroy, + Targets: n.Targets, + }) // Additional destroy modifications. - switch n.DestroyMode { - case DestroyPrimary: - // If we're destroying the primary instance, then we want to + if n.Destroy { + // If we're destroying a primary or tainted resource, we want to // expand orphans, which have all the same semantics in a destroy - // as a primary. + // as a primary or tainted resource. steps = append(steps, &OrphanTransformer{ State: state, View: n.Resource.Id(), @@ -188,19 +172,12 @@ func (n *GraphNodeConfigResource) DynamicExpand(ctx EvalContext) (*Graph, error) State: state, View: n.Resource.Id(), }) - case DestroyTainted: - // If we're only destroying tainted resources, then we only - // want to find tainted resources and destroy them here. - steps = append(steps, &TaintedTransformer{ - State: state, - View: n.Resource.Id(), - }) } // We always want to apply targeting steps = append(steps, &TargetsTransformer{ ParsedTargets: n.Targets, - Destroy: n.DestroyMode != DestroyNone, + Destroy: n.Destroy, }) // Always end with the root being added @@ -258,9 +235,9 @@ func (n *GraphNodeConfigResource) ProvisionedBy() []string { } // GraphNodeDestroyable -func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNodeDestroy { +func (n *GraphNodeConfigResource) DestroyNode() GraphNodeDestroy { // If we're already a destroy node, then don't do anything - if n.DestroyMode != DestroyNone { + if n.Destroy { return nil } @@ -268,7 +245,8 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo GraphNodeConfigResource: *n.Copy(), Original: n, } - result.DestroyMode = mode + result.Destroy = true + return result } @@ -276,7 +254,7 @@ func (n *GraphNodeConfigResource) DestroyNode(mode GraphNodeDestroyMode) GraphNo func (n *GraphNodeConfigResource) Noop(opts *NoopOpts) bool { log.Printf("[DEBUG] Checking resource noop: %s", n.Name()) // We don't have any noop optimizations for destroy nodes yet - if n.DestroyMode != DestroyNone { + if n.Destroy { log.Printf("[DEBUG] Destroy node, not a noop") return false } @@ -365,9 +343,9 @@ func (n *GraphNodeConfigResourceFlat) ProvisionedBy() []string { } // GraphNodeDestroyable impl. -func (n *GraphNodeConfigResourceFlat) DestroyNode(mode GraphNodeDestroyMode) GraphNodeDestroy { +func (n *GraphNodeConfigResourceFlat) DestroyNode() GraphNodeDestroy { // Get our parent destroy node. If we don't have any, just return - raw := n.GraphNodeConfigResource.DestroyNode(mode) + raw := n.GraphNodeConfigResource.DestroyNode() if raw == nil { return nil } @@ -423,13 +401,8 @@ type graphNodeResourceDestroy struct { } func (n *graphNodeResourceDestroy) CreateBeforeDestroy() bool { - // CBD is enabled if the resource enables it in addition to us - // being responsible for destroying the primary state. The primary - // state destroy node is the only destroy node that needs to be - // "shuffled" according to the CBD rules, since tainted resources - // don't have the same inverse dependencies. - return n.Original.Resource.Lifecycle.CreateBeforeDestroy && - n.DestroyMode == DestroyPrimary + // CBD is enabled if the resource enables it + return n.Original.Resource.Lifecycle.CreateBeforeDestroy && n.Destroy } func (n *graphNodeResourceDestroy) CreateNode() dag.Vertex { @@ -437,43 +410,14 @@ func (n *graphNodeResourceDestroy) CreateNode() dag.Vertex { } func (n *graphNodeResourceDestroy) DestroyInclude(d *ModuleDiff, s *ModuleState) bool { - switch n.DestroyMode { - case DestroyPrimary: - return n.destroyIncludePrimary(d, s) - case DestroyTainted: - return n.destroyIncludeTainted(d, s) - default: - return true + if n.Destroy { + return n.destroyInclude(d, s) } + + return true } -func (n *graphNodeResourceDestroy) destroyIncludeTainted( - d *ModuleDiff, s *ModuleState) bool { - // If there is no state, there can't by any tainted. - if s == nil { - return false - } - - // Grab the ID which is the prefix (in the case count > 0 at some point) - prefix := n.Original.Resource.Id() - - // Go through the resources and find any with our prefix. If there - // are any tainted, we need to keep it. - for k, v := range s.Resources { - if !strings.HasPrefix(k, prefix) { - continue - } - - if len(v.Tainted) > 0 { - return true - } - } - - // We didn't find any tainted nodes, return - return false -} - -func (n *graphNodeResourceDestroy) destroyIncludePrimary( +func (n *graphNodeResourceDestroy) destroyInclude( d *ModuleDiff, s *ModuleState) bool { // Get the count, and specifically the raw value of the count // (with interpolations and all). If the count is NOT a static "1", @@ -516,7 +460,7 @@ func (n *graphNodeResourceDestroy) destroyIncludePrimary( // If the count is set to ANYTHING other than a static "1" (variable, // computed attribute, static number greater than 1), then we keep the // destroy, since it is required for dynamic graph expansion to find - // orphan/tainted count objects. + // orphan count objects. // // This isn't ideal logic, but its strictly better without introducing // new impossibilities. It breaks the cycle in practical cases, and the @@ -535,9 +479,9 @@ func (n *graphNodeResourceDestroy) destroyIncludePrimary( // only for resources in the diff that match our resource or a count-index // of our resource that are marked for destroy. if d != nil { - for k, d := range d.Resources { + for k, v := range d.Resources { match := k == prefix || strings.HasPrefix(k, prefix+".") - if match && d.Destroy { + if match && v.Destroy { return true } } diff --git a/terraform/state.go b/terraform/state.go index 12fb901e4..19b781b55 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -780,7 +780,7 @@ func (m *ModuleState) prune() { for k, v := range m.Resources { v.prune() - if (v.Primary == nil || v.Primary.ID == "") && len(v.Tainted) == 0 && len(v.Deposed) == 0 { + if (v.Primary == nil || v.Primary.ID == "") && len(v.Deposed) == 0 { delete(m.Resources, k) } } @@ -826,8 +826,8 @@ func (m *ModuleState) String() string { } taintStr := "" - if len(rs.Tainted) > 0 { - taintStr = fmt.Sprintf(" (%d tainted)", len(rs.Tainted)) + if rs.Primary.Tainted { + taintStr = " (tainted)" } deposedStr := "" @@ -860,10 +860,6 @@ func (m *ModuleState) String() string { buf.WriteString(fmt.Sprintf(" %s = %s\n", ak, av)) } - for idx, t := range rs.Tainted { - buf.WriteString(fmt.Sprintf(" Tainted ID %d = %s\n", idx+1, t.ID)) - } - for idx, t := range rs.Deposed { buf.WriteString(fmt.Sprintf(" Deposed ID %d = %s\n", idx+1, t.ID)) } @@ -1031,14 +1027,6 @@ type ResourceState struct { // This is the instances on which providers will act. Primary *InstanceState `json:"primary"` - // Tainted is used to track any underlying instances that - // have been created but are in a bad or unknown state and - // need to be cleaned up subsequently. In the - // standard case, there is only at most a single instance. - // However, in pathological cases, it is possible for the number - // of instances to accumulate. - Tainted []*InstanceState `json:"tainted,omitempty"` - // Deposed is used in the mechanics of CreateBeforeDestroy: the existing // Primary is Deposed to get it out of the way for the replacement Primary to // be created by Apply. If the replacement Primary creates successfully, the @@ -1084,80 +1072,21 @@ func (s *ResourceState) Equal(other *ResourceState) bool { } // Tainted - taints := make(map[string]*InstanceState) - for _, t := range other.Tainted { - if t == nil { - continue - } - - taints[t.ID] = t - } - for _, t := range s.Tainted { - if t == nil { - continue - } - - otherT, ok := taints[t.ID] - if !ok { - return false - } - delete(taints, t.ID) - - if !t.Equal(otherT) { - return false - } - } - - // This means that we have stuff in other tainted that we don't - // have, so it is not equal. - if len(taints) > 0 { + if s.Primary.Tainted != other.Primary.Tainted { return false } return true } -// Taint takes the primary state and marks it as tainted. If there is no -// primary state, this does nothing. +// Taint marks a resource as tainted. func (r *ResourceState) Taint() { - // If there is no primary, nothing to do - if r.Primary == nil { - return - } - - // Shuffle to the end of the taint list and set primary to nil - r.Tainted = append(r.Tainted, r.Primary) - r.Primary = nil + r.Primary.Tainted = true } -// Untaint takes a tainted InstanceState and marks it as primary. -// The index argument is used to select a single InstanceState from the -// array of Tainted when there are more than one. If index is -1, the -// first Tainted InstanceState will be untainted iff there is only one -// Tainted InstanceState. Index must be >= 0 to specify an InstanceState -// when Tainted has more than one member. -func (r *ResourceState) Untaint(index int) error { - if len(r.Tainted) == 0 { - return fmt.Errorf("Nothing to untaint.") - } - if r.Primary != nil { - return fmt.Errorf("Resource has a primary instance in the state that would be overwritten by untainting. If you want to restore a tainted resource to primary, taint the existing primary instance first.") - } - if index == -1 && len(r.Tainted) > 1 { - return fmt.Errorf("There are %d tainted instances for this resource, please specify an index to select which one to untaint.", len(r.Tainted)) - } - if index == -1 { - index = 0 - } - if index >= len(r.Tainted) { - return fmt.Errorf("There are %d tainted instances for this resource, the index specified (%d) is out of range.", len(r.Tainted), index) - } - - // Perform the untaint - r.Primary = r.Tainted[index] - r.Tainted = append(r.Tainted[:index], r.Tainted[index+1:]...) - - return nil +// Untaint unmarks a resource as tainted. +func (r *ResourceState) Untaint() { + r.Primary.Tainted = false } func (r *ResourceState) init() { @@ -1176,19 +1105,12 @@ func (r *ResourceState) deepcopy() *ResourceState { Type: r.Type, Dependencies: nil, Primary: r.Primary.DeepCopy(), - Tainted: nil, Provider: r.Provider, } if r.Dependencies != nil { n.Dependencies = make([]string, len(r.Dependencies)) copy(n.Dependencies, r.Dependencies) } - if r.Tainted != nil { - n.Tainted = make([]*InstanceState, 0, len(r.Tainted)) - for _, inst := range r.Tainted { - n.Tainted = append(n.Tainted, inst.DeepCopy()) - } - } if r.Deposed != nil { n.Deposed = make([]*InstanceState, 0, len(r.Deposed)) for _, inst := range r.Deposed { @@ -1201,20 +1123,7 @@ func (r *ResourceState) deepcopy() *ResourceState { // prune is used to remove any instances that are no longer required func (r *ResourceState) prune() { - n := len(r.Tainted) - for i := 0; i < n; i++ { - inst := r.Tainted[i] - if inst == nil || inst.ID == "" { - copy(r.Tainted[i:], r.Tainted[i+1:]) - r.Tainted[n-1] = nil - n-- - i-- - } - } - - r.Tainted = r.Tainted[:n] - - n = len(r.Deposed) + n := len(r.Deposed) for i := 0; i < n; i++ { inst := r.Deposed[i] if inst == nil || inst.ID == "" { @@ -1263,6 +1172,9 @@ type InstanceState struct { // ignored by Terraform core. It's meant to be used for accounting by // external client code. Meta map[string]string `json:"meta,omitempty"` + + // Tainted is used to mark a resource for recreation. + Tainted bool `json:"tainted,omitempty"` } func (i *InstanceState) init() { @@ -1282,6 +1194,7 @@ func (i *InstanceState) DeepCopy() *InstanceState { n := &InstanceState{ ID: i.ID, Ephemeral: *i.Ephemeral.DeepCopy(), + Tainted: i.Tainted, } if i.Attributes != nil { n.Attributes = make(map[string]string, len(i.Attributes)) @@ -1343,6 +1256,10 @@ func (s *InstanceState) Equal(other *InstanceState) bool { } } + if s.Tainted != other.Tainted { + return false + } + return true } @@ -1413,6 +1330,8 @@ func (i *InstanceState) String() string { buf.WriteString(fmt.Sprintf("%s = %s\n", ak, av)) } + buf.WriteString(fmt.Sprintf("Tainted = %t\n", i.Tainted)) + return buf.String() } diff --git a/terraform/transform_destroy.go b/terraform/transform_destroy.go index 8d5aeb4c9..af8ccc4ab 100644 --- a/terraform/transform_destroy.go +++ b/terraform/transform_destroy.go @@ -4,14 +4,6 @@ import ( "github.com/hashicorp/terraform/dag" ) -type GraphNodeDestroyMode byte - -const ( - DestroyNone GraphNodeDestroyMode = 0 - DestroyPrimary GraphNodeDestroyMode = 1 << iota - DestroyTainted -) - // GraphNodeDestroyable is the interface that nodes that can be destroyed // must implement. This is used to automatically handle the creation of // destroy nodes in the graph and the dependency ordering of those destroys. @@ -19,7 +11,7 @@ type GraphNodeDestroyable interface { // DestroyNode returns the node used for the destroy with the given // mode. If this returns nil, then a destroy node for that mode // will not be added. - DestroyNode(GraphNodeDestroyMode) GraphNodeDestroy + DestroyNode() GraphNodeDestroy } // GraphNodeDestroy is the interface that must implemented by @@ -60,32 +52,6 @@ type DestroyTransformer struct { func (t *DestroyTransformer) Transform(g *Graph) error { var connect, remove []dag.Edge - - modes := []GraphNodeDestroyMode{DestroyPrimary, DestroyTainted} - for _, m := range modes { - connectMode, removeMode, err := t.transform(g, m) - if err != nil { - return err - } - - connect = append(connect, connectMode...) - remove = append(remove, removeMode...) - } - - // Atomatically add/remove the edges - for _, e := range connect { - g.Connect(e) - } - for _, e := range remove { - g.RemoveEdge(e) - } - - return nil -} - -func (t *DestroyTransformer) transform( - g *Graph, mode GraphNodeDestroyMode) ([]dag.Edge, []dag.Edge, error) { - var connect, remove []dag.Edge nodeToCn := make(map[dag.Vertex]dag.Vertex, len(g.Vertices())) nodeToDn := make(map[dag.Vertex]dag.Vertex, len(g.Vertices())) for _, v := range g.Vertices() { @@ -96,7 +62,7 @@ func (t *DestroyTransformer) transform( } // Grab the destroy side of the node and connect it through - n := cn.DestroyNode(mode) + n := cn.DestroyNode() if n == nil { continue } @@ -155,7 +121,15 @@ func (t *DestroyTransformer) transform( } } - return connect, remove, nil + // Atomatically add/remove the edges + for _, e := range connect { + g.Connect(e) + } + for _, e := range remove { + g.RemoveEdge(e) + } + + return nil } // CreateBeforeDestroyTransformer is a GraphTransformer that modifies diff --git a/terraform/transform_orphan.go b/terraform/transform_orphan.go index 540bd2cc6..f47f51681 100644 --- a/terraform/transform_orphan.go +++ b/terraform/transform_orphan.go @@ -368,11 +368,7 @@ func (n *graphNodeOrphanResource) dependableName() string { } // GraphNodeDestroyable impl. -func (n *graphNodeOrphanResource) DestroyNode(mode GraphNodeDestroyMode) GraphNodeDestroy { - if mode != DestroyPrimary { - return nil - } - +func (n *graphNodeOrphanResource) DestroyNode() GraphNodeDestroy { return n } @@ -402,11 +398,7 @@ func (n *graphNodeOrphanResourceFlat) Path() []string { } // GraphNodeDestroyable impl. -func (n *graphNodeOrphanResourceFlat) DestroyNode(mode GraphNodeDestroyMode) GraphNodeDestroy { - if mode != DestroyPrimary { - return nil - } - +func (n *graphNodeOrphanResourceFlat) DestroyNode() GraphNodeDestroy { return n } diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index dccc3cd94..07da19b77 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -355,10 +355,6 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Dependencies: n.StateDependencies(), State: &state, }, - &EvalDiffTainted{ - Diff: &diff, - Name: n.stateId(), - }, &EvalWriteDiff{ Name: n.stateId(), Diff: &diff, @@ -540,37 +536,12 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Diff: nil, }, - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - return tainted, nil - }, - Then: &EvalSequence{ - Nodes: []EvalNode{ - &EvalWriteStateTainted{ - Name: n.stateId(), - ResourceType: n.Resource.Type, - Provider: n.Resource.Provider, - Dependencies: n.StateDependencies(), - State: &state, - Index: -1, - }, - &EvalIf{ - If: func(ctx EvalContext) (bool, error) { - return !n.Resource.Lifecycle.CreateBeforeDestroy, nil - }, - Then: &EvalClearPrimaryState{ - Name: n.stateId(), - }, - }, - }, - }, - Else: &EvalWriteState{ - Name: n.stateId(), - ResourceType: n.Resource.Type, - Provider: n.Resource.Provider, - Dependencies: n.StateDependencies(), - State: &state, - }, + &EvalWriteState{ + Name: n.stateId(), + ResourceType: n.Resource.Type, + Provider: n.Resource.Provider, + Dependencies: n.StateDependencies(), + State: &state, }, &EvalApplyPost{ Info: info, diff --git a/terraform/transform_tainted.go b/terraform/transform_tainted.go deleted file mode 100644 index 37e25df32..000000000 --- a/terraform/transform_tainted.go +++ /dev/null @@ -1,154 +0,0 @@ -package terraform - -import ( - "fmt" -) - -// TaintedTransformer is a GraphTransformer that adds tainted resources -// to the graph. -type TaintedTransformer struct { - // State is the global state. We'll automatically find the correct - // ModuleState based on the Graph.Path that is being transformed. - State *State - - // View, if non-empty, is the ModuleState.View used around the state - // to find tainted resources. - View string -} - -func (t *TaintedTransformer) Transform(g *Graph) error { - state := t.State.ModuleByPath(g.Path) - if state == nil { - // If there is no state for our module there can't be any tainted - // resources, since they live in the state. - return nil - } - - // If we have a view, apply it now - if t.View != "" { - state = state.View(t.View) - } - - // Go through all the resources in our state to look for tainted resources - for k, rs := range state.Resources { - // If we have no tainted resources, then move on - if len(rs.Tainted) == 0 { - continue - } - tainted := rs.Tainted - - for i, _ := range tainted { - // Add the graph node and make the connection from any untainted - // resources with this name to the tainted resource, so that - // the tainted resource gets destroyed first. - g.Add(&graphNodeTaintedResource{ - Index: i, - ResourceName: k, - ResourceType: rs.Type, - Provider: rs.Provider, - }) - } - } - - return nil -} - -// graphNodeTaintedResource is the graph vertex representing a tainted resource. -type graphNodeTaintedResource struct { - Index int - ResourceName string - ResourceType string - Provider string -} - -func (n *graphNodeTaintedResource) Name() string { - return fmt.Sprintf("%s (tainted #%d)", n.ResourceName, n.Index+1) -} - -func (n *graphNodeTaintedResource) ProvidedBy() []string { - return []string{resourceProvider(n.ResourceName, n.Provider)} -} - -// GraphNodeEvalable impl. -func (n *graphNodeTaintedResource) EvalTree() EvalNode { - var provider ResourceProvider - var state *InstanceState - - seq := &EvalSequence{Nodes: make([]EvalNode, 0, 5)} - - // Build instance info - info := &InstanceInfo{Id: n.ResourceName, Type: n.ResourceType} - seq.Nodes = append(seq.Nodes, &EvalInstanceInfo{Info: info}) - - // Refresh the resource - seq.Nodes = append(seq.Nodes, &EvalOpFilter{ - Ops: []walkOperation{walkRefresh}, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalGetProvider{ - Name: n.ProvidedBy()[0], - Output: &provider, - }, - &EvalReadStateTainted{ - Name: n.ResourceName, - Index: n.Index, - Output: &state, - }, - &EvalRefresh{ - Info: info, - Provider: &provider, - State: &state, - Output: &state, - }, - &EvalWriteStateTainted{ - Name: n.ResourceName, - ResourceType: n.ResourceType, - Provider: n.Provider, - State: &state, - Index: n.Index, - }, - }, - }, - }) - - // Apply - var diff *InstanceDiff - seq.Nodes = append(seq.Nodes, &EvalOpFilter{ - Ops: []walkOperation{walkApply, walkDestroy}, - Node: &EvalSequence{ - Nodes: []EvalNode{ - &EvalGetProvider{ - Name: n.ProvidedBy()[0], - Output: &provider, - }, - &EvalReadStateTainted{ - Name: n.ResourceName, - Index: n.Index, - Output: &state, - }, - &EvalDiffDestroy{ - Info: info, - State: &state, - Output: &diff, - }, - &EvalApply{ - Info: info, - State: &state, - Diff: &diff, - Provider: &provider, - Output: &state, - }, - &EvalWriteStateTainted{ - Name: n.ResourceName, - ResourceType: n.ResourceType, - Provider: n.Provider, - State: &state, - Index: n.Index, - }, - &EvalUpdateStateHook{}, - }, - }, - }) - - return seq -} diff --git a/terraform/transform_tainted_test.go b/terraform/transform_tainted_test.go deleted file mode 100644 index 208165ea7..000000000 --- a/terraform/transform_tainted_test.go +++ /dev/null @@ -1,71 +0,0 @@ -package terraform - -import ( - "strings" - "testing" - - "github.com/hashicorp/terraform/dag" -) - -func TestTaintedTransformer(t *testing.T) { - mod := testModule(t, "transform-tainted-basic") - state := &State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: RootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.web": &ResourceState{ - Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, - }, - }, - }, - }, - }, - } - - g := Graph{Path: RootModulePath} - { - tf := &ConfigTransformer{Module: mod} - if err := tf.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - } - - transform := &TaintedTransformer{State: state} - if err := transform.Transform(&g); err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(g.String()) - expected := strings.TrimSpace(testTransformTaintedBasicStr) - if actual != expected { - t.Fatalf("bad:\n\n%s", actual) - } -} - -func TestGraphNodeTaintedResource_impl(t *testing.T) { - var _ dag.Vertex = new(graphNodeTaintedResource) - var _ dag.NamedVertex = new(graphNodeTaintedResource) - var _ GraphNodeProviderConsumer = new(graphNodeTaintedResource) -} - -func TestGraphNodeTaintedResource_ProvidedBy(t *testing.T) { - n := &graphNodeTaintedResource{ResourceName: "aws_instance.foo"} - if v := n.ProvidedBy(); v[0] != "aws" { - t.Fatalf("bad: %#v", v) - } -} - -func TestGraphNodeTaintedResource_ProvidedBy_alias(t *testing.T) { - n := &graphNodeTaintedResource{ResourceName: "aws_instance.foo", Provider: "aws.bar"} - if v := n.ProvidedBy(); v[0] != "aws.bar" { - t.Fatalf("bad: %#v", v) - } -} - -const testTransformTaintedBasicStr = ` -aws_instance.web -aws_instance.web (tainted #1) -` From d97b24e3c13e7c74bbe4f3be77e161bb91812cbf Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Thu, 21 Apr 2016 21:59:10 +0200 Subject: [PATCH 2/3] Add tests and fix last issues --- command/apply_test.go | 4 + command/plan_test.go | 2 + command/taint_test.go | 10 +- command/untaint_test.go | 155 +++++------------------- terraform/context_apply_test.go | 75 ++++++------ terraform/context_plan_test.go | 68 ++--------- terraform/context_refresh_test.go | 17 ++- terraform/context_test.go | 42 ++++--- terraform/context_validate_test.go | 7 +- terraform/eval_apply.go | 11 +- terraform/eval_diff.go | 10 +- terraform/eval_state.go | 5 +- terraform/eval_state_test.go | 41 ------- terraform/graph_builder_test.go | 5 - terraform/graph_config_node_resource.go | 6 +- terraform/resource.go | 2 - terraform/state.go | 34 +++--- terraform/state_filter.go | 13 -- terraform/state_test.go | 152 ++++++----------------- terraform/terraform_test.go | 19 ++- terraform/transform_destroy_test.go | 22 +--- terraform/transform_resource.go | 40 +++--- 22 files changed, 229 insertions(+), 511 deletions(-) diff --git a/command/apply_test.go b/command/apply_test.go index b847a769d..a01ff03a4 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -1361,16 +1361,20 @@ const applyVarFileJSON = ` const testApplyDisableBackupStr = ` ID = bar +Tainted = false ` const testApplyDisableBackupStateStr = ` ID = bar +Tainted = false ` const testApplyStateStr = ` ID = bar +Tainted = false ` const testApplyStateDiffStr = ` ID = bar +Tainted = false ` diff --git a/command/plan_test.go b/command/plan_test.go index e0bc6c41d..710d993a9 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -603,8 +603,10 @@ const testPlanNoStateStr = ` const testPlanStateStr = ` ID = bar +Tainted = false ` const testPlanStateDefaultStr = ` ID = bar +Tainted = false ` diff --git a/command/taint_test.go b/command/taint_test.go index 617555b00..c2ad652fa 100644 --- a/command/taint_test.go +++ b/command/taint_test.go @@ -347,9 +347,8 @@ func TestTaint_module(t *testing.T) { } const testTaintStr = ` -test_instance.foo: (1 tainted) - ID = - Tainted ID 1 = bar +test_instance.foo: (tainted) + ID = bar ` const testTaintDefaultStr = ` @@ -362,7 +361,6 @@ test_instance.foo: ID = bar module.child: - test_instance.blah: (1 tainted) - ID = - Tainted ID 1 = blah + test_instance.blah: (tainted) + ID = blah ` diff --git a/command/untaint_test.go b/command/untaint_test.go index 4627f9fa8..4923a6f7c 100644 --- a/command/untaint_test.go +++ b/command/untaint_test.go @@ -17,8 +17,9 @@ func TestUntaint(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -49,101 +50,6 @@ test_instance.foo: testStateOutput(t, statePath, expected) } -func TestUntaint_indexRequired(t *testing.T) { - state := &terraform.State{ - Modules: []*terraform.ModuleState{ - &terraform.ModuleState{ - Path: []string{"root"}, - Resources: map[string]*terraform.ResourceState{ - "test_instance.foo": &terraform.ResourceState{ - Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, - &terraform.InstanceState{ID: "bar2"}, - }, - }, - }, - }, - }, - } - statePath := testStateFile(t, state) - - ui := new(cli.MockUi) - c := &UntaintCommand{ - Meta: Meta{ - Ui: ui, - }, - } - - args := []string{ - "-state", statePath, - "test_instance.foo", - } - if code := c.Run(args); code == 0 { - t.Fatalf("Expected non-zero exit. Output:\n\n%s", ui.OutputWriter.String()) - } - - // Nothing should have gotten untainted - expected := strings.TrimSpace(` -test_instance.foo: (2 tainted) - ID = - Tainted ID 1 = bar - Tainted ID 2 = bar2 - `) - testStateOutput(t, statePath, expected) - - // Should have gotten an error message mentioning index - errOut := ui.ErrorWriter.String() - errContains := "please specify an index" - if !strings.Contains(errOut, errContains) { - t.Fatalf("Expected err output: %s, to contain: %s", errOut, errContains) - } -} - -func TestUntaint_indexSpecified(t *testing.T) { - state := &terraform.State{ - Modules: []*terraform.ModuleState{ - &terraform.ModuleState{ - Path: []string{"root"}, - Resources: map[string]*terraform.ResourceState{ - "test_instance.foo": &terraform.ResourceState{ - Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, - &terraform.InstanceState{ID: "bar2"}, - }, - }, - }, - }, - }, - } - statePath := testStateFile(t, state) - - ui := new(cli.MockUi) - c := &UntaintCommand{ - Meta: Meta{ - Ui: ui, - }, - } - - args := []string{ - "-state", statePath, - "-index", "1", - "test_instance.foo", - } - if code := c.Run(args); code != 0 { - t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) - } - - // Nothing should have gotten untainted - expected := strings.TrimSpace(` -test_instance.foo: (1 tainted) - ID = bar2 - Tainted ID 1 = bar - `) - testStateOutput(t, statePath, expected) -} - func TestUntaint_backup(t *testing.T) { // Get a temp cwd tmp, cwd := testCwd(t) @@ -157,8 +63,9 @@ func TestUntaint_backup(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -183,9 +90,8 @@ func TestUntaint_backup(t *testing.T) { // Backup is still tainted testStateOutput(t, path+".backup", strings.TrimSpace(` -test_instance.foo: (1 tainted) - ID = - Tainted ID 1 = bar +test_instance.foo: (tainted) + ID = bar `)) // State is untainted @@ -208,8 +114,9 @@ func TestUntaint_backupDisable(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -273,8 +180,9 @@ func TestUntaint_defaultState(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -311,8 +219,9 @@ func TestUntaint_missing(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -345,8 +254,9 @@ func TestUntaint_missingAllow(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -385,8 +295,9 @@ func TestUntaint_stateOut(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -411,9 +322,8 @@ func TestUntaint_stateOut(t *testing.T) { } testStateOutput(t, path, strings.TrimSpace(` -test_instance.foo: (1 tainted) - ID = - Tainted ID 1 = bar +test_instance.foo: (tainted) + ID = bar `)) testStateOutput(t, "foo", strings.TrimSpace(` test_instance.foo: @@ -429,8 +339,9 @@ func TestUntaint_module(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.foo": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -440,8 +351,9 @@ func TestUntaint_module(t *testing.T) { Resources: map[string]*terraform.ResourceState{ "test_instance.blah": &terraform.ResourceState{ Type: "test_instance", - Tainted: []*terraform.InstanceState{ - &terraform.InstanceState{ID: "bar"}, + Primary: &terraform.InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -467,9 +379,8 @@ func TestUntaint_module(t *testing.T) { } testStateOutput(t, statePath, strings.TrimSpace(` -test_instance.foo: (1 tainted) - ID = - Tainted ID 1 = bar +test_instance.foo: (tainted) + ID = bar module.child: test_instance.blah: diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index 150ac13fe..99f2d805b 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -884,14 +884,13 @@ func TestContext2Apply_countTainted(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo.0": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "foo": "foo", - "type": "aws_instance", - }, + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "foo": "foo", + "type": "aws_instance", }, + Tainted: true, }, }, }, @@ -2989,13 +2988,12 @@ func TestContext2Apply_destroyTaintedProvisioner(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "bar", - Attributes: map[string]string{ - "id": "bar", - }, + Primary: &InstanceState{ + ID: "bar", + Attributes: map[string]string{ + "id": "bar", }, + Tainted: true, }, }, }, @@ -3505,14 +3503,13 @@ func TestContext2Apply_taint(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.bar": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "baz", - Attributes: map[string]string{ - "num": "2", - "type": "aws_instance", - }, + Primary: &InstanceState{ + ID: "baz", + Attributes: map[string]string{ + "num": "2", + "type": "aws_instance", }, + Tainted: true, }, }, }, @@ -3559,14 +3556,13 @@ func TestContext2Apply_taintDep(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "baz", - Attributes: map[string]string{ - "num": "2", - "type": "aws_instance", - }, + Primary: &InstanceState{ + ID: "baz", + Attributes: map[string]string{ + "num": "2", + "type": "aws_instance", }, + Tainted: true, }, }, "aws_instance.bar": &ResourceState{ @@ -3622,14 +3618,13 @@ func TestContext2Apply_taintDepRequiresNew(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "baz", - Attributes: map[string]string{ - "num": "2", - "type": "aws_instance", - }, + Primary: &InstanceState{ + ID: "baz", + Attributes: map[string]string{ + "num": "2", + "type": "aws_instance", }, + Tainted: true, }, }, "aws_instance.bar": &ResourceState{ @@ -4438,10 +4433,9 @@ func TestContext2Apply_targetedWithTaintedInState(t *testing.T) { Path: rootModulePath, Resources: map[string]*ResourceState{ "aws_instance.ifailedprovisioners": &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ - ID: "ifailedprovisioners", - }, + Primary: &InstanceState{ + ID: "ifailedprovisioners", + Tainted: true, }, }, }, @@ -4485,9 +4479,8 @@ func TestContext2Apply_targetedWithTaintedInState(t *testing.T) { expected := strings.TrimSpace(` aws_instance.iambeingadded: ID = foo -aws_instance.ifailedprovisioners: (1 tainted) - ID = - Tainted ID 1 = ifailedprovisioners +aws_instance.ifailedprovisioners: (tainted) + ID = ifailedprovisioners `) if actual != expected { t.Fatalf("expected state: \n%s\ngot: \n%s", expected, actual) diff --git a/terraform/context_plan_test.go b/terraform/context_plan_test.go index a0dd442ab..956065383 100644 --- a/terraform/context_plan_test.go +++ b/terraform/context_plan_test.go @@ -1718,10 +1718,9 @@ func TestContext2Plan_taint(t *testing.T) { }, "aws_instance.bar": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "baz", - }, + Primary: &InstanceState{ + ID: "baz", + Tainted: true, }, }, }, @@ -1748,57 +1747,6 @@ func TestContext2Plan_taint(t *testing.T) { } } -func TestContext2Plan_multiple_taint(t *testing.T) { - m := testModule(t, "plan-taint") - p := testProvider("aws") - p.DiffFn = testDiffFn - s := &State{ - Modules: []*ModuleState{ - &ModuleState{ - Path: rootModulePath, - Resources: map[string]*ResourceState{ - "aws_instance.foo": &ResourceState{ - Type: "aws_instance", - Primary: &InstanceState{ - ID: "bar", - Attributes: map[string]string{"num": "2"}, - }, - }, - "aws_instance.bar": &ResourceState{ - Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "baz", - }, - &InstanceState{ - ID: "zip", - }, - }, - }, - }, - }, - }, - } - ctx := testContext2(t, &ContextOpts{ - Module: m, - Providers: map[string]ResourceProviderFactory{ - "aws": testProviderFuncFixed(p), - }, - State: s, - }) - - plan, err := ctx.Plan() - if err != nil { - t.Fatalf("err: %s", err) - } - - actual := strings.TrimSpace(plan.String()) - expected := strings.TrimSpace(testTerraformPlanMultipleTaintStr) - if actual != expected { - t.Fatalf("bad:\n%s", actual) - } -} - // Fails about 50% of the time before the fix for GH-4982, covers the fix. func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { m := testModule(t, "plan-taint-interpolated-count") @@ -1811,8 +1759,9 @@ func TestContext2Plan_taintDestroyInterpolatedCountRace(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo.0": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, + Primary: &InstanceState{ + ID: "bar", + Tainted: true, }, }, "aws_instance.foo.1": &ResourceState{ @@ -1849,9 +1798,8 @@ DESTROY/CREATE: aws_instance.foo.0 STATE: -aws_instance.foo.0: (1 tainted) - ID = - Tainted ID 1 = bar +aws_instance.foo.0: (tainted) + ID = bar aws_instance.foo.1: ID = bar aws_instance.foo.2: diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 862028eee..9f4ee58f2 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -324,10 +324,9 @@ func TestContext2Refresh_modules(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.web": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "bar", - }, + Primary: &InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -650,10 +649,9 @@ func TestContext2Refresh_tainted(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.web": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "bar", - }, + Primary: &InstanceState{ + ID: "bar", + Tainted: true, }, }, }, @@ -670,7 +668,8 @@ func TestContext2Refresh_tainted(t *testing.T) { p.RefreshFn = nil p.RefreshReturn = &InstanceState{ - ID: "foo", + ID: "foo", + Tainted: true, } s, err := ctx.Refresh() diff --git a/terraform/context_test.go b/terraform/context_test.go index eee648cd2..897539f6d 100644 --- a/terraform/context_test.go +++ b/terraform/context_test.go @@ -107,9 +107,13 @@ func testDiffFn( info *InstanceInfo, s *InstanceState, c *ResourceConfig) (*InstanceDiff, error) { - var diff InstanceDiff + diff := new(InstanceDiff) diff.Attributes = make(map[string]*ResourceAttrDiff) + if s != nil { + diff.DestroyTainted = s.Tainted + } + for k, v := range c.Raw { if _, ok := v.(string); !ok { continue @@ -181,17 +185,21 @@ func testDiffFn( } } - for k, v := range diff.Attributes { - if v.NewComputed { - continue - } + // If we recreate this resource because it's tainted, we keep all attrs + if !diff.RequiresNew() { + for k, v := range diff.Attributes { + if v.NewComputed { + continue + } - old, ok := s.Attributes[k] - if !ok { - continue - } - if old == v.New { - delete(diff.Attributes, k) + old, ok := s.Attributes[k] + if !ok { + continue + } + + if old == v.New { + delete(diff.Attributes, k) + } } } @@ -202,7 +210,7 @@ func testDiffFn( } } - return &diff, nil + return diff, nil } func testProvider(prefix string) *MockResourceProvider { @@ -276,9 +284,8 @@ root ` const testContextRefreshModuleStr = ` -aws_instance.web: (1 tainted) - ID = - Tainted ID 1 = bar +aws_instance.web: (tainted) + ID = bar module.child: aws_instance.web: @@ -300,7 +307,6 @@ const testContextRefreshOutputPartialStr = ` ` const testContextRefreshTaintedStr = ` -aws_instance.web: (1 tainted) - ID = - Tainted ID 1 = foo +aws_instance.web: (tainted) + ID = foo ` diff --git a/terraform/context_validate_test.go b/terraform/context_validate_test.go index 56de23a36..8b47a17eb 100644 --- a/terraform/context_validate_test.go +++ b/terraform/context_validate_test.go @@ -651,10 +651,9 @@ func TestContext2Validate_tainted(t *testing.T) { Resources: map[string]*ResourceState{ "aws_instance.foo": &ResourceState{ Type: "aws_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "bar", - }, + Primary: &InstanceState{ + ID: "bar", + Tainted: true, }, }, }, diff --git a/terraform/eval_apply.go b/terraform/eval_apply.go index 6314baa86..fd687c5a1 100644 --- a/terraform/eval_apply.go +++ b/terraform/eval_apply.go @@ -139,7 +139,6 @@ type EvalApplyProvisioners struct { Resource *config.Resource InterpResource *Resource CreateNew *bool - Tainted *bool Error *error } @@ -159,9 +158,7 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) { if n.Error != nil && *n.Error != nil { // We're already errored creating, so mark as tainted and continue - if n.Tainted != nil { - *n.Tainted = true - } + state.Tainted = true // We're already tainted, so just return out return nil, nil @@ -180,10 +177,10 @@ func (n *EvalApplyProvisioners) Eval(ctx EvalContext) (interface{}, error) { // If there are no errors, then we append it to our output error // if we have one, otherwise we just output it. err := n.apply(ctx) - if n.Tainted != nil { - *n.Tainted = err != nil - } if err != nil { + // Provisioning failed, so mark the resource as tainted + state.Tainted = true + if n.Error != nil { *n.Error = multierror.Append(*n.Error, err) } else { diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 10deacb26..6cbd20e03 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -69,8 +69,9 @@ type EvalDiff struct { Info *InstanceInfo Config **ResourceConfig Provider *ResourceProvider + Diff **InstanceDiff State **InstanceState - Output **InstanceDiff + OutputDiff **InstanceDiff OutputState **InstanceState } @@ -104,6 +105,11 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { diff = new(InstanceDiff) } + // Preserve the DestroyTainted flag + if n.Diff != nil { + diff.DestroyTainted = (*n.Diff).DestroyTainted + } + // Require a destroy if there is an ID and it requires new. if diff.RequiresNew() && state != nil && state.ID != "" { diff.Destroy = true @@ -135,7 +141,7 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { } // Update our output - *n.Output = diff + *n.OutputDiff = diff // Update the state if we care if n.OutputState != nil { diff --git a/terraform/eval_state.go b/terraform/eval_state.go index aa1d1e28e..35e7c2f26 100644 --- a/terraform/eval_state.go +++ b/terraform/eval_state.go @@ -285,7 +285,8 @@ func (n *EvalDeposeState) Eval(ctx EvalContext) (interface{}, error) { // EvalUndeposeState is an EvalNode implementation that reads the // InstanceState for a specific resource out of the state. type EvalUndeposeState struct { - Name string + Name string + State **InstanceState } // TODO: test @@ -316,7 +317,7 @@ func (n *EvalUndeposeState) Eval(ctx EvalContext) (interface{}, error) { // Undepose idx := len(rs.Deposed) - 1 rs.Primary = rs.Deposed[idx] - rs.Deposed[idx] = nil + rs.Deposed[idx] = *n.State return nil, nil } diff --git a/terraform/eval_state_test.go b/terraform/eval_state_test.go index 3e5265f4a..e702634e0 100644 --- a/terraform/eval_state_test.go +++ b/terraform/eval_state_test.go @@ -88,21 +88,6 @@ func TestEvalReadState(t *testing.T) { }, ExpectedInstanceId: "i-abc123", }, - "ReadStateTainted gets tainted instance": { - Resources: map[string]*ResourceState{ - "aws_instance.bar": &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "i-abc123"}, - }, - }, - }, - Node: &EvalReadStateTainted{ - Name: "aws_instance.bar", - Output: &output, - Index: 0, - }, - ExpectedInstanceId: "i-abc123", - }, "ReadStateDeposed gets deposed instance": { Resources: map[string]*ResourceState{ "aws_instance.bar": &ResourceState{ @@ -175,32 +160,6 @@ restype.resname: `) } -func TestEvalWriteStateTainted(t *testing.T) { - state := &State{} - ctx := new(MockEvalContext) - ctx.StateState = state - ctx.StateLock = new(sync.RWMutex) - ctx.PathPath = rootModulePath - - is := &InstanceState{ID: "i-abc123"} - node := &EvalWriteStateTainted{ - Name: "restype.resname", - ResourceType: "restype", - State: &is, - Index: -1, - } - _, err := node.Eval(ctx) - if err != nil { - t.Fatalf("Got err: %#v", err) - } - - checkStateString(t, state, ` -restype.resname: (1 tainted) - ID = - Tainted ID 1 = i-abc123 - `) -} - func TestEvalWriteStateDeposed(t *testing.T) { state := &State{} ctx := new(MockEvalContext) diff --git a/terraform/graph_builder_test.go b/terraform/graph_builder_test.go index b2ddd8d4e..2cb76757b 100644 --- a/terraform/graph_builder_test.go +++ b/terraform/graph_builder_test.go @@ -295,16 +295,11 @@ provider.aws (close) const testBuiltinGraphBuilderVerboseStr = ` aws_instance.db - aws_instance.db (destroy tainted) aws_instance.db (destroy) -aws_instance.db (destroy tainted) - aws_instance.web (destroy tainted) aws_instance.db (destroy) aws_instance.web (destroy) aws_instance.web aws_instance.db -aws_instance.web (destroy tainted) - provider.aws aws_instance.web (destroy) provider.aws provider.aws diff --git a/terraform/graph_config_node_resource.go b/terraform/graph_config_node_resource.go index dbe5a0ecd..36e1d9112 100644 --- a/terraform/graph_config_node_resource.go +++ b/terraform/graph_config_node_resource.go @@ -120,7 +120,11 @@ func (n *GraphNodeConfigResource) VarWalk(fn func(config.InterpolatedVariable)) } func (n *GraphNodeConfigResource) Name() string { - return n.Resource.Id() + " (destroy)" + result := n.Resource.Id() + if n.Destroy { + result += " (destroy)" + } + return result } // GraphNodeDotter impl. diff --git a/terraform/resource.go b/terraform/resource.go index 5b9fbf7d2..7d22a6e9b 100644 --- a/terraform/resource.go +++ b/terraform/resource.go @@ -42,7 +42,6 @@ type Resource struct { State *InstanceState Provisioners []*ResourceProvisionerConfig Flags ResourceFlag - TaintedIndex int } // ResourceKind specifies what kind of instance we're working with, whether @@ -53,7 +52,6 @@ const ( FlagPrimary ResourceFlag = 1 << iota FlagTainted FlagOrphan - FlagHasTainted FlagReplacePrimary FlagDeposed ) diff --git a/terraform/state.go b/terraform/state.go index 19b781b55..02ee9cf4f 100644 --- a/terraform/state.go +++ b/terraform/state.go @@ -327,7 +327,7 @@ func (s *State) removeInstance(path []string, r *ResourceState, v *InstanceState } // Check lists - lists := [][]*InstanceState{r.Tainted, r.Deposed} + lists := [][]*InstanceState{r.Deposed} for _, is := range lists { for i, instance := range is { if instance == v { @@ -861,7 +861,11 @@ func (m *ModuleState) String() string { } for idx, t := range rs.Deposed { - buf.WriteString(fmt.Sprintf(" Deposed ID %d = %s\n", idx+1, t.ID)) + taintStr := "" + if t.Tainted { + taintStr = " (tainted)" + } + buf.WriteString(fmt.Sprintf(" Deposed ID %d = %s%s\n", idx+1, t.ID, taintStr)) } if len(rs.Dependencies) > 0 { @@ -1030,11 +1034,14 @@ type ResourceState struct { // Deposed is used in the mechanics of CreateBeforeDestroy: the existing // Primary is Deposed to get it out of the way for the replacement Primary to // be created by Apply. If the replacement Primary creates successfully, the - // Deposed instance is cleaned up. If there were problems creating the - // replacement, the instance remains in the Deposed list so it can be - // destroyed in a future run. Functionally, Deposed instances are very - // similar to Tainted instances in that Terraform is only tracking them in - // order to remember to destroy them. + // Deposed instance is cleaned up. + // + // If there were problems creating the replacement Primary, the Deposed + // instance and the (now tainted) replacement Primary will be swapped so the + // tainted replacement will be cleaned up instead. + // + // An instance will remain in the Deposed list until it is successfully + // destroyed and purged. Deposed []*InstanceState `json:"deposed,omitempty"` // Provider is used when a resource is connected to a provider with an alias. @@ -1071,22 +1078,21 @@ func (s *ResourceState) Equal(other *ResourceState) bool { return false } - // Tainted - if s.Primary.Tainted != other.Primary.Tainted { - return false - } - return true } // Taint marks a resource as tainted. func (r *ResourceState) Taint() { - r.Primary.Tainted = true + if r.Primary != nil { + r.Primary.Tainted = true + } } // Untaint unmarks a resource as tainted. func (r *ResourceState) Untaint() { - r.Primary.Tainted = false + if r.Primary != nil { + r.Primary.Tainted = false + } } func (r *ResourceState) init() { diff --git a/terraform/state_filter.go b/terraform/state_filter.go index 7a2937123..89cf0d898 100644 --- a/terraform/state_filter.go +++ b/terraform/state_filter.go @@ -133,19 +133,6 @@ func (f *StateFilter) filterSingle(a *ResourceAddress) []*StateFilterResult { }) } - for _, instance := range r.Tainted { - if f.relevant(a, instance) { - addr.InstanceType = TypeTainted - addr.InstanceTypeSet = true - results = append(results, &StateFilterResult{ - Path: addr.Path, - Address: addr.String(), - Parent: resourceResult, - Value: instance, - }) - } - } - for _, instance := range r.Deposed { if f.relevant(a, instance) { addr.InstanceType = TypeDeposed diff --git a/terraform/state_test.go b/terraform/state_test.go index 7a7dd0f0a..e3ee69137 100644 --- a/terraform/state_test.go +++ b/terraform/state_test.go @@ -742,11 +742,14 @@ func TestResourceStateEqual(t *testing.T) { { false, &ResourceState{ - Tainted: nil, + Primary: &InstanceState{ + ID: "foo", + }, }, &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, @@ -754,28 +757,15 @@ func TestResourceStateEqual(t *testing.T) { { true, &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, - }, - }, - }, - - { - true, - &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, - nil, - }, - }, - &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, @@ -801,28 +791,29 @@ func TestResourceStateTaint(t *testing.T) { &ResourceState{}, }, - "primary, no tainted": { + "primary, not tainted": { &ResourceState{ Primary: &InstanceState{ID: "foo"}, }, &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, - "primary, with tainted": { + "primary, tainted": { &ResourceState{ - Primary: &InstanceState{ID: "foo"}, - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, @@ -841,92 +832,36 @@ func TestResourceStateTaint(t *testing.T) { func TestResourceStateUntaint(t *testing.T) { cases := map[string]struct { Input *ResourceState - Index func() int ExpectedOutput *ResourceState - ExpectedErrMsg string }{ - "no primary, no tainted, err": { + "no primary, err": { Input: &ResourceState{}, ExpectedOutput: &ResourceState{}, - ExpectedErrMsg: "Nothing to untaint", }, - "one tainted, no primary": { + "primary, not tainted": { Input: &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ID: "foo"}, + }, + ExpectedOutput: &ResourceState{ + Primary: &InstanceState{ID: "foo"}, + }, + }, + "primary, tainted": { + Input: &ResourceState{ + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, ExpectedOutput: &ResourceState{ Primary: &InstanceState{ID: "foo"}, - Tainted: []*InstanceState{}, }, }, - - "one tainted, existing primary error": { - Input: &ResourceState{ - Primary: &InstanceState{ID: "foo"}, - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, - }, - }, - ExpectedErrMsg: "Resource has a primary", - }, - - "multiple tainted, no index": { - Input: &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, - &InstanceState{ID: "foo"}, - }, - }, - ExpectedErrMsg: "please specify an index", - }, - - "multiple tainted, with index": { - Input: &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, - &InstanceState{ID: "foo"}, - }, - }, - Index: func() int { return 1 }, - ExpectedOutput: &ResourceState{ - Primary: &InstanceState{ID: "foo"}, - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, - }, - }, - }, - - "index out of bounds error": { - Input: &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "bar"}, - &InstanceState{ID: "foo"}, - }, - }, - Index: func() int { return 2 }, - ExpectedErrMsg: "out of range", - }, } for k, tc := range cases { - index := -1 - if tc.Index != nil { - index = tc.Index() - } - err := tc.Input.Untaint(index) - if tc.ExpectedErrMsg == "" && err != nil { - t.Fatalf("[%s] unexpected err: %s", k, err) - } - if tc.ExpectedErrMsg != "" { - if strings.Contains(err.Error(), tc.ExpectedErrMsg) { - continue - } - t.Fatalf("[%s] expected err: %s to contain: %s", - k, err, tc.ExpectedErrMsg) - } + tc.Input.Untaint() if !reflect.DeepEqual(tc.Input, tc.ExpectedOutput) { t.Fatalf( "Failure: %s\n\nExpected: %#v\n\nGot: %#v", @@ -1152,7 +1087,7 @@ func TestInstanceState_MergeDiff(t *testing.T) { } func TestInstanceState_MergeDiff_nil(t *testing.T) { - var is *InstanceState = nil + var is *InstanceState diff := &InstanceDiff{ Attributes: map[string]*ResourceAttrDiff{ @@ -1504,7 +1439,7 @@ func TestUpgradeV0State(t *testing.T) { foo.Primary.Attributes["key"] != "val" { t.Fatalf("bad: %#v", foo) } - if len(foo.Tainted) > 0 { + if foo.Primary.Tainted { t.Fatalf("bad: %#v", foo) } @@ -1512,16 +1447,9 @@ func TestUpgradeV0State(t *testing.T) { if bar.Type != "test_resource" { t.Fatalf("bad: %#v", bar) } - if bar.Primary != nil { + if !bar.Primary.Tainted { t.Fatalf("bad: %#v", bar) } - if len(bar.Tainted) != 1 { - t.Fatalf("bad: %#v", bar) - } - bt := bar.Tainted[0] - if bt.ID != "1234" || bt.Attributes["a"] != "b" { - t.Fatalf("bad: %#v", bt) - } } func TestParseResourceStateKey(t *testing.T) { diff --git a/terraform/terraform_test.go b/terraform/terraform_test.go index ad19d7b1b..33412a8ce 100644 --- a/terraform/terraform_test.go +++ b/terraform/terraform_test.go @@ -399,9 +399,8 @@ aws_instance.foo: ` const testTerraformApplyProvisionerFailStr = ` -aws_instance.bar: (1 tainted) - ID = - Tainted ID 1 = foo +aws_instance.bar: (tainted) + ID = foo aws_instance.foo: ID = foo num = 2 @@ -409,9 +408,8 @@ aws_instance.foo: ` const testTerraformApplyProvisionerFailCreateStr = ` -aws_instance.bar: (1 tainted) - ID = - Tainted ID 1 = foo +aws_instance.bar: (tainted) + ID = foo ` const testTerraformApplyProvisionerFailCreateNoIdStr = ` @@ -419,10 +417,10 @@ const testTerraformApplyProvisionerFailCreateNoIdStr = ` ` const testTerraformApplyProvisionerFailCreateBeforeDestroyStr = ` -aws_instance.bar: (1 tainted) +aws_instance.bar: (1 deposed) ID = bar require_new = abc - Tainted ID 1 = foo + Deposed ID 1 = foo (tainted) ` const testTerraformApplyProvisionerResourceRefStr = ` @@ -1242,9 +1240,8 @@ DESTROY/CREATE: aws_instance.bar STATE: -aws_instance.bar: (1 tainted) - ID = - Tainted ID 1 = baz +aws_instance.bar: (tainted) + ID = baz aws_instance.foo: ID = bar num = 2 diff --git a/terraform/transform_destroy_test.go b/terraform/transform_destroy_test.go index 1a50ce053..23b994aed 100644 --- a/terraform/transform_destroy_test.go +++ b/terraform/transform_destroy_test.go @@ -359,8 +359,9 @@ func TestPruneDestroyTransformer_tainted(t *testing.T) { Path: RootModulePath, Resources: map[string]*ResourceState{ "aws_instance.bar": &ResourceState{ - Tainted: []*InstanceState{ - &InstanceState{ID: "foo"}, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, @@ -399,16 +400,11 @@ func TestPruneDestroyTransformer_tainted(t *testing.T) { const testTransformDestroyBasicStr = ` aws_instance.bar - aws_instance.bar (destroy tainted) aws_instance.bar (destroy) aws_instance.foo -aws_instance.bar (destroy tainted) aws_instance.bar (destroy) aws_instance.foo - aws_instance.foo (destroy tainted) aws_instance.foo (destroy) -aws_instance.foo (destroy tainted) - aws_instance.bar (destroy tainted) aws_instance.foo (destroy) aws_instance.bar (destroy) ` @@ -456,40 +452,28 @@ aws_instance.foo-bar (destroy) const testTransformPruneDestroyTaintedStr = ` aws_instance.bar - aws_instance.bar (destroy tainted) aws_instance.foo -aws_instance.bar (destroy tainted) aws_instance.foo ` const testTransformCreateBeforeDestroyBasicStr = ` aws_instance.web - aws_instance.web (destroy tainted) -aws_instance.web (destroy tainted) - aws_load_balancer.lb (destroy tainted) aws_instance.web (destroy) aws_instance.web aws_load_balancer.lb aws_load_balancer.lb (destroy) aws_load_balancer.lb aws_instance.web - aws_load_balancer.lb (destroy tainted) aws_load_balancer.lb (destroy) -aws_load_balancer.lb (destroy tainted) aws_load_balancer.lb (destroy) ` const testTransformCreateBeforeDestroyTwiceStr = ` aws_autoscale.bar - aws_autoscale.bar (destroy tainted) aws_lc.foo -aws_autoscale.bar (destroy tainted) aws_autoscale.bar (destroy) aws_autoscale.bar aws_lc.foo - aws_lc.foo (destroy tainted) -aws_lc.foo (destroy tainted) - aws_autoscale.bar (destroy tainted) aws_lc.foo (destroy) aws_autoscale.bar aws_autoscale.bar (destroy) diff --git a/terraform/transform_resource.go b/terraform/transform_resource.go index 07da19b77..0b2df921a 100644 --- a/terraform/transform_resource.go +++ b/terraform/transform_resource.go @@ -337,7 +337,7 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Config: &resourceConfig, Provider: &provider, State: &state, - Output: &diff, + OutputDiff: &diff, OutputState: &state, }, &EvalCheckPreventDestroy{ @@ -392,7 +392,7 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, // Apply var diffApply *InstanceDiff var err error - var createNew, tainted bool + var createNew bool var createBeforeDestroyEnabled bool var wasChangeType DiffChangeType nodes = append(nodes, &EvalOpFilter{ @@ -456,11 +456,12 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, }, &EvalDiff{ - Info: info, - Config: &resourceConfig, - Provider: &provider, - State: &state, - Output: &diffApply, + Info: info, + Config: &resourceConfig, + Provider: &provider, + Diff: &diffApply, + State: &state, + OutputDiff: &diffApply, }, &EvalIgnoreChanges{ Resource: n.Resource, @@ -511,20 +512,22 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Resource: n.Resource, InterpResource: resource, CreateNew: &createNew, - Tainted: &tainted, Error: &err, }, &EvalIf{ If: func(ctx EvalContext) (bool, error) { - if createBeforeDestroyEnabled { - tainted = err != nil - } - - failure := tainted || err != nil - return createBeforeDestroyEnabled && failure, nil + return createBeforeDestroyEnabled && err != nil, nil }, Then: &EvalUndeposeState{ - Name: n.stateId(), + Name: n.stateId(), + State: &state, + }, + Else: &EvalWriteState{ + Name: n.stateId(), + ResourceType: n.Resource.Type, + Provider: n.Resource.Provider, + Dependencies: n.StateDependencies(), + State: &state, }, }, @@ -536,13 +539,6 @@ func (n *graphNodeExpandedResource) managedResourceEvalNodes(resource *Resource, Diff: nil, }, - &EvalWriteState{ - Name: n.stateId(), - ResourceType: n.Resource.Type, - Provider: n.Resource.Provider, - Dependencies: n.StateDependencies(), - State: &state, - }, &EvalApplyPost{ Info: info, State: &state, From 5af1afd64e070fa1cf5960c98cc39b662908609b Mon Sep 17 00:00:00 2001 From: Sander van Harmelen Date: Mon, 23 May 2016 12:23:48 +0200 Subject: [PATCH 3/3] Update newly added code to work with the updated taint semantics --- terraform/state_add.go | 18 +----------------- terraform/state_add_test.go | 19 ++++++++----------- terraform/state_v0.go | 11 +++++------ terraform/state_v1.go | 13 ------------- 4 files changed, 14 insertions(+), 47 deletions(-) diff --git a/terraform/state_add.go b/terraform/state_add.go index f59c86225..83643a042 100644 --- a/terraform/state_add.go +++ b/terraform/state_add.go @@ -133,11 +133,6 @@ func stateAddFunc_Resource_Resource(s *State, fromAddr, addr *ResourceAddress, r } } - // Move all tainted - if len(src.Tainted) > 0 { - resource.Tainted = src.Tainted - } - // Move all deposed if len(src.Deposed) > 0 { resource.Deposed = src.Deposed @@ -281,23 +276,12 @@ func stateAddInitAddr(s *State, addr *ResourceAddress) (interface{}, bool) { exists = true instance := &InstanceState{} switch addr.InstanceType { - case TypePrimary: + case TypePrimary, TypeTainted: if v := resource.Primary; v != nil { instance = resource.Primary } else { exists = false } - case TypeTainted: - idx := addr.Index - if addr.Index < 0 { - idx = 0 - } - if len(resource.Tainted) > idx { - instance = resource.Tainted[idx] - } else { - resource.Tainted = append(resource.Tainted, instance) - exists = false - } case TypeDeposed: idx := addr.Index if addr.Index < 0 { diff --git a/terraform/state_add_test.go b/terraform/state_add_test.go index b9fe35592..c69e99056 100644 --- a/terraform/state_add_test.go +++ b/terraform/state_add_test.go @@ -256,16 +256,15 @@ func TestStateAdd(t *testing.T) { }, }, - "ResourceState w/ tainted => Resource Addr (new)": { + "ResourceState tainted => Resource Addr (new)": { false, "aws_instance.bar", "aws_instance.foo", &ResourceState{ Type: "test_instance", - Tainted: []*InstanceState{ - &InstanceState{ - ID: "foo", - }, + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, @@ -276,12 +275,10 @@ func TestStateAdd(t *testing.T) { Path: []string{"root"}, Resources: map[string]*ResourceState{ "aws_instance.foo": &ResourceState{ - Type: "test_instance", - Primary: &InstanceState{}, - Tainted: []*InstanceState{ - &InstanceState{ - ID: "foo", - }, + Type: "test_instance", + Primary: &InstanceState{ + ID: "foo", + Tainted: true, }, }, }, diff --git a/terraform/state_v0.go b/terraform/state_v0.go index d034c25a9..33bc9b238 100644 --- a/terraform/state_v0.go +++ b/terraform/state_v0.go @@ -10,8 +10,9 @@ import ( "strings" "sync" - "github.com/hashicorp/terraform/config" "log" + + "github.com/hashicorp/terraform/config" ) // The format byte is prefixed into the state file format so that we have @@ -342,16 +343,14 @@ func (old *StateV0) upgrade() (*State, error) { root.Resources[id] = newRs // Migrate to an instance state - instance := &InstanceState{ + newRs.Primary = &InstanceState{ ID: rs.ID, Attributes: rs.Attributes, } - // Check if this is the primary or tainted instance + // Check if old resource was tainted if _, ok := old.Tainted[id]; ok { - newRs.Tainted = append(newRs.Tainted, instance) - } else { - newRs.Primary = instance + newRs.Primary.Tainted = true } // Warn if the resource uses Extra, as there is diff --git a/terraform/state_v1.go b/terraform/state_v1.go index 2872f8364..72611e11d 100644 --- a/terraform/state_v1.go +++ b/terraform/state_v1.go @@ -235,18 +235,6 @@ func (old *resourceStateV1) upgrade() (*ResourceState, error) { return nil, fmt.Errorf("Error upgrading ResourceState V1: %v", err) } - tainted := make([]*InstanceState, len(old.Tainted)) - for i, v := range old.Tainted { - upgraded, err := v.upgrade() - if err != nil { - return nil, fmt.Errorf("Error upgrading ResourceState V1: %v", err) - } - tainted[i] = upgraded - } - if len(tainted) == 0 { - tainted = nil - } - deposed := make([]*InstanceState, len(old.Deposed)) for i, v := range old.Deposed { upgraded, err := v.upgrade() @@ -263,7 +251,6 @@ func (old *resourceStateV1) upgrade() (*ResourceState, error) { Type: old.Type, Dependencies: dependencies.([]string), Primary: primary, - Tainted: tainted, Deposed: deposed, Provider: old.Provider, }, nil