describe how the CVO can support TechPreviewNoUpgrade #957

deads2k · 2021-11-15T18:37:32Z

Operators can provide a manifest annotated as .metadata.annotations["config.openshift.io/feature-gate"]="TechPreviewNoUpgrade" . If this annotation is set, the manifest is only reconciled when featuregates.config.openshift.io|.spec.featureSet="TechPreviewNoUpgrade" is set.

/assign @sdodson @wking @JoelSpeed

enhancements/update/cvo-techpreview-manifests.md

wking · 2021-11-15T19:45:41Z

enhancements/update/cvo-techpreview-manifests.md

+If `featuregates.config.openshift.io|.spec.featureSet="TechPreviewNoUpgrade"`, then the manifest is reconciled as normal
+for whatever stage the CVO is in.
+The `featuregates.config.openshift.io|.spec.featureSet="TechPreviewNoUpgrade"` can be included in the installation
+manifests provided by a customer (we do this some CI scenarios as well), but there is no expected CVO change for bootstrapping.


Customer-provided manifests go up through the installer to the bootstrap node and are applied by cluster-bootstrap. There's no CVO connection there.

The CVO-bootstrapping bit I'm concerned about is:

Bootstrap node comes up.

Bootstrap CVO launched in a static pod.

Bootstrap CVO tries to fetch FeatureGates, but because it's early, that fetch fails.

CVO assumes no gates, and skips TechPreviewNoUpgrade manifests.

Something locks up, because bootstrapping this particular cluster depended on a tech-preview feature like "include support for $NEW_HARDWARE".

I think we can resolve my concern via either:

a. Declare that there won't be any TechPreviewNoUpgrade manifests that are needed for bootstrapping, so locking up if someone tries to add something like that is acceptable CVO behavior.
b. Declare that we might have that sort of thing, and explain how the CVO is expected to handle it (e.g. installer sets an environment variable or somehow informs the static bootstrap CVO pod that for this particular install, TechPreviewNoUpgrade is enabled).

a. Declare that there won't be any TechPreviewNoUpgrade manifests that are needed for bootstrapping, so locking up if someone tries to add something like that is acceptable CVO behavior.

This is what I was trying to describe with "there is no expected CVO change for bootstrapping". How can I phrase it more clearly?

You can drop:

The featuregates.config.openshift.io|.spec.featureSet="TechPreviewNoUpgrade" can be included in the installation manifests provided by a customer...

Because while folks could do that, it's completely orthogonal to this enhancement and would have no effect unless cluster-bootstrap or the installer grows similar handling.

Instead, say something like:

During bootstrapping, the CVO will assume no feature sets are enabled until it can successfully retrieve featuregates.config.openshift.io from the Kubernetes API server.

enhancements/update/cvo-techpreview-manifests.md

wking · 2021-11-15T19:56:29Z

enhancements/update/cvo-techpreview-manifests.md

+
+### Version Skew Strategy
+
+Because upgrades are prevented when this feature is active, there is no supported skew.


There can still be skew between 4.y.z -> 4.y.z', and the CVO does not block those on TechPreviewNoUpgrade today, because the current TechPreviewNoUpgrade block is based on Upgradeable=False in a ClusterOperator, and since the SCC kerfuffle in 4.3, that no longer blocks patch updates. We could teach the CVO to block patch updates on this property, or we could link the release.openshift.io/delete enhancement and point out that component maintainers have the same skew exposure as everyone else for patch updates.

wking · 2021-11-15T19:59:06Z

enhancements/update/cvo-techpreview-manifests.md

+5. create a clusteroperator that every customer has ignore
+
+This fans the problem out from a single point (CVO), to every tech preview operator.
+It also means that a cluster-admins have to ignore a clusteroperator in the list that is tech preview. 


I'm not clear on why this ignoring is a burden. Is it something we need to fix for the image registry operator's managementState: Unmanaged?

I'm not clear on why this ignoring is a burden. Is it something we need to fix for the image registry operator's managementState: Unmanaged?

The image-registry operator is not a techpreview operator. It is fully supported. When you search for it, you find docs, if it has a problem you know you are fully supported. Seeing a techpreview operator on your not techpreview cluster doesn't seem comparable to me.

ah, so the concern isn't:

huh, a ClusterOperator for $COMPONENT? I didn't think I had that installed...

We're ok with that conflation between the inert-mode operator and its backing operands. The concern is:

huh, a ClusterOperator for $COMPONENT? I wonder what that does... Getting the docs... What?! TechPreviewNoUpgrade! My cluster is doomed!

Can we be more specific in the wording? Something like:

It also increases the chance that a cluster admin would misinterpret an inert-mode operator as tainting their cluster with unsupported, tech-preview bits.

One thing to note, Tech Preview features should always be documented.

enhancements/update/cvo-techpreview-manifests.md

LalatenduMohanty · 2021-11-16T14:53:11Z

enhancements/update/cvo-techpreview-manifests.md

+
+### Goals
+
+1. CVO honors an "only create or update this manifest in clusters with tech preview enabled".


I think it should be only create and update instead of only create or update. Better if we can write Create, reconcile and update

deads2k · 2021-11-16T20:02:57Z

/retest

JoelSpeed · 2021-11-17T12:23:22Z

Have given this a read through and have nothing really to comment on. This seems like a sensible improvement to CVO and would save our CAPI project a lot of effort in the long run.

+1 from me

deads2k · 2021-11-18T14:03:33Z

/retest

sdodson · 2021-11-18T16:02:30Z

enhancements/update/cvo-techpreview-manifests.md

+## Summary
+
+[TechPreviewNoUpgrade](https://github.com/openshift/api/blob/be1be0e89115702f8b508d351c4f5c9a16e5ae95/config/v1/types_feature.go#L32-L34)
+is the canonical way to enable tech preview features.


It's the canonical method to enable them or a result of enabling tech preview features? I thought they were enabled via per feature gates but an outcome of enabling some may be TechPreviewNoUpgrade having been set.

I guess part of my confusion is that it's not clear to me if this line is saying what will be the case after this enhancement lands or how things are today.

sdodson · 2021-11-18T16:11:26Z

enhancements/update/cvo-techpreview-manifests.md

+5. create a clusteroperator that every customer has ignore
+
+This fans the problem out from a single point (CVO), to every tech preview operator.
+It also means that a cluster-admins have to ignore a clusteroperator in the list that is tech preview. 


One thing to note, Tech Preview features should always be documented.

sdodson · 2021-11-18T16:15:21Z

/lgtm
/approve
Some of my questions were just as I came to them but clarified later in the doc or by reading up on additional resources, so approving. Feel free to update if you think others may have the same questions I had.

openshift-ci · 2021-11-18T16:19:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

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:

~~OWNERS~~ [sdodson]

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

openshift-ci bot assigned JoelSpeed, sdodson and wking Nov 15, 2021

openshift-ci bot requested review from joelanford and stevekuznetsov November 15, 2021 18:38

wking reviewed Nov 15, 2021

View reviewed changes

enhancements/update/cvo-techpreview-manifests.md Outdated Show resolved Hide resolved

wking reviewed Nov 15, 2021

View reviewed changes

enhancements/update/cvo-techpreview-manifests.md Show resolved Hide resolved

wking reviewed Nov 15, 2021

View reviewed changes

enhancements/update/cvo-techpreview-manifests.md Outdated Show resolved Hide resolved

wking reviewed Nov 15, 2021

View reviewed changes

enhancements/update/cvo-techpreview-manifests.md Outdated Show resolved Hide resolved

LalatenduMohanty reviewed Nov 16, 2021

View reviewed changes

describe how the CVO can support TechPreviewNoUpgrade

7e240e2

deads2k force-pushed the cvo-techpreview branch from b5c06de to 7e240e2 Compare November 16, 2021 20:12

sdodson reviewed Nov 18, 2021

View reviewed changes

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

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 18, 2021

openshift-merge-robot merged commit f74ffe7 into openshift:master Nov 18, 2021

deads2k mentioned this pull request Nov 22, 2021

Exclude featuregate.release.openshift/tech-preview=true manifests openshift/cluster-version-operator#694

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

describe how the CVO can support TechPreviewNoUpgrade #957

describe how the CVO can support TechPreviewNoUpgrade #957

deads2k commented Nov 15, 2021

wking Nov 15, 2021

deads2k Nov 15, 2021

wking Nov 15, 2021

wking Nov 15, 2021

wking Nov 15, 2021

deads2k Nov 15, 2021

wking Nov 16, 2021

sdodson Nov 18, 2021

LalatenduMohanty Nov 16, 2021 •

edited

deads2k commented Nov 16, 2021

JoelSpeed commented Nov 17, 2021

deads2k commented Nov 18, 2021

sdodson Nov 18, 2021

sdodson Nov 18, 2021

sdodson Nov 18, 2021

sdodson commented Nov 18, 2021

openshift-ci bot commented Nov 18, 2021


		### Version Skew Strategy

		Because upgrades are prevented when this feature is active, there is no supported skew.


		### Goals

		1. CVO honors an "only create or update this manifest in clusters with tech preview enabled".

describe how the CVO can support TechPreviewNoUpgrade #957

describe how the CVO can support TechPreviewNoUpgrade #957

Conversation

deads2k commented Nov 15, 2021

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

LalatenduMohanty Nov 16, 2021 • edited

Choose a reason for hiding this comment

deads2k commented Nov 16, 2021

JoelSpeed commented Nov 17, 2021

deads2k commented Nov 18, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sdodson commented Nov 18, 2021

openshift-ci bot commented Nov 18, 2021

LalatenduMohanty Nov 16, 2021 •

edited