From 44a5f6ec7552d6817a259359fd20b93694b59278 Mon Sep 17 00:00:00 2001 From: Heather Lanigan Date: Fri, 12 Jul 2024 16:41:14 -0400 Subject: [PATCH] fix(application): don't return nil if there's an error. Related to #520, panic on nil pointer from ReadApplicationWithRetryOnNotFound. Add IsFatalError func to retry. Allow for known types or "connection refused" which is intermittent. Doesn't guarentee that everything works as expected, but doesn't panic either. --- internal/juju/applications.go | 42 ++++++++++++++++++------------ internal/juju/applications_test.go | 19 ++++++++++++++ 2 files changed, 45 insertions(+), 16 deletions(-) diff --git a/internal/juju/applications.go b/internal/juju/applications.go index f9b101ce..b01e01f2 100644 --- a/internal/juju/applications.go +++ b/internal/juju/applications.go @@ -71,6 +71,17 @@ func (se *storageNotFoundError) Error() string { return fmt.Sprintf("storage %s not found", se.storageName) } +var RetryReadError = &retryReadError{} + +// retryReadError +type retryReadError struct { + msg string +} + +func (e *retryReadError) Error() string { + return fmt.Sprintf("retrying: %s", e.msg) +} + type applicationsClient struct { SharedClient controllerVersion version.Number @@ -756,11 +767,7 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte if errors.As(err, &ApplicationNotFoundError) || errors.As(err, &StorageNotFoundError) { return err } else if err != nil { - // Log the error to the terraform Diagnostics to be - // caught in the provider. Return nil so we stop - // retrying. - c.Errorf(err, fmt.Sprintf("ReadApplicationWithRetryOnNotFound %q", input.AppName)) - return nil + return err } // NOTE: Applications can always have storage. However, they @@ -768,9 +775,9 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte // we need to wait for the storage to be ready. And we need to // check if all storage constraints have pool equal "" and size equal 0 // to drop the error. - for _, storage := range output.Storage { + for label, storage := range output.Storage { if storage.Pool == "" || storage.Size == 0 { - return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no storage found in output") + return &retryReadError{msg: fmt.Sprintf("storage label %q missing detail", label)} } } @@ -784,21 +791,25 @@ func (c applicationsClient) ReadApplicationWithRetryOnNotFound(ctx context.Conte return nil } if output.Placement == "" { - return fmt.Errorf("ReadApplicationWithRetryOnNotFound: no machines found in output") + return &retryReadError{msg: "no machines found in output"} } machines := strings.Split(output.Placement, ",") if len(machines) != output.Units { - return fmt.Errorf("ReadApplicationWithRetryOnNotFound: need %d machines, have %d", output.Units, len(machines)) + return &retryReadError{msg: fmt.Sprintf("need %d machines, have %d", output.Units, len(machines))} } - // NOTE: An IAAS subordinate should also have machines. However, they - // will not be listed until after the relation has been created. - // Those happen with the integration resource which will not be - // run by terraform before the application resource finishes. Thus - // do not block here for subordinates. c.Tracef("Have machines - returning", map[string]interface{}{"output": *output}) return nil }, + IsFatalError: func(err error) bool { + if errors.As(err, &ApplicationNotFoundError) || + errors.As(err, &StorageNotFoundError) || + errors.As(err, &RetryReadError) || + strings.Contains(err.Error(), "connection refused") { + return false + } + return true + }, NotifyFunc: func(err error, attempt int) { if attempt%4 == 0 { message := fmt.Sprintf("waiting for application %q", input.AppName) @@ -874,8 +885,7 @@ func (c applicationsClient) ReadApplication(input *ReadApplicationInput) (*ReadA apps, err := applicationAPIClient.ApplicationsInfo([]names.ApplicationTag{names.NewApplicationTag(input.AppName)}) if err != nil { - c.Errorf(err, "found when querying the applications info") - return nil, err + return nil, jujuerrors.Annotate(err, "when querying the applications info") } if len(apps) > 1 { return nil, fmt.Errorf("more than one result for application: %s", input.AppName) diff --git a/internal/juju/applications_test.go b/internal/juju/applications_test.go index ec7b996f..0235cc8f 100644 --- a/internal/juju/applications_test.go +++ b/internal/juju/applications_test.go @@ -6,6 +6,7 @@ package juju // Basic imports import ( "context" + "fmt" "testing" "github.com/juju/juju/api" @@ -168,6 +169,24 @@ func (s *ApplicationSuite) TestReadApplicationRetry() { s.Assert().Equal("ubuntu@22.04", resp.Base) } +func (s *ApplicationSuite) TestReadApplicationRetryDoNotPanic() { + defer s.setupMocks(s.T()).Finish() + s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes() + + appName := "testapplication" + aExp := s.mockApplicationClient.EXPECT() + + aExp.ApplicationsInfo(gomock.Any()).Return(nil, fmt.Errorf("don't panic")) + + client := s.getApplicationsClient() + _, err := client.ReadApplicationWithRetryOnNotFound(context.Background(), + &ReadApplicationInput{ + ModelName: s.testModelName, + AppName: appName, + }) + s.Require().Error(err, "don't panic") +} + func (s *ApplicationSuite) TestReadApplicationRetryWaitForMachines() { defer s.setupMocks(s.T()).Finish() s.mockSharedClient.EXPECT().ModelType(gomock.Any()).Return(model.IAAS, nil).AnyTimes()