From 95d1c21c9dd5f69e6dbdb922e3f0934f14426110 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 11 Jul 2024 17:05:10 -0400 Subject: [PATCH 1/3] fix(application): wait for complete on destroy When destroying an application, wait for it to be destroyed before returning. Issue #521 and maybe others. Otherwise terraform fails when RequireReplace is set. --- internal/juju/applications.go | 91 ++++++++++++++++++++--- internal/juju/applications_test.go | 52 +++++++++++++ internal/provider/resource_application.go | 9 ++- 3 files changed, 139 insertions(+), 13 deletions(-) diff --git a/internal/juju/applications.go b/internal/juju/applications.go index c8ba9813..4ba99d09 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -57,7 +57,7 @@ type applicationNotFoundError struct { } func (ae *applicationNotFoundError) Error() string { - return fmt.Sprintf("application %s not found", ae.appName) + return fmt.Sprintf("application %q not found", ae.appName) } var StorageNotFoundError = &storageNotFoundError{} @@ -68,7 +68,20 @@ type storageNotFoundError struct { } func (se *storageNotFoundError) Error() string { - return fmt.Sprintf("storage %s not found", se.storageName) + return fmt.Sprintf("storage %q not found", se.storageName) +} + +var KeepWaitingForDestroyError = &keepWaitingForDestroyError{} + +// keepWaitingForDestroyError +type keepWaitingForDestroyError struct { + itemDestroying string + life string +} + +func (e *keepWaitingForDestroyError) Error() string { + + return fmt.Sprintf("%q still alive, life = %s", e.itemDestroying, e.life) } type applicationsClient struct { @@ -158,7 +171,7 @@ type CreateApplicationInput struct { // validateAndTransform returns transformedCreateApplicationInput which // validated and in the proper format for both the new and legacy deployment // methods. Select input is not transformed due to differences in the -// 2 deployement methods, such as config. +// 2 deployment methods, such as config. func (input CreateApplicationInput) validateAndTransform(conn api.Connection) (parsed transformedCreateApplicationInput, err error) { parsed.charmChannel = input.CharmChannel parsed.charmName = input.CharmName @@ -1268,29 +1281,89 @@ func (c applicationsClient) UpdateApplication(input *UpdateApplicationInput) err return nil } -func (c applicationsClient) DestroyApplication(input *DestroyApplicationInput) error { +func (c applicationsClient) DestroyApplication(ctx context.Context, input *DestroyApplicationInput) error { conn, err := c.GetConnection(&input.ModelName) if err != nil { return err } defer func() { _ = conn.Close() }() - applicationAPIClient := apiapplication.NewClient(conn) + applicationAPIClient := c.getApplicationAPIClient(conn) + appName := input.ApplicationName var destroyParams = apiapplication.DestroyApplicationsParams{ Applications: []string{ - input.ApplicationName, + appName, }, DestroyStorage: true, } - _, err = applicationAPIClient.DestroyApplications(destroyParams) - + results, err := applicationAPIClient.DestroyApplications(destroyParams) if err != nil { return err } + if len(results) != 1 { + return errors.New(fmt.Sprintf("Unexpected number of results from DestroyApplication for %q", appName)) + } + if results[0].Error != nil { + err := typedError(results[0].Error) + // No need to error if the application is not found. It may have been + // destroyed as part of a different destroy operation, e.g. machine. + // Only care that it's gone. + if jujuerrors.Is(err, jujuerrors.NotFound) { + return nil + } + return err + } - return nil + // Best effort wait for the application to be destroyed. + retryErr := retry.Call(retry.CallArgs{ + Func: func() error { + apps, err := applicationAPIClient.ApplicationsInfo([]names.ApplicationTag{names.NewApplicationTag(appName)}) + if err != nil { + return jujuerrors.Annotatef(err, fmt.Sprintf("querying the applications info after destroy on %q", appName)) + } + if len(apps) != 1 { + c.Debugf(fmt.Sprintf("unexpected number of results (%d) for application %q info after destroy", len(apps), appName)) + return nil + } + if apps[0].Error != nil { + typedErr := typedError(apps[0].Error) + if jujuerrors.Is(typedErr, jujuerrors.NotFound) { + c.Tracef(fmt.Sprintf("%q not found, application has been destroyed", appName)) + return nil + } + return jujuerrors.Annotatef(typedErr, fmt.Sprintf("verifying application %q distruction", appName)) + } + + life := apps[0].Result.Life + return &keepWaitingForDestroyError{ + itemDestroying: appName, + life: life, + } + }, + NotifyFunc: func(err error, attempt int) { + if attempt%4 == 0 { + message := fmt.Sprintf("waiting for application %q to be destroyed", appName) + if attempt != 4 { + message = "still " + message + } + c.Debugf(message, map[string]interface{}{"attempt": attempt, "err": err}) + } + }, + IsFatalError: func(err error) bool { + if errors.As(err, &KeepWaitingForDestroyError) { + return false + } + return true + }, + BackoffFunc: retry.DoubleDelay, + Attempts: 30, + Delay: time.Second, + Clock: clock.WallClock, + Stop: ctx.Done(), + }) + return retryErr } // computeSetCharmConfig populates the corresponding configuration object diff --git a/internal/juju/applications_test.go b/internal/juju/applications_test.go index ec7b996f..02a8d76d 100644 --- a/internal/juju/applications_test.go +++ b/internal/juju/applications_test.go @@ -364,6 +364,58 @@ func (s *ApplicationSuite) TestReadApplicationRetryNotFoundStorageNotFoundError( s.Assert().Equal("ubuntu@22.04", resp.Base) } +func (s *ApplicationSuite) TestDestroyApplicationDoNotFailOnNotFound() { + defer s.setupMocks(s.T()).Finish() + s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes() + + appName := "testapplication" + aExp := s.mockApplicationClient.EXPECT() + + aExp.DestroyApplications(gomock.Any()).Return([]params.DestroyApplicationResult{{ + Error: ¶ms.Error{Message: `application "testapplication" not found`, Code: "not found"}, + }}, nil) + + client := s.getApplicationsClient() + err := client.DestroyApplication(context.Background(), + &DestroyApplicationInput{ + ApplicationName: appName, + ModelName: s.testModelName, + }) + s.Require().NoError(err) +} + +func (s *ApplicationSuite) TestDestroyApplicationRetry() { + defer s.setupMocks(s.T()).Finish() + s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes() + + appName := "testapplication" + aExp := s.mockApplicationClient.EXPECT() + + aExp.DestroyApplications(gomock.Any()).Return([]params.DestroyApplicationResult{{ + Info: nil, Error: nil, + }}, nil) + + infoResult := params.ApplicationInfoResult{ + Result: ¶ms.ApplicationResult{ + Life: "dying", + }, + Error: nil, + } + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{infoResult}, nil) + + aExp.ApplicationsInfo(gomock.Any()).Return([]params.ApplicationInfoResult{{ + Error: ¶ms.Error{Message: `application "testapplication" not found`, Code: "not found"}, + }}, nil) + + client := s.getApplicationsClient() + err := client.DestroyApplication(context.Background(), + &DestroyApplicationInput{ + ApplicationName: appName, + ModelName: s.testModelName, + }) + s.Require().NoError(err) +} + // In order for 'go test' to run this suite, we need to create // a normal test function and pass our suite to suite.Run func TestApplicationSuite(t *testing.T) { diff --git a/internal/provider/resource_application.go b/internal/provider/resource_application.go index cf885ad0..b1d3ea22 100644 --- a/internal/provider/resource_application.go +++ b/internal/provider/resource_application.go @@ -1230,10 +1230,11 @@ func (r *applicationResource) Delete(ctx context.Context, req resource.DeleteReq resp.Diagnostics.Append(dErr...) } - if err := r.client.Applications.DestroyApplication(&juju.DestroyApplicationInput{ - ApplicationName: appName, - ModelName: modelName, - }); err != nil { + if err := r.client.Applications.DestroyApplication(ctx, + &juju.DestroyApplicationInput{ + ApplicationName: appName, + ModelName: modelName, + }); err != nil { resp.Diagnostics.AddError("Client Error", fmt.Sprintf("Unable to delete application, got error: %s", err)) } r.trace(fmt.Sprintf("deleted application resource %q", state.ID.ValueString())) From d01ddda2258b8b9666d139a484f317fe8e579010 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 11 Jul 2024 17:16:54 -0400 Subject: [PATCH 2/3] refactor: move error types to the error file Replace ProcessErrorResults with errors.Join which does the same thing for us. --- internal/juju/applications.go | 35 ---------------- internal/juju/errors.go | 76 +++++++++++++++++++++++++++++++++++ internal/juju/integrations.go | 11 ----- internal/juju/models.go | 28 ++++--------- internal/juju/secrets.go | 22 +--------- internal/juju/utils.go | 8 ---- 6 files changed, 85 insertions(+), 95 deletions(-) diff --git a/internal/juju/applications.go b/internal/juju/applications.go index 4ba99d09..6ed580c1 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -49,41 +49,6 @@ import ( goyaml "gopkg.in/yaml.v2" ) -var ApplicationNotFoundError = &applicationNotFoundError{} - -// ApplicationNotFoundError -type applicationNotFoundError struct { - appName string -} - -func (ae *applicationNotFoundError) Error() string { - return fmt.Sprintf("application %q not found", ae.appName) -} - -var StorageNotFoundError = &storageNotFoundError{} - -// StorageNotFoundError -type storageNotFoundError struct { - storageName string -} - -func (se *storageNotFoundError) Error() string { - return fmt.Sprintf("storage %q not found", se.storageName) -} - -var KeepWaitingForDestroyError = &keepWaitingForDestroyError{} - -// keepWaitingForDestroyError -type keepWaitingForDestroyError struct { - itemDestroying string - life string -} - -func (e *keepWaitingForDestroyError) Error() string { - - return fmt.Sprintf("%q still alive, life = %s", e.itemDestroying, e.life) -} - type applicationsClient struct { SharedClient controllerVersion version.Number diff --git a/internal/juju/errors.go b/internal/juju/errors.go index 92cb153c..3d7405f6 100644 --- a/internal/juju/errors.go +++ b/internal/juju/errors.go @@ -4,6 +4,7 @@ package juju import ( + "fmt" "strings" jujuerrors "github.com/juju/errors" @@ -30,3 +31,78 @@ func typedError(err error) error { return err } } + +var ApplicationNotFoundError = &applicationNotFoundError{} + +// ApplicationNotFoundError +type applicationNotFoundError struct { + appName string +} + +func (ae *applicationNotFoundError) Error() string { + return fmt.Sprintf("application %q not found", ae.appName) +} + +var StorageNotFoundError = &storageNotFoundError{} + +// StorageNotFoundError +type storageNotFoundError struct { + storageName string +} + +func (se *storageNotFoundError) Error() string { + return fmt.Sprintf("storage %q not found", se.storageName) +} + +var KeepWaitingForDestroyError = &keepWaitingForDestroyError{} + +// keepWaitingForDestroyError +type keepWaitingForDestroyError struct { + itemDestroying string + life string +} + +func (e *keepWaitingForDestroyError) Error() string { + + return fmt.Sprintf("%q still alive, life = %s", e.itemDestroying, e.life) +} + +var NoIntegrationFoundError = &noIntegrationFoundError{} + +// NoIntegrationFoundError +type noIntegrationFoundError struct { + ModelUUID string +} + +func (ie *noIntegrationFoundError) Error() string { + return fmt.Sprintf("no integrations exist in model %v", ie.ModelUUID) +} + +var ModelNotFoundError = &modelNotFoundError{} + +type modelNotFoundError struct { + uuid string + name string +} + +func (me *modelNotFoundError) Error() string { + toReturn := "model %q was not found" + if me.name != "" { + return fmt.Sprintf(toReturn, me.name) + } + return fmt.Sprintf(toReturn, me.uuid) +} + +var SecretNotFoundError = &secretNotFoundError{} + +type secretNotFoundError struct { + secretId string +} + +func (se *secretNotFoundError) Error() string { + if se.secretId != "" { + return fmt.Sprintf("secret %q was not found", se.secretId) + } else { + return "secret was not found" + } +} diff --git a/internal/juju/integrations.go b/internal/juju/integrations.go index 1656d4e2..ac69f86c 100644 --- a/internal/juju/integrations.go +++ b/internal/juju/integrations.go @@ -27,17 +27,6 @@ const ( IntegrationAppAvailableTimeout = time.Second * 60 ) -var NoIntegrationFoundError = &noIntegrationFoundError{} - -// NoIntegrationFoundError -type noIntegrationFoundError struct { - ModelUUID string -} - -func (ie *noIntegrationFoundError) Error() string { - return fmt.Sprintf("no integrations exist in model %v", ie.ModelUUID) -} - type integrationsClient struct { SharedClient } diff --git a/internal/juju/models.go b/internal/juju/models.go index 33358114..edbf6099 100644 --- a/internal/juju/models.go +++ b/internal/juju/models.go @@ -7,7 +7,7 @@ import ( "fmt" "time" - "github.com/juju/errors" + jujuerrors "github.com/juju/errors" "github.com/juju/juju/api/client/modelconfig" "github.com/juju/juju/api/client/modelmanager" "github.com/juju/juju/core/constraints" @@ -15,21 +15,6 @@ import ( "github.com/juju/names/v5" ) -var ModelNotFoundError = &modelNotFoundError{} - -type modelNotFoundError struct { - uuid string - name string -} - -func (me *modelNotFoundError) Error() string { - toReturn := "model %q was not found" - if me.name != "" { - return fmt.Sprintf(toReturn, me.name) - } - return fmt.Sprintf(toReturn, me.uuid) -} - type modelsClient struct { SharedClient } @@ -209,7 +194,7 @@ func (c *modelsClient) ReadModel(name string) (*ReadModelResponse, error) { modelconfigConn, err := c.GetConnection(&name) if err != nil { - return nil, errors.Wrap(err, &modelNotFoundError{uuid: name}) + return nil, jujuerrors.Wrap(err, &modelNotFoundError{uuid: name}) } defer func() { _ = modelconfigConn.Close() }() @@ -218,7 +203,7 @@ func (c *modelsClient) ReadModel(name string) (*ReadModelResponse, error) { modelUUIDTag, modelOk := modelconfigConn.ModelTag() if !modelOk { - return nil, errors.Errorf("Not connected to model %q", name) + return nil, jujuerrors.Errorf("Not connected to model %q", name) } models, err := modelmanagerClient.ModelInfo([]names.ModelTag{modelUUIDTag}) if err != nil { @@ -304,7 +289,7 @@ func (c *modelsClient) UpdateModel(input UpdateModelInput) error { defer func() { _ = conn.Close() }() modelUUIDTag, modelOk := conn.ModelTag() if !modelOk { - return errors.Errorf("Not connected to model %q", input.Name) + return jujuerrors.Errorf("Not connected to model %q", input.Name) } clientModelManager := modelmanager.NewClient(connModelManager) if err := clientModelManager.ChangeModelCredential(modelUUIDTag, *cloudCredTag); err != nil { @@ -334,7 +319,10 @@ func (c *modelsClient) DestroyModel(input DestroyModelInput) error { err = client.DestroyModel(tag, &destroyStorage, &forceDestroy, &maxWait, &timeout) if err != nil { - return err + typedErr := typedError(err) + if !jujuerrors.Is(typedErr, jujuerrors.NotFound) { + return err + } } c.RemoveModel(input.UUID) diff --git a/internal/juju/secrets.go b/internal/juju/secrets.go index 99c1ac2e..14961cb4 100644 --- a/internal/juju/secrets.go +++ b/internal/juju/secrets.go @@ -6,7 +6,6 @@ package juju import ( "encoding/base64" "errors" - "fmt" "strings" jujuerrors "github.com/juju/errors" @@ -15,20 +14,6 @@ import ( coresecrets "github.com/juju/juju/core/secrets" ) -var SecretNotFoundError = &secretNotFoundError{} - -type secretNotFoundError struct { - secretId string -} - -func (se *secretNotFoundError) Error() string { - if se.secretId != "" { - return fmt.Sprintf("secret %q was not found", se.secretId) - } else { - return "secret was not found" - } -} - type secretsClient struct { SharedClient @@ -303,12 +288,7 @@ func (c *secretsClient) UpdateAccessSecret(input *GrantRevokeAccessSecretInput, if err != nil { return typedError(err) } - err = ProcessErrorResults(results) - if err != nil { - return err - } - - return nil + return errors.Join(results...) } // getApplicationsFromAccessInfo returns a list of applications from the access info. diff --git a/internal/juju/utils.go b/internal/juju/utils.go index dd558c83..8bd66d94 100644 --- a/internal/juju/utils.go +++ b/internal/juju/utils.go @@ -152,11 +152,3 @@ func WaitForAppsAvailable(ctx context.Context, client *apiapplication.Client, ap } } } - -// ProcessErrorResults processes the results of a secret operation. -func ProcessErrorResults(results []error) error { - if results[0] != nil && len(results) > 1 { - return &MultiError{Errors: results} - } - return nil -} From 171557100a5ae34cecf04b9ce2a8e2a1c62f853d Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Thu, 11 Jul 2024 17:19:57 -0400 Subject: [PATCH 3/3] refactor: stop using 2 version of the juju/names package Two versions of the juju/names pacakge were directly used. Luckily there was no conflict. Move to the most recent. --- go.mod | 1 - go.sum | 2 -- internal/juju/applications_test.go | 2 +- internal/provider/resource_model.go | 2 +- 4 files changed, 2 insertions(+), 5 deletions(-) diff --git a/go.mod b/go.mod index 4d170508..fb4b444d 100644 --- a/go.mod +++ b/go.mod @@ -22,7 +22,6 @@ require ( github.com/juju/cmd/v3 v3.0.14 github.com/juju/collections v1.0.4 github.com/juju/errors v1.0.0 - github.com/juju/names/v4 v4.0.0-20220207005702-9c6532a52823 github.com/juju/names/v5 v5.0.0 github.com/juju/retry v1.0.0 github.com/juju/utils/v3 v3.1.1 diff --git a/go.sum b/go.sum index f785694a..9ee02fa9 100644 --- a/go.sum +++ b/go.sum @@ -366,8 +366,6 @@ github.com/juju/mgo/v3 v3.0.4 h1:ek6YDy71tqikpoFSpvLkpCZ7zvYNYH+xSk/MebMkCEE= github.com/juju/mgo/v3 v3.0.4/go.mod h1:fAvhDCRbUlEbRIae6UQT8RvPUoLwKnJsBgO6OzHKNxw= github.com/juju/mutex/v2 v2.0.0 h1:rVmJdOaXGWF8rjcFHBNd4x57/1tks5CgXHx55O55SB0= github.com/juju/mutex/v2 v2.0.0/go.mod h1:jwCfBs/smYDaeZLqeaCi8CB8M+tOes4yf827HoOEoqk= -github.com/juju/names/v4 v4.0.0-20220207005702-9c6532a52823 h1:Sv0+v4107/GHA0S25ay/rgGVmLyc+5Fjp0NnTksW/IQ= -github.com/juju/names/v4 v4.0.0-20220207005702-9c6532a52823/go.mod h1:xpkrQpHbz1DGY+0Geo32ZnyognGA/2vSB++rpu/Z+Lc= github.com/juju/names/v5 v5.0.0 h1:3IkRTUaniNXsgjy4lNqbJx7dVdsONlzuH6YMYT7uXss= github.com/juju/names/v5 v5.0.0/go.mod h1:PkvHbErUTniKvLu1ejJ5m/AbXOW55MFn1jsGVEbVXk8= github.com/juju/naturalsort v1.0.0 h1:kGmUUy3h8mJ5/SJYaqKOBR3f3owEd5R52Lh+Tjg/dNM= diff --git a/internal/juju/applications_test.go b/internal/juju/applications_test.go index 02a8d76d..e85bbf1f 100644 --- a/internal/juju/applications_test.go +++ b/internal/juju/applications_test.go @@ -15,7 +15,7 @@ import ( "github.com/juju/juju/core/resources" "github.com/juju/juju/environs/config" "github.com/juju/juju/rpc/params" - "github.com/juju/names/v4" + "github.com/juju/names/v5" "github.com/juju/utils/v3" "github.com/juju/version/v2" "github.com/stretchr/testify/suite" diff --git a/internal/provider/resource_model.go b/internal/provider/resource_model.go index 442f8710..8cac55e9 100644 --- a/internal/provider/resource_model.go +++ b/internal/provider/resource_model.go @@ -24,7 +24,7 @@ import ( "github.com/hashicorp/terraform-plugin-framework/types" "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/juju/juju/core/constraints" - "github.com/juju/names/v4" + "github.com/juju/names/v5" "github.com/juju/utils/v3" "github.com/juju/terraform-provider-juju/internal/juju"