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 9966f9e9..d59bedac 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 74bec7f0..1eb43e49 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) {