Merge pull request #638 from aryan9600/garbage-retention

Garbage collect with provided retention options

controllers/bucket_controller.go

@@ -632,14 +632,19 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Buc
 		return nil
 	}
 	if obj.GetArtifact() != 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),
+		delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
+		if err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("garbage collection of artifacts failed: %w", err),
 				Reason: "GarbageCollectionFailed",
 			}
-		} else if len(deleted) > 0 {
+			r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
+			return e
+		}
+		if len(delFiles) > 0 {
 			r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
-				"garbage collected old artifacts")
+				fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
+			return nil
 		}
 	}
 	return nil

controllers/bucket_controller_test.go

@@ -176,7 +176,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 		{
 			name: "garbage collects",
 			beforeFunc: func(obj *sourcev1.Bucket, storage *Storage) error {
-				revisions := []string{"a", "b", "c"}
+				revisions := []string{"a", "b", "c", "d"}
 				for n := range revisions {
 					v := revisions[n]
 					obj.Status.Artifact = &sourcev1.Artifact{
@@ -186,26 +186,30 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 					if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
 						return err
 					}
-					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0644); err != nil {
+					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
 						return err
 					}
+					if n != len(revisions)-1 {
+						time.Sleep(time.Second * 1)
+					}
 				}
 				testStorage.SetArtifactURL(obj.Status.Artifact)
 				return nil
 			},
-			want: sreconcile.ResultSuccess,
 			assertArtifact: &sourcev1.Artifact{
-				Path:     "/reconcile-storage/c.txt",
-				Revision: "c",
-				Checksum: "2e7d2c03a9507ae265ecf5b5356885a53393a2029d241394997265a1a25aefc6",
-				URL:      testStorage.Hostname + "/reconcile-storage/c.txt",
-				Size:     int64p(int64(len("c"))),
+				Path:     "/reconcile-storage/d.txt",
+				Revision: "d",
+				Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
+				URL:      testStorage.Hostname + "/reconcile-storage/d.txt",
+				Size:     int64p(int64(len("d"))),
 			},
 			assertPaths: []string{
+				"/reconcile-storage/d.txt",
 				"/reconcile-storage/c.txt",
 				"!/reconcile-storage/b.txt",
 				"!/reconcile-storage/a.txt",
 			},
+			want: sreconcile.ResultSuccess,
 		},
 		{
 			name: "notices missing artifact in storage",
@@ -237,7 +241,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 				if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
 					return err
 				}
-				if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0644); err != nil {
+				if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
 					return err
 				}
 				return nil
@@ -259,6 +263,10 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
+			defer func() {
+				g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
+			}()
+
 			r := &BucketReconciler{
 				EventRecorder: record.NewFakeRecorder(32),
 				Storage:       testStorage,

controllers/gitrepository_controller.go

@@ -708,13 +708,19 @@ func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourc
 		return nil
 	}
 	if obj.GetArtifact() != 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),
+		delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
+		if err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("garbage collection of artifacts failed: %w", err),
+				Reason: "GarbageCollectionFailed",
 			}
-		} else if len(deleted) > 0 {
+			r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
+			return e
+		}
+		if len(delFiles) > 0 {
 			r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
-				"garbage collected old artifacts")
+				fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
+			return nil
 		}
 	}
 	return nil

controllers/gitrepository_controller_test.go

@@ -1104,6 +1104,148 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) {
 	}
 }
 
+func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
+	tests := []struct {
+		name             string
+		beforeFunc       func(obj *sourcev1.GitRepository, storage *Storage) error
+		want             sreconcile.Result
+		wantErr          bool
+		assertArtifact   *sourcev1.Artifact
+		assertConditions []metav1.Condition
+		assertPaths      []string
+	}{
+		{
+			name: "garbage collects",
+			beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
+				revisions := []string{"a", "b", "c", "d"}
+				for n := range revisions {
+					v := revisions[n]
+					obj.Status.Artifact = &sourcev1.Artifact{
+						Path:     fmt.Sprintf("/reconcile-storage/%s.txt", v),
+						Revision: v,
+					}
+					if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
+						return err
+					}
+					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
+						return err
+					}
+					if n != len(revisions)-1 {
+						time.Sleep(time.Second * 1)
+					}
+				}
+				testStorage.SetArtifactURL(obj.Status.Artifact)
+				return nil
+			},
+			assertArtifact: &sourcev1.Artifact{
+				Path:     "/reconcile-storage/d.txt",
+				Revision: "d",
+				Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
+				URL:      testStorage.Hostname + "/reconcile-storage/d.txt",
+				Size:     int64p(int64(len("d"))),
+			},
+			assertPaths: []string{
+				"/reconcile-storage/d.txt",
+				"/reconcile-storage/c.txt",
+				"!/reconcile-storage/b.txt",
+				"!/reconcile-storage/a.txt",
+			},
+			want: sreconcile.ResultSuccess,
+		},
+		{
+			name: "notices missing artifact in storage",
+			beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Path:     "/reconcile-storage/invalid.txt",
+					Revision: "e",
+				}
+				testStorage.SetArtifactURL(obj.Status.Artifact)
+				return nil
+			},
+			want: sreconcile.ResultSuccess,
+			assertPaths: []string{
+				"!/reconcile-storage/invalid.txt",
+			},
+			assertConditions: []metav1.Condition{
+				*conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"),
+			},
+		},
+		{
+			name: "updates hostname on diff from current",
+			beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
+				obj.Status.Artifact = &sourcev1.Artifact{
+					Path:     "/reconcile-storage/hostname.txt",
+					Revision: "f",
+					Checksum: "3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
+					URL:      "http://outdated.com/reconcile-storage/hostname.txt",
+				}
+				if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
+					return err
+				}
+				if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
+					return err
+				}
+				return nil
+			},
+			want: sreconcile.ResultSuccess,
+			assertPaths: []string{
+				"/reconcile-storage/hostname.txt",
+			},
+			assertArtifact: &sourcev1.Artifact{
+				Path:     "/reconcile-storage/hostname.txt",
+				Revision: "f",
+				Checksum: "3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
+				URL:      testStorage.Hostname + "/reconcile-storage/hostname.txt",
+				Size:     int64p(int64(len("file"))),
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			g := NewWithT(t)
+
+			defer func() {
+				g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
+			}()
+
+			r := &GitRepositoryReconciler{
+				EventRecorder: record.NewFakeRecorder(32),
+				Storage:       testStorage,
+			}
+
+			obj := &sourcev1.GitRepository{
+				ObjectMeta: metav1.ObjectMeta{
+					GenerateName: "test-",
+				},
+			}
+			if tt.beforeFunc != nil {
+				g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed())
+			}
+
+			var c *git.Commit
+			var as artifactSet
+			got, err := r.reconcileStorage(context.TODO(), obj, c, &as, "")
+			g.Expect(err != nil).To(Equal(tt.wantErr))
+			g.Expect(got).To(Equal(tt.want))
+
+			g.Expect(obj.Status.Artifact).To(MatchArtifact(tt.assertArtifact))
+			if tt.assertArtifact != nil && tt.assertArtifact.URL != "" {
+				g.Expect(obj.Status.Artifact.URL).To(Equal(tt.assertArtifact.URL))
+			}
+			g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
+
+			for _, p := range tt.assertPaths {
+				absoluteP := filepath.Join(testStorage.BasePath, p)
+				if !strings.HasPrefix(p, "!") {
+					g.Expect(absoluteP).To(BeAnExistingFile())
+					continue
+				}
+				g.Expect(absoluteP).NotTo(BeAnExistingFile())
+			}
+		})
+	}
+}
+
 func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) {
 	g := NewWithT(t)
 

controllers/helmchart_controller.go

@@ -285,6 +285,9 @@ func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmC
 // they match the Storage server hostname of current runtime.
 func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) {
 	// Garbage collect previous advertised artifact(s) from storage
+	// Abort if it takes more than 5 seconds.
+	ctx, cancel := context.WithTimeout(ctx, time.Second*5)
+	defer cancel()
 	_ = r.garbageCollect(ctx, obj)
 
 	// Determine if the advertised artifact is still in storage
@@ -801,14 +804,19 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
 		return nil
 	}
 	if obj.GetArtifact() != 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),
+		delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
+		if err != nil {
+			e := &serror.Event{
+				Err:    fmt.Errorf("garbage collection of artifacts failed: %w", err),
 				Reason: "GarbageCollectionFailed",
 			}
-		} else if len(deleted) > 0 {
+			r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
+			return e
+		}
+		if len(delFiles) > 0 {
 			r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
-				"garbage collected old artifacts")
+				fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
+			return nil
 		}
 	}
 	return nil

controllers/helmchart_controller_test.go

@@ -177,7 +177,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
 		{
 			name: "garbage collects",
 			beforeFunc: func(obj *sourcev1.HelmChart, storage *Storage) error {
-				revisions := []string{"a", "b", "c"}
+				revisions := []string{"a", "b", "c", "d"}
 				for n := range revisions {
 					v := revisions[n]
 					obj.Status.Artifact = &sourcev1.Artifact{
@@ -187,21 +187,25 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
 					if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
 						return err
 					}
-					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0644); err != nil {
+					if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
 						return err
 					}
+					if n != len(revisions)-1 {
+						time.Sleep(time.Second * 1)
+					}
 				}
 				testStorage.SetArtifactURL(obj.Status.Artifact)
 				return nil
 			},
 			assertArtifact: &sourcev1.Artifact{
-				Path:     "/reconcile-storage/c.txt",
-				Revision: "c",
-				Checksum: "2e7d2c03a9507ae265ecf5b5356885a53393a2029d241394997265a1a25aefc6",
-				URL:      testStorage.Hostname + "/reconcile-storage/c.txt",
-				Size:     int64p(int64(len("c"))),
+				Path:     "/reconcile-storage/d.txt",
+				Revision: "d",
+				Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
+				URL:      testStorage.Hostname + "/reconcile-storage/d.txt",
+				Size:     int64p(int64(len("d"))),
 			},
 			assertPaths: []string{
+				"/reconcile-storage/d.txt",
 				"/reconcile-storage/c.txt",
 				"!/reconcile-storage/b.txt",
 				"!/reconcile-storage/a.txt",
@@ -238,7 +242,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
 				if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
 					return err
 				}
-				if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0644); err != nil {
+				if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
 					return err
 				}
 				return nil
@@ -260,6 +264,10 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
 		t.Run(tt.name, func(t *testing.T) {
 			g := NewWithT(t)
 
+			defer func() {
+				g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
+			}()
+
 			r := &HelmChartReconciler{
 				EventRecorder: record.NewFakeRecorder(32),
 				Storage:       testStorage,
@@ -303,7 +311,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
 	g.Expect(err).ToNot(HaveOccurred())
 	defer os.RemoveAll(tmpDir)
 
-	storage, err := NewStorage(tmpDir, "example.com", timeout)
+	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
 	g.Expect(err).ToNot(HaveOccurred())
 
 	gitArtifact := &sourcev1.Artifact{
@@ -777,7 +785,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
 	g.Expect(err).ToNot(HaveOccurred())
 	defer os.RemoveAll(tmpDir)
 
-	storage, err := NewStorage(tmpDir, "example.com", timeout)
+	storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
 	g.Expect(err).ToNot(HaveOccurred())
 
 	chartsArtifact := &sourcev1.Artifact{