OCPBUGS-61061: pkg/cli/admin/upgrade/recommend: Add a blank line after --version conditional risk #2045
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@wking: This pull request references OTA-1576 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: wking The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
| } | ||
| unaccepted := issues.Difference(accept) | ||
| if unaccepted.Len() > 0 { | ||
| if !o.quiet { |
There was a problem hiding this comment.
Won't it be duplicate with line 327 to 329?
There was a problem hiding this comment.
let me create a OC client from this pr to test it. I need to set up a cluster, I will submit my test result, maybe 2 hours later.
|
I am using following OC client test this PR: There is an extra string &{0xc0001520c0} when I use new OC: Compare to the new OC, the old oc client will output: |
|
After accept all risks, the output looks good: |
|
/cc |
wking
force-pushed
the
blank-line-after-recommend-version-conditional-risk
branch
from
4e93466 to
f3de5e0
Compare
| unaccepted := issues.Difference(accept) | ||
| if unaccepted.Len() > 0 { | ||
| if !o.quiet { | ||
| fmt.Fprintln(o.Out) |
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 error: ... 👍
| 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 { | ||
| 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), ",")) | ||
| fmt.Fprintf(o.Out, "\nUpdate to %s has no known issues relevant to this cluster other than the accepted %s.\n", update.Release.Version, strings.Join(sets.List(issues), ",")) |
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 --accept KubeContainerWaiting
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, but were explicitly accepted via --accept: recommended/PodImagePullAlerts/KubeContainerWaiting/0
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
Update to 4.18.19 has no known issues relevant to this cluster other than the accepted KubeContainerWaiting.So that has the blank line before the ...other than the accepted... 👍 The two, similar has no known issues relevant to this cluster lines are a bit weird, but that predates this newline-adding change, and we can decide if it's worth addressing in follow-up work.
There was a problem hiding this comment.
Testing with ecf80c8:
$ ./oc adm upgrade recommend
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-pgm7g 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, stable-4.19)
Updates to 4.18:
VERSION ISSUES
4.18.25 no known issues relevant to this cluster
4.18.24 no known issues relevant to this cluster
And 19 older 4.18 updates you can see with '--show-outdated-releases' or '--version VERSION'.
$ ./oc adm upgrade recommend --version 4.18.25
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-pgm7g 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, stable-4.19)
Update to 4.18.25 has no known issues relevant to this cluster.
Image: quay.io/openshift-release-dev/ocp-release@sha256:ba6f0f2eca65cd386a5109ddbbdb3bab9bb9801e32de56ef34f80e634a7787be
Release URL: https://access.redhat.com/errata/RHBA-2025:16732
error: issues that apply to this cluster but which were not included in --accept: KubeContainerWaiting
$ ./oc adm upgrade recommend --version 4.18.25 --accept KubeContainerWaiting
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, but were explicitly accepted via --accept: recommended/PodImagePullAlerts/KubeContainerWaiting/0
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, stable-4.19)
Update to 4.18.25 has no known issues relevant to this cluster.
Image: quay.io/openshift-release-dev/ocp-release@sha256:ba6f0f2eca65cd386a5109ddbbdb3bab9bb9801e32de56ef34f80e634a7787be
Release URL: https://access.redhat.com/errata/RHBA-2025:16732
Update to 4.18.25 has no known issues relevant to this cluster other than the accepted KubeContainerWaiting.So that has the blank line before the ...other than the accepted... 👍 The two, similar has no known issues relevant to this cluster lines are a bit weird, but that predates this newline-adding change, and we can decide if it's worth addressing in follow-up work.
|
Thank you @wking, it works normal now. Here are some of my test sceanrios:
|
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@wking: This pull request references OTA-1576 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.20.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
wking
force-pushed
the
blank-line-after-recommend-version-conditional-risk
branch
from
f3de5e0 to
2a2c132
Compare
|
Would |
|
/retest |
openshift-merge-robot
added
the
needs-rebase
wking
changed the title
openshift-ci-robot
added
the
jira/invalid-bug
|
@wking: This pull request references Jira Issue OCPBUGS-61061, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@wking: This pull request references Jira Issue OCPBUGS-61061, which is valid. 3 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira ([email protected]), skipping review request. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository. |
openshift-ci-robot
removed
the
jira/invalid-bug
Since 8bdac16 (pkg/cli/admin/upgrade/recommend: Enable precheck and accept gates, 2025-09-03, openshift#2088), this knob has always been enabled. Save ourselves a no longer necessary level of indentation by removing it completely.
…ditional risk When I added the: Update to 4.18.6 Recommended=False: ... block in c1ef328 (pkg/cli/admin/upgrade/recommend: Add --version option for specific releases, 2024-10-14, openshift#1897), there was no subsequent output in this "--version has conditional risks" case, so the earlier lack-of-blank-line made sense. But then 2d7e004 (pkg/cli/admin/upgrade/recommend: New, feature-gated --accept, 2025-05-06, openshift#2023) gave the option for a subsequent: error: issues that apply to this cluster but which were not included in --accept: ... or: Update to %s has no known issues relevant... This commit adds trackers to all the output blocks in the command for "was there previous output on this stream?", so that a new block (or a returned error) can be prefixed by the blank line needed to set off the new block from the previous block. For example, it now sets off the error line from the previous, possibly-long message strings: ... Update to 4.18.6 Recommended=False: Image: quay.io/openshift-release-dev/ocp-release@sha256:61fdad894f035a8b192647c224faf565279518255bdbf60a91db4ee0479adaa6 Release URL: https://access.redhat.com/errata/RHSA-2025:3066 Reason: CRIOLayerCompressionPulls Message: The CRI-O container runtime may fail to pull images with certain layer compression characteristics https://issues.redhat.com/browse/OCPNODE-3074 error: issues that apply to this cluster but which were not included in --accept: ConditionalUpdateRisk,KubeContainerWaiting instead of cramming that 'error: ...' or '... has no known issues...' right onto the end of the output. The error is getting written to standard error instead of the standard output, so there's still some wiggle room for poor spacing for terminal users and others who are flattening those two streams together. And now that we have the additional newline handling in recommend.go, I can drop the newline I'd been manually injecting in examples_test.go to make the test fixtures more readable. I should have noticed that test-specific injection as a sign that the better recommend.go newline handling was useful. Oh well, better late than never.
wking
force-pushed
the
blank-line-after-recommend-version-conditional-risk
branch
from
2a2c132 to
ecf80c8
Compare
WalkthroughUpdates the upgrade recommend command: removes precheck feature flag, refactors precheck invocation and error handling, restructures condition classification and output spacing, and adjusts user-facing messages. Test expectations are updated to match new error formatting and removal of precheck option usage. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to 📒 Files selected for processing (2)
🔇 Additional comments (13)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
openshift-merge-robot
removed
the
needs-rebase
|
@wking: The following tests failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
|
/cc |
6 participants
When I added the:
block in c1ef328 (#1897), there was no subsequent output in this "
--versionhas conditional risks" case, so the earlier lack-of-blank-line made sense.But then 2d7e004 (#2023) gave the option for a subsequent:
or:
This commit adds a new line to standard output, to offset those summaries from the possibly-long Message:
instead of cramming that
error: ...or... has no known issues...right onto the end of the output. Although the error is getting written to standard error instead of the standard output, so in that case, the newline will only matter for terminal users and others who are flattening those two streams together.