From be9e22f5f5f530b4d2d97739de9ca15c59962ed7 Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Wed, 26 Aug 2020 16:10:44 -0500 Subject: [PATCH 1/4] More graceful failure recovery - Ensure upgrade actually occurs if known state was not reached for any reason (other than install failure). - After transient failures not tied to new state application, ensure spurious upgrades do not occur and ready state is again reached, by remembering that the known state was already successfully applied. - Reset failure counts after success so they're not stale. - Only lookup post-deployment release revision on remediation, since otherwise we already have it. - Push status update after finding new state so user can observe. --- api/v2alpha1/helmrelease_types.go | 56 ++++++++++++------- .../helm.toolkit.fluxcd.io_helmreleases.yaml | 13 ++++- controllers/helmrelease_controller.go | 47 +++++++++------- docs/api/helmrelease.md | 21 ++++++- docs/spec/v2alpha1/helmreleases.md | 1 + 5 files changed, 92 insertions(+), 46 deletions(-) diff --git a/api/v2alpha1/helmrelease_types.go b/api/v2alpha1/helmrelease_types.go index d7c69431a..c9c28e9b9 100644 --- a/api/v2alpha1/helmrelease_types.go +++ b/api/v2alpha1/helmrelease_types.go @@ -571,6 +571,10 @@ type HelmReleaseStatus struct { // +optional Conditions []Condition `json:"conditions,omitempty"` + // KnownStateApplied represents whether the known state has been successfully applied. + // +optional + KnownStateApplied bool `json:"knownStateApplied,omitempty"` + // LastAppliedRevision is the revision of the last successfully applied source. // +optional LastAppliedRevision string `json:"lastAppliedRevision,omitempty"` @@ -592,15 +596,18 @@ type HelmReleaseStatus struct { // +optional HelmChart string `json:"helmChart,omitempty"` - // Failures is the reconciliation failure count. + // Failures is the reconciliation failure count against the known state. + // It is reset after a successful reconciliation. // +optional Failures int64 `json:"failures,omitempty"` - // InstallFailures is the install failure count. + // InstallFailures is the install failure count against the known state. + // It is reset after a successful reconciliation. // +optional InstallFailures int64 `json:"installFailures,omitempty"` - // UpgradeFailures is the upgrade failure count. + // UpgradeFailures is the upgrade failure count against the known state. + // It is reset after a successful reconciliation. // +optional UpgradeFailures int64 `json:"upgradeFailures,omitempty"` } @@ -617,27 +624,13 @@ func (in HelmReleaseStatus) GetHelmChart() (string, string) { // HelmReleaseProgressing resets any failures and registers progress toward reconciling the given HelmRelease // by setting the ReadyCondition to ConditionUnknown for ProgressingReason. func HelmReleaseProgressing(hr HelmRelease) HelmRelease { - hr.Status.Failures = 0 - hr.Status.InstallFailures = 0 - hr.Status.UpgradeFailures = 0 + resetFailureCounts(&hr) + hr.Status.KnownStateApplied = false hr.Status.Conditions = []Condition{} SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionUnknown, ProgressingReason, "reconciliation in progress") return hr } -// SetHelmReleaseCondition sets the given condition with the given status, reason and message -// on the HelmRelease. -func SetHelmReleaseCondition(hr *HelmRelease, condition string, status corev1.ConditionStatus, reason, message string) { - hr.Status.Conditions = filterOutCondition(hr.Status.Conditions, condition) - hr.Status.Conditions = append(hr.Status.Conditions, Condition{ - Type: condition, - Status: status, - LastTransitionTime: metav1.Now(), - Reason: reason, - Message: message, - }) -} - // HelmReleaseNotReady registers a failed release attempt of the given HelmRelease. func HelmReleaseNotReady(hr HelmRelease, reason, message string) HelmRelease { SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionFalse, reason, message) @@ -646,9 +639,11 @@ func HelmReleaseNotReady(hr HelmRelease, reason, message string) HelmRelease { } // HelmReleaseReady registers a successful release attempt of the given HelmRelease. -func HelmReleaseReady(hr HelmRelease, reason, message string) HelmRelease { - SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionTrue, reason, message) +func HelmReleaseReady(hr HelmRelease) HelmRelease { + resetFailureCounts(&hr) + hr.Status.KnownStateApplied = true hr.Status.LastAppliedRevision = hr.Status.LastAttemptedRevision + SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionTrue, ReconciliationSucceededReason, "release reconciliation succeeded") return hr } @@ -665,6 +660,25 @@ func HelmReleaseAttempted(hr HelmRelease, revision string, releaseRevision int, return hr, changed } +func resetFailureCounts(hr *HelmRelease) { + hr.Status.Failures = 0 + hr.Status.InstallFailures = 0 + hr.Status.UpgradeFailures = 0 +} + +// SetHelmReleaseCondition sets the given condition with the given status, reason and message +// on the HelmRelease. +func SetHelmReleaseCondition(hr *HelmRelease, condition string, status corev1.ConditionStatus, reason, message string) { + hr.Status.Conditions = filterOutCondition(hr.Status.Conditions, condition) + hr.Status.Conditions = append(hr.Status.Conditions, Condition{ + Type: condition, + Status: status, + LastTransitionTime: metav1.Now(), + Reason: reason, + Message: message, + }) +} + const ( // ReconcileAtAnnotation is the annotation used for triggering a // reconciliation outside of the defined schedule. diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index f3e3467c3..14229be9c 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -384,7 +384,8 @@ spec: type: object type: array failures: - description: Failures is the reconciliation failure count. + description: Failures is the reconciliation failure count against + the known state. It is reset after a successful reconciliation. format: int64 type: integer helmChart: @@ -392,9 +393,14 @@ spec: created by the controller for the HelmRelease. type: string installFailures: - description: InstallFailures is the install failure count. + description: InstallFailures is the install failure count against + the known state. It is reset after a successful reconciliation. format: int64 type: integer + knownStateApplied: + description: KnownStateApplied represents whether the known state + has been successfully applied. + type: boolean lastAppliedRevision: description: LastAppliedRevision is the revision of the last successfully applied source. @@ -416,7 +422,8 @@ spec: format: int64 type: integer upgradeFailures: - description: UpgradeFailures is the upgrade failure count. + description: UpgradeFailures is the upgrade failure count against + the known state. It is reset after a successful reconciliation. format: int64 type: integer type: object diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index c81673a24..7c569b286 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -137,8 +137,7 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) } // Observe the HelmRelease generation. - hasNewGeneration := hr.Status.ObservedGeneration != hr.Generation - if hasNewGeneration { + if hr.Status.ObservedGeneration != hr.Generation { hr.Status.ObservedGeneration = hr.Generation hr = v2.HelmReleaseProgressing(hr) } @@ -211,7 +210,7 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) return ctrl.Result{}, nil } - reconciledHr, reconcileErr := r.release(log, *hr.DeepCopy(), hc, values, hasNewGeneration) + reconciledHr, reconcileErr := r.release(ctx, log, *hr.DeepCopy(), hc, values) if reconcileErr != nil { r.event(hr, hc.GetArtifact().Revision, recorder.EventSeverityError, fmt.Sprintf("reconciliation failed: %s", reconcileErr.Error())) } @@ -295,7 +294,7 @@ func (r *HelmReleaseReconciler) reconcileChart(ctx context.Context, hr *v2.HelmR return &helmChart, true, nil } -func (r *HelmReleaseReconciler) release(log logr.Logger, hr v2.HelmRelease, source sourcev1.Source, values chartutil.Values, hasNewGeneration bool) (v2.HelmRelease, error) { +func (r *HelmReleaseReconciler) release(ctx context.Context, log logr.Logger, hr v2.HelmRelease, source sourcev1.Source, values chartutil.Values) (v2.HelmRelease, error) { // Acquire lock unlock, err := lock(fmt.Sprintf("%s-%s", hr.GetName(), hr.GetNamespace())) if err != nil { @@ -342,20 +341,29 @@ func (r *HelmReleaseReconciler) release(log logr.Logger, hr v2.HelmRelease, sour hr, hasNewState := v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum) if hasNewState { hr = v2.HelmReleaseProgressing(hr) + if err := r.Status().Update(ctx, &hr); err != nil { + log.Error(err, "unable to update status") + return hr, err + } } // Determine release deployment action. var deployAction v2.DeploymentAction switch { - // Install if there is none. + // Install if there is no release. case rel == nil: deployAction = hr.Spec.GetInstall() - // Upgrade if there is a new generation, new state, or this is an upgrade retry. - case hasNewGeneration || hasNewState || hr.Spec.GetUpgrade().GetRemediation().GetFailureCount(hr) > 0: - deployAction = hr.Spec.GetUpgrade() - // Otherwise no action needed. + // Fail if the release was due to a failed install (which was not uninstalled). + // The uninstall may have failed, or was not needed due to retries being exhausted + // and remediateLastFailure being false. + case hr.Spec.GetInstall().GetRemediation().GetFailureCount(hr) > 0: + return hr, fmt.Errorf("last install failed but was not uninstalled") + // Skip and mark ready if the known state was already applied. + case hr.Status.KnownStateApplied: + return v2.HelmReleaseReady(hr), nil + // Otherwise upgrade. default: - return hr, nil + deployAction = hr.Spec.GetUpgrade() } // Check if retries exhausted. @@ -405,17 +413,18 @@ func (r *HelmReleaseReconciler) release(log logr.Logger, hr v2.HelmRelease, sour err = uninstallConditionErr } } - } - } - // Determine release revision after deployment/remediation. - rel, observeLastReleaseErr = observeLastRelease(cfg, hr) - if observeLastReleaseErr != nil { - err = &ConditionError{ - Reason: v2.GetLastReleaseFailedReason, - Err: errors.New("failed to get last release revision after deployment/remediation"), + // Determine release after remediation. + rel, observeLastReleaseErr = observeLastRelease(cfg, hr) + if observeLastReleaseErr != nil { + err = &ConditionError{ + Reason: v2.GetLastReleaseFailedReason, + Err: errors.New("failed to get last release revision after remediation"), + } + } } } + hr.Status.LastReleaseRevision = getReleaseRevision(rel) if err != nil { @@ -426,7 +435,7 @@ func (r *HelmReleaseReconciler) release(log logr.Logger, hr v2.HelmRelease, sour } return v2.HelmReleaseNotReady(hr, reason, err.Error()), err } - return v2.HelmReleaseReady(hr, v2.ReconciliationSucceededReason, "release reconciliation succeeded"), nil + return v2.HelmReleaseReady(hr), nil } func (r *HelmReleaseReconciler) checkDependencies(hr v2.HelmRelease) error { diff --git a/docs/api/helmrelease.md b/docs/api/helmrelease.md index 2d7acab09..0519a0651 100644 --- a/docs/api/helmrelease.md +++ b/docs/api/helmrelease.md @@ -789,6 +789,18 @@ int64 +knownStateApplied
+ +bool + + + +(Optional) +

KnownStateApplied represents whether the known state has been successfully applied.

+ + + + lastAppliedRevision
string @@ -857,7 +869,8 @@ int64 (Optional) -

Failures is the reconciliation failure count.

+

Failures is the reconciliation failure count against the known state. +It is reset after a successful reconciliation.

@@ -869,7 +882,8 @@ int64 (Optional) -

InstallFailures is the install failure count.

+

InstallFailures is the install failure count against the known state. +It is reset after a successful reconciliation.

@@ -881,7 +895,8 @@ int64 (Optional) -

UpgradeFailures is the upgrade failure count.

+

UpgradeFailures is the upgrade failure count against the known state. +It is reset after a successful reconciliation.

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md index 9b7b18657..7491b0d60 100644 --- a/docs/spec/v2alpha1/helmreleases.md +++ b/docs/spec/v2alpha1/helmreleases.md @@ -671,6 +671,7 @@ status: reason: ReconciliationSucceeded status: "True" type: Ready + knownStateApplied: true lastAppliedRevision: 4.0.6 lastAttemptedRevision: 4.0.6 lastReleaseRevision: 1 From fd7d23256ab9f680cebabb021222e2dbbbaffaca Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Mon, 31 Aug 2020 12:12:48 -0500 Subject: [PATCH 2/4] Rename knownStateApplied to observedStateReconciled This is more consistent with the existing terminology used. --- api/v2alpha1/helmrelease_types.go | 14 +++++++------- .../bases/helm.toolkit.fluxcd.io_helmreleases.yaml | 14 +++++++------- controllers/helmrelease_controller.go | 4 ++-- docs/api/helmrelease.md | 10 +++++----- docs/spec/v2alpha1/helmreleases.md | 2 +- 5 files changed, 22 insertions(+), 22 deletions(-) diff --git a/api/v2alpha1/helmrelease_types.go b/api/v2alpha1/helmrelease_types.go index c9c28e9b9..73210d0d6 100644 --- a/api/v2alpha1/helmrelease_types.go +++ b/api/v2alpha1/helmrelease_types.go @@ -571,9 +571,9 @@ type HelmReleaseStatus struct { // +optional Conditions []Condition `json:"conditions,omitempty"` - // KnownStateApplied represents whether the known state has been successfully applied. + // ObservedStateReconciled represents whether the observed state has been successfully reconciled. // +optional - KnownStateApplied bool `json:"knownStateApplied,omitempty"` + ObservedStateReconciled bool `json:"observedStateReconciled,omitempty"` // LastAppliedRevision is the revision of the last successfully applied source. // +optional @@ -596,17 +596,17 @@ type HelmReleaseStatus struct { // +optional HelmChart string `json:"helmChart,omitempty"` - // Failures is the reconciliation failure count against the known state. + // Failures is the reconciliation failure count against the latest observed state. // It is reset after a successful reconciliation. // +optional Failures int64 `json:"failures,omitempty"` - // InstallFailures is the install failure count against the known state. + // InstallFailures is the install failure count against the latest observed state. // It is reset after a successful reconciliation. // +optional InstallFailures int64 `json:"installFailures,omitempty"` - // UpgradeFailures is the upgrade failure count against the known state. + // UpgradeFailures is the upgrade failure count against the latest observed state. // It is reset after a successful reconciliation. // +optional UpgradeFailures int64 `json:"upgradeFailures,omitempty"` @@ -625,7 +625,7 @@ func (in HelmReleaseStatus) GetHelmChart() (string, string) { // by setting the ReadyCondition to ConditionUnknown for ProgressingReason. func HelmReleaseProgressing(hr HelmRelease) HelmRelease { resetFailureCounts(&hr) - hr.Status.KnownStateApplied = false + hr.Status.ObservedStateReconciled = false hr.Status.Conditions = []Condition{} SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionUnknown, ProgressingReason, "reconciliation in progress") return hr @@ -641,7 +641,7 @@ func HelmReleaseNotReady(hr HelmRelease, reason, message string) HelmRelease { // HelmReleaseReady registers a successful release attempt of the given HelmRelease. func HelmReleaseReady(hr HelmRelease) HelmRelease { resetFailureCounts(&hr) - hr.Status.KnownStateApplied = true + hr.Status.ObservedStateReconciled = true hr.Status.LastAppliedRevision = hr.Status.LastAttemptedRevision SetHelmReleaseCondition(&hr, ReadyCondition, corev1.ConditionTrue, ReconciliationSucceededReason, "release reconciliation succeeded") return hr diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index 14229be9c..5c055cc73 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -385,7 +385,7 @@ spec: type: array failures: description: Failures is the reconciliation failure count against - the known state. It is reset after a successful reconciliation. + the latest observed state. It is reset after a successful reconciliation. format: int64 type: integer helmChart: @@ -394,13 +394,9 @@ spec: type: string installFailures: description: InstallFailures is the install failure count against - the known state. It is reset after a successful reconciliation. + the latest observed state. It is reset after a successful reconciliation. format: int64 type: integer - knownStateApplied: - description: KnownStateApplied represents whether the known state - has been successfully applied. - type: boolean lastAppliedRevision: description: LastAppliedRevision is the revision of the last successfully applied source. @@ -421,9 +417,13 @@ spec: description: ObservedGeneration is the last reconciled generation. format: int64 type: integer + observedStateReconciled: + description: ObservedStateReconciled represents whether the observed + state has been successfully reconciled. + type: boolean upgradeFailures: description: UpgradeFailures is the upgrade failure count against - the known state. It is reset after a successful reconciliation. + the latest observed state. It is reset after a successful reconciliation. format: int64 type: integer type: object diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index 7c569b286..0d4b19384 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -358,8 +358,8 @@ func (r *HelmReleaseReconciler) release(ctx context.Context, log logr.Logger, hr // and remediateLastFailure being false. case hr.Spec.GetInstall().GetRemediation().GetFailureCount(hr) > 0: return hr, fmt.Errorf("last install failed but was not uninstalled") - // Skip and mark ready if the known state was already applied. - case hr.Status.KnownStateApplied: + // Skip and mark ready if the observed state was already reconciled. + case hr.Status.ObservedStateReconciled: return v2.HelmReleaseReady(hr), nil // Otherwise upgrade. default: diff --git a/docs/api/helmrelease.md b/docs/api/helmrelease.md index 0519a0651..ffc62fa8c 100644 --- a/docs/api/helmrelease.md +++ b/docs/api/helmrelease.md @@ -789,14 +789,14 @@ int64 -knownStateApplied
+observedStateReconciled
bool (Optional) -

KnownStateApplied represents whether the known state has been successfully applied.

+

ObservedStateReconciled represents whether the observed state has been successfully reconciled.

@@ -869,7 +869,7 @@ int64 (Optional) -

Failures is the reconciliation failure count against the known state. +

Failures is the reconciliation failure count against the latest observed state. It is reset after a successful reconciliation.

@@ -882,7 +882,7 @@ int64 (Optional) -

InstallFailures is the install failure count against the known state. +

InstallFailures is the install failure count against the latest observed state. It is reset after a successful reconciliation.

@@ -895,7 +895,7 @@ int64 (Optional) -

UpgradeFailures is the upgrade failure count against the known state. +

UpgradeFailures is the upgrade failure count against the latest observed state. It is reset after a successful reconciliation.

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md index 7491b0d60..c80f66353 100644 --- a/docs/spec/v2alpha1/helmreleases.md +++ b/docs/spec/v2alpha1/helmreleases.md @@ -671,7 +671,7 @@ status: reason: ReconciliationSucceeded status: "True" type: Ready - knownStateApplied: true + observedStateReconciled: true lastAppliedRevision: 4.0.6 lastAttemptedRevision: 4.0.6 lastReleaseRevision: 1 From 55f603806df922b8335606290c39501ca4b53d09 Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Mon, 31 Aug 2020 13:22:10 -0500 Subject: [PATCH 3/4] Record last observed time in status This adds a .status.lastObservedTime field which records when the HelmRelease was last observed by the controller. This allows the user to observe whether the spec.interval and reconcileAt annotations are triggering reconciliation attempts as desired. --- api/v2alpha1/helmrelease_types.go | 14 +++++---- api/v2alpha1/zz_generated.deepcopy.go | 1 + .../helm.toolkit.fluxcd.io_helmreleases.yaml | 7 ++++- controllers/helmrelease_controller.go | 15 ++++++---- docs/api/helmrelease.md | 30 ++++++++++++++----- docs/spec/v2alpha1/helmreleases.md | 2 ++ 6 files changed, 49 insertions(+), 20 deletions(-) diff --git a/api/v2alpha1/helmrelease_types.go b/api/v2alpha1/helmrelease_types.go index 73210d0d6..9c7d8e55c 100644 --- a/api/v2alpha1/helmrelease_types.go +++ b/api/v2alpha1/helmrelease_types.go @@ -563,18 +563,22 @@ func (in Uninstall) GetTimeout(defaultTimeout metav1.Duration) metav1.Duration { // HelmReleaseStatus defines the observed state of HelmRelease type HelmReleaseStatus struct { - // ObservedGeneration is the last reconciled generation. + // ObservedGeneration is the last observed generation. // +optional ObservedGeneration int64 `json:"observedGeneration,omitempty"` - // Conditions holds the conditions for the HelmRelease. - // +optional - Conditions []Condition `json:"conditions,omitempty"` - // ObservedStateReconciled represents whether the observed state has been successfully reconciled. // +optional ObservedStateReconciled bool `json:"observedStateReconciled,omitempty"` + // LastObservedTime is the last time at which the HelmRelease was observed. + // +optional + LastObservedTime metav1.Time `json:"lastObservedTime,omitempty"` + + // Conditions holds the conditions for the HelmRelease. + // +optional + Conditions []Condition `json:"conditions,omitempty"` + // LastAppliedRevision is the revision of the last successfully applied source. // +optional LastAppliedRevision string `json:"lastAppliedRevision,omitempty"` diff --git a/api/v2alpha1/zz_generated.deepcopy.go b/api/v2alpha1/zz_generated.deepcopy.go index 93b9603d3..2dffa35bf 100644 --- a/api/v2alpha1/zz_generated.deepcopy.go +++ b/api/v2alpha1/zz_generated.deepcopy.go @@ -207,6 +207,7 @@ func (in *HelmReleaseSpec) DeepCopy() *HelmReleaseSpec { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *HelmReleaseStatus) DeepCopyInto(out *HelmReleaseStatus) { *out = *in + in.LastObservedTime.DeepCopyInto(&out.LastObservedTime) if in.Conditions != nil { in, out := &in.Conditions, &out.Conditions *out = make([]Condition, len(*in)) diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml index 5c055cc73..9f3e591b9 100644 --- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml +++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml @@ -409,12 +409,17 @@ spec: description: LastAttemptedValuesChecksum is the SHA1 checksum of the values of the last reconciliation attempt. type: string + lastObservedTime: + description: LastObservedTime is the last time at which the HelmRelease + was observed. + format: date-time + type: string lastReleaseRevision: description: LastReleaseRevision is the revision of the last successful Helm release. type: integer observedGeneration: - description: ObservedGeneration is the last reconciled generation. + description: ObservedGeneration is the last observed generation. format: int64 type: integer observedStateReconciled: diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index 0d4b19384..c67c37010 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -125,6 +125,15 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) return ctrl.Result{}, nil } + // Record time of reconciliation attempt. + hr.Status.LastObservedTime = v1.Now() + + // Observe HelmRelease generation. + if hr.Status.ObservedGeneration != hr.Generation { + hr.Status.ObservedGeneration = hr.Generation + hr = v2.HelmReleaseProgressing(hr) + } + if hr.Spec.Suspend { msg := "HelmRelease is suspended, skipping reconciliation" hr = v2.HelmReleaseNotReady(hr, v2.SuspendedReason, msg) @@ -136,12 +145,6 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) return ctrl.Result{}, nil } - // Observe the HelmRelease generation. - if hr.Status.ObservedGeneration != hr.Generation { - hr.Status.ObservedGeneration = hr.Generation - hr = v2.HelmReleaseProgressing(hr) - } - if err := r.Status().Update(ctx, &hr); err != nil { log.Error(err, "unable to update status") return ctrl.Result{Requeue: true}, err diff --git a/docs/api/helmrelease.md b/docs/api/helmrelease.md index ffc62fa8c..cbdaf827c 100644 --- a/docs/api/helmrelease.md +++ b/docs/api/helmrelease.md @@ -770,33 +770,47 @@ int64 (Optional) -

ObservedGeneration is the last reconciled generation.

+

ObservedGeneration is the last observed generation.

-conditions
+observedStateReconciled
- -[]Condition +bool + + + +(Optional) +

ObservedStateReconciled represents whether the observed state has been successfully reconciled.

+ + + + +lastObservedTime
+ + +Kubernetes meta/v1.Time (Optional) -

Conditions holds the conditions for the HelmRelease.

+

LastObservedTime is the last time at which the HelmRelease was observed.

-observedStateReconciled
+conditions
-bool + +[]Condition + (Optional) -

ObservedStateReconciled represents whether the observed state has been successfully reconciled.

+

Conditions holds the conditions for the HelmRelease.

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md index c80f66353..24ada4d95 100644 --- a/docs/spec/v2alpha1/helmreleases.md +++ b/docs/spec/v2alpha1/helmreleases.md @@ -674,6 +674,7 @@ status: observedStateReconciled: true lastAppliedRevision: 4.0.6 lastAttemptedRevision: 4.0.6 + lastObservedTime: "2020-07-13T13:18:42Z" lastReleaseRevision: 1 observedGeneration: 2 ``` @@ -704,6 +705,7 @@ status: failures: 1 lastAppliedRevision: 4.0.6 lastAttemptedRevision: 4.0.6 + lastObservedTime: "2020-07-13T18:17:28Z" lastReleaseRevision: 1 observedGeneration: 3 ``` From 0d64e8dc7353cd4a309e28d3b7ceee0260167b55 Mon Sep 17 00:00:00 2001 From: Sean Eagan Date: Mon, 31 Aug 2020 14:55:49 -0500 Subject: [PATCH 4/4] Update status iff new state or done reconciling Also set status.lastObservedTime to the actual time of the status update. --- controllers/helmrelease_controller.go | 87 +++++++++++---------------- 1 file changed, 35 insertions(+), 52 deletions(-) diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go index c67c37010..0e987531a 100644 --- a/controllers/helmrelease_controller.go +++ b/controllers/helmrelease_controller.go @@ -125,29 +125,38 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) return ctrl.Result{}, nil } - // Record time of reconciliation attempt. - hr.Status.LastObservedTime = v1.Now() + hr, result, err := r.reconcile(ctx, log, hr) + + // Update status after reconciliation. + if updateStatusErr := r.updateStatus(ctx, &hr); updateStatusErr != nil { + log.Error(updateStatusErr, "unable to update status after reconciliation") + return ctrl.Result{Requeue: true}, updateStatusErr + } + + // Log reconciliation duration + log.Info(fmt.Sprintf("reconcilation finished in %s, next run in %s", + time.Now().Sub(start).String(), + hr.Spec.Interval.Duration.String(), + )) + return result, err +} + +func (r *HelmReleaseReconciler) reconcile(ctx context.Context, log logr.Logger, hr v2.HelmRelease) (v2.HelmRelease, ctrl.Result, error) { // Observe HelmRelease generation. if hr.Status.ObservedGeneration != hr.Generation { hr.Status.ObservedGeneration = hr.Generation hr = v2.HelmReleaseProgressing(hr) + if updateStatusErr := r.updateStatus(ctx, &hr); updateStatusErr != nil { + log.Error(updateStatusErr, "unable to update status after generation update") + return hr, ctrl.Result{Requeue: true}, updateStatusErr + } } if hr.Spec.Suspend { msg := "HelmRelease is suspended, skipping reconciliation" - hr = v2.HelmReleaseNotReady(hr, v2.SuspendedReason, msg) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } log.Info(msg) - return ctrl.Result{}, nil - } - - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err + return v2.HelmReleaseNotReady(hr, v2.SuspendedReason, msg), ctrl.Result{}, nil } // Reconcile chart based on the HelmChartTemplate @@ -161,25 +170,15 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) msg = "HelmChart is not ready" r.event(hr, hr.Status.LastAttemptedRevision, recorder.EventSeverityInfo, msg) } - hr = v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } - return ctrl.Result{}, reconcileErr + return v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg), ctrl.Result{}, reconcileErr } // Check chart artifact readiness if hc.GetArtifact() == nil { msg := "HelmChart is not ready" - hr = v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg) r.event(hr, hr.Status.LastAttemptedRevision, recorder.EventSeverityInfo, msg) log.Info(msg) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } - return ctrl.Result{}, nil + return v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg), ctrl.Result{}, nil } // Check dependencies @@ -189,14 +188,9 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) r.event(hr, hc.GetArtifact().Revision, recorder.EventSeverityInfo, msg) log.Info(msg) - hr = v2.HelmReleaseNotReady(hr, v2.DependencyNotReadyReason, err.Error()) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } // Exponential backoff would cause execution to be prolonged too much, // instead we requeue on a fixed interval. - return ctrl.Result{RequeueAfter: r.requeueDependency}, nil + return v2.HelmReleaseNotReady(hr, v2.DependencyNotReadyReason, err.Error()), ctrl.Result{RequeueAfter: r.requeueDependency}, nil } log.Info("all dependencies are ready, proceeding with release") } @@ -204,13 +198,8 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) // Compose values values, err := r.composeValues(ctx, hr) if err != nil { - hr = v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error()) r.event(hr, hr.Status.LastAttemptedRevision, recorder.EventSeverityError, err.Error()) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return ctrl.Result{Requeue: true}, err - } - return ctrl.Result{}, nil + return v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error()), ctrl.Result{}, nil } reconciledHr, reconcileErr := r.release(ctx, log, *hr.DeepCopy(), hc, values) @@ -218,18 +207,7 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) r.event(hr, hc.GetArtifact().Revision, recorder.EventSeverityError, fmt.Sprintf("reconciliation failed: %s", reconcileErr.Error())) } - if err := r.Status().Update(ctx, &reconciledHr); err != nil { - log.Error(err, "unable to update status after reconciliation") - return ctrl.Result{Requeue: true}, err - } - - // Log reconciliation duration - log.Info(fmt.Sprintf("reconcilation finished in %s, next run in %s", - time.Now().Sub(start).String(), - hr.Spec.Interval.Duration.String(), - )) - - return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, reconcileErr + return reconciledHr, ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, reconcileErr } type HelmReleaseReconcilerOptions struct { @@ -344,9 +322,9 @@ func (r *HelmReleaseReconciler) release(ctx context.Context, log logr.Logger, hr hr, hasNewState := v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum) if hasNewState { hr = v2.HelmReleaseProgressing(hr) - if err := r.Status().Update(ctx, &hr); err != nil { - log.Error(err, "unable to update status") - return hr, err + if updateStatusErr := r.updateStatus(ctx, &hr); updateStatusErr != nil { + log.Error(updateStatusErr, "unable to update status after state update") + return hr, updateStatusErr } } @@ -441,6 +419,11 @@ func (r *HelmReleaseReconciler) release(ctx context.Context, log logr.Logger, hr return v2.HelmReleaseReady(hr), nil } +func (r *HelmReleaseReconciler) updateStatus(ctx context.Context, hr *v2.HelmRelease) error { + hr.Status.LastObservedTime = v1.Now() + return r.Status().Update(ctx, hr) +} + func (r *HelmReleaseReconciler) checkDependencies(hr v2.HelmRelease) error { for _, dep := range hr.Spec.DependsOn { depName := types.NamespacedName{