pkg/types: add feature set support by patrickdillon · Pull Request #6336 · openshift/installer

patrickdillon · 2022-09-12T21:02:02Z

Adds basic support for feature sets in the install config, as well as validation for gated features.

patrickdillon · 2022-09-12T21:02:57Z

/wip

Needs further work for explain

patrickdillon · 2022-09-13T20:55:44Z

Oops. Need to run go gen to bump explain

Adds feature gate support: - featureset field in installconfig - validation for this field - featuregate manifest This change allows users to specify featuresets in the install config, most notably TechPreview. When a featureset is specified, the installer will validate the provided value against the valid values in the OpenShift API, and, if valid, create the cluster featuregate manifest.

Indicate that the networkProjectID field is in tech preview. This is important for the output of openshift-install explain. In future work, we hope to have a structured way of decorating fields, such as // +openshift:enable:FeatureSets=TechPreviewNoUpgrade.

Emit a warning when feature sets are enabled so that users understand the impact of the feature set.

patrickdillon · 2022-10-05T15:43:45Z

@jcpowermac @rvanderp3 0d9b518 sets vsphere multizone as tech preview. PTAL

jcpowermac · 2022-10-05T15:53:26Z

@jcpowermac @rvanderp3 0d9b518 sets vsphere multizone as tech preview. PTAL

Looks great, thanks @patrickdillon !

rvanderp3 · 2022-10-05T16:07:25Z

@jcpowermac @rvanderp3 0d9b518 sets vsphere multizone as tech preview. PTAL

Looks great, thanks @patrickdillon !

very cool, lgtm. thanks Patrick!

barbacbd · 2022-10-05T16:07:37Z

Do we need to include the other tech preview variables added to the GCP platform in #6288?

Only if those features are "tech preview." I think those were going to be released as GA (not tech preview), but if they are tech preview we can add them (in a later PR).

patrickdillon · 2022-10-05T19:51:38Z

/retest

Enforces that vSphere multizone fields in the install config are only enabled with tech preview.

sdodson · 2022-10-06T14:33:14Z

/approve

wking

/lgtm

docs/ entries to come in follow-up work.

openshift-ci · 2022-10-06T16:35:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson, 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~~ [sdodson]

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

sadasu · 2022-10-06T16:36:59Z

 	}

 	o.FileList = append(o.FileList, openshiftInstall.Files()...)
+	o.FileList = append(o.FileList, featureGate.Files()...)


Since FeatureGate's Generate() could return nil, do we want to check for that before calling the append()?

append should gracefully handle this case:
https://go.dev/play/p/tOrMzBn_wS_I

openshift-ci-robot · 2022-10-06T16:42:31Z

/retest-required

Remaining retests: 0 against base HEAD 3379601 and 2 for PR HEAD d97b684 in total

sadasu · 2022-10-06T16:56:08Z

+	}
+
+	if c.FeatureSet != configv1.TechPreviewNoUpgrade {
+		errMsg := "the TechPreviewNoUpgrade feature set must be enabled to use this field"


Could CustomNoUpgrade also be allowed? Could there be a scenario where a custome feature and a tech preview need to be tested together?

sadasu · 2022-10-06T17:02:04Z

Couple of comments below. Not need to stop the merge for that:

Implementation for adding FeatureSets to the Installconfig and adding other features (GCP networkProjectID and some vSphere config) as a TechPreview project could have been separated into PRs. There has been an attempt to put them in different commits but there are some networkProjectID changes that have made it into other commits. At the least, we could edit the PR title and description to indicate that some additional features is being touched in this PRi to move them into tech preview.
Adding the validation of the features in TechPreview within the validation of FeatureSet config seems counter intuitive. What if we moved the validation for TechPreview config into their own validation methods, and added an additional check for FeatureSet in there? That way, when the feature is GA-ed, all we need to do is to remove the FeatureGate check. But, I do see what the thought process behind the current implementation is.

patrickdillon · 2022-10-06T18:46:02Z

Adding the validation of the features in TechPreview within the validation of FeatureSet config seems counter intuitive. What if we moved the validation for TechPreview config into their own validation methods, and added an additional check for FeatureSet in there? That way, when the feature is GA-ed, all we need to do is to remove the FeatureGate check. But, I do see what the thought process behind the current implementation is.

This is a good idea. I want to get this PR in before FF, so I'm not going to try to push this suggestion in this PR.

But I am hoping there are other ways we could make it easier to add/remove fields from this protection (I've been hoping reflection could be a solution for this, but so far no dice). So I will create a card for this idea and include your suggestion there.

patrickdillon · 2022-10-06T18:46:12Z

/skip

patrickdillon · 2022-10-06T19:04:38Z

/override ci/prow/e2e-aws-ovn ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn

openshift-ci · 2022-10-06T19:06:57Z

@patrickdillon: Overrode contexts on behalf of patrickdillon: ci/prow/e2e-aws-ovn, ci/prow/e2e-azure-ovn, ci/prow/e2e-gcp-ovn, ci/prow/e2e-vsphere-ovn

Details

In response to this:

/override ci/prow/e2e-aws-ovn ci/prow/e2e-azure-ovn ci/prow/e2e-gcp-ovn ci/prow/e2e-vsphere-ovn

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 · 2022-10-06T19:07:19Z

@patrickdillon: The following tests 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/okd-e2e-gcp-ovn-upgrade	84b714bf2280401b071764966d9278188f9071aa	link	false	`/test okd-e2e-gcp-ovn-upgrade`
ci/prow/okd-scos-e2e-vsphere	`d97b684`	link	false	`/test okd-scos-e2e-vsphere`
ci/prow/e2e-metal-ipi	`d97b684`	link	false	`/test e2e-metal-ipi`
ci/prow/okd-scos-e2e-gcp-ovn-upgrade	`d97b684`	link	false	`/test okd-scos-e2e-gcp-ovn-upgrade`
ci/prow/e2e-libvirt	`d97b684`	link	false	`/test e2e-libvirt`
ci/prow/e2e-metal-assisted	`d97b684`	link	false	`/test e2e-metal-assisted`
ci/prow/okd-e2e-aws-ovn	`d97b684`	link	false	`/test okd-e2e-aws-ovn`
ci/prow/okd-scos-e2e-aws-ovn	`d97b684`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-ibmcloud-ovn	`d97b684`	link	false	`/test e2e-ibmcloud-ovn`
ci/prow/e2e-ovirt	`d97b684`	link	false	`/test e2e-ovirt`
ci/prow/okd-scos-e2e-gcp	`d97b684`	link	false	`/test okd-scos-e2e-gcp`
ci/prow/e2e-agent-mce	`d97b684`	link	false	`/test e2e-agent-mce`
ci/prow/okd-scos-e2e-aws-upgrade	`d97b684`	link	false	`/test okd-scos-e2e-aws-upgrade`
ci/prow/okd-e2e-aws-upgrade	`d97b684`	link	false	`/test okd-e2e-aws-upgrade`
ci/prow/e2e-gcp-ovn-shared-vpc	`d97b684`	link	false	`/test e2e-gcp-ovn-shared-vpc`

Full PR test history. Your PR dashboard.

Details

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-ci Bot requested review from AnnaZivkovic and sadasu September 12, 2022 21:03

deads2k mentioned this pull request Sep 12, 2022

[SPLAT-594] vSphere: add zonal spec to infrastructure object openshift/api#1278

Merged

wking reviewed Sep 13, 2022

View reviewed changes

Comment thread data/data/manifests/openshift/feature-gate.yaml.template Outdated

wking reviewed Sep 13, 2022

View reviewed changes

Comment thread pkg/asset/cluster/cluster.go Outdated

wking reviewed Sep 13, 2022

View reviewed changes

Comment thread pkg/types/validation/installconfig.go Outdated

wking reviewed Sep 13, 2022

View reviewed changes

Comment thread pkg/types/validation/installconfig.go Outdated

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2022

patrickdillon force-pushed the feature-flags branch from 84b714b to 97517a0 Compare October 3, 2022 20:07

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 3, 2022

patrickdillon force-pushed the feature-flags branch 4 times, most recently from 93906b9 to 0fbb2a6 Compare October 3, 2022 20:44

wking reviewed Oct 3, 2022

View reviewed changes

Comment thread pkg/asset/manifests/featuregate.go Outdated

wking reviewed Oct 3, 2022

View reviewed changes

Comment thread pkg/asset/manifests/featuregate.go Outdated

patrickdillon added 3 commits October 5, 2022 11:08

pkg/asset/cluster: warn when feature sets are enabled

26d6f52

Emit a warning when feature sets are enabled so that users understand the impact of the feature set.

patrickdillon force-pushed the feature-flags branch from 0fbb2a6 to 0d9b518 Compare October 5, 2022 15:41

barbacbd reviewed Oct 5, 2022

View reviewed changes

vSphere: set multizone as tech preview

d97b684

Enforces that vSphere multizone fields in the install config are only enabled with tech preview.

patrickdillon force-pushed the feature-flags branch from 0d9b518 to d97b684 Compare October 6, 2022 14:22

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 6, 2022

wking approved these changes Oct 6, 2022

View reviewed changes

openshift-ci Bot assigned wking Oct 6, 2022

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Oct 6, 2022

sadasu reviewed Oct 6, 2022

View reviewed changes

openshift-merge-robot merged commit cf68b81 into openshift:master Oct 6, 2022

Conversation

patrickdillon commented Sep 12, 2022

Uh oh!

patrickdillon commented Sep 12, 2022

Uh oh!

patrickdillon commented Sep 13, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

patrickdillon commented Oct 5, 2022

Uh oh!

jcpowermac commented Oct 5, 2022

Uh oh!

rvanderp3 commented Oct 5, 2022

Uh oh!

barbacbd Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon Oct 5, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon commented Oct 5, 2022

Uh oh!

sdodson commented Oct 6, 2022

Uh oh!

wking left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci Bot commented Oct 6, 2022

Uh oh!

sadasu Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

patrickdillon Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Oct 6, 2022

Uh oh!

sadasu Oct 6, 2022

Choose a reason for hiding this comment

Uh oh!

sadasu commented Oct 6, 2022

Uh oh!

patrickdillon commented Oct 6, 2022

Uh oh!

patrickdillon commented Oct 6, 2022

Uh oh!

patrickdillon commented Oct 6, 2022

Uh oh!

openshift-ci Bot commented Oct 6, 2022

Uh oh!

openshift-ci Bot commented Oct 6, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants