Merge pull request #2017 from wking/oc-adm-upgrade-recommend-accept

OTA-1532: pkg/cli/admin/upgrade/recommend: New, feature-gated --accept

Author: openshift-merge-bot[bot]

pkg/cli/admin/upgrade/recommend/alerts.go

@@ -18,15 +18,18 @@ import (
 // Conditions that are True for happy signals, False for sad signals,
 // and Unknown when we do not have enough information to make a
 // happy-or-sad determination.
-func (o *options) alerts(ctx context.Context) ([]metav1.Condition, error) {
+func (o *options) alerts(ctx context.Context) ([]acceptableCondition, error) {
 	var alertsBytes []byte
 	if o.mockData.alertsPath != "" {
 		if len(o.mockData.alerts) == 0 {
-			return []metav1.Condition{{
-				Type:    "recommended/Alert",
-				Status:  metav1.ConditionUnknown,
-				Reason:  "NoTestData",
-				Message: fmt.Sprintf("This --mock-clusterversion run did not have alert data available at %v", o.mockData.alertsPath),
+			return []acceptableCondition{{
+				Condition: metav1.Condition{
+					Type:    "recommended/Alert",
+					Status:  metav1.ConditionUnknown,
+					Reason:  "NoTestData",
+					Message: fmt.Sprintf("This --mock-clusterversion run did not have alert data available at %v", o.mockData.alertsPath),
+				},
+				acceptanceName: "AlertNoTestData",
 			}}, nil
 		}
 		alertsBytes = o.mockData.alerts
@@ -51,7 +54,7 @@ func (o *options) alerts(ctx context.Context) ([]metav1.Condition, error) {
 		return nil, fmt.Errorf("parsing alerts: %w", err)
 	}
 
-	var conditions []metav1.Condition
+	var conditions []acceptableCondition
 	i := 0
 	haveCritical := false
 	havePodDisruptionBudget := false
@@ -100,11 +103,14 @@ func (o *options) alerts(ctx context.Context) ([]metav1.Condition, error) {
 				// ideally the upstream PodDisruptionBudget*Limit alerts templated in the namespace and PDB, but until they do, inject those ourselves
 				details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
 			}
-			conditions = append(conditions, metav1.Condition{
-				Type:    fmt.Sprintf("recommended/CriticalAlerts/%s/%d", alertName, i),
-				Status:  metav1.ConditionFalse,
-				Reason:  fmt.Sprintf("Alert:%s", alert.State),
-				Message: fmt.Sprintf("%s alert %s %s, suggesting significant cluster issues worth investigating. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+			conditions = append(conditions, acceptableCondition{
+				Condition: metav1.Condition{
+					Type:    fmt.Sprintf("recommended/CriticalAlerts/%s/%d", alertName, i),
+					Status:  metav1.ConditionFalse,
+					Reason:  fmt.Sprintf("Alert:%s", alert.State),
+					Message: fmt.Sprintf("%s alert %s %s, suggesting significant cluster issues worth investigating. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+				},
+				acceptanceName: alertName,
 			})
 			i += 1
 			continue
@@ -114,75 +120,84 @@ func (o *options) alerts(ctx context.Context) ([]metav1.Condition, error) {
 			havePodDisruptionBudget = true
 			// ideally the upstream PodDisruptionBudget*Limit alerts templated in the namespace and PDB, but until they do, inject those ourselves
 			details = fmt.Sprintf("Namespace=%s, PodDisruptionBudget=%s. %s", alert.Labels.Namespace, alert.Labels.PodDisruptionBudget, details)
-			conditions = append(conditions, metav1.Condition{
-				Type:    fmt.Sprintf("recommended/PodDisruptionBudgetAlerts/%s/%d", alertName, i),
-				Status:  metav1.ConditionFalse,
-				Reason:  fmt.Sprintf("Alert:%s", alert.State),
-				Message: fmt.Sprintf("%s alert %s %s, which might slow node drains. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+			conditions = append(conditions, acceptableCondition{
+				Condition: metav1.Condition{
+					Type:    fmt.Sprintf("recommended/PodDisruptionBudgetAlerts/%s/%d", alertName, i),
+					Status:  metav1.ConditionFalse,
+					Reason:  fmt.Sprintf("Alert:%s", alert.State),
+					Message: fmt.Sprintf("%s alert %s %s, which might slow node drains. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+				},
+				acceptanceName: alertName,
 			})
 			i += 1
 			continue
 		}
 
 		if alertName == "KubeContainerWaiting" {
 			havePullWaiting = true
-			conditions = append(conditions, metav1.Condition{
-				Type:    fmt.Sprintf("recommended/PodImagePullAlerts/%s/%d", alertName, i),
-				Status:  metav1.ConditionFalse,
-				Reason:  fmt.Sprintf("Alert:%s", alert.State),
-				Message: fmt.Sprintf("%s alert %s %s, which may slow workload redistribution during rolling node updates. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+			conditions = append(conditions, acceptableCondition{
+				Condition: metav1.Condition{
+					Type:    fmt.Sprintf("recommended/PodImagePullAlerts/%s/%d", alertName, i),
+					Status:  metav1.ConditionFalse,
+					Reason:  fmt.Sprintf("Alert:%s", alert.State),
+					Message: fmt.Sprintf("%s alert %s %s, which may slow workload redistribution during rolling node updates. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+				},
+				acceptanceName: alertName,
 			})
 			i += 1
 			continue
 		}
 
 		if alertName == "KubeNodeNotReady" || alertName == "KubeNodeReadinessFlapping" || alertName == "KubeNodeUnreachable" {
 			haveNodes = true
-			conditions = append(conditions, metav1.Condition{
-				Type:    fmt.Sprintf("recommended/NodeAlerts/%s/%d", alertName, i),
-				Status:  metav1.ConditionFalse,
-				Reason:  fmt.Sprintf("Alert:%s", alert.State),
-				Message: fmt.Sprintf("%s alert %s %s, which may slow workload redistribution during rolling node updates. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+			conditions = append(conditions, acceptableCondition{
+				Condition: metav1.Condition{
+					Type:    fmt.Sprintf("recommended/NodeAlerts/%s/%d", alertName, i),
+					Status:  metav1.ConditionFalse,
+					Reason:  fmt.Sprintf("Alert:%s", alert.State),
+					Message: fmt.Sprintf("%s alert %s %s, which may slow workload redistribution during rolling node updates. %s", alert.Labels.Severity, alert.Labels.AlertName, alert.State, details),
+				},
+				acceptanceName: alertName,
 			})
 			i += 1
 			continue
 		}
 	}
 
 	if !haveCritical {
-		conditions = append(conditions, metav1.Condition{
+		conditions = append(conditions, acceptableCondition{Condition: metav1.Condition{
 			Type:    "recommended/CriticalAlerts",
 			Status:  metav1.ConditionTrue,
 			Reason:  "AsExpected",
 			Message: "No critical alerts firing.",
-		})
+		}})
 	}
 
 	if !havePodDisruptionBudget {
-		conditions = append(conditions, metav1.Condition{
+		conditions = append(conditions, acceptableCondition{Condition: metav1.Condition{
 			Type:    "recommended/PodDisruptionBudgetAlerts",
 			Status:  metav1.ConditionTrue,
 			Reason:  "AsExpected",
 			Message: "No PodDisruptionBudget alerts firing.",
-		})
+		}})
 	}
 
 	if !havePullWaiting {
-		conditions = append(conditions, metav1.Condition{
+		conditions = append(conditions, acceptableCondition{Condition: metav1.Condition{
 			Type:    "recommended/PodImagePullAlerts",
 			Status:  metav1.ConditionTrue,
 			Reason:  "AsExpected",
 			Message: "No Pod container image pull alerts firing.",
-		})
+		}})
 	}
 
 	if !haveNodes {
-		conditions = append(conditions, metav1.Condition{
+		conditions = append(conditions, acceptableCondition{Condition: metav1.Condition{
 			Type:    "recommended/NodeAlerts",
 			Status:  metav1.ConditionTrue,
 			Reason:  "AsExpected",
 			Message: "No Node alerts firing.",
-		})
+		}})
 	}
 
 	return conditions, nil

pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-not-recommended.version-4.12.51-output

@@ -15,3 +15,5 @@ Reason: MultipleReasons
 Message: An unintended reversion to the default kubelet nodeStatusReportFrequency can cause significant load on the control plane. https://issues.redhat.com/browse/MCO-1094
 
   After rebooting into kernel-4.18.0-372.88.1.el8_6 or later, kernel nodes experience high load average and io_wait times. The nodes might fail to start or stop pods and probes may fail. Workload and host processes may become unresponsive and workload may be disrupted. https://issues.redhat.com/browse/COS-2705
+
+error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk

pkg/cli/admin/upgrade/recommend/examples/4.12.16-longest-recommended.version-4.12.51-output

@@ -15,3 +15,5 @@ Reason: MultipleReasons
 Message: An unintended reversion to the default kubelet nodeStatusReportFrequency can cause significant load on the control plane. https://issues.redhat.com/browse/MCO-1094
 
   After rebooting into kernel-4.18.0-372.88.1.el8_6 or later, kernel nodes experience high load average and io_wait times. The nodes might fail to start or stop pods and probes may fail. Workload and host processes may become unresponsive and workload may be disrupted. https://issues.redhat.com/browse/COS-2705
+
+error: issues that apply to this cluster but which were not included in --accept: AlertNoTestData,ConditionalUpdateRisk

pkg/cli/admin/upgrade/recommend/examples/4.16.27-degraded-monitoring.version-4.16.32-output

@@ -23,3 +23,5 @@ Channel: candidate-4.16 (available channels: candidate-4.16, candidate-4.17, can
 Update to 4.16.32 has no known issues relevant to this cluster.
 Image: quay.io/openshift-release-dev/ocp-release@sha256:0e71cb61694473b40e8d95f530eaf250a62616debb98199f31b4034808687dae
 Release URL: https://access.redhat.com/errata/RHSA-2025:0650
+
+error: issues that apply to this cluster but which were not included in --accept: ClusterOperatorDown,Failing,PodDisruptionBudgetAtLimit

pkg/cli/admin/upgrade/recommend/precheck.go

@@ -7,14 +7,28 @@ import (
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 )
 
-type check func(ctx context.Context) ([]metav1.Condition, error)
+// acceptableCondition extends metav1.Condition and a risk-acceptance
+// name, to give users a way to identify unhappy conditions they are
+// willing to accept.
+type acceptableCondition struct {
+	metav1.Condition
 
-// precheck runs a set of Condition-generating checkers, and
-// aggregates their results.  The Conditions must use True for happy
+	// acceptanceName should be set for Status != True
+	// conditions, to give users a way to identify unhappy conditions
+	// they are willing to accept.  The value does not need to be
+	// unique among conditions, for example, multiple alerts with
+	// the same name could all use the same risk acceptance name.
+	acceptanceName string
+}
+
+type check func(ctx context.Context) ([]acceptableCondition, error)
+
+// precheck runs a set of condition-generating checkers, and
+// aggregates their results.  The conditions must use True for happy
 // signals, False for sad signals, and Unknown when we do not have enough
 // information to make a happy-or-sad determination.
-func (o *options) precheck(ctx context.Context) ([]metav1.Condition, error) {
-	var conditions []metav1.Condition
+func (o *options) precheck(ctx context.Context) ([]acceptableCondition, error) {
+	var conditions []acceptableCondition
 	var errs []error
 	for _, check := range []check{
 		o.alerts,

pkg/cli/admin/upgrade/recommend/recommend.go

@@ -14,6 +14,7 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/util/sets"
 	"k8s.io/cli-runtime/pkg/genericiooptions"
 	"k8s.io/client-go/rest"
 	kcmdutil "k8s.io/kubectl/pkg/cmd/util"
@@ -61,6 +62,10 @@ func New(f kcmdutil.Factory, streams genericiooptions.IOStreams) *cobra.Command
 	flags.BoolVar(&o.showOutdatedReleases, "show-outdated-releases", o.showOutdatedReleases, "Display additional older releases.  These releases may be exposed to known issues which have been fixed in more recent releases.  But all updates will contain fixes not present in your current release.")
 	flags.StringVar(&o.rawVersion, "version", o.rawVersion, "Select a particular target release to display by version.")
 
+	if kcmdutil.FeatureGate("OC_ENABLE_CMD_UPGRADE_RECOMMEND_ACCEPT").IsEnabled() {
+		flags.StringSliceVar(&o.accept, "accept", o.accept, "Comma-delimited names for issues that you find acceptable.  With --version, any unaccepted issues will result in a non-zero exit code.")
+	}
+
 	// TODO: We can remove this flag once the idea about `oc adm upgrade recommend` stabilizes and the command
 	//       is promoted out of the OC_ENABLE_CMD_UPGRADE_RECOMMEND feature gate
 	flags.StringVar(&o.mockData.cvPath, "mock-clusterversion", "", "Path to a YAML ClusterVersion object to use for testing (will be removed later).")
@@ -81,6 +86,9 @@ type options struct {
 	// version is the parsed form of rawVersion.  Consumers after options.Complete should prefer this property.
 	version *semver.Version
 
+	// accept is a slice of acceptable issue names.
+	accept []string
+
 	RESTConfig *rest.Config
 	Client     configv1client.Interface
 }
@@ -125,6 +133,9 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string
 }
 
 func (o *options) Run(ctx context.Context) error {
+	issues := sets.New[string]()
+	accept := sets.New[string](o.accept...)
+
 	var cv *configv1.ClusterVersion
 	if cv = o.mockData.clusterVersion; cv == nil {
 		var err error
@@ -139,34 +150,60 @@ func (o *options) Run(ctx context.Context) error {
 
 	if c := findClusterOperatorStatusCondition(cv.Status.Conditions, clusterStatusFailing); c != nil {
 		if c.Status != configv1.ConditionFalse {
-			fmt.Fprintf(o.Out, "%s=%s:\n\n  Reason: %s\n  Message: %s\n\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n  "))
+			acceptContext := ""
+			if accept.Has(string(clusterStatusFailing)) {
+				acceptContext = "accepted "
+			}
+			fmt.Fprintf(o.Out, "%s%s=%s:\n\n  Reason: %s\n  Message: %s\n\n", acceptContext, c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n  "))
+			issues.Insert(string(clusterStatusFailing))
 		}
 	} else {
-		fmt.Fprintf(o.ErrOut, "warning: No current %s info, see `oc describe clusterversion` for more details.\n", clusterStatusFailing)
+		acceptContext := ""
+		if accept.Has(string(clusterStatusFailing)) {
+			acceptContext = "accepted "
+		}
+		fmt.Fprintf(o.ErrOut, "%swarning: No current %s info, see `oc describe clusterversion` for more details.\n", acceptContext, clusterStatusFailing)
+		issues.Insert(string(clusterStatusFailing))
 	}
 
 	if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorProgressing); c != nil && c.Status == configv1.ConditionTrue && len(c.Message) > 0 {
-		fmt.Fprintf(o.Out, "info: An update is in progress.  You may wish to let this update complete before requesting a new update.\n  %s\n\n", strings.ReplaceAll(c.Message, "\n", "\n  "))
+		acceptContext := ""
+		if accept.Has(string(configv1.OperatorProgressing)) {
+			acceptContext = "accepted "
+		}
+		fmt.Fprintf(o.Out, "%sinfo: An update is in progress.  You may wish to let this update complete before requesting a new update.\n  %s\n\n", acceptContext, strings.ReplaceAll(c.Message, "\n", "\n  "))
+		issues.Insert(string(configv1.OperatorProgressing))
 	}
 
 	if o.precheckEnabled {
 		conditions, err := o.precheck(ctx)
 		if err != nil {
 			fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err)
+			issues.Insert("FailedToCompletePrecheck")
 		}
 		var happyConditions []string
+		var acceptedConditions []string
 		var unhappyConditions []string
 		for _, condition := range conditions {
 			if condition.Status == metav1.ConditionTrue {
 				happyConditions = append(happyConditions, fmt.Sprintf("%s (%s)", condition.Type, condition.Reason))
 			} else {
-				unhappyConditions = append(unhappyConditions, condition.Type)
+				issues.Insert(condition.acceptanceName)
+				if accept.Has(condition.acceptanceName) {
+					acceptedConditions = append(acceptedConditions, condition.Type)
+				} else {
+					unhappyConditions = append(unhappyConditions, condition.Type)
+				}
 			}
 		}
 		if len(happyConditions) > 0 {
 			sort.Strings(happyConditions)
 			fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(happyConditions, ", "))
 		}
+		if len(acceptedConditions) > 0 {
+			sort.Strings(acceptedConditions)
+			fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases, but were explicitly accepted via --accept: %s\n\n", strings.Join(acceptedConditions, ", "))
+		}
 		if len(unhappyConditions) > 0 {
 			sort.Strings(unhappyConditions)
 			fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n\n", strings.Join(unhappyConditions, ", "))
@@ -261,7 +298,18 @@ func (o *options) Run(ctx context.Context) error {
 					if c := notRecommendedCondition(update); c == nil {
 						fmt.Fprintf(o.Out, "Update to %s has no known issues relevant to this cluster.\nImage: %s\nRelease URL: %s\n", update.Release.Version, update.Release.Image, update.Release.URL)
 					} else {
-						fmt.Fprintf(o.Out, "Update to %s %s=%s:\nImage: %s\nRelease URL: %s\nReason: %s\nMessage: %s\n", update.Release.Version, c.Type, c.Status, update.Release.Image, update.Release.URL, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n  "))
+						acceptContext := ""
+						if accept.Has("ConditionalUpdateRisk") {
+							acceptContext = "accepted "
+						}
+						fmt.Fprintf(o.Out, "Update to %s %s=%s:\nImage: %s\nRelease URL: %s\nReason: %s%s\nMessage: %s\n", update.Release.Version, c.Type, c.Status, update.Release.Image, update.Release.URL, acceptContext, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n  "))
+						issues.Insert("ConditionalUpdateRisk")
+					}
+					unaccepted := issues.Difference(accept)
+					if unaccepted.Len() > 0 {
+						return fmt.Errorf("issues that apply to this cluster but which were not included in --accept: %s", strings.Join(sets.List(unaccepted), ","))
+					} else if issues.Len() > 0 {
+						fmt.Fprintf(o.Out, "Update to %s has no known issues relevant to this cluster other than the accepted %s.\n", update.Release.Version, strings.Join(sets.List(issues), ","))
 					}
 					return nil
 				}