From 60140b28f4f2fc4a576e884f44e02049abfd488d Mon Sep 17 00:00:00 2001 From: Mitchell Hashimoto Date: Tue, 25 Oct 2016 12:00:36 -0700 Subject: [PATCH] Revert "Merge pull request #9536 from hashicorp/f-provider-stop" This reverts commit c3a4cff1337f211f4ca565dca686564386ba05cf, reversing changes made to 791a02e6e456df7e36f7c24813850519df947e91. This change requires plugin recompilation and we should hold off until a minor release for that. --- builtin/providers/azurerm/config.go | 3 - builtin/providers/azurerm/provider.go | 55 ++++++++----------- .../azurerm/resource_arm_storage_account.go | 25 +++++++-- helper/schema/provider.go | 34 ------------ helper/schema/provider_test.go | 53 ------------------ plugin/resource_provider.go | 28 ---------- plugin/resource_provider_test.go | 55 ------------------- terraform/context.go | 44 --------------- terraform/context_apply_test.go | 4 -- terraform/resource_provider.go | 20 ------- terraform/resource_provider_mock.go | 15 ----- terraform/shadow_resource_provider.go | 9 --- 12 files changed, 43 insertions(+), 302 deletions(-) diff --git a/builtin/providers/azurerm/config.go b/builtin/providers/azurerm/config.go index 3638c6a51..d860c7f1b 100644 --- a/builtin/providers/azurerm/config.go +++ b/builtin/providers/azurerm/config.go @@ -1,7 +1,6 @@ package azurerm import ( - "context" "fmt" "log" "net/http" @@ -31,8 +30,6 @@ type ArmClient struct { tenantId string subscriptionId string - StopContext context.Context - rivieraClient *riviera.Client availSetClient compute.AvailabilitySetsClient diff --git a/builtin/providers/azurerm/provider.go b/builtin/providers/azurerm/provider.go index 12fd48f45..c65679fa4 100644 --- a/builtin/providers/azurerm/provider.go +++ b/builtin/providers/azurerm/provider.go @@ -17,8 +17,7 @@ import ( // Provider returns a terraform.ResourceProvider. func Provider() terraform.ResourceProvider { - var p *schema.Provider - p = &schema.Provider{ + return &schema.Provider{ Schema: map[string]*schema.Schema{ "subscription_id": { Type: schema.TypeString, @@ -105,10 +104,8 @@ func Provider() terraform.ResourceProvider { "azurerm_sql_firewall_rule": resourceArmSqlFirewallRule(), "azurerm_sql_server": resourceArmSqlServer(), }, - ConfigureFunc: providerConfigure(p), + ConfigureFunc: providerConfigure, } - - return p } // Config is the configuration structure used to instantiate a @@ -143,33 +140,29 @@ func (c *Config) validate() error { return err.ErrorOrNil() } -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 - } - - client, err := config.getArmClient() - if err != nil { - return nil, err - } - - client.StopContext = p.StopContext() - - err = registerAzureResourceProvidersWithSubscription(client.rivieraClient) - if err != nil { - return nil, err - } - - return client, nil +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), } + + if err := config.validate(); 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 + } + + 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 bba3e6b1c..e31f3a0db 100644 --- a/builtin/providers/azurerm/resource_arm_storage_account.go +++ b/builtin/providers/azurerm/resource_arm_storage_account.go @@ -1,7 +1,6 @@ package azurerm import ( - "context" "fmt" "log" "net/http" @@ -12,6 +11,7 @@ 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,11 +192,24 @@ func resourceArmStorageAccountCreate(d *schema.ResourceData, meta interface{}) e opts.Properties.AccessTier = storage.AccessTier(accessTier.(string)) } - // Create - cancelCtx, cancelFunc := context.WithTimeout(client.StopContext, 1*time.Hour) - _, createErr := storageClient.Create( - resourceGroupName, storageAccountName, opts, cancelCtx.Done()) - cancelFunc() + // 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. + } // 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 5b50d54a1..546794f40 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -1,11 +1,9 @@ package schema import ( - "context" "errors" "fmt" "sort" - "sync" "github.com/hashicorp/go-multierror" "github.com/hashicorp/terraform/terraform" @@ -51,10 +49,6 @@ type Provider struct { ConfigureFunc ConfigureFunc meta interface{} - - stopCtx context.Context - stopCtxCancel context.CancelFunc - stopOnce sync.Once } // ConfigureFunc is the function used to configure a Provider. @@ -110,34 +104,6 @@ func (p *Provider) SetMeta(v interface{}) { p.meta = v } -// Stopped reports whether the provider has been stopped or not. -func (p *Provider) Stopped() bool { - 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) StopContext() context.Context { - p.stopOnce.Do(p.stopInit) - return p.stopCtx -} - -func (p *Provider) stopInit() { - p.stopCtx, p.stopCtxCancel = context.WithCancel(context.Background()) -} - -// Stop implementation of terraform.ResourceProvider interface. -func (p *Provider) Stop() error { - p.stopOnce.Do(p.stopInit) - p.stopCtxCancel() - 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 ed5918844..aa8a787bf 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -4,7 +4,6 @@ import ( "fmt" "reflect" "testing" - "time" "github.com/hashicorp/terraform/config" "github.com/hashicorp/terraform/terraform" @@ -329,55 +328,3 @@ 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.StopContext().Done() - 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.StopContext().Done(): - case <-time.After(10 * time.Millisecond): - t.Fatal("should be stopped") - } -} diff --git a/plugin/resource_provider.go b/plugin/resource_provider.go index 473f78601..b711864a1 100644 --- a/plugin/resource_provider.go +++ b/plugin/resource_provider.go @@ -28,19 +28,6 @@ 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) { @@ -308,10 +295,6 @@ type ResourceProviderServer struct { Provider terraform.ResourceProvider } -type ResourceProviderStopResponse struct { - Error *plugin.BasicError -} - type ResourceProviderConfigureResponse struct { Error *plugin.BasicError } @@ -407,17 +390,6 @@ 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 9c2d43dab..41997b132 100644 --- a/plugin/resource_provider_test.go +++ b/plugin/resource_provider_test.go @@ -14,61 +14,6 @@ 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) diff --git a/terraform/context.go b/terraform/context.go index e8f821127..7714455c9 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -105,7 +105,6 @@ type Context struct { parallelSem Semaphore providerInputConfig map[string]map[string]interface{} runCh <-chan struct{} - stopCh chan struct{} shadowErr error } @@ -588,9 +587,6 @@ 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 @@ -676,9 +672,6 @@ 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() @@ -694,7 +687,6 @@ func (c *Context) releaseRun(ch chan<- struct{}) { close(ch) c.runCh = nil - c.stopCh = nil } func (c *Context) walk( @@ -722,16 +714,9 @@ 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 @@ -811,35 +796,6 @@ 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 0dfd47ec1..e90c6e941 100644 --- a/terraform/context_apply_test.go +++ b/terraform/context_apply_test.go @@ -1043,10 +1043,6 @@ 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) { diff --git a/terraform/resource_provider.go b/terraform/resource_provider.go index 1a68c8699..542f14a61 100644 --- a/terraform/resource_provider.go +++ b/terraform/resource_provider.go @@ -47,26 +47,6 @@ 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 *********************************************************************/ diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index f5315339f..f8acfafa9 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -56,9 +56,6 @@ 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 @@ -144,18 +141,6 @@ 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 72bb49cf5..0c79edf75 100644 --- a/terraform/shadow_resource_provider.go +++ b/terraform/shadow_resource_provider.go @@ -107,10 +107,6 @@ 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 @@ -445,11 +441,6 @@ 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