From 57285329b320daabd53b6b40a3b03fdc6e3632df Mon Sep 17 00:00:00 2001 From: John Collier Date: Mon, 10 Jul 2023 11:20:31 -0400 Subject: [PATCH 1/2] [RHTAPBUGS-442] Ensure gitops repository is cleaned up in error states Signed-off-by: John Collier --- ...licationsnapshotenvironmentbinding_controller.go | 13 +++++++++++-- controllers/component_controller_finalizer.go | 1 + pkg/util/ioutils/ioutils.go | 8 +++++--- pkg/util/ioutils/ioutils_test.go | 13 +++++++++++-- 4 files changed, 28 insertions(+), 7 deletions(-) diff --git a/controllers/applicationsnapshotenvironmentbinding_controller.go b/controllers/applicationsnapshotenvironmentbinding_controller.go index eb011cbb0..a169255fc 100644 --- a/controllers/applicationsnapshotenvironmentbinding_controller.go +++ b/controllers/applicationsnapshotenvironmentbinding_controller.go @@ -168,6 +168,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if err != nil { log.Error(err, fmt.Sprintf("unable to get the Component %s %v", componentName, req.NamespacedName)) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) return ctrl.Result{}, err } @@ -179,6 +180,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if hasComponent.Spec.Application != applicationName { err := fmt.Errorf("component %s does not belong to the application %s", componentName, applicationName) log.Error(err, "") + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -194,6 +196,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if isKubernetesCluster && clusterIngressDomain == "" { err = fmt.Errorf("ingress domain cannot be empty on a Kubernetes cluster") log.Error(err, "unable to create an ingress resource on a Kubernetes cluster") + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -205,6 +208,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if err != nil { errMsg := fmt.Sprintf("Unable to parse the devfile from Component status, exiting reconcile loop %v", req.NamespacedName) log.Error(err, errMsg) + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, fmt.Errorf("%v: %v", errMsg, err)) return ctrl.Result{}, err } @@ -212,6 +216,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re deployAssociatedComponents, err := devfileParser.GetDeployComponents(compDevfileData) if err != nil { log.Error(err, "unable to get deploy components") + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -221,6 +226,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re hostname, err = devfile.GetIngressHostName(hasComponent.Name, appSnapshotEnvBinding.Namespace, clusterIngressDomain) if err != nil { log.Error(err, fmt.Sprintf("unable to get generate a host name from an ingress domain for %s %v", hasComponent.Name, req.NamespacedName)) + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -231,6 +237,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re kubernetesResources, err := devfile.GetResourceFromDevfile(log, compDevfileData, deployAssociatedComponents, hasComponent.Name, hasComponent.Spec.Application, hasComponent.Spec.ContainerImage, hostname) if err != nil { log.Error(err, "unable to get kubernetes resources from the devfile outerloop components") + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -269,13 +276,14 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if imageName == "" { err := fmt.Errorf("application snapshot %s did not reference component %s", snapshotName, componentName) log.Error(err, "") - r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) return ctrl.Result{}, err } gitOpsRemoteURL, gitOpsBranch, gitOpsContext, err := util.ProcessGitOpsStatus(hasComponent.Status.GitOps, ghClient.Token) if err != nil { r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) return ctrl.Result{}, err } @@ -347,7 +355,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re err = r.Generator.GenerateOverlaysAndPush(tempDir, clone, gitOpsRemoteURL, genOptions, applicationName, environmentName, imageName, "", r.AppFS, gitOpsBranch, gitOpsContext, true, componentGeneratedResources) if err != nil { log.Error(err, fmt.Sprintf("unable to get generate gitops resources for %s %v", componentName, req.NamespacedName)) - _ = r.AppFS.RemoveAll(tempDir) // not worried with an err, its a best case attempt to delete the temp clone dir + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) // not worried with an err, its a best case attempt to delete the temp clone dir r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } @@ -360,6 +368,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re if commitID, err = r.Generator.GetCommitIDFromRepo(r.AppFS, repoPath); err != nil { //gitops generator errors are sanitized log.Error(err, "") + ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err } diff --git a/controllers/component_controller_finalizer.go b/controllers/component_controller_finalizer.go index d37142dc8..74e5dee94 100644 --- a/controllers/component_controller_finalizer.go +++ b/controllers/component_controller_finalizer.go @@ -86,6 +86,7 @@ func (r *ComponentReconciler) Finalize(ctx context.Context, component *appstudio //Gitops functions return sanitized error messages err = r.Generator.GitRemoveComponent(tempDir, gitOpsURL, component.Name, gitOpsBranch, gitOpsContext) if err != nil { + ioutils.RemoveFolderAndLogError(r.Log, r.AppFS, tempDir) return err } diff --git a/pkg/util/ioutils/ioutils.go b/pkg/util/ioutils/ioutils.go index c981d7d46..6152fee0b 100644 --- a/pkg/util/ioutils/ioutils.go +++ b/pkg/util/ioutils/ioutils.go @@ -61,8 +61,10 @@ func CreateTempPath(prefix string, appFs afero.Afero) (string, error) { // RemoveFolderAndLogError removes the specified folder. If the delete fails, no error is returned, but an error is logged // Used in cases where we're cleaning up after encountering an error, but want to return the original error instead. func RemoveFolderAndLogError(log logr.Logger, fs afero.Fs, path string) { - err := fs.RemoveAll(path) - if err != nil { - log.Error(err, fmt.Sprintf("Unable to delete folder %s", path)) + if path != "" { + err := fs.RemoveAll(path) + if err != nil { + log.Error(err, fmt.Sprintf("Unable to delete folder %s", path)) + } } } diff --git a/pkg/util/ioutils/ioutils_test.go b/pkg/util/ioutils/ioutils_test.go index 47ed7d905..917431580 100644 --- a/pkg/util/ioutils/ioutils_test.go +++ b/pkg/util/ioutils/ioutils_test.go @@ -164,6 +164,7 @@ func TestRemoveFolderAndLogError(t *testing.T) { tests := []struct { name string fs afero.Afero + path string }{ { name: "inmemory fs", @@ -172,21 +173,29 @@ func TestRemoveFolderAndLogError(t *testing.T) { { name: "read only fs", fs: readOnlyFs, + path: "/somepath", }, { name: "local fs", fs: fs, }, + { + name: "empty path", + fs: fs, + path: "", + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - path, _ := CreateTempPath("TestCreateTempPath", tt.fs) + if tt.path == "" && tt.name != "empty path" { + tt.path, _ = CreateTempPath("TestCreateTempPath", tt.fs) + } log := zap.New(zap.UseFlagOptions(&zap.Options{ Development: true, TimeEncoder: zapcore.ISO8601TimeEncoder, })) - RemoveFolderAndLogError(log, tt.fs, path) + RemoveFolderAndLogError(log, tt.fs, tt.path) }) } } From 5540ac324f60c7810e8a3d8268a60cca3100091f Mon Sep 17 00:00:00 2001 From: John Collier Date: Tue, 11 Jul 2023 12:34:06 -0400 Subject: [PATCH 2/2] Fix typo --- controllers/applicationsnapshotenvironmentbinding_controller.go | 1 + 1 file changed, 1 insertion(+) diff --git a/controllers/applicationsnapshotenvironmentbinding_controller.go b/controllers/applicationsnapshotenvironmentbinding_controller.go index a169255fc..3a6a91782 100644 --- a/controllers/applicationsnapshotenvironmentbinding_controller.go +++ b/controllers/applicationsnapshotenvironmentbinding_controller.go @@ -277,6 +277,7 @@ func (r *SnapshotEnvironmentBindingReconciler) Reconcile(ctx context.Context, re err := fmt.Errorf("application snapshot %s did not reference component %s", snapshotName, componentName) log.Error(err, "") ioutils.RemoveFolderAndLogError(log, r.AppFS, tempDir) + r.SetConditionAndUpdateCR(ctx, req, &appSnapshotEnvBinding, err) return ctrl.Result{}, err }