From 9b5613732f9a79b3a10a09118e973bff782b51d1 Mon Sep 17 00:00:00 2001 From: Sunny Date: Mon, 31 Jan 2022 16:00:24 +0530 Subject: [PATCH] storage: Return details about the deleted items Update Storage.RemoveAll() and Storage.RemoveAllButCurrent() to return the details about the deleted items. This helps emit useful information about garbage collection in the controllers and ignore no-op garbage collections. RemoveAll() returns the path of the deleted directory if any. RemoveAllButCurrent() returns a slice of path of all the deleted items from a resource's artifact dir. Signed-off-by: Sunny --- controllers/bucket_controller.go | 15 ++-- controllers/gitrepository_controller.go | 16 ++--- controllers/helmchart_controller.go | 15 ++-- controllers/helmrepository_controller.go | 16 ++--- controllers/storage.go | 20 ++++-- controllers/storage_test.go | 89 +++++++++++++++++++++++- 6 files changed, 135 insertions(+), 36 deletions(-) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 54270940..7d520c41 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -741,27 +741,28 @@ func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.Bu // resource. func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Bucket) error { if !obj.DeletionTimestamp.IsZero() { - if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { + if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection for deleted resource failed: %s", err), Reason: "GarbageCollectionFailed", } + } else if deleted != "" { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") } obj.Status.Artifact = nil - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected artifacts for deleted resource") return nil } if obj.GetArtifact() != nil { - if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { + if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection of old artifacts failed: %s", err), Reason: "GarbageCollectionFailed", } + } else if len(deleted) > 0 { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts") } return nil } diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 34bb2611..bbf63c6c 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -642,27 +642,27 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj // resource. func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.GitRepository) error { if !obj.DeletionTimestamp.IsZero() { - if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { + if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if deleted != "" { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") } obj.Status.Artifact = nil - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected artifacts for deleted resource") return nil } if obj.GetArtifact() != nil { - if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { + if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), } + } else if len(deleted) > 0 { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected old artifacts") } return nil } diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index e8a5c015..d767be01 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -750,27 +750,28 @@ func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, obj *sourcev1 // resource. func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmChart) error { if !obj.DeletionTimestamp.IsZero() { - if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { + if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if deleted != "" { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") } obj.Status.Artifact = nil - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected artifacts for deleted resource") return nil } if obj.GetArtifact() != nil { - if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { + if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if len(deleted) > 0 { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts") } return nil } diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index ff55c78d..7386c6bb 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -499,28 +499,28 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sou // resource. func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error { if !obj.DeletionTimestamp.IsZero() { - if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { + if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if deleted != "" { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected artifacts for deleted resource") } obj.Status.Artifact = nil - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected artifacts for deleted resource") return nil } if obj.GetArtifact() != nil { - if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { + if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil { return &serror.Event{ Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err), Reason: "GarbageCollectionFailed", } + } else if len(deleted) > 0 { + r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", + "garbage collected old artifacts") } - // TODO(hidde): we should only push this event if we actually garbage collected something - r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", - "garbage collected old artifacts") } return nil } diff --git a/controllers/storage.go b/controllers/storage.go index 8f892da6..0e9e5fe8 100644 --- a/controllers/storage.go +++ b/controllers/storage.go @@ -105,13 +105,20 @@ func (s *Storage) MkdirAll(artifact sourcev1.Artifact) error { } // RemoveAll calls os.RemoveAll for the given v1beta1.Artifact base dir. -func (s *Storage) RemoveAll(artifact sourcev1.Artifact) error { +func (s *Storage) RemoveAll(artifact sourcev1.Artifact) (string, error) { + var deletedDir string dir := filepath.Dir(s.LocalPath(artifact)) - return os.RemoveAll(dir) + // Check if the dir exists. + _, err := os.Stat(dir) + if err == nil { + deletedDir = dir + } + return deletedDir, os.RemoveAll(dir) } // RemoveAllButCurrent removes all files for the given v1beta1.Artifact base dir, excluding the current one. -func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error { +func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) ([]string, error) { + deletedFiles := []string{} localPath := s.LocalPath(artifact) dir := filepath.Dir(localPath) var errors []string @@ -124,15 +131,18 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error { if path != localPath && !info.IsDir() && info.Mode()&os.ModeSymlink != os.ModeSymlink { if err := os.Remove(path); err != nil { errors = append(errors, info.Name()) + } else { + // Collect the successfully deleted file paths. + deletedFiles = append(deletedFiles, path) } } return nil }) if len(errors) > 0 { - return fmt.Errorf("failed to remove files: %s", strings.Join(errors, " ")) + return deletedFiles, fmt.Errorf("failed to remove files: %s", strings.Join(errors, " ")) } - return nil + return deletedFiles, nil } // ArtifactExist returns a boolean indicating whether the v1beta1.Artifact exists in storage and is a regular file. diff --git a/controllers/storage_test.go b/controllers/storage_test.go index c3e7393e..7da575c6 100644 --- a/controllers/storage_test.go +++ b/controllers/storage_test.go @@ -28,6 +28,7 @@ import ( "time" "github.com/go-git/go-git/v5/plumbing/format/gitignore" + . "github.com/onsi/gomega" sourcev1 "github.com/fluxcd/source-controller/api/v1beta2" ) @@ -293,10 +294,96 @@ func TestStorageRemoveAllButCurrent(t *testing.T) { t.Fatalf("Valid path did not successfully return: %v", err) } - if err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil { + if _, err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil { t.Fatal("Did not error while pruning non-existent path") } }) + + t.Run("collect names of deleted items", func(t *testing.T) { + g := NewWithT(t) + dir, err := os.MkdirTemp("", "") + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { os.RemoveAll(dir) }) + + s, err := NewStorage(dir, "hostname", time.Minute) + g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage") + + artifact := sourcev1.Artifact{ + Path: path.Join("foo", "bar", "artifact1.tar.gz"), + } + + // Create artifact dir and artifacts. + artifactDir := path.Join(dir, "foo", "bar") + g.Expect(os.MkdirAll(artifactDir, 0755)).NotTo(HaveOccurred()) + current := []string{ + path.Join(artifactDir, "artifact1.tar.gz"), + } + wantDeleted := []string{ + path.Join(artifactDir, "file1.txt"), + path.Join(artifactDir, "file2.txt"), + } + createFile := func(files []string) { + for _, c := range files { + f, err := os.Create(c) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(f.Close()).ToNot(HaveOccurred()) + } + } + createFile(current) + createFile(wantDeleted) + _, err = s.Symlink(artifact, "latest.tar.gz") + g.Expect(err).ToNot(HaveOccurred(), "failed to create symlink") + + deleted, err := s.RemoveAllButCurrent(artifact) + g.Expect(err).ToNot(HaveOccurred(), "failed to remove all but current") + g.Expect(deleted).To(Equal(wantDeleted)) + }) +} + +func TestStorageRemoveAll(t *testing.T) { + tests := []struct { + name string + artifactPath string + createArtifactPath bool + wantDeleted string + }{ + { + name: "delete non-existent path", + artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"), + createArtifactPath: false, + wantDeleted: "", + }, + { + name: "delete existing path", + artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"), + createArtifactPath: true, + wantDeleted: path.Join("foo", "bar"), + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + g := NewWithT(t) + dir, err := os.MkdirTemp("", "") + g.Expect(err).ToNot(HaveOccurred()) + t.Cleanup(func() { os.RemoveAll(dir) }) + + s, err := NewStorage(dir, "hostname", time.Minute) + g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage") + + artifact := sourcev1.Artifact{ + Path: tt.artifactPath, + } + + if tt.createArtifactPath { + g.Expect(os.MkdirAll(path.Join(dir, tt.artifactPath), 0755)).ToNot(HaveOccurred()) + } + + deleted, err := s.RemoveAll(artifact) + g.Expect(err).ToNot(HaveOccurred()) + g.Expect(deleted).To(ContainSubstring(tt.wantDeleted), "unexpected deleted path") + }) + } } func TestStorageCopyFromPath(t *testing.T) {