OTA-861: Set Upgradeable=False when there is an upgrade in progress (#1080)

* inhibit the 2nd minor version upgrade

* Block Y stream upgrade if any upgrade is in progress

This commits entends the guard on the 2nd Y-stream upgrade,
i.e., blocking an Y-stream upgrade if there is already an
Y-stream upgrade in progress,
to the guard on any Y-stream upgrade if there is already an
upgrade in progress, reguardless of Y-stream or Z-stream.

For example, it covers the case 4.14.15-> 4.14.35 -> 4.15.29
where the upgrade 4.14.35 -> 4.15.29 is blocked until the upgrade
4.14.15-> 4.14.35 completes.

Note that we still allow for upgrade to 4.y+1.z''
in the middle of upgrade 4.y.z -> 4.y+1.z', even though direct upgrade
4.y.z -> 4.y+1.z'' might not be supported.
This is because the ugprade 4.y.z -> 4.y+1.z' might not be completed
up to a bug in 4.y+1.z' that has a fix in 4.(y+1).z''.
We need the retarget to it to land 4.y+1 on the cluster.

* Generate accepted risks in an unblocking case

This commit generates a message in the accepted risks
for the unblocking case 4.y.z -> 4.y+1.z' -> 4.y+1.z''.

* Use Progressing=True to block minor level upgrade

* Drop the guard on UpgradeInProgress

The guard will be added on another pull request.

* Make the type UpgradeableUpgradeInProgress private

* Improve condition's reason and message

Author: hongkailiu

pkg/cvo/cvo.go

@@ -449,7 +449,9 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co
 	resultChannelCount++
 	go func() {
 		defer utilruntime.HandleCrash()
-		wait.UntilWithContext(runContext, func(runContext context.Context) { optr.worker(runContext, optr.upgradeableQueue, optr.upgradeableSync) }, time.Second)
+		wait.UntilWithContext(runContext, func(runContext context.Context) {
+			optr.worker(runContext, optr.upgradeableQueue, optr.upgradeableSyncFunc(false))
+		}, time.Second)
 		resultChannel <- asyncResult{name: "upgradeable"}
 	}()
 
@@ -459,6 +461,10 @@ func (optr *Operator) Run(runContext context.Context, shutdownContext context.Co
 		wait.UntilWithContext(runContext, func(runContext context.Context) {
 			// run the worker, then when the queue is closed sync one final time to flush any pending status
 			optr.worker(runContext, optr.queue, func(runContext context.Context, key string) error { return optr.sync(runContext, key) })
+			// This is to ensure upgradeableCondition to be synced and thus to avoid the race caused by the throttle
+			if err := optr.upgradeableSyncFunc(true)(shutdownContext, optr.queueKey()); err != nil {
+				utilruntime.HandleError(fmt.Errorf("unable to perform final upgradeable sync: %v", err))
+			}
 			if err := optr.sync(shutdownContext, optr.queueKey()); err != nil {
 				utilruntime.HandleError(fmt.Errorf("unable to perform final sync: %v", err))
 			}
@@ -743,27 +749,29 @@ func (optr *Operator) availableUpdatesSync(ctx context.Context, key string) erro
 	return optr.syncAvailableUpdates(ctx, config)
 }
 
-// upgradeableSync is triggered on cluster version change (and periodic requeues) to
+// upgradeableSyncFunc returns a function that is triggered on cluster version change (and periodic requeues) to
 // sync upgradeableCondition. It only modifies cluster version.
-func (optr *Operator) upgradeableSync(_ context.Context, key string) error {
-	startTime := time.Now()
-	klog.V(2).Infof("Started syncing upgradeable %q", key)
-	defer func() {
-		klog.V(2).Infof("Finished syncing upgradeable %q (%v)", key, time.Since(startTime))
-	}()
+func (optr *Operator) upgradeableSyncFunc(ignoreThrottlePeriod bool) func(_ context.Context, key string) error {
+	return func(_ context.Context, key string) error {
+		startTime := time.Now()
+		klog.V(2).Infof("Started syncing upgradeable %q", key)
+		defer func() {
+			klog.V(2).Infof("Finished syncing upgradeable %q (%v)", key, time.Since(startTime))
+		}()
 
-	config, err := optr.cvLister.Get(optr.name)
-	if apierrors.IsNotFound(err) {
-		return nil
-	}
-	if err != nil {
-		return err
-	}
-	if errs := validation.ValidateClusterVersion(config); len(errs) > 0 {
-		return nil
-	}
+		config, err := optr.cvLister.Get(optr.name)
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		if err != nil {
+			return err
+		}
+		if errs := validation.ValidateClusterVersion(config); len(errs) > 0 {
+			return nil
+		}
 
-	return optr.syncUpgradeable(config)
+		return optr.syncUpgradeable(config, ignoreThrottlePeriod)
+	}
 }
 
 // isOlderThanLastUpdate returns true if the cluster version is older than

pkg/cvo/cvo_test.go

@@ -3868,7 +3868,7 @@ func TestOperator_upgradeableSync(t *testing.T) {
 					waitForCm(t, cms)
 				}
 
-				err = optr.upgradeableSync(ctx, optr.queueKey())
+				err = optr.upgradeableSyncFunc(false)(ctx, optr.queueKey())
 				if err != nil && tt.wantErr == nil {
 					t.Fatalf("Operator.sync() unexpected error: %v", err)
 				}

pkg/cvo/upgradeable.go

@@ -61,8 +61,8 @@ func defaultUpgradeableCheckIntervals() upgradeableCheckIntervals {
 
 // syncUpgradeable synchronizes the upgradeable status only if the sufficient time passed since its last update. This
 // throttling period is dynamic and is driven by upgradeableCheckIntervals.
-func (optr *Operator) syncUpgradeable(cv *configv1.ClusterVersion) error {
-	if u := optr.getUpgradeable(); u != nil {
+func (optr *Operator) syncUpgradeable(cv *configv1.ClusterVersion, ignoreThrottlePeriod bool) error {
+	if u := optr.getUpgradeable(); u != nil && !ignoreThrottlePeriod {
 		throttleFor := optr.upgradeableCheckIntervals.throttlePeriod(cv)
 		if earliestNext := u.At.Add(throttleFor); time.Now().Before(earliestNext) {
 			klog.V(2).Infof("Upgradeability last checked %s ago, will not re-check until %s", time.Since(u.At), earliestNext.Format(time.RFC3339))
@@ -176,7 +176,7 @@ func (optr *Operator) getUpgradeable() *upgradeable {
 }
 
 type upgradeableCheck interface {
-	// returns a not-nil condition when the check fails.
+	// Check returns a not-nil condition that should be addressed before a minor level upgrade when the check fails.
 	Check() *configv1.ClusterOperatorStatusCondition
 }
 
@@ -262,6 +262,34 @@ func (check *clusterVersionOverridesUpgradeable) Check() *configv1.ClusterOperat
 	return cond
 }
 
+type upgradeInProgressUpgradeable struct {
+	name     string
+	cvLister configlistersv1.ClusterVersionLister
+}
+
+func (check *upgradeInProgressUpgradeable) Check() *configv1.ClusterOperatorStatusCondition {
+	cond := &configv1.ClusterOperatorStatusCondition{
+		Type:   "UpgradeableUpgradeInProgress",
+		Status: configv1.ConditionTrue,
+	}
+
+	cv, err := check.cvLister.Get(check.name)
+	if meta.IsNoMatchError(err) || apierrors.IsNotFound(err) {
+		return nil
+	} else if err != nil {
+		klog.Error(err)
+		return nil
+	}
+
+	if progressingCondition := resourcemerge.FindOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); progressingCondition != nil &&
+		progressingCondition.Status == configv1.ConditionTrue {
+		cond.Reason = "UpdateInProgress"
+		cond.Message = fmt.Sprintf("An update is already in progress and the details are in the %s condition", configv1.OperatorProgressing)
+		return cond
+	}
+	return nil
+}
+
 type clusterManifestDeleteInProgressUpgradeable struct {
 }
 
@@ -420,6 +448,7 @@ func (optr *Operator) defaultUpgradeableChecks() []upgradeableCheck {
 		},
 		&clusterOperatorsUpgradeable{coLister: optr.coLister},
 		&clusterManifestDeleteInProgressUpgradeable{},
+		&upgradeInProgressUpgradeable{name: optr.name, cvLister: optr.cvLister},
 	}
 }
 

pkg/cvo/upgradeable_test.go

@@ -4,7 +4,9 @@ import (
 	"testing"
 	"time"
 
+	"github.com/google/go-cmp/cmp"
 	configv1 "github.com/openshift/api/config/v1"
+	"github.com/openshift/client-go/config/clientset/versioned/fake"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
@@ -57,3 +59,89 @@ func TestUpgradeableCheckIntervalsThrottlePeriod(t *testing.T) {
 		})
 	}
 }
+
+func TestUpgradeInProgressUpgradeable(t *testing.T) {
+	testCases := []struct {
+		name     string
+		cv       *configv1.ClusterVersion
+		expected *configv1.ClusterOperatorStatusCondition
+	}{
+		{
+			name: "empty conditions",
+			cv: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "unit-test",
+				},
+			},
+		},
+		{
+			name: "progressing is true",
+			cv: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "unit-test",
+				},
+				Status: configv1.ClusterVersionStatus{
+					Conditions: []configv1.ClusterOperatorStatusCondition{
+						{
+							Type:    configv1.OperatorProgressing,
+							Status:  configv1.ConditionTrue,
+							Reason:  "UpdateInProgress",
+							Message: "An update is already in progress and the details are in the Progressing condition",
+						},
+					},
+				},
+			},
+			expected: &configv1.ClusterOperatorStatusCondition{
+				Type:    "UpgradeableUpgradeInProgress",
+				Status:  configv1.ConditionTrue,
+				Reason:  "UpdateInProgress",
+				Message: "An update is already in progress and the details are in the Progressing condition",
+			},
+		},
+		{
+			name: "progressing is false",
+			cv: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "unit-test",
+				},
+				Status: configv1.ClusterVersionStatus{
+					Conditions: []configv1.ClusterOperatorStatusCondition{
+						{
+							Type:    configv1.OperatorProgressing,
+							Status:  configv1.ConditionFalse,
+							Reason:  "a",
+							Message: "b",
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "progressing has no status",
+			cv: &configv1.ClusterVersion{
+				ObjectMeta: metav1.ObjectMeta{
+					Name: "unit-test",
+				},
+				Status: configv1.ClusterVersionStatus{
+					Conditions: []configv1.ClusterOperatorStatusCondition{
+						{
+							Type:    configv1.OperatorProgressing,
+							Reason:  "a",
+							Message: "b",
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			client := fake.NewSimpleClientset(tc.cv)
+			unit := upgradeInProgressUpgradeable{name: "unit-test", cvLister: &clientCVLister{client: client}}
+			actual := unit.Check()
+			if diff := cmp.Diff(tc.expected, actual); diff != "" {
+				t.Errorf("%s differs from expected:\n%s", tc.name, diff)
+			}
+		})
+	}
+}