fix garbage collection logic and add tests

Signed-off-by: Sanskar Jaiswal <jaiswalsanskar078@gmail.com>

Author: aryan9600

controllers/gitrepository_controller_test.go

@@ -1102,6 +1102,154 @@ 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), 0644); 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"), 0644); 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() {
+				for _, p := range tt.assertPaths {
+					if !strings.HasPrefix(p, "!") {
+						g.Expect(os.Remove(filepath.Join(testStorage.BasePath, p))).To(Succeed())
+					}
+				}
+			}()
+
+			r := &GitRepositoryReconciler{
+				EventRecorder:            record.NewFakeRecorder(32),
+				Storage:                  testStorage,
+				artifactRetentionRecords: 2,
+				artifactRetentionTTL:     2 * time.Second,
+			}
+
+			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/storage.go

@@ -146,28 +146,41 @@ func (s *Storage) RemoveAllButCurrent(artifact sourcev1.Artifact) ([]string, err
 	return deletedFiles, nil
 }
 
-func (s *Storage) getGarbageFiles(artifact sourcev1.Artifact, n int, ttl time.Duration) []string {
+// getGarbageFiles returns all files that need to be garbage collected for the given artifact.
+// Garbage files are determined based on the below flow:
+// 1. collect all files with an expired ttl
+// 2. if we satisfy maxItemsToBeRetained, then return
+// 3. else, remove all files till the latest n files remain, where n=maxItemsToBeRetained
+func (s *Storage) getGarbageFiles(artifact sourcev1.Artifact, maxItemsToBeRetained int, ttl time.Duration) []string {
 	localPath := s.LocalPath(artifact)
 	dir := filepath.Dir(localPath)
 	garbageFiles := []string{}
 	filesWithCreatedTs := make(map[time.Time]string)
 	// sortedPaths contain all files sorted according to their created ts.
 	sortedPaths := []string{}
 	now := time.Now().UTC()
+	totalFiles := 0
 	_ = filepath.Walk(dir, func(path string, info os.FileInfo, err error) error {
 		createdAt := info.ModTime().UTC()
 		diff := now.Sub(createdAt)
 		// compare the time difference between now and the time at which the file was created
 		// with the provided ttl. delete if difference is greater than the ttl.
 		expired := diff > ttl
-		if path != localPath && !info.IsDir() && info.Mode()&os.ModeSymlink != os.ModeSymlink && expired {
-			garbageFiles = append(garbageFiles, path)
-		}
 		if !info.IsDir() && info.Mode()&os.ModeSymlink != os.ModeSymlink {
+			if path != localPath && expired {
+				garbageFiles = append(garbageFiles, path)
+			}
+			totalFiles += 1
 			filesWithCreatedTs[info.ModTime().UTC()] = path
 		}
 		return nil
 	})
+	// We already collected enough garbage files to satisfy the no of max
+	// items that are supposed to be retained, so exit early.
+	if totalFiles-len(garbageFiles) < maxItemsToBeRetained {
+		return garbageFiles
+	}
+
 	creationTimestamps := []time.Time{}
 	for ts := range filesWithCreatedTs {
 		creationTimestamps = append(creationTimestamps, ts)
@@ -182,11 +195,25 @@ func (s *Storage) getGarbageFiles(artifact sourcev1.Artifact, n int, ttl time.Du
 		sortedPaths = append(sortedPaths, path)
 	}
 
-	for i, path := range sortedPaths {
-		if path != localPath && !stringInSlice(path, garbageFiles) && len(sortedPaths) <= n {
-			// append path to garbageFiles and remove it from sortedPaths
-			garbageFiles = append(garbageFiles, path)
-			sortedPaths = append(sortedPaths[:i], sortedPaths[i+1:]...)
+	var collected int
+	noOfGarbageFiles := len(garbageFiles)
+	for _, path := range sortedPaths {
+		if path != localPath && !stringInSlice(path, garbageFiles) {
+			// If we previously collected a few garbage files with an expired ttl, then take that into account
+			// when checking whether we need to remove more files to satisfy the max no of items allowed
+			// in the filesystem, along with the no of files already removed in this loop.
+			if noOfGarbageFiles > 0 {
+				if (len(sortedPaths) - collected - len(garbageFiles)) > maxItemsToBeRetained {
+					// append path to garbageFiles and remove it from sortedPaths
+					garbageFiles = append(garbageFiles, path)
+					collected += 1
+				}
+			} else {
+				if len(sortedPaths)-collected > maxItemsToBeRetained {
+					garbageFiles = append(garbageFiles, path)
+					collected += 1
+				}
+			}
 		}
 	}
 
@@ -195,8 +222,8 @@ func (s *Storage) getGarbageFiles(artifact sourcev1.Artifact, n int, ttl time.Du
 
 // RemoveGarbageFiles removes all garabge files in the artifact dir according to the provided
 // retention options.
-func (s *Storage) RemoveGarbageFiles(artifact sourcev1.Artifact, n int, ttl time.Duration) ([]string, error) {
-	garbageFiles := s.getGarbageFiles(artifact, n, ttl)
+func (s *Storage) RemoveGarbageFiles(artifact sourcev1.Artifact, maxItemsToBeRetained int, ttl time.Duration) ([]string, error) {
+	garbageFiles := s.getGarbageFiles(artifact, maxItemsToBeRetained, ttl)
 	var errors []string
 	var deleted []string
 	if len(garbageFiles) > 0 {

controllers/storage_test.go

@@ -24,6 +24,7 @@ import (
 	"os"
 	"path"
 	"path/filepath"
+	"strings"
 	"testing"
 	"time"
 
@@ -486,3 +487,107 @@ func TestStorageCopyFromPath(t *testing.T) {
 		})
 	}
 }
+
+func TestStorage_getGarbageFiles(t *testing.T) {
+	artifactFolder := path.Join("foo", "bar")
+	tests := []struct {
+		name                 string
+		artifactPaths        []string
+		createPause          time.Duration
+		ttl                  time.Duration
+		maxItemsToBeRetained int
+		wantDeleted          []string
+	}{
+		{
+			name: "delete files based on maxItemsToBeRetained",
+			artifactPaths: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+				path.Join(artifactFolder, "artifact3.tar.gz"),
+				path.Join(artifactFolder, "artifact4.tar.gz"),
+				path.Join(artifactFolder, "artifact5.tar.gz"),
+			},
+			createPause:          time.Nanosecond,
+			ttl:                  time.Minute * 2,
+			maxItemsToBeRetained: 2,
+			wantDeleted: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+				path.Join(artifactFolder, "artifact3.tar.gz"),
+			},
+		},
+		{
+			name: "delete files based on ttl",
+			artifactPaths: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+				path.Join(artifactFolder, "artifact3.tar.gz"),
+				path.Join(artifactFolder, "artifact4.tar.gz"),
+				path.Join(artifactFolder, "artifact5.tar.gz"),
+			},
+			createPause:          time.Second * 1,
+			ttl:                  time.Second*3 + time.Millisecond*500,
+			maxItemsToBeRetained: 4,
+			wantDeleted: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+			},
+		},
+		{
+			name: "delete files based on ttl and maxItemsToBeRetained",
+			artifactPaths: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+				path.Join(artifactFolder, "artifact3.tar.gz"),
+				path.Join(artifactFolder, "artifact4.tar.gz"),
+				path.Join(artifactFolder, "artifact5.tar.gz"),
+				path.Join(artifactFolder, "artifact6.tar.gz"),
+			},
+			createPause:          time.Second * 1,
+			ttl:                  time.Second*5 + time.Millisecond*500,
+			maxItemsToBeRetained: 4,
+			wantDeleted: []string{
+				path.Join(artifactFolder, "artifact1.tar.gz"),
+				path.Join(artifactFolder, "artifact2.tar.gz"),
+			},
+		},
+	}
+
+	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.artifactPaths[len(tt.artifactPaths)-1],
+			}
+			g.Expect(os.MkdirAll(path.Join(dir, artifactFolder), 0755)).ToNot(HaveOccurred())
+			for _, artifactPath := range tt.artifactPaths {
+				f, err := os.Create(path.Join(dir, artifactPath))
+				g.Expect(err).ToNot(HaveOccurred())
+				g.Expect(f.Close()).ToNot(HaveOccurred())
+				time.Sleep(tt.createPause)
+			}
+
+			deletedPaths := s.getGarbageFiles(artifact, tt.maxItemsToBeRetained, tt.ttl)
+			g.Expect(len(tt.wantDeleted)).To(Equal(len(deletedPaths)))
+			for _, wantDeletedPath := range tt.wantDeleted {
+				present := false
+				for _, deletedPath := range deletedPaths {
+					if strings.Contains(deletedPath, wantDeletedPath) {
+						present = true
+						break
+					}
+				}
+				if present == false {
+					g.Fail(fmt.Sprintf("expected file to be deleted, still exists: %s", wantDeletedPath))
+				}
+			}
+		})
+	}
+}