helm-controller: conditional remediation on failed Helm actions

[hiddeco]

Outline

As a human you are able to take this into account while you perform helm operations by hand, due to a response -> interpretation -> human decision flow. However, if you automate all of this it simply means that there isn't a "one configuration fits all" answer, there may be a "this will work for most things" one (which we can offer by providing "sane defaults") but there will always be exceptions to this.

In addition to this, it also depends on the people operating (or rather watching) over the HelmRelease, and what their desires are for automated things failing. Again, there isn't a "one configuration fits all" answer.

This is why we need some knobs (but not too many) to be able to offer users the automated behaviour they are after for their precious release.

Proposal

The idea is to add a Remediation struct to the API, to be implemented by the Helm actions that should offer a remediation strategy in case they fail (Install, Upgrade and Test). This is based on @seaneagan his API sketch.

Spec changes

Remediation

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -96,6 +96,21 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+type Remediation struct {
+	// Retries is the number of retries that should be attempted on failures before
+	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// +optional
+	Retries int `json:"retries,omitempty"`
+
+	// Strategy to use for the remediation of a failed action.
+	// +kubebuilder:validation:Enum=rollback,uninstall,ignore
+	// +optional
+	Strategy string `json:"strategy,omitempty"`
+
+	// DoNotRemediateOnLastRetry tells the controller to skip remediation when
+	// there are no retries left so the release can be debugged.
+	// +optional
+	DoNotRemediateOnLastRetry bool `json:"doNotRemediateOnLastRetry,omitempty"`
+}

Install

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
 // Install holds the configuration for Helm install actions.
 type Install struct {
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs
@@ -104,6 +119,12 @@ type Install struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

+	// Remediation holds the remediation configuration for when the
+	// Helm install action fails. The default install failure
+	// remediation is an uninstall action.
+	// +optional
+	Remediation Remediation `json:"remediation,omitempty"`
+
 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm install has been performed.
 	// +optional

Upgrade

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -137,10 +158,11 @@ type Upgrade struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

-	// MaxRetries is the number of retries that should be attempted on failures before
-	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// Remediation holds the remediation configuration for when the
+	// Helm upgrade action for the HelmRelease fails. The default
+	// upgrade failure remediation is to not perform any action.
 	// +optional
-	MaxRetries int `json:"maxRetries,omitempty"`
+	Remediation Remediation `json:"remediation,omitempty"`

 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm upgrade has been performed.

Test

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -179,6 +201,12 @@ type Test struct {
 	// +optional
 	Enable bool `json:"enable,omitempty"`

+	// Remediation holds the remediation configuration for when the
+	// Helm test action for the HelmRelease fails. The default test
+	// failure remediation is to ignore the test result.
+	// +optional
+	Remediation Remediation `json:"remediation,omitempty"`
+
 	// Timeout is the time to wait for any individual Kubernetes operation
 	// during the performance of a Helm test action. Defaults to
 	// 'HelmReleaseSpec.Timeout'.

Rollback

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -188,11 +216,6 @@ type Test struct {

 // Rollback holds the configuration for Helm rollback actions.
 type Rollback struct {
-	// Enable enables Helm rollback actions for this release after an
-	// Helm install or upgrade action failure.
-	// +optional
-	Enable bool `json:"enable,omitempty"`
-
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs
 	// for hooks) during the performance of a Helm rollback action. Defaults to
 	// 'HelmReleaseSpec.Timeout'.

Examples

Infinite installation retries, rollback on upgrade failure, ignore test errors:

spec:
  install:
    remediation:
      retries: -1
  upgrade:
    remediation:
      strategy: rollback
  test:
    enable: true
    remediation:
      strategy: ignore

Keep failed installation on last retry, rollback on upgrade failure, rollback on test errors:

spec:
  install:
    remediation:
      retries: 5
      doNotRemediateOnLastRetry: true
  upgrade:
    remediation:
      strategy: rollback
  test:
    enable: true
    remediation:
      strategy: rollback

Uninstall on test and upgrade failures:

spec:
  upgrade:
    remediation:
      strategy: uninstall
  test:
    enable: true
    remediation:
      strategy: uninstall

Notes

• When a remediation strategy is defined in the Test configuration, failures will result in a Ready==False condition, when no remediation strategy is defined Ready==True.
• If a rollback fails, the retry of the rollback will count as a retry of the action performed. This means that if the rollback issue is persistent and retries is not set to a negative integer, the controller will behave like DoNotRemediateOnLastRetry is set.
• Tests on an interval are not taken into account in this proposal but are discussed elsewhere. It would however fit into this design by adding an Interval (default nil) to the Test configuration, and by extending the Remediation struct for the Test to offer a flag to ignore test failures that happen on the interval.

[squaremo]

Given the division into Install/Upgrade/Test stanzas, this is an economic expression of rollback parameters (so easy for a user to encode what they want, with not too much fuss) 👍

• When a remediation strategy is defined in the Test configuration, failures will result in a Ready==False condition, when no remediation strategy is defined Ready==True.

That latter part seems counter-intuitive -- surely if tests are enabled, and fail, the release should not be considered ready?

[hiddeco]

That latter part seems counter-intuitive -- surely if tests are enabled, and fail, the release should not be considered ready?

The release is technically ready (as it has been successfully deployed), but the post-install or post-upgrade check failed. This is still reflected in the HelmReleaseStatus.Conditions with Test==False.

The reason for this is that there have been reports from users that they do want to be notified about the failure of the test, but do not see it as something that should mark the resource as failed (i.e. because the test that come with the charts are flakey).

Your comment is however valid as this no longer provides the user with the option to not remediate the failed release, but do mark it as a complete failure. Which I (somehow) did not take into account while drafting this spec.

An option would be to introduce an ignore strategy which would cause Ready to be True on test failures.

[stefanprodan]

If we take #103 into account, then a failed scheduled test will mark the release as not ready, even if at upgrade time the test passed. I think adding an ignore strategy is a must for people that want to use Helm test as an alerting mechanism with no impact to the cluster state.

[hiddeco]

I have updated the proposal to include the ignore strategy.

[seaneagan]

Thanks for taking up this discussion! I like the uniformity of the Remediation struct. However, I think some of the remediation fields and/or field values are only relevant to certain actions:

1. ignore use case seems specific to the test action, seems dangerous to ignore a failed install / upgrade. default remediation strategy for upgrade should then be rollback rather than ignore, defaulting Retries to 0 and DoNotRemediateOnLastRetry to true would accomplish the goal of no remediation by default.
2. rollback is an invalid remediation strategy for installs (nothing to rollback to), only uninstall is valid, so strategy field seems undesired for install.
3. I think tests should just inherit the remediation configuration of the last install / upgrade, the test failure simply indicates that the install / upgrade has failed. And the thing you want to retry is the whole install / upgrade / test cycle, not just the test, which can already include retries at the native helm level, since they are defined as kubernetes Jobs (Job.backoffLimit). For the case where the tests are not fully trusted, an ignore or remediateFailures (reverse polarity) boolean field could be added. I think when tests are enabled, by default they should not be ignored. I would even consider enabling tests by default, as if there are no tests helm considers this a test success, and if there are tests, why not run them? (there may very well be a good answer to this)
4. the only other use case for remediation strategy would be uninstalling upgrade failures, perhaps that isn't even needed, since one can use upgrade.force, the only thing I don't like about that is it also forces delete/re-create of individual resources when update fails, whereas ideally that would have it's own separate flag.

One other small thing, is the reason for the verbosity of doNotRemediateOnLastRetry vs just remediateLastRetry to have it be true/false by default? I think a more important consideration for naming of boolean fields is avoiding negatives e.g. "doNot" as this makes it harder to parse for the reader.

Updating the API sketch from before, these are the fields I see as definitely useful (ignoring #103 for now), with proposed defaults:

spec:
  install:
    retries: 0
    remediateLastRetry: false
  upgrade:
    retries: 0
    remediateLastRetry: false
  test:
    enable: true
    remediateFailures: true 

[seaneagan]

We ignore failed upgrades because we want to provide the safest defaults to users. Rolling back on e.g. charts that contain a StatefulSet is not always a safe operation depending on what is in the chart, this is why the behaviour is (/has a history of being) opt-in.

I agree with not rolling back by default, but I think a better way to accomplish this would be via the defaults of the Retries (0) and DoNotRemediateOnLastRetry (true).

Rolling back from revision 1 should result in an uninstall while keeping the history and makes it thus a valid action. See the logic in the helm/action pkg: https://github.com/helm/helm/blob/master/pkg/action/rollback.go#L141.

I get an error (Error: release: not found) when running a rollback on a release with a single revision when using the Helm 3 CLI.

I do not think this is necessarily true. Helm upgrades rely on the reflected state in the cluster (i.e. is the pod ready?) while a Helm test may fail because of almost any reason (as people can put whatever they want in their tests, and will likely abuse this). Due to this, I think fine grain control over what happens when the test fails is better than stripping your users from configuration options.

Yes, I totally agree that helm tests can be unreliable, especially if you're not in control of the chart. And so there needs to be a way to ignore such failures, and treat them as informational only. However, I don't see the utility in doing a rollback if a wait fails, but then an uninstall if an immediately following test fails, for example. The existing proposal here defines a single static remediation strategy for any failed test, with no input as to whether it was triggered by an install or upgrade, which seems like the main consideration of whether to do an uninstall or rollback.

I would even consider enabling tests by default, as if there are no tests helm considers this a test success, and if there are tests, why not run them?

I think most people want to opt-in because they know what is in their tests.

👍 that is easy enough to do

One other small thing, is the reason for the verbosity of doNotRemediateOnLastRetry vs just remediateLastRetry to have it be true/false by default? I think a more important consideration for naming of boolean fields is avoiding negatives e.g. "doNot" as this makes it harder to parse for the reader.

This derives from a discussion I had with @stefanprodan were he found the flag hard to interpreted, and what is proposed here was more clear to him. I have no strong opinion, but do think the "doNot" clarifies that it deviates from the default behaviour.

So the proposed default behavior is to remediate the last retry? I was thinking the opposite, due to for example the difficulty in rolling back stateful sets mentioned before. In the case where retries is 0 I interpret the "last retry" as the initial try. And for installs, it seems mostly harmless to leave the release in a failed state, and thus beneficial to make it available for debugging.

[seaneagan]

In the case where retries is 0 I interpret the "last retry" as the initial try

To clarify this, it may make sense to switch the terminology from "retries" to "tries" or equivalentally "attempts" which matches the "status.LastAttemptedRevision" terminology:

spec:
  install:
    attempts: 1 # set to 0 for infinite
    remediateLastAttempt: false
  upgrade:
    attempts: 1 # set to 0 for infinite
    remediateLastAttempt: false

[hiddeco]

So the proposed default behavior is to remediate the last retry? I was thinking the opposite, due to for example the difficulty in rolling back stateful sets mentioned before

The proposed default behaviour is to not remediate if it is not enabled, but to remediate the last retry if some remediation strategy is configured. This again to provide sane defaults, in which the release is left in the best condition we can leave it in after (multiple) failures.

I agree with not rolling back by default, but I think a better way to accomplish this would be via the defaults of the Retries (0) and DoNotRemediateOnLastRetry (true).

To clarify this, it may make sense to switch the terminology from "retries" to "tries" or equivalentally "attempts" which matches the "status.LastAttemptedRevision" terminology

This is not an option due to the above, as the DoNotRemediateOnLastRetry (true) / RemediateLastAttempt (false) default would cause the release to end up in a failed state as soon as retries would be set to a positive (some retries) or negative (unlimited retries) integer / attempts to 0 or a positive integer.

I get an error (Error: release: not found) when running a rollback on a release with a single revision when using the Helm 3 CLI.

Ack.

However, I don't see the utility in doing a rollback if a wait fails, but then an uninstall if an immediately following test fails, for example. The existing proposal here defines a single static remediation strategy for any failed test, with no input as to whether it was triggered by an install or upgrade, which seems like the main consideration of whether to do an uninstall or rollback.

This, and your observation about the rollback not working for a first revision means we will need two separate Remediation structs with both their own set of validation rules and/or options.

Install

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -96,6 +96,21 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+type InstallRemediation struct {
+	// Retries is the number of retries that should be attempted on failures before
+	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// +optional
+	Retries int `json:"retries,omitempty"`
+
+	// IgnoreTestFailures tells the controller to skip remediation when
+	// the Helm tests are run after an installation but fail.
+	// +optional
+	IgnoreTestFailures bool `json:"ignoreTestFailures,omitempty"`
+
+	// DoNotRemediateOnLastRetry tells the controller to skip remediation when
+	// there are no retries left so the release can be debugged.
+	// +optional
+	DoNotRemediateOnLastRetry bool `json:"doNotRemediateOnLastRetry,omitempty"`
+}

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
 // Install holds the configuration for Helm install actions.
 type Install struct {
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs
@@ -104,6 +119,12 @@ type Install struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

+	// Remediation holds the remediation configuration for when the
+	// Helm install action fails. The default install failure
+	// remediation is an uninstall action.
+	// +optional
+	Remediation InstallRemediation `json:"remediation,omitempty"`
+
 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm install has been performed.
 	// +optional

Upgrade

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -96,6 +96,21 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+type UpgradeRemediation struct {
+	// Retries is the number of retries that should be attempted on failures before
+	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// +optional
+	Retries int `json:"retries,omitempty"`
+
+	// Strategy to use for the remediation of a failed action.
+	// +kubebuilder:validation:Enum=rollback,uninstall
+	// +optional
+	Strategy string `json:"strategy,omitempty"`
+
+	// IgnoreTestFailures tells the controller to skip remediation when
+	// the Helm tests are run after an installation but fail.
+	// +optional
+	IgnoreTestFailures bool `json:"ignoreTestFailures,omitempty"`
+
+	// DoNotRemediateOnLastRetry tells the controller to skip remediation when
+	// there are no retries left so the release can be debugged.
+	// +optional
+	DoNotRemediateOnLastRetry bool `json:"doNotRemediateOnLastRetry,omitempty"`
+}

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -137,10 +158,11 @@ type Upgrade struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

-	// MaxRetries is the number of retries that should be attempted on failures before
-	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// Remediation holds the remediation configuration for when the
+	// Helm upgrade action for the HelmRelease fails. The default
+	// upgrade failure remediation is to not perform any action.
 	// +optional
-	MaxRetries int `json:"maxRetries,omitempty"`
+	Remediation UpgradeRemediation `json:"remediation,omitempty"`

 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm upgrade has been performed.

[seaneagan]

Proposal looks good! Some feedback:

1. Having to set/unset Install.Remediation.IgnoreTestFailures and Upgrade.Remediation.IgnoreTestFailures may be a bit tedious, perhaps could be moved to Test.IgnoreFailures or that could be used as the default for the other two. Not a big deal.
2. With the Upgrade.Remediation.Strategy value of uninstall, the retry will be an install, not sure how that affects the retry configs, just something to think about. I could also imagine a strategy of e.g. rollback_then_uninstall which tries a rollback first, then uninstalls, but that could always be added later.

[hiddeco]

Having to set/unset Install.Remediation.IgnoreTestFailures and Upgrade.Remediation.IgnoreTestFailures may be a bit tedious, perhaps could be moved to Test.IgnoreFailures or that could be used as the default for the other two. Not a big deal.

The latter is likely best to offer users an overwrite option.

With the Upgrade.Remediation.Strategy value of uninstall, the retry will be an install, not sure how that affects the retry configs, just something to think about.

Option: when it is set to uninstall, the failure counter should be reset on a successful uninstall, and if it continues to fail on install the install retry config takes over.

[hiddeco]

To combine the starter post and #102 (reply in thread), and to be able to select it as the answer.

Spec changes

Install

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -96,6 +96,21 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+type InstallRemediation struct {
+	// Retries is the number of retries that should be attempted on failures before
+	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// +optional
+	Retries int `json:"retries,omitempty"`
+
+	// IgnoreTestFailures tells the controller to skip remediation when
+	// the Helm tests are run after an install action but fail.
+	// Defaults to 'Test.IgnoreTestFailures'.
+	// +optional
+	IgnoreTestFailures bool `json:"ignoreTestFailures,omitempty"`
+
+	// SkipLastRemediation tells the controller to skip remediation when
+	// there are no retries left so the release can be debugged.
+	// +optional
+	SkipLastRemediation bool `json:"skipLastRemediation,omitempty"`
+}

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
 // Install holds the configuration for Helm install actions.
 type Install struct {
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs
@@ -104,6 +119,12 @@ type Install struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

+	// Remediation holds the remediation configuration for when the
+	// Helm install action fails. The default install failure
+	// remediation is an uninstall action.
+	// +optional
+	Remediation InstallRemediation `json:"remediation,omitempty"`
+
 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm install has been performed.
 	// +optional

Upgrade

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
--- a/docs/spec/v2alpha1/helmreleases.md
+++ b/docs/spec/v2alpha1/helmreleases.md
@@ -96,6 +96,21 @@ type HelmChartTemplate struct {
 	Interval *metav1.Duration `json:"interval,omitempty"`
 }
 
+type UpgradeRemediation struct {
+	// Retries is the number of retries that should be attempted on failures before
+	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// +optional
+	Retries int `json:"retries,omitempty"`
+
+	// Strategy to use for the remediation of a failed action.
+	// +kubebuilder:validation:Enum=rollback,uninstall
+	// +optional
+	Strategy string `json:"strategy,omitempty"`
+
+	// IgnoreTestFailures tells the controller to skip remediation when
+	// the Helm tests are run after an upgrade action but fail.
+	// Defaults to 'Test.IgnoreTestFailures'.
+	// +optional
+	IgnoreTestFailures bool `json:"ignoreTestFailures,omitempty"`
+
+	// SkipLastRemediation tells the controller to skip remediation when
+	// there are no retries left so the release can be debugged.
+	// +optional
+	SkipLastRemediation bool `json:"skipLastRemediation,omitempty"`
+}

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -137,10 +158,11 @@ type Upgrade struct {
 	// +optional
 	Timeout *metav1.Duration `json:"timeout,omitempty"`

-	// MaxRetries is the number of retries that should be attempted on failures before
-	// bailing. Defaults to '0', a negative integer equals to unlimited retries.
+	// Remediation holds the remediation configuration for when the
+	// Helm upgrade action for the HelmRelease fails. The default
+	// upgrade failure remediation is to not perform any action.
 	// +optional
-	MaxRetries int `json:"maxRetries,omitempty"`
+	Remediation UpgradeRemediation `json:"remediation,omitempty"`

 	// DisableWait disables the waiting for resources to be ready after a
 	// Helm upgrade has been performed.

Test

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -179,6 +201,12 @@ type Test struct {
 	// +optional
 	Enable bool `json:"enable,omitempty"`

+	// IgnoreTestFailures tells the controller to skip remediation when
+	// the Helm tests are run but fail.
+	// Can be overwritten for tests run after install or upgrade actions
+	// in 'Install.IgnoreTestFailures' and 'Upgrade.IgnoreTestFailures'.
+	// +optional
+	IgnoreTestFailures bool `json:"ignoreTestFailures,omitempty"`

 	// Timeout is the time to wait for any individual Kubernetes operation
 	// during the performance of a Helm test action. Defaults to
 	// 'HelmReleaseSpec.Timeout'.

Rollback

diff --git a/docs/spec/v2alpha1/helmreleases.md b/docs/spec/v2alpha1/helmreleases.md
index bf6010d..127c196 100644
@@ -188,11 +216,6 @@ type Test struct {

 // Rollback holds the configuration for Helm rollback actions.
 type Rollback struct {
-	// Enable enables Helm rollback actions for this release after an
-	// Helm install or upgrade action failure.
-	// +optional
-	Enable bool `json:"enable,omitempty"`
-
 	// Timeout is the time to wait for any individual Kubernetes operation (like Jobs
 	// for hooks) during the performance of a Helm rollback action. Defaults to
 	// 'HelmReleaseSpec.Timeout'.

Behavioral notes

• With the Upgrade.Remediation.Strategy value of uninstall, the retry will be an install. This means that after a successful uninstall, the install remediation configuration takes over.