pkg/payload/precondition: File shuffling, drop ClusterVersion argument, etc.

pkg/cvo/sync_test.go

@@ -465,7 +465,7 @@ func (pf *testPrecondition) Name() string {
 	return fmt.Sprintf("TestPrecondition SuccessAfter: %d", pf.SuccessAfter)
 }
 
-func (pf *testPrecondition) Run(_ context.Context, _ precondition.ReleaseContext, cv *configv1.ClusterVersion) error {
+func (pf *testPrecondition) Run(_ context.Context, _ precondition.ReleaseContext) error {
 	if pf.SuccessAfter == 0 {
 		return nil
 	}

pkg/cvo/sync_worker.go

@@ -630,12 +630,12 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in
 				Actual:      desired,
 				Verified:    info.Verified,
 			})
-			if err := precondition.Summarize(w.preconditions.RunAll(ctx, precondition.ReleaseContext{DesiredVersion: payloadUpdate.Release.Version}, clusterVersion)); err != nil {
-				if work.Desired.Force {
-					klog.V(4).Infof("Forcing past precondition failures: %s", err)
-					w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionsForced", "preconditions forced for payload loaded version=%q image=%q failures=%v", desired.Version, desired.Image, err)
-				} else {
-					w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionsFailed", "preconditions failed for payload loaded version=%q image=%q failures=%v", desired.Version, desired.Image, err)
+			if block, err := precondition.Summarize(w.preconditions.RunAll(ctx, precondition.ReleaseContext{
+				DesiredVersion: payloadUpdate.Release.Version,
+			}), work.Desired.Force); err != nil {
+				klog.V(4).Infof("Precondition error (force %t, block %t): %v", work.Desired.Force, block, err)
+				if block {
+					w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionBlock", "preconditions failed for payload loaded version=%q image=%q: %v", desired.Version, desired.Image, err)
 					reporter.Report(SyncWorkerStatus{
 						Generation:  work.Generation,
 						Failure:     err,
@@ -646,6 +646,8 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in
 						Verified:    info.Verified,
 					})
 					return err
+				} else {
+					w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionWarn", "precondition warning for payload loaded version=%q image=%q: %v", desired.Version, desired.Image, err)
 				}
 			}
 			w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeNormal, "PreconditionsPassed", "preconditions passed for payload loaded version=%q image=%q", desired.Version, desired.Image)

pkg/payload/precondition/clusterversion/etcdbackup.go

@@ -0,0 +1,90 @@
+package clusterversion
+
+import (
+	"context"
+	"fmt"
+
+	configv1 "github.com/openshift/api/config/v1"
+	configv1listers "github.com/openshift/client-go/config/listers/config/v1"
+	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	"k8s.io/apimachinery/pkg/api/meta"
+
+	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
+	precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition"
+)
+
+const backupConditionType = "RecentBackup"
+
+// RecentEtcdBackup checks if a recent etcd backup has been taken.
+type RecentEtcdBackup struct {
+	key      string
+	cvLister configv1listers.ClusterVersionLister
+	coLister configv1listers.ClusterOperatorLister
+}
+
+// NewRecentEtcdBackup returns a new RecentEtcdBackup precondition check.
+func NewRecentEtcdBackup(cvLister configv1listers.ClusterVersionLister, coLister configv1listers.ClusterOperatorLister) *RecentEtcdBackup {
+	return &RecentEtcdBackup{
+		key:      "version",
+		cvLister: cvLister,
+		coLister: coLister,
+	}
+}
+
+func recentEtcdBackupCondition(lister configv1listers.ClusterOperatorLister) (reason string, message string) {
+	var msgDetail string
+	ops, err := lister.Get("etcd")
+	if err == nil {
+		backupCondition := resourcemerge.FindOperatorStatusCondition(ops.Status.Conditions, backupConditionType)
+		if backupCondition == nil {
+			reason = "EtcdRecentBackupNotSet"
+			msgDetail = "etcd backup condition is not set."
+		} else if backupCondition.Status != configv1.ConditionTrue {
+			reason = backupCondition.Reason
+			msgDetail = backupCondition.Message
+		}
+	} else {
+		reason = "UnableToGetEtcdOperator"
+		msgDetail = fmt.Sprintf("Unable to get etcd operator, err=%v.", err)
+	}
+	if len(msgDetail) > 0 {
+		message = fmt.Sprintf("%s: %s", backupConditionType, msgDetail)
+	}
+	return reason, message
+}
+
+// Run runs the RecentEtcdBackup precondition. It returns a PreconditionError until Etcd indicates that a
+// recent etcd backup has been taken.
+func (pf *RecentEtcdBackup) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error {
+	cv, err := pf.cvLister.Get(pf.key)
+	if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
+		return nil
+	}
+	if err != nil {
+		return &precondition.Error{
+			Nested:  err,
+			Reason:  "UnknownError",
+			Message: err.Error(),
+			Name:    pf.Name(),
+		}
+	}
+
+	currentVersion := GetCurrentVersion(cv.Status.History)
+	currentMinor := GetEffectiveMinor(currentVersion)
+	desiredMinor := GetEffectiveMinor(releaseContext.DesiredVersion)
+
+	if minorVersionUpgrade(currentMinor, desiredMinor) {
+		reason, message := recentEtcdBackupCondition(pf.coLister)
+		if len(reason) > 0 {
+			return &precondition.Error{
+				Reason:  reason,
+				Message: message,
+				Name:    pf.Name(),
+			}
+		}
+	}
+	return nil
+}
+
+// Name returns Name for the precondition.
+func (pf *RecentEtcdBackup) Name() string { return "EtcdRecentBackup" }

pkg/payload/precondition/clusterversion/upgradeable.go

@@ -2,7 +2,6 @@ package clusterversion
 
 import (
 	"context"
-	"fmt"
 	"strconv"
 	"strings"
 
@@ -16,8 +15,6 @@ import (
 	precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition"
 )
 
-const backupConditionType = "RecentBackup"
-
 // Upgradeable checks if clusterversion is upgradeable currently.
 type Upgradeable struct {
 	key    string
@@ -51,7 +48,7 @@ func ClusterVersionOverridesCondition(cv *configv1.ClusterVersion) *configv1.Clu
 // Run runs the Upgradeable precondition.
 // If the feature gate `key` is not found, or the api for clusterversion doesn't exist, this check is inert and always returns nil error.
 // Otherwise, if Upgradeable condition is set to false in the object, it returns an PreconditionError when possible.
-func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext, clusterVersion *configv1.ClusterVersion) error {
+func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error {
 	cv, err := pf.lister.Get(pf.key)
 	if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
 		return nil
@@ -91,7 +88,7 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 	// if no cluster overrides have been set
 	if currentMinor == desiredMinor {
 		klog.V(4).Infof("Precondition %q passed: minor from the current %s matches minor from the target %s (both %s).", pf.Name(), currentVersion, releaseContext.DesiredVersion, currentMinor)
-		if condition := ClusterVersionOverridesCondition(clusterVersion); condition != nil {
+		if condition := ClusterVersionOverridesCondition(cv); condition != nil {
 			klog.V(4).Infof("Update from %s to %s blocked by %s: %s", currentVersion, releaseContext.DesiredVersion, condition.Reason, condition.Message)
 
 			return &precondition.Error{
@@ -115,80 +112,6 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 // Name returns Name for the precondition.
 func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" }
 
-//  RecentEtcdBackup checks if a recent etcd backup has been taken.
-type RecentEtcdBackup struct {
-	key      string
-	cvLister configv1listers.ClusterVersionLister
-	coLister configv1listers.ClusterOperatorLister
-}
-
-// NewRecentEtcdBackup returns a new RecentEtcdBackup precondition check.
-func NewRecentEtcdBackup(cvLister configv1listers.ClusterVersionLister, coLister configv1listers.ClusterOperatorLister) *RecentEtcdBackup {
-	return &RecentEtcdBackup{
-		key:      "version",
-		cvLister: cvLister,
-		coLister: coLister,
-	}
-}
-
-func recentEtcdBackupCondition(lister configv1listers.ClusterOperatorLister) (reason string, message string) {
-	var msgDetail string
-	ops, err := lister.Get("etcd")
-	if err == nil {
-		backupCondition := resourcemerge.FindOperatorStatusCondition(ops.Status.Conditions, backupConditionType)
-		if backupCondition == nil {
-			reason = "EtcdRecentBackupNotSet"
-			msgDetail = "etcd backup condition is not set."
-		} else if backupCondition.Status != configv1.ConditionTrue {
-			reason = backupCondition.Reason
-			msgDetail = backupCondition.Message
-		}
-	} else {
-		reason = "UnableToGetEtcdOperator"
-		msgDetail = fmt.Sprintf("Unable to get etcd operator, err=%v.", err)
-	}
-	if len(msgDetail) > 0 {
-		message = fmt.Sprintf("%s: %s", backupConditionType, msgDetail)
-	}
-	return reason, message
-}
-
-// Run runs the RecentEtcdBackup precondition. It returns a PreconditionError until Etcd indicates that a
-// recent etcd backup has been taken.
-func (pf *RecentEtcdBackup) Run(ctx context.Context, releaseContext precondition.ReleaseContext, clusterVersion *configv1.ClusterVersion) error {
-	cv, err := pf.cvLister.Get(pf.key)
-	if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) {
-		return nil
-	}
-	if err != nil {
-		return &precondition.Error{
-			Nested:  err,
-			Reason:  "UnknownError",
-			Message: err.Error(),
-			Name:    pf.Name(),
-		}
-	}
-
-	currentVersion := GetCurrentVersion(cv.Status.History)
-	currentMinor := GetEffectiveMinor(currentVersion)
-	desiredMinor := GetEffectiveMinor(releaseContext.DesiredVersion)
-
-	if minorVersionUpgrade(currentMinor, desiredMinor) {
-		reason, message := recentEtcdBackupCondition(pf.coLister)
-		if len(reason) > 0 {
-			return &precondition.Error{
-				Reason:  reason,
-				Message: message,
-				Name:    pf.Name(),
-			}
-		}
-	}
-	return nil
-}
-
-// Name returns Name for the precondition.
-func (pf *RecentEtcdBackup) Name() string { return "EtcdRecentBackup" }
-
 // GetCurrentVersion determines and returns the cluster's current version by iterating through the
 // provided update history until it finds the first version with update State of Completed. If a
 // Completed version is not found the version of the oldest history entry, which is the originally

pkg/payload/precondition/clusterversion/upgradable_test.go → pkg/payload/precondition/clusterversion/upgradeable_test.go

@@ -54,6 +54,7 @@ func TestGetEffectiveMinor(t *testing.T) {
 }
 
 func TestUpgradeableRun(t *testing.T) {
+	ctx := context.Background()
 	ptr := func(status configv1.ConditionStatus) *configv1.ConditionStatus {
 		return &status
 	}
@@ -128,7 +129,9 @@ func TestUpgradeableRun(t *testing.T) {
 			cvLister := fakeClusterVersionLister(t, clusterVersion)
 			instance := NewUpgradeable(cvLister)
 
-			err := instance.Run(context.TODO(), precondition.ReleaseContext{DesiredVersion: tc.desiredVersion}, clusterVersion)
+			err := instance.Run(ctx, precondition.ReleaseContext{
+				DesiredVersion: tc.desiredVersion,
+			})
 			switch {
 			case err != nil && len(tc.expected) == 0:
 				t.Error(err)

pkg/payload/precondition/precondition.go

@@ -5,7 +5,6 @@ import (
 	"fmt"
 	"strings"
 
-	configv1 "github.com/openshift/api/config/v1"
 	"k8s.io/klog/v2"
 
 	"github.com/openshift/cluster-version-operator/pkg/payload"
@@ -42,7 +41,7 @@ type ReleaseContext struct {
 // Precondition defines the precondition check for a payload.
 type Precondition interface {
 	// Run executes the precondition checks ands returns an error when the precondition fails.
-	Run(ctx context.Context, releaseContext ReleaseContext, cv *configv1.ClusterVersion) error
+	Run(ctx context.Context, releaseContext ReleaseContext) error
 
 	// Name returns a human friendly name for the precondition.
 	Name() string
@@ -53,10 +52,10 @@ type List []Precondition
 
 // RunAll runs all the reflight checks in order, returning a list of errors if any.
 // All checks are run, regardless if any one precondition fails.
-func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext, cv *configv1.ClusterVersion) []error {
+func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext) []error {
 	var errs []error
 	for _, pf := range pfList {
-		if err := pf.Run(ctx, releaseContext, cv); err != nil {
+		if err := pf.Run(ctx, releaseContext); err != nil {
 			klog.Errorf("Precondition %q failed: %v", pf.Name(), err)
 			errs = append(errs, err)
 		}
@@ -65,9 +64,11 @@ func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext, cv
 }
 
 // Summarize summarizes all the precondition.Error from errs.
-func Summarize(errs []error) error {
+// Returns the consolidated error and a boolean for whether the error
+// is blocking (true) or a warning (false).
+func Summarize(errs []error, force bool) (bool, error) {
 	if len(errs) == 0 {
-		return nil
+		return false, nil
 	}
 	var msgs []string
 	for _, e := range errs {
@@ -83,7 +84,12 @@ func Summarize(errs []error) error {
 	} else {
 		msg = fmt.Sprintf("Multiple precondition checks failed:\n* %s", strings.Join(msgs, "\n* "))
 	}
-	return &payload.UpdateError{
+
+	if force {
+		msg = fmt.Sprintf("Forced through blocking failures: %s", msg)
+	}
+
+	return !force, &payload.UpdateError{
 		Nested:  nil,
 		Reason:  "UpgradePreconditionCheckFailed",
 		Message: msg,