Bug 1878925: pkg/cli/admin/upgrade: Remove help text around history lookups #566

wking · 2020-09-14T22:57:41Z

Copying from the CVO, and adjusting to return a pointer instead of a (non-pointer,found) tuple. Future work can shift this function into library-go. This avoids:

$ oc adm upgrade --to 4.6.0-fc.5
error: Can't look up image for version 4.6.0-fc.5. The update channel has not been configured.

When that lookup would succeed when investigating ClusterVersion's status.history:

$ oc patch clusterversion version --type json -p '[{"op": "add", "path": "/spec/desiredUpdate", "value": {"version": "4.6.0-fc.5"}}]'
$ oc adm upgrade
info: An upgrade is in progress. Working towards 4.6.0-fc.5: downloading update

warning: Cannot display available updates:
  Reason: NoChannel
  Message: The update channel has not been configured.

openshift-ci-robot · 2020-09-14T22:58:00Z

[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:

~~pkg/cli/admin/upgrade/OWNERS~~ [wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot · 2020-09-14T23:00:33Z

@wking: This pull request references Bugzilla bug 1878925, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup

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/test-infra repository.

openshift-ci-robot · 2020-09-14T23:01:07Z

@wking: This pull request references Bugzilla bug 1878925, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.6.0) matches configured target release for branch (4.6.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup

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/test-infra repository.

wking · 2020-09-15T01:17:12Z

/assign @jottofar

LalatenduMohanty · 2020-09-15T10:58:24Z

pkg/cli/admin/upgrade/upgrade.go

 			if update == nil {
 				if len(cv.Status.AvailableUpdates) == 0 {
 					if c := findCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status == configv1.ConditionFalse {
 						return fmt.Errorf("Can't look up image for version %s. %v", o.To, c.Message)
 					}
 					return fmt.Errorf("No available updates, specify --to-image or wait for new updates to be available")
 				}
-				return fmt.Errorf("The update %s is not one of the available updates: %s", o.To, strings.Join(versionStrings(cv.Status.AvailableUpdates), ", "))
+				return fmt.Errorf("The update %s is neither one of the available updates (%s) nor a history entry", o.To, strings.Join(versionStrings(cv.Status.AvailableUpdates), ", "))


Why can not we just say The %s is not available to update? Do we really need to print the available update versions?

I not comfortable in printing about nor a history entry when we do not encourage customers to downgrade.

Should we put this behind --allow-versions-from-history or some such with a warning when it's needed, like our other --allow-... handling?

But is a version from history still necessarily available in channel for an update?

Good point from both of you about having a guard around downgrades. I've pushed 2b92106 -> adb05a7 to wire this up to the existing --allow-explicit-upgrade. If --to ... picks a value from availableUpdates, it doesn't matter whether you've set --allow-explicit-upgrade or not. But with adb05a7, when --to ... picks a value from the history, you will either fail to update (when --allow-explicit-upgrade was not set) or successfully trigger the update but with a stderr complaint (when --allow-explicit-upgrade was set). I think that addresses the safety concerns.

Do we really need to print the available update versions?

No, but mentioning them means folks can skip a separate oc adm upgrade step that may return a different list of available updates than the set we just used to reject their choice.

I not comfortable in printing about nor a history entry...

Does the new --allow-explicit-upgrade integration address your concerns there?

But is a version from history still necessarily available in channel for an update?

Nope. And the point of this PR is allowing these updates to previous versions without requiring either oc patch ... or folks looking up the by-digest pullspec themselves. But yes, there is a safety issue with rolling the cluster back. Does the new --allow-explicit-upgrade integration address your concerns there?

@wking nor a history entry , will the customer understand what this means? If yes then I am ok with this one.

@wking nor a history entry , will the customer understand what this means?

Are you asking for a rephrase that says "you can draw from either set" without using "neither ... nor ..."? Or something that mentions the fact this history entries may not be recommended updates? Or...?

Here is my suggestion The update %s is neither one of the available updates (%s) nor present in the update history

Here is my suggestion...

Rebased onto master and updated to use your text with adb05a7 -> 754657b.

pkg/cli/admin/upgrade/upgrade.go

openshift-ci-robot · 2020-09-15T19:50:11Z

@wking: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-cmd	`adb05a7`	link	`/test e2e-cmd`

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/test-infra repository. I understand the commands that are listed here.

stevekuznetsov · 2020-10-27T17:26:36Z

/bugzilla refresh

openshift-ci-robot · 2020-10-27T17:26:40Z

@stevekuznetsov: This pull request references Bugzilla bug 1878925, which is invalid:

expected the bug to target the "4.7.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh

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/test-infra repository.

openshift-merge-robot · 2020-11-05T20:19:35Z

@wking: The following tests failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
ci/prow/e2e-agnostic-cmd	`adb05a7`	link	`/test e2e-agnostic-cmd`
ci/prow/e2e-aws-serial	`adb05a7`	link	`/test e2e-aws-serial`

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/test-infra repository. I understand the commands that are listed here.

openshift-bot · 2021-02-04T01:35:46Z

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

wking · 2021-02-15T23:36:34Z

/bugzilla refresh
/retest

openshift-ci-robot · 2021-02-15T23:36:37Z

@wking: This pull request references Bugzilla bug 1878925, which is invalid:

expected the bug to target the "4.8.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

In response to this:

/bugzilla refresh
/retest

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/test-infra repository.

wking · 2021-02-15T23:38:20Z

/bugzilla refresh
/remove-lifecycle stale

openshift-ci-robot · 2021-02-15T23:38:24Z

@wking: This pull request references Bugzilla bug 1878925, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.8.0) matches configured target release for branch (4.8.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

In response to this:

/bugzilla refresh
/remove-lifecycle stale

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/test-infra repository.

LalatenduMohanty · 2021-02-16T16:59:34Z

pkg/cli/admin/upgrade/upgrade.go

+				if !o.AllowExplicitUpgrade {
+					return fmt.Errorf("The requested upgrade image is not one of the available updates, you must pass --allow-explicit-upgrade to continue")
+				}
+				fmt.Fprintln(o.ErrOut, "warning: The requested upgrade version is not one of the available updates.  You have used --allow-explicit-upgrade to the update to proceed anyway")


IMO we do not need You have used --allow-explicit-upgrade to the update to proceed anyway" in the sentence.

It matches the existing pattern I used in previous work which has already landed. If you would rather not point out why particular guard failures are being waved, can you submit a master PR to remove the existing instances?

LalatenduMohanty · 2021-02-16T19:58:13Z

pkg/cli/admin/upgrade/upgrade.go

 			if update == nil {
 				if len(cv.Status.AvailableUpdates) == 0 {
 					if c := findCondition(cv.Status.Conditions, configv1.RetrievedUpdates); c != nil && c.Status == configv1.ConditionFalse {
 						return fmt.Errorf("Can't look up image for version %s. %v", o.To, c.Message)
 					}
 					return fmt.Errorf("No available updates, specify --to-image or wait for new updates to be available")
 				}
-				return fmt.Errorf("The update %s is not one of the available updates: %s", o.To, strings.Join(versionStrings(cv.Status.AvailableUpdates), ", "))
+				return fmt.Errorf("The update %s is neither one of the available updates (%s) nor a history entry", o.To, strings.Join(versionStrings(cv.Status.AvailableUpdates), ", "))


Here is my suggestion The update %s is neither one of the available updates (%s) nor present in the update history

openshift-bot · 2021-09-01T08:22:20Z

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

openshift-merge-robot · 2021-09-06T07:27:31Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

openshift-ci · 2021-09-06T07:28:35Z

@openshift-merge-robot: This pull request references Bugzilla bug 1878925, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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/test-infra repository.

openshift-merge-robot · 2021-09-06T07:57:06Z

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

openshift-ci · 2021-09-06T07:57:09Z

@openshift-merge-robot: This pull request references Bugzilla bug 1878925, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

/bugzilla refresh

The requirements for Bugzilla bugs have changed, recalculating validity.

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/test-infra repository.

openshift-bot · 2021-10-06T09:00:10Z

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

openshift-ci · 2021-10-06T09:00:37Z

@openshift-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.

Reopen the issue by commenting /reopen.
Mark the issue as fresh by commenting /remove-lifecycle rotten.
Exclude this issue from closing again by commenting /lifecycle frozen.

/close

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/test-infra repository.

openshift-ci · 2021-10-06T09:00:44Z

@wking: This pull request references Bugzilla bug 1878925. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state.

In response to this:

Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup

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/test-infra repository.

wking · 2021-11-03T17:27:13Z

/reopen

openshift-ci · 2021-11-03T17:27:45Z

@wking: Reopened this PR.

In response to this:

/reopen

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/test-infra repository.

openshift-ci · 2021-11-03T17:27:47Z

@wking: This pull request references Bugzilla bug 1878925, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Retaining the bugzilla/valid-bug label as it was manually added.

In response to this:

Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup

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/test-infra repository.

We've had it at least since this repo was created in 3abf60a (setup oc repo, 2019-06-17), but the --to implementation has only ever iterated over availableUpdates. This catches the help text up with reality. The confused help text is likely because cluster-version operator does currently search history as well [1], but we're about to remove that too, to make it harder to ask for dangerous rollbacks. [1]: https://github.com/openshift/cluster-version-operator/blob/6d56c655ea16f6faee4b65ffef43dcd912657bc6/pkg/cvo/updatepayload.go#L305-L325

wking · 2021-11-03T17:46:07Z

I've pushed 754657b -> fcc58db to pivot to dropping previous from the --help text, per @sdodson's request.

wking · 2021-11-03T17:53:46Z

/bugzilla refresh

openshift-ci · 2021-11-03T17:53:53Z

@wking: This pull request references Bugzilla bug 1878925, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @zhouying7780

In response to this:

/bugzilla refresh

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/test-infra repository.

openshift-ci · 2021-11-03T20:14:30Z

@wking: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-metal-ipi-ovn-ipv6	`fcc58db`	link	false	`/test e2e-metal-ipi-ovn-ipv6`

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/test-infra repository. I understand the commands that are listed here.

jottofar · 2021-11-05T13:08:20Z

/lgtm
/retest

openshift-ci · 2021-11-05T13:12:10Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jottofar, 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:

~~pkg/cli/admin/upgrade/OWNERS~~ [jottofar,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci · 2021-11-05T15:13:40Z

@wking: All pull requests linked via external trackers have merged:

Bugzilla bug 1878925 has been moved to the MODIFIED state.

In response to this:

Bug 1878925: pkg/cli/admin/upgrade: Remove help text around history lookups

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/test-infra repository.

openshift-ci-robot requested review from smarterclayton and soltysh September 14, 2020 22:57

openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 14, 2020

wking changed the title ~~pkg/cli/admin/upgrade: Teach --to about history lookup~~ Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup Sep 14, 2020

openshift-ci-robot added the bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. label Sep 14, 2020

openshift-ci-robot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Sep 14, 2020

openshift-ci-robot assigned jottofar Sep 15, 2020

LalatenduMohanty suggested changes Sep 15, 2020

View reviewed changes

jottofar reviewed Sep 15, 2020

View reviewed changes

pkg/cli/admin/upgrade/upgrade.go Outdated Show resolved Hide resolved

wking force-pushed the update-to-without-channel branch from 2b92106 to adb05a7 Compare September 15, 2020 18:15

openshift-ci-robot removed the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Oct 27, 2020

openshift-ci-robot added the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Oct 27, 2020

openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 4, 2021

LalatenduMohanty suggested changes Feb 16, 2021

View reviewed changes

openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Sep 1, 2021

openshift-ci bot closed this Oct 6, 2021

openshift-ci bot reopened this Nov 3, 2021

wking changed the title ~~Bug 1878925: pkg/cli/admin/upgrade: Teach --to about history lookup~~ Bug 1878925: pkg/cli/admin/upgrade: Remove help text around history lookups Nov 3, 2021

wking force-pushed the update-to-without-channel branch from 754657b to fcc58db Compare November 3, 2021 17:45

wking mentioned this pull request Nov 3, 2021

Bug 1878925: pkg/cvo/updatepayload: Drop history from target pullspec lookup openshift/cluster-version-operator#684

Merged

openshift-ci bot requested a review from zhouying7780 November 3, 2021 17:53

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 5, 2021

openshift-merge-robot merged commit 15c4e26 into openshift:master Nov 5, 2021

wking deleted the update-to-without-channel branch November 5, 2021 15:23

Bug 1878925: pkg/cli/admin/upgrade: Remove help text around history lookups #566

Bug 1878925: pkg/cli/admin/upgrade: Remove help text around history lookups #566

Conversation

wking commented Sep 14, 2020 • edited

openshift-ci-robot commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020

openshift-ci-robot commented Sep 14, 2020

wking commented Sep 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-ci-robot commented Sep 15, 2020 • edited

stevekuznetsov commented Oct 27, 2020

openshift-ci-robot commented Oct 27, 2020

openshift-merge-robot commented Nov 5, 2020

openshift-bot commented Feb 4, 2021

wking commented Feb 15, 2021

openshift-ci-robot commented Feb 15, 2021

wking commented Feb 15, 2021

openshift-ci-robot commented Feb 15, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

openshift-bot commented Sep 1, 2021

openshift-merge-robot commented Sep 6, 2021

openshift-ci bot commented Sep 6, 2021

openshift-merge-robot commented Sep 6, 2021

openshift-ci bot commented Sep 6, 2021

openshift-bot commented Oct 6, 2021

openshift-ci bot commented Oct 6, 2021

openshift-ci bot commented Oct 6, 2021

wking commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021

wking commented Nov 3, 2021

wking commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021

openshift-ci bot commented Nov 3, 2021 • edited

jottofar commented Nov 5, 2021

openshift-ci bot commented Nov 5, 2021

openshift-ci bot commented Nov 5, 2021

wking commented Sep 14, 2020 •

edited

openshift-ci-robot commented Sep 15, 2020 •

edited

openshift-ci bot commented Nov 3, 2021 •

edited