From 2961ecb784d80af77471f6454e00c746ed483639 Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Fri, 17 Dec 2021 18:08:14 +0100 Subject: [PATCH 1/6] Allow marking releases stuck in a pending state as failed --- pkg/client/actionclient.go | 9 +++ .../internal/conditions/conditions.go | 1 + pkg/reconciler/internal/fake/actionclient.go | 36 ++++++---- pkg/reconciler/reconciler.go | 51 ++++++++++++++ pkg/reconciler/reconciler_test.go | 66 +++++++++++++++++++ 5 files changed, 152 insertions(+), 11 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 8f3fc80a..95811719 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -62,6 +62,7 @@ type ActionInterface interface { Get(name string, opts ...GetOption) (*release.Release, error) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...InstallOption) (*release.Release, error) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...UpgradeOption) (*release.Release, error) + MarkFailed(release *release.Release, reason string) error Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) Reconcile(rel *release.Release) error } @@ -180,6 +181,14 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return rel, nil } +func (c *actionClient) MarkFailed(rel *release.Release, reason string) error { + infoCopy := *rel.Info + releaseCopy := *rel + releaseCopy.Info = &infoCopy + releaseCopy.SetStatus(release.StatusFailed, reason) + return c.conf.Releases.Update(&releaseCopy) +} + func (c *actionClient) Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) { uninstall := action.NewUninstall(c.conf) for _, o := range opts { diff --git a/pkg/reconciler/internal/conditions/conditions.go b/pkg/reconciler/internal/conditions/conditions.go index 55e2c656..86beb90b 100644 --- a/pkg/reconciler/internal/conditions/conditions.go +++ b/pkg/reconciler/internal/conditions/conditions.go @@ -41,6 +41,7 @@ const ( ReasonUpgradeError = status.ConditionReason("UpgradeError") ReasonReconcileError = status.ConditionReason("ReconcileError") ReasonUninstallError = status.ConditionReason("UninstallError") + ReasonPendingError = status.ConditionReason("PendingError") ) func Initialized(stat corev1.ConditionStatus, reason status.ConditionReason, message interface{}) status.Condition { diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index 92b7da63..cdf5f77f 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -48,17 +48,19 @@ func (hcg *fakeActionClientGetter) ActionClientFor(_ crclient.Object) (client.Ac } type ActionClient struct { - Gets []GetCall - Installs []InstallCall - Upgrades []UpgradeCall - Uninstalls []UninstallCall - Reconciles []ReconcileCall - - HandleGet func() (*release.Release, error) - HandleInstall func() (*release.Release, error) - HandleUpgrade func() (*release.Release, error) - HandleUninstall func() (*release.UninstallReleaseResponse, error) - HandleReconcile func() error + Gets []GetCall + Installs []InstallCall + Upgrades []UpgradeCall + MarkFaileds []MarkFailedCall + Uninstalls []UninstallCall + Reconciles []ReconcileCall + + HandleGet func() (*release.Release, error) + HandleInstall func() (*release.Release, error) + HandleUpgrade func() (*release.Release, error) + HandleMarkFailed func() error + HandleUninstall func() (*release.UninstallReleaseResponse, error) + HandleReconcile func() error } func NewActionClient() ActionClient { @@ -77,12 +79,14 @@ func NewActionClient() ActionClient { Upgrades: make([]UpgradeCall, 0), Uninstalls: make([]UninstallCall, 0), Reconciles: make([]ReconcileCall, 0), + MarkFaileds: make([]MarkFailedCall, 0), HandleGet: relFunc(errors.New("get not implemented")), HandleInstall: relFunc(errors.New("install not implemented")), HandleUpgrade: relFunc(errors.New("upgrade not implemented")), HandleUninstall: uninstFunc(errors.New("uninstall not implemented")), HandleReconcile: recFunc(errors.New("reconcile not implemented")), + HandleMarkFailed: recFunc(errors.New("mark failed not implemented")), } } @@ -109,6 +113,11 @@ type UpgradeCall struct { Opts []client.UpgradeOption } +type MarkFailedCall struct { + Release *release.Release + Reason string +} + type UninstallCall struct { Name string Opts []client.UninstallOption @@ -133,6 +142,11 @@ func (c *ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return c.HandleUpgrade() } +func (c *ActionClient) MarkFailed(rel *release.Release, reason string) error { + c.MarkFaileds = append(c.MarkFaileds, MarkFailedCall{rel, reason}) + return c.HandleMarkFailed() +} + func (c *ActionClient) Uninstall(name string, opts ...client.UninstallOption) (*release.UninstallReleaseResponse, error) { c.Uninstalls = append(c.Uninstalls, UninstallCall{name, opts}) return c.HandleUninstall() diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 8b1f7df9..55fbf542 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -77,6 +77,7 @@ type Reconciler struct { skipDependentWatches bool maxConcurrentReconciles int reconcilePeriod time.Duration + markFailedAfter time.Duration maxHistory int annotSetupOnce sync.Once @@ -297,6 +298,18 @@ func WithMaxReleaseHistory(maxHistory int) Option { } } +// WithMarkFailedAfter specifies the duration after which the reconciler will mark a release in a pending (locked) +// state as false in order to allow rolling forward. +func WithMarkFailedAfter(duration time.Duration) Option { + return func(r *Reconciler) error { + if duration < 0 { + return errors.New("auto-rollback after duration must not be negative") + } + r.markFailedAfter = duration + return nil + } +} + // WithInstallAnnotations is an Option that configures Install annotations // to enable custom action.Install fields to be set based on the value of // annotations found in the custom resource watched by this reconciler. @@ -531,6 +544,10 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. ) return ctrl.Result{}, err } + if state == statePending { + return r.handlePending(actionClient, rel, &u, log) + } + u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) for _, h := range r.preHooks { @@ -597,6 +614,7 @@ const ( stateNeedsInstall helmReleaseState = "needs install" stateNeedsUpgrade helmReleaseState = "needs upgrade" stateUnchanged helmReleaseState = "unchanged" + statePending helmReleaseState = "pending" stateError helmReleaseState = "error" ) @@ -645,6 +663,10 @@ func (r *Reconciler) getReleaseState(client helmclient.ActionInterface, obj meta return nil, stateNeedsInstall, nil } + if currentRelease.Info != nil && currentRelease.Info.Status.IsPending() { + return currentRelease, statePending, nil + } + var opts []helmclient.UpgradeOption if r.maxHistory > 0 { opts = append(opts, func(u *action.Upgrade) error { @@ -722,6 +744,35 @@ func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updat return rel, nil } +func (r *Reconciler) handlePending(actionClient helmclient.ActionInterface, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { + err := r.doHandlePending(actionClient, rel, log) + if err == nil { + err = errors.New("unknown error handling pending release") + } + u.UpdateStatus( + updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionTrue, conditions.ReasonPendingError, err))) + return ctrl.Result{}, err +} + +func (r *Reconciler) doHandlePending(actionClient helmclient.ActionInterface, rel *release.Release, log logr.Logger) error { + if r.markFailedAfter <= 0 { + return errors.New("Release is in a pending (locked) state and cannot be modified. User intervention is required.") + } + if rel.Info == nil || rel.Info.LastDeployed.IsZero() { + return errors.New("Release is in a pending (locked) state and lacks 'last deployed' timestamp. User intervention is required.") + } + if pendingSince := time.Since(rel.Info.LastDeployed.Time); pendingSince < r.markFailedAfter { + return fmt.Errorf("Release is in a pending (locked) state and cannot currently be modified. Release will be marked failed to allow a roll-forward in %v.", r.markFailedAfter-pendingSince) + } + + log.Info("Marking release as failed", "releaseName", rel.Name) + err := actionClient.MarkFailed(rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) + if err != nil { + return fmt.Errorf("Failed to mark pending (locked) release as failed: %w", err) + } + return fmt.Errorf("marked release %s as failed to allow upgrade to succeed in next reconcile attempt", rel.Name) +} + func (r *Reconciler) reportOverrideEvents(obj runtime.Object) { for k, v := range r.overrideValues { r.eventRecorder.Eventf(obj, "Warning", "ValueOverridden", diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 3478a481..a2709e91 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -58,6 +58,7 @@ import ( helmfake "github.com/operator-framework/helm-operator-plugins/pkg/reconciler/internal/fake" "github.com/operator-framework/helm-operator-plugins/pkg/sdk/controllerutil" "github.com/operator-framework/helm-operator-plugins/pkg/values" + helmTime "helm.sh/helm/v3/pkg/time" ) var _ = Describe("Reconciler", func() { @@ -382,6 +383,12 @@ var _ = Describe("Reconciler", func() { Expect(r.valueTranslator.Translate(context.Background(), &unstructured.Unstructured{})).To(Equal(chartutil.Values{"translated": true})) }) }) + var _ = Describe("WithMarkedFailAfter", func() { + It("should set the reconciler mark failed after duration", func() { + Expect(WithMarkFailedAfter(1*time.Minute)(r)).To(Succeed()) + Expect(r.markFailedAfter).To(Equal(1*time.Minute)) + }) + }) }) var _ = Describe("Reconcile", func() { @@ -474,6 +481,65 @@ var _ = Describe("Reconciler", func() { Expect(mgr.GetClient().Create(ctx, obj)).To(Succeed()) }) + When("release is in pending state", func() { + fakeClient := helmfake.NewActionClient() + deployTime := helmTime.Now().Add(-5 * time.Minute) + exampleRelease := &release.Release{ + Name: "example-release", + Info: &release.Info{ + Status: release.StatusPendingUpgrade, + LastDeployed: deployTime, + FirstDeployed: deployTime, + }, + } + + BeforeEach(func() { + r.actionClientGetter = helmclient.ActionClientGetterFunc(func(object client.Object) (helmclient.ActionInterface, error) { + fakeClient.HandleGet = func() (*release.Release, error) { + return exampleRelease, nil + } + fakeClient.HandleMarkFailed = func() error { + return nil + } + return &fakeClient, nil + }) + }) + AfterEach(func() { + r.actionClientGetter = nil + }) + + When("time elapsed since last deployment exceeds markFailedAfter duration", func() { + It("should be marked as failed", func() { + r.markFailedAfter = 1 * time.Minute + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).ToNot(BeNil()) + Expect(err).To(MatchError("marked release example-release as failed to allow upgrade to succeed in next reconcile attempt")) + Expect(len(fakeClient.MarkFaileds)).Should(Equal(1)) + }) + }) + + When("markFailedAfter is disabled", func() { + It("should require user intervention", func() { + r.markFailedAfter = 0 + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).ToNot(BeNil()) + Expect(err).To(MatchError("Release is in a pending (locked) state and cannot be modified. User intervention is required.")) + }) + }) + + When("time since last deployment is higher than markFiledAfter duration", func () { + It("should return duration until the release will be marked as failed", func () { + r.markFailedAfter = 10*time.Minute + res, err := r.Reconcile(ctx, req) + Expect(res).To(Equal(reconcile.Result{})) + Expect(err).ToNot(BeNil()) + Expect(err.Error()).Should(ContainSubstring("Release is in a pending (locked) state and cannot currently be modified. Release will be marked failed to allow a roll-forward in")) + }) + }) + }) + When("requested CR release is not present", func() { When("action client getter is not working", func() { It("returns an error getting the action client", func() { From 3deb69bcaae14ec2bd6a4c03e1953959ef8340f5 Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Fri, 17 Dec 2021 18:12:49 +0100 Subject: [PATCH 2/6] Fix style --- pkg/reconciler/reconciler_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index a2709e91..03a22910 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -385,8 +385,8 @@ var _ = Describe("Reconciler", func() { }) var _ = Describe("WithMarkedFailAfter", func() { It("should set the reconciler mark failed after duration", func() { - Expect(WithMarkFailedAfter(1*time.Minute)(r)).To(Succeed()) - Expect(r.markFailedAfter).To(Equal(1*time.Minute)) + Expect(WithMarkFailedAfter(1 * time.Minute)(r)).To(Succeed()) + Expect(r.markFailedAfter).To(Equal(1 * time.Minute)) }) }) }) @@ -531,7 +531,7 @@ var _ = Describe("Reconciler", func() { When("time since last deployment is higher than markFiledAfter duration", func () { It("should return duration until the release will be marked as failed", func () { - r.markFailedAfter = 10*time.Minute + r.markFailedAfter = 10 * time.Minute res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) Expect(err).ToNot(BeNil()) From aa12c68044dbb108eb3381b0311ede5337e604ec Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Fri, 17 Dec 2021 18:13:24 +0100 Subject: [PATCH 3/6] fix style --- pkg/reconciler/internal/fake/actionclient.go | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index cdf5f77f..4e682c0b 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -74,18 +74,18 @@ func NewActionClient() ActionClient { return func() error { return err } } return ActionClient{ - Gets: make([]GetCall, 0), - Installs: make([]InstallCall, 0), - Upgrades: make([]UpgradeCall, 0), - Uninstalls: make([]UninstallCall, 0), - Reconciles: make([]ReconcileCall, 0), + Gets: make([]GetCall, 0), + Installs: make([]InstallCall, 0), + Upgrades: make([]UpgradeCall, 0), + Uninstalls: make([]UninstallCall, 0), + Reconciles: make([]ReconcileCall, 0), MarkFaileds: make([]MarkFailedCall, 0), - HandleGet: relFunc(errors.New("get not implemented")), - HandleInstall: relFunc(errors.New("install not implemented")), - HandleUpgrade: relFunc(errors.New("upgrade not implemented")), - HandleUninstall: uninstFunc(errors.New("uninstall not implemented")), - HandleReconcile: recFunc(errors.New("reconcile not implemented")), + HandleGet: relFunc(errors.New("get not implemented")), + HandleInstall: relFunc(errors.New("install not implemented")), + HandleUpgrade: relFunc(errors.New("upgrade not implemented")), + HandleUninstall: uninstFunc(errors.New("uninstall not implemented")), + HandleReconcile: recFunc(errors.New("reconcile not implemented")), HandleMarkFailed: recFunc(errors.New("mark failed not implemented")), } } From a98ee77e037577b352b0fe32a1f1fc6d04b42e50 Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Fri, 17 Dec 2021 18:17:55 +0100 Subject: [PATCH 4/6] fix style --- pkg/reconciler/reconciler_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 03a22910..d349d68a 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -529,8 +529,8 @@ var _ = Describe("Reconciler", func() { }) }) - When("time since last deployment is higher than markFiledAfter duration", func () { - It("should return duration until the release will be marked as failed", func () { + When("time since last deployment is higher than markFiledAfter duration", func() { + It("should return duration until the release will be marked as failed", func() { r.markFailedAfter = 10 * time.Minute res, err := r.Reconcile(ctx, req) Expect(res).To(Equal(reconcile.Result{})) From 44e9cd15708a19fa8395ce9b15000ab0d2a54a17 Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Mon, 20 Dec 2021 14:52:35 +0100 Subject: [PATCH 5/6] WIP --- pkg/client/actionclient.go | 9 ---- pkg/reconciler/internal/fake/actionclient.go | 5 -- pkg/reconciler/reconciler.go | 52 ++++++++++++++------ pkg/reconciler/reconciler_test.go | 6 +-- 4 files changed, 40 insertions(+), 32 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 95811719..8f3fc80a 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -62,7 +62,6 @@ type ActionInterface interface { Get(name string, opts ...GetOption) (*release.Release, error) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...InstallOption) (*release.Release, error) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...UpgradeOption) (*release.Release, error) - MarkFailed(release *release.Release, reason string) error Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) Reconcile(rel *release.Release) error } @@ -181,14 +180,6 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return rel, nil } -func (c *actionClient) MarkFailed(rel *release.Release, reason string) error { - infoCopy := *rel.Info - releaseCopy := *rel - releaseCopy.Info = &infoCopy - releaseCopy.SetStatus(release.StatusFailed, reason) - return c.conf.Releases.Update(&releaseCopy) -} - func (c *actionClient) Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) { uninstall := action.NewUninstall(c.conf) for _, o := range opts { diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index 4e682c0b..d5f44ff4 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -142,11 +142,6 @@ func (c *ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return c.HandleUpgrade() } -func (c *ActionClient) MarkFailed(rel *release.Release, reason string) error { - c.MarkFaileds = append(c.MarkFaileds, MarkFailedCall{rel, reason}) - return c.HandleMarkFailed() -} - func (c *ActionClient) Uninstall(name string, opts ...client.UninstallOption) (*release.UninstallReleaseResponse, error) { c.Uninstalls = append(c.Uninstalls, UninstallCall{name, opts}) return c.HandleUninstall() diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 55fbf542..2f545405 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -61,13 +61,14 @@ const uninstallFinalizer = "uninstall-helm-release" // Reconciler reconciles a Helm object type Reconciler struct { - client client.Client - actionClientGetter helmclient.ActionClientGetter - valueTranslator values.Translator - valueMapper values.Mapper // nolint:staticcheck - eventRecorder record.EventRecorder - preHooks []hook.PreHook - postHooks []hook.PostHook + client client.Client + actionClientGetter helmclient.ActionClientGetter + actionConfigGetter helmclient.ActionConfigGetter + valueTranslator values.Translator + valueMapper values.Mapper // nolint:staticcheck + eventRecorder record.EventRecorder + preHooks []hook.PreHook + postHooks []hook.PostHook log logr.Logger gvk *schema.GroupVersionKind @@ -176,6 +177,16 @@ func WithActionClientGetter(actionClientGetter helmclient.ActionClientGetter) Op } } +// WithActionConfigGetter is an Option that configures a Reconciler's ActionConfigGetter +// +// A default ActionConfigGetter is used if this option is not configured. +func WithActionConfigGetter(actionConfigGetter helmclient.ActionConfigGetter) Option { + return func(r *Reconciler) error { + r.actionConfigGetter = actionConfigGetter + return nil + } +} + // WithEventRecorder is an Option that configures a Reconciler's EventRecorder. // // By default, manager.GetEventRecorderFor() is used if this option is not @@ -545,7 +556,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, err } if state == statePending { - return r.handlePending(actionClient, rel, &u, log) + return r.handlePending(obj, rel, &u, log) } u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) @@ -744,8 +755,8 @@ func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updat return rel, nil } -func (r *Reconciler) handlePending(actionClient helmclient.ActionInterface, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { - err := r.doHandlePending(actionClient, rel, log) +func (r *Reconciler) handlePending(obj *unstructured.Unstructured, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { + err := r.doHandlePending(obj, rel, log) if err == nil { err = errors.New("unknown error handling pending release") } @@ -754,7 +765,7 @@ func (r *Reconciler) handlePending(actionClient helmclient.ActionInterface, rel return ctrl.Result{}, err } -func (r *Reconciler) doHandlePending(actionClient helmclient.ActionInterface, rel *release.Release, log logr.Logger) error { +func (r *Reconciler) doHandlePending(obj *unstructured.Unstructured, rel *release.Release, log logr.Logger) error { if r.markFailedAfter <= 0 { return errors.New("Release is in a pending (locked) state and cannot be modified. User intervention is required.") } @@ -766,13 +777,26 @@ func (r *Reconciler) doHandlePending(actionClient helmclient.ActionInterface, re } log.Info("Marking release as failed", "releaseName", rel.Name) - err := actionClient.MarkFailed(rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) + err := r.markReleaseFailed(obj, rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) if err != nil { return fmt.Errorf("Failed to mark pending (locked) release as failed: %w", err) } return fmt.Errorf("marked release %s as failed to allow upgrade to succeed in next reconcile attempt", rel.Name) } +func (r *Reconciler) markReleaseFailed(obj *unstructured.Unstructured, rel *release.Release, reason string) error { + infoCopy := *rel.Info + releaseCopy := *rel + releaseCopy.Info = &infoCopy + releaseCopy.SetStatus(release.StatusFailed, reason) + + actionConfig, err := r.actionConfigGetter.ActionConfigFor(obj) + if err != nil { + return err + } + return actionConfig.Releases.Update(&releaseCopy) +} + func (r *Reconciler) reportOverrideEvents(obj runtime.Object) { for k, v := range r.overrideValues { r.eventRecorder.Eventf(obj, "Warning", "ValueOverridden", @@ -847,8 +871,8 @@ func (r *Reconciler) addDefaults(mgr ctrl.Manager, controllerName string) { r.log = ctrl.Log.WithName("controllers").WithName("Helm") } if r.actionClientGetter == nil { - actionConfigGetter := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), r.log) - r.actionClientGetter = helmclient.NewActionClientGetter(actionConfigGetter) + r.actionConfigGetter = helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), r.log) + r.actionClientGetter = helmclient.NewActionClientGetter(r.actionConfigGetter) } if r.eventRecorder == nil { r.eventRecorder = mgr.GetEventRecorderFor(controllerName) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index d349d68a..40602126 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -498,11 +498,10 @@ var _ = Describe("Reconciler", func() { fakeClient.HandleGet = func() (*release.Release, error) { return exampleRelease, nil } - fakeClient.HandleMarkFailed = func() error { - return nil - } return &fakeClient, nil }) + //TODO: add actionConfigGetter fake + //r.actionConfigGetter = }) AfterEach(func() { r.actionClientGetter = nil @@ -515,7 +514,6 @@ var _ = Describe("Reconciler", func() { Expect(res).To(Equal(reconcile.Result{})) Expect(err).ToNot(BeNil()) Expect(err).To(MatchError("marked release example-release as failed to allow upgrade to succeed in next reconcile attempt")) - Expect(len(fakeClient.MarkFaileds)).Should(Equal(1)) }) }) From 3ef32564a7045547c24f5b4923976ee79e4b11f1 Mon Sep 17 00:00:00 2001 From: Simon Baeumer Date: Fri, 28 Jan 2022 22:05:49 +0100 Subject: [PATCH 6/6] Rename MarkFailed to Update --- pkg/client/actionclient.go | 5 ++ pkg/reconciler/internal/fake/actionclient.go | 58 +++++++++++--------- pkg/reconciler/reconciler.go | 48 ++++++---------- pkg/reconciler/reconciler_test.go | 5 +- 4 files changed, 55 insertions(+), 61 deletions(-) diff --git a/pkg/client/actionclient.go b/pkg/client/actionclient.go index 8f3fc80a..8949c8d8 100644 --- a/pkg/client/actionclient.go +++ b/pkg/client/actionclient.go @@ -62,6 +62,7 @@ type ActionInterface interface { Get(name string, opts ...GetOption) (*release.Release, error) Install(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...InstallOption) (*release.Release, error) Upgrade(name, namespace string, chrt *chart.Chart, vals map[string]interface{}, opts ...UpgradeOption) (*release.Release, error) + Update(release *release.Release) error Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) Reconcile(rel *release.Release) error } @@ -180,6 +181,10 @@ func (c *actionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return rel, nil } +func (c *actionClient) Update(rel *release.Release) error { + return c.conf.Releases.Update(rel) +} + func (c *actionClient) Uninstall(name string, opts ...UninstallOption) (*release.UninstallReleaseResponse, error) { uninstall := action.NewUninstall(c.conf) for _, o := range opts { diff --git a/pkg/reconciler/internal/fake/actionclient.go b/pkg/reconciler/internal/fake/actionclient.go index d5f44ff4..c2804429 100644 --- a/pkg/reconciler/internal/fake/actionclient.go +++ b/pkg/reconciler/internal/fake/actionclient.go @@ -48,19 +48,19 @@ func (hcg *fakeActionClientGetter) ActionClientFor(_ crclient.Object) (client.Ac } type ActionClient struct { - Gets []GetCall - Installs []InstallCall - Upgrades []UpgradeCall - MarkFaileds []MarkFailedCall - Uninstalls []UninstallCall - Reconciles []ReconcileCall + Gets []GetCall + Installs []InstallCall + Upgrades []UpgradeCall + Updates []UpdateCall + Uninstalls []UninstallCall + Reconciles []ReconcileCall - HandleGet func() (*release.Release, error) - HandleInstall func() (*release.Release, error) - HandleUpgrade func() (*release.Release, error) - HandleMarkFailed func() error - HandleUninstall func() (*release.UninstallReleaseResponse, error) - HandleReconcile func() error + HandleGet func() (*release.Release, error) + HandleInstall func() (*release.Release, error) + HandleUpgrade func() (*release.Release, error) + HandleUpdate func() error + HandleUninstall func() (*release.UninstallReleaseResponse, error) + HandleReconcile func() error } func NewActionClient() ActionClient { @@ -74,19 +74,19 @@ func NewActionClient() ActionClient { return func() error { return err } } return ActionClient{ - Gets: make([]GetCall, 0), - Installs: make([]InstallCall, 0), - Upgrades: make([]UpgradeCall, 0), - Uninstalls: make([]UninstallCall, 0), - Reconciles: make([]ReconcileCall, 0), - MarkFaileds: make([]MarkFailedCall, 0), - - HandleGet: relFunc(errors.New("get not implemented")), - HandleInstall: relFunc(errors.New("install not implemented")), - HandleUpgrade: relFunc(errors.New("upgrade not implemented")), - HandleUninstall: uninstFunc(errors.New("uninstall not implemented")), - HandleReconcile: recFunc(errors.New("reconcile not implemented")), - HandleMarkFailed: recFunc(errors.New("mark failed not implemented")), + Gets: make([]GetCall, 0), + Installs: make([]InstallCall, 0), + Upgrades: make([]UpgradeCall, 0), + Uninstalls: make([]UninstallCall, 0), + Reconciles: make([]ReconcileCall, 0), + Updates: make([]UpdateCall, 0), + + HandleGet: relFunc(errors.New("get not implemented")), + HandleInstall: relFunc(errors.New("install not implemented")), + HandleUpgrade: relFunc(errors.New("upgrade not implemented")), + HandleUninstall: uninstFunc(errors.New("uninstall not implemented")), + HandleReconcile: recFunc(errors.New("reconcile not implemented")), + HandleUpdate: recFunc(errors.New("mark failed not implemented")), } } @@ -113,9 +113,8 @@ type UpgradeCall struct { Opts []client.UpgradeOption } -type MarkFailedCall struct { +type UpdateCall struct { Release *release.Release - Reason string } type UninstallCall struct { @@ -142,6 +141,11 @@ func (c *ActionClient) Upgrade(name, namespace string, chrt *chart.Chart, vals m return c.HandleUpgrade() } +func (c *ActionClient) Update(release *release.Release) error { + c.Updates = append(c.Updates, UpdateCall{Release: release}) + return c.HandleUpdate() +} + func (c *ActionClient) Uninstall(name string, opts ...client.UninstallOption) (*release.UninstallReleaseResponse, error) { c.Uninstalls = append(c.Uninstalls, UninstallCall{name, opts}) return c.HandleUninstall() diff --git a/pkg/reconciler/reconciler.go b/pkg/reconciler/reconciler.go index 2f545405..79d46813 100644 --- a/pkg/reconciler/reconciler.go +++ b/pkg/reconciler/reconciler.go @@ -61,14 +61,13 @@ const uninstallFinalizer = "uninstall-helm-release" // Reconciler reconciles a Helm object type Reconciler struct { - client client.Client - actionClientGetter helmclient.ActionClientGetter - actionConfigGetter helmclient.ActionConfigGetter - valueTranslator values.Translator - valueMapper values.Mapper // nolint:staticcheck - eventRecorder record.EventRecorder - preHooks []hook.PreHook - postHooks []hook.PostHook + client client.Client + actionClientGetter helmclient.ActionClientGetter + valueTranslator values.Translator + valueMapper values.Mapper // nolint:staticcheck + eventRecorder record.EventRecorder + preHooks []hook.PreHook + postHooks []hook.PostHook log logr.Logger gvk *schema.GroupVersionKind @@ -177,16 +176,6 @@ func WithActionClientGetter(actionClientGetter helmclient.ActionClientGetter) Op } } -// WithActionConfigGetter is an Option that configures a Reconciler's ActionConfigGetter -// -// A default ActionConfigGetter is used if this option is not configured. -func WithActionConfigGetter(actionConfigGetter helmclient.ActionConfigGetter) Option { - return func(r *Reconciler) error { - r.actionConfigGetter = actionConfigGetter - return nil - } -} - // WithEventRecorder is an Option that configures a Reconciler's EventRecorder. // // By default, manager.GetEventRecorderFor() is used if this option is not @@ -556,7 +545,7 @@ func (r *Reconciler) Reconcile(ctx context.Context, req ctrl.Request) (res ctrl. return ctrl.Result{}, err } if state == statePending { - return r.handlePending(obj, rel, &u, log) + return r.handlePending(actionClient, rel, &u, log) } u.UpdateStatus(updater.EnsureCondition(conditions.Irreconcilable(corev1.ConditionFalse, "", ""))) @@ -755,8 +744,8 @@ func (r *Reconciler) doUpgrade(actionClient helmclient.ActionInterface, u *updat return rel, nil } -func (r *Reconciler) handlePending(obj *unstructured.Unstructured, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { - err := r.doHandlePending(obj, rel, log) +func (r *Reconciler) handlePending(actionClient helmclient.ActionInterface, rel *release.Release, u *updater.Updater, log logr.Logger) (ctrl.Result, error) { + err := r.doHandlePending(actionClient, rel, log) if err == nil { err = errors.New("unknown error handling pending release") } @@ -765,7 +754,7 @@ func (r *Reconciler) handlePending(obj *unstructured.Unstructured, rel *release. return ctrl.Result{}, err } -func (r *Reconciler) doHandlePending(obj *unstructured.Unstructured, rel *release.Release, log logr.Logger) error { +func (r *Reconciler) doHandlePending(actionClient helmclient.ActionInterface, rel *release.Release, log logr.Logger) error { if r.markFailedAfter <= 0 { return errors.New("Release is in a pending (locked) state and cannot be modified. User intervention is required.") } @@ -777,24 +766,19 @@ func (r *Reconciler) doHandlePending(obj *unstructured.Unstructured, rel *releas } log.Info("Marking release as failed", "releaseName", rel.Name) - err := r.markReleaseFailed(obj, rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) + err := r.markReleaseFailed(actionClient, rel, fmt.Sprintf("operator marked pending (locked) release as failed after state did not change for %v", r.markFailedAfter)) if err != nil { return fmt.Errorf("Failed to mark pending (locked) release as failed: %w", err) } return fmt.Errorf("marked release %s as failed to allow upgrade to succeed in next reconcile attempt", rel.Name) } -func (r *Reconciler) markReleaseFailed(obj *unstructured.Unstructured, rel *release.Release, reason string) error { +func (r *Reconciler) markReleaseFailed(actionClient helmclient.ActionInterface, rel *release.Release, reason string) error { infoCopy := *rel.Info releaseCopy := *rel releaseCopy.Info = &infoCopy releaseCopy.SetStatus(release.StatusFailed, reason) - - actionConfig, err := r.actionConfigGetter.ActionConfigFor(obj) - if err != nil { - return err - } - return actionConfig.Releases.Update(&releaseCopy) + return actionClient.Update(&releaseCopy) } func (r *Reconciler) reportOverrideEvents(obj runtime.Object) { @@ -871,8 +855,8 @@ func (r *Reconciler) addDefaults(mgr ctrl.Manager, controllerName string) { r.log = ctrl.Log.WithName("controllers").WithName("Helm") } if r.actionClientGetter == nil { - r.actionConfigGetter = helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), r.log) - r.actionClientGetter = helmclient.NewActionClientGetter(r.actionConfigGetter) + actionConfigGetter := helmclient.NewActionConfigGetter(mgr.GetConfig(), mgr.GetRESTMapper(), r.log) + r.actionClientGetter = helmclient.NewActionClientGetter(actionConfigGetter) } if r.eventRecorder == nil { r.eventRecorder = mgr.GetEventRecorderFor(controllerName) diff --git a/pkg/reconciler/reconciler_test.go b/pkg/reconciler/reconciler_test.go index 40602126..df3b4704 100644 --- a/pkg/reconciler/reconciler_test.go +++ b/pkg/reconciler/reconciler_test.go @@ -498,10 +498,11 @@ var _ = Describe("Reconciler", func() { fakeClient.HandleGet = func() (*release.Release, error) { return exampleRelease, nil } + fakeClient.HandleUpdate = func() error { + return nil + } return &fakeClient, nil }) - //TODO: add actionConfigGetter fake - //r.actionConfigGetter = }) AfterEach(func() { r.actionClientGetter = nil