From 2be72cfe03fdb4c4fec164ab8d7133108addf85e Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 17:32:09 -0700 Subject: [PATCH 01/10] terraform: Stop API added to ResourceProvider --- terraform/resource_provider.go | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/terraform/resource_provider.go b/terraform/resource_provider.go index 542f14a61..1a68c8699 100644 --- a/terraform/resource_provider.go +++ b/terraform/resource_provider.go @@ -47,6 +47,26 @@ type ResourceProvider interface { // knows how to manage. Resources() []ResourceType + // Stop is called when the provider should halt any in-flight actions. + // + // This can be used to make a nicer Ctrl-C experience for Terraform. + // Even if this isn't implemented to do anything (just returns nil), + // Terraform will still cleanly stop after the currently executing + // graph node is complete. However, this API can be used to make more + // efficient halts. + // + // Stop doesn't have to and shouldn't block waiting for in-flight actions + // to complete. It should take any action it wants and return immediately + // acknowledging it has received the stop request. Terraform core will + // automatically not make any further API calls to the provider soon + // after Stop is called (technically exactly once the currently executing + // graph nodes are complete). + // + // The error returned, if non-nil, is assumed to mean that signaling the + // stop somehow failed and that the user should expect potentially waiting + // a longer period of time. + Stop() error + /********************************************************************* * Functions related to individual resources *********************************************************************/ From 7e2582c47b5279cbe54e44c4aa96e7549d3eee0b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 17:33:11 -0700 Subject: [PATCH 02/10] terraform: implement Stop in the mock and shadow --- terraform/resource_provider_mock.go | 15 +++++++++++++++ terraform/shadow_resource_provider.go | 9 +++++++++ 2 files changed, 24 insertions(+) diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index f8acfafa9..f5315339f 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -56,6 +56,9 @@ type MockResourceProvider struct { ReadDataDiffFn func(*InstanceInfo, *ResourceConfig) (*InstanceDiff, error) ReadDataDiffReturn *InstanceDiff ReadDataDiffReturnError error + StopCalled bool + StopFn func() error + StopReturnError error DataSourcesCalled bool DataSourcesReturn []DataSource ValidateCalled bool @@ -141,6 +144,18 @@ func (p *MockResourceProvider) Configure(c *ResourceConfig) error { return p.ConfigureReturnError } +func (p *MockResourceProvider) Stop() error { + p.Lock() + defer p.Unlock() + + p.StopCalled = true + if p.StopFn != nil { + return p.StopFn() + } + + return p.StopReturnError +} + func (p *MockResourceProvider) Apply( info *InstanceInfo, state *InstanceState, diff --git a/terraform/shadow_resource_provider.go b/terraform/shadow_resource_provider.go index 0c79edf75..72bb49cf5 100644 --- a/terraform/shadow_resource_provider.go +++ b/terraform/shadow_resource_provider.go @@ -107,6 +107,10 @@ func (p *shadowResourceProviderReal) Configure(c *ResourceConfig) error { return err } +func (p *shadowResourceProviderReal) Stop() error { + return p.ResourceProvider.Stop() +} + func (p *shadowResourceProviderReal) ValidateResource( t string, c *ResourceConfig) ([]string, []error) { key := t @@ -441,6 +445,11 @@ func (p *shadowResourceProviderShadow) Configure(c *ResourceConfig) error { return result.Result } +// Stop returns immediately. +func (p *shadowResourceProviderShadow) Stop() error { + return nil +} + func (p *shadowResourceProviderShadow) ValidateResource(t string, c *ResourceConfig) ([]string, []error) { // Unique key key := t From 0cca4fc09310c87eacf3b2c8b8ee42f036868cf6 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 17:55:45 -0700 Subject: [PATCH 03/10] terraform: Context.Stop() calls Stop on providers if running --- terraform/context.go | 44 +++++++++++++++++++++++++++++++++ terraform/context_apply_test.go | 4 +++ 2 files changed, 48 insertions(+) diff --git a/terraform/context.go b/terraform/context.go index 7714455c9..e8f821127 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -105,6 +105,7 @@ type Context struct { parallelSem Semaphore providerInputConfig map[string]map[string]interface{} runCh <-chan struct{} + stopCh chan struct{} shadowErr error } @@ -587,6 +588,9 @@ func (c *Context) Stop() { // Tell the hook we want to stop c.sh.Stop() + // Close the stop channel + close(c.stopCh) + // Wait for us to stop c.l.Unlock() <-ch @@ -672,6 +676,9 @@ func (c *Context) acquireRun() chan<- struct{} { ch := make(chan struct{}) c.runCh = ch + // Reset the stop channel so we can watch that + c.stopCh = make(chan struct{}) + // Reset the stop hook so we're not stopped c.sh.Reset() @@ -687,6 +694,7 @@ func (c *Context) releaseRun(ch chan<- struct{}) { close(ch) c.runCh = nil + c.stopCh = nil } func (c *Context) walk( @@ -714,9 +722,16 @@ func (c *Context) walk( log.Printf("[DEBUG] Starting graph walk: %s", operation.String()) walker := &ContextGraphWalker{Context: realCtx, Operation: operation} + // Watch for a stop so we can call the provider Stop() API. + doneCh := make(chan struct{}) + go c.watchStop(walker, c.stopCh, doneCh) + // Walk the real graph, this will block until it completes realErr := graph.Walk(walker) + // Close the done channel so the watcher stops + close(doneCh) + // If we have a shadow graph and we interrupted the real graph, then // we just close the shadow and never verify it. It is non-trivial to // recreate the exact execution state up until an interruption so this @@ -796,6 +811,35 @@ func (c *Context) walk( return walker, realErr } +func (c *Context) watchStop(walker *ContextGraphWalker, stopCh, doneCh <-chan struct{}) { + // Wait for a stop or completion + select { + case <-stopCh: + // Stop was triggered. Fall out of the select + case <-doneCh: + // Done, just exit completely + return + } + + // If we're here, we're stopped, trigger the call. + + // Copy the providers so that a misbehaved blocking Stop doesn't + // completely hang Terraform. + walker.providerLock.Lock() + ps := make([]ResourceProvider, 0, len(walker.providerCache)) + for _, p := range walker.providerCache { + ps = append(ps, p) + } + defer walker.providerLock.Unlock() + + for _, p := range ps { + // We ignore the error for now since there isn't any reasonable + // action to take if there is an error here, since the stop is still + // advisory: Terraform will exit once the graph node completes. + p.Stop() + } +} + // parseVariableAsHCL parses the value of a single variable as would have been specified // on the command line via -var or in an environment variable named TF_VAR_x, where x is // the name of the variable. In order to get around the restriction of HCL requiring a diff --git a/terraform/context_apply_test.go b/terraform/context_apply_test.go index e90c6e941..0dfd47ec1 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1043,6 +1043,10 @@ func TestContext2Apply_cancel(t *testing.T) { if actual != expected { t.Fatalf("bad: \n%s", actual) } + + if !p.StopCalled { + t.Fatal("stop should be called") + } } func TestContext2Apply_compute(t *testing.T) { From 8c11f137f5d8cc113a38cec129ac57e226b1c550 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 18:07:17 -0700 Subject: [PATCH 04/10] helper/schema: support Stop() --- helper/schema/provider.go | 44 ++++++++++++++++++++++++++++ helper/schema/provider_test.go | 53 ++++++++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 546794f40..636ea5518 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "sort" + "sync" "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/terraform" @@ -49,6 +50,10 @@ type Provider struct { ConfigureFunc ConfigureFunc meta interface{} + + stopCh chan struct{} // stopCh is closed when Stop is called + stopped bool // set to true once stopped to avoid double close of stopCh + stopLock sync.Mutex } // ConfigureFunc is the function used to configure a Provider. @@ -104,6 +109,45 @@ func (p *Provider) SetMeta(v interface{}) { p.meta = v } +// Stopped reports whether the provider has been stopped or not. +func (p *Provider) Stopped() bool { + p.stopLock.Lock() + defer p.stopLock.Unlock() + return p.stopped +} + +// StopCh returns a channel that is closed once the provider is stopped. +func (p *Provider) StopCh() <-chan struct{} { + p.stopLock.Lock() + defer p.stopLock.Unlock() + + if p.stopCh == nil { + p.stopCh = make(chan struct{}) + } + + return p.stopCh +} + +// Stop implementation of terraform.ResourceProvider interface. +func (p *Provider) Stop() error { + p.stopLock.Lock() + defer p.stopLock.Unlock() + + // Close the stop channel and mark as stopped if we haven't + if !p.stopped { + // Initialize the stop channel so future calls to StopCh work + if p.stopCh == nil { + p.stopCh = make(chan struct{}) + } + + // Close and mark + close(p.stopCh) + p.stopped = true + } + + return nil +} + // Input implementation of terraform.ResourceProvider interface. func (p *Provider) Input( input terraform.UIInput, diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index aa8a787bf..cde72d86b 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -4,6 +4,7 @@ import ( "fmt" "reflect" "testing" + "time" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/terraform" @@ -328,3 +329,55 @@ func TestProviderMeta(t *testing.T) { t.Fatalf("bad: %#v", v) } } + +func TestProviderStop(t *testing.T) { + var p Provider + + if p.Stopped() { + t.Fatal("should not be stopped") + } + + // Verify stopch blocks + ch := p.StopCh() + select { + case <-ch: + t.Fatal("should not be stopped") + case <-time.After(10 * time.Millisecond): + } + + // Stop it + if err := p.Stop(); err != nil { + t.Fatalf("err: %s", err) + } + + // Verify + if !p.Stopped() { + t.Fatal("should be stopped") + } + + select { + case <-ch: + case <-time.After(10 * time.Millisecond): + t.Fatal("should be stopped") + } +} + +func TestProviderStop_stopFirst(t *testing.T) { + var p Provider + + // Stop it + if err := p.Stop(); err != nil { + t.Fatalf("err: %s", err) + } + + // Verify + if !p.Stopped() { + t.Fatal("should be stopped") + } + + select { + case <-p.StopCh(): + case <-time.After(10 * time.Millisecond): + t.Fatal("should be stopped") + } +} From 89647745b0985a1dd74d04418f7436c63363f7d7 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 18:25:47 -0700 Subject: [PATCH 05/10] helper/resource: StopCh as a helper for provider stopCh + timeout --- helper/resource/stop.go | 40 +++++++++++++++++++++++ helper/resource/stop_test.go | 62 ++++++++++++++++++++++++++++++++++++ 2 files changed, 102 insertions(+) create mode 100644 helper/resource/stop.go create mode 100644 helper/resource/stop_test.go diff --git a/helper/resource/stop.go b/helper/resource/stop.go new file mode 100644 index 000000000..b3bfa776c --- /dev/null +++ b/helper/resource/stop.go @@ -0,0 +1,40 @@ +package resource + +import ( + "time" +) + +// StopCh is used to construct a channel that is closed when the provider +// stopCh is closed, or a timeout is reached. +// +// The first return value is the channel that is closed when the operation +// should stop. The second return value should be closed when the operation +// completes so that the stop channel is never closed and to cleanup the +// goroutine watching the channels. +func StopCh(stopCh <-chan struct{}, max time.Duration) (<-chan struct{}, chan<- struct{}) { + ch := make(chan struct{}) + doneCh := make(chan struct{}) + + // If we have a max of 0 then it is unlimited. A nil channel blocks on + // receive forever so this ensures that behavior. + var timeCh <-chan time.Time + if max > 0 { + timeCh = time.After(max) + } + + // Start a goroutine to watch the various cases of cancellation + go func() { + select { + case <-doneCh: + // If we are done, don't trigger the cancel + return + case <-timeCh: + case <-stopCh: + } + + // Trigger the cancel + close(ch) + }() + + return ch, doneCh +} diff --git a/helper/resource/stop_test.go b/helper/resource/stop_test.go new file mode 100644 index 000000000..839f4af9a --- /dev/null +++ b/helper/resource/stop_test.go @@ -0,0 +1,62 @@ +package resource + +import ( + "testing" + "time" +) + +func TestStopCh_stop(t *testing.T) { + stopCh := make(chan struct{}) + ch, _ := StopCh(stopCh, 0) + + // ch should block + select { + case <-ch: + t.Fatal("ch should block") + case <-time.After(10 * time.Millisecond): + } + + // Close the stop channel + close(stopCh) + + // ch should return + select { + case <-ch: + case <-time.After(10 * time.Millisecond): + t.Fatal("ch should not block") + } +} + +func TestStopCh_done(t *testing.T) { + stopCh := make(chan struct{}) + ch, doneCh := StopCh(stopCh, 0) + + // ch should block + select { + case <-ch: + t.Fatal("ch should block") + case <-time.After(10 * time.Millisecond): + } + + // Close the done channel + close(doneCh) + + // ch should block + select { + case <-ch: + t.Fatal("ch should block") + case <-time.After(10 * time.Millisecond): + } +} + +func TestStopCh_timeout(t *testing.T) { + stopCh := make(chan struct{}) + ch, _ := StopCh(stopCh, 10*time.Millisecond) + + // ch should return + select { + case <-ch: + case <-time.After(100 * time.Millisecond): + t.Fatal("ch should not block") + } +} From 9089aa24d5058b298cdd9a4dd86ce60381af916c Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 18:26:06 -0700 Subject: [PATCH 06/10] providers/azurerm: convert to Stop() usage as example --- builtin/providers/azurerm/config.go | 8 +++ builtin/providers/azurerm/provider.go | 49 +++++++++++-------- .../azurerm/resource_arm_storage_account.go | 24 ++------- 3 files changed, 41 insertions(+), 40 deletions(-) diff --git a/builtin/providers/azurerm/config.go b/builtin/providers/azurerm/config.go index d860c7f1b..460355604 100644 --- a/builtin/providers/azurerm/config.go +++ b/builtin/providers/azurerm/config.go @@ -5,6 +5,7 @@ import ( "log" "net/http" "net/http/httputil" + "time" "github.com/Azure/azure-sdk-for-go/arm/cdn" "github.com/Azure/azure-sdk-for-go/arm/compute" @@ -19,6 +20,7 @@ import ( mainStorage "github.com/Azure/azure-sdk-for-go/storage" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/azure" + "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" riviera "github.com/jen20/riviera/azure" ) @@ -30,6 +32,8 @@ type ArmClient struct { tenantId string subscriptionId string + stopCh <-chan struct{} // From the provider + rivieraClient *riviera.Client availSetClient compute.AvailabilitySetsClient @@ -485,3 +489,7 @@ func (armClient *ArmClient) getQueueServiceClientForStorageAccount(resourceGroup queueClient := storageClient.GetQueueService() return &queueClient, true, nil } + +func (armClient *ArmClient) CancelCh(max time.Duration) (<-chan struct{}, chan<- struct{}) { + return resource.StopCh(armClient.stopCh, max) +} diff --git a/builtin/providers/azurerm/provider.go b/builtin/providers/azurerm/provider.go index c65679fa4..5415fb985 100644 --- a/builtin/providers/azurerm/provider.go +++ b/builtin/providers/azurerm/provider.go @@ -17,7 +17,8 @@ import ( // Provider returns a terraform.ResourceProvider. func Provider() terraform.ResourceProvider { - return &schema.Provider{ + var p *schema.Provider + p = &schema.Provider{ Schema: map[string]*schema.Schema{ "subscription_id": { Type: schema.TypeString, @@ -104,8 +105,10 @@ func Provider() terraform.ResourceProvider { "azurerm_sql_firewall_rule": resourceArmSqlFirewallRule(), "azurerm_sql_server": resourceArmSqlServer(), }, - ConfigureFunc: providerConfigure, + ConfigureFunc: providerConfigure(p), } + + return p } // Config is the configuration structure used to instantiate a @@ -140,29 +143,33 @@ func (c *Config) validate() error { return err.ErrorOrNil() } -func providerConfigure(d *schema.ResourceData) (interface{}, error) { - config := &Config{ - SubscriptionID: d.Get("subscription_id").(string), - ClientID: d.Get("client_id").(string), - ClientSecret: d.Get("client_secret").(string), - TenantID: d.Get("tenant_id").(string), - } +func providerConfigure(p *schema.Provider) schema.ConfigureFunc { + return func(d *schema.ResourceData) (interface{}, error) { + config := &Config{ + SubscriptionID: d.Get("subscription_id").(string), + ClientID: d.Get("client_id").(string), + ClientSecret: d.Get("client_secret").(string), + TenantID: d.Get("tenant_id").(string), + } - if err := config.validate(); err != nil { - return nil, err - } + if err := config.validate(); err != nil { + return nil, err + } - client, err := config.getArmClient() - if err != nil { - return nil, err - } + client, err := config.getArmClient() + if err != nil { + return nil, err + } - err = registerAzureResourceProvidersWithSubscription(client.rivieraClient) - if err != nil { - return nil, err - } + client.stopCh = p.StopCh() - return client, nil + err = registerAzureResourceProvidersWithSubscription(client.rivieraClient) + if err != nil { + return nil, err + } + + return client, nil + } } func registerProviderWithSubscription(providerName string, client *riviera.Client) error { diff --git a/builtin/providers/azurerm/resource_arm_storage_account.go b/builtin/providers/azurerm/resource_arm_storage_account.go index e31f3a0db..9ce8aae5f 100644 --- a/builtin/providers/azurerm/resource_arm_storage_account.go +++ b/builtin/providers/azurerm/resource_arm_storage_account.go @@ -11,7 +11,6 @@ import ( "github.com/Azure/azure-sdk-for-go/arm/storage" "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/helper/schema" - "github.com/hashicorp/terraform/helper/signalwrapper" "github.com/hashicorp/terraform/helper/validation" ) @@ -192,24 +191,11 @@ func resourceArmStorageAccountCreate(d *schema.ResourceData, meta interface{}) e opts.Properties.AccessTier = storage.AccessTier(accessTier.(string)) } - // Create the storage account. We wrap this so that it is cancellable - // with a Ctrl-C since this can take a LONG time. - wrap := signalwrapper.Run(func(cancelCh <-chan struct{}) error { - _, err := storageClient.Create(resourceGroupName, storageAccountName, opts, cancelCh) - return err - }) - - // Check the result of the wrapped function. - var createErr error - select { - case <-time.After(1 * time.Hour): - // An hour is way above the expected P99 for this API call so - // we premature cancel and error here. - createErr = wrap.Cancel() - case createErr = <-wrap.ErrCh: - // Successfully ran (but perhaps not successfully completed) - // the function. - } + // Create + cancelCh, doneCh := client.CancelCh(1 * time.Hour) + _, createErr := storageClient.Create( + resourceGroupName, storageAccountName, opts, cancelCh) + close(doneCh) // The only way to get the ID back apparently is to read the resource again read, err := storageClient.GetProperties(resourceGroupName, storageAccountName) From 43b5818b550ac340e6e809c70d290672d6871e8b Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Sun, 23 Oct 2016 18:39:52 -0700 Subject: [PATCH 07/10] plugin: implement ResourceProvider.Stop --- plugin/resource_provider.go | 28 ++++++++++++++++ plugin/resource_provider_test.go | 55 ++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+) diff --git a/plugin/resource_provider.go b/plugin/resource_provider.go index b711864a1..473f78601 100644 --- a/plugin/resource_provider.go +++ b/plugin/resource_provider.go @@ -28,6 +28,19 @@ type ResourceProvider struct { Client *rpc.Client } +func (p *ResourceProvider) Stop() error { + var resp ResourceProviderStopResponse + err := p.Client.Call("Plugin.Stop", new(interface{}), &resp) + if err != nil { + return err + } + if resp.Error != nil { + err = resp.Error + } + + return err +} + func (p *ResourceProvider) Input( input terraform.UIInput, c *terraform.ResourceConfig) (*terraform.ResourceConfig, error) { @@ -295,6 +308,10 @@ type ResourceProviderServer struct { Provider terraform.ResourceProvider } +type ResourceProviderStopResponse struct { + Error *plugin.BasicError +} + type ResourceProviderConfigureResponse struct { Error *plugin.BasicError } @@ -390,6 +407,17 @@ type ResourceProviderValidateResourceResponse struct { Errors []*plugin.BasicError } +func (s *ResourceProviderServer) Stop( + _ interface{}, + reply *ResourceProviderStopResponse) error { + err := s.Provider.Stop() + *reply = ResourceProviderStopResponse{ + Error: plugin.NewBasicError(err), + } + + return nil +} + func (s *ResourceProviderServer) Input( args *ResourceProviderInputArgs, reply *ResourceProviderInputResponse) error { diff --git a/plugin/resource_provider_test.go b/plugin/resource_provider_test.go index 41997b132..9c2d43dab 100644 --- a/plugin/resource_provider_test.go +++ b/plugin/resource_provider_test.go @@ -14,6 +14,61 @@ func TestResourceProvider_impl(t *testing.T) { var _ terraform.ResourceProvider = new(ResourceProvider) } +func TestResourceProvider_stop(t *testing.T) { + // Create a mock provider + p := new(terraform.MockResourceProvider) + client, _ := plugin.TestPluginRPCConn(t, pluginMap(&ServeOpts{ + ProviderFunc: testProviderFixed(p), + })) + defer client.Close() + + // Request the provider + raw, err := client.Dispense(ProviderPluginName) + if err != nil { + t.Fatalf("err: %s", err) + } + provider := raw.(terraform.ResourceProvider) + + // Stop + e := provider.Stop() + if !p.StopCalled { + t.Fatal("stop should be called") + } + if e != nil { + t.Fatalf("bad: %#v", e) + } +} + +func TestResourceProvider_stopErrors(t *testing.T) { + p := new(terraform.MockResourceProvider) + p.StopReturnError = errors.New("foo") + + // Create a mock provider + client, _ := plugin.TestPluginRPCConn(t, pluginMap(&ServeOpts{ + ProviderFunc: testProviderFixed(p), + })) + defer client.Close() + + // Request the provider + raw, err := client.Dispense(ProviderPluginName) + if err != nil { + t.Fatalf("err: %s", err) + } + provider := raw.(terraform.ResourceProvider) + + // Stop + e := provider.Stop() + if !p.StopCalled { + t.Fatal("stop should be called") + } + if e == nil { + t.Fatal("should have error") + } + if e.Error() != "foo" { + t.Fatalf("bad: %s", e) + } +} + func TestResourceProvider_input(t *testing.T) { // Create a mock provider p := new(terraform.MockResourceProvider) From 86eb30b8a220639917ca9e867fa80fd8bc60a4a8 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 25 Oct 2016 11:29:24 -0700 Subject: [PATCH 08/10] helper/schema: expose stop information as a Context --- builtin/providers/azurerm/config.go | 9 +--- builtin/providers/azurerm/provider.go | 2 +- .../azurerm/resource_arm_storage_account.go | 6 +-- helper/schema/provider.go | 48 ++++++++----------- helper/schema/provider_test.go | 4 +- 5 files changed, 27 insertions(+), 42 deletions(-) diff --git a/builtin/providers/azurerm/config.go b/builtin/providers/azurerm/config.go index 460355604..3638c6a51 100644 --- a/builtin/providers/azurerm/config.go +++ b/builtin/providers/azurerm/config.go @@ -1,11 +1,11 @@ package azurerm import ( + "context" "fmt" "log" "net/http" "net/http/httputil" - "time" "github.com/Azure/azure-sdk-for-go/arm/cdn" "github.com/Azure/azure-sdk-for-go/arm/compute" @@ -20,7 +20,6 @@ import ( mainStorage "github.com/Azure/azure-sdk-for-go/storage" "github.com/Azure/go-autorest/autorest" "github.com/Azure/go-autorest/autorest/azure" - "github.com/hashicorp/terraform/helper/resource" "github.com/hashicorp/terraform/terraform" riviera "github.com/jen20/riviera/azure" ) @@ -32,7 +31,7 @@ type ArmClient struct { tenantId string subscriptionId string - stopCh <-chan struct{} // From the provider + StopContext context.Context rivieraClient *riviera.Client @@ -489,7 +488,3 @@ func (armClient *ArmClient) getQueueServiceClientForStorageAccount(resourceGroup queueClient := storageClient.GetQueueService() return &queueClient, true, nil } - -func (armClient *ArmClient) CancelCh(max time.Duration) (<-chan struct{}, chan<- struct{}) { - return resource.StopCh(armClient.stopCh, max) -} diff --git a/builtin/providers/azurerm/provider.go b/builtin/providers/azurerm/provider.go index 5415fb985..12fd48f45 100644 --- a/builtin/providers/azurerm/provider.go +++ b/builtin/providers/azurerm/provider.go @@ -161,7 +161,7 @@ func providerConfigure(p *schema.Provider) schema.ConfigureFunc { return nil, err } - client.stopCh = p.StopCh() + client.StopContext = p.StopContext() err = registerAzureResourceProvidersWithSubscription(client.rivieraClient) if err != nil { diff --git a/builtin/providers/azurerm/resource_arm_storage_account.go b/builtin/providers/azurerm/resource_arm_storage_account.go index 9ce8aae5f..7e196980b 100644 --- a/builtin/providers/azurerm/resource_arm_storage_account.go +++ b/builtin/providers/azurerm/resource_arm_storage_account.go @@ -1,6 +1,7 @@ package azurerm import ( + "context" "fmt" "log" "net/http" @@ -192,10 +193,9 @@ func resourceArmStorageAccountCreate(d *schema.ResourceData, meta interface{}) e } // Create - cancelCh, doneCh := client.CancelCh(1 * time.Hour) + cancelCtx, _ := context.WithTimeout(client.StopContext, 1*time.Hour) _, createErr := storageClient.Create( - resourceGroupName, storageAccountName, opts, cancelCh) - close(doneCh) + resourceGroupName, storageAccountName, opts, cancelCtx.Done()) // The only way to get the ID back apparently is to read the resource again read, err := storageClient.GetProperties(resourceGroupName, storageAccountName) diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 636ea5518..5b50d54a1 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -1,6 +1,7 @@ package schema import ( + "context" "errors" "fmt" "sort" @@ -51,9 +52,9 @@ type Provider struct { meta interface{} - stopCh chan struct{} // stopCh is closed when Stop is called - stopped bool // set to true once stopped to avoid double close of stopCh - stopLock sync.Mutex + stopCtx context.Context + stopCtxCancel context.CancelFunc + stopOnce sync.Once } // ConfigureFunc is the function used to configure a Provider. @@ -111,40 +112,29 @@ func (p *Provider) SetMeta(v interface{}) { // Stopped reports whether the provider has been stopped or not. func (p *Provider) Stopped() bool { - p.stopLock.Lock() - defer p.stopLock.Unlock() - return p.stopped + ctx := p.StopContext() + select { + case <-ctx.Done(): + return true + default: + return false + } } // StopCh returns a channel that is closed once the provider is stopped. -func (p *Provider) StopCh() <-chan struct{} { - p.stopLock.Lock() - defer p.stopLock.Unlock() +func (p *Provider) StopContext() context.Context { + p.stopOnce.Do(p.stopInit) + return p.stopCtx +} - if p.stopCh == nil { - p.stopCh = make(chan struct{}) - } - - return p.stopCh +func (p *Provider) stopInit() { + p.stopCtx, p.stopCtxCancel = context.WithCancel(context.Background()) } // Stop implementation of terraform.ResourceProvider interface. func (p *Provider) Stop() error { - p.stopLock.Lock() - defer p.stopLock.Unlock() - - // Close the stop channel and mark as stopped if we haven't - if !p.stopped { - // Initialize the stop channel so future calls to StopCh work - if p.stopCh == nil { - p.stopCh = make(chan struct{}) - } - - // Close and mark - close(p.stopCh) - p.stopped = true - } - + p.stopOnce.Do(p.stopInit) + p.stopCtxCancel() return nil } diff --git a/helper/schema/provider_test.go b/helper/schema/provider_test.go index cde72d86b..ed5918844 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -338,7 +338,7 @@ func TestProviderStop(t *testing.T) { } // Verify stopch blocks - ch := p.StopCh() + ch := p.StopContext().Done() select { case <-ch: t.Fatal("should not be stopped") @@ -376,7 +376,7 @@ func TestProviderStop_stopFirst(t *testing.T) { } select { - case <-p.StopCh(): + case <-p.StopContext().Done(): case <-time.After(10 * time.Millisecond): t.Fatal("should be stopped") } From 61bbaf6f85e0b2edafe40dbd93d6c6f90cacec76 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 25 Oct 2016 11:31:32 -0700 Subject: [PATCH 09/10] helper/resource: remove StopCh, use contexts instead --- helper/resource/stop.go | 40 ----------------------- helper/resource/stop_test.go | 62 ------------------------------------ 2 files changed, 102 deletions(-) delete mode 100644 helper/resource/stop.go delete mode 100644 helper/resource/stop_test.go diff --git a/helper/resource/stop.go b/helper/resource/stop.go deleted file mode 100644 index b3bfa776c..000000000 --- a/helper/resource/stop.go +++ /dev/null @@ -1,40 +0,0 @@ -package resource - -import ( - "time" -) - -// StopCh is used to construct a channel that is closed when the provider -// stopCh is closed, or a timeout is reached. -// -// The first return value is the channel that is closed when the operation -// should stop. The second return value should be closed when the operation -// completes so that the stop channel is never closed and to cleanup the -// goroutine watching the channels. -func StopCh(stopCh <-chan struct{}, max time.Duration) (<-chan struct{}, chan<- struct{}) { - ch := make(chan struct{}) - doneCh := make(chan struct{}) - - // If we have a max of 0 then it is unlimited. A nil channel blocks on - // receive forever so this ensures that behavior. - var timeCh <-chan time.Time - if max > 0 { - timeCh = time.After(max) - } - - // Start a goroutine to watch the various cases of cancellation - go func() { - select { - case <-doneCh: - // If we are done, don't trigger the cancel - return - case <-timeCh: - case <-stopCh: - } - - // Trigger the cancel - close(ch) - }() - - return ch, doneCh -} diff --git a/helper/resource/stop_test.go b/helper/resource/stop_test.go deleted file mode 100644 index 839f4af9a..000000000 --- a/helper/resource/stop_test.go +++ /dev/null @@ -1,62 +0,0 @@ -package resource - -import ( - "testing" - "time" -) - -func TestStopCh_stop(t *testing.T) { - stopCh := make(chan struct{}) - ch, _ := StopCh(stopCh, 0) - - // ch should block - select { - case <-ch: - t.Fatal("ch should block") - case <-time.After(10 * time.Millisecond): - } - - // Close the stop channel - close(stopCh) - - // ch should return - select { - case <-ch: - case <-time.After(10 * time.Millisecond): - t.Fatal("ch should not block") - } -} - -func TestStopCh_done(t *testing.T) { - stopCh := make(chan struct{}) - ch, doneCh := StopCh(stopCh, 0) - - // ch should block - select { - case <-ch: - t.Fatal("ch should block") - case <-time.After(10 * time.Millisecond): - } - - // Close the done channel - close(doneCh) - - // ch should block - select { - case <-ch: - t.Fatal("ch should block") - case <-time.After(10 * time.Millisecond): - } -} - -func TestStopCh_timeout(t *testing.T) { - stopCh := make(chan struct{}) - ch, _ := StopCh(stopCh, 10*time.Millisecond) - - // ch should return - select { - case <-ch: - case <-time.After(100 * time.Millisecond): - t.Fatal("ch should not block") - } -} From d7402d0473d9876f91068b6a1f2715c4e21a20c0 Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 25 Oct 2016 11:47:47 -0700 Subject: [PATCH 10/10] providers/azurerm: don't leak the context cancellation function --- builtin/providers/azurerm/resource_arm_storage_account.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/builtin/providers/azurerm/resource_arm_storage_account.go b/builtin/providers/azurerm/resource_arm_storage_account.go index 7e196980b..bba3e6b1c 100644 --- a/builtin/providers/azurerm/resource_arm_storage_account.go +++ b/builtin/providers/azurerm/resource_arm_storage_account.go @@ -193,9 +193,10 @@ func resourceArmStorageAccountCreate(d *schema.ResourceData, meta interface{}) e } // Create - cancelCtx, _ := context.WithTimeout(client.StopContext, 1*time.Hour) + cancelCtx, cancelFunc := context.WithTimeout(client.StopContext, 1*time.Hour) _, createErr := storageClient.Create( resourceGroupName, storageAccountName, opts, cancelCtx.Done()) + cancelFunc() // The only way to get the ID back apparently is to read the resource again read, err := storageClient.GetProperties(resourceGroupName, storageAccountName)