From 1d9d7be28c32d25bfcc557071b5aac285ec06e8a Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 09:19:02 -0500 Subject: [PATCH 1/7] Add schema.Provider.TestReset method Provider.TestReset resets the internal state of the Provider at the start of a test. This also adds a MetaReset function field to schema.Provider, which is called by TestReset and can be used to reset any other tsated stored in the provider metadata. This is currently used to reset the internal Context returned by StopContext between tests, and should be implemented by a provider if it stores a Context from a previous test. --- helper/schema/provider.go | 30 ++++++++++++++++++++++++++++++ helper/schema/provider_test.go | 26 ++++++++++++++++++++++++++ 2 files changed, 56 insertions(+) diff --git a/helper/schema/provider.go b/helper/schema/provider.go index 5b50d54a1f..d52d2f5f06 100644 --- a/helper/schema/provider.go +++ b/helper/schema/provider.go @@ -50,8 +50,15 @@ type Provider struct { // See the ConfigureFunc documentation for more information. ConfigureFunc ConfigureFunc + // MetaReset is called by TestReset to reset any state stored in the meta + // interface. This is especially important if the StopContext is stored by + // the provider. + MetaReset func() error + meta interface{} + // a mutex is required because TestReset can directly repalce the stopCtx + stopMu sync.Mutex stopCtx context.Context stopCtxCancel context.CancelFunc stopOnce sync.Once @@ -124,20 +131,43 @@ func (p *Provider) Stopped() bool { // StopCh returns a channel that is closed once the provider is stopped. func (p *Provider) StopContext() context.Context { p.stopOnce.Do(p.stopInit) + + p.stopMu.Lock() + defer p.stopMu.Unlock() + return p.stopCtx } func (p *Provider) stopInit() { + p.stopMu.Lock() + defer p.stopMu.Unlock() + 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.stopMu.Lock() + defer p.stopMu.Unlock() + p.stopCtxCancel() return nil } +// TestReset resets any state stored in the Provider, and will call TestReset +// on Meta if it implements the TestProvider interface. +// This may be used to reset the schema.Provider at the start of a test, and is +// automatically called by resource.Test. +func (p *Provider) TestReset() error { + p.stopInit() + if p.MetaReset != nil { + return p.MetaReset() + } + 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 ed5918844b..5b06c5e576 100644 --- a/helper/schema/provider_test.go +++ b/helper/schema/provider_test.go @@ -381,3 +381,29 @@ func TestProviderStop_stopFirst(t *testing.T) { t.Fatal("should be stopped") } } + +func TestProviderReset(t *testing.T) { + var p Provider + stopCtx := p.StopContext() + p.MetaReset = func() error { + stopCtx = p.StopContext() + return nil + } + + // cancel the current context + p.Stop() + + if err := p.TestReset(); err != nil { + t.Fatal(err) + } + + // the first context should have been replaced + if err := stopCtx.Err(); err != nil { + t.Fatal(err) + } + + // we should not get a canceled context here either + if err := p.StopContext().Err(); err != nil { + t.Fatal(err) + } +} From 1eb9a2d083eb070a2b49b4ca7261a23e452b8bb9 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:03:03 -0500 Subject: [PATCH 2/7] Reset ResourceProviders Call all ResourceProviderFactories and reset the providers before tests. Store the provider and/or the error in a fixed factory function to be returned again later. --- helper/resource/testing.go | 53 +++++++++++++++++++++++++++++++++----- 1 file changed, 46 insertions(+), 7 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index 6d2deb9e53..b166c28fef 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -22,6 +22,12 @@ import ( const TestEnvVar = "TF_ACC" +// TestProvider is implemented by scheme.Provider to allow resetting the +// provider state between tests. +type TestProvider interface { + TestReset() error +} + // TestCheckFunc is the callback type used with acceptance tests to check // the state of a resource. The state passed in is the latest state known, // or in the case of being after a destroy, it is the last known state when @@ -216,13 +222,9 @@ func Test(t TestT, c TestCase) { c.PreCheck() } - // Build our context options that we can - ctxProviders := c.ProviderFactories - if ctxProviders == nil { - ctxProviders = make(map[string]terraform.ResourceProviderFactory) - for k, p := range c.Providers { - ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p) - } + ctxProviders, err := testProviderFactories(c) + if err != nil { + t.Fatal(err) } opts := terraform.ContextOpts{Providers: ctxProviders} @@ -333,6 +335,43 @@ func Test(t TestT, c TestCase) { } } +// testProviderFactories is a helper to build the ResourceProviderFactory map +// with pre instantiated ResourceProviders, so that we can reset them for the +// test, while only calling the factory function once. +// Any errors are stored so that they can be returned by the factory in +// terraform to match non-test behavior. +func testProviderFactories(c TestCase) (map[string]terraform.ResourceProviderFactory, error) { + ctxProviders := make(map[string]terraform.ResourceProviderFactory) + + // add any fixed providers + for k, p := range c.Providers { + ctxProviders[k] = terraform.ResourceProviderFactoryFixed(p) + } + + // call any factory functions and store the result. + for k, pf := range c.ProviderFactories { + p, err := pf() + ctxProviders[k] = func() (terraform.ResourceProvider, error) { + return p, err + } + } + + // reset the providers if needed + for k, pf := range ctxProviders { + // we can ignore any errors here, if we don't have a provider to reset + // the error will be handled later + p, _ := pf() + if p, ok := p.(TestProvider); ok { + err := p.TestReset() + if err != nil { + return nil, fmt.Errorf("[ERROR] failed to reset provider %q: %s", k, err) + } + } + } + + return ctxProviders, nil +} + // UnitTest is a helper to force the acceptance testing harness to run in the // normal unit test suite. This should only be used for resource that don't // have any external dependencies. From 0279d11c8a2bd5ee667635d6c882e69377bf80d6 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:05:07 -0500 Subject: [PATCH 3/7] Add TestReset to terraformMockResourceProvider Have MockResourceProvider implement TestProvider to check that TestReset is called by the test harness. --- terraform/resource_provider_mock.go | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index f5315339fb..19a4495978 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -56,6 +56,8 @@ type MockResourceProvider struct { ReadDataDiffFn func(*InstanceInfo, *ResourceConfig) (*InstanceDiff, error) ReadDataDiffReturn *InstanceDiff ReadDataDiffReturnError error + TestResetCalled bool + TestResetError error StopCalled bool StopFn func() error StopReturnError error @@ -144,6 +146,14 @@ func (p *MockResourceProvider) Configure(c *ResourceConfig) error { return p.ConfigureReturnError } +func (p *MockResourceProvider) TestReset() error { + p.Lock() + defer p.Unlock() + + p.TestResetCalled = true + return p.TestResetError +} + func (p *MockResourceProvider) Stop() error { p.Lock() defer p.Unlock() From 4b2e96b2e2663eae5a89f7bcdadb6a011dc7d201 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Tue, 7 Mar 2017 10:18:09 -0500 Subject: [PATCH 4/7] test for TestReset and fixed resource factories --- helper/resource/testing_test.go | 49 +++++++++++++++++++++++++++++++++ 1 file changed, 49 insertions(+) diff --git a/helper/resource/testing_test.go b/helper/resource/testing_test.go index d2e05c0c52..56e9cd02eb 100644 --- a/helper/resource/testing_test.go +++ b/helper/resource/testing_test.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "os" + "regexp" "strings" "sync/atomic" "testing" @@ -95,6 +96,9 @@ func TestTest(t *testing.T) { if !checkDestroy { t.Fatal("didn't call check for destroy") } + if !mp.TestResetCalled { + t.Fatal("didn't call TestReset") + } } func TestTest_idRefresh(t *testing.T) { @@ -355,6 +359,51 @@ func TestTest_stepError(t *testing.T) { } } +func TestTest_factoryError(t *testing.T) { + resourceFactoryError := fmt.Errorf("resource factory error") + + factory := func() (terraform.ResourceProvider, error) { + return nil, resourceFactoryError + } + + mt := new(mockT) + Test(mt, TestCase{ + ProviderFactories: map[string]terraform.ResourceProviderFactory{ + "test": factory, + }, + Steps: []TestStep{ + TestStep{ + ExpectError: regexp.MustCompile("resource factory error"), + }, + }, + }) + + if !mt.failed() { + t.Fatal("test should've failed") + } +} + +func TestTest_resetError(t *testing.T) { + mp := testProvider() + mp.TestResetError = fmt.Errorf("provider reset error") + + mt := new(mockT) + Test(mt, TestCase{ + Providers: map[string]terraform.ResourceProvider{ + "test": mp, + }, + Steps: []TestStep{ + TestStep{ + ExpectError: regexp.MustCompile("provider reset error"), + }, + }, + }) + + if !mt.failed() { + t.Fatal("test should've failed") + } +} + func TestComposeAggregateTestCheckFunc(t *testing.T) { check1 := func(s *terraform.State) error { return errors.New("Error 1") From 38d2a8f6aca4526b6cb6e5eb2b4b95e6b5626886 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 8 Mar 2017 14:43:31 -0500 Subject: [PATCH 5/7] Fix logic race with Context.watchStop Always wait for watchStop to return during context.walk. Context.walk would often complete immediately after sending the close signal to watchStop, which would in turn call the deferred releaseRun cancelling the runContext. Without any synchronization points after the select statement in watchStop, that goroutine was not guaranteed to be scheduled immediately, and in fact it often didn't continue until after the runContext was canceled. This in turn left the select statement with multiple successful cases, and half the time it would chose to Stop the providers. Stopping the providers after the walk of course didn't cause any immediate failures, but if there was another walk performed, the provider StopContext would no longer be valid and could cause cancellation errors in the provider. --- terraform/context.go | 111 +++++++++++++++++++++++++------------------ 1 file changed, 66 insertions(+), 45 deletions(-) diff --git a/terraform/context.go b/terraform/context.go index 06eef4300c..b8657a6b59 100644 --- a/terraform/context.go +++ b/terraform/context.go @@ -775,15 +775,14 @@ func (c *Context) walk( } // Watch for a stop so we can call the provider Stop() API. - doneCh := make(chan struct{}) - stopCh := c.runContext.Done() - go c.watchStop(walker, doneCh, stopCh) + watchStop, watchWait := c.watchStop(walker) // Walk the real graph, this will block until it completes realErr := graph.Walk(walker) - // Close the done channel so the watcher stops - close(doneCh) + // Close the channel so the watcher stops, and wait for it to return. + close(watchStop) + <-watchWait // 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 @@ -872,52 +871,74 @@ func (c *Context) walk( return walker, realErr } -func (c *Context) watchStop(walker *ContextGraphWalker, doneCh, stopCh <-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) +// watchStop immediately returns a `stop` and a `wait` chan after dispatching +// the watchStop goroutine. This will watch the runContext for cancellation and +// stop the providers accordingly. When the watch is no longer needed, the +// `stop` chan should be closed before waiting on the `wait` chan. +// The `wait` chan is important, because without synchronizing with the end of +// the watchStop goroutine, the runContext may also be closed during the select +// incorrectly causing providers to be stopped. Even if the graph walk is done +// at that point, stopping a provider permanently cancels its StopContext which +// can cause later actions to fail. +func (c *Context) watchStop(walker *ContextGraphWalker) (chan struct{}, <-chan struct{}) { + stop := make(chan struct{}) + wait := make(chan struct{}) + + // get the runContext cancellation channel now, because releaseRun will + // write to the runContext field. + done := c.runContext.Done() + + go func() { + defer close(wait) + // Wait for a stop or completion + select { + case <-done: + // done means the context was canceled, so we need to try and stop + // providers. + case <-stop: + // our own stop channel was closed. + return } - 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() - } - } + // If we're here, we're stopped, trigger the call. - { - // Call stop on all the provisioners - walker.provisionerLock.Lock() - ps := make([]ResourceProvisioner, 0, len(walker.provisionerCache)) - for _, p := range walker.provisionerCache { - ps = append(ps, p) + { + // 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() + } } - defer walker.provisionerLock.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() + { + // Call stop on all the provisioners + walker.provisionerLock.Lock() + ps := make([]ResourceProvisioner, 0, len(walker.provisionerCache)) + for _, p := range walker.provisionerCache { + ps = append(ps, p) + } + defer walker.provisionerLock.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() + } } - } + }() + + return stop, wait } // parseVariableAsHCL parses the value of a single variable as would have been specified From 6fcb55d09e00ec74c7d2005a6137e45469dd2aea Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 8 Mar 2017 17:41:35 -0500 Subject: [PATCH 6/7] reword TestProvider doc --- helper/resource/testing.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/helper/resource/testing.go b/helper/resource/testing.go index b166c28fef..c50d6d1204 100644 --- a/helper/resource/testing.go +++ b/helper/resource/testing.go @@ -22,8 +22,9 @@ import ( const TestEnvVar = "TF_ACC" -// TestProvider is implemented by scheme.Provider to allow resetting the -// provider state between tests. +// TestProvider can be implemented by any ResourceProvider to provide custom +// reset functionality at the start of an acceptance test. +// The helper/schema Provider implements this interface. type TestProvider interface { TestReset() error } From 5238f51dc7ba7199ab9d97fdd4eb8449e61adae2 Mon Sep 17 00:00:00 2001 From: James Bardin Date: Wed, 8 Mar 2017 17:42:05 -0500 Subject: [PATCH 7/7] move TestReset mock from terraform to helper the terraform package doesn't know about TestProvider, so don't put the hooks in terraform.MockResourceProvider. Wrap the mock in the test where we need to check the TestProvider functionality. --- helper/resource/testing_test.go | 27 ++++++++++++++++++++++++--- terraform/resource_provider_mock.go | 10 ---------- 2 files changed, 24 insertions(+), 13 deletions(-) diff --git a/helper/resource/testing_test.go b/helper/resource/testing_test.go index 56e9cd02eb..634f307734 100644 --- a/helper/resource/testing_test.go +++ b/helper/resource/testing_test.go @@ -6,6 +6,7 @@ import ( "os" "regexp" "strings" + "sync" "sync/atomic" "testing" @@ -26,8 +27,26 @@ func init() { } } +// wrap the mock provider to implement TestProvider +type resetProvider struct { + *terraform.MockResourceProvider + mu sync.Mutex + TestResetCalled bool + TestResetError error +} + +func (p *resetProvider) TestReset() error { + p.mu.Lock() + defer p.mu.Unlock() + p.TestResetCalled = true + return p.TestResetError +} + func TestTest(t *testing.T) { - mp := testProvider() + mp := &resetProvider{ + MockResourceProvider: testProvider(), + } + mp.DiffReturn = nil mp.ApplyFn = func( @@ -384,8 +403,10 @@ func TestTest_factoryError(t *testing.T) { } func TestTest_resetError(t *testing.T) { - mp := testProvider() - mp.TestResetError = fmt.Errorf("provider reset error") + mp := &resetProvider{ + MockResourceProvider: testProvider(), + TestResetError: fmt.Errorf("provider reset error"), + } mt := new(mockT) Test(mt, TestCase{ diff --git a/terraform/resource_provider_mock.go b/terraform/resource_provider_mock.go index 19a4495978..f5315339fb 100644 --- a/terraform/resource_provider_mock.go +++ b/terraform/resource_provider_mock.go @@ -56,8 +56,6 @@ type MockResourceProvider struct { ReadDataDiffFn func(*InstanceInfo, *ResourceConfig) (*InstanceDiff, error) ReadDataDiffReturn *InstanceDiff ReadDataDiffReturnError error - TestResetCalled bool - TestResetError error StopCalled bool StopFn func() error StopReturnError error @@ -146,14 +144,6 @@ func (p *MockResourceProvider) Configure(c *ResourceConfig) error { return p.ConfigureReturnError } -func (p *MockResourceProvider) TestReset() error { - p.Lock() - defer p.Unlock() - - p.TestResetCalled = true - return p.TestResetError -} - func (p *MockResourceProvider) Stop() error { p.Lock() defer p.Unlock()