From eef933f2a79fec837b943783824c6bc7924b4e64 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Tue, 20 Jun 2017 07:37:32 -0700 Subject: [PATCH 01/12] core: Don't count scaled-out resources twice in the UI This fixes a bug with the new refresh graph behaviour where a resource was being counted twice in the UI on part of being scaled out: * We are no longer transforming refresh nodes without state to plannable resources (the transformer will be removed shortly) * A Quiet flag has been added to EvalDiff and InstanceDiff - this allows for the flagging of a diff that should not be treated as real diff for purposes of planning * When there is no state for a refresh node now, a new path is taken that is similar to plan, but flags Quiet, and does nothing with the diff afterwards. Tests pending - light testing has confirmed this should fix the double count issue, but we should have some tests to actually confirm the bug. --- backend/local/hook_count.go | 5 ++ terraform/diff.go | 5 ++ terraform/eval_diff.go | 14 ++++- terraform/node_resource_refresh.go | 84 +++++++++++++++++++++++++++--- 4 files changed, 99 insertions(+), 9 deletions(-) diff --git a/backend/local/hook_count.go b/backend/local/hook_count.go index 4708159dc..0981ec1c0 100644 --- a/backend/local/hook_count.go +++ b/backend/local/hook_count.go @@ -100,6 +100,11 @@ func (h *CountHook) PostDiff( return terraform.HookActionContinue, nil } + // Don't count anything for a Quiet diff + if d.Quiet { + return terraform.HookActionContinue, nil + } + switch d.ChangeType() { case terraform.DiffDestroyCreate: h.ToRemoveAndAdd += 1 diff --git a/terraform/diff.go b/terraform/diff.go index a9fae6c2c..b179a2766 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -373,6 +373,11 @@ type InstanceDiff struct { // mean to be used for additional data a resource may want to pass through. // The value here must only contain Go primitives and collections. Meta map[string]interface{} + + // Quiet should be set when this diff exists only for purposes of providing a + // diff to various pre-plan or dry-run steps in the graph. A diff with this + // enabled should not be acted on in the plan. + Quiet bool } func (d *InstanceDiff) Lock() { d.mu.Lock() } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 6f09526a4..b5258c9fb 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -81,6 +81,11 @@ type EvalDiff struct { // Resource is needed to fetch the ignore_changes list so we can // filter user-requested ignored attributes from the diff. Resource *config.Resource + + // Quiet is used to indicate that this count should not be used in the UI. + // This is used with refresh nodes on scale-out so that resources do not get + // counted twice in the UI output. + Quiet bool } // TODO: test @@ -157,6 +162,9 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, err } + // Flag quiet to ensure that this resource is skipped in post-diff hooks, such as count, etc. + diff.Quiet = n.Quiet + // Call post-refresh hook err = ctx.Hook(func(h Hook) (HookAction, error) { return h.PostDiff(n.Info, diff) @@ -165,8 +173,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - // Update our output - *n.OutputDiff = diff + // Update our output if we care + if n.OutputDiff != nil { + *n.OutputDiff = diff + } // Update the state if we care if n.OutputState != nil { diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 6ab9df7a2..a9e60b168 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -45,13 +45,6 @@ func (n *NodeRefreshableManagedResource) DynamicExpand(ctx EvalContext) (*Graph, Addr: n.ResourceAddr(), }, - // Switch up any node missing state to a plannable resource. This helps - // catch cases where data sources depend on the counts from this resource - // during a scale out. - &ResourceRefreshPlannableTransformer{ - State: state, - }, - // Add the count orphans to make sure these resources are accounted for // during a scale in. &OrphanResourceCountTransformer{ @@ -100,6 +93,9 @@ func (n *NodeRefreshableManagedResourceInstance) EvalTree() EvalNode { // Eval info is different depending on what kind of resource this is switch mode := n.Addr.Mode; mode { case config.ManagedResourceMode: + if n.ResourceState == nil { + return n.evalTreeManagedScaleInResource() + } return n.evalTreeManagedResource() case config.DataResourceMode: @@ -176,3 +172,77 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResource() EvalN }, } } + +func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedScaleInResource() EvalNode { + // Declare a bunch of variables that are used for state during + // evaluation. Most of this are written to by-address below. + var provider ResourceProvider + var state *InstanceState + var resourceConfig *ResourceConfig + + addr := n.NodeAbstractResource.Addr + stateID := addr.stateId() + info := &InstanceInfo{ + Id: stateID, + Type: addr.Type, + ModulePath: normalizeModulePath(addr.Path), + } + + // Build the resource for eval + resource := &Resource{ + Name: addr.Name, + Type: addr.Type, + CountIndex: addr.Index, + } + if resource.CountIndex < 0 { + resource.CountIndex = 0 + } + + // Determine the dependencies for the state. + stateDeps := n.StateReferences() + + return &EvalSequence{ + Nodes: []EvalNode{ + &EvalInterpolate{ + Config: n.Config.RawConfig.Copy(), + Resource: resource, + Output: &resourceConfig, + }, + &EvalGetProvider{ + Name: n.ProvidedBy()[0], + Output: &provider, + }, + // Re-run validation to catch any errors we missed, e.g. type + // mismatches on computed values. + &EvalValidateResource{ + Provider: &provider, + Config: &resourceConfig, + ResourceName: n.Config.Name, + ResourceType: n.Config.Type, + ResourceMode: n.Config.Mode, + IgnoreWarnings: true, + }, + &EvalReadState{ + Name: stateID, + Output: &state, + }, + &EvalDiff{ + Name: stateID, + Info: info, + Config: &resourceConfig, + Resource: n.Config, + Provider: &provider, + State: &state, + OutputState: &state, + Quiet: true, + }, + &EvalWriteState{ + Name: stateID, + ResourceType: n.Config.Type, + Provider: n.Config.Provider, + Dependencies: stateDeps, + State: &state, + }, + }, + } +} From 45528b221708fd0366f8c118a8572a31153f6efe Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Wed, 21 Jun 2017 09:04:01 -0700 Subject: [PATCH 02/12] core: Instance/EvalDiff.Quiet -> Stub Changed the language of this field to indicate that this diff is not a "real" diff, in that it should not be acted on, versus a "quiet" mode, which would indicate just simply to act silently. --- backend/local/hook_count.go | 4 ++-- terraform/diff.go | 4 ++-- terraform/eval_diff.go | 15 +++++++++------ terraform/node_resource_refresh.go | 2 +- 4 files changed, 14 insertions(+), 11 deletions(-) diff --git a/backend/local/hook_count.go b/backend/local/hook_count.go index 0981ec1c0..9a58e91b4 100644 --- a/backend/local/hook_count.go +++ b/backend/local/hook_count.go @@ -100,8 +100,8 @@ func (h *CountHook) PostDiff( return terraform.HookActionContinue, nil } - // Don't count anything for a Quiet diff - if d.Quiet { + // Don't count anything for a Stub diff + if d.Stub { return terraform.HookActionContinue, nil } diff --git a/terraform/diff.go b/terraform/diff.go index b179a2766..941c0fe9c 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -374,10 +374,10 @@ type InstanceDiff struct { // The value here must only contain Go primitives and collections. Meta map[string]interface{} - // Quiet should be set when this diff exists only for purposes of providing a + // Stub should be set when this diff exists only for purposes of providing a // diff to various pre-plan or dry-run steps in the graph. A diff with this // enabled should not be acted on in the plan. - Quiet bool + Stub bool } func (d *InstanceDiff) Lock() { d.mu.Lock() } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index b5258c9fb..083e63ca1 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -82,10 +82,11 @@ type EvalDiff struct { // filter user-requested ignored attributes from the diff. Resource *config.Resource - // Quiet is used to indicate that this count should not be used in the UI. - // This is used with refresh nodes on scale-out so that resources do not get - // counted twice in the UI output. - Quiet bool + // Stub is used to flag the generated InstanceDiff as a stub. This is used to + // ensure that the node exists to perform interpolations and generate + // computed paths off of, but not as an actual diff where resouces should be + // counted, and not as a diff that should be acted on. + Stub bool } // TODO: test @@ -162,8 +163,10 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - // Flag quiet to ensure that this resource is skipped in post-diff hooks, such as count, etc. - diff.Quiet = n.Quiet + // Flag stub in the diff if set in the eval node. This ensures that this + // resource is skipped in post-diff hooks, such as count, etc, and is usually + // set in a pre-plan phase. + diff.Stub = n.Stub // Call post-refresh hook err = ctx.Hook(func(h Hook) (HookAction, error) { diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index a9e60b168..288290d22 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -234,7 +234,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedScaleInResource( Provider: &provider, State: &state, OutputState: &state, - Quiet: true, + Stub: true, }, &EvalWriteState{ Name: stateID, From 565790d8da0adebc8f1a4c10abe10567aa2345a3 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Wed, 21 Jun 2017 09:15:50 -0700 Subject: [PATCH 03/12] core: Fix scale-out refresh graph test Since the transformer that changed stateless nodes in refresh to NodePlannableResourceInstance is not being used anymore, this test needed to be adjusted to ensure that the right output was expected. --- terraform/node_resource_refresh_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_refresh_test.go b/terraform/node_resource_refresh_test.go index b2ac4d346..8eca1c1f8 100644 --- a/terraform/node_resource_refresh_test.go +++ b/terraform/node_resource_refresh_test.go @@ -59,11 +59,11 @@ func TestNodeRefreshableManagedResourceDynamicExpand_scaleOut(t *testing.T) { actual := g.StringWithNodeTypes() expected := `aws_instance.foo[0] - *terraform.NodeRefreshableManagedResourceInstance aws_instance.foo[1] - *terraform.NodeRefreshableManagedResourceInstance -aws_instance.foo[2] - *terraform.NodePlannableResourceInstance +aws_instance.foo[2] - *terraform.NodeRefreshableManagedResourceInstance root - terraform.graphNodeRoot aws_instance.foo[0] - *terraform.NodeRefreshableManagedResourceInstance aws_instance.foo[1] - *terraform.NodeRefreshableManagedResourceInstance - aws_instance.foo[2] - *terraform.NodePlannableResourceInstance + aws_instance.foo[2] - *terraform.NodeRefreshableManagedResourceInstance ` if expected != actual { t.Fatalf("Expected:\n%s\nGot:\n%s", expected, actual) From f249386c8ab2ff9adcd8e3453bc078bfd24a89b4 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Wed, 21 Jun 2017 09:39:52 -0700 Subject: [PATCH 04/12] core: Test to ensure PostDiff is ignoring stubs Added a test that shows that PostDiff is ignoring diffs where the Stub attribute is set. --- backend/local/hook_count_test.go | 39 ++++++++++++++++++++++++++++++++ 1 file changed, 39 insertions(+) diff --git a/backend/local/hook_count_test.go b/backend/local/hook_count_test.go index 45e6e20d9..488f1a278 100644 --- a/backend/local/hook_count_test.go +++ b/backend/local/hook_count_test.go @@ -241,3 +241,42 @@ func TestCountHookPostDiff_DataSource(t *testing.T) { expected, h) } } + +func TestCountHookPostDiff_IgnoreStub(t *testing.T) { + h := new(CountHook) + + resources := []*terraform.InstanceDiff{ + &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.0": &terraform.ResourceAttrDiff{}, + "foo.1": &terraform.ResourceAttrDiff{}, + "foo.2": &terraform.ResourceAttrDiff{RequiresNew: true}, + }, + Stub: true, + }, + &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "foo.0": &terraform.ResourceAttrDiff{}, + "foo.1": &terraform.ResourceAttrDiff{}, + "foo.2": &terraform.ResourceAttrDiff{RequiresNew: true}, + }, + }, + } + + n := &terraform.InstanceInfo{} + + for _, d := range resources { + h.PostDiff(n, d) + } + + expected := new(CountHook) + expected.ToAdd = 1 + expected.ToChange = 0 + expected.ToRemoveAndAdd = 0 + expected.ToRemove = 0 + + if !reflect.DeepEqual(expected, h) { + t.Fatalf("Expected %#v, got %#v instead.", + expected, h) + } +} From 42ebbc6e0e1c25a1ef21f4b5fa701d59d1515a8a Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 22 Jun 2017 03:43:05 -0700 Subject: [PATCH 05/12] core: ScaleIn should have been ScaleOut We are actually acting on/fixing the scale-out here (ie: new child node from count with no state), not scale-in. --- terraform/node_resource_refresh.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index 288290d22..ec7631ac7 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -94,7 +94,7 @@ func (n *NodeRefreshableManagedResourceInstance) EvalTree() EvalNode { switch mode := n.Addr.Mode; mode { case config.ManagedResourceMode: if n.ResourceState == nil { - return n.evalTreeManagedScaleInResource() + return n.evalTreeManagedScaleOutResource() } return n.evalTreeManagedResource() @@ -173,7 +173,7 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResource() EvalN } } -func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedScaleInResource() EvalNode { +func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedScaleOutResource() EvalNode { // Declare a bunch of variables that are used for state during // evaluation. Most of this are written to by-address below. var provider ResourceProvider From 01e3386e13527dbd75aea90da82df316bf107859 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 22 Jun 2017 03:44:16 -0700 Subject: [PATCH 06/12] core: Add resource count scale-out EvalTree test This test ensures that the right EvalSequence gets set for a refresh node with no state. This will ultimately assert that nodes on scale out will not go down the regular refresh path, which would result in an error due to the nil state - instead, we stub this node so that we get a diff on it that can be used to effect computed/unknown values on interpolations that may depend on this node. --- terraform/node_resource_refresh_test.go | 80 +++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/terraform/node_resource_refresh_test.go b/terraform/node_resource_refresh_test.go index 8eca1c1f8..5a542a9fe 100644 --- a/terraform/node_resource_refresh_test.go +++ b/terraform/node_resource_refresh_test.go @@ -1,8 +1,11 @@ package terraform import ( + "reflect" "sync" "testing" + + "github.com/davecgh/go-spew/spew" ) func TestNodeRefreshableManagedResourceDynamicExpand_scaleOut(t *testing.T) { @@ -152,3 +155,80 @@ root - terraform.graphNodeRoot t.Fatalf("Expected:\n%s\nGot:\n%s", expected, actual) } } + +func TestNodeRefreshableManagedResourceEvalTree_scaleOut(t *testing.T) { + var provider ResourceProvider + var state *InstanceState + var resourceConfig *ResourceConfig + + addr, err := ParseResourceAddress("aws_instance.foo[2]") + if err != nil { + t.Fatalf("bad: %s", err) + } + + m := testModule(t, "refresh-resource-scale-inout") + + n := &NodeRefreshableManagedResourceInstance{ + NodeAbstractResource: &NodeAbstractResource{ + Addr: addr, + Config: m.Config().Resources[0], + }, + } + + actual := n.EvalTree() + + expected := &EvalSequence{ + Nodes: []EvalNode{ + &EvalInterpolate{ + Config: n.Config.RawConfig.Copy(), + Resource: &Resource{ + Name: addr.Name, + Type: addr.Type, + CountIndex: addr.Index, + }, + Output: &resourceConfig, + }, + &EvalGetProvider{ + Name: n.ProvidedBy()[0], + Output: &provider, + }, + &EvalValidateResource{ + Provider: &provider, + Config: &resourceConfig, + ResourceName: n.Config.Name, + ResourceType: n.Config.Type, + ResourceMode: n.Config.Mode, + IgnoreWarnings: true, + }, + &EvalReadState{ + Name: addr.stateId(), + Output: &state, + }, + &EvalDiff{ + Name: addr.stateId(), + Info: &InstanceInfo{ + Id: addr.stateId(), + Type: addr.Type, + ModulePath: normalizeModulePath(addr.Path), + }, + Config: &resourceConfig, + Resource: n.Config, + Provider: &provider, + State: &state, + OutputState: &state, + Stub: true, + }, + &EvalWriteState{ + Name: addr.stateId(), + ResourceType: n.Config.Type, + Provider: n.Config.Provider, + Dependencies: n.StateReferences(), + State: &state, + }, + }, + } + + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Expected:\n\n%s\nGot:\n\n%s\n", spew.Sdump(expected), spew.Sdump(actual)) + } +} From 0e3aedcea39b22920a5465e3b26fea71f1c6f0ae Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Thu, 22 Jun 2017 04:14:35 -0700 Subject: [PATCH 07/12] core: Remove ResourceRefreshPlannableTransformer This transformer is no longer needed, as we are not transforming scale-out resource nodes into plannable nodes anymore, but rather just taking a different eval sequence for resource refresh nodes with no state. --- .../transform_resource_refresh_plannable.go | 55 ------------------- 1 file changed, 55 deletions(-) delete mode 100644 terraform/transform_resource_refresh_plannable.go diff --git a/terraform/transform_resource_refresh_plannable.go b/terraform/transform_resource_refresh_plannable.go deleted file mode 100644 index 35358a318..000000000 --- a/terraform/transform_resource_refresh_plannable.go +++ /dev/null @@ -1,55 +0,0 @@ -package terraform - -import ( - "fmt" - "log" -) - -// ResourceRefreshPlannableTransformer is a GraphTransformer that replaces any -// nodes that don't have state yet exist in config with -// NodePlannableResourceInstance. -// -// This transformer is used when expanding count on managed resource nodes -// during the refresh phase to ensure that data sources that have -// interpolations that depend on resources existing in the graph can be walked -// properly. -type ResourceRefreshPlannableTransformer struct { - // The full global state. - State *State -} - -// Transform implements GraphTransformer for -// ResourceRefreshPlannableTransformer. -func (t *ResourceRefreshPlannableTransformer) Transform(g *Graph) error { -nextVertex: - for _, v := range g.Vertices() { - addr := v.(*NodeRefreshableManagedResourceInstance).Addr - - // Find the state for this address, if there is one - filter := &StateFilter{State: t.State} - results, err := filter.Filter(addr.String()) - if err != nil { - return err - } - - // Check to see if we have a state for this resource. If we do, skip this - // node. - for _, result := range results { - if _, ok := result.Value.(*ResourceState); ok { - continue nextVertex - } - } - // If we don't, convert this resource to a NodePlannableResourceInstance node - // with all of the data we need to make it happen. - log.Printf("[TRACE] No state for %s, converting to NodePlannableResourceInstance", addr.String()) - new := &NodePlannableResourceInstance{ - NodeAbstractResource: v.(*NodeRefreshableManagedResourceInstance).NodeAbstractResource, - } - // Replace the node in the graph - if !g.Replace(v, new) { - return fmt.Errorf("ResourceRefreshPlannableTransformer: Could not replace node %#v with %#v", v, new) - } - } - - return nil -} From b486780cf09ae854fca4c569a938b29fde70f1cf Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 23 Jun 2017 17:35:30 -0700 Subject: [PATCH 08/12] core: evalTreeManagedScaleOutResource -> evalTreeManagedResourceNoState We want to be a bit more explicit here as to when this eval sequence is carried out. The why is now in the top-level comments. --- terraform/node_resource_refresh.go | 15 +++++++++++++-- 1 file changed, 13 insertions(+), 2 deletions(-) diff --git a/terraform/node_resource_refresh.go b/terraform/node_resource_refresh.go index ec7631ac7..cd4fe9201 100644 --- a/terraform/node_resource_refresh.go +++ b/terraform/node_resource_refresh.go @@ -94,7 +94,7 @@ func (n *NodeRefreshableManagedResourceInstance) EvalTree() EvalNode { switch mode := n.Addr.Mode; mode { case config.ManagedResourceMode: if n.ResourceState == nil { - return n.evalTreeManagedScaleOutResource() + return n.evalTreeManagedResourceNoState() } return n.evalTreeManagedResource() @@ -173,7 +173,18 @@ func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResource() EvalN } } -func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedScaleOutResource() EvalNode { +// evalTreeManagedResourceNoState produces an EvalSequence for refresh resource +// nodes that don't have state attached. An example of where this functionality +// is useful is when a resource that already exists in state is being scaled +// out, ie: has its resource count increased. In this case, the scaled out node +// needs to be available to other nodes (namely data sources) that may depend +// on it for proper interpolation, or confusing "index out of range" errors can +// occur. +// +// The steps in this sequence are very similar to the steps carried out in +// plan, but nothing is done with the diff after it is created - it is dropped, +// and its changes are not counted in the UI. +func (n *NodeRefreshableManagedResourceInstance) evalTreeManagedResourceNoState() EvalNode { // Declare a bunch of variables that are used for state during // evaluation. Most of this are written to by-address below. var provider ResourceProvider From 656115387b1d63905b3e3c44cb7657868fe73bd3 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Fri, 23 Jun 2017 17:37:51 -0700 Subject: [PATCH 09/12] core: Simplify TestNodeRefreshableManagedResourceEvalTree_scaleOut This should make things a bit more clear as to what we are doing in the EvalTree scale-out test - ensuring that we get the correct eval sequence for a node with no state through EvalTree. --- terraform/node_resource_refresh_test.go | 56 +------------------------ 1 file changed, 1 insertion(+), 55 deletions(-) diff --git a/terraform/node_resource_refresh_test.go b/terraform/node_resource_refresh_test.go index 5a542a9fe..2c9f69219 100644 --- a/terraform/node_resource_refresh_test.go +++ b/terraform/node_resource_refresh_test.go @@ -157,10 +157,6 @@ root - terraform.graphNodeRoot } func TestNodeRefreshableManagedResourceEvalTree_scaleOut(t *testing.T) { - var provider ResourceProvider - var state *InstanceState - var resourceConfig *ResourceConfig - addr, err := ParseResourceAddress("aws_instance.foo[2]") if err != nil { t.Fatalf("bad: %s", err) @@ -176,57 +172,7 @@ func TestNodeRefreshableManagedResourceEvalTree_scaleOut(t *testing.T) { } actual := n.EvalTree() - - expected := &EvalSequence{ - Nodes: []EvalNode{ - &EvalInterpolate{ - Config: n.Config.RawConfig.Copy(), - Resource: &Resource{ - Name: addr.Name, - Type: addr.Type, - CountIndex: addr.Index, - }, - Output: &resourceConfig, - }, - &EvalGetProvider{ - Name: n.ProvidedBy()[0], - Output: &provider, - }, - &EvalValidateResource{ - Provider: &provider, - Config: &resourceConfig, - ResourceName: n.Config.Name, - ResourceType: n.Config.Type, - ResourceMode: n.Config.Mode, - IgnoreWarnings: true, - }, - &EvalReadState{ - Name: addr.stateId(), - Output: &state, - }, - &EvalDiff{ - Name: addr.stateId(), - Info: &InstanceInfo{ - Id: addr.stateId(), - Type: addr.Type, - ModulePath: normalizeModulePath(addr.Path), - }, - Config: &resourceConfig, - Resource: n.Config, - Provider: &provider, - State: &state, - OutputState: &state, - Stub: true, - }, - &EvalWriteState{ - Name: addr.stateId(), - ResourceType: n.Config.Type, - Provider: n.Config.Provider, - Dependencies: n.StateReferences(), - State: &state, - }, - }, - } + expected := n.evalTreeManagedResourceNoState() if !reflect.DeepEqual(expected, actual) { t.Fatalf("Expected:\n\n%s\nGot:\n\n%s\n", spew.Sdump(expected), spew.Sdump(actual)) From 50cd33f781ec2c9e1ab00caf697acf9ade7b2b7f Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 24 Jun 2017 07:54:40 -0700 Subject: [PATCH 10/12] core: Move Refreh/Plan diff count to general operation test We are changing the behaviour of the "stub" diff operation to just have the pre/post-diff hooks skipped on eval, meaning that the test against CountHook will ultimately be meaningless and fail, hence we need a different test here that tests it on a more general level. --- backend/local/backend_plan_test.go | 68 +++++++++++++++++++ .../local/test-fixtures/plan-scaleout/main.tf | 10 +++ 2 files changed, 78 insertions(+) create mode 100644 backend/local/test-fixtures/plan-scaleout/main.tf diff --git a/backend/local/backend_plan_test.go b/backend/local/backend_plan_test.go index 144260308..b0d6419fa 100644 --- a/backend/local/backend_plan_test.go +++ b/backend/local/backend_plan_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "reflect" "strings" "testing" @@ -207,6 +208,73 @@ func TestLocal_planOutPathNoChange(t *testing.T) { } } +// TestLocal_planScaleOutNoDupeCount tests a Refresh/Plan sequence when a +// resource count is scaled out. The scaled out node needs to exist in the +// graph and run through a plan-style sequence during the refresh phase, but +// can conflate the count if its post-diff count hooks are not skipped. This +// checks to make sure the correct resource count is ultimately given to the +// UI. +func TestLocal_planScaleOutNoDupeCount(t *testing.T) { + b := TestLocal(t) + TestLocalProvider(t, b, "test") + state := &terraform.State{ + Version: 2, + Modules: []*terraform.ModuleState{ + &terraform.ModuleState{ + Path: []string{"root"}, + Resources: map[string]*terraform.ResourceState{ + "test_instance.foo.0": &terraform.ResourceState{ + Type: "test_instance", + Primary: &terraform.InstanceState{ + ID: "bar", + }, + }, + "test_instance.foo.1": &terraform.ResourceState{ + Type: "test_instance", + Primary: &terraform.InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + } + terraform.TestStateFile(t, b.StatePath, state) + + actual := new(CountHook) + b.ContextOpts.Hooks = append(b.ContextOpts.Hooks, actual) + + mod, modCleanup := module.TestTree(t, "./test-fixtures/plan-scaleout") + defer modCleanup() + + outDir := testTempDir(t) + defer os.RemoveAll(outDir) + + op := testOperationPlan() + op.Module = mod + op.PlanRefresh = true + + run, err := b.Operation(context.Background(), op) + if err != nil { + t.Fatalf("bad: %s", err) + } + <-run.Done() + if run.Err != nil { + t.Fatalf("err: %s", err) + } + + expected := new(CountHook) + expected.ToAdd = 1 + expected.ToChange = 0 + expected.ToRemoveAndAdd = 0 + expected.ToRemove = 0 + + if !reflect.DeepEqual(expected, actual) { + t.Fatalf("Expected %#v, got %#v instead.", + expected, actual) + } +} + func testOperationPlan() *backend.Operation { return &backend.Operation{ Type: backend.OperationTypePlan, diff --git a/backend/local/test-fixtures/plan-scaleout/main.tf b/backend/local/test-fixtures/plan-scaleout/main.tf new file mode 100644 index 000000000..4067af592 --- /dev/null +++ b/backend/local/test-fixtures/plan-scaleout/main.tf @@ -0,0 +1,10 @@ +resource "test_instance" "foo" { + count = 3 + ami = "bar" + + # This is here because at some point it caused a test failure + network_interface { + device_index = 0 + description = "Main network interface" + } +} From 5654a676d9ab71ce77028cc95263d3a864aeee2a Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 24 Jun 2017 08:01:17 -0700 Subject: [PATCH 11/12] core: Skip diff hooks for stubs on eval altogether Rather than overloading InstanceDiff with a "Stub" attribute that is going to be largely meaningless, we are just going to skip pre/post-diff hooks altogether. This is under the notion that we will eventually not need to "stub" a diff for scale-out, stateless nodes on refresh at all, so diff behaviour won't be necessary at that point, so we should not assume that hooks will run at this stage anyway. Also as part of this removed the CountHook test that is now failing because CountHook is out of scope of the new behaviour. --- backend/local/hook_count.go | 5 ---- backend/local/hook_count_test.go | 39 -------------------------------- terraform/diff.go | 5 ---- terraform/eval_diff.go | 29 ++++++++++++------------ 4 files changed, 14 insertions(+), 64 deletions(-) diff --git a/backend/local/hook_count.go b/backend/local/hook_count.go index 9a58e91b4..4708159dc 100644 --- a/backend/local/hook_count.go +++ b/backend/local/hook_count.go @@ -100,11 +100,6 @@ func (h *CountHook) PostDiff( return terraform.HookActionContinue, nil } - // Don't count anything for a Stub diff - if d.Stub { - return terraform.HookActionContinue, nil - } - switch d.ChangeType() { case terraform.DiffDestroyCreate: h.ToRemoveAndAdd += 1 diff --git a/backend/local/hook_count_test.go b/backend/local/hook_count_test.go index 488f1a278..45e6e20d9 100644 --- a/backend/local/hook_count_test.go +++ b/backend/local/hook_count_test.go @@ -241,42 +241,3 @@ func TestCountHookPostDiff_DataSource(t *testing.T) { expected, h) } } - -func TestCountHookPostDiff_IgnoreStub(t *testing.T) { - h := new(CountHook) - - resources := []*terraform.InstanceDiff{ - &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "foo.0": &terraform.ResourceAttrDiff{}, - "foo.1": &terraform.ResourceAttrDiff{}, - "foo.2": &terraform.ResourceAttrDiff{RequiresNew: true}, - }, - Stub: true, - }, - &terraform.InstanceDiff{ - Attributes: map[string]*terraform.ResourceAttrDiff{ - "foo.0": &terraform.ResourceAttrDiff{}, - "foo.1": &terraform.ResourceAttrDiff{}, - "foo.2": &terraform.ResourceAttrDiff{RequiresNew: true}, - }, - }, - } - - n := &terraform.InstanceInfo{} - - for _, d := range resources { - h.PostDiff(n, d) - } - - expected := new(CountHook) - expected.ToAdd = 1 - expected.ToChange = 0 - expected.ToRemoveAndAdd = 0 - expected.ToRemove = 0 - - if !reflect.DeepEqual(expected, h) { - t.Fatalf("Expected %#v, got %#v instead.", - expected, h) - } -} diff --git a/terraform/diff.go b/terraform/diff.go index 941c0fe9c..a9fae6c2c 100644 --- a/terraform/diff.go +++ b/terraform/diff.go @@ -373,11 +373,6 @@ type InstanceDiff struct { // mean to be used for additional data a resource may want to pass through. // The value here must only contain Go primitives and collections. Meta map[string]interface{} - - // Stub should be set when this diff exists only for purposes of providing a - // diff to various pre-plan or dry-run steps in the graph. A diff with this - // enabled should not be acted on in the plan. - Stub bool } func (d *InstanceDiff) Lock() { d.mu.Lock() } diff --git a/terraform/eval_diff.go b/terraform/eval_diff.go index 083e63ca1..c35f9083f 100644 --- a/terraform/eval_diff.go +++ b/terraform/eval_diff.go @@ -96,11 +96,13 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { provider := *n.Provider // Call pre-diff hook - err := ctx.Hook(func(h Hook) (HookAction, error) { - return h.PreDiff(n.Info, state) - }) - if err != nil { - return nil, err + if !n.Stub { + err := ctx.Hook(func(h Hook) (HookAction, error) { + return h.PreDiff(n.Info, state) + }) + if err != nil { + return nil, err + } } // The state for the diff must never be nil @@ -163,17 +165,14 @@ func (n *EvalDiff) Eval(ctx EvalContext) (interface{}, error) { return nil, err } - // Flag stub in the diff if set in the eval node. This ensures that this - // resource is skipped in post-diff hooks, such as count, etc, and is usually - // set in a pre-plan phase. - diff.Stub = n.Stub - // Call post-refresh hook - err = ctx.Hook(func(h Hook) (HookAction, error) { - return h.PostDiff(n.Info, diff) - }) - if err != nil { - return nil, err + if !n.Stub { + err = ctx.Hook(func(h Hook) (HookAction, error) { + return h.PostDiff(n.Info, diff) + }) + if err != nil { + return nil, err + } } // Update our output if we care From 0ca5eab545a9f8bfaf22fb091a280758d1fa4529 Mon Sep 17 00:00:00 2001 From: Chris Marchesi Date: Sat, 24 Jun 2017 22:41:12 -0700 Subject: [PATCH 12/12] core: Context-level test for stub EvalDiff Added a new test that ensures that pre/post-diff hooks are not called when EvalDiff is run with Stub set, tested through a full refresh run. This helps test the expected behaviour of EvalDiff itself, versus the end result of the diff being counted in a plan, which is what the TestLocal_planScaleOutNoDupeCount test in backend/local checks. --- terraform/context_refresh_test.go | 60 +++++++++++++++++++++++++++++++ 1 file changed, 60 insertions(+) diff --git a/terraform/context_refresh_test.go b/terraform/context_refresh_test.go index 0a977b4cc..22b959315 100644 --- a/terraform/context_refresh_test.go +++ b/terraform/context_refresh_test.go @@ -1049,3 +1049,63 @@ func TestContext2Validate(t *testing.T) { t.Fatalf("bad: %s", e) } } + +// TestContext2Refresh_noDiffHookOnScaleOut tests to make sure that +// pre/post-diff hooks are not called when running EvalDiff on scale-out nodes +// (nodes with no state). The effect here is to make sure that the diffs - +// which only exist for interpolation of parallel resources or data sources - +// do not end up being counted in the UI. +func TestContext2Refresh_noDiffHookOnScaleOut(t *testing.T) { + h := new(MockHook) + p := testProvider("aws") + m := testModule(t, "refresh-resource-scale-inout") + p.RefreshFn = nil + + state := &State{ + Modules: []*ModuleState{ + &ModuleState{ + Path: rootModulePath, + Resources: map[string]*ResourceState{ + "aws_instance.foo.0": &ResourceState{ + Type: "aws_instance", + Deposed: []*InstanceState{ + &InstanceState{ + ID: "foo", + }, + }, + }, + "aws_instance.foo.1": &ResourceState{ + Type: "aws_instance", + Deposed: []*InstanceState{ + &InstanceState{ + ID: "bar", + }, + }, + }, + }, + }, + }, + } + + ctx := testContext2(t, &ContextOpts{ + Module: m, + Hooks: []Hook{h}, + ProviderResolver: ResourceProviderResolverFixed( + map[string]ResourceProviderFactory{ + "aws": testProviderFuncFixed(p), + }, + ), + State: state, + }) + + _, err := ctx.Refresh() + if err != nil { + t.Fatalf("bad: %s", err) + } + if h.PreDiffCalled { + t.Fatal("PreDiff should not have been called") + } + if h.PostDiffCalled { + t.Fatal("PostDiff should not have been called") + } +}