Add Retry to CreateApplication.

In the case where RequiresReplace is used in the resource schema, terraform calls Destroy then Create. When Destroy is not complete before Create starts, it can cause problems with Create. Retry the various pieces of create until they succeed or timeout. Removed skip from test cases skipped due to this bug.
juju · Oct 24, 2023 · 35af322 · 35af322
1 parent a97a07a
commit 35af322
Show file tree

Hide file tree

Showing 5 changed files with 99 additions and 64 deletions.
diff --git a/internal/juju/applications.go b/internal/juju/applications.go
@@ -177,7 +177,7 @@ func resolveCharmURL(charmName string) (*charm.URL, error) {
 	return charmURL, nil
 }
 
-func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*CreateApplicationResponse, error) {
+func (c applicationsClient) CreateApplication(ctx context.Context, input *CreateApplicationInput) (*CreateApplicationResponse, error) {
 	appName := input.ApplicationName
 	if appName == "" {
 		appName = input.CharmName
@@ -276,25 +276,6 @@ func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*C
 			userBase, suggestedBase)
 	}
 
-	// Add the charm to the model
-	origin = resolvedOrigin.WithSeries(seriesToUse)
-	charmURL = resolvedURL.WithSeries(seriesToUse)
-
-	resultOrigin, err := charmsAPIClient.AddCharm(charmURL, origin, false)
-	if err != nil {
-		return nil, err
-	}
-
-	charmID := apiapplication.CharmID{
-		URL:    charmURL,
-		Origin: resultOrigin,
-	}
-
-	resources, err := c.processResources(charmsAPIClient, conn, charmID, appName)
-	if err != nil {
-		return nil, err
-	}
-
 	appConfig := input.Config
 	if appConfig == nil {
 		appConfig = make(map[string]string)
@@ -318,24 +299,76 @@ func (c applicationsClient) CreateApplication(input *CreateApplicationInput) (*C
 		}
 	}
 
-	args := apiapplication.DeployArgs{
-		CharmID:         charmID,
-		ApplicationName: appName,
-		NumUnits:        input.Units,
-		// Still supply series, to be compatible with juju 2.9 controllers.
-		// 3.x controllers will only use the CharmOrigin and its base.
-		Series:      resultOrigin.Series,
-		CharmOrigin: resultOrigin,
-		Config:      appConfig,
-		Cons:        input.Constraints,
-		Resources:   resources,
-		Placement:   placements,
-	}
-	c.Tracef("Calling Deploy", map[string]interface{}{"args": args})
-	err = applicationAPIClient.Deploy(args)
+	// Add the charm to the model
+	origin = resolvedOrigin.WithSeries(seriesToUse)
+	charmURL = resolvedURL.WithSeries(seriesToUse)
 
+	// If a plan element, with RequiresReplace in the schema, is
+	// changed. Terraform calls the Destroy method then the Create
+	// method for resource. This provider does not wait for Destroy
+	// to be complete before returning. Therefore, a race may occur
+	// of tearing down and reading the same charm.
+	//
+	// Do the actual work to create an application within Retry.
+	// Errors seen so far include:
+	// * cannot add application "replace": charm "ch:amd64/jammy/mysql-196" not found
+	// * cannot add application "replace": application already exists
+	// * cannot add application "replace": charm: not found or not alive
+	err := retry.Call(retry.CallArgs{
+		Func: func() error {
+			resultOrigin, err := charmsAPIClient.AddCharm(charmURL, origin, false)
+			if err != nil {
+				err2 := typedError(err)
+				// If the charm is AlreadyExists, keep going, we
+				// may still be able to create the application. It's
+				// also possible we have multiple applications using
+				// the same charm.
+				if !jujuerrors.Is(err2, jujuerrors.AlreadyExists) {
+					return err2
+				}
+			}
+
+			charmID := apiapplication.CharmID{
+				URL:    charmURL,
+				Origin: resultOrigin,
+			}
+
+			resources, err := c.processResources(charmsAPIClient, conn, charmID, appName)
+			if err != nil && !jujuerrors.Is(err, jujuerrors.AlreadyExists) {
+				return err
+			}
+
+			args := apiapplication.DeployArgs{
+				CharmID:         charmID,
+				ApplicationName: appName,
+				NumUnits:        input.Units,
+				// Still supply series, to be compatible with juju 2.9 controllers.
+				// 3.x controllers will only use the CharmOrigin and its base.
+				Series:      resultOrigin.Series,
+				CharmOrigin: resultOrigin,
+				Config:      appConfig,
+				Cons:        input.Constraints,
+				Resources:   resources,
+				Placement:   placements,
+			}
+			c.Tracef("Calling Deploy", map[string]interface{}{"args": args})
+			if err = applicationAPIClient.Deploy(args); err != nil {
+				return typedError(err)
+			}
+			return nil
+		},
+		NotifyFunc: func(err error, attempt int) {
+			c.Errorf(err, fmt.Sprintf("deploy application %q retry", appName))
+			message := fmt.Sprintf("waiting for application %q deploy, attempt %d", appName, attempt)
+			c.Debugf(message)
+		},
+		BackoffFunc: retry.DoubleDelay,
+		Attempts:    30,
+		Delay:       time.Second,
+		Clock:       clock.WallClock,
+		Stop:        ctx.Done(),
+	})
 	if err != nil {
-		// unfortunate error during deployment
 		return nil, err
 	}
 
@@ -516,7 +549,7 @@ func splitCommaDelimitedList(list string) []string {
 func (c applicationsClient) processResources(charmsAPIClient *apicharms.Client, conn api.Connection, charmID apiapplication.CharmID, appName string) (map[string]string, error) {
 	charmInfo, err := charmsAPIClient.CharmInfo(charmID.URL.String())
 	if err != nil {
-		return nil, err
+		return nil, typedError(err)
 	}
 
 	// check if we have resources to request
@@ -1061,7 +1094,7 @@ func addPendingResources(appName string, resourcesToBeAdded map[string]charmreso
 
 	toRequest, err := resourcesAPIClient.AddPendingResources(resourcesReq)
 	if err != nil {
-		return nil, err
+		return nil, typedError(err)
 	}
 
 	// now build a map with the resource name and the corresponding UUID

diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go
@@ -385,21 +385,23 @@ func (r *applicationResource) Create(ctx context.Context, req resource.CreateReq
 	}
 
 	modelName := plan.ModelName.ValueString()
-	createResp, err := r.client.Applications.CreateApplication(&juju.CreateApplicationInput{
-		ApplicationName: plan.ApplicationName.ValueString(),
-		ModelName:       modelName,
-		CharmName:       charmName,
-		CharmChannel:    channel,
-		CharmRevision:   revision,
-		CharmBase:       planCharm.Base.ValueString(),
-		CharmSeries:     planCharm.Series.ValueString(),
-		Units:           int(plan.UnitCount.ValueInt64()),
-		Config:          configField,
-		Constraints:     parsedConstraints,
-		Trust:           plan.Trust.ValueBool(),
-		Expose:          expose,
-		Placement:       plan.Placement.ValueString(),
-	})
+	createResp, err := r.client.Applications.CreateApplication(ctx,
+		&juju.CreateApplicationInput{
+			ApplicationName: plan.ApplicationName.ValueString(),
+			ModelName:       modelName,
+			CharmName:       charmName,
+			CharmChannel:    channel,
+			CharmRevision:   revision,
+			CharmBase:       planCharm.Base.ValueString(),
+			CharmSeries:     planCharm.Series.ValueString(),
+			Units:           int(plan.UnitCount.ValueInt64()),
+			Config:          configField,
+			Constraints:     parsedConstraints,
+			Trust:           plan.Trust.ValueBool(),
+			Expose:          expose,
+			Placement:       plan.Placement.ValueString(),
+		},
+	)
 	if err != nil {
 		resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to create application, got error: %s", err))
 		return

diff --git a/internal/provider/resource_application_test.go b/internal/provider/resource_application_test.go
@@ -26,7 +26,7 @@ func TestAcc_ResourceApplication_Edge(t *testing.T) {
 				// "When tests have an ExpectError[...]; this results in any previous state being cleared. "
 				// https://github.com/hashicorp/terraform-plugin-sdk/issues/118
 				Config:      testAccResourceApplicationBasic(modelName, appInvalidName),
-				ExpectError: regexp.MustCompile(fmt.Sprintf("Unable to create application, got error: invalid application name %q,\nunexpected character _", appInvalidName)),
+				ExpectError: regexp.MustCompile(fmt.Sprintf("Unable to create application, got error: invalid application name %q,unexpected character _", appInvalidName)),
 			},
 			{
 				Config: testAccResourceApplicationBasic(modelName, appName),
@@ -51,8 +51,8 @@ func TestAcc_ResourceApplication_Edge(t *testing.T) {
 					// There is a timing window with destroying an application
 					// before a new one is created when RequiresReplace is used in the
 					// resource schema.
-					return true, nil
-					//return testingCloud != LXDCloudTesting, nil
+					//return true, nil
+					return testingCloud != LXDCloudTesting, nil
 				},
 				Config: testAccResourceApplicationConstraints(modelName, "arch=amd64 cores=1 mem=4096M"),
 				Check: resource.ComposeTestCheckFunc(
@@ -72,8 +72,8 @@ func TestAcc_ResourceApplication_Edge(t *testing.T) {
 					// There is a timing window with destroying an application
 					// before a new one is created when RequiresReplace is used in the
 					// resource schema.
-					return true, nil
-					//return testingCloud != MicroK8sTesting, nil
+					//return true, nil
+					return testingCloud != MicroK8sTesting, nil
 				},
 				Config: testAccResourceApplicationConstraints(modelName, "arch=amd64 mem=4096M"),
 				Check: resource.ComposeTestCheckFunc(
@@ -92,8 +92,8 @@ func TestAcc_ResourceApplication_Edge(t *testing.T) {
 					// There is a timing window with destroying an application
 					// before a new one is created when RequiresReplace is used in the
 					// resource schema.
-					return true, nil
-					//return testingCloud != LXDCloudTesting, nil
+					//return true, nil
+					return testingCloud != LXDCloudTesting, nil
 				},
 				Config: testAccResourceApplicationConstraintsSubordinate(modelName, "arch=amd64 cores=1 mem=4096M"),
 				Check: resource.ComposeTestCheckFunc(
@@ -378,7 +378,7 @@ func TestAcc_ResourceApplication_Updates_Stable(t *testing.T) {
 					//        	"placement": ",",
 					//        }
 					//--- FAIL: TestAcc_ResourceApplication_Updates_Stable (25.87s)
-					return testingCloud != MicroK8sTesting && TestProviderStableVersion == "0.8.0", nil
+					return testingCloud != MicroK8sTesting && TestProviderStableVersion == "0.9.0", nil
 				},
 				ImportStateVerify: true,
 				ImportState:       true,

diff --git a/internal/provider/resource_credential_test.go b/internal/provider/resource_credential_test.go
@@ -94,11 +94,11 @@ func TestAcc_ResourceCredential_Stable(t *testing.T) {
 				// "When tests have an ExpectError[...]; this results in any previous state being cleared. "
 				// https://github.com/hashicorp/terraform-plugin-sdk/issues/118
 				Config:      testAccResourceCredential(credentialName, authTypeInvalid),
-				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: supported auth-types (.*), \"%s\" not supported", authTypeInvalid)),
+				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: supported auth-types (.*), %q not supported", authTypeInvalid)),
 			},
 			{
 				Config:      testAccResourceCredential(credentialInvalidName, authType),
-				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: \"%s\" is not a valid credential name", credentialInvalidName)),
+				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: %q is not a valid credential name", credentialInvalidName)),
 			},
 			{
 				Config: testAccResourceCredential(credentialName, authType),

diff --git a/internal/provider/resource_model_test.go b/internal/provider/resource_model_test.go
@@ -144,7 +144,7 @@ func TestAcc_ResourceModel_Stable(t *testing.T) {
 				// "When tests have an ExpectError[...]; this results in any previous state being cleared. "
 				// https://github.com/hashicorp/terraform-plugin-sdk/issues/118
 				Config:      testAccResourceModel(modelInvalidName, testingCloud.CloudName(), logLevelInfo),
-				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: \"%s\" is not a valid name: model names may only contain lowercase letters, digits and hyphens", modelInvalidName)),
+				ExpectError: regexp.MustCompile(fmt.Sprintf("Error: %q is not a valid name: model names may only contain lowercase letters, digits and hyphens", modelInvalidName)),
 			},
 			{
 				Config: testAccResourceModel(modelName, testingCloud.CloudName(), logLevelInfo),