From 7fa7b978cc5d760ee1ff87b03a4c6c60ff70ed57 Mon Sep 17 00:00:00 2001 From: Vlad Bologa Date: Fri, 2 Sep 2022 12:51:41 +0200 Subject: [PATCH 1/3] Add support for pause-reconcile annotation --- .../internal/conditions/conditions.go | 12 +++- pkg/reconciler/reconciler.go | 37 ++++++++++-- pkg/reconciler/reconciler_test.go | 60 +++++++++++++++++++ 3 files changed, 101 insertions(+), 8 deletions(-) diff --git a/pkg/reconciler/internal/conditions/conditions.go b/pkg/reconciler/internal/conditions/conditions.go index 86beb90..a357da8 100644 --- a/pkg/reconciler/internal/conditions/conditions.go +++ b/pkg/reconciler/internal/conditions/conditions.go @@ -29,10 +29,12 @@ const ( TypeDeployed = "Deployed" TypeReleaseFailed = "ReleaseFailed" TypeIrreconcilable = "Irreconcilable" + TypePaused = "Paused" - ReasonInstallSuccessful = status.ConditionReason("InstallSuccessful") - ReasonUpgradeSuccessful = status.ConditionReason("UpgradeSuccessful") - ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") + ReasonInstallSuccessful = status.ConditionReason("InstallSuccessful") + ReasonUpgradeSuccessful = status.ConditionReason("UpgradeSuccessful") + ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") + ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") ReasonErrorGettingClient = status.ConditionReason("ErrorGettingClient") ReasonErrorGettingValues = status.ConditionReason("ErrorGettingValues") @@ -60,6 +62,10 @@ func Irreconcilable(stat corev1.ConditionStatus, reason status.ConditionReason, return newCondition(TypeIrreconcilable, stat, reason, message) } +func Paused(stat corev1.ConditionStatus, reason status.ConditionReason, message interface{}) status.Condition { + return newCondition(TypePaused, stat, reason, message) +} + func newCondition(t status.ConditionType, s corev1.ConditionStatus, r status.ConditionReason, m interface{}) status.Condition { message := fmt.Sprintf("%s", m) return status.Condition{ diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index fb45f40..6789702 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -88,11 +88,12 @@ type Reconciler struct { stripManifestFromStatus bool - annotSetupOnce sync.Once - annotations map[string]struct{} - installAnnotations map[string]annotation.Install - upgradeAnnotations map[string]annotation.Upgrade - uninstallAnnotations map[string]annotation.Uninstall + annotSetupOnce sync.Once + annotations map[string]struct{} + installAnnotations map[string]annotation.Install + upgradeAnnotations map[string]annotation.Upgrade + uninstallAnnotations map[string]annotation.Uninstall + pauseReconcileAnnotation string } type watchDescription struct { @@ -449,6 +450,18 @@ func WithUninstallAnnotations(as ...annotation.Uninstall) Option { } } +// WithPauseReconcileAnnotation is an Option that configures and enables +// a PauseReconcile annotation. If the Custom Resource watched by this +// reconciler has the given annotation, and its value is set to `true`, +// then reconciliation for this CR will not be performed until this annotation +// is removed. +func WithPauseReconcileAnnotation(annotationName string) Option { + return func(r *Reconciler) error { + r.pauseReconcileAnnotation = annotationName + return nil + } +} + // WithPreHook is an Option that configures the reconciler to run the given // PreHook just before performing any actions (e.g. install, upgrade, uninstall, // or reconciliation). @@ -611,6 +624,20 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. } }() + if r.pauseReconcileAnnotation != "" { + if v, ok := obj.GetAnnotations()[r.pauseReconcileAnnotation]; ok { + if v == "true" { + log.Info(fmt.Sprintf("Resource has '%s' annotation set to 'true', reconcile stopped", r.pauseReconcileAnnotation)) + u.UpdateStatus( + updater.EnsureCondition(conditions.Paused(corev1.ConditionTrue, conditions.ReasonPauseReconcileAnnotationSet, ""))) + return ctrl.Result{}, nil + } + } + } + + u.UpdateStatus( + updater.EnsureCondition(conditions.Paused(corev1.ConditionFalse, "", ""))) + actionClient, err := r.actionClientGetter.ActionClientFor(obj) if err != nil { u.UpdateStatus( diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index e1f2a91..694632b 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -382,6 +382,13 @@ var _ = Describe("Reconciler", func() { })) }) }) + var _ = Describe("WithPauseReconcileAnnotation", func() { + It("should set the pauseReconcileAnnotation field to the annotation name", func() { + a := "my.domain/pause-reconcile" + Expect(WithPauseReconcileAnnotation(a)(r)).To(Succeed()) + Expect(r.pauseReconcileAnnotation).To(Equal(a)) + }) + }) var _ = Describe("WithPreHook", func() { It("should set a reconciler prehook", func() { called := false @@ -524,6 +531,7 @@ var _ = Describe("Reconciler", func() { WithInstallAnnotations(annotation.InstallDescription{}), WithUpgradeAnnotations(annotation.UpgradeDescription{}), WithUninstallAnnotations(annotation.UninstallDescription{}), + WithPauseReconcileAnnotation("my.domain/pause-reconcile"), WithOverrideValues(map[string]string{ "image.repository": "custom-nginx", }), @@ -538,6 +546,7 @@ var _ = Describe("Reconciler", func() { WithInstallAnnotations(annotation.InstallDescription{}), WithUpgradeAnnotations(annotation.UpgradeDescription{}), WithUninstallAnnotations(annotation.UninstallDescription{}), + WithPauseReconcileAnnotation("my.domain/pause-reconcile"), WithOverrideValues(map[string]string{ "image.repository": "custom-nginx", }), @@ -1316,6 +1325,57 @@ var _ = Describe("Reconciler", func() { verifyNoRelease(ctx, mgr.GetClient(), obj.GetNamespace(), obj.GetName(), currentRelease) }) + By("ensuring the finalizer is removed and the CR is deleted", func() { + err := mgr.GetAPIReader().Get(ctx, objKey, obj) + Expect(apierrors.IsNotFound(err)).To(BeTrue()) + }) + }) + }) + When("pause-reconcile annotation is present", func() { + It("pauses reconciliation", func() { + By("adding the pause-reconcile annotation to the CR", func() { + Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) + obj.SetAnnotations(map[string]string{ + "my.domain/pause-reconcile": "true"}) + Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) + }) + + By("deleting the CR", func() { + Expect(mgr.GetClient().Delete(ctx, obj)).To(Succeed()) + }) + + By("successfully reconciling a request", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(BeNil()) + }) + + By("getting the CR", func() { + Expect(mgr.GetAPIReader().Get(ctx, objKey, obj)).To(Succeed()) + }) + + By("verifying the CR status is Paused", func() { + objStat := &objStatus{} + Expect(runtime.DefaultUnstructuredConverter.FromUnstructured(obj.Object, objStat)).To(Succeed()) + Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypePaused)).To(BeTrue()) + }) + + By("removing the pause-reconcile annotation from the CR", func() { + Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) + obj.SetAnnotations(nil) + Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) + }) + + By("successfully reconciling a request", func() { + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).To(BeNil()) + }) + + By("verifying the release is uninstalled", func() { + verifyNoRelease(ctx, mgr.GetClient(), obj.GetNamespace(), obj.GetName(), currentRelease) + }) + By("ensuring the finalizer is removed and the CR is deleted", func() { err := mgr.GetAPIReader().Get(ctx, objKey, obj) Expect(apierrors.IsNotFound(err)).To(BeTrue()) From 2985fab4b0000128bf96940b6f9408d613385d81 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Thu, 15 Sep 2022 11:29:39 +0200 Subject: [PATCH 2/3] simple review feedback --- .../internal/conditions/conditions.go | 8 ++++---- pkg/reconciler/internal/updater/updater.go | 6 ++++++ pkg/reconciler/reconciler.go | 19 +++++++++++++++---- pkg/reconciler/reconciler_test.go | 5 ++--- 4 files changed, 27 insertions(+), 11 deletions(-) diff --git a/pkg/reconciler/internal/conditions/conditions.go b/pkg/reconciler/internal/conditions/conditions.go index a357da8..12ee6a4 100644 --- a/pkg/reconciler/internal/conditions/conditions.go +++ b/pkg/reconciler/internal/conditions/conditions.go @@ -31,10 +31,10 @@ const ( TypeIrreconcilable = "Irreconcilable" TypePaused = "Paused" - ReasonInstallSuccessful = status.ConditionReason("InstallSuccessful") - ReasonUpgradeSuccessful = status.ConditionReason("UpgradeSuccessful") - ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") - ReasonPauseReconcileAnnotationSet = status.ConditionReason("PauseReconcileAnnotationSet") + ReasonInstallSuccessful = status.ConditionReason("InstallSuccessful") + ReasonUpgradeSuccessful = status.ConditionReason("UpgradeSuccessful") + ReasonUninstallSuccessful = status.ConditionReason("UninstallSuccessful") + ReasonPauseReconcileAnnotationTrue = status.ConditionReason("PauseReconcileAnnotationTrue") ReasonErrorGettingClient = status.ConditionReason("ErrorGettingClient") ReasonErrorGettingValues = status.ConditionReason("ErrorGettingValues") diff --git a/pkg/reconciler/internal/updater/updater.go b/pkg/reconciler/internal/updater/updater.go index b1e09fd..6e0f46a 100644 --- a/pkg/reconciler/internal/updater/updater.go +++ b/pkg/reconciler/internal/updater/updater.go @@ -142,6 +142,12 @@ func EnsureConditionUnknown(t status.ConditionType) UpdateStatusFunc { } } +func EnsureConditionAbsent(t status.ConditionType) UpdateStatusFunc { + return func(status *helmAppStatus) bool { + return status.Conditions.RemoveCondition(t) + } +} + func EnsureDeployedRelease(rel *release.Release) UpdateStatusFunc { return func(status *helmAppStatus) bool { newRel := helmAppReleaseFor(rel) diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 6789702..f097639 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -450,7 +450,7 @@ func WithUninstallAnnotations(as ...annotation.Uninstall) Option { } } -// WithPauseReconcileAnnotation is an Option that configures and enables +// WithPauseReconcileAnnotation is an Option that sets // a PauseReconcile annotation. If the Custom Resource watched by this // reconciler has the given annotation, and its value is set to `true`, // then reconciliation for this CR will not be performed until this annotation @@ -627,16 +627,27 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. if r.pauseReconcileAnnotation != "" { if v, ok := obj.GetAnnotations()[r.pauseReconcileAnnotation]; ok { if v == "true" { - log.Info(fmt.Sprintf("Resource has '%s' annotation set to 'true', reconcile stopped", r.pauseReconcileAnnotation)) + log.Info(fmt.Sprintf("Resource has '%s' annotation set to 'true', reconcile paused.", r.pauseReconcileAnnotation)) u.UpdateStatus( - updater.EnsureCondition(conditions.Paused(corev1.ConditionTrue, conditions.ReasonPauseReconcileAnnotationSet, ""))) + updater.EnsureCondition(conditions.Paused(corev1.ConditionTrue, conditions.ReasonPauseReconcileAnnotationTrue, "")), + updater.EnsureConditionUnknown(conditions.TypeIrreconcilable), + updater.EnsureConditionUnknown(conditions.TypeDeployed), + updater.EnsureConditionUnknown(conditions.TypeInitialized), + updater.EnsureConditionUnknown(conditions.TypeReleaseFailed), + updater.EnsureDeployedRelease(nil), + ) return ctrl.Result{}, nil } } } u.UpdateStatus( - updater.EnsureCondition(conditions.Paused(corev1.ConditionFalse, "", ""))) + // TODO(ROX-12637): change to updater.EnsureCondition(conditions.Paused(corev1.ConditionFalse, "", ""))) + // once stackrox operator with pause support is released. + // At that time also add `Paused` to the list of conditions expected in stackrox operator e2e tests. + // Otherwise, the number of conditions in the `status.conditions` list will vary depending on the version + // of used operator, which is cumbersome due to https://github.com/kudobuilder/kuttl/issues/76 + updater.EnsureConditionAbsent(conditions.TypePaused)) actionClient, err := r.actionClientGetter.ActionClientFor(obj) if err != nil { diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 694632b..830797d 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -1335,8 +1335,7 @@ var _ = Describe("Reconciler", func() { It("pauses reconciliation", func() { By("adding the pause-reconcile annotation to the CR", func() { Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) - obj.SetAnnotations(map[string]string{ - "my.domain/pause-reconcile": "true"}) + obj.SetAnnotations(map[string]string{"my.domain/pause-reconcile": "true"}) Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) }) @@ -1344,7 +1343,7 @@ var _ = Describe("Reconciler", func() { Expect(mgr.GetClient().Delete(ctx, obj)).To(Succeed()) }) - By("successfully reconciling a request", func() { + By("successfully reconciling a request when paused", func() { res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) Expect(err).To(BeNil()) From d1c8fe0fabdbe9d9d0fbe193d134683446a0ce24 Mon Sep 17 00:00:00 2001 From: Marcin Owsiany Date: Mon, 19 Sep 2022 09:58:14 +0200 Subject: [PATCH 3/3] remaining review feedback --- pkg/reconciler/reconciler_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 830797d..5fc858c 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -1336,6 +1336,7 @@ var _ = Describe("Reconciler", func() { By("adding the pause-reconcile annotation to the CR", func() { Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) obj.SetAnnotations(map[string]string{"my.domain/pause-reconcile": "true"}) + obj.Object["spec"] = map[string]interface{}{"replicaCount": "666"} Expect(mgr.GetClient().Update(ctx, obj)).To(Succeed()) }) @@ -1359,6 +1360,13 @@ var _ = Describe("Reconciler", func() { Expect(objStat.Status.Conditions.IsTrueFor(conditions.TypePaused)).To(BeTrue()) }) + By("verifying the release has not changed", func() { + rel, err := ac.Get(obj.GetName()) + Expect(err).To(BeNil()) + Expect(rel).NotTo(BeNil()) + Expect(*rel).To(Equal(*currentRelease)) + }) + By("removing the pause-reconcile annotation from the CR", func() { Expect(mgr.GetClient().Get(ctx, objKey, obj)).To(Succeed()) obj.SetAnnotations(nil)