Skip to content

Commit f8c27a8

Browse files
aryan9600aryan9600
committed
Garbage collect with provided retention options.
Introduce two new flags to configure the ttl of an artifact and the max no. of files to retain for an artifact. Modify the gc process to consider the options and use timeouts to prevent the controller from hanging. This helps in situations when the SC has already garbage collected the current artifact but the advertised artifact url is still the same, which leads to the server returning a 404. Signed-off-by: Sanskar Jaiswal <[email protected]>
1 parent 62604a2 commit f8c27a8

13 files changed

+665
-92
lines changed

‎controllers/bucket_controller.go‎

Lines changed: 10 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -632,14 +632,19 @@ func (r *BucketReconciler) garbageCollect(ctx context.Context, obj *sourcev1.Buc
632632
return nil
633633
}
634634
if obj.GetArtifact() != nil {
635-
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
636-
return &serror.Event{
637-
Err: fmt.Errorf("garbage collection of old artifacts failed: %s", err),
635+
delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
636+
if err != nil {
637+
e := &serror.Event{
638+
Err: fmt.Errorf("garbage collection of artifacts failed: %w", err),
638639
Reason: "GarbageCollectionFailed",
639640
}
640-
} else if len(deleted) > 0 {
641+
r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
642+
return e
643+
}
644+
if len(delFiles) > 0 {
641645
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
642-
"garbage collected old artifacts")
646+
fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
647+
return nil
643648
}
644649
}
645650
return nil

‎controllers/bucket_controller_test.go‎

Lines changed: 17 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -176,7 +176,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
176176
{
177177
name: "garbage collects",
178178
beforeFunc: func(obj *sourcev1.Bucket, storage *Storage) error {
179-
revisions := []string{"a", "b", "c"}
179+
revisions := []string{"a", "b", "c", "d"}
180180
for n := range revisions {
181181
v := revisions[n]
182182
obj.Status.Artifact = &sourcev1.Artifact{
@@ -186,26 +186,30 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
186186
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
187187
return err
188188
}
189-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0644); err != nil {
189+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
190190
return err
191191
}
192+
if n != len(revisions)-1 {
193+
time.Sleep(time.Second * 1)
194+
}
192195
}
193196
testStorage.SetArtifactURL(obj.Status.Artifact)
194197
return nil
195198
},
196-
want: sreconcile.ResultSuccess,
197199
assertArtifact: &sourcev1.Artifact{
198-
Path: "/reconcile-storage/c.txt",
199-
Revision: "c",
200-
Checksum: "2e7d2c03a9507ae265ecf5b5356885a53393a2029d241394997265a1a25aefc6",
201-
URL: testStorage.Hostname + "/reconcile-storage/c.txt",
202-
Size: int64p(int64(len("c"))),
200+
Path: "/reconcile-storage/d.txt",
201+
Revision: "d",
202+
Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
203+
URL: testStorage.Hostname + "/reconcile-storage/d.txt",
204+
Size: int64p(int64(len("d"))),
203205
},
204206
assertPaths: []string{
207+
"/reconcile-storage/d.txt",
205208
"/reconcile-storage/c.txt",
206209
"!/reconcile-storage/b.txt",
207210
"!/reconcile-storage/a.txt",
208211
},
212+
want: sreconcile.ResultSuccess,
209213
},
210214
{
211215
name: "notices missing artifact in storage",
@@ -237,7 +241,7 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
237241
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
238242
return err
239243
}
240-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0644); err != nil {
244+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
241245
return err
242246
}
243247
return nil
@@ -259,6 +263,10 @@ func TestBucketReconciler_reconcileStorage(t *testing.T) {
259263
t.Run(tt.name, func(t *testing.T) {
260264
g := NewWithT(t)
261265

266+
defer func() {
267+
g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
268+
}()
269+
262270
r := &BucketReconciler{
263271
EventRecorder: record.NewFakeRecorder(32),
264272
Storage: testStorage,

‎controllers/gitrepository_controller.go‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -708,13 +708,19 @@ func (r *GitRepositoryReconciler) garbageCollect(ctx context.Context, obj *sourc
708708
return nil
709709
}
710710
if obj.GetArtifact() != nil {
711-
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
712-
return &serror.Event{
713-
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
711+
delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
712+
if err != nil {
713+
e := &serror.Event{
714+
Err: fmt.Errorf("garbage collection of artifacts failed: %w", err),
715+
Reason: "GarbageCollectionFailed",
714716
}
715-
} else if len(deleted) > 0 {
717+
r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
718+
return e
719+
}
720+
if len(delFiles) > 0 {
716721
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
717-
"garbage collected old artifacts")
722+
fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
723+
return nil
718724
}
719725
}
720726
return nil

‎controllers/gitrepository_controller_test.go‎

Lines changed: 142 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1104,6 +1104,148 @@ func TestGitRepositoryReconciler_reconcileInclude(t *testing.T) {
11041104
}
11051105
}
11061106

1107+
func TestGitRepositoryReconciler_reconcileStorage(t *testing.T) {
1108+
tests := []struct {
1109+
name string
1110+
beforeFunc func(obj *sourcev1.GitRepository, storage *Storage) error
1111+
want sreconcile.Result
1112+
wantErr bool
1113+
assertArtifact *sourcev1.Artifact
1114+
assertConditions []metav1.Condition
1115+
assertPaths []string
1116+
}{
1117+
{
1118+
name: "garbage collects",
1119+
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
1120+
revisions := []string{"a", "b", "c", "d"}
1121+
for n := range revisions {
1122+
v := revisions[n]
1123+
obj.Status.Artifact = &sourcev1.Artifact{
1124+
Path: fmt.Sprintf("/reconcile-storage/%s.txt", v),
1125+
Revision: v,
1126+
}
1127+
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
1128+
return err
1129+
}
1130+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
1131+
return err
1132+
}
1133+
if n != len(revisions)-1 {
1134+
time.Sleep(time.Second * 1)
1135+
}
1136+
}
1137+
testStorage.SetArtifactURL(obj.Status.Artifact)
1138+
return nil
1139+
},
1140+
assertArtifact: &sourcev1.Artifact{
1141+
Path: "/reconcile-storage/d.txt",
1142+
Revision: "d",
1143+
Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
1144+
URL: testStorage.Hostname + "/reconcile-storage/d.txt",
1145+
Size: int64p(int64(len("d"))),
1146+
},
1147+
assertPaths: []string{
1148+
"/reconcile-storage/d.txt",
1149+
"/reconcile-storage/c.txt",
1150+
"!/reconcile-storage/b.txt",
1151+
"!/reconcile-storage/a.txt",
1152+
},
1153+
want: sreconcile.ResultSuccess,
1154+
},
1155+
{
1156+
name: "notices missing artifact in storage",
1157+
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
1158+
obj.Status.Artifact = &sourcev1.Artifact{
1159+
Path: "/reconcile-storage/invalid.txt",
1160+
Revision: "e",
1161+
}
1162+
testStorage.SetArtifactURL(obj.Status.Artifact)
1163+
return nil
1164+
},
1165+
want: sreconcile.ResultSuccess,
1166+
assertPaths: []string{
1167+
"!/reconcile-storage/invalid.txt",
1168+
},
1169+
assertConditions: []metav1.Condition{
1170+
*conditions.TrueCondition(meta.ReconcilingCondition, "NoArtifact", "no artifact for resource in storage"),
1171+
},
1172+
},
1173+
{
1174+
name: "updates hostname on diff from current",
1175+
beforeFunc: func(obj *sourcev1.GitRepository, storage *Storage) error {
1176+
obj.Status.Artifact = &sourcev1.Artifact{
1177+
Path: "/reconcile-storage/hostname.txt",
1178+
Revision: "f",
1179+
Checksum: "3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
1180+
URL: "http://outdated.com/reconcile-storage/hostname.txt",
1181+
}
1182+
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
1183+
return err
1184+
}
1185+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
1186+
return err
1187+
}
1188+
return nil
1189+
},
1190+
want: sreconcile.ResultSuccess,
1191+
assertPaths: []string{
1192+
"/reconcile-storage/hostname.txt",
1193+
},
1194+
assertArtifact: &sourcev1.Artifact{
1195+
Path: "/reconcile-storage/hostname.txt",
1196+
Revision: "f",
1197+
Checksum: "3b9c358f36f0a31b6ad3e14f309c7cf198ac9246e8316f9ce543d5b19ac02b80",
1198+
URL: testStorage.Hostname + "/reconcile-storage/hostname.txt",
1199+
Size: int64p(int64(len("file"))),
1200+
},
1201+
},
1202+
}
1203+
for _, tt := range tests {
1204+
t.Run(tt.name, func(t *testing.T) {
1205+
g := NewWithT(t)
1206+
1207+
defer func() {
1208+
g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
1209+
}()
1210+
1211+
r := &GitRepositoryReconciler{
1212+
EventRecorder: record.NewFakeRecorder(32),
1213+
Storage: testStorage,
1214+
}
1215+
1216+
obj := &sourcev1.GitRepository{
1217+
ObjectMeta: metav1.ObjectMeta{
1218+
GenerateName: "test-",
1219+
},
1220+
}
1221+
if tt.beforeFunc != nil {
1222+
g.Expect(tt.beforeFunc(obj, testStorage)).To(Succeed())
1223+
}
1224+
1225+
var c *git.Commit
1226+
var as artifactSet
1227+
got, err := r.reconcileStorage(context.TODO(), obj, c, &as, "")
1228+
g.Expect(err != nil).To(Equal(tt.wantErr))
1229+
g.Expect(got).To(Equal(tt.want))
1230+
1231+
g.Expect(obj.Status.Artifact).To(MatchArtifact(tt.assertArtifact))
1232+
if tt.assertArtifact != nil && tt.assertArtifact.URL != "" {
1233+
g.Expect(obj.Status.Artifact.URL).To(Equal(tt.assertArtifact.URL))
1234+
}
1235+
g.Expect(obj.Status.Conditions).To(conditions.MatchConditions(tt.assertConditions))
1236+
1237+
for _, p := range tt.assertPaths {
1238+
absoluteP := filepath.Join(testStorage.BasePath, p)
1239+
if !strings.HasPrefix(p, "!") {
1240+
g.Expect(absoluteP).To(BeAnExistingFile())
1241+
continue
1242+
}
1243+
g.Expect(absoluteP).NotTo(BeAnExistingFile())
1244+
}
1245+
})
1246+
}
1247+
}
1248+
11071249
func TestGitRepositoryReconciler_reconcileDelete(t *testing.T) {
11081250
g := NewWithT(t)
11091251

‎controllers/helmchart_controller.go‎

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -285,6 +285,9 @@ func (r *HelmChartReconciler) reconcile(ctx context.Context, obj *sourcev1.HelmC
285285
// they match the Storage server hostname of current runtime.
286286
func (r *HelmChartReconciler) reconcileStorage(ctx context.Context, obj *sourcev1.HelmChart, build *chart.Build) (sreconcile.Result, error) {
287287
// Garbage collect previous advertised artifact(s) from storage
288+
// Abort if it takes more than 5 seconds.
289+
ctx, cancel := context.WithTimeout(ctx, time.Second*5)
290+
defer cancel()
288291
_ = r.garbageCollect(ctx, obj)
289292

290293
// Determine if the advertised artifact is still in storage
@@ -801,14 +804,19 @@ func (r *HelmChartReconciler) garbageCollect(ctx context.Context, obj *sourcev1.
801804
return nil
802805
}
803806
if obj.GetArtifact() != nil {
804-
if deleted, err := r.Storage.RemoveAllButCurrent(*obj.GetArtifact()); err != nil {
805-
return &serror.Event{
806-
Err: fmt.Errorf("garbage collection of old artifacts failed: %w", err),
807+
delFiles, err := r.Storage.GarbageCollect(ctx, *obj.GetArtifact(), time.Second*5)
808+
if err != nil {
809+
e := &serror.Event{
810+
Err: fmt.Errorf("garbage collection of artifacts failed: %w", err),
807811
Reason: "GarbageCollectionFailed",
808812
}
809-
} else if len(deleted) > 0 {
813+
r.eventLogf(ctx, obj, corev1.EventTypeWarning, e.Reason, e.Err.Error())
814+
return e
815+
}
816+
if len(delFiles) > 0 {
810817
r.eventLogf(ctx, obj, events.EventTypeTrace, "GarbageCollectionSucceeded",
811-
"garbage collected old artifacts")
818+
fmt.Sprintf("garbage collected %d artifacts", len(delFiles)))
819+
return nil
812820
}
813821
}
814822
return nil

‎controllers/helmchart_controller_test.go‎

Lines changed: 18 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -177,7 +177,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
177177
{
178178
name: "garbage collects",
179179
beforeFunc: func(obj *sourcev1.HelmChart, storage *Storage) error {
180-
revisions := []string{"a", "b", "c"}
180+
revisions := []string{"a", "b", "c", "d"}
181181
for n := range revisions {
182182
v := revisions[n]
183183
obj.Status.Artifact = &sourcev1.Artifact{
@@ -187,21 +187,25 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
187187
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
188188
return err
189189
}
190-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0644); err != nil {
190+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader(v), 0o644); err != nil {
191191
return err
192192
}
193+
if n != len(revisions)-1 {
194+
time.Sleep(time.Second * 1)
195+
}
193196
}
194197
testStorage.SetArtifactURL(obj.Status.Artifact)
195198
return nil
196199
},
197200
assertArtifact: &sourcev1.Artifact{
198-
Path: "/reconcile-storage/c.txt",
199-
Revision: "c",
200-
Checksum: "2e7d2c03a9507ae265ecf5b5356885a53393a2029d241394997265a1a25aefc6",
201-
URL: testStorage.Hostname + "/reconcile-storage/c.txt",
202-
Size: int64p(int64(len("c"))),
201+
Path: "/reconcile-storage/d.txt",
202+
Revision: "d",
203+
Checksum: "18ac3e7343f016890c510e93f935261169d9e3f565436429830faf0934f4f8e4",
204+
URL: testStorage.Hostname + "/reconcile-storage/d.txt",
205+
Size: int64p(int64(len("d"))),
203206
},
204207
assertPaths: []string{
208+
"/reconcile-storage/d.txt",
205209
"/reconcile-storage/c.txt",
206210
"!/reconcile-storage/b.txt",
207211
"!/reconcile-storage/a.txt",
@@ -238,7 +242,7 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
238242
if err := testStorage.MkdirAll(*obj.Status.Artifact); err != nil {
239243
return err
240244
}
241-
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0644); err != nil {
245+
if err := testStorage.AtomicWriteFile(obj.Status.Artifact, strings.NewReader("file"), 0o644); err != nil {
242246
return err
243247
}
244248
return nil
@@ -260,6 +264,10 @@ func TestHelmChartReconciler_reconcileStorage(t *testing.T) {
260264
t.Run(tt.name, func(t *testing.T) {
261265
g := NewWithT(t)
262266

267+
defer func() {
268+
g.Expect(os.RemoveAll(filepath.Join(testStorage.BasePath, "/reconcile-storage"))).To(Succeed())
269+
}()
270+
263271
r := &HelmChartReconciler{
264272
EventRecorder: record.NewFakeRecorder(32),
265273
Storage: testStorage,
@@ -303,7 +311,7 @@ func TestHelmChartReconciler_reconcileSource(t *testing.T) {
303311
g.Expect(err).ToNot(HaveOccurred())
304312
defer os.RemoveAll(tmpDir)
305313

306-
storage, err := NewStorage(tmpDir, "example.com", timeout)
314+
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
307315
g.Expect(err).ToNot(HaveOccurred())
308316

309317
gitArtifact := &sourcev1.Artifact{
@@ -777,7 +785,7 @@ func TestHelmChartReconciler_buildFromTarballArtifact(t *testing.T) {
777785
g.Expect(err).ToNot(HaveOccurred())
778786
defer os.RemoveAll(tmpDir)
779787

780-
storage, err := NewStorage(tmpDir, "example.com", timeout)
788+
storage, err := NewStorage(tmpDir, "example.com", retentionTTL, retentionRecords)
781789
g.Expect(err).ToNot(HaveOccurred())
782790

783791
chartsArtifact := &sourcev1.Artifact{

0 commit comments

Comments
 (0)