Cloud Stack: More reliable retrying (#1035)

Closes #996
There have been reliability issues with stacks related to Terraform
Grafana.com has a very asynchronous process that works well in the web but not so much in Terraform
While an API 500 in the web UI is not that annoying, since the stack shows up on the next refresh, it disrupts Terraform

In this PR, I make the stack creation process retry on pretty much every error
To test that everything works well, I added two test cases:
- Tainting a stack to test that it can recreate right after deletion
- Creating a stack in double, making sure we can't take-over a stack that already exists

Author: julienduchesne

internal/resources/cloud/resource_cloud_stack.go

@@ -238,20 +238,32 @@ func CreateStack(ctx context.Context, d *schema.ResourceData, meta interface{})
 		Region: d.Get("region_slug").(string),
 	}
 
-	stackID, err := client.NewStack(stack)
-	switch {
-	case err != nil && strings.Contains(err.Error(), "409"):
-		return diag.Errorf("Error: A Grafana stack with the name '%s' already exists.", stack.Name)
-	case err != nil:
-		// If we had an error that isn't a 409 (already exists), try to read the stack
-		// Sometimes, the stack is created but the API returns an error (e.g. 504)
-		readStack, readErr := client.StackBySlug(stack.Slug)
-		if readErr != nil {
-			return diag.Errorf("Failed to create stack: %v", err)
+	err := retry.RetryContext(ctx, 1*time.Minute, func() *retry.RetryError {
+		stackID, err := client.NewStack(stack)
+		switch {
+		case err != nil && strings.Contains(err.Error(), "409"):
+			// If the API returns a 409, it means that the stack already exists
+			// It may also mean that the stack was recently deleted and is still in the process of being deleted
+			// In that case, we want to retry
+			time.Sleep(10 * time.Second) // Do not retry too fast, default is 500ms
+			return retry.RetryableError(fmt.Errorf("a stack with the name '%s' already exists", stack.Name))
+		case err != nil:
+			// If we had an error that isn't a 409 (already exists), try to read the stack
+			// Sometimes, the stack is created but the API returns an error (e.g. 504)
+			readStack, readErr := client.StackBySlug(stack.Slug)
+			if readErr == nil {
+				d.SetId(strconv.FormatInt(readStack.ID, 10))
+				return nil
+			}
+			time.Sleep(10 * time.Second) // Do not retry too fast, default is 500ms
+			return retry.RetryableError(fmt.Errorf("failed to create stack: %v", err))
+		default:
+			d.SetId(strconv.FormatInt(stackID, 10))
 		}
-		d.SetId(strconv.FormatInt(readStack.ID, 10))
-	default:
-		d.SetId(strconv.FormatInt(stackID, 10))
+		return nil
+	})
+	if err != nil {
+		return diag.FromErr(err)
 	}
 
 	if diag := ReadStack(ctx, d, meta); diag != nil {

internal/resources/cloud/resource_cloud_stack_test.go

@@ -2,6 +2,7 @@ package cloud_test
 
 import (
 	"fmt"
+	"regexp"
 	"strings"
 	"time"
 
@@ -25,29 +26,47 @@ func TestResourceStack_Basic(t *testing.T) {
 	resourceName := GetRandomStackName(prefix)
 	stackDescription := "This is a test stack"
 
+	firstStepChecks := resource.ComposeTestCheckFunc(
+		testAccStackCheckExists("grafana_cloud_stack.test", &stack),
+		resource.TestMatchResourceAttr("grafana_cloud_stack.test", "id", common.IDRegexp),
+		resource.TestCheckResourceAttr("grafana_cloud_stack.test", "name", resourceName),
+		resource.TestCheckResourceAttr("grafana_cloud_stack.test", "slug", resourceName),
+		resource.TestCheckResourceAttr("grafana_cloud_stack.test", "status", "active"),
+		resource.TestCheckResourceAttr("grafana_cloud_stack.test", "prometheus_remote_endpoint", "https://prometheus-prod-01-eu-west-0.grafana.net/api/prom"),
+		resource.TestCheckResourceAttr("grafana_cloud_stack.test", "prometheus_remote_write_endpoint", "https://prometheus-prod-01-eu-west-0.grafana.net/api/prom/push"),
+		resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "prometheus_user_id"),
+		resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "alertmanager_user_id"),
+		resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "logs_user_id"),
+		resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "traces_user_id"),
+		resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "graphite_user_id"),
+	)
+
 	resource.ParallelTest(t, resource.TestCase{
 		PreCheck: func() {
 			testAccDeleteExistingStacks(t, prefix)
 		},
 		ProviderFactories: testutils.ProviderFactories,
 		CheckDestroy:      testAccStackCheckDestroy(&stack),
 		Steps: []resource.TestStep{
+			// Create a basic stack
 			{
 				Config: testAccStackConfigBasic(resourceName, resourceName),
-				Check: resource.ComposeTestCheckFunc(
-					testAccStackCheckExists("grafana_cloud_stack.test", &stack),
-					resource.TestMatchResourceAttr("grafana_cloud_stack.test", "id", common.IDRegexp),
-					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "name", resourceName),
-					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "slug", resourceName),
-					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "status", "active"),
-					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "prometheus_remote_endpoint", "https://prometheus-prod-01-eu-west-0.grafana.net/api/prom"),
-					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "prometheus_remote_write_endpoint", "https://prometheus-prod-01-eu-west-0.grafana.net/api/prom/push"),
-					resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "prometheus_user_id"),
-					resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "alertmanager_user_id"),
-					resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "logs_user_id"),
-					resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "traces_user_id"),
-					resource.TestCheckResourceAttrSet("grafana_cloud_stack.test", "graphite_user_id"),
-				),
+				Check:  firstStepChecks,
+			},
+			// Check that we can't takeover a stack without importing it
+			// The retrying logic for creation is very permissive,
+			// but it shouldn't allow to apply an already existing stack on a new resource
+			{
+				Config: testAccStackConfigBasic(resourceName, resourceName) +
+					testAccStackConfigBasicWithCustomResourceName(resourceName, resourceName, "test2"), // new stack with same name/slug
+				ExpectError: regexp.MustCompile(fmt.Sprintf(".*a stack with the name '%s' already exists.*", resourceName)),
+			},
+			// Test that the stack is correctly recreated if it's tainted and reapplied
+			// This is a special case because stack deletion is asynchronous
+			{
+				Config: testAccStackConfigBasic(resourceName, resourceName),
+				Check:  firstStepChecks,
+				Taint:  []string{"grafana_cloud_stack.test"},
 			},
 			{
 				// Delete the stack outside of the test and make sure it is recreated
@@ -65,6 +84,7 @@ func TestResourceStack_Basic(t *testing.T) {
 					resource.TestCheckResourceAttr("grafana_cloud_stack.test", "status", "active"),
 				),
 			},
+			// Update the stack
 			{
 				Config: testAccStackConfigUpdate(resourceName+"new", resourceName, stackDescription),
 				Check: resource.ComposeTestCheckFunc(
@@ -150,13 +170,17 @@ func testAccStackCheckDestroy(a *gapi.Stack) resource.TestCheckFunc {
 }
 
 func testAccStackConfigBasic(name string, slug string) string {
+	return testAccStackConfigBasicWithCustomResourceName(name, slug, "test")
+}
+
+func testAccStackConfigBasicWithCustomResourceName(name string, slug string, resourceName string) string {
 	return fmt.Sprintf(`
-	resource "grafana_cloud_stack" "test" {
+	resource "grafana_cloud_stack" "%s" {
 		name  = "%s"
 		slug  = "%s"
 		region_slug = "eu"
 	  }
-	`, name, slug)
+	`, resourceName, name, slug)
 }
 
 func testAccStackConfigUpdate(name string, slug string, description string) string {