From 3aaa1e9d04cda706b75df3e733f0ba73ac52432a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Dec 2017 11:03:41 -0500 Subject: [PATCH 1/3] make plans cancellable There was no cancellation context for a plan, so it would always have to run to completion as SIGINT was being swallowed. Move the shutdown channel to the command Meta since it's used in multiple commands. --- command/apply.go | 3 --- command/apply_test.go | 3 +-- command/console.go | 3 --- command/meta.go | 3 +++ command/plan.go | 30 ++++++++++++++++++++++++------ 5 files changed, 28 insertions(+), 14 deletions(-) diff --git a/command/apply.go b/command/apply.go index 18f3f981f..a9588f7e5 100644 --- a/command/apply.go +++ b/command/apply.go @@ -24,9 +24,6 @@ type ApplyCommand struct { // If true, then this apply command will become the "destroy" // command. It is just like apply but only processes a destroy. Destroy bool - - // When this channel is closed, the apply will be cancelled. - ShutdownCh <-chan struct{} } func (c *ApplyCommand) Run(args []string) int { diff --git a/command/apply_test.go b/command/apply_test.go index 808bbbe2a..b30e287d7 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -837,9 +837,8 @@ func TestApply_shutdown(t *testing.T) { Meta: Meta{ testingOverrides: metaOverridesForProvider(p), Ui: ui, + ShutdownCh: shutdownCh, }, - - ShutdownCh: shutdownCh, } p.DiffFn = func( diff --git a/command/console.go b/command/console.go index 823558311..b1361c26f 100644 --- a/command/console.go +++ b/command/console.go @@ -17,9 +17,6 @@ import ( // configuration and actually builds or changes infrastructure. type ConsoleCommand struct { Meta - - // When this channel is closed, the apply will be cancelled. - ShutdownCh <-chan struct{} } func (c *ConsoleCommand) Run(args []string) int { diff --git a/command/meta.go b/command/meta.go index 92ddd8315..27f7765f9 100644 --- a/command/meta.go +++ b/command/meta.go @@ -76,6 +76,9 @@ type Meta struct { // is not suitable, e.g. because of a read-only filesystem. OverrideDataDir string + // When this channel is closed, the command will be cancelled. + ShutdownCh <-chan struct{} + //---------------------------------------------------------- // Protected: commands can set these //---------------------------------------------------------- diff --git a/command/plan.go b/command/plan.go index 757984f8f..ac557bbc8 100644 --- a/command/plan.go +++ b/command/plan.go @@ -104,17 +104,35 @@ func (c *PlanCommand) Run(args []string) int { opReq.Type = backend.OperationTypePlan // Perform the operation - op, err := b.Operation(context.Background(), opReq) + ctx, ctxCancel := context.WithCancel(context.Background()) + defer ctxCancel() + + op, err := b.Operation(ctx, opReq) if err != nil { c.Ui.Error(fmt.Sprintf("Error starting operation: %s", err)) return 1 } - // Wait for the operation to complete - <-op.Done() - if err := op.Err; err != nil { - c.showDiagnostics(err) - return 1 + select { + case <-c.ShutdownCh: + // Cancel our context so we can start gracefully exiting + ctxCancel() + // Notify the user + c.Ui.Output(outputInterrupt) + + // Still get the result, since there is still one + select { + case <-c.ShutdownCh: + c.Ui.Error( + "Two interrupts received. Exiting immediately") + return 1 + case <-op.Done(): + } + case <-op.Done(): + if err := op.Err; err != nil { + c.showDiagnostics(err) + return 1 + } } /* From 6884a07bbae0b4163a2d423249b056015086856a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Dec 2017 11:06:39 -0500 Subject: [PATCH 2/3] use the new Meta.ShutdownCh when building commands --- commands.go | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/commands.go b/commands.go index d65437f66..3335d2cdb 100644 --- a/commands.go +++ b/commands.go @@ -63,6 +63,8 @@ func initCommands(config *Config) { RunningInAutomation: inAutomation, PluginCacheDir: config.PluginCacheDir, OverrideDataDir: dataDir, + + ShutdownCh: makeShutdownCh(), } // The command list is included in the terraform -help @@ -80,23 +82,20 @@ func initCommands(config *Config) { Commands = map[string]cli.CommandFactory{ "apply": func() (cli.Command, error) { return &command.ApplyCommand{ - Meta: meta, - ShutdownCh: makeShutdownCh(), + Meta: meta, }, nil }, "console": func() (cli.Command, error) { return &command.ConsoleCommand{ - Meta: meta, - ShutdownCh: makeShutdownCh(), + Meta: meta, }, nil }, "destroy": func() (cli.Command, error) { return &command.ApplyCommand{ - Meta: meta, - Destroy: true, - ShutdownCh: makeShutdownCh(), + Meta: meta, + Destroy: true, }, nil }, From e2501d7830408d7f44cf62e00a40a58b584e3882 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Fri, 1 Dec 2017 11:27:32 -0500 Subject: [PATCH 3/3] make apply shutdown test completely deterministic Add a shutdown hook to verify that a context has been correctly cancelled, so we can remove the sleep and stop guessing. Add a plan version of the shutdown test as well. --- command/apply.go | 5 +++++ command/apply_test.go | 38 ++++++++++++++++---------------- command/meta.go | 4 ++++ command/plan.go | 6 ++++++ command/plan_test.go | 50 +++++++++++++++++++++++++++++++++++++++++++ 5 files changed, 83 insertions(+), 20 deletions(-) diff --git a/command/apply.go b/command/apply.go index a9588f7e5..3c3d2a189 100644 --- a/command/apply.go +++ b/command/apply.go @@ -183,6 +183,11 @@ func (c *ApplyCommand) Run(args []string) int { // Cancel our context so we can start gracefully exiting ctxCancel() + // notify tests that the command context was canceled + if testShutdownHook != nil { + testShutdownHook() + } + // Notify the user c.Ui.Output(outputInterrupt) diff --git a/command/apply_test.go b/command/apply_test.go index b30e287d7..2707f293c 100644 --- a/command/apply_test.go +++ b/command/apply_test.go @@ -824,14 +824,20 @@ func TestApply_refresh(t *testing.T) { } func TestApply_shutdown(t *testing.T) { - stopped := false - stopCh := make(chan struct{}) - stopReplyCh := make(chan struct{}) + cancelled := false + cancelDone := make(chan struct{}) + testShutdownHook = func() { + cancelled = true + close(cancelDone) + } + defer func() { + testShutdownHook = nil + }() statePath := testTempFile(t) - p := testProvider() shutdownCh := make(chan struct{}) + ui := new(cli.MockUi) c := &ApplyCommand{ Meta: Meta{ @@ -857,10 +863,10 @@ func TestApply_shutdown(t *testing.T) { *terraform.InstanceInfo, *terraform.InstanceState, *terraform.InstanceDiff) (*terraform.InstanceState, error) { - if !stopped { - stopped = true - close(stopCh) - <-stopReplyCh + + if !cancelled { + shutdownCh <- struct{}{} + <-cancelDone } return &terraform.InstanceState{ @@ -871,18 +877,6 @@ func TestApply_shutdown(t *testing.T) { }, nil } - go func() { - <-stopCh - shutdownCh <- struct{}{} - - // This is really dirty, but we have no other way to assure that - // tf.Stop() has been called. This doesn't assure it either, but - // it makes it much more certain. - time.Sleep(50 * time.Millisecond) - - close(stopReplyCh) - }() - args := []string{ "-state", statePath, "-auto-approve", @@ -896,6 +890,10 @@ func TestApply_shutdown(t *testing.T) { t.Fatalf("err: %s", err) } + if !cancelled { + t.Fatal("command not cancelled") + } + state := testStateRead(t, statePath) if state == nil { t.Fatal("state should not be nil") diff --git a/command/meta.go b/command/meta.go index 27f7765f9..729020b2b 100644 --- a/command/meta.go +++ b/command/meta.go @@ -641,3 +641,7 @@ func isAutoVarFile(path string) bool { return strings.HasSuffix(path, ".auto.tfvars") || strings.HasSuffix(path, ".auto.tfvars.json") } + +// testShutdownHook is used by tests to verify that a command context has been +// canceled +var testShutdownHook func() diff --git a/command/plan.go b/command/plan.go index ac557bbc8..f6f4f8f2f 100644 --- a/command/plan.go +++ b/command/plan.go @@ -117,6 +117,12 @@ func (c *PlanCommand) Run(args []string) int { case <-c.ShutdownCh: // Cancel our context so we can start gracefully exiting ctxCancel() + + // notify tests that the command context was canceled + if testShutdownHook != nil { + testShutdownHook() + } + // Notify the user c.Ui.Output(outputInterrupt) diff --git a/command/plan_test.go b/command/plan_test.go index c0f8f98d1..152486c7f 100644 --- a/command/plan_test.go +++ b/command/plan_test.go @@ -831,6 +831,56 @@ func TestPlan_detailedExitcode_emptyDiff(t *testing.T) { } } +func TestPlan_shutdown(t *testing.T) { + cancelled := false + cancelDone := make(chan struct{}) + testShutdownHook = func() { + cancelled = true + close(cancelDone) + } + defer func() { + testShutdownHook = nil + }() + + shutdownCh := make(chan struct{}) + p := testProvider() + ui := new(cli.MockUi) + c := &PlanCommand{ + Meta: Meta{ + testingOverrides: metaOverridesForProvider(p), + Ui: ui, + ShutdownCh: shutdownCh, + }, + } + + p.DiffFn = func( + *terraform.InstanceInfo, + *terraform.InstanceState, + *terraform.ResourceConfig) (*terraform.InstanceDiff, error) { + + if !cancelled { + shutdownCh <- struct{}{} + <-cancelDone + } + + return &terraform.InstanceDiff{ + Attributes: map[string]*terraform.ResourceAttrDiff{ + "ami": &terraform.ResourceAttrDiff{ + New: "bar", + }, + }, + }, nil + } + + if code := c.Run([]string{testFixturePath("apply-shutdown")}); code != 0 { + t.Fatalf("bad: %d\n\n%s", code, ui.ErrorWriter.String()) + } + + if !cancelled { + t.Fatal("command not cancelled") + } +} + const planVarFile = ` foo = "bar" `