Skip to content

Commit 9b56137

Browse files
darkowlzzhiddeco
darkowlzz
authored and
hiddeco
committed
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 <[email protected]>
1 parent 849f7ee commit 9b56137

File tree

6 files changed

+135
-36
lines changed

6 files changed

+135
-36
lines changed

‎controllers/bucket_controller.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -741,27 +741,28 @@ func (r *BucketReconciler) reconcileDelete(ctx context.Context, obj *sourcev1.Bu
741741
// resource.
742742
func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Bucket) error {
743743
if !obj.DeletionTimestamp.IsZero() {
744-
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
744+
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
745745
return &serror.Event{
746746
Err: fmt.Errorf("garbage collection for deleted resource failed: %s", err),
747747
Reason: "GarbageCollectionFailed",
748748
}
749+
} else if deleted != "" {
750+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
751+
"garbage collected artifacts for deleted resource")
749752
}
750753
obj.Status.Artifact = nil
751-
// TODO(hidde): we should only push this event if we actually garbage collected something
752-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
753-
"garbage collected artifacts for deleted resource")
754754
return nil
755755
}
756756
if obj.GetArtifact() != nil {
757-
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
757+
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
758758
return &serror.Event{
759759
Err: fmt.Errorf("garbage collection of old artifacts failed: %s", err),
760760
Reason: "GarbageCollectionFailed",
761761
}
762+
} else if len(deleted) > 0 {
763+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
764+
"garbage collected old artifacts")
762765
}
763-
// TODO(hidde): we should only push this event if we actually garbage collected something
764-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
765766
}
766767
return nil
767768
}

‎controllers/gitrepository_controller.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -642,27 +642,27 @@ func (r *GitRepositoryReconciler) verifyCommitSignature(ctx context.Context, obj
642642
// resource.
643643
func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.GitRepository) error {
644644
if !obj.DeletionTimestamp.IsZero() {
645-
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
645+
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
646646
return &serror.Event{
647647
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
648648
Reason: "GarbageCollectionFailed",
649649
}
650+
} else if deleted != "" {
651+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
652+
"garbage collected artifacts for deleted resource")
650653
}
651654
obj.Status.Artifact = nil
652-
// TODO(hidde): we should only push this event if we actually garbage collected something
653-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
654-
"garbage collected artifacts for deleted resource")
655655
return nil
656656
}
657657
if obj.GetArtifact() != nil {
658-
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
658+
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
659659
return &serror.Event{
660660
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
661661
}
662+
} else if len(deleted) > 0 {
663+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
664+
"garbage collected old artifacts")
662665
}
663-
// TODO(hidde): we should only push this event if we actually garbage collected something
664-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
665-
"garbage collected old artifacts")
666666
}
667667
return nil
668668
}

‎controllers/helmchart_controller.go

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -750,27 +750,28 @@ func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, obj *sourcev1
750750
// resource.
751751
func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmChart) error {
752752
if !obj.DeletionTimestamp.IsZero() {
753-
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
753+
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
754754
return &serror.Event{
755755
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
756756
Reason: "GarbageCollectionFailed",
757757
}
758+
} else if deleted != "" {
759+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
760+
"garbage collected artifacts for deleted resource")
758761
}
759762
obj.Status.Artifact = nil
760-
// TODO(hidde): we should only push this event if we actually garbage collected something
761-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
762-
"garbage collected artifacts for deleted resource")
763763
return nil
764764
}
765765
if obj.GetArtifact() != nil {
766-
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
766+
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
767767
return &serror.Event{
768768
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
769769
Reason: "GarbageCollectionFailed",
770770
}
771+
} else if len(deleted) > 0 {
772+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
773+
"garbage collected old artifacts")
771774
}
772-
// TODO(hidde): we should only push this event if we actually garbage collected something
773-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded", "garbage collected old artifacts")
774775
}
775776
return nil
776777
}

‎controllers/helmrepository_controller.go

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -499,28 +499,28 @@ func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, obj *sou
499499
// resource.
500500
func (r *HelmRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourcev1.HelmRepository) error {
501501
if !obj.DeletionTimestamp.IsZero() {
502-
if err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
502+
if deleted, err := r.Storage.RemoveAll(r.Storage.NewArtifactFor(obj.Kind, obj.GetObjectMeta(), "", "*")); err != nil {
503503
return &serror.Event{
504504
Err: fmt.Errorf("garbage collection for deleted resource failed: %w", err),
505505
Reason: "GarbageCollectionFailed",
506506
}
507+
} else if deleted != "" {
508+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
509+
"garbage collected artifacts for deleted resource")
507510
}
508511
obj.Status.Artifact = nil
509-
// TODO(hidde): we should only push this event if we actually garbage collected something
510-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
511-
"garbage collected artifacts for deleted resource")
512512
return nil
513513
}
514514
if obj.GetArtifact() != nil {
515-
if err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
515+
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
516516
return &serror.Event{
517517
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
518518
Reason: "GarbageCollectionFailed",
519519
}
520+
} else if len(deleted) > 0 {
521+
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
522+
"garbage collected old artifacts")
520523
}
521-
// TODO(hidde): we should only push this event if we actually garbage collected something
522-
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
523-
"garbage collected old artifacts")
524524
}
525525
return nil
526526
}

‎controllers/storage.go

Lines changed: 15 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -105,13 +105,20 @@ func (s *Storage) MkdirAll(artifact sourcev1.Artifact) error {
105105
}
106106

107107
// RemoveAll calls os.RemoveAll for the given v1beta1.Artifact base dir.
108-
func (s *Storage) RemoveAll(artifact sourcev1.Artifact) error {
108+
func (s *Storage) RemoveAll(artifact sourcev1.Artifact) (string, error) {
109+
var deletedDir string
109110
dir := filepath.Dir(s.LocalPath(artifact))
110-
return os.RemoveAll(dir)
111+
// Check if the dir exists.
112+
_, err := os.Stat(dir)
113+
if err == nil {
114+
deletedDir = dir
115+
}
116+
return deletedDir, os.RemoveAll(dir)
111117
}
112118

113119
// RemoveAllButCurrent removes all files for the given v1beta1.Artifact base dir, excluding the current one.
114-
func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
120+
func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) ([]string, error) {
121+
deletedFiles := []string{}
115122
localPath := s.LocalPath(artifact)
116123
dir := filepath.Dir(localPath)
117124
var errors []string
@@ -124,15 +131,18 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) error {
124131
if path != localPath && !info.IsDir() && info.Mode()&os.ModeSymlink != os.ModeSymlink {
125132
if err := os.Remove(path); err != nil {
126133
errors = append(errors, info.Name())
134+
} else {
135+
// Collect the successfully deleted file paths.
136+
deletedFiles = append(deletedFiles, path)
127137
}
128138
}
129139
return nil
130140
})
131141

132142
if len(errors) > 0 {
133-
return fmt.Errorf("failed to remove files: %s", strings.Join(errors, " "))
143+
return deletedFiles, fmt.Errorf("failed to remove files: %s", strings.Join(errors, " "))
134144
}
135-
return nil
145+
return deletedFiles, nil
136146
}
137147

138148
// ArtifactExist returns a boolean indicating whether the v1beta1.Artifact exists in storage and is a regular file.

‎controllers/storage_test.go

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import (
2828
"time"
2929

3030
"github.com/go-git/go-git/v5/plumbing/format/gitignore"
31+
. "github.com/onsi/gomega"
3132

3233
sourcev1 "github.com/fluxcd/source-controller/api/v1beta2"
3334
)
@@ -293,10 +294,96 @@ func TestStorageRemoveAllButCurrent(t *testing.T) {
293294
t.Fatalf("Valid path did not successfully return: %v", err)
294295
}
295296

296-
if err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil {
297+
if _, err := s.RemoveAllButCurrent(sourcev1.Artifact{Path: path.Join(dir, "really", "nonexistent")}); err == nil {
297298
t.Fatal("Did not error while pruning non-existent path")
298299
}
299300
})
301+
302+
t.Run("collect names of deleted items", func(t *testing.T) {
303+
g := NewWithT(t)
304+
dir, err := os.MkdirTemp("", "")
305+
g.Expect(err).ToNot(HaveOccurred())
306+
t.Cleanup(func() { os.RemoveAll(dir) })
307+
308+
s, err := NewStorage(dir, "hostname", time.Minute)
309+
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
310+
311+
artifact := sourcev1.Artifact{
312+
Path: path.Join("foo", "bar", "artifact1.tar.gz"),
313+
}
314+
315+
// Create artifact dir and artifacts.
316+
artifactDir := path.Join(dir, "foo", "bar")
317+
g.Expect(os.MkdirAll(artifactDir, 0755)).NotTo(HaveOccurred())
318+
current := []string{
319+
path.Join(artifactDir, "artifact1.tar.gz"),
320+
}
321+
wantDeleted := []string{
322+
path.Join(artifactDir, "file1.txt"),
323+
path.Join(artifactDir, "file2.txt"),
324+
}
325+
createFile := func(files []string) {
326+
for _, c := range files {
327+
f, err := os.Create(c)
328+
g.Expect(err).ToNot(HaveOccurred())
329+
g.Expect(f.Close()).ToNot(HaveOccurred())
330+
}
331+
}
332+
createFile(current)
333+
createFile(wantDeleted)
334+
_, err = s.Symlink(artifact, "latest.tar.gz")
335+
g.Expect(err).ToNot(HaveOccurred(), "failed to create symlink")
336+
337+
deleted, err := s.RemoveAllButCurrent(artifact)
338+
g.Expect(err).ToNot(HaveOccurred(), "failed to remove all but current")
339+
g.Expect(deleted).To(Equal(wantDeleted))
340+
})
341+
}
342+
343+
func TestStorageRemoveAll(t *testing.T) {
344+
tests := []struct {
345+
name string
346+
artifactPath string
347+
createArtifactPath bool
348+
wantDeleted string
349+
}{
350+
{
351+
name: "delete non-existent path",
352+
artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"),
353+
createArtifactPath: false,
354+
wantDeleted: "",
355+
},
356+
{
357+
name: "delete existing path",
358+
artifactPath: path.Join("foo", "bar", "artifact1.tar.gz"),
359+
createArtifactPath: true,
360+
wantDeleted: path.Join("foo", "bar"),
361+
},
362+
}
363+
364+
for _, tt := range tests {
365+
t.Run(tt.name, func(t *testing.T) {
366+
g := NewWithT(t)
367+
dir, err := os.MkdirTemp("", "")
368+
g.Expect(err).ToNot(HaveOccurred())
369+
t.Cleanup(func() { os.RemoveAll(dir) })
370+
371+
s, err := NewStorage(dir, "hostname", time.Minute)
372+
g.Expect(err).ToNot(HaveOccurred(), "failed to create new storage")
373+
374+
artifact := sourcev1.Artifact{
375+
Path: tt.artifactPath,
376+
}
377+
378+
if tt.createArtifactPath {
379+
g.Expect(os.MkdirAll(path.Join(dir, tt.artifactPath), 0755)).ToNot(HaveOccurred())
380+
}
381+
382+
deleted, err := s.RemoveAll(artifact)
383+
g.Expect(err).ToNot(HaveOccurred())
384+
g.Expect(deleted).To(ContainSubstring(tt.wantDeleted), "unexpected deleted path")
385+
})
386+
}
300387
}
301388

302389
func TestStorageCopyFromPath(t *testing.T) {

0 commit comments

Comments
 (0)