OCPBUGS-42880: Upgradeable=False should not block a 4.(y+1).z to 4.(y…

…+1).z' retarget (#1094) * OCPBUGS-42880: Upgradeable=False should not block a 4.(y+1).z to 4.(y+1).z' retarget * Update pkg/payload/precondition/clusterversion/upgradeable.go Co-authored-by: Petr Muller <[email protected]> --------- Co-authored-by: Petr Muller <[email protected]>
openshift · Oct 20, 2024 · adf0cf5 · adf0cf5
1 parent e546515
commit adf0cf5
Show file tree

Hide file tree

Showing 5 changed files with 166 additions and 127 deletions.
diff --git a/pkg/cvo/status_history.go b/pkg/cvo/status_history.go
@@ -215,3 +215,21 @@ 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 ""
+}
diff --git a/pkg/cvo/status_history_test.go b/pkg/cvo/status_history_test.go
@@ -775,3 +775,41 @@ func Test_sameZStreamVersion(t *testing.T) {
 		})
 	}
 }
+
+func TestGetEffectiveMinor(t *testing.T) {
+	tests := []struct {
+		name     string
+		input    string
+		expected string
+	}{
+		{
+			name:     "empty",
+			input:    "",
+			expected: "",
+		},
+		{
+			name:     "invalid",
+			input:    "something@very-differe",
+			expected: "",
+		},
+		{
+			name:     "multidot",
+			input:    "v4.7.12.3+foo",
+			expected: "7",
+		},
+		{
+			name:     "single",
+			input:    "v4.7",
+			expected: "7",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			actual := getEffectiveMinor(tc.input)
+			if tc.expected != actual {
+				t.Error(actual)
+			}
+		})
+	}
+}
diff --git a/pkg/cvo/upgradeable.go b/pkg/cvo/upgradeable.go
@@ -23,7 +23,6 @@ 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 (
@@ -288,8 +287,8 @@ func gateApplicableToCurrentVersion(gateName string, currentVersion string) (boo
 			internal.AdminGatesConfigMap, gateName, adminAckGateFmt)
 	} else {
 		parts := strings.Split(ackVersion, "-")
-		ackMinor := clusterversion.GetEffectiveMinor(parts[1])
-		cvMinor := clusterversion.GetEffectiveMinor(currentVersion)
+		ackMinor := getEffectiveMinor(parts[1])
+		cvMinor := getEffectiveMinor(currentVersion)
 		if ackMinor == cvMinor {
 			applicable = true
 		}
@@ -338,7 +337,7 @@ func (check *clusterAdminAcksCompletedUpgradeable) Check() *configv1.ClusterOper
 			Message: message,
 		}
 	}
-	currentVersion := clusterversion.GetCurrentVersion(cv.Status.History)
+	currentVersion := 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.

diff --git a/pkg/payload/precondition/clusterversion/upgradeable.go b/pkg/payload/precondition/clusterversion/upgradeable.go
@@ -2,18 +2,17 @@ package clusterversion
 
 import (
 	"context"
-	"strconv"
-	"strings"
 	"time"
 
+	"github.com/blang/semver/v4"
 	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"
 	"k8s.io/klog/v2"
 
 	"github.com/openshift/cluster-version-operator/lib/resourcemerge"
-	precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition"
+	"github.com/openshift/cluster-version-operator/pkg/payload/precondition"
 )
 
 // Upgradeable checks if clusterversion is upgradeable currently.
@@ -74,30 +73,40 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 		return nil
 	}
 
-	// we can always allow the upgrade if there isn't a version already installed
-	if len(cv.Status.History) == 0 {
-		klog.V(2).Infof("Precondition %s passed: no release history.", pf.Name())
-		return nil
+	currentVersion, err := semver.Parse(cv.Status.Desired.Version)
+	if err != nil {
+		return &precondition.Error{
+			Nested:             err,
+			Reason:             "InvalidCurrentVersion",
+			Message:            err.Error(),
+			Name:               pf.Name(),
+			NonBlockingWarning: true, // do not block on issues that require an update to fix
+		}
 	}
 
-	currentVersion := GetCurrentVersion(cv.Status.History)
-	currentMinor := GetEffectiveMinor(currentVersion)
-	desiredMinor := GetEffectiveMinor(releaseContext.DesiredVersion)
-	klog.V(2).Infof("currentMinor %s releaseContext.DesiredVersion %s desiredMinor %s", currentMinor, releaseContext.DesiredVersion, desiredMinor)
+	targetVersion, err := semver.Parse(releaseContext.DesiredVersion)
+	if err != nil {
+		return &precondition.Error{
+			Nested:  err,
+			Reason:  "InvalidDesiredVersion",
+			Message: err.Error(),
+			Name:    pf.Name(),
+		}
+	}
 
-	// if there is no difference in the minor version (4.y.z where 4.y is the same for current and desired), then we can still upgrade
-	// if no cluster overrides have been set
-	if !minorVersionUpgrade(currentMinor, desiredMinor) {
-		klog.V(2).Infof("Precondition %q passed: minor from the target %s is not a minor version update from the current %s.%s.", pf.Name(), releaseContext.DesiredVersion, currentVersion, currentMinor)
+	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) {
+		// 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 {
-			klog.V(2).Infof("Update from %s to %s blocked by %s: %s", currentVersion, releaseContext.DesiredVersion, condition.Reason, condition.Message)
-
+			klog.V(2).Infof("Retarget from %s to %s is blocked by %s: %s", currentVersion.String(), targetVersion.String(), condition.Reason, condition.Message)
 			return &precondition.Error{
 				Reason:  condition.Reason,
 				Message: condition.Message,
 				Name:    pf.Name(),
 			}
 		} else {
+			klog.V(2).Infof("Precondition %q passed on update to %s", pf.Name(), targetVersion.String())
 			return nil
 		}
 	}
@@ -112,45 +121,3 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele
 
 // Name returns Name for 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 ""
-}
-
-// GetEffectiveMinor attempts to do a simple parse of the version provided.  If it does not parse, the value is considered
-// empty string, which works for the comparison done here for equivalence.
-func GetEffectiveMinor(version string) string {
-	splits := strings.Split(version, ".")
-	if len(splits) < 2 {
-		return ""
-	}
-	return splits[1]
-}
-
-// minorVersionUpgrade returns true if the the desired update minor version number is greater
-// than the current version minor version number. Errors resulting from either version
-// number being unset or NaN are ignored simply resulting in false returned.
-func minorVersionUpgrade(currentMinor string, desiredMinor string) bool {
-	if currentMinorNum, err := strconv.Atoi(currentMinor); err == nil {
-		if desiredMinorNum, err := strconv.Atoi(desiredMinor); err == nil {
-			if desiredMinorNum > currentMinorNum {
-				return true
-			}
-		}
-	}
-	return false
-}