From 93d1559b50e48452e30c7352802ee3010ac6e953 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 18 Nov 2020 22:56:08 +0100 Subject: [PATCH 1/2] Prevent GC from wiping artifacts in the same NS Signed-off-by: Hidde Beydals --- controllers/bucket_controller.go | 2 +- controllers/gitrepository_controller.go | 2 +- controllers/helmchart_controller.go | 2 +- controllers/helmrepository_controller.go | 2 +- 4 files changed, 4 insertions(+), 4 deletions(-) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index de9b31be..0717a1a2 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -352,7 +352,7 @@ func (r *BucketReconciler) resetStatus(bucket sourcev1.Bucket) (sourcev1.Bucket, // gc performs a garbage collection on all but current artifacts of the given bucket. func (r *BucketReconciler) gc(bucket sourcev1.Bucket, all bool) error { if all { - return r.Storage.RemoveAll(r.Storage.NewArtifactFor(bucket.Kind, bucket.GetObjectMeta(), "", "")) + return r.Storage.RemoveAll(r.Storage.NewArtifactFor(bucket.Kind, bucket.GetObjectMeta(), "", "*")) } if bucket.GetArtifact() != nil { return r.Storage.RemoveAllButCurrent(*bucket.GetArtifact()) diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 8be85e67..8607562d 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -312,7 +312,7 @@ func (r *GitRepositoryReconciler) resetStatus(repository sourcev1.GitRepository) // the given repository. func (r *GitRepositoryReconciler) gc(repository sourcev1.GitRepository, all bool) error { if all { - return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "")) + return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "*")) } if repository.GetArtifact() != nil { return r.Storage.RemoveAllButCurrent(*repository.GetArtifact()) diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 1a0245b7..9a361b18 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -628,7 +628,7 @@ func (r *HelmChartReconciler) resetStatus(chart sourcev1.HelmChart) (sourcev1.He // the given chart. func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart, all bool) error { if all { - return r.Storage.RemoveAll(r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), "", "")) + return r.Storage.RemoveAll(r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), "", "*")) } if chart.GetArtifact() != nil { return r.Storage.RemoveAllButCurrent(*chart.GetArtifact()) diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index 52d0a835..c6865678 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -286,7 +286,7 @@ func (r *HelmRepositoryReconciler) resetStatus(repository sourcev1.HelmRepositor // the given repository. func (r *HelmRepositoryReconciler) gc(repository sourcev1.HelmRepository, all bool) error { if all { - return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "")) + return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "*")) } if repository.GetArtifact() != nil { return r.Storage.RemoveAllButCurrent(*repository.GetArtifact()) From 2f50e3ff59306efc8d9fe74376967c31461216b9 Mon Sep 17 00:00:00 2001 From: Hidde Beydals Date: Wed, 18 Nov 2020 23:03:44 +0100 Subject: [PATCH 2/2] Make GC behavior depend on deletion timestamp Signed-off-by: Hidde Beydals --- controllers/bucket_controller.go | 13 ++++++++----- controllers/gitrepository_controller.go | 14 ++++++++------ controllers/helmchart_controller.go | 14 ++++++++------ controllers/helmrepository_controller.go | 14 ++++++++------ 4 files changed, 32 insertions(+), 23 deletions(-) diff --git a/controllers/bucket_controller.go b/controllers/bucket_controller.go index 0717a1a2..23d70dd8 100644 --- a/controllers/bucket_controller.go +++ b/controllers/bucket_controller.go @@ -108,7 +108,7 @@ func (r *BucketReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } // purge old artifacts from storage - if err := r.gc(bucket, false); err != nil { + if err := r.gc(bucket); err != nil { log.Error(err, "unable to purge old artifacts") } @@ -252,7 +252,7 @@ func (r *BucketReconciler) reconcile(ctx context.Context, bucket sourcev1.Bucket } func (r *BucketReconciler) reconcileDelete(ctx context.Context, bucket sourcev1.Bucket) (ctrl.Result, error) { - if err := r.gc(bucket, true); err != nil { + if err := r.gc(bucket); err != nil { r.event(bucket, events.EventSeverityError, fmt.Sprintf("garbage collection for deleted resource failed: %s", err.Error())) // Return the error so we retry the failed garbage collection return ctrl.Result{}, err @@ -349,9 +349,12 @@ func (r *BucketReconciler) resetStatus(bucket sourcev1.Bucket) (sourcev1.Bucket, return bucket, false } -// gc performs a garbage collection on all but current artifacts of the given bucket. -func (r *BucketReconciler) gc(bucket sourcev1.Bucket, all bool) error { - if all { +// gc performs a garbage collection for the given v1beta1.Bucket. +// It removes all but the current artifact except for when the +// deletion timestamp is set, which will result in the removal of +// all artifacts for the resource. +func (r *BucketReconciler) gc(bucket sourcev1.Bucket) error { + if !bucket.DeletionTimestamp.IsZero() { return r.Storage.RemoveAll(r.Storage.NewArtifactFor(bucket.Kind, bucket.GetObjectMeta(), "", "*")) } if bucket.GetArtifact() != nil { diff --git a/controllers/gitrepository_controller.go b/controllers/gitrepository_controller.go index 8607562d..6a5ac386 100644 --- a/controllers/gitrepository_controller.go +++ b/controllers/gitrepository_controller.go @@ -107,7 +107,7 @@ func (r *GitRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, erro } // purge old artifacts from storage - if err := r.gc(repository, false); err != nil { + if err := r.gc(repository); err != nil { log.Error(err, "unable to purge old artifacts") } @@ -250,7 +250,7 @@ func (r *GitRepositoryReconciler) reconcile(ctx context.Context, repository sour } func (r *GitRepositoryReconciler) reconcileDelete(ctx context.Context, repository sourcev1.GitRepository) (ctrl.Result, error) { - if err := r.gc(repository, true); err != nil { + if err := r.gc(repository); err != nil { r.event(repository, events.EventSeverityError, fmt.Sprintf("garbage collection for deleted resource failed: %s", err.Error())) // Return the error so we retry the failed garbage collection return ctrl.Result{}, err @@ -308,10 +308,12 @@ func (r *GitRepositoryReconciler) resetStatus(repository sourcev1.GitRepository) return repository, false } -// gc performs a garbage collection on all but current artifacts of -// the given repository. -func (r *GitRepositoryReconciler) gc(repository sourcev1.GitRepository, all bool) error { - if all { +// gc performs a garbage collection for the given v1beta1.GitRepository. +// It removes all but the current artifact except for when the +// deletion timestamp is set, which will result in the removal of +// all artifacts for the resource. +func (r *GitRepositoryReconciler) gc(repository sourcev1.GitRepository) error { + if !repository.DeletionTimestamp.IsZero() { return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "*")) } if repository.GetArtifact() != nil { diff --git a/controllers/helmchart_controller.go b/controllers/helmchart_controller.go index 9a361b18..97b4af80 100644 --- a/controllers/helmchart_controller.go +++ b/controllers/helmchart_controller.go @@ -114,7 +114,7 @@ func (r *HelmChartReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) { } // Purge all but current artifact from storage - if err := r.gc(chart, false); err != nil { + if err := r.gc(chart); err != nil { log.Error(err, "unable to purge old artifacts") } @@ -589,7 +589,7 @@ func (r *HelmChartReconciler) reconcileFromTarballArtifact(ctx context.Context, func (r *HelmChartReconciler) reconcileDelete(ctx context.Context, chart sourcev1.HelmChart) (ctrl.Result, error) { // Our finalizer is still present, so lets handle garbage collection - if err := r.gc(chart, true); err != nil { + if err := r.gc(chart); err != nil { r.event(chart, events.EventSeverityError, fmt.Sprintf("garbage collection for deleted resource failed: %s", err.Error())) // Return the error so we retry the failed garbage collection return ctrl.Result{}, err @@ -624,10 +624,12 @@ func (r *HelmChartReconciler) resetStatus(chart sourcev1.HelmChart) (sourcev1.He return chart, false } -// gc performs a garbage collection on all but the current artifact of -// the given chart. -func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart, all bool) error { - if all { +// gc performs a garbage collection for the given v1beta1.HelmChart. +// It removes all but the current artifact except for when the +// deletion timestamp is set, which will result in the removal of +// all artifacts for the resource. +func (r *HelmChartReconciler) gc(chart sourcev1.HelmChart) error { + if !chart.DeletionTimestamp.IsZero() { return r.Storage.RemoveAll(r.Storage.NewArtifactFor(chart.Kind, chart.GetObjectMeta(), "", "*")) } if chart.GetArtifact() != nil { diff --git a/controllers/helmrepository_controller.go b/controllers/helmrepository_controller.go index c6865678..dcbe49a7 100644 --- a/controllers/helmrepository_controller.go +++ b/controllers/helmrepository_controller.go @@ -108,7 +108,7 @@ func (r *HelmRepositoryReconciler) Reconcile(req ctrl.Request) (ctrl.Result, err } // purge old artifacts from storage - if err := r.gc(repository, false); err != nil { + if err := r.gc(repository); err != nil { log.Error(err, "unable to purge old artifacts") } @@ -248,7 +248,7 @@ func (r *HelmRepositoryReconciler) reconcile(ctx context.Context, repository sou func (r *HelmRepositoryReconciler) reconcileDelete(ctx context.Context, repository sourcev1.HelmRepository) (ctrl.Result, error) { // Our finalizer is still present, so lets handle garbage collection - if err := r.gc(repository, true); err != nil { + if err := r.gc(repository); err != nil { r.event(repository, events.EventSeverityError, fmt.Sprintf("garbage collection for deleted resource failed: %s", err.Error())) // Return the error so we retry the failed garbage collection return ctrl.Result{}, err @@ -282,10 +282,12 @@ func (r *HelmRepositoryReconciler) resetStatus(repository sourcev1.HelmRepositor return repository, false } -// gc performs a garbage collection on all but current artifacts of -// the given repository. -func (r *HelmRepositoryReconciler) gc(repository sourcev1.HelmRepository, all bool) error { - if all { +// gc performs a garbage collection for the given v1beta1.HelmRepository. +// It removes all but the current artifact except for when the +// deletion timestamp is set, which will result in the removal of +// all artifacts for the resource. +func (r *HelmRepositoryReconciler) gc(repository sourcev1.HelmRepository) error { + if !repository.DeletionTimestamp.IsZero() { return r.Storage.RemoveAll(r.Storage.NewArtifactFor(repository.Kind, repository.GetObjectMeta(), "", "*")) } if repository.GetArtifact() != nil {