Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented May 19, 2020

I would prefer to configure this by clearing the upstream setting, which seems more intuitive for "there is no upstream" to me. But sadly, it seems that the CVO has been falling back to a default URI when the ClusterVersion upstream is empty since way back, and that this behavior is enshrined in the API. Although the channel docs also talk about defaults, the only channel defaulting in the CVO is when creating a ClusterVersion object after the in-cluster copy was (accidentally?) deleted. So maybe we could talk folks into adjusting the CVO logic to return NoUpstream in the empty-upstream case, but at the moment, clearing the channel is the best approach for "the CVO keeps complaining that it can't hit the upstream, and I want to quiet it down" (rhbz#1827378)5.

@wking wking force-pushed the recommend-no-channel branch from 9afff16 to 635018c Compare May 19, 2020 04:01
@wking
Copy link
Member Author

wking commented Jun 8, 2020

/assign @kalexand-rh

Copy link
Contributor

@kalexand-rh kalexand-rh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a question and some suggestions.

@wking wking force-pushed the recommend-no-channel branch 2 times, most recently from eb93d29 to e8552a5 Compare June 24, 2020 21:31
@kalexand-rh
Copy link
Contributor

@jianlinliu, will you PTAL? I think this change should go back to 4.2. Do you agree?

@jianlinliu
Copy link

LGTM. @jiajliu Can you help review this from your side?

@jiajliu
Copy link

jiajliu commented Jun 28, 2020

There is a note in official doc https://docs.openshift.com/container-platform/4.4/updating/updating-cluster-between-minor.html.

Because of the difficulty of changing update channels by using oc, use the web console to change the update channel. It is recommended to complete the update process within the web console. You can follow the steps in Updating a cluster within a minor version by using the CLI to complete the update after you change to a 4.4 channel.

But from web console, we don't provide null channel in "Channel" option currently. So my suggestion here is that, we'd better to recommend users to clear channel trough CLI and provide the command to clear channel.

@kalexand-rh
Copy link
Contributor

@LalatenduMohanty, @wking, do you have the command to clear the upgrade channel?

@LalatenduMohanty
Copy link
Member

@LalatenduMohanty, @wking, do you have the command to clear the upgrade channel?

$ oc patch clusterversion/version --patch '{"spec":{"channel":""}}' --type=merge would clear the channel. But I think on a fresh installed cluster the channel will not be set. Users need to clear it only if it is set already.

@wking
Copy link
Member Author

wking commented Jul 18, 2020

But I think on a fresh installed cluster the channel will not be set.

This is only true in CI since openshift/release#8631. User installs should have default channels, see openshift/installer#3848.

@wking
Copy link
Member Author

wking commented Sep 18, 2020

Is this blocked on docs with an oc patch ... command to clear the channel? I'll try to get an oc wrapper around channel maintenance in place in the meantime...

@wking
Copy link
Member Author

wking commented Sep 18, 2020

I've floated openshift/oc#576 for convenient, patch-free channel management.

@openshift-bot
Copy link

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

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 18, 2020
@wking
Copy link
Member Author

wking commented Dec 19, 2020

/lifecycle frozen

Still trying to get review on openshift/oc#576. Now that 4.7 is in feature freeze, I'll push again once 4.7 forks off master and 4.8 opens up for new features.

@openshift-ci-robot openshift-ci-robot added lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Dec 19, 2020
@vikram-redhat
Copy link
Contributor

@wking is this still valid?

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 6, 2021
@wking
Copy link
Member Author

wking commented May 6, 2021

@wking is this still valid?

Yeah, we still want to recommend clearing the channel as the way to get the CVO to stop trying to poll the configured upstream. @jiajliu sort of blocked this PR because we didn't have a convenient way to clear the channel short of oc patch .... I have not been successful in motivating folks to review openshift/oc#576, but will do another round of begging once master opens for 4.9. It's also possible that the console folks have landed something that makes clearing your channel easy; I'll check on that front. And personally, I think we should be distributing this advice, even if we can't make up our minds about how we want customers to accomplish the clearing. And I'm fine recommending the patch command if that's our current best option, even if it isn't particularly elegant.

@wking wking force-pushed the recommend-no-channel branch from e8552a5 to bf742cc Compare June 8, 2021 21:18
@openshift-ci openshift-ci bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jun 8, 2021
@netlify
Copy link

netlify bot commented Jun 8, 2021

✔️ Deploy Preview for osdocs ready!

🔨 Explore the source changes: 4244510

🔍 Inspect the deploy log: https://app.netlify.com/sites/osdocs/deploys/6100461cd998620007195424

😎 Browse the preview: https://deploy-preview-22252--osdocs.netlify.app/openshift-enterprise/latest/updating/updating-cluster-between-minor

@wking
Copy link
Member Author

wking commented Jun 8, 2021

Rebased onto master with e8552a5732 -> bf742cc4e5.

@wking wking force-pushed the recommend-no-channel branch from bf742cc to e494b86 Compare July 7, 2021 21:32
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 7, 2021
@wking
Copy link
Member Author

wking commented Jul 7, 2021

I've pushed bf742cc4e -> e494b8661, rebasing onto master and updating to mention oc adm upgrade channel for clearing channels (just landed for 4.9 via openshift/oc#576).

@wking wking force-pushed the recommend-no-channel branch from e494b86 to a0f4278 Compare July 7, 2021 21:35
@wking
Copy link
Member Author

wking commented Jul 7, 2021

And I've pushed e494b8661 -> a0f42786e to update the:

Because of the difficulty of changing update channels by using oc...

language mentioned earlier, and instead mention oc adm upgrade channel ....

@jiajliu
Copy link

jiajliu commented Jul 8, 2021

LGTM.

@jiajliu
Copy link

jiajliu commented Jul 8, 2021

@wking @LalatenduMohanty Since we introduce oc adm upgrade channel command in this pr(for official doc), and i notice pr #576 has been merged now. I think it will be available soon in v4.9. Do you think QE should have some test against the new added subcommand? If so, we'd better have a jira us to track it, so that QE can involve it. HDYT?

Copy link
Member

@LalatenduMohanty LalatenduMohanty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2021
@wking
Copy link
Member Author

wking commented Jul 22, 2021

If so, we'd better have a jira us to track it, so that QE can involve it.

OTA-416 is our tracker for this change, if that's useful for QE to hang some test-cases on.

@jiajliu
Copy link

jiajliu commented Jul 23, 2021

If so, we'd better have a jira us to track it, so that QE can involve it.

OTA-416 is our tracker for this change, if that's useful for QE to hang some test-cases on.

QE will follow up asap.

Copy link
Contributor

@jeana-redhat jeana-redhat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a couple minor suggestions.

@wking wking force-pushed the recommend-no-channel branch from a0f4278 to 4085e6d Compare July 27, 2021 17:42
@openshift-ci
Copy link

openshift-ci bot commented Jul 27, 2021

New changes are detected. LGTM label has been removed.

@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 27, 2021
I would prefer to configure this by clearing the 'upstream' setting,
which seems more intuitive for "there is no upstream" to me.  But
sadly, it seems that the CVO has been falling back to a default URI
when the ClusterVersion upstream is empty since way back [1,2], and
that this behavior is enshrined in the API [3].  Although the channel
docs also talk about defaults [4], the only channel defaulting in the
CVO is when creating a ClusterVersion object after the in-cluster copy
was (accidentally?)  deleted [5].  So maybe we could talk folks into
adjusting the CVO logic to return NoUpstream in the empty-upstream
case, but at the moment, clearing the channel is the best approach for
"the CVO keeps complaining that it can't hit the upstream, and I want
ot quiet it down [6]".

The 'oc adm upgrade channel' command just landed for 4.9 in [7].

[1]: https://github.com/openshift/cluster-version-operator/blame/2c4931dc283c551938be1a00fee290de0b79d99c/pkg/cvo/availableupdates.go#L27-L31
[2]: openshift/cluster-version-operator@ab4d84a#diff-78f2af341fa49292dd6930f378018867R24
[3]: https://github.com/openshift/api/blame/0422dc17083e9e8df18d029f3f34322e96e9c326/config/v1/types_cluster_version.go#L56-L57
[4]: https://github.com/openshift/api/blame/0422dc17083e9e8df18d029f3f34322e96e9c326/config/v1/types_cluster_version.go#L62-L63
[5]: https://github.com/openshift/cluster-version-operator/blob/2c4931dc283c551938be1a00fee290de0b79d99c/pkg/cvo/cvo.go#L602
[6]: https://bugzilla.redhat.com/show_bug.cgi?id=1827378
[7]: openshift/oc#576
@wking wking force-pushed the recommend-no-channel branch from 4085e6d to 4244510 Compare July 27, 2021 17:44
@jeana-redhat jeana-redhat added peer-review-done Signifies that the peer review team has reviewed this PR and removed lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. labels Jul 27, 2021
@jeana-redhat jeana-redhat added this to the Future Release milestone Jul 27, 2021
@jeana-redhat jeana-redhat merged commit 45d8514 into openshift:main Jul 27, 2021
@jeana-redhat
Copy link
Contributor

/cherrypick enterprise-4.9

@openshift-cherrypick-robot

@jeana-redhat: new pull request created: #34900

Details

In response to this:

/cherrypick enterprise-4.9

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

branch/enterprise-4.9 peer-review-done Signifies that the peer review team has reviewed this PR size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants