Merge pull request #1276 from wking/Upgradeable-blocks-major-too

OTA-1787: pkg: Update Upgradeable condition to explicitly mention major version updates

Author: openshift-merge-bot[bot]

install/0000_90_cluster-version-operator_02_servicemonitor.yaml

@@ -87,7 +87,7 @@ spec:
     rules:
     - alert: ClusterNotUpgradeable
       annotations:
-        summary: One or more cluster operators have been blocking minor version cluster upgrades for at least an hour.
+        summary: One or more cluster operators have been blocking minor or major version cluster updates for at least an hour.
         description: In most cases, you will still be able to apply patch releases. Reason {{ "{{ with $cluster_operator_conditions := \"cluster_operator_conditions\" | query}}{{range $value := .}}{{if and (eq (label \"name\" $value) \"version\") (eq (label \"condition\" $value) \"Upgradeable\") (eq (label \"endpoint\" $value) \"metrics\") (eq (value $value) 0.0) (ne (len (label \"reason\" $value)) 0) }}{{label \"reason\" $value}}.{{end}}{{end}}{{end}}"}} For more information refer to 'oc adm upgrade'{{ "{{ with $console_url := \"console_url\" | query }}{{ if ne (len (label \"url\" (first $console_url ) ) ) 0}} or {{ label \"url\" (first $console_url ) }}/settings/cluster/{{ end }}{{ end }}" }}.
       expr: |
         max by (namespace, name, condition, endpoint) (cluster_operator_conditions{name="version", condition="Upgradeable", endpoint="metrics"} == 0)

pkg/cvo/cvo_test.go

@@ -2929,7 +2929,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					Type:    configv1.OperatorUpgradeable,
 					Status:  configv1.ConditionFalse,
 					Reason:  "RandomReason",
-					Message: "Cluster operator default-operator-1 should not be upgraded between minor versions: some random reason why upgrades are not safe.",
+					Message: "Cluster operator default-operator-1 should not be upgraded between minor or major versions: some random reason why upgrades are not safe.",
 				}},
 			},
 		},
@@ -2990,7 +2990,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					Type:    configv1.OperatorUpgradeable,
 					Status:  configv1.ConditionFalse,
 					Reason:  "RandomReason",
-					Message: "Cluster operator default-operator-1 should not be upgraded between minor versions: some random reason why upgrades are not safe.",
+					Message: "Cluster operator default-operator-1 should not be upgraded between minor or major versions: some random reason why upgrades are not safe.",
 				}},
 			},
 		},
@@ -3054,7 +3054,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					Type:    configv1.OperatorUpgradeable,
 					Status:  configv1.ConditionFalse,
 					Reason:  "RandomReason",
-					Message: "Cluster operator default-operator-1 should not be upgraded between minor versions: some random reason why upgrades are not safe.",
+					Message: "Cluster operator default-operator-1 should not be upgraded between minor or major versions: some random reason why upgrades are not safe.",
 				}},
 			},
 		},
@@ -3120,7 +3120,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					Type:    configv1.OperatorUpgradeable,
 					Status:  configv1.ConditionFalse,
 					Reason:  "ClusterOperatorsNotUpgradeable",
-					Message: "Multiple cluster operators should not be upgraded between minor versions:\n* Cluster operator default-operator-1 should not be upgraded between minor versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor versions: RandomReason2: some random reason 2 why upgrades are not safe.",
+					Message: "Multiple cluster operators should not be upgraded between minor or major versions:\n* Cluster operator default-operator-1 should not be upgraded between minor or major versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor or major versions: RandomReason2: some random reason 2 why upgrades are not safe.",
 				}},
 			},
 		},
@@ -3189,12 +3189,12 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					Type:    configv1.OperatorUpgradeable,
 					Status:  configv1.ConditionFalse,
 					Reason:  "MultipleReasons",
-					Message: "Cluster should not be upgraded between minor versions for multiple reasons: ClusterVersionOverridesSet,ClusterOperatorsNotUpgradeable\n* Disabling ownership via cluster version overrides prevents upgrades. Please remove overrides before continuing.\n* Multiple cluster operators should not be upgraded between minor versions:\n* Cluster operator default-operator-1 should not be upgraded between minor versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor versions: RandomReason2: some random reason 2 why upgrades are not safe.",
+					Message: "Cluster should not be upgraded between minor or major versions for multiple reasons: ClusterVersionOverridesSet,ClusterOperatorsNotUpgradeable\n* Disabling ownership via cluster version overrides prevents upgrades. Please remove overrides before continuing.\n* Multiple cluster operators should not be upgraded between minor or major versions:\n* Cluster operator default-operator-1 should not be upgraded between minor or major versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor or major versions: RandomReason2: some random reason 2 why upgrades are not safe.",
 				}, {
 					Type:    "UpgradeableClusterOperators",
 					Status:  configv1.ConditionFalse,
 					Reason:  "ClusterOperatorsNotUpgradeable",
-					Message: "Multiple cluster operators should not be upgraded between minor versions:\n* Cluster operator default-operator-1 should not be upgraded between minor versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor versions: RandomReason2: some random reason 2 why upgrades are not safe.",
+					Message: "Multiple cluster operators should not be upgraded between minor or major versions:\n* Cluster operator default-operator-1 should not be upgraded between minor or major versions: RandomReason: some random reason why upgrades are not safe.\n* Cluster operator default-operator-2 should not be upgraded between minor or major versions: RandomReason2: some random reason 2 why upgrades are not safe.",
 				}, {
 					Type:    "UpgradeableClusterVersionOverrides",
 					Status:  configv1.ConditionFalse,

pkg/cvo/upgradeable.go

@@ -104,7 +104,7 @@ func (optr *Operator) setUpgradeableConditions() {
 			Type:               configv1.OperatorUpgradeable,
 			Status:             configv1.ConditionFalse,
 			Reason:             "MultipleReasons",
-			Message:            fmt.Sprintf("Cluster should not be upgraded between minor versions for multiple reasons: %s\n* %s", strings.Join(reasons, ","), strings.Join(msgs, "\n* ")),
+			Message:            fmt.Sprintf("Cluster should not be upgraded between minor or major versions for multiple reasons: %s\n* %s", strings.Join(reasons, ","), strings.Join(msgs, "\n* ")),
 			LastTransitionTime: now,
 		})
 	} else {
@@ -176,7 +176,7 @@ func (optr *Operator) getUpgradeable() *upgradeable {
 }
 
 type upgradeableCheck interface {
-	// Check returns a not-nil condition that should be addressed before a minor level upgrade when the check fails.
+	// Check returns a not-nil condition that should be addressed before a minor or major version upgrade when the check fails.
 	Check() *configv1.ClusterOperatorStatusCondition
 }
 
@@ -217,14 +217,14 @@ func (check *clusterOperatorsUpgradeable) Check() *configv1.ClusterOperatorStatu
 	reason := ""
 	if len(notup) == 1 {
 		reason = notup[0].condition.Reason
-		msg = fmt.Sprintf("Cluster operator %s should not be upgraded between minor versions: %s", notup[0].name, notup[0].condition.Message)
+		msg = fmt.Sprintf("Cluster operator %s should not be upgraded between minor or major versions: %s", notup[0].name, notup[0].condition.Message)
 	} else {
 		reason = "ClusterOperatorsNotUpgradeable"
 		var msgs []string
 		for _, cond := range notup {
-			msgs = append(msgs, fmt.Sprintf("Cluster operator %s should not be upgraded between minor versions: %s: %s", cond.name, cond.condition.Reason, cond.condition.Message))
+			msgs = append(msgs, fmt.Sprintf("Cluster operator %s should not be upgraded between minor or major versions: %s: %s", cond.name, cond.condition.Reason, cond.condition.Message))
 		}
-		msg = fmt.Sprintf("Multiple cluster operators should not be upgraded between minor versions:\n* %s", strings.Join(msgs, "\n* "))
+		msg = fmt.Sprintf("Multiple cluster operators should not be upgraded between minor or major versions:\n* %s", strings.Join(msgs, "\n* "))
 	}
 	cond.Reason = reason
 	cond.Message = msg
@@ -302,7 +302,7 @@ func (check *clusterManifestDeleteInProgressUpgradeable) Check() *configv1.Clust
 		resources := strings.Join(deletes, ",")
 		klog.V(2).Infof("Resource deletions in progress; resources=%s", resources)
 		cond.Reason = "ResourceDeletesInProgress"
-		cond.Message = fmt.Sprintf("Cluster minor level upgrades are not allowed while resource deletions are in progress; resources=%s", resources)
+		cond.Message = fmt.Sprintf("Cluster minor or major version upgrades are not allowed while resource deletions are in progress; resources=%s", resources)
 		return cond
 	}
 	return nil

pkg/internal/constants.go

@@ -56,18 +56,18 @@ const (
 	ImplicitlyEnabledCapabilities configv1.ClusterStatusConditionType = "ImplicitlyEnabledCapabilities"
 
 	// UpgradeableAdminAckRequired is False if there is API removed from the Kubernetes API server which requires admin
-	// consideration, and thus update to the next minor version is blocked.
+	// consideration, and thus update to the next minor or major version is blocked.
 	UpgradeableAdminAckRequired configv1.ClusterStatusConditionType = "UpgradeableAdminAckRequired"
-	// UpgradeableDeletesInProgress is False if deleting resources is in progress, and thus update to the next minor
+	// UpgradeableDeletesInProgress is False if deleting resources is in progress, and thus update to the next minor or major
 	// version is blocked.
 	UpgradeableDeletesInProgress configv1.ClusterStatusConditionType = "UpgradeableDeletesInProgress"
-	// UpgradeableClusterOperators is False if something is wrong with Cluster Operators, and thus update to the next minor
+	// UpgradeableClusterOperators is False if something is wrong with Cluster Operators, and thus update to the next minor or major
 	// version is blocked.
 	UpgradeableClusterOperators configv1.ClusterStatusConditionType = "UpgradeableClusterOperators"
-	// UpgradeableClusterVersionOverrides is False if there are overrides in the Cluster Version, and thus update to the next minor
+	// UpgradeableClusterVersionOverrides is False if there are overrides in the Cluster Version, and thus update to the next minor or major
 	// version is blocked.
 	UpgradeableClusterVersionOverrides configv1.ClusterStatusConditionType = "UpgradeableClusterVersionOverrides"
 
-	// UpgradeableUpgradeInProgress is True if an update is in progress
+	// UpgradeableUpgradeInProgress is True if an update is in progress.
 	UpgradeableUpgradeInProgress configv1.ClusterStatusConditionType = "UpgradeableUpgradeInProgress"
 )

pkg/payload/precondition/clusterversion/upgradeable.go

@@ -3,6 +3,7 @@ package clusterversion
 import (
 	"context"
 	"fmt"
+	"strings"
 	"time"
 
 	"github.com/blang/semver/v4"
@@ -97,7 +98,8 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 	klog.V(4).Infof("The current version is %s parsed from %s and the target version is %s parsed from %s", currentVersion.String(), cv.Status.Desired.Version, targetVersion.String(), releaseContext.DesiredVersion)
 	patchOnly := targetVersion.Major == currentVersion.Major && targetVersion.Minor == currentVersion.Minor
 	if targetVersion.LTE(currentVersion) || patchOnly {
-		// When Upgradeable==False, a patch level update with the same minor level is allowed unless overrides are set
+		// When Upgradeable==False, a patch level update with the same minor version is allowed unless overrides are set.
+		// However, minor or major version updates are blocked when Upgradeable==False.
 		// This Upgradeable precondition is only concerned about moving forward, i.e., do not care about downgrade which is taken care of by the Rollback precondition
 		if condition := ClusterVersionOverridesCondition(cv); condition != nil {
 			klog.V(2).Infof("Retarget from %s to %s is blocked by %s: %s", currentVersion.String(), targetVersion.String(), condition.Reason, condition.Message)
@@ -107,11 +109,11 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 				Name:    pf.Name(),
 			}
 		} else {
-			if completedVersion := minorUpdateFrom(cv.Status, currentVersion); completedVersion != "" && patchOnly {
+			if completedVersion, majorOrMinor := majorOrMinorUpdateFrom(cv.Status, currentVersion); completedVersion != "" && patchOnly {
 				// This is to generate an accepted risk for the accepting case 4.y.z -> 4.y+1.z' -> 4.y+1.z''
 				return &precondition.Error{
-					Reason:             "MinorVersionClusterUpdateInProgress",
-					Message:            fmt.Sprintf("Retarget to %s while a minor level update from %s to %s is in progress", targetVersion, completedVersion, currentVersion),
+					Reason:             fmt.Sprintf("%sVersionClusterUpdateInProgress", majorOrMinor),
+					Message:            fmt.Sprintf("Retarget to %s while a %s version update from %s to %s is in progress", targetVersion, strings.ToLower(majorOrMinor), completedVersion, currentVersion),
 					Name:               pf.Name(),
 					NonBlockingWarning: true,
 				}
@@ -129,24 +131,29 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 	}
 }
 
-// minorUpdateFrom returns the version that was installed completed if a minor level upgrade is in progress
-// and the empty string otherwise
-func minorUpdateFrom(status configv1.ClusterVersionStatus, currentVersion semver.Version) string {
+// majorOrMinorUpdateFrom returns the version that was installed
+// completed if a minor or major version upgrade is in progress and the
+// empty string otherwise.  It also returns "Major", "Minor", or "" to name
+// the transition.
+func majorOrMinorUpdateFrom(status configv1.ClusterVersionStatus, currentVersion semver.Version) (string, string) {
 	completedVersion := GetCurrentVersion(status.History)
 	if completedVersion == "" {
-		return ""
+		return "", ""
 	}
 	v, err := semver.Parse(completedVersion)
 	if err != nil {
-		return ""
+		return "", ""
 	}
 	if cond := resourcemerge.FindOperatorStatusCondition(status.Conditions, configv1.OperatorProgressing); cond != nil &&
-		cond.Status == configv1.ConditionTrue &&
-		v.Major == currentVersion.Major &&
-		v.Minor < currentVersion.Minor {
-		return completedVersion
+		cond.Status == configv1.ConditionTrue {
+		if v.Major < currentVersion.Major {
+			return completedVersion, "Major"
+		}
+		if v.Major == currentVersion.Major && v.Minor < currentVersion.Minor {
+			return completedVersion, "Minor"
+		}
 	}
-	return ""
+	return "", ""
 }
 
 // Name returns the name of the precondition.

pkg/payload/precondition/clusterversion/upgradeable_test.go

@@ -136,10 +136,23 @@ func TestUpgradeableRun(t *testing.T) {
 			expected: &precondition.Error{
 				NonBlockingWarning: true,
 				Name:               "ClusterVersionUpgradeable",
-				Message:            "Retarget to 4.2.3 while a minor level update from 4.1.1 to 4.2.1 is in progress",
+				Message:            "Retarget to 4.2.3 while a minor version update from 4.1.1 to 4.2.1 is in progress",
 				Reason:             "MinorVersionClusterUpdateInProgress",
 			},
 		},
+		{
+			name:             "move-x-then-z, with false condition",
+			upgradeable:      ptr(configv1.ConditionFalse),
+			completedVersion: "4.1.1",
+			currVersion:      "5.0.1",
+			desiredVersion:   "5.0.3",
+			expected: &precondition.Error{
+				NonBlockingWarning: true,
+				Name:               "ClusterVersionUpgradeable",
+				Message:            "Retarget to 5.0.3 while a major version update from 4.1.1 to 5.0.1 is in progress",
+				Reason:             "MajorVersionClusterUpdateInProgress",
+			},
+		},
 		{
 			name:             "move-z-then-z, with false condition",
 			upgradeable:      ptr(configv1.ConditionFalse),