Merge pull request openshift#1093 from hongkailiu/OTA-861-better-guard

OTA-861: Generate an accepted risk for Y-then-Z upgrade

Author: openshift-merge-bot[bot]

pkg/cvo/status_history.go

@@ -215,21 +215,3 @@ func getEffectiveMicro(version string) string {
 	}
 	return splits[2]
 }
-
-// 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
-// installed version, is returned. If history is empty the empty string is returned.
-func getCurrentVersion(history []configv1.UpdateHistory) string {
-	for _, h := range history {
-		if h.State == configv1.CompletedUpdate {
-			klog.V(2).Infof("Cluster current version=%s", h.Version)
-			return h.Version
-		}
-	}
-	// Empty history should only occur if method is called early in startup before history is populated.
-	if len(history) != 0 {
-		return history[len(history)-1].Version
-	}
-	return ""
-}

pkg/cvo/upgradeable.go

@@ -23,6 +23,7 @@ import (
 	"github.com/openshift/cluster-version-operator/lib/resourcedelete"
 	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
 	"github.com/openshift/cluster-version-operator/pkg/internal"
+	"github.com/openshift/cluster-version-operator/pkg/payload/precondition/clusterversion"
 )
 
 const (
@@ -365,7 +366,7 @@ func (check *clusterAdminAcksCompletedUpgradeable) Check() *configv1.ClusterOper
 			Message: message,
 		}
 	}
-	currentVersion := getCurrentVersion(cv.Status.History)
+	currentVersion := clusterversion.GetCurrentVersion(cv.Status.History)
 
 	// This can occur in early start up when the configmap is first added and version history
 	// has not yet been populated.

pkg/payload/precondition/clusterversion/upgradeable.go

@@ -2,6 +2,7 @@ package clusterversion
 
 import (
 	"context"
+	"fmt"
 	"time"
 
 	"github.com/blang/semver/v4"
@@ -93,7 +94,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)
-	if targetVersion.LTE(currentVersion) || (targetVersion.Major == currentVersion.Major && targetVersion.Minor == currentVersion.Minor) {
+	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
 		// 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 {
@@ -104,6 +106,15 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 				Name:    pf.Name(),
 			}
 		} else {
+			if completedVersion := minorUpdateFrom(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),
+					Name:               pf.Name(),
+					NonBlockingWarning: true,
+				}
+			}
 			klog.V(2).Infof("Precondition %q passed on update to %s", pf.Name(), targetVersion.String())
 			return nil
 		}
@@ -117,5 +128,43 @@ 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 {
+	completedVersion := GetCurrentVersion(status.History)
+	if completedVersion == "" {
+		return ""
+	}
+	v, err := semver.Parse(completedVersion)
+	if err != nil {
+		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
+	}
+	return ""
+}
+
 // Name returns the name of the precondition.
 func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" }
+
+// 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
+// installed version, is returned. If history is empty the empty string is returned.
+func GetCurrentVersion(history []configv1.UpdateHistory) string {
+	for _, h := range history {
+		if h.State == configv1.CompletedUpdate {
+			klog.V(2).Infof("Cluster current version=%s", h.Version)
+			return h.Version
+		}
+	}
+	// Empty history should only occur if method is called early in startup before history is populated.
+	if len(history) != 0 {
+		return history[len(history)-1].Version
+	}
+	return ""
+}

pkg/payload/precondition/clusterversion/upgradeable_test.go

@@ -2,6 +2,7 @@ package clusterversion
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"testing"
 
@@ -23,11 +24,12 @@ func TestUpgradeableRun(t *testing.T) {
 	}
 
 	tests := []struct {
-		name           string
-		upgradeable    *configv1.ConditionStatus
-		currVersion    string
-		desiredVersion string
-		expected       *precondition.Error
+		name             string
+		upgradeable      *configv1.ConditionStatus
+		completedVersion string
+		currVersion      string
+		desiredVersion   string
+		expected         *precondition.Error
 	}{
 		{
 			name:           "first",
@@ -125,6 +127,26 @@ func TestUpgradeableRun(t *testing.T) {
 			currVersion:    "4.17.0-0.test-2024-10-07-173400-ci-ln-pwn9ngk-latest",
 			desiredVersion: "4.17.0-rc.6",
 		},
+		{
+			name:             "move-y-then-z, with false condition",
+			upgradeable:      ptr(configv1.ConditionFalse),
+			completedVersion: "4.1.1",
+			currVersion:      "4.2.1",
+			desiredVersion:   "4.2.3",
+			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",
+				Reason:             "MinorVersionClusterUpdateInProgress",
+			},
+		},
+		{
+			name:             "move-z-then-z, with false condition",
+			upgradeable:      ptr(configv1.ConditionFalse),
+			completedVersion: "4.2.1",
+			currVersion:      "4.2.2",
+			desiredVersion:   "4.2.3",
+		},
 	}
 
 	for _, tc := range tests {
@@ -134,8 +156,13 @@ func TestUpgradeableRun(t *testing.T) {
 				Spec:       configv1.ClusterVersionSpec{},
 				Status: configv1.ClusterVersionStatus{
 					Desired: configv1.Release{Version: tc.currVersion},
+					History: []configv1.UpdateHistory{{State: configv1.CompletedUpdate, Version: tc.completedVersion}},
 				},
 			}
+			if tc.completedVersion != "" && tc.completedVersion != tc.currVersion {
+				clusterVersion.Status.Conditions = append(clusterVersion.Status.Conditions,
+					configv1.ClusterOperatorStatusCondition{Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue})
+			}
 			if tc.upgradeable != nil {
 				clusterVersion.Status.Conditions = append(clusterVersion.Status.Conditions, configv1.ClusterOperatorStatusCondition{
 					Type:    configv1.OperatorUpgradeable,
@@ -154,7 +181,8 @@ func TestUpgradeableRun(t *testing.T) {
 			if (tc.expected == nil && err != nil) || (tc.expected != nil && err == nil) {
 				t.Errorf("%s: expected %v but got %v", tc.name, tc.expected, err)
 			} else if tc.expected != nil && err != nil {
-				if pError, ok := err.(*precondition.Error); !ok {
+				var pError *precondition.Error
+				if !errors.As(err, &pError) {
 					t.Errorf("Failed to convert to err: %v", err)
 				} else if diff := cmp.Diff(tc.expected, pError, cmpopts.IgnoreFields(precondition.Error{}, "Nested")); diff != "" {
 					t.Errorf("%s differs from expected:\n%s", tc.name, diff)