OCPCLOUD-2514,OCPCLOUD-2516: Remove reliance on feature gates for cloud provider flags #4228

JoelSpeed · 2024-02-29T12:11:57Z

In 4.15 the final feature gates were promoted for the CCMs, all in-tree code is now disabled and the decision as to whether the --cloud-provider flag should be set to external has been simplified to a case of, which platform are we on.
Additionally the --cloud-config flag is no longer required at all.

This PR removes the reliance on the feature gate, removes the cloud config flag and then removes a bunch of feature gates being passed around where they aren't needed.
The latter part of this may need to be reverted in the future if templates regain a need for feature gates, but in theory it should be pretty easy to plumb back through by reverting the commit.

In 4.15, the final CCM was promoted to out of tree, therefore this flag has been set to the empty string for all 4.15 kubelets. We no longer need to set the flag at all, under any condition.

openshift-ci-robot · 2024-02-29T12:12:02Z

@JoelSpeed: This pull request references OCPCLOUD-2514 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

In 4.15 the final feature gates were promoted for the CCMs, all in-tree code is now disabled and the decision as to whether the --cloud-provider flag should be set to external has been simplified to a case of, which platform are we on.
Additionally the --cloud-config flag is no longer required at all.

This PR removes the reliance on the feature gate, removes the cloud config flag and then removes a bunch of feature gates being passed around where they aren't needed.
The latter part of this may need to be reverted in the future if templates regain a need for feature gates, but in theory it should be pretty easy to plumb back through by reverting the commit.

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 openshift-eng/jira-lifecycle-plugin repository.

cdoern

looks good but:

The latter part of this may need to be reverted in the future if templates regain a need for feature gates, but in theory it should be pretty easy to plumb back through by reverting the commit.

concerns me a bit. I know we put a lot of this plumbing around the MCC for the MCN featuregate. Is there any benefit to leaving this stuff in since we are adding some features behind FG in 4.16?

JoelSpeed · 2024-03-01T10:38:03Z

I know we put a lot of this plumbing around the MCC for the MCN featuregate. Is there any benefit to leaving this stuff in since we are adding some features behind FG in 4.16?

I deliberately put this as a separate commit as I wasn't sure if you'd want this unwound or not. Currently, there's nothing within the template controller that requires feature gates, so the PR cleans up the unused variables. I can however revert this last commit and the code still compiles, let me know which you'd prefer, to keep the plumbing or clean it up.

Are you aware of any of the TP features needing to use the template controller?

openshift-ci-robot · 2024-03-01T10:38:25Z

@JoelSpeed: This pull request references OCPCLOUD-2514 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

This pull request references OCPCLOUD-2516 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.16.0" version, but no target version was set.

In response to this:

In 4.15 the final feature gates were promoted for the CCMs, all in-tree code is now disabled and the decision as to whether the --cloud-provider flag should be set to external has been simplified to a case of, which platform are we on.
Additionally the --cloud-config flag is no longer required at all.

This PR removes the reliance on the feature gate, removes the cloud config flag and then removes a bunch of feature gates being passed around where they aren't needed.
The latter part of this may need to be reverted in the future if templates regain a need for feature gates, but in theory it should be pretty easy to plumb back through by reverting the commit.

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 openshift-eng/jira-lifecycle-plugin repository.

yuqi-zhang

I think this should be safe, one question on the containerruntimeconfigcontroller inline

yuqi-zhang · 2024-03-01T22:06:58Z

pkg/controller/container-runtime-config/container_runtime_config_controller.go

 	clusterVersionLister       cligolistersv1.ClusterVersionLister
 	clusterVersionListerSynced cache.InformerSynced

-	featureGateAccess featuregates.FeatureGateAccess


re: containerruntimeconfigcontroller specifically, I know @QiWang19 has been working on #4160 as a FG'ed feature. Would that conflict at all?

yes, it's a conflict, #4160 relies on this field to check if the API that containerruntime config watching for is featuregate enabled.

How do you want to handle this then? I can drop the specific CRC parts, drop the whole commit, or Qi can handle the conflict if this merges first. Any preference?

I don't have strong feelings here, @QiWang19 what would work for you?

can we drop the specific CRC parts from this PR? so we don't have to have separate PR deleting and adding the same lines of code.

I've reviewed the other PR and updated the unwinding here to not conflict, I've still removed from generateOriginalContainerRuntimeConfigs but as far as I can tell that shouldn't conflict with your PR

@JoelSpeed Thanks

yuqi-zhang · 2024-03-01T22:07:40Z

pkg/controller/template/render.go

 	*mcfgv1.ControllerConfigSpec
 	PullSecret                 string
 	InternalRegistryPullSecret string
-	FeatureGateAccess          featuregates.FeatureGateAccess


I don't think the template/render controller controls any FG'ed items and there shouldn't be any plans to immediately, so I'm fine with this (but please correct me if I'm missing something)

yuqi-zhang

/lgtm

updated changes look good, thanks! There's 2 minor verify failures that need to be fixed

yuqi-zhang · 2024-03-11T17:59:21Z

pkg/controller/container-runtime-config/container_runtime_config_bootstrap.go

 			}
 			role := pool.Name
 			// Generate the original ContainerRuntimeConfig
-			originalStorageIgn, _, _, err := generateOriginalContainerRuntimeConfigs(templateDir, controllerConfig, role, featureGateAccess)


verify is complaining that the function header still has featuregate being passed in

yuqi-zhang · 2024-03-11T18:00:12Z

pkg/controller/container-runtime-config/container_runtime_config_controller.go

 	)

 	// Generate the original registries config
-	_, originalRegistriesIgn, originalPolicyIgn, err := generateOriginalContainerRuntimeConfigs(templateDir, controllerConfig, role, featureGateAccess)


same for the registriesConfigIgnition function

openshift-ci-robot · 2024-03-11T18:35:43Z

/retest-required

Remaining retests: 0 against base HEAD da2a7ae and 2 for PR HEAD 4680a9c in total

QiWang19 · 2024-03-11T20:15:21Z

For adbd27b, I can handle the conflict when after this merged.

yuqi-zhang · 2024-03-12T00:00:01Z

pkg/controller/container-runtime-config/container_runtime_config_controller.go

 func registriesConfigIgnition(templateDir string, controllerConfig *mcfgv1.ControllerConfig, role, releaseImage string,
 	insecureRegs, registriesBlocked, policyBlocked, allowedRegs, searchRegs []string,
-	icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy, idmsRules []*apicfgv1.ImageDigestMirrorSet, itmsRules []*apicfgv1.ImageTagMirrorSet, featureGateAccess featuregates.FeatureGateAccess) (*ign3types.Config, error) {
+	icspRules []*apioperatorsv1alpha1.ImageContentSourcePolicy, idmsRules []*apicfgv1.ImageDigestMirrorSet, itmsRules []*apicfgv1.ImageTagMirrorSet, featureGateAccess featuregates.FeatureGateAccess) (*ign3types.Config, error) { //nolint:revive


Now this is complaining about Category: unparam instead

Since Qi is fine with resolving conflicts, maybe it's easier to remove them. I'm fine either way

JoelSpeed · 2024-03-12T13:46:22Z

@yuqi-zhang @QiWang19 I've got the nolint directive working, so Qi if you could unwind the nolint in your PR, that would be appreciated

damdo

Re-adding LGTM after lint fix
/lgtm

elmiko

/lgtm

openshift-ci · 2024-03-12T13:54:36Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cdoern, damdo, elmiko, JoelSpeed, yuqi-zhang

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~~ [cdoern,yuqi-zhang]

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

JoelSpeed · 2024-03-12T14:31:30Z

/test e2e-hypershift

openshift-ci · 2024-03-12T16:05:47Z

@JoelSpeed: 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/okd-scos-e2e-aws-ovn	`c1553d7`	link	false	`/test okd-scos-e2e-aws-ovn`

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.

QiWang19 · 2024-03-12T16:57:07Z

@JoelSpeed Thanks for your consideration.

openshift-bot · 2024-03-12T22:52:17Z

[ART PR BUILD NOTIFIER]

This PR has been included in build ose-machine-config-operator-container-v4.16.0-202403122214.p0.g562ef07.assembly.stream.el8 for distgit ose-machine-config-operator.
All builds following this will include this PR.

JoelSpeed added 3 commits February 29, 2024 11:03

Update library-go

1ff7e3b

Remove reliance on feature gate from cloud provider flags

1c389d3

Remove cloudConfigFlag from Kubelet Definitions

b4d5776

In 4.15, the final CCM was promoted to out of tree, therefore this flag has been set to the empty string for all 4.15 kubelets. We no longer need to set the flag at all, under any condition.

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Feb 29, 2024

openshift-ci bot requested review from dkhater-redhat and umohnani8 February 29, 2024 12:12

cdoern approved these changes Feb 29, 2024

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 29, 2024

JoelSpeed mentioned this pull request Mar 1, 2024

OCPCLOUD-2561: Remove CloudProviderExternal feature gates openshift/api#1780

Merged

JoelSpeed changed the title ~~OCPCLOUD-2514: Remove reliance on feature gates for cloud provider flags~~ OCPCLOUD-2514,OCPCLOUD-2516: Remove reliance on feature gates for cloud provider flags Mar 1, 2024

yuqi-zhang reviewed Mar 1, 2024

View reviewed changes

Unwind feature gates from template controller

4680a9c

JoelSpeed force-pushed the remove-cloud-fg branch from 1ae2c05 to 4680a9c Compare March 11, 2024 16:59

yuqi-zhang approved these changes Mar 11, 2024

View reviewed changes

openshift-ci bot assigned yuqi-zhang Mar 11, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2024

yuqi-zhang reviewed Mar 11, 2024

View reviewed changes

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Mar 11, 2024

JoelSpeed force-pushed the remove-cloud-fg branch from 24fb177 to adbd27b Compare March 11, 2024 19:50

yuqi-zhang reviewed Mar 12, 2024

View reviewed changes

Add temporary nolint for unused FG params

c1553d7

JoelSpeed force-pushed the remove-cloud-fg branch from adbd27b to c1553d7 Compare March 12, 2024 10:38

damdo reviewed Mar 12, 2024

View reviewed changes

elmiko reviewed Mar 12, 2024

View reviewed changes

openshift-ci bot assigned damdo Mar 12, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 12, 2024

openshift-ci bot assigned elmiko Mar 12, 2024

openshift-merge-bot bot merged commit 562ef07 into openshift:master Mar 12, 2024

OCPCLOUD-2514,OCPCLOUD-2516: Remove reliance on feature gates for cloud provider flags #4228

OCPCLOUD-2514,OCPCLOUD-2516: Remove reliance on feature gates for cloud provider flags #4228

Uh oh!

Conversation

JoelSpeed commented Feb 29, 2024

Uh oh!

openshift-ci-robot commented Feb 29, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cdoern left a comment

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Mar 1, 2024

Uh oh!

openshift-ci-robot commented Mar 1, 2024 • edited by openshift-ci bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yuqi-zhang left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

openshift-ci-robot commented Mar 11, 2024

Uh oh!

QiWang19 commented Mar 11, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Mar 12, 2024

Uh oh!

damdo left a comment

Choose a reason for hiding this comment

Uh oh!

elmiko left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Mar 12, 2024

Uh oh!

JoelSpeed commented Mar 12, 2024

Uh oh!

openshift-ci bot commented Mar 12, 2024

Uh oh!

QiWang19 commented Mar 12, 2024

Uh oh!

openshift-bot commented Mar 12, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

openshift-ci-robot commented Feb 29, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 1, 2024 •

edited by openshift-ci bot

Loading

yuqi-zhang left a comment •

edited

Loading