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

[1] is suggesting ClusterVersion API changes to make it easier for
cluster administrators to manage many clusters and updates vs. a more
limited set of update issues.  One example scenario:

1. A cluster admin gets conditional updates of a cluster and evaluates
   the issues in the conditional updates.
2. After evaluation, the admin decides that some issues can be accepted
   on all or some of the managed clusters.
3. The administrator runs the 'recommend' command with '--accept A,B,C
   --version 4.y.z' to see if any other issues turn up for that update
   on that cluster.  Nonzero exit code says more investigation is
   needed, some subset of which will be "additional issue D to
   assess".

Besides making life easier for admins with lots of clusters
considering one A->B update, this option will make life easier for
admins with one cluster that updates multiple hops all exposed to a
single issue (e.g. if PodDisruptionBudgetAtLimit becomes a general
issue, but the admin never wants to resolve that pre-update, they can
accept it for multiple updates in a single cluster).

The exit code handling is only tied to --version.

When listing multiple update target options, the admin is asking for
help in chosing an update target, and that cannot be bundled up into
an exit code; they'll have to read the report to get enough context to
decide.  And there may be some that are acceptable and some that have
unaccepted issues, and trying to bundle that all up into a single exit
code could go a few ways (exit non-zero if any choice has unaccepted
issues?  Exit 0 if any choice is acceptable, even if other choices have
unaccepted issues?), and it seems like folks could misinterpret
whichever approach we took and be surprised.  Exiting zero on "we
succesfully compiled this report" forces the admin to read the report
to extract any value, and that reduces the chance that they
misinterpret what we were trying to say.

[1]: https://issues.redhat.com/browse/OCPSTRAT-2118

Author: wking

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
 				}