Merge pull request #2968 from hashicorp/b-orphan-module-deadlock

core: fix deadlock when dependable node replaced with non-dependable one

dag/dag.go

@@ -2,9 +2,11 @@ package dag
 
 import (
 	"fmt"
+	"log"
 	"sort"
 	"strings"
 	"sync"
+	"time"
 
 	"github.com/hashicorp/go-multierror"
 )
@@ -197,8 +199,19 @@ func (g *AcyclicGraph) Walk(cb WalkFunc) error {
 		readyCh := make(chan bool)
 		go func(v Vertex, deps []Vertex, chs []<-chan struct{}, readyCh chan<- bool) {
 			// First wait for all the dependencies
-			for _, ch := range chs {
-				<-ch
+			for i, ch := range chs {
+			DepSatisfied:
+				for {
+					select {
+					case <-ch:
+						break DepSatisfied
+					case <-time.After(time.Second * 5):
+						log.Printf("[DEBUG] vertex %s, waiting for: %s",
+							VertexName(v), VertexName(deps[i]))
+					}
+				}
+				log.Printf("[DEBUG] vertex %s, got dep: %s",
+					VertexName(v), VertexName(deps[i]))
 			}
 
 			// Then, check the map to see if any of our dependencies failed

terraform/context_plan_test.go

@@ -9,7 +9,6 @@ import (
 	"strings"
 	"sync"
 	"testing"
-	"time"
 )
 
 func TestContext2Plan(t *testing.T) {
@@ -176,16 +175,11 @@ func TestContext2Plan_moduleCycle(t *testing.T) {
 }
 
 func TestContext2Plan_moduleDeadlock(t *testing.T) {
+	testCheckDeadlock(t, func() {
 		m := testModule(t, "plan-module-deadlock")
 		p := testProvider("aws")
 		p.DiffFn = testDiffFn
-	timeout := make(chan bool, 1)
-	done := make(chan bool, 1)
-	go func() {
-		time.Sleep(3 * time.Second)
-		timeout <- true
-	}()
-	go func() {
+
 		ctx := testContext2(t, &ContextOpts{
 			Module: m,
 			Providers: map[string]ResourceProviderFactory{
@@ -194,7 +188,6 @@ func TestContext2Plan_moduleDeadlock(t *testing.T) {
 		})
 
 		plan, err := ctx.Plan()
-		done <- true
 		if err != nil {
 			t.Fatalf("err: %s", err)
 		}
@@ -215,14 +208,7 @@ STATE:
 		if actual != expected {
 			t.Fatalf("expected:\n%sgot:\n%s", expected, actual)
 		}
-	}()
-
-	select {
-	case <-timeout:
-		t.Fatalf("timed out! probably deadlock")
-	case <-done:
-		// ok
-	}
+	})
 }
 
 func TestContext2Plan_moduleInput(t *testing.T) {

terraform/context_refresh_test.go

@@ -4,6 +4,7 @@ import (
 	"reflect"
 	"sort"
 	"strings"
+	"sync"
 	"testing"
 )
 
@@ -685,6 +686,100 @@ func TestContext2Refresh_vars(t *testing.T) {
 	}
 }
 
+func TestContext2Refresh_orphanModule(t *testing.T) {
+	p := testProvider("aws")
+	m := testModule(t, "refresh-module-orphan")
+
+	// Create a custom refresh function to track the order they were visited
+	var order []string
+	var orderLock sync.Mutex
+	p.RefreshFn = func(
+		info *InstanceInfo,
+		is *InstanceState) (*InstanceState, error) {
+		orderLock.Lock()
+		defer orderLock.Unlock()
+
+		order = append(order, is.ID)
+		return is, nil
+	}
+
+	state := &State{
+		Modules: []*ModuleState{
+			&ModuleState{
+				Path: rootModulePath,
+				Resources: map[string]*ResourceState{
+					"aws_instance.foo": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-abc123",
+							Attributes: map[string]string{
+								"childid":      "i-bcd234",
+								"grandchildid": "i-cde345",
+							},
+						},
+						Dependencies: []string{
+							"module.child",
+							"module.child",
+						},
+					},
+				},
+			},
+			&ModuleState{
+				Path: append(rootModulePath, "child"),
+				Resources: map[string]*ResourceState{
+					"aws_instance.bar": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-bcd234",
+							Attributes: map[string]string{
+								"grandchildid": "i-cde345",
+							},
+						},
+						Dependencies: []string{
+							"module.grandchild",
+						},
+					},
+				},
+				Outputs: map[string]string{
+					"id":            "i-bcd234",
+					"grandchild_id": "i-cde345",
+				},
+			},
+			&ModuleState{
+				Path: append(rootModulePath, "child", "grandchild"),
+				Resources: map[string]*ResourceState{
+					"aws_instance.baz": &ResourceState{
+						Primary: &InstanceState{
+							ID: "i-cde345",
+						},
+					},
+				},
+				Outputs: map[string]string{
+					"id": "i-cde345",
+				},
+			},
+		},
+	}
+	ctx := testContext2(t, &ContextOpts{
+		Module: m,
+		Providers: map[string]ResourceProviderFactory{
+			"aws": testProviderFuncFixed(p),
+		},
+		State: state,
+	})
+
+	testCheckDeadlock(t, func() {
+		_, err := ctx.Refresh()
+		if err != nil {
+			t.Fatalf("err: %s", err)
+		}
+
+		// TODO: handle order properly for orphaned modules / resources
+		// expected := []string{"i-abc123", "i-bcd234", "i-cde345"}
+		// if !reflect.DeepEqual(order, expected) {
+		// 	t.Fatalf("expected: %#v, got: %#v", expected, order)
+		// }
+	})
+}
+
 func TestContext2Validate(t *testing.T) {
 	p := testProvider("aws")
 	m := testModule(t, "validate-good")

terraform/context_test.go

@@ -4,6 +4,7 @@ import (
 	"fmt"
 	"strings"
 	"testing"
+	"time"
 )
 
 func testContext2(t *testing.T, opts *ContextOpts) *Context {
@@ -170,6 +171,27 @@ func resourceState(resourceType, resourceID string) *ResourceState {
 	}
 }
 
+// Test helper that gives a function 3 seconds to finish, assumes deadlock and
+// fails test if it does not.
+func testCheckDeadlock(t *testing.T, f func()) {
+	timeout := make(chan bool, 1)
+	done := make(chan bool, 1)
+	go func() {
+		time.Sleep(3 * time.Second)
+		timeout <- true
+	}()
+	go func(f func(), done chan bool) {
+		defer func() { done <- true }()
+		f()
+	}(f, done)
+	select {
+	case <-timeout:
+		t.Fatalf("timed out! probably deadlock")
+	case <-done:
+		// ok
+	}
+}
+
 const testContextGraph = `
 root: root
 aws_instance.bar

terraform/graph.go

@@ -74,7 +74,11 @@ func (g *Graph) Replace(o, n dag.Vertex) bool {
 	// Go through and update our lookaside to point to the new vertex
 	for k, v := range g.dependableMap {
 		if v == o {
+			if _, ok := n.(GraphNodeDependable); ok {
 				g.dependableMap[k] = n
+			} else {
+				delete(g.dependableMap, k)
+			}
 		}
 	}
 

terraform/graph_test.go

@@ -1,6 +1,7 @@
 package terraform
 
 import (
+	"reflect"
 	"strings"
 	"testing"
 )
@@ -20,7 +21,7 @@ func TestGraphAdd(t *testing.T) {
 
 func TestGraphConnectDependent(t *testing.T) {
 	var g Graph
-	g.Add(&testGraphDependable{VertexName: "a", Mock: []string{"a"}})
+	g.Add(&testGraphDependable{VertexName: "a"})
 	b := g.Add(&testGraphDependable{
 		VertexName:      "b",
 		DependentOnMock: []string{"a"},
@@ -37,10 +38,40 @@ func TestGraphConnectDependent(t *testing.T) {
 	}
 }
 
+func TestGraphReplace_DependableWithNonDependable(t *testing.T) {
+	var g Graph
+	a := g.Add(&testGraphDependable{VertexName: "a"})
+	b := g.Add(&testGraphDependable{
+		VertexName:      "b",
+		DependentOnMock: []string{"a"},
+	})
+	newA := "non-dependable-a"
+
+	if missing := g.ConnectDependent(b); len(missing) > 0 {
+		t.Fatalf("bad: %#v", missing)
+	}
+
+	if !g.Replace(a, newA) {
+		t.Fatalf("failed to replace")
+	}
+
+	c := g.Add(&testGraphDependable{
+		VertexName:      "c",
+		DependentOnMock: []string{"a"},
+	})
+
+	// This should fail by reporting missing, since a node with dependable
+	// name "a" is no longer in the graph.
+	missing := g.ConnectDependent(c)
+	expected := []string{"a"}
+	if !reflect.DeepEqual(expected, missing) {
+		t.Fatalf("expected: %#v, got: %#v", expected, missing)
+	}
+}
+
 type testGraphDependable struct {
 	VertexName      string
 	DependentOnMock []string
-	Mock            []string
 }
 
 func (v *testGraphDependable) Name() string {
@@ -48,7 +79,7 @@ func (v *testGraphDependable) Name() string {
 }
 
 func (v *testGraphDependable) DependableName() []string {
-	return v.Mock
+	return []string{v.VertexName}
 }
 
 func (v *testGraphDependable) DependentOn() []string {

terraform/test-fixtures/refresh-module-orphan/child/grandchild/main.tf

@@ -0,0 +1,3 @@
+resource "aws_instance" "baz" {}
+
+output "id" { value = "${aws_instance.baz.id}" }

terraform/test-fixtures/refresh-module-orphan/child/main.tf

@@ -0,0 +1,10 @@
+module "grandchild" {
+  source = "./grandchild"
+}
+
+resource "aws_instance" "bar" {
+  grandchildid = "${module.grandchild.id}"
+}
+
+output "id" { value = "${aws_instance.bar.id}" }
+output "grandchild_id" { value = "${module.grandchild.id}" }

terraform/test-fixtures/refresh-module-orphan/main.tf

@@ -0,0 +1,10 @@
+/*
+module "child" {
+  source = "./child"
+}
+
+resource "aws_instance" "bar" {
+  childid      = "${module.child.id}"
+  grandchildid = "${module.child.grandchild_id}"
+}
+*/