OCPBUGS-61061: pkg/cli/admin/upgrade/recommend: Add a blank line after --version conditional risk #2045
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -76,7 +76,6 @@ type options struct { | |
|
|
||
| mockData mockData | ||
| showOutdatedReleases bool | ||
|
|
||
| // quiet configures the verbosity of output. When 'quiet' is true and 'version' is set, only print unaccepted issue names. | ||
| quiet bool | ||
|
|
@@ -134,14 +133,13 @@ func (o *options) Complete(f kcmdutil.Factory, cmd *cobra.Command, args []string | |
| } | ||
| } | ||
|
|
||
| return nil | ||
| } | ||
|
|
||
| func (o *options) Run(ctx context.Context) error { | ||
| issues := sets.New[string]() | ||
| accept := sets.New[string](o.accept...) | ||
| var previousStdout, previousStderr bool | ||
|
|
||
| var cv *configv1.ClusterVersion | ||
| if cv = o.mockData.clusterVersion; cv == nil { | ||
|
|
@@ -162,7 +160,8 @@ func (o *options) Run(ctx context.Context) error { | |
| acceptContext = "accepted " | ||
| } | ||
| if !o.quiet { | ||
| fmt.Fprintf(o.Out, "%s%s=%s:\n\n Reason: %s\n Message: %s\n", acceptContext, c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStdout = true | ||
| } | ||
| issues.Insert(string(clusterStatusFailing)) | ||
| } | ||
|
|
@@ -173,6 +172,7 @@ func (o *options) Run(ctx context.Context) error { | |
| } | ||
| if !o.quiet { | ||
| fmt.Fprintf(o.ErrOut, "%swarning: No current %s info, see `oc describe clusterversion` for more details.\n", acceptContext, clusterStatusFailing) | ||
| previousStderr = true | ||
| } | ||
| issues.Insert(string(clusterStatusFailing)) | ||
| } | ||
|
|
@@ -183,66 +183,95 @@ func (o *options) Run(ctx context.Context) error { | |
| acceptContext = "accepted " | ||
| } | ||
| if !o.quiet { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| 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", acceptContext, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStdout = true | ||
| } | ||
| issues.Insert(string(configv1.OperatorProgressing)) | ||
| } | ||
|
|
||
| conditions, err := o.precheck(ctx) | ||
| if err != nil { | ||
| if !o.quiet { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "Failed to check for at least some preconditions: %v\n", err) | ||
| previousStdout = true | ||
| } | ||
| 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 { | ||
| issues.Insert(condition.acceptanceName) | ||
| if accept.Has(condition.acceptanceName) { | ||
| acceptedConditions = append(acceptedConditions, condition.Type) | ||
| } else { | ||
| unhappyConditions = append(unhappyConditions, condition.Type) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if !o.quiet { | ||
| if len(happyConditions) > 0 { | ||
| sort.Strings(happyConditions) | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "The following conditions found no cause for concern in updating this cluster to later releases: %s\n", strings.Join(happyConditions, ", ")) | ||
| previousStdout = true | ||
| } | ||
| if len(acceptedConditions) > 0 { | ||
| sort.Strings(acceptedConditions) | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| 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", strings.Join(acceptedConditions, ", ")) | ||
| previousStdout = true | ||
| } | ||
| if len(unhappyConditions) > 0 { | ||
| sort.Strings(unhappyConditions) | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "The following conditions found cause for concern in updating this cluster to later releases: %s\n", strings.Join(unhappyConditions, ", ")) | ||
| previousStdout = true | ||
|
|
||
| for _, c := range conditions { | ||
| if c.Status != metav1.ConditionTrue { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "%s=%s:\n\n Reason: %s\n Message: %s\n", c.Type, c.Status, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStdout = true | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status != configv1.ConditionTrue { | ||
| if !o.quiet { | ||
| if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| fmt.Fprintf(o.ErrOut, "warning: Cannot refresh available updates:\n Reason: %s\n Message: %s\n", c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStderr = true | ||
| } | ||
| issues.Insert("CannotRetrieveUpdates") | ||
| } | ||
|
|
||
| if !o.quiet { | ||
| if cv.Spec.Channel != "" { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| if cv.Spec.Upstream == "" { | ||
| fmt.Fprint(o.Out, "Upstream update service is unset, so the cluster will use an appropriate default.\n") | ||
| } else { | ||
|
|
@@ -253,6 +282,7 @@ func (o *options) Run(ctx context.Context) error { | |
| } else { | ||
| fmt.Fprintf(o.Out, "Channel: %s\n", cv.Spec.Channel) | ||
| } | ||
| previousStdout = true | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -262,7 +292,11 @@ func (o *options) Run(ctx context.Context) error { | |
| version, err := semver.Parse(update.Release.Version) | ||
| if err != nil { | ||
| if !o.quiet { | ||
| if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Release.Version, err) | ||
| previousStderr = true | ||
| } | ||
| continue | ||
| } | ||
|
|
@@ -289,7 +323,11 @@ func (o *options) Run(ctx context.Context) error { | |
| version, err := semver.Parse(update.Version) | ||
| if err != nil { | ||
| if !o.quiet { | ||
| if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| fmt.Fprintf(o.ErrOut, "warning: Cannot parse SemVer available update %q: %v\n", update.Version, err) | ||
| previousStderr = true | ||
| } | ||
| continue | ||
| } | ||
|
|
@@ -306,54 +344,95 @@ func (o *options) Run(ctx context.Context) error { | |
| if c := findClusterOperatorStatusCondition(cv.Status.Conditions, configv1.OperatorUpgradeable); c != nil && c.Status == configv1.ConditionFalse { | ||
| if err := injectUpgradeableAsCondition(cv.Status.Desired.Version, c, majorMinorBuckets); err != nil && !o.quiet { | ||
| if !o.quiet { | ||
| if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| fmt.Fprintf(o.ErrOut, "warning: Cannot inject %s=%s as a conditional update risk: %s\n\nReason: %s\n Message: %s\n", c.Type, c.Status, err, c.Reason, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStderr = true | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if o.version != nil { | ||
| if len(majorMinorBuckets) == 0 { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } else if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| return fmt.Errorf("no updates available, so cannot display context for the requested release %s", o.version) | ||
| } | ||
|
|
||
| if major, ok := majorMinorBuckets[o.version.Major]; !ok { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } else if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| return fmt.Errorf("no updates to %d available, so cannot display context for the requested release %s", o.version.Major, o.version) | ||
| } else if minor, ok := major[o.version.Minor]; !ok { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } else if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| return fmt.Errorf("no updates to %d.%d available, so cannot display context for the requested release %s", o.version.Major, o.version.Minor, o.version) | ||
| } else { | ||
| for _, update := range minor { | ||
| if update.Release.Version == o.version.String() { | ||
| if c := notRecommendedCondition(update); c == nil { | ||
| if !o.quiet { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| 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) | ||
| previousStdout = true | ||
| } | ||
| } else { | ||
| if !o.quiet { | ||
| reason := c.Reason | ||
| if accept.Has("ConditionalUpdateRisk") { | ||
| reason = fmt.Sprintf("accepted %s via ConditionalUpdateRisk", c.Reason) | ||
| } | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| 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, reason, strings.ReplaceAll(c.Message, "\n", "\n ")) | ||
| previousStdout = true | ||
| } | ||
| issues.Insert("ConditionalUpdateRisk") | ||
| } | ||
| unaccepted := issues.Difference(accept) | ||
| if unaccepted.Len() > 0 { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
|
Member
Author
There was a problem hiding this comment. testing with f3de5e0: $ export OC_ENABLE_CMD_UPGRADE_RECOMMEND=true
$ export OC_ENABLE_CMD_UPGRADE_RECOMMEND_ACCEPT=true
$ export OC_ENABLE_CMD_UPGRADE_RECOMMEND_PRECHECK=true
$ ./oc adm upgrade recommend --version 4.18.19
The following conditions found no cause for concern in updating this cluster to later releases: recommended/CriticalAlerts (AsExpected), recommended/NodeAlerts (AsExpected), recommended/PodDisruptionBudgetAlerts (AsExpected)
The following conditions found cause for concern in updating this cluster to later releases: recommended/PodImagePullAlerts/KubeContainerWaiting/0
recommended/PodImagePullAlerts/KubeContainerWaiting/0=False:
Reason: Alert:firing
Message: warning alert KubeContainerWaiting firing, which may slow workload redistribution during rolling node updates. Pod container waiting longer than 1 hour. The alert description is: pod/nfd-gc-6cdb49f845-2plm7 in namespace openshift-nfd on container nfd-gc has been in waiting state for longer than 1 hour. <alert does not have a runbook_url annotation>
Upstream update service is unset, so the cluster will use an appropriate default.
Channel: candidate-4.18 (available channels: candidate-4.18, candidate-4.19, candidate-4.20, eus-4.18, fast-4.18, fast-4.19, stable-4.18)
Update to 4.18.19 has no known issues relevant to this cluster.
Image: quay.io/openshift-release-dev/ocp-release@sha256:e6d80b9ab85b17b47e90cb8de1b9ad0e3fe457780148629d329d532ef902d222
Release URL: https://access.redhat.com/errata/RHSA-2025:9725
error: issues that apply to this cluster but which were not included in --accept: KubeContainerWaitingSo that has the blank line before the |
||
| } else if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| 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 && !o.quiet { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| 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), ",")) | ||
| previousStdout = true | ||
| } | ||
| return nil | ||
| } | ||
| } | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } else if previousStderr { | ||
| fmt.Fprintln(o.ErrOut) | ||
| } | ||
| return fmt.Errorf("no update to %s available, so cannot display context for the requested release", o.version) | ||
| } | ||
| } | ||
|
|
||
| if len(majorMinorBuckets) == 0 { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "No updates available. You may still upgrade to a specific release image with --to-image or wait for new updates to be available.\n") | ||
| return nil | ||
| } | ||
|
|
@@ -374,8 +453,11 @@ func (o *options) Run(ctx context.Context) error { | |
| return minors[i] > minors[j] // sort descending, minor updates bring both feature and bugfixes | ||
| }) | ||
| for _, minor := range minors { | ||
| if previousStdout { | ||
| fmt.Fprintln(o.Out) | ||
| } | ||
| fmt.Fprintf(o.Out, "Updates to %d.%d:\n", major, minor) | ||
| previousStdout = true | ||
| lastWasLong := false | ||
| headerQueued := true | ||
|
|
||
|
|
||
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am little bit nervous regarding the code and a potential inclination for manual errors in the future.
Given the requirement to check for
previous*before "sections", printing a new line if so, settingprevious*after sections, and checkingif...else ifbefore returning an error.What do you think about using some kind of wrapper?
For example (output from Claude to give a sense of the idea):
and a helper like:
I am curious what do you think about this approach.