Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing dynamic expansion issues #578

Merged
merged 5 commits into from
Nov 19, 2014
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,12 @@
## 0.3.2 (unreleased)

BUG FIXES:

* core: Fixed issue causing double delete. [GH-555]
* core: Fixed issue with create-before-destroy not being respected in
some circumstances.
* core: Fixing issue with count expansion with non-homogenous instance
plans.

## 0.3.1 (October 21, 2014)

Expand Down
89 changes: 88 additions & 1 deletion terraform/context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -2656,7 +2656,7 @@ func TestContextApply_createBefore_depends(t *testing.T) {
// Test that things were managed _in the right order_
order := h.States
diffs := h.Diffs
if order[0].ID != "bar" || diffs[0].Destroy {
if order[0].ID != "" || diffs[0].Destroy {
t.Fatalf("should create new instance first: %#v", order)
}

Expand All @@ -2669,6 +2669,93 @@ func TestContextApply_createBefore_depends(t *testing.T) {
}
}

func TestContextApply_singleDestroy(t *testing.T) {
m := testModule(t, "apply-depends-create-before")
h := new(HookRecordApplyOrder)
p := testProvider("aws")

invokeCount := 0
p.ApplyFn = func(info *InstanceInfo, s *InstanceState, d *InstanceDiff) (*InstanceState, error) {
invokeCount++
switch invokeCount {
case 1:
if d.Destroy {
t.Fatalf("should not destroy")
}
if s.ID != "" {
t.Fatalf("should not have ID")
}
case 2:
if d.Destroy {
t.Fatalf("should not destroy")
}
if s.ID != "baz" {
t.Fatalf("should have id")
}
case 3:
if !d.Destroy {
t.Fatalf("should destroy")
}
if s.ID == "" {
t.Fatalf("should have ID")
}
default:
t.Fatalf("bad invoke count %d", invokeCount)
}
return testApplyFn(info, s, d)
}
p.DiffFn = testDiffFn
state := &State{
Modules: []*ModuleState{
&ModuleState{
Path: rootModulePath,
Resources: map[string]*ResourceState{
"aws_instance.web": &ResourceState{
Type: "aws_instance",
Primary: &InstanceState{
ID: "bar",
Attributes: map[string]string{
"require_new": "ami-old",
},
},
},
"aws_instance.lb": &ResourceState{
Type: "aws_instance",
Primary: &InstanceState{
ID: "baz",
Attributes: map[string]string{
"instance": "bar",
},
},
},
},
},
},
}
ctx := testContext(t, &ContextOpts{
Module: m,
Hooks: []Hook{h},
Providers: map[string]ResourceProviderFactory{
"aws": testProviderFuncFixed(p),
},
State: state,
})

if _, err := ctx.Plan(nil); err != nil {
t.Fatalf("err: %s", err)
}

h.Active = true
state, err := ctx.Apply()
if err != nil {
t.Fatalf("err: %s", err)
}

if invokeCount != 3 {
t.Fatalf("bad: %d", invokeCount)
}
}

func TestContextPlan(t *testing.T) {
m := testModule(t, "plan-good")
p := testProvider("aws")
Expand Down
140 changes: 90 additions & 50 deletions terraform/graph.go
Original file line number Diff line number Diff line change
Expand Up @@ -539,6 +539,8 @@ func graphAddDiff(g *depgraph.Graph, d *ModuleDiff) error {
rn.Diff = d
}

// If we are not expanding, then we assign the
// instance diff to the resource.
var rd *InstanceDiff
if rn.ExpandMode == ResourceExpandNone {
rd = diffs[0]
Expand Down Expand Up @@ -1648,32 +1650,30 @@ func (n *GraphNodeResource) Expand() (*depgraph.Graph, error) {
ModulePath: n.Resource.Info.ModulePath,
}

// Determine the nodes to create. If we're just looking for the
// nodes to create, return that.
n.expand(g, count)

// Add in the diff if we have it
if n.Diff != nil {
if err := graphAddDiff(g, n.Diff); err != nil {
return nil, err
}
}
// Do the initial expansion of the nodes, attaching diffs if
// applicable
n.expand(g, count, n.Diff)

// Add all the variable dependencies
graphAddVariableDeps(g)

// If we're just expanding the apply, then filter those out and
// return them now.
if n.ExpandMode == ResourceExpandApply {
return n.finalizeGraph(g, false)
// Filter the nodes depending on the expansion type
switch n.ExpandMode {
case ResourceExpandApply:
n.filterResources(g, false)
case ResourceExpandDestroy:
n.filterResources(g, true)
default:
panic(fmt.Sprintf("Unhandled expansion mode %d", n.ExpandMode))
}

return n.finalizeGraph(g, true)
// Return the finalized graph
return g, n.finalizeGraph(g)
}

// expand expands this resource and adds the resources to the graph. It
// adds both create and destroy resources.
func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) {
func (n *GraphNodeResource) expand(g *depgraph.Graph, count int, diff *ModuleDiff) {
// Create the list of nouns
result := make([]*depgraph.Noun, 0, count)

Expand Down Expand Up @@ -1727,13 +1727,70 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) {
}
}

// Add in the diff if we have it
var inDiff *InstanceDiff
if diff != nil {
// Looup the instance diff
if d, ok := diff.Resources[name]; ok {
inDiff = d
}

if inDiff == nil {
if count == 1 {
// If the count is one, check the state for ".0"
// appended, which might exist if we go from
// count > 1 to count == 1.
k := r.Id() + ".0"
inDiff = diff.Resources[k]
} else if i == 0 {
// If count is greater than one, check for state
// with just the ID, which might exist if we go
// from count == 1 to count > 1
inDiff = diff.Resources[r.Id()]
}
}
}

// Initialize a default state if not available
if state == nil {
state = &ResourceState{
Type: r.Type,
}
}

flags := FlagPrimary
// Prepare the diff if it exists
if inDiff != nil {
switch n.ExpandMode {
case ResourceExpandApply:
// Disable Destroy if we aren't doing a destroy expansion.
// There is a seperate expansion for the destruction action.
d := new(InstanceDiff)
*d = *inDiff
inDiff = d
inDiff.Destroy = false

// If we require a new resource, there is a seperate delete
// phase, so the create phase must not have access to the ID.
if inDiff.RequiresNew() {
s := new(ResourceState)
*s = *state
state = s
state.Primary = nil
}

case ResourceExpandDestroy:
// If we are doing a destroy, make sure it is exclusively
// a destroy, since there is a seperate expansion for the apply
inDiff = new(InstanceDiff)
inDiff.Destroy = true

default:
panic(fmt.Sprintf("Unhandled expansion mode %d", n.ExpandMode))
}
}

// Inherit the existing flags!
flags := n.Resource.Flags
if len(state.Tainted) > 0 {
flags |= FlagHasTainted
}
Expand All @@ -1743,6 +1800,7 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) {
resource.CountIndex = i
resource.State = state.Primary
resource.Flags = flags
resource.Diff = inDiff

// Add the result
result = append(result, &depgraph.Noun{
Expand All @@ -1763,6 +1821,7 @@ func (n *GraphNodeResource) expand(g *depgraph.Graph, count int) {
resource.Config = NewResourceConfig(nil)
resource.State = rs.Primary
resource.Flags = FlagOrphan
resource.Diff = &InstanceDiff{Destroy: true}

noun := &depgraph.Noun{
Name: k,
Expand Down Expand Up @@ -1790,53 +1849,35 @@ func (n *GraphNodeResource) copyResource(id string) *Resource {
return &resource
}

func (n *GraphNodeResource) finalizeGraph(
g *depgraph.Graph, destroy bool) (*depgraph.Graph, error) {
// filterResources is used to remove resources from the sub-graph based
// on the ExpandMode. This is because there is a Destroy sub-graph, and
// Apply sub-graph, and we cannot includes the same instances in both
// sub-graphs.
func (n *GraphNodeResource) filterResources(g *depgraph.Graph, destroy bool) {
result := make([]*depgraph.Noun, 0, len(g.Nouns))
for _, n := range g.Nouns {
rn, ok := n.Meta.(*GraphNodeResource)
if !ok {
continue
}

// If the diff is nil, then we're not destroying, so append only
// in that case.
if rn.Resource.Diff == nil {
if !destroy {
result = append(result, n)
}

continue
}

// If we are destroying, append it only if we care about destroys
if rn.Resource.Diff.Destroy {
if destroy {
result = append(result, n)
}

continue
}

// If this is an oprhan, we only care about it if we're destroying.
if rn.Resource.Flags&FlagOrphan != 0 {
if destroy {
if destroy {
if rn.Resource.Diff != nil && rn.Resource.Diff.Destroy {
result = append(result, n)
}

continue
}

// If we're not destroying, then add it only if we don't
// care about deploys.
if !destroy {
if rn.Resource.Flags&FlagOrphan != 0 ||
rn.Resource.Diff == nil || !rn.Resource.Diff.Destroy {
result = append(result, n)
}
}

// Set the nouns to be only those we care about
g.Nouns = result
}

// finalizeGraph is used to ensure the generated graph is valid
func (n *GraphNodeResource) finalizeGraph(g *depgraph.Graph) error {
// Remove the dependencies that don't exist
graphRemoveInvalidDeps(g)

Expand All @@ -1845,10 +1886,9 @@ func (n *GraphNodeResource) finalizeGraph(

// Validate
if err := g.Validate(); err != nil {
return nil, err
return err
}

return g, nil
return nil
}

// matchingPrefixes takes a resource type and a set of resource
Expand Down