Skip to content

Conversation

@wking
Copy link
Member

@wking wking commented Aug 20, 2021

Hopefully we never actually have to stuff a CVO-generated ClusterVersion into the cluster; it's just for recovery after admins accidentally delete their existing ClusterVersion. But if we ever do hit this code, we want to push it without spec.upstream, to allow later ClusterVersion-consuming code to say "ah, user doesn't care which upstream I use, so I'll use the best default I'm aware of". This effectively pushes default choice from the CVO that creates the ClusterVersion out to the CVO that consumes the ClusterVersion, and that later CVO is almost certainly more current on which default is best.

Similar to openshift/installer@c9095b3451 (openshift/installer#4112).

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2021
},
Spec: configv1.ClusterVersionSpec{
Upstream: upstream,
Channel: "fast",
Copy link
Member

Choose a reason for hiding this comment

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

Why can't we remove the fast channel as default?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's good to have a signal that this code is happening, because it's very serious admin errors or a bug when it trips. For example, we had rhbz#1708697, from the CVO racing cluster-bootstrap during install (fixed by telling the CVO not do push this default into the cluster during install).

Copy link
Member

Choose a reason for hiding this comment

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

I can live with that.

…sion

Hopefully we never actually have to stuff a CVO-generated
ClusterVersion into the cluster; it's just for recovery after admins
accidentally delete their existing ClusterVersion.  But if we ever do
hit this code, we want to push it without spec.upstream, to allow
later ClusterVersion-consuming code to say "ah, user doesn't care
which upstream I use, so I'll use the best default I'm aware of".
This effectively pushes default choice from the CVO that creates the
ClusterVersion out to the CVO that consumes the ClusterVersion, and
that later CVO is almost certainly more current on which default is
best.

Similar to openshift/installer@c9095b3451
(data/manifests/bootkube/cvo-overrides: Drop the explicit upstream,
2020-08-28, openshift/installer#4112).
@wking wking force-pushed the drop-default-upstream-from-default-ClusterVersion branch from b8975db to 0b60b87 Compare August 20, 2021 21:58
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 Aug 23, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 23, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: LalatenduMohanty, wking

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:
  • OWNERS [LalatenduMohanty,wking]

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

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

4 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@vrutkovs
Copy link

vrutkovs commented Sep 6, 2021

/refresh

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit c34d95a into openshift:master Sep 8, 2021
@wking wking deleted the drop-default-upstream-from-default-ClusterVersion branch September 9, 2021 20:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants