From dbd4d775384ce914e13ac0806e67ebe346aeaab7 Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Thu, 26 Nov 2020 13:39:42 +0100
Subject: [PATCH 1/4] Refactor release reconciliation

Signed-off-by: Hidde Beydals <hello@hidde.co>
---
 api/v2beta1/helmrelease_types.go              |  46 +-
 .../helm.toolkit.fluxcd.io_helmreleases.yaml  |   6 +-
 controllers/helmrelease_controller.go         | 473 ++++--------------
 controllers/helmrelease_controller_chart.go   |  20 +-
 .../helmrelease_controller_chart_test.go      |   2 +-
 controllers/helmrelease_controller_release.go | 426 ++++++++++++++++
 ...=> helmrelease_controller_release_test.go} |  33 +-
 docs/api/helmrelease.md                       |  14 +-
 internal/runner/runner.go                     |  50 +-
 internal/storage/observer.go                  | 118 +++++
 internal/util/util.go                         |  71 +++
 11 files changed, 819 insertions(+), 440 deletions(-)
 create mode 100644 controllers/helmrelease_controller_release.go
 rename controllers/{helmrelease_controller_test.go => helmrelease_controller_release_test.go} (89%)
 create mode 100644 internal/storage/observer.go

diff --git a/api/v2beta1/helmrelease_types.go b/api/v2beta1/helmrelease_types.go
index 4e3533887..03ca604df 100644
--- a/api/v2beta1/helmrelease_types.go
+++ b/api/v2beta1/helmrelease_types.go
@@ -659,10 +659,14 @@ type HelmReleaseStatus struct {
 	// +optional
 	LastAttemptedValuesChecksum string `json:"lastAttemptedValuesChecksum,omitempty"`
 
-	// LastReleaseRevision is the revision of the last successful Helm release.
+	// LastReleaseRevision is the revision of the last performed Helm release.
 	// +optional
 	LastReleaseRevision int `json:"lastReleaseRevision,omitempty"`
 
+	// LastSuccessfulReleaseRevision is the revision of the last successful Helm release.
+	// +optional
+	LastSuccessfulReleaseRevision int `json:"lastSuccessfulReleaseRevision,omitempty"`
+
 	// HelmChart is the namespaced name of the HelmChart resource created by
 	// the controller for the HelmRelease.
 	// +optional
@@ -696,41 +700,39 @@ func (in HelmReleaseStatus) GetHelmChart() (string, string) {
 // HelmReleaseProgressing resets any failures and registers progress toward
 // reconciling the given HelmRelease by setting the meta.ReadyCondition to
 // 'Unknown' for meta.ProgressingReason.
-func HelmReleaseProgressing(hr HelmRelease) HelmRelease {
+func HelmReleaseProgressing(hr *HelmRelease) {
 	hr.Status.Conditions = []metav1.Condition{}
-	meta.SetResourceCondition(&hr, meta.ReadyCondition, metav1.ConditionUnknown, meta.ProgressingReason,
-		"Reconciliation in progress")
-	resetFailureCounts(&hr)
-	return hr
+	meta.SetResourceCondition(hr, meta.ReadyCondition, metav1.ConditionUnknown, meta.ProgressingReason,
+		"reconciliation in progress")
+	resetFailureCounts(hr)
 }
 
 // HelmReleaseNotReady registers a failed reconciliation of the given HelmRelease.
-func HelmReleaseNotReady(hr HelmRelease, reason, message string) HelmRelease {
-	meta.SetResourceCondition(&hr, meta.ReadyCondition, metav1.ConditionFalse, reason, message)
+func HelmReleaseNotReady(hr *HelmRelease, reason, message string) {
+	meta.SetResourceCondition(hr, meta.ReadyCondition, metav1.ConditionFalse, reason, message)
 	hr.Status.Failures++
-	return hr
 }
 
 // HelmReleaseReady registers a successful reconciliation of the given HelmRelease.
-func HelmReleaseReady(hr HelmRelease) HelmRelease {
-	meta.SetResourceCondition(&hr, meta.ReadyCondition, metav1.ConditionTrue, meta.ReconciliationSucceededReason,
-		"Release reconciliation succeeded")
+func HelmReleaseReady(hr *HelmRelease, message string) {
+	meta.SetResourceCondition(hr, meta.ReadyCondition, metav1.ConditionTrue, meta.ReconciliationSucceededReason, message)
 	hr.Status.LastAppliedRevision = hr.Status.LastAttemptedRevision
-	resetFailureCounts(&hr)
-	return hr
+	resetFailureCounts(hr)
 }
 
-// HelmReleaseAttempted registers an attempt of the given HelmRelease with the given state.
-// and returns the modified HelmRelease and a boolean indicating a state change.
-func HelmReleaseAttempted(hr HelmRelease, revision string, releaseRevision int, valuesChecksum string) (HelmRelease, bool) {
-	changed := hr.Status.LastAttemptedRevision != revision ||
-		hr.Status.LastReleaseRevision != releaseRevision ||
-		hr.Status.LastAttemptedValuesChecksum != valuesChecksum
+// HelmReleaseAttempt registers a release attempt of the given HelmRelease.
+func HelmReleaseAttempt(hr *HelmRelease, revision string, valuesChecksum string) {
 	hr.Status.LastAttemptedRevision = revision
-	hr.Status.LastReleaseRevision = releaseRevision
 	hr.Status.LastAttemptedValuesChecksum = valuesChecksum
+}
 
-	return hr, changed
+// StateChanged returns true if the given values differ from the values
+// in the HelmRelease.
+func StateChanged(hr *HelmRelease, revision string, releaseRevision int, valuesChecksum string) bool {
+	return hr.Status.LastAttemptedRevision != revision ||
+		hr.Status.LastReleaseRevision != releaseRevision ||
+		hr.Status.LastAttemptedValuesChecksum != valuesChecksum ||
+		hr.Status.ObservedGeneration != hr.Generation
 }
 
 func resetFailureCounts(hr *HelmRelease) {
diff --git a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml
index 0ef7ee33e..99a74babb 100644
--- a/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml
+++ b/config/crd/bases/helm.toolkit.fluxcd.io_helmreleases.yaml
@@ -527,9 +527,13 @@ spec:
                   reconcile request value, so a change can be detected.
                 type: string
               lastReleaseRevision:
-                description: LastReleaseRevision is the revision of the last successful
+                description: LastReleaseRevision is the revision of the last performed
                   Helm release.
                 type: integer
+              lastSuccessfulReleaseRevision:
+                description: LastSuccessfulReleaseRevision is the revision of the
+                  last successful Helm release.
+                type: integer
               observedGeneration:
                 description: ObservedGeneration is the last observed generation.
                 format: int64
diff --git a/controllers/helmrelease_controller.go b/controllers/helmrelease_controller.go
index 006a74f4f..6be2cd62b 100644
--- a/controllers/helmrelease_controller.go
+++ b/controllers/helmrelease_controller.go
@@ -24,16 +24,13 @@ import (
 	"time"
 
 	"github.com/go-logr/logr"
-	"helm.sh/helm/v3/pkg/chart"
-	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/storage/driver"
-	"helm.sh/helm/v3/pkg/strvals"
 	corev1 "k8s.io/api/core/v1"
-	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	apimeta "k8s.io/apimachinery/pkg/api/meta"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
 	"k8s.io/apimachinery/pkg/types"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 	"k8s.io/client-go/rest"
 	kuberecorder "k8s.io/client-go/tools/record"
@@ -78,6 +75,11 @@ type HelmReleaseReconciler struct {
 	MetricsRecorder       *metrics.Recorder
 }
 
+type HelmReleaseReconcilerOptions struct {
+	MaxConcurrentReconciles   int
+	DependencyRequeueInterval time.Duration
+}
+
 func (r *HelmReleaseReconciler) SetupWithManager(mgr ctrl.Manager, opts HelmReleaseReconcilerOptions) error {
 	// Index the HelmRelease by the HelmChart references they point at
 	if err := mgr.GetFieldIndexer().IndexField(context.TODO(), &v2.HelmRelease{}, v2.SourceIndexKey,
@@ -101,37 +103,31 @@ func (r *HelmReleaseReconciler) SetupWithManager(mgr ctrl.Manager, opts HelmRele
 		Complete(r)
 }
 
-// ConditionError represents an error with a status condition reason attached.
-type ConditionError struct {
-	Reason string
-	Err    error
-}
-
-func (c ConditionError) Error() string {
-	return c.Err.Error()
-}
-
-func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error) {
+func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (_ ctrl.Result, retErr error) {
 	ctx := context.Background()
 	start := time.Now()
 
-	var hr v2.HelmRelease
-	if err := r.Get(ctx, req.NamespacedName, &hr); err != nil {
+	hr := &v2.HelmRelease{}
+	if err := r.Get(ctx, req.NamespacedName, hr); err != nil {
 		return ctrl.Result{}, client.IgnoreNotFound(err)
 	}
 
 	log := r.Log.WithValues("controller", strings.ToLower(v2.HelmReleaseKind), "request", req.NamespacedName)
 
-	// Add our finalizer if it does not exist
-	if !controllerutil.ContainsFinalizer(&hr, v2.HelmReleaseFinalizer) {
-		controllerutil.AddFinalizer(&hr, v2.HelmReleaseFinalizer)
-		if err := r.Update(ctx, &hr); err != nil {
+	// Always record the readiness of the HelmRelease at the end of
+	// a reconciliation run.
+	defer r.recordReadiness(hr)
+
+	// Add our finalizer if it does not exist.
+	if !controllerutil.ContainsFinalizer(hr, v2.HelmReleaseFinalizer) {
+		controllerutil.AddFinalizer(hr, v2.HelmReleaseFinalizer)
+		if err := r.Update(ctx, hr); err != nil {
 			log.Error(err, "unable to register finalizer")
 			return ctrl.Result{}, err
 		}
 	}
 
-	// Examine if the object is under deletion
+	// Examine if the object is under deletion.
 	if !hr.ObjectMeta.DeletionTimestamp.IsZero() {
 		return r.reconcileDelete(ctx, log, hr)
 	}
@@ -142,262 +138,104 @@ func (r *HelmReleaseReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
 		return ctrl.Result{}, nil
 	}
 
-	hr, result, err := r.reconcile(ctx, log, hr)
-
-	// Update status after reconciliation.
-	if updateStatusErr := r.patchStatus(ctx, &hr); updateStatusErr != nil {
-		log.Error(updateStatusErr, "unable to update status after reconciliation")
-		return ctrl.Result{Requeue: true}, updateStatusErr
-	}
-
-	// Record ready status
-	r.recordReadiness(hr)
-
-	// Log reconciliation duration
-	durationMsg := fmt.Sprintf("reconcilation finished in %s", time.Now().Sub(start).String())
-	if result.RequeueAfter > 0 {
-		durationMsg = fmt.Sprintf("%s, next run in %s", durationMsg, result.RequeueAfter.String())
+	// Record reconciliation duration.
+	if r.MetricsRecorder != nil {
+		objRef, err := reference.GetReference(r.Scheme, hr)
+		if err != nil {
+			return ctrl.Result{Requeue: true}, err
+		}
+		defer r.MetricsRecorder.RecordDuration(*objRef, start)
 	}
-	log.Info(durationMsg)
 
-	return result, err
-}
-
-func (r *HelmReleaseReconciler) reconcile(ctx context.Context, log logr.Logger, hr v2.HelmRelease) (v2.HelmRelease, ctrl.Result, error) {
-	reconcileStart := time.Now()
-	// Record the value of the reconciliation request, if any
-	if v, ok := meta.ReconcileAnnotationValue(hr.GetAnnotations()); ok {
-		hr.Status.SetLastHandledReconcileRequest(v)
-	}
+	defer func() {
+		// Record the value of the reconciliation request, this can be used
+		// by consumers to observe their request has been handled.
+		if v, ok := meta.ReconcileAnnotationValue(hr.GetAnnotations()); ok {
+			hr.Status.LastHandledReconcileAt = v
+		}
 
-	// Observe HelmRelease generation.
-	if hr.Status.ObservedGeneration != hr.Generation {
+		// Record the observed generation.
 		hr.Status.ObservedGeneration = hr.Generation
-		hr = v2.HelmReleaseProgressing(hr)
-		if updateStatusErr := r.patchStatus(ctx, &hr); updateStatusErr != nil {
-			log.Error(updateStatusErr, "unable to update status after generation update")
-			return hr, ctrl.Result{Requeue: true}, updateStatusErr
-		}
-		// Record progressing status
-		r.recordReadiness(hr)
-	}
 
-	// Record reconciliation duration
-	if r.MetricsRecorder != nil {
-		objRef, err := reference.GetReference(r.Scheme, &hr)
-		if err != nil {
-			return hr, ctrl.Result{Requeue: true}, err
+		// Always attempt to patch the status after each reconciliation.
+		if err := r.patchStatus(ctx, hr); err != nil {
+			retErr = utilerrors.NewAggregate([]error{retErr, err})
 		}
-		defer r.MetricsRecorder.RecordDuration(*objRef, reconcileStart)
-	}
+	}()
 
-	// Reconcile chart based on the HelmChartTemplate
-	hc, reconcileErr := r.reconcileChart(ctx, &hr)
-	if reconcileErr != nil {
-		msg := fmt.Sprintf("chart reconciliation failed: %s", reconcileErr.Error())
+	// Reconcile the chart for this resource.
+	if result, err := r.reconcileChart(ctx, log, hr); err != nil {
+		msg := fmt.Sprintf("HelmChart reconciliation failed: %s", err.Error())
+		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, msg)
 		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, msg)
-		return v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg), ctrl.Result{Requeue: true}, reconcileErr
-	}
-
-	// Check chart readiness
-	if hc.Generation != hc.Status.ObservedGeneration || !apimeta.IsStatusConditionTrue(hc.Status.Conditions, meta.ReadyCondition) {
-		msg := fmt.Sprintf("HelmChart '%s/%s' is not ready", hc.GetNamespace(), hc.GetName())
-		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo, msg)
-		log.Info(msg)
-		// Do not requeue immediately, when the artifact is created
-		// the watcher should trigger a reconciliation.
-		return v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, msg), ctrl.Result{RequeueAfter: hc.Spec.Interval.Duration}, nil
-	}
-
-	// Check dependencies
-	if len(hr.Spec.DependsOn) > 0 {
-		if err := r.checkDependencies(hr); err != nil {
-			msg := fmt.Sprintf("dependencies do not meet ready condition (%s), retrying in %s",
-				err.Error(), r.requeueDependency.String())
-			r.event(hr, hc.GetArtifact().Revision, events.EventSeverityInfo, msg)
-			log.Info(msg)
-
-			// Exponential backoff would cause execution to be prolonged too much,
-			// instead we requeue on a fixed interval.
-			return v2.HelmReleaseNotReady(hr,
-				meta.DependencyNotReadyReason, err.Error()), ctrl.Result{RequeueAfter: r.requeueDependency}, nil
-		}
-		log.Info("all dependencies are ready, proceeding with release")
+		return result, err
 	}
 
-	// Compose values
-	values, err := r.composeValues(ctx, hr)
-	if err != nil {
-		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
-		return v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error()), ctrl.Result{Requeue: true}, nil
+	// Reconcile dependencies before proceeding.
+	if err := r.checkDependencies(hr); err != nil {
+		v2.HelmReleaseNotReady(hr, meta.DependencyNotReadyReason, err.Error())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo,
+			fmt.Sprintf("%s (retrying in %s)", err.Error(), r.requeueDependency.String()))
+		// We do not return the error here on purpose, as this would
+		// result in a back-off strategy.
+		return ctrl.Result{RequeueAfter: r.requeueDependency}, nil
 	}
 
-	// Load chart from artifact
-	chart, err := r.loadHelmChart(hc)
-	if err != nil {
-		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
-		return v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, err.Error()), ctrl.Result{Requeue: true}, nil
-	}
+	result, err := r.reconcile(ctx, log, hr)
 
-	// Reconcile Helm release
-	reconciledHr, reconcileErr := r.reconcileRelease(ctx, log, *hr.DeepCopy(), chart, values)
-	if reconcileErr != nil {
-		r.event(hr, hc.GetArtifact().Revision, events.EventSeverityError,
-			fmt.Sprintf("reconciliation failed: %s", reconcileErr.Error()))
+	// Log reconciliation duration
+	durationMsg := fmt.Sprintf("reconcilation finished in %s", time.Now().Sub(start).String())
+	if result.RequeueAfter > 0 {
+		durationMsg = fmt.Sprintf("%s (next run in %s)", durationMsg, result.RequeueAfter.String())
+	} else if result.Requeue {
+		durationMsg = fmt.Sprintf("%s (requeue %v)", durationMsg, result.Requeue)
 	}
-	return reconciledHr, ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, reconcileErr
-}
+	log.Info(durationMsg)
 
-type HelmReleaseReconcilerOptions struct {
-	MaxConcurrentReconciles   int
-	DependencyRequeueInterval time.Duration
+	return result, err
 }
 
-func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.Logger,
-	hr v2.HelmRelease, chart *chart.Chart, values chartutil.Values) (v2.HelmRelease, error) {
-	// Initialize Helm action runner
+func (r *HelmReleaseReconciler) reconcile(ctx context.Context, log logr.Logger, hr *v2.HelmRelease) (ctrl.Result, error) {
+	// Get the REST client for the Helm operations performed for this release.
 	getter, err := r.getRESTClientGetter(ctx, hr)
 	if err != nil {
-		return v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error()), err
+		v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error())
+		return ctrl.Result{}, err
 	}
-	run, err := runner.NewRunner(getter, hr.GetNamespace(), r.Log)
+
+	// Initialize the Helm action runner.
+	run, err := runner.NewRunner(getter, hr, log)
 	if err != nil {
-		return v2.HelmReleaseNotReady(hr, v2.InitFailedReason, "failed to initialize Helm action runner"), err
-	}
-
-	// Determine last release revision.
-	rel, observeLastReleaseErr := run.ObserveLastRelease(hr)
-	if observeLastReleaseErr != nil {
-		err = fmt.Errorf("failed to get last release revision: %w", observeLastReleaseErr)
-		return v2.HelmReleaseNotReady(hr, v2.GetLastReleaseFailedReason, "failed to get last release revision"), err
-	}
-
-	// Register the current release attempt.
-	revision := chart.Metadata.Version
-	releaseRevision := util.ReleaseRevision(rel)
-	valuesChecksum := util.ValuesChecksum(values)
-	hr, hasNewState := v2.HelmReleaseAttempted(hr, revision, releaseRevision, valuesChecksum)
-	if hasNewState {
-		hr = v2.HelmReleaseProgressing(hr)
-		if updateStatusErr := r.patchStatus(ctx, &hr); updateStatusErr != nil {
-			log.Error(updateStatusErr, "unable to update status after state update")
-			return hr, updateStatusErr
-		}
-		// Record progressing status
-		r.recordReadiness(hr)
+		v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error())
+		return ctrl.Result{}, err
 	}
 
-	// Check status of any previous release attempt.
-	released := apimeta.FindStatusCondition(hr.Status.Conditions, v2.ReleasedCondition)
-	if released != nil {
-		switch released.Status {
-		// Succeed if the previous release attempt succeeded.
-		case metav1.ConditionTrue:
-			return v2.HelmReleaseReady(hr), nil
-		case metav1.ConditionFalse:
-			// Fail if the previous release attempt remediation failed.
-			remediated := apimeta.FindStatusCondition(hr.Status.Conditions, v2.RemediatedCondition)
-			if remediated != nil && remediated.Status == metav1.ConditionFalse {
-				err = fmt.Errorf("previous release attempt remediation failed")
-				return v2.HelmReleaseNotReady(hr, remediated.Reason, remediated.Message), err
-			}
-		}
-
-		// Fail if install retries are exhausted.
-		if hr.Spec.GetInstall().GetRemediation().RetriesExhausted(hr) {
-			err = fmt.Errorf("install retries exhausted")
-			return v2.HelmReleaseNotReady(hr, released.Reason, released.Message), err
-		}
-
-		// Fail if there is a release and upgrade retries are exhausted.
-		// This avoids failing after an upgrade uninstall remediation strategy.
-		if rel != nil && hr.Spec.GetUpgrade().GetRemediation().RetriesExhausted(hr) {
-			err = fmt.Errorf("upgrade retries exhausted")
-			return v2.HelmReleaseNotReady(hr, released.Reason, released.Message), err
-		}
+	// Step through the release phases.
+	steps := []func(context.Context, logr.Logger, *runner.Runner, *v2.HelmRelease) (ctrl.Result, error){
+		r.reconcileRelease,
+		r.reconcileTest,
+		r.reconcileRemediation,
 	}
-
-	// Deploy the release.
-	var deployAction v2.DeploymentAction
-	if rel == nil {
-		deployAction = hr.Spec.GetInstall()
-		rel, err = run.Install(hr, chart, values)
-		err = r.handleHelmActionResult(&hr, revision, err, deployAction.GetDescription(), v2.ReleasedCondition, v2.InstallSucceededReason, v2.InstallFailedReason)
-	} else {
-		deployAction = hr.Spec.GetUpgrade()
-		rel, err = run.Upgrade(hr, chart, values)
-		err = r.handleHelmActionResult(&hr, revision, err, deployAction.GetDescription(), v2.ReleasedCondition, v2.UpgradeSucceededReason, v2.UpgradeFailedReason)
-	}
-	remediation := deployAction.GetRemediation()
-
-	// If there is a new release revision...
-	if util.ReleaseRevision(rel) > releaseRevision {
-		// Ensure release is not marked remediated.
-		apimeta.RemoveStatusCondition(&hr.Status.Conditions, v2.RemediatedCondition)
-
-		// If new release revision is successful and tests are enabled, run them.
-		if err == nil && hr.Spec.GetTest().Enable {
-			_, testErr := run.Test(hr)
-			testErr = r.handleHelmActionResult(&hr, revision, testErr, "test", v2.TestSuccessCondition, v2.TestSucceededReason, v2.TestFailedReason)
-
-			// Propagate any test error if not marked ignored.
-			if testErr != nil && !remediation.MustIgnoreTestFailures(hr.Spec.GetTest().IgnoreFailures) {
-				testsPassing := apimeta.FindStatusCondition(hr.Status.Conditions, v2.TestSuccessCondition)
-				meta.SetResourceCondition(&hr, v2.ReleasedCondition, metav1.ConditionFalse, testsPassing.Reason, testsPassing.Message)
-				err = testErr
-			}
+	result := ctrl.Result{}
+	var errs []error
+	for _, step := range steps {
+		stepResult, err := step(ctx, log, run, hr)
+		if err != nil {
+			errs = append(errs, err)
 		}
-	}
-
-	if err != nil {
-		// Increment failure count for deployment action.
-		remediation.IncrementFailureCount(&hr)
-		// Remediate deployment failure if necessary.
-		if !remediation.RetriesExhausted(hr) || remediation.MustRemediateLastFailure() {
-			if util.ReleaseRevision(rel) <= releaseRevision {
-				log.Info(fmt.Sprintf("skipping remediation, no new release revision created"))
-			} else {
-				var remediationErr error
-				switch remediation.GetStrategy() {
-				case v2.RollbackRemediationStrategy:
-					rollbackErr := run.Rollback(hr)
-					remediationErr = r.handleHelmActionResult(&hr, revision, rollbackErr, "rollback",
-						v2.RemediatedCondition, v2.RollbackSucceededReason, v2.RollbackFailedReason)
-				case v2.UninstallRemediationStrategy:
-					uninstallErr := run.Uninstall(hr)
-					remediationErr = r.handleHelmActionResult(&hr, revision, uninstallErr, "uninstall",
-						v2.RemediatedCondition, v2.UninstallSucceededReason, v2.UninstallFailedReason)
-				}
-				if remediationErr != nil {
-					err = remediationErr
-				}
-			}
-
-			// Determine release after remediation.
-			rel, observeLastReleaseErr = run.ObserveLastRelease(hr)
-			if observeLastReleaseErr != nil {
-				err = &ConditionError{
-					Reason: v2.GetLastReleaseFailedReason,
-					Err:    errors.New("failed to get last release revision after remediation"),
-				}
-			}
+		r.recordReadiness(hr)
+		if err := r.patchStatus(ctx, hr); err != nil {
+			log.Error(err, "failed to patch status sub-resource")
 		}
-	}
-
-	hr.Status.LastReleaseRevision = util.ReleaseRevision(rel)
-
-	if err != nil {
-		reason := meta.ReconciliationFailedReason
-		var cerr *ConditionError
-		if errors.As(err, &cerr) {
-			reason = cerr.Reason
+		if len(errs) > 0 {
+			continue
 		}
-		return v2.HelmReleaseNotReady(hr, reason, err.Error()), err
+		result = util.LowestNonZeroResult(result, stepResult)
 	}
-	return v2.HelmReleaseReady(hr), nil
+	return result, utilerrors.NewAggregate(errs)
 }
 
-func (r *HelmReleaseReconciler) checkDependencies(hr v2.HelmRelease) error {
+func (r *HelmReleaseReconciler) checkDependencies(hr *v2.HelmRelease) error {
 	for _, d := range hr.Spec.DependsOn {
 		if d.Namespace == "" {
 			d.Namespace = hr.GetNamespace()
@@ -420,7 +258,7 @@ func (r *HelmReleaseReconciler) checkDependencies(hr v2.HelmRelease) error {
 	return nil
 }
 
-func (r *HelmReleaseReconciler) getRESTClientGetter(ctx context.Context, hr v2.HelmRelease) (genericclioptions.RESTClientGetter, error) {
+func (r *HelmReleaseReconciler) getRESTClientGetter(ctx context.Context, hr *v2.HelmRelease) (genericclioptions.RESTClientGetter, error) {
 	if hr.Spec.KubeConfig == nil {
 		// impersonate service account if specified
 		if hr.Spec.ServiceAccountName != "" {
@@ -451,7 +289,7 @@ func (r *HelmReleaseReconciler) getRESTClientGetter(ctx context.Context, hr v2.H
 	return kube.NewMemoryRESTClientGetter(kubeConfig, hr.GetReleaseNamespace()), nil
 }
 
-func (r *HelmReleaseReconciler) getServiceAccountToken(ctx context.Context, hr v2.HelmRelease) (string, error) {
+func (r *HelmReleaseReconciler) getServiceAccountToken(ctx context.Context, hr *v2.HelmRelease) (string, error) {
 	namespacedName := types.NamespacedName{
 		Namespace: hr.Namespace,
 		Name:      hr.Spec.ServiceAccountName,
@@ -491,114 +329,13 @@ func (r *HelmReleaseReconciler) getServiceAccountToken(ctx context.Context, hr v
 	return token, nil
 }
 
-// composeValues attempts to resolve all v2beta1.ValuesReference resources
-// and merges them as defined. Referenced resources are only retrieved once
-// to ensure a single version is taken into account during the merge.
-func (r *HelmReleaseReconciler) composeValues(ctx context.Context, hr v2.HelmRelease) (chartutil.Values, error) {
-	result := chartutil.Values{}
-
-	configMaps := make(map[string]*corev1.ConfigMap)
-	secrets := make(map[string]*corev1.Secret)
-
-	for _, v := range hr.Spec.ValuesFrom {
-		namespacedName := types.NamespacedName{Namespace: hr.Namespace, Name: v.Name}
-		var valuesData []byte
-
-		switch v.Kind {
-		case "ConfigMap":
-			resource, ok := configMaps[namespacedName.String()]
-			if !ok {
-				// The resource may not exist, but we want to act on a single version
-				// of the resource in case the values reference is marked as optional.
-				configMaps[namespacedName.String()] = nil
-
-				resource = &corev1.ConfigMap{}
-				if err := r.Get(ctx, namespacedName, resource); err != nil {
-					if apierrors.IsNotFound(err) {
-						if v.Optional {
-							r.Log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
-							continue
-						}
-						return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
-					}
-					return nil, err
-				}
-				configMaps[namespacedName.String()] = resource
-			}
-			if resource == nil {
-				if v.Optional {
-					r.Log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
-					continue
-				}
-				return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
-			}
-			if data, ok := resource.Data[v.GetValuesKey()]; !ok {
-				return nil, fmt.Errorf("missing key '%s' in %s '%s'", v.GetValuesKey(), v.Kind, namespacedName)
-			} else {
-				valuesData = []byte(data)
-			}
-		case "Secret":
-			resource, ok := secrets[namespacedName.String()]
-			if !ok {
-				// The resource may not exist, but we want to act on a single version
-				// of the resource in case the values reference is marked as optional.
-				secrets[namespacedName.String()] = nil
-
-				resource = &corev1.Secret{}
-				if err := r.Get(ctx, namespacedName, resource); err != nil {
-					if apierrors.IsNotFound(err) {
-						if v.Optional {
-							r.Log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
-							continue
-						}
-						return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
-					}
-					return nil, err
-				}
-				secrets[namespacedName.String()] = resource
-			}
-			if resource == nil {
-				if v.Optional {
-					r.Log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
-					continue
-				}
-				return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
-			}
-			if data, ok := resource.Data[v.GetValuesKey()]; !ok {
-				return nil, fmt.Errorf("missing key '%s' in %s '%s'", v.GetValuesKey(), v.Kind, namespacedName)
-			} else {
-				valuesData = data
-			}
-		default:
-			return nil, fmt.Errorf("unsupported ValuesReference kind '%s'", v.Kind)
-		}
-		switch v.TargetPath {
-		case "":
-			values, err := chartutil.ReadValues(valuesData)
-			if err != nil {
-				return nil, fmt.Errorf("unable to read values from key '%s' in %s '%s': %w", v.GetValuesKey(), v.Kind, namespacedName, err)
-			}
-			result = util.MergeMaps(result, values)
-		default:
-			// TODO(hidde): this is a bit of hack, as it mimics the way the option string is passed
-			// 	to Helm from a CLI perspective. Given the parser is however not publicly accessible
-			// 	while it contains all logic around parsing the target path, it is a fair trade-off.
-			singleValue := v.TargetPath + "=" + string(valuesData)
-			if err := strvals.ParseInto(singleValue, result); err != nil {
-				return nil, fmt.Errorf("unable to merge value from key '%s' in %s '%s' into target path '%s': %w", v.GetValuesKey(), v.Kind, namespacedName, v.TargetPath, err)
-			}
-		}
-	}
-	return util.MergeMaps(result, hr.GetValues()), nil
-}
-
 // reconcileDelete deletes the v1beta1.HelmChart of the v2beta1.HelmRelease,
 // and uninstalls the Helm release if the resource has not been suspended.
-func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, logger logr.Logger, hr v2.HelmRelease) (ctrl.Result, error) {
+func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, logger logr.Logger, hr *v2.HelmRelease) (ctrl.Result, error) {
 	r.recordReadiness(hr)
 
 	// Delete the HelmChart that belongs to this resource.
-	if err := r.deleteHelmChart(ctx, &hr); err != nil {
+	if err := r.deleteHelmChart(ctx, hr); err != nil {
 		return ctrl.Result{}, err
 	}
 
@@ -608,7 +345,7 @@ func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, logger logr
 		if err != nil {
 			return ctrl.Result{}, err
 		}
-		run, err := runner.NewRunner(getter, hr.GetNamespace(), logger)
+		run, err := runner.NewRunner(getter, hr, logger)
 		if err != nil {
 			return ctrl.Result{}, err
 		}
@@ -622,28 +359,14 @@ func (r *HelmReleaseReconciler) reconcileDelete(ctx context.Context, logger logr
 	}
 
 	// Remove our finalizer from the list and update it.
-	controllerutil.RemoveFinalizer(&hr, v2.HelmReleaseFinalizer)
-	if err := r.Update(ctx, &hr); err != nil {
+	controllerutil.RemoveFinalizer(hr, v2.HelmReleaseFinalizer)
+	if err := r.Update(ctx, hr); err != nil {
 		return ctrl.Result{}, err
 	}
 
 	return ctrl.Result{}, nil
 }
 
-func (r *HelmReleaseReconciler) handleHelmActionResult(hr *v2.HelmRelease, revision string, err error, action string, condition string, succeededReason string, failedReason string) error {
-	if err != nil {
-		msg := fmt.Sprintf("Helm %s failed: %s", action, err.Error())
-		meta.SetResourceCondition(hr, condition, metav1.ConditionFalse, failedReason, msg)
-		r.event(*hr, revision, events.EventSeverityError, msg)
-		return &ConditionError{Reason: failedReason, Err: errors.New(msg)}
-	} else {
-		msg := fmt.Sprintf("Helm %s succeeded", action)
-		meta.SetResourceCondition(hr, condition, metav1.ConditionTrue, succeededReason, msg)
-		r.event(*hr, revision, events.EventSeverityInfo, msg)
-		return nil
-	}
-}
-
 func (r *HelmReleaseReconciler) patchStatus(ctx context.Context, hr *v2.HelmRelease) error {
 	key, err := client.ObjectKeyFromObject(hr)
 	if err != nil {
@@ -692,9 +415,9 @@ func (r *HelmReleaseReconciler) requestsForHelmChartChange(obj handler.MapObject
 }
 
 // event emits a Kubernetes event and forwards the event to notification controller if configured.
-func (r *HelmReleaseReconciler) event(hr v2.HelmRelease, revision, severity, msg string) {
-	r.EventRecorder.Event(&hr, "Normal", severity, msg)
-	objRef, err := reference.GetReference(r.Scheme, &hr)
+func (r *HelmReleaseReconciler) event(hr *v2.HelmRelease, revision, severity, msg string) {
+	r.EventRecorder.Event(hr, "Normal", severity, msg)
+	objRef, err := reference.GetReference(r.Scheme, hr)
 	if err != nil {
 		r.Log.WithValues(
 			"request",
@@ -718,12 +441,12 @@ func (r *HelmReleaseReconciler) event(hr v2.HelmRelease, revision, severity, msg
 	}
 }
 
-func (r *HelmReleaseReconciler) recordReadiness(hr v2.HelmRelease) {
+func (r *HelmReleaseReconciler) recordReadiness(hr *v2.HelmRelease) {
 	if r.MetricsRecorder == nil {
 		return
 	}
 
-	objRef, err := reference.GetReference(r.Scheme, &hr)
+	objRef, err := reference.GetReference(r.Scheme, hr)
 	if err != nil {
 		r.Log.WithValues(
 			strings.ToLower(hr.Kind),
diff --git a/controllers/helmrelease_controller_chart.go b/controllers/helmrelease_controller_chart.go
index c2fa42094..8db77516e 100644
--- a/controllers/helmrelease_controller_chart.go
+++ b/controllers/helmrelease_controller_chart.go
@@ -25,18 +25,20 @@ import (
 	"os"
 	"strings"
 
+	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chart/loader"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
 
 	sourcev1 "github.com/fluxcd/source-controller/api/v1beta1"
 
 	v2 "github.com/fluxcd/helm-controller/api/v2beta1"
 )
 
-func (r *HelmReleaseReconciler) reconcileChart(ctx context.Context, hr *v2.HelmRelease) (*sourcev1.HelmChart, error) {
+func (r *HelmReleaseReconciler) reconcileChart(ctx context.Context, log logr.Logger, hr *v2.HelmRelease) (ctrl.Result, error) {
 	chartName := types.NamespacedName{
 		Namespace: hr.Spec.Chart.GetNamespace(hr.Namespace),
 		Name:      hr.GetHelmChartName(),
@@ -45,7 +47,7 @@ func (r *HelmReleaseReconciler) reconcileChart(ctx context.Context, hr *v2.HelmR
 	// Garbage collect the previous HelmChart if the namespace named changed.
 	if hr.Status.HelmChart != "" && hr.Status.HelmChart != chartName.String() {
 		if err := r.deleteHelmChart(ctx, hr); err != nil {
-			return nil, err
+			return ctrl.Result{}, err
 		}
 	}
 
@@ -53,25 +55,23 @@ func (r *HelmReleaseReconciler) reconcileChart(ctx context.Context, hr *v2.HelmR
 	var helmChart sourcev1.HelmChart
 	err := r.Client.Get(ctx, chartName, &helmChart)
 	if err != nil && !apierrors.IsNotFound(err) {
-		return nil, err
+		return ctrl.Result{}, err
 	}
 	hc := buildHelmChartFromTemplate(hr)
 	switch {
 	case apierrors.IsNotFound(err):
 		if err = r.Client.Create(ctx, hc); err != nil {
-			return nil, err
+			return ctrl.Result{}, err
 		}
-		hr.Status.HelmChart = chartName.String()
-		return hc, nil
 	case helmChartRequiresUpdate(hr, &helmChart):
-		r.Log.Info("chart diverged from template", strings.ToLower(sourcev1.HelmChartKind), chartName.String())
+		log.Info("chart diverged from template", strings.ToLower(sourcev1.HelmChartKind), chartName.String())
 		helmChart.Spec = hc.Spec
 		if err = r.Client.Update(ctx, &helmChart); err != nil {
-			return nil, err
+			return ctrl.Result{}, err
 		}
-		hr.Status.HelmChart = chartName.String()
 	}
-	return &helmChart, nil
+	hr.Status.HelmChart = chartName.String()
+	return ctrl.Result{}, nil
 }
 
 // getHelmChart retrieves the v1beta1.HelmChart for the given
diff --git a/controllers/helmrelease_controller_chart_test.go b/controllers/helmrelease_controller_chart_test.go
index 5db272906..a13799c04 100644
--- a/controllers/helmrelease_controller_chart_test.go
+++ b/controllers/helmrelease_controller_chart_test.go
@@ -161,7 +161,7 @@ func TestHelmReleaseReconciler_reconcileChart(t *testing.T) {
 				Log:    log.NullLogger{},
 			}
 
-			hc, err := r.reconcileChart(context.TODO(), tt.hr)
+			hc, err := r.reconcileChart(context.TODO(), log.NullLogger{}, tt.hr)
 			if tt.expectErr {
 				g.Expect(err).To(HaveOccurred())
 				g.Expect(hc).To(BeNil())
diff --git a/controllers/helmrelease_controller_release.go b/controllers/helmrelease_controller_release.go
new file mode 100644
index 000000000..2887e8a8d
--- /dev/null
+++ b/controllers/helmrelease_controller_release.go
@@ -0,0 +1,426 @@
+/*
+Copyright 2020 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package controllers
+
+import (
+	"context"
+	"fmt"
+
+	"github.com/fluxcd/pkg/apis/meta"
+	"github.com/fluxcd/pkg/runtime/events"
+	"github.com/go-logr/logr"
+	"helm.sh/helm/v3/pkg/chartutil"
+	"helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/strvals"
+	corev1 "k8s.io/api/core/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	apimeta "k8s.io/apimachinery/pkg/api/meta"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/types"
+	ctrl "sigs.k8s.io/controller-runtime"
+
+	v2 "github.com/fluxcd/helm-controller/api/v2beta1"
+	"github.com/fluxcd/helm-controller/internal/runner"
+	"github.com/fluxcd/helm-controller/internal/util"
+)
+
+func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.Logger, run *runner.Runner, hr *v2.HelmRelease) (ctrl.Result, error) {
+	// Given we are in control over creating the chart, we should
+	// return an error as we either need to reconcile again to recreate
+	// it, or reattempt because we encountered a transient error.
+	hc, err := r.getHelmChart(ctx, hr)
+	if err != nil {
+		err = fmt.Errorf("failed to get HelmChart for resource: %w", err)
+		v2.HelmReleaseNotReady(hr, v2.GetLastReleaseFailedReason, err.Error())
+		return ctrl.Result{}, err
+	}
+
+	// Check if the chart has an artifact. If there is no artifact,
+	// record the fact that we observed this but do not return an error.
+	// Our watcher should observe the chart becoming ready, and queue
+	// a new reconciliation.
+	if hc.GetArtifact() == nil {
+		msg := fmt.Sprintf("HelmChart '%s/%s' has no artifact", hc.GetNamespace(), hc.GetName())
+		v2.HelmReleaseNotReady(hr, meta.DependencyNotReadyReason, msg)
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	// Compose the values. As race conditions may happen due to
+	// unordered applies (due to e.g. a HelmRelease with a reference
+	// to a Secret is applied before the Secret itself), we expect
+	// most errors to be transient and return them.
+	values, err := r.composeValues(ctx, log, hr)
+	if err != nil {
+		v2.HelmReleaseNotReady(hr, v2.InitFailedReason, err.Error())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+		return ctrl.Result{}, err
+	}
+
+	// Observe the last release. If this fails, we likely encountered a
+	// transient error and should return it to requeue a reconciliation.
+	rls, err := run.ObserveLastRelease(hr)
+	if err != nil {
+		msg := "failed to determine last deployed Helm release revision"
+		v2.HelmReleaseNotReady(hr, v2.GetLastReleaseFailedReason, msg)
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, msg)
+		return ctrl.Result{}, err
+	}
+
+	// Determine the release strategy based on the existence of a
+	// release in the Helm storage.
+	makeRelease, remediation := run.Install, hr.Spec.GetInstall().GetRemediation()
+	successReason, failureReason := v2.InstallSucceededReason, v2.InstallFailedReason
+	if rls != nil {
+		makeRelease, remediation = run.Upgrade, hr.Spec.GetUpgrade().GetRemediation()
+		successReason, failureReason = v2.UpgradeSucceededReason, v2.UpgradeFailedReason
+	}
+
+	// Previous release attempt resulted in a locked pending state,
+	// attempt to remediate.
+	releaseRevision := util.ReleaseRevision(rls)
+	if rls != nil && hr.Status.LastReleaseRevision == releaseRevision && rls.Info.Status.IsPending() {
+		msg := fmt.Sprintf("previous release did not finish (%s)", rls.Info.Status)
+		v2.HelmReleaseNotReady(hr, v2.RemediatedCondition, msg)
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, msg)
+		remediation.IncrementFailureCount(hr)
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	valuesChecksum := util.ValuesChecksum(values)
+
+	// We must have gathered all information to decide if we actually
+	// need to move forward with the release at this point.
+	switch {
+	// Determine if there are any state changes to things we depend on
+	// (chart revision, values checksum), or if the revision of the
+	// release does not match what we have run ourselves.
+	case v2.StateChanged(hr, hc.GetArtifact().Revision, releaseRevision, valuesChecksum):
+		v2.HelmReleaseProgressing(hr)
+		r.patchStatus(ctx, hr)
+	// The state has not changed and the release is not in a failed state.
+	case rls != nil && remediation.GetFailureCount(*hr) == 0:
+		v2.HelmReleaseReady(hr, rls.Info.Description)
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	// We have exhausted our retries.
+	case remediation.RetriesExhausted(*hr):
+		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, "exhausted release retries")
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	// Our previous reconciliation attempt failed, skip release to retry.
+	case hr.Status.LastSuccessfulReleaseRevision > 0 && hr.Status.LastReleaseRevision != hr.Status.LastSuccessfulReleaseRevision:
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	// Attempt to download and load the chart. As we already checked
+	// if an artifact is advertised in the chart object, any error
+	// we encounter is expected to be transient.
+	// This must always be done right before making the actual release,
+	// as depending on the chart this can be a (memory) expensive
+	// operation.
+	loadedChart, err := r.loadHelmChart(hc)
+	if err != nil {
+		v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, err.Error())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+		return ctrl.Result{}, err
+	}
+
+	// Register our new release attempt and make the release.
+	v2.HelmReleaseAttempt(hr, hc.GetArtifact().Revision, valuesChecksum)
+	err = makeRelease(*hr, loadedChart, values)
+
+	// Always record the revision when a new release has been made.
+	hr.Status.LastReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+
+	if err != nil {
+		// Record the release failure and increment the failure count.
+		meta.SetResourceCondition(hr, v2.ReleasedCondition, metav1.ConditionFalse, failureReason, err.Error())
+		v2.HelmReleaseNotReady(hr, failureReason, err.Error())
+		remediation.IncrementFailureCount(hr)
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+
+		// We should still requeue on the configured interval so that
+		// both the Helm storage and ValuesFrom references are observed
+		// on an interval.
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	meta.SetResourceCondition(hr, v2.ReleasedCondition, metav1.ConditionTrue, successReason,
+		run.GetLastPersistedRelease().Info.Description)
+	r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo, run.GetLastPersistedRelease().Info.Description)
+
+	return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+}
+
+func (r *HelmReleaseReconciler) reconcileTest(ctx context.Context, log logr.Logger, run *runner.Runner, hr *v2.HelmRelease) (ctrl.Result, error) {
+	// If this release was already marked as successful,
+	// we have nothing to do.
+	if hr.Status.LastReleaseRevision == hr.Status.LastSuccessfulReleaseRevision {
+		return ctrl.Result{}, nil
+	}
+
+	// Confirm the last release in storage equals to the release
+	// we made ourselves.
+	if revision := util.ReleaseRevision(run.GetLastPersistedRelease()); revision != hr.Status.LastReleaseRevision {
+		err := fmt.Errorf("unexpected revision change from %q to %q", hr.Status.LastReleaseRevision, revision)
+		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, err.Error())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+		return ctrl.Result{}, err
+	}
+
+	// If the release is not in a deployed state, we should not run the
+	// tests.
+	if run.GetLastPersistedRelease() == nil || run.GetLastPersistedRelease().Info.Status != release.StatusDeployed {
+		return ctrl.Result{}, nil
+	}
+
+	// If tests are not enabled for this resource, we have nothing to do.
+	if !hr.Spec.GetTest().Enable {
+		hr.Status.LastSuccessfulReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+		apimeta.RemoveStatusCondition(hr.GetStatusConditions(), v2.TestSuccessCondition)
+		v2.HelmReleaseReady(hr, run.GetLastPersistedRelease().Info.Description)
+		return ctrl.Result{}, nil
+	}
+
+	// Test suite was already run for this release.
+	if util.HasRunTestSuite(run.GetLastPersistedRelease()) {
+		return ctrl.Result{}, nil
+	}
+
+	// If the release does not have a test suite, we are successful.
+	if !util.HasTestSuite(run.GetLastPersistedRelease()) {
+		msg := "No test suite"
+		meta.SetResourceCondition(hr, v2.TestSuccessCondition, metav1.ConditionTrue, v2.TestSucceededReason, msg)
+		v2.HelmReleaseReady(hr, msg)
+		hr.Status.LastSuccessfulReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo, msg)
+		return ctrl.Result{}, nil
+	}
+
+	// Remove any previous test condition and run the tests.
+	apimeta.RemoveStatusCondition(hr.GetStatusConditions(), v2.TestSuccessCondition)
+	err := run.Test(*hr)
+
+	// We can not target the revision to test, and it is possible that
+	// the Helm storage state changed while we were doing other things.
+	// Confirm the tests run against our last recorded release revision.
+	if revision := util.ReleaseRevision(run.GetLastPersistedRelease()); revision != hr.Status.LastReleaseRevision {
+		err := fmt.Errorf("unexpected revision change from %q to %q during test run", hr.Status, revision)
+		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, err.Error())
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+		return ctrl.Result{}, err
+	}
+
+	// Calculate the test results.
+	successful, failed := util.CalculateTestSuiteResult(run.GetLastPersistedRelease())
+
+	if err != nil {
+		msg := fmt.Sprintf("Helm test suite failed (success: %v, failed: %v)", successful, failed)
+		meta.SetResourceCondition(hr, v2.TestSuccessCondition, metav1.ConditionFalse, v2.TestFailedReason, msg)
+		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, msg)
+
+		// Determine the remediation strategy that applies, and if we
+		// need to increment the failure count.
+		remediation := hr.Spec.GetInstall().GetRemediation()
+		if hr.Status.LastSuccessfulReleaseRevision > 0 {
+			remediation = hr.Spec.GetUpgrade().GetRemediation()
+		}
+
+		// Return if me must ignore the test results for the readiness of
+		// the resource.
+		if remediation.MustIgnoreTestFailures(hr.Spec.GetTest().IgnoreFailures) {
+			hr.Status.LastSuccessfulReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+			v2.HelmReleaseReady(hr, run.GetLastPersistedRelease().Info.Description)
+			return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+		}
+
+		remediation.IncrementFailureCount(hr)
+		v2.HelmReleaseNotReady(hr, v2.TestFailedReason, msg)
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	// Record the test success and mark the release as ready.
+	msg := fmt.Sprintf("Helm test suite succeeded (success: %v)", successful)
+	meta.SetResourceCondition(hr, v2.TestSuccessCondition, metav1.ConditionTrue, v2.TestSucceededReason, msg)
+	v2.HelmReleaseReady(hr, msg)
+	hr.Status.LastSuccessfulReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+	r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo, msg)
+
+	return ctrl.Result{}, nil
+}
+
+func (r *HelmReleaseReconciler) reconcileRemediation(ctx context.Context, log logr.Logger, run *runner.Runner, hr *v2.HelmRelease) (ctrl.Result, error) {
+	// Last release was successful, nothing to remediate.
+	if hr.Status.LastReleaseRevision == hr.Status.LastSuccessfulReleaseRevision {
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	// Determine the remediation strategy that applies.
+	remediation := hr.Spec.GetInstall().GetRemediation()
+	if run.GetLastPersistedRelease() != nil && remediation.GetFailureCount(*hr) == 0 {
+		remediation = hr.Spec.GetUpgrade().GetRemediation()
+	}
+
+	// Return if there are no retries left, or if we should not remediate
+	// the last failure.
+	if remediation.RetriesExhausted(*hr) && !remediation.MustRemediateLastFailure() {
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
+	}
+
+	// Perform the remediation action for the configured strategy.
+	switch remediation.GetStrategy() {
+	case v2.RollbackRemediationStrategy:
+		err := run.Rollback(*hr)
+		hr.Status.LastReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+		if err != nil {
+			// Record observed upgrade remediation failure
+			meta.SetResourceCondition(hr, v2.RemediatedCondition, metav1.ConditionFalse, v2.RollbackFailedReason, err.Error())
+			v2.HelmReleaseNotReady(hr, v2.RollbackFailedReason, err.Error())
+			r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+			return ctrl.Result{Requeue: true}, nil
+		}
+
+		// Record observed rollback remediation success.
+		meta.SetResourceCondition(hr, v2.RemediatedCondition, metav1.ConditionTrue, v2.RollbackSucceededReason,
+			run.GetLastPersistedRelease().Info.Description)
+		hr.Status.LastSuccessfulReleaseRevision = hr.Status.LastReleaseRevision
+	case v2.UninstallRemediationStrategy:
+		if err := run.Uninstall(hr); err != nil {
+			// Record observed uninstall failure.
+			meta.SetResourceCondition(hr, v2.RemediatedCondition, metav1.ConditionFalse, v2.UninstallFailedReason, err.Error())
+			v2.HelmReleaseNotReady(hr, v2.UninstallFailedReason, err.Error())
+			r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, err.Error())
+			return ctrl.Result{Requeue: true}, nil
+		}
+
+		// Record observed uninstall remediation success.
+		meta.SetResourceCondition(hr, v2.RemediatedCondition, metav1.ConditionTrue, v2.UninstallSucceededReason,
+			run.GetLastPersistedRelease().Info.Description)
+		hr.Status.LastReleaseRevision = util.ReleaseRevision(run.GetLastPersistedRelease())
+		hr.Status.LastSuccessfulReleaseRevision = hr.Status.LastReleaseRevision
+
+		// If we did not keep our Helm release history, we are now
+		// back to square one.
+		if !hr.Spec.GetUninstall().KeepHistory {
+			hr.Status.LastReleaseRevision = 0
+			hr.Status.LastSuccessfulReleaseRevision = 0
+		}
+	}
+
+	// Requeue instantly to retry.
+	r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityInfo, run.GetLastPersistedRelease().Info.Description)
+	return ctrl.Result{Requeue: true}, nil
+}
+
+// composeValues attempts to resolve all v2beta1.ValuesReference resources
+// and merges them as defined. Referenced resources are only retrieved once
+// to ensure a single version is taken into account during the merge.
+func (r *HelmReleaseReconciler) composeValues(ctx context.Context, log logr.Logger, hr *v2.HelmRelease) (chartutil.Values, error) {
+	result := chartutil.Values{}
+
+	configMaps := make(map[string]*corev1.ConfigMap)
+	secrets := make(map[string]*corev1.Secret)
+
+	for _, v := range hr.Spec.ValuesFrom {
+		namespacedName := types.NamespacedName{Namespace: hr.Namespace, Name: v.Name}
+		var valuesData []byte
+
+		switch v.Kind {
+		case "ConfigMap":
+			resource, ok := configMaps[namespacedName.String()]
+			if !ok {
+				// The resource may not exist, but we want to act on a single version
+				// of the resource in case the values reference is marked as optional.
+				configMaps[namespacedName.String()] = nil
+
+				resource = &corev1.ConfigMap{}
+				if err := r.Get(ctx, namespacedName, resource); err != nil {
+					if apierrors.IsNotFound(err) {
+						if v.Optional {
+							log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
+							continue
+						}
+						return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
+					}
+					return nil, err
+				}
+				configMaps[namespacedName.String()] = resource
+			}
+			if resource == nil {
+				if v.Optional {
+					log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
+					continue
+				}
+				return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
+			}
+			if data, ok := resource.Data[v.GetValuesKey()]; !ok {
+				return nil, fmt.Errorf("missing key '%s' in %s '%s'", v.GetValuesKey(), v.Kind, namespacedName)
+			} else {
+				valuesData = []byte(data)
+			}
+		case "Secret":
+			resource, ok := secrets[namespacedName.String()]
+			if !ok {
+				// The resource may not exist, but we want to act on a single version
+				// of the resource in case the values reference is marked as optional.
+				secrets[namespacedName.String()] = nil
+
+				resource = &corev1.Secret{}
+				if err := r.Get(ctx, namespacedName, resource); err != nil {
+					if apierrors.IsNotFound(err) {
+						if v.Optional {
+							log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
+							continue
+						}
+						return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
+					}
+					return nil, err
+				}
+				secrets[namespacedName.String()] = resource
+			}
+			if resource == nil {
+				if v.Optional {
+					log.Info("could not find optional %s '%s'", v.Kind, namespacedName)
+					continue
+				}
+				return nil, fmt.Errorf("could not find %s '%s'", v.Kind, namespacedName)
+			}
+			if data, ok := resource.Data[v.GetValuesKey()]; !ok {
+				return nil, fmt.Errorf("missing key '%s' in %s '%s'", v.GetValuesKey(), v.Kind, namespacedName)
+			} else {
+				valuesData = data
+			}
+		default:
+			return nil, fmt.Errorf("unsupported ValuesReference kind '%s'", v.Kind)
+		}
+		switch v.TargetPath {
+		case "":
+			values, err := chartutil.ReadValues(valuesData)
+			if err != nil {
+				return nil, fmt.Errorf("unable to read values from key '%s' in %s '%s': %w", v.GetValuesKey(), v.Kind, namespacedName, err)
+			}
+			result = util.MergeMaps(result, values)
+		default:
+			// TODO(hidde): this is a bit of hack, as it mimics the way the option string is passed
+			// 	to Helm from a CLI perspective. Given the parser is however not publicly accessible
+			// 	while it contains all logic around parsing the target path, it is a fair trade-off.
+			singleValue := v.TargetPath + "=" + string(valuesData)
+			if err := strvals.ParseInto(singleValue, result); err != nil {
+				return nil, fmt.Errorf("unable to merge value from key '%s' in %s '%s' into target path '%s': %w", v.GetValuesKey(), v.Kind, namespacedName, v.TargetPath, err)
+			}
+		}
+	}
+	return util.MergeMaps(result, hr.GetValues()), nil
+}
diff --git a/controllers/helmrelease_controller_test.go b/controllers/helmrelease_controller_release_test.go
similarity index 89%
rename from controllers/helmrelease_controller_test.go
rename to controllers/helmrelease_controller_release_test.go
index 8cb4306f4..29b36646a 100644
--- a/controllers/helmrelease_controller_test.go
+++ b/controllers/helmrelease_controller_release_test.go
@@ -18,14 +18,15 @@ package controllers
 
 import (
 	"context"
-	"reflect"
 	"testing"
 
+	. "github.com/onsi/gomega"
 	"helm.sh/helm/v3/pkg/chartutil"
 	corev1 "k8s.io/api/core/v1"
 	apiextensionsv1 "k8s.io/apiextensions-apiserver/pkg/apis/apiextensions/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/client-go/kubernetes/scheme"
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 	"sigs.k8s.io/controller-runtime/pkg/log"
 	"sigs.k8s.io/yaml"
@@ -34,10 +35,6 @@ import (
 )
 
 func TestHelmReleaseReconciler_composeValues(t *testing.T) {
-	scheme := runtime.NewScheme()
-	_ = corev1.AddToScheme(scheme)
-	_ = v2.AddToScheme(scheme)
-
 	tests := []struct {
 		name       string
 		resources  []runtime.Object
@@ -207,26 +204,32 @@ invalid`,
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			c := fake.NewFakeClientWithScheme(scheme, tt.resources...)
-			r := &HelmReleaseReconciler{Client: c, Log: log.NullLogger{}}
+			g := NewWithT(t)
+
+			g.Expect(corev1.AddToScheme(scheme.Scheme)).To(Succeed())
+			g.Expect(v2.AddToScheme(scheme.Scheme)).To(Succeed())
+
+			c := fake.NewFakeClientWithScheme(scheme.Scheme, tt.resources...)
+			r := &HelmReleaseReconciler{Client: c}
+
 			var values *apiextensionsv1.JSON
 			if tt.values != "" {
 				v, _ := yaml.YAMLToJSON([]byte(tt.values))
 				values = &apiextensionsv1.JSON{Raw: v}
 			}
-			hr := v2.HelmRelease{
+			hr := &v2.HelmRelease{
 				Spec: v2.HelmReleaseSpec{
 					ValuesFrom: tt.references,
 					Values:     values,
 				},
 			}
-			got, err := r.composeValues(context.TODO(), hr)
-			if (err != nil) != tt.wantErr {
-				t.Errorf("composeValues() error = %v, wantErr %v", err, tt.wantErr)
-				return
-			}
-			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("composeValues() got = %v, want %v", got, tt.want)
+			got, err := r.composeValues(context.TODO(), log.NullLogger{}, hr)
+			if !tt.wantErr {
+				g.Expect(err).NotTo(HaveOccurred())
+				g.Expect(got).To(Equal(tt.want))
+			} else {
+				g.Expect(err).To(HaveOccurred())
+				g.Expect(got).To(BeNil())
 			}
 		})
 	}
diff --git a/docs/api/helmrelease.md b/docs/api/helmrelease.md
index 449dece87..0daa75f23 100644
--- a/docs/api/helmrelease.md
+++ b/docs/api/helmrelease.md
@@ -947,7 +947,19 @@ int
 </td>
 <td>
 <em>(Optional)</em>
-<p>LastReleaseRevision is the revision of the last successful Helm release.</p>
+<p>LastReleaseRevision is the revision of the last performed Helm release.</p>
+</td>
+</tr>
+<tr>
+<td>
+<code>lastSuccessfulReleaseRevision</code><br>
+<em>
+int
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>LastSuccessfulReleaseRevision is the revision of the last successful Helm release.</p>
 </td>
 </tr>
 <tr>
diff --git a/internal/runner/runner.go b/internal/runner/runner.go
index 0eb1a6b1c..81216680d 100644
--- a/internal/runner/runner.go
+++ b/internal/runner/runner.go
@@ -25,31 +25,40 @@ import (
 	"helm.sh/helm/v3/pkg/chart"
 	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/release"
+	helmstorage "helm.sh/helm/v3/pkg/storage"
 	"helm.sh/helm/v3/pkg/storage/driver"
 	"k8s.io/cli-runtime/pkg/genericclioptions"
 
 	v2 "github.com/fluxcd/helm-controller/api/v2beta1"
+	"github.com/fluxcd/helm-controller/internal/storage"
 )
 
-// Runner represents a Helm action runner capable of performing Helm
-// operations for a v2beta1.HelmRelease.
+// Runner is a Helm action runner capable of performing Helm operations
+// for a v2beta1.HelmRelease.
 type Runner struct {
-	config *action.Configuration
+	config   *action.Configuration
+	observer *storage.Observer
 }
 
 // NewRunner constructs a new Runner configured to run Helm actions with the
 // given genericclioptions.RESTClientGetter, and the release and storage
 // namespace configured to the provided values.
-func NewRunner(getter genericclioptions.RESTClientGetter, storageNamespace string, logger logr.Logger) (*Runner, error) {
+func NewRunner(getter genericclioptions.RESTClientGetter, hr *v2.HelmRelease, log logr.Logger) (*Runner, error) {
 	cfg := new(action.Configuration)
-	if err := cfg.Init(getter, storageNamespace, "secret", debugLogger(logger)); err != nil {
+	if err := cfg.Init(getter, hr.GetNamespace(), "secret", debugLogger(log)); err != nil {
 		return nil, err
 	}
-	return &Runner{config: cfg}, nil
+	last, err := cfg.Releases.Get(hr.GetReleaseName(), hr.Status.LastReleaseRevision)
+	if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
+		return nil, err
+	}
+	observer := storage.NewObserver(cfg.Releases.Driver, last)
+	cfg.Releases = helmstorage.Init(observer)
+	return &Runner{config: cfg, observer: observer}, nil
 }
 
 // Install runs an Helm install action for the given v2beta1.HelmRelease.
-func (r *Runner) Install(hr v2.HelmRelease, chart *chart.Chart, values chartutil.Values) (*release.Release, error) {
+func (r *Runner) Install(hr v2.HelmRelease, chart *chart.Chart, values chartutil.Values) error {
 	install := action.NewInstall(r.config)
 	install.ReleaseName = hr.GetReleaseName()
 	install.Namespace = hr.GetReleaseNamespace()
@@ -64,11 +73,12 @@ func (r *Runner) Install(hr v2.HelmRelease, chart *chart.Chart, values chartutil
 		install.CreateNamespace = hr.Spec.GetInstall().CreateNamespace
 	}
 
-	return install.Run(chart, values.AsMap())
+	_, err := install.Run(chart, values.AsMap())
+	return err
 }
 
 // Upgrade runs an Helm upgrade action for the given v2beta1.HelmRelease.
-func (r *Runner) Upgrade(hr v2.HelmRelease, chart *chart.Chart, values chartutil.Values) (*release.Release, error) {
+func (r *Runner) Upgrade(hr v2.HelmRelease, chart *chart.Chart, values chartutil.Values) error {
 	upgrade := action.NewUpgrade(r.config)
 	upgrade.Namespace = hr.GetReleaseNamespace()
 	upgrade.ResetValues = !hr.Spec.GetUpgrade().PreserveValues
@@ -80,19 +90,22 @@ func (r *Runner) Upgrade(hr v2.HelmRelease, chart *chart.Chart, values chartutil
 	upgrade.Force = hr.Spec.GetUpgrade().Force
 	upgrade.CleanupOnFail = hr.Spec.GetUpgrade().CleanupOnFail
 
-	return upgrade.Run(hr.GetReleaseName(), chart, values.AsMap())
+	_, err := upgrade.Run(hr.GetReleaseName(), chart, values.AsMap())
+	return err
 }
 
 // Test runs an Helm test action for the given v2beta1.HelmRelease.
-func (r *Runner) Test(hr v2.HelmRelease) (*release.Release, error) {
+func (r *Runner) Test(hr v2.HelmRelease) error {
 	test := action.NewReleaseTesting(r.config)
 	test.Namespace = hr.GetReleaseNamespace()
 	test.Timeout = hr.Spec.GetTest().GetTimeout(hr.GetTimeout()).Duration
 
-	return test.Run(hr.GetReleaseName())
+	_, err := test.Run(hr.GetReleaseName())
+	return err
 }
 
-// Rollback runs an Helm rollback action for the given v2beta1.HelmRelease.
+// Rollback runs an Helm rollback action to the last successful release
+// revision of the given v2beta1.HelmRelease.
 func (r *Runner) Rollback(hr v2.HelmRelease) error {
 	rollback := action.NewRollback(r.config)
 	rollback.Timeout = hr.Spec.GetRollback().GetTimeout(hr.GetTimeout()).Duration
@@ -101,12 +114,13 @@ func (r *Runner) Rollback(hr v2.HelmRelease) error {
 	rollback.Force = hr.Spec.GetRollback().Force
 	rollback.Recreate = hr.Spec.GetRollback().Recreate
 	rollback.CleanupOnFail = hr.Spec.GetRollback().CleanupOnFail
+	rollback.Version = hr.Status.LastSuccessfulReleaseRevision
 
 	return rollback.Run(hr.GetReleaseName())
 }
 
 // Uninstall runs an Helm uninstall action for the given v2beta1.HelmRelease.
-func (r *Runner) Uninstall(hr v2.HelmRelease) error {
+func (r *Runner) Uninstall(hr *v2.HelmRelease) error {
 	uninstall := action.NewUninstall(r.config)
 	uninstall.Timeout = hr.Spec.GetUninstall().GetTimeout(hr.GetTimeout()).Duration
 	uninstall.DisableHooks = hr.Spec.GetUninstall().DisableHooks
@@ -118,7 +132,7 @@ func (r *Runner) Uninstall(hr v2.HelmRelease) error {
 
 // ObserveLastRelease observes the last revision, if there is one,
 // for the actual Helm release associated with the given v2beta1.HelmRelease.
-func (r *Runner) ObserveLastRelease(hr v2.HelmRelease) (*release.Release, error) {
+func (r *Runner) ObserveLastRelease(hr *v2.HelmRelease) (*release.Release, error) {
 	rel, err := r.config.Releases.Last(hr.GetReleaseName())
 	if err != nil && errors.Is(err, driver.ErrReleaseNotFound) {
 		err = nil
@@ -126,6 +140,12 @@ func (r *Runner) ObserveLastRelease(hr v2.HelmRelease) (*release.Release, error)
 	return rel, err
 }
 
+// GetLastPersistedRelease returns the release last persisted to the
+// Helm storage by a Runner action.
+func (r *Runner) GetLastPersistedRelease() *release.Release {
+	return r.observer.GetLastObservedRelease()
+}
+
 func debugLogger(logger logr.Logger) func(format string, v ...interface{}) {
 	return func(format string, v ...interface{}) {
 		logger.V(1).Info(fmt.Sprintf(format, v...))
diff --git a/internal/storage/observer.go b/internal/storage/observer.go
new file mode 100644
index 000000000..8cd60cd1b
--- /dev/null
+++ b/internal/storage/observer.go
@@ -0,0 +1,118 @@
+/*
+Copyright 2020 The Flux authors
+
+Licensed under the Apache License, Version 2.0 (the "License");
+you may not use this file except in compliance with the License.
+You may obtain a copy of the License at
+
+    http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing, software
+distributed under the License is distributed on an "AS IS" BASIS,
+WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+See the License for the specific language governing permissions and
+limitations under the License.
+*/
+
+package storage
+
+import (
+	"sync"
+
+	"helm.sh/helm/v3/pkg/release"
+	"helm.sh/helm/v3/pkg/storage/driver"
+)
+
+const ObserverDriverName = "observer"
+
+// Observer observes the writes to the Helm storage driver it embeds
+// and caches the persisted release.Release if its revision is higher
+// or equal to the revision of the release.Release the cache holds
+// at that moment.
+//
+// This allows for observations on persisted state that were the result
+// of our own actions, and works around the inconsistent behavior of
+// some of the Helm actions that may return an object that was not
+// actually persisted to the Helm storage (e.g. because a validation
+// error occurred during a Helm upgrade).
+//
+// NB: the implementation is simple, and while it does perform locking
+// operations for cache writes, it was designed to only deal with a
+// single Helm action at a time, including read operations on the
+// cached release.
+type Observer struct {
+	sync.Mutex
+	driver.Driver
+	release *release.Release
+}
+
+// NewObserver creates a new observing Helm storage driver, the given
+// release is the initial cached release.Release item and may be
+// omitted.
+func NewObserver(driver driver.Driver, rls *release.Release) *Observer {
+	return &Observer{Driver: driver, release: rls}
+}
+
+// Name returns the name of the driver.
+func (o *Observer) Name() string {
+	return ObserverDriverName
+}
+
+// Get returns the release named by key or returns ErrReleaseNotFound.
+func (o *Observer) Get(key string) (*release.Release, error) {
+	return o.Driver.Get(key)
+}
+
+// List returns the list of all releases such that filter(release) == true
+func (o *Observer) List(filter func(*release.Release) bool) ([]*release.Release, error) {
+	return o.Driver.List(filter)
+}
+
+// Query returns the set of releases that match the provided set of labels
+func (o *Observer) Query(keyvals map[string]string) ([]*release.Release, error) {
+	return o.Driver.Query(keyvals)
+}
+
+// Create creates a new release or returns driver.ErrReleaseExists.
+func (o *Observer) Create(key string, rls *release.Release) error {
+	o.Lock()
+	defer o.Unlock()
+	if err := o.Driver.Create(key, rls); err != nil {
+		return err
+	}
+	if hasNewerRevision(o.release, rls) {
+		o.release = rls
+	}
+	return nil
+}
+
+// Update updates a release or returns driver.ErrReleaseNotFound.
+func (o *Observer) Update(key string, rls *release.Release) error {
+	o.Lock()
+	defer o.Unlock()
+	if err := o.Driver.Update(key, rls); err != nil {
+		return err
+	}
+	if hasNewerRevision(o.release, rls) {
+		o.release = rls
+	}
+	return nil
+}
+
+// Delete deletes a release or returns driver.ErrReleaseNotFound.
+func (o *Observer) Delete(key string) (*release.Release, error) {
+	return o.Driver.Delete(key)
+}
+
+// GetLastObservedRelease returns the last persisted release that was
+// observed.
+func (o *Observer) GetLastObservedRelease() *release.Release {
+	return o.release
+}
+
+func hasNewerRevision(j, k *release.Release) bool {
+	if j == nil {
+		return true
+	}
+	return k.Version >= j.Version
+}
diff --git a/internal/util/util.go b/internal/util/util.go
index 0d337419b..1e18839d6 100644
--- a/internal/util/util.go
+++ b/internal/util/util.go
@@ -22,6 +22,7 @@ import (
 
 	"helm.sh/helm/v3/pkg/chartutil"
 	"helm.sh/helm/v3/pkg/release"
+	ctrl "sigs.k8s.io/controller-runtime"
 )
 
 // MergeMaps merges map b into given map a and returns the result.
@@ -65,3 +66,73 @@ func ReleaseRevision(rel *release.Release) int {
 	}
 	return rel.Version
 }
+
+// LowestNonZeroResult compares two reconciliation results and returns
+// the one with lowest requeue time.
+func LowestNonZeroResult(i, j ctrl.Result) ctrl.Result {
+	switch {
+	case i.IsZero():
+		return j
+	case j.IsZero():
+		return i
+	case i.Requeue:
+		return i
+	case j.Requeue:
+		return j
+	case i.RequeueAfter < j.RequeueAfter:
+		return i
+	default:
+		return j
+	}
+}
+
+func CalculateTestSuiteResult(rls *release.Release) ([]string, []string) {
+	result := make(map[release.HookPhase][]string)
+	executions := executionsByHookEvent(rls)
+	tests, ok := executions[release.HookTest]
+	if !ok {
+		return nil, nil
+	}
+	for _, h := range tests {
+		names := result[h.LastRun.Phase]
+		result[h.LastRun.Phase] = append(names, h.Name)
+	}
+	return result[release.HookPhaseSucceeded], result[release.HookPhaseFailed]
+}
+
+func HasRunTestSuite(rls *release.Release) bool {
+	executions := executionsByHookEvent(rls)
+	tests, ok := executions[release.HookTest]
+	if !ok {
+		return false
+	}
+	for _, h := range tests {
+		if !h.LastRun.StartedAt.IsZero() {
+			return true
+		}
+	}
+	return false
+}
+
+func HasTestSuite(rls *release.Release) bool {
+	executions := executionsByHookEvent(rls)
+	tests, ok := executions[release.HookTest]
+	if !ok {
+		return false
+	}
+	return len(tests) > 0
+}
+
+func executionsByHookEvent(rls *release.Release) map[release.HookEvent][]*release.Hook {
+	result := make(map[release.HookEvent][]*release.Hook)
+	for _, h := range rls.Hooks {
+		for _, e := range h.Events {
+			executions, ok := result[e]
+			if !ok {
+				executions = []*release.Hook{}
+			}
+			result[e] = append(executions, h)
+		}
+	}
+	return result
+}

From 4eaa6a5e4211dec30217345b2827c01111aabbcc Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Thu, 26 Nov 2020 13:43:59 +0100
Subject: [PATCH 2/4] Modify e2e checks to match changed Released cond

Signed-off-by: Hidde Beydals <hello@hidde.co>
---
 .github/workflows/e2e.yaml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.github/workflows/e2e.yaml b/.github/workflows/e2e.yaml
index d1dca73a0..6d490325e 100644
--- a/.github/workflows/e2e.yaml
+++ b/.github/workflows/e2e.yaml
@@ -134,7 +134,7 @@ jobs:
           kubectl -n helm-system apply -f config/testdata/$test_name
           echo -n ">>> Waiting for expected conditions"
           count=0
-          until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="False" and .TestSuccess=="False" and .Ready=="False"' )" ]; do
+          until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="True" and .TestSuccess=="False" and .Ready=="False"' )" ]; do
             echo -n '.'
             sleep 5
             count=$((count + 1))
@@ -301,7 +301,7 @@ jobs:
           kubectl -n helm-system apply -f config/testdata/$test_name/upgrade.yaml
           echo -n ">>> Waiting for expected conditions"
           count=0
-          until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="False" and .TestSuccess=="False" and .Ready=="False"' )" ]; do
+          until [ 'true' == "$( kubectl -n helm-system get helmrelease/$test_name -o json | jq '.status.conditions | map( { (.type): .status } ) | add | .Released=="True" and .TestSuccess=="False" and .Ready=="False"' )" ]; do
             echo -n '.'
             sleep 5
             count=$((count + 1))

From c5c58a970341364672036fab32086dbc294c511f Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Tue, 1 Dec 2020 16:27:41 +0100
Subject: [PATCH 3/4] Address review comments

Signed-off-by: Hidde Beydals <hello@hidde.co>
---
 api/v2beta1/condition_types.go                |  4 ++++
 controllers/helmrelease_controller_release.go | 14 +++++++-------
 2 files changed, 11 insertions(+), 7 deletions(-)

diff --git a/api/v2beta1/condition_types.go b/api/v2beta1/condition_types.go
index 2aed2bb15..d24139be7 100644
--- a/api/v2beta1/condition_types.go
+++ b/api/v2beta1/condition_types.go
@@ -72,6 +72,10 @@ const (
 	// HelmRelease failed.
 	UninstallFailedReason string = "UninstallFailed"
 
+	// ArtifactNotReadyReason represents the fact that the artifact for the HelmChart
+	// is not available.
+	ArtifactNotReadyReason string = "ArtifactNotReady"
+
 	// ArtifactFailedReason represents the fact that the artifact download for the
 	// HelmRelease failed.
 	ArtifactFailedReason string = "ArtifactFailed"
diff --git a/controllers/helmrelease_controller_release.go b/controllers/helmrelease_controller_release.go
index 2887e8a8d..97a7eb021 100644
--- a/controllers/helmrelease_controller_release.go
+++ b/controllers/helmrelease_controller_release.go
@@ -45,7 +45,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.L
 	hc, err := r.getHelmChart(ctx, hr)
 	if err != nil {
 		err = fmt.Errorf("failed to get HelmChart for resource: %w", err)
-		v2.HelmReleaseNotReady(hr, v2.GetLastReleaseFailedReason, err.Error())
+		v2.HelmReleaseNotReady(hr, v2.ArtifactFailedReason, err.Error())
 		return ctrl.Result{}, err
 	}
 
@@ -55,7 +55,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.L
 	// a new reconciliation.
 	if hc.GetArtifact() == nil {
 		msg := fmt.Sprintf("HelmChart '%s/%s' has no artifact", hc.GetNamespace(), hc.GetName())
-		v2.HelmReleaseNotReady(hr, meta.DependencyNotReadyReason, msg)
+		v2.HelmReleaseNotReady(hr, v2.ArtifactNotReadyReason, msg)
 		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
 	}
 
@@ -84,7 +84,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.L
 	// release in the Helm storage.
 	makeRelease, remediation := run.Install, hr.Spec.GetInstall().GetRemediation()
 	successReason, failureReason := v2.InstallSucceededReason, v2.InstallFailedReason
-	if rls != nil {
+	if rls != nil && hr.Status.LastSuccessfulReleaseRevision > 0 {
 		makeRelease, remediation = run.Upgrade, hr.Spec.GetUpgrade().GetRemediation()
 		successReason, failureReason = v2.UpgradeSucceededReason, v2.UpgradeFailedReason
 	}
@@ -94,7 +94,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.L
 	releaseRevision := util.ReleaseRevision(rls)
 	if rls != nil && hr.Status.LastReleaseRevision == releaseRevision && rls.Info.Status.IsPending() {
 		msg := fmt.Sprintf("previous release did not finish (%s)", rls.Info.Status)
-		v2.HelmReleaseNotReady(hr, v2.RemediatedCondition, msg)
+		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, msg)
 		r.event(hr, hr.Status.LastAttemptedRevision, events.EventSeverityError, msg)
 		remediation.IncrementFailureCount(hr)
 		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
@@ -119,8 +119,8 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, log logr.L
 	case remediation.RetriesExhausted(*hr):
 		v2.HelmReleaseNotReady(hr, meta.ReconciliationFailedReason, "exhausted release retries")
 		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
-	// Our previous reconciliation attempt failed, skip release to retry.
-	case hr.Status.LastSuccessfulReleaseRevision > 0 && hr.Status.LastReleaseRevision != hr.Status.LastSuccessfulReleaseRevision:
+	// Our previous remediation attempt failed, skip release to retry.
+	case hr.Status.LastReleaseRevision != hr.Status.LastSuccessfulReleaseRevision:
 		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
 	}
 
@@ -168,7 +168,7 @@ func (r *HelmReleaseReconciler) reconcileTest(ctx context.Context, log logr.Logg
 	// If this release was already marked as successful,
 	// we have nothing to do.
 	if hr.Status.LastReleaseRevision == hr.Status.LastSuccessfulReleaseRevision {
-		return ctrl.Result{}, nil
+		return ctrl.Result{RequeueAfter: hr.Spec.Interval.Duration}, nil
 	}
 
 	// Confirm the last release in storage equals to the release

From 55febb2cbf6ec031aa61aa13a48f76fce503cb3a Mon Sep 17 00:00:00 2001
From: Hidde Beydals <hello@hidde.co>
Date: Thu, 3 Dec 2020 12:59:37 +0100
Subject: [PATCH 4/4] Add NewRunnerWithConfig constructor method

This makes it possible to inject a custom Helm action configuration,
which is useful for tests where you want to be able to inject a (fake)
test client and/or different (memory) storage driver.

Signed-off-by: Hidde Beydals <hello@hidde.co>
---
 internal/runner/runner.go | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/internal/runner/runner.go b/internal/runner/runner.go
index 81216680d..41f406b74 100644
--- a/internal/runner/runner.go
+++ b/internal/runner/runner.go
@@ -19,6 +19,7 @@ package runner
 import (
 	"errors"
 	"fmt"
+	"strings"
 
 	"github.com/go-logr/logr"
 	"helm.sh/helm/v3/pkg/action"
@@ -45,9 +46,15 @@ type Runner struct {
 // namespace configured to the provided values.
 func NewRunner(getter genericclioptions.RESTClientGetter, hr *v2.HelmRelease, log logr.Logger) (*Runner, error) {
 	cfg := new(action.Configuration)
-	if err := cfg.Init(getter, hr.GetNamespace(), "secret", debugLogger(log)); err != nil {
+	if err := cfg.Init(getter, hr.GetNamespace(), strings.ToLower(driver.SecretsDriverName), debugLogger(log)); err != nil {
 		return nil, err
 	}
+	return NewRunnerWithConfig(cfg, hr)
+}
+
+// NewRunnerWithConfig constructs a new Runner configured with the
+// given action.Configuration.
+func NewRunnerWithConfig(cfg *action.Configuration, hr *v2.HelmRelease) (*Runner, error) {
 	last, err := cfg.Releases.Get(hr.GetReleaseName(), hr.Status.LastReleaseRevision)
 	if err != nil && !errors.Is(err, driver.ErrReleaseNotFound) {
 		return nil, err