OSASINFRA-3652: openstack-cinder: Use new --with-topology flag #345

stephenfin · 2024-12-10T13:23:44Z

Rather than relying on the soon-to-be-removed feature gate.

/hold

~~This requires kubernetes/cloud-provider-openstack#2743~~

EDIT: This is now available on the release-4.18 and release-4.19 branches openshift/cloud-provider-openstack@6cb478d

openshift-ci-robot · 2024-12-10T13:23:49Z

@stephenfin: This pull request references OSASINFRA-3652 which is a valid jira issue.

In response to this:

Rather than relying on the soon-to-be-removed feature gate.

/hold

This requires kubernetes/cloud-provider-openstack#2743

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.

openshift-ci · 2024-12-10T13:23:51Z

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

openshift-ci-robot · 2025-03-10T09:18:30Z

@stephenfin: This pull request references OSASINFRA-3652 which is a valid jira issue.

In response to this:

Rather than relying on the soon-to-be-removed feature gate.

/hold

~~This requires kubernetes/cloud-provider-openstack#2743~~

This is now available on the release-4.18 and release-4.19 branches openshift/cloud-provider-openstack@6cb478d

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.

openshift-ci-robot · 2025-03-10T09:18:42Z

@stephenfin: This pull request references OSASINFRA-3652 which is a valid jira issue.

In response to this:

Rather than relying on the soon-to-be-removed feature gate.

/hold

~~This requires kubernetes/cloud-provider-openstack#2743~~

EDIT: This is now available on the release-4.18 and release-4.19 branches openshift/cloud-provider-openstack@6cb478d

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.

stephenfin · 2025-03-10T09:19:00Z

/hold cancel

mdbooth · 2025-03-10T09:51:55Z

This changes a command line argument to the provisioner from:

        - --feature-gates=Topology=$(ENABLE_TOPOLOGY)

to

        - --feature-gates=Topology=true
        - --with-topology=$(ENABLE_TOPOLOGY)

So we're now enabling the feature gate unconditionally, but disabling it when ENABLE_TOPOLOGY is false. Why do we enable it when ENABLE_TOPOLOGY is false? Does it do anything else? Or is it just for simpler templating?

stephenfin · 2025-03-10T10:04:11Z

This changes a command line argument to the provisioner from:
        - --feature-gates=Topology=$(ENABLE_TOPOLOGY)
to
        - --feature-gates=Topology=true
        - --with-topology=$(ENABLE_TOPOLOGY)
So we're now enabling the feature gate unconditionally, but disabling it when ENABLE_TOPOLOGY is false. Why do we enable it when ENABLE_TOPOLOGY is false? Does it do anything else? Or is it just for simpler templating?

We were previously relying on the feature gate to enable/disable the feature, but that feature gate is planned for removal in a future release. As a result, we've added a new boolean option to enable/disable the feature, which we can use in place of the feature gate. The reason I kept the feature gate flag was because I didn't know whether this defaulted to enabled or disabled, however, your comment was enough for me to go investigate, and it seems this defaults to true. I will push a follow-up commit to remove this option shortly.

mdbooth · 2025-03-10T10:15:02Z

Thanks for the explanation.

/lgtm

stephenfin · 2025-03-11T11:04:08Z

/retest

stephenfin · 2025-03-11T21:38:25Z

/retest

stephenfin · 2025-03-12T09:23:28Z

/test e2e-openstack

openshift-ci · 2025-03-12T16:19:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: stephenfin
Once this PR has been reviewed and has the lgtm label, please assign jsafrane for approval. For more information see the Code Review Process.

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

Needs approval from an approver in each of these files:

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

stephenfin · 2025-03-19T23:35:20Z

/retest

stephenfin · 2025-03-20T09:11:09Z

/test e2e-openstack

mdbooth

This is now keyed from the enable_topology key in some config map. I'm guessing the configmap is the one produced by configsync from merging these 2 configmaps:

csi-operator/pkg/openstack-cinder/config/configsync.go

Lines 118 to 125 in 608149d

    
           configSources = []configSource{ 
        
           	// First, we try to retrieve from the Cinder CSI-specific config map 
        
           	{util.OpenShiftConfigNamespace, "cinder-csi-config", "config"}, 
        
           	// Failing that, we attempt to retrieve from the cloud provider-specific config map 
        
           	// TODO(stephenfin): We should stop retrieving this once Installer creates the new 
        
           	// config, which will allow us to simplify things somewhat here 
        
           	{util.OpenShiftConfigNamespace, infra.Spec.CloudConfig.Name, infra.Spec.CloudConfig.Key}, 
        
           }

It seems we've been using this key for a while, so guessing there's no upgrade or documentation impact here.

By my admittedly limited understand of how we provision this driver this looks fine to me. I have one nit. If it's there for a reason, a comment would be good because it looks weird, but it's not important.

/lgtm

mdbooth · 2025-03-20T13:29:44Z

assets/overlays/openstack-cinder/generated/standalone/controller.yaml

-            configMapKeyRef:
-              key: enable_topology
-              name: cloud-conf
+        env: []


nit: omit this?.

This is auto-generated (and generated from scratch each time rather than updated) so I'm thinking I can't.

assets/overlays/openstack-cinder/patches/node_add_driver.yaml

rlobillo · 2025-03-20T14:58:35Z

According to https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/test-platform-results/pr-logs/pull/openshift_csi-operator/345/pull-ci-openshift-csi-operator-main-e2e-openstack-cinder-csi/1902504203914317824/artifacts/e2e-openstack-cinder-csi/openshift-e2e-test/build-log.txt, the cinder-csi tests are passing so we are ready to merge it and run full regresion on 4.19 release.

/label qe-approved

openshift-ci-robot · 2025-03-20T14:58:44Z

@stephenfin: This pull request references OSASINFRA-3652 which is a valid jira issue.

In response to this:

Rather than relying on the soon-to-be-removed feature gate.

/hold

~~This requires kubernetes/cloud-provider-openstack#2743~~

EDIT: This is now available on the release-4.18 and release-4.19 branches openshift/cloud-provider-openstack@6cb478d

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.

stephenfin · 2025-03-21T15:47:55Z

/hold

While we investigate a potential QE failure.

Rather than relying on the soon-to-be-removed feature gate. Signed-off-by: Stephen Finucane <[email protected]>

This defaults to true [1], meaning there is no reason to set it manually. [1] https://github.com/kubernetes-csi/external-provisioner/blob/c7277b7a5181ff6eab89dc9e36390306417471d4/pkg/features/features.go#L63 Signed-off-by: Stephen Finucane <[email protected]>

openshift-ci · 2025-03-24T14:53:09Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2025-03-24T18:44:50Z

@stephenfin: 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/hypershift-e2e-aks	`53ee18f`	link	true	`/test hypershift-e2e-aks`
ci/prow/okd-scos-e2e-aws-ovn	`53ee18f`	link	false	`/test okd-scos-e2e-aws-ovn`
ci/prow/e2e-openstack-cinder-csi	`53ee18f`	link	true	`/test e2e-openstack-cinder-csi`
ci/prow/e2e-openstack	`53ee18f`	link	false	`/test e2e-openstack`

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-sigs/prow repository. I understand the commands that are listed here.

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

openshift-ci bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. labels Dec 10, 2024

stephenfin force-pushed the OSASINFRA-3652 branch from 3db5d09 to f051ebe Compare March 10, 2025 09:15

stephenfin marked this pull request as ready for review March 10, 2025 09:17

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 10, 2025

openshift-ci bot requested review from EmilienM and mpatlasov March 10, 2025 09:18

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 10, 2025

openshift-ci bot assigned mdbooth Mar 10, 2025

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

stephenfin force-pushed the OSASINFRA-3652 branch from 63d5579 to 56eb54b Compare March 10, 2025 16:30

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

stephenfin force-pushed the OSASINFRA-3652 branch from 56eb54b to 574f975 Compare March 12, 2025 16:13

stephenfin force-pushed the OSASINFRA-3652 branch from 574f975 to 7a32614 Compare March 12, 2025 18:23

mdbooth reviewed Mar 20, 2025

View reviewed changes

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

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Mar 20, 2025

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 21, 2025

stephenfin added 2 commits March 24, 2025 14:45

OSASINFRA-3652: openstack-cinder: Use new --with-topology flag

2029bc4

Rather than relying on the soon-to-be-removed feature gate. Signed-off-by: Stephen Finucane <[email protected]>

stephenfin force-pushed the OSASINFRA-3652 branch from 7a32614 to 53ee18f Compare March 24, 2025 14:53

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OSASINFRA-3652: openstack-cinder: Use new --with-topology flag #345

OSASINFRA-3652: openstack-cinder: Use new --with-topology flag #345

stephenfin commented Dec 10, 2024 •

edited

Loading

openshift-ci-robot commented Dec 10, 2024 •

edited by openshift-ci bot

Loading

openshift-ci bot commented Dec 10, 2024

openshift-ci-robot commented Mar 10, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 10, 2025 •

edited by openshift-ci bot

Loading

stephenfin commented Mar 10, 2025

mdbooth commented Mar 10, 2025

stephenfin commented Mar 10, 2025

mdbooth commented Mar 10, 2025

stephenfin commented Mar 11, 2025

stephenfin commented Mar 11, 2025

stephenfin commented Mar 12, 2025

openshift-ci bot commented Mar 12, 2025

stephenfin commented Mar 19, 2025

stephenfin commented Mar 20, 2025

mdbooth left a comment

mdbooth Mar 20, 2025

stephenfin Mar 20, 2025

rlobillo commented Mar 20, 2025 •

edited

Loading

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading

stephenfin commented Mar 21, 2025

openshift-ci bot commented Mar 24, 2025

openshift-ci bot commented Mar 24, 2025

	configSources = []configSource{
	// First, we try to retrieve from the Cinder CSI-specific config map
	{util.OpenShiftConfigNamespace, "cinder-csi-config", "config"},
	// Failing that, we attempt to retrieve from the cloud provider-specific config map
	// TODO(stephenfin): We should stop retrieving this once Installer creates the new
	// config, which will allow us to simplify things somewhat here
	{util.OpenShiftConfigNamespace, infra.Spec.CloudConfig.Name, infra.Spec.CloudConfig.Key},
	}

OSASINFRA-3652: openstack-cinder: Use new --with-topology flag #345

Are you sure you want to change the base?

OSASINFRA-3652: openstack-cinder: Use new --with-topology flag #345

Conversation

stephenfin commented Dec 10, 2024 • edited Loading

openshift-ci-robot commented Dec 10, 2024 • edited by openshift-ci bot Loading

openshift-ci bot commented Dec 10, 2024

openshift-ci-robot commented Mar 10, 2025 • edited by openshift-ci bot Loading

openshift-ci-robot commented Mar 10, 2025 • edited by openshift-ci bot Loading

stephenfin commented Mar 10, 2025

mdbooth commented Mar 10, 2025

stephenfin commented Mar 10, 2025

mdbooth commented Mar 10, 2025

stephenfin commented Mar 11, 2025

stephenfin commented Mar 11, 2025

stephenfin commented Mar 12, 2025

openshift-ci bot commented Mar 12, 2025

stephenfin commented Mar 19, 2025

stephenfin commented Mar 20, 2025

mdbooth left a comment

Choose a reason for hiding this comment

mdbooth Mar 20, 2025

Choose a reason for hiding this comment

stephenfin Mar 20, 2025

Choose a reason for hiding this comment

rlobillo commented Mar 20, 2025 • edited Loading

openshift-ci-robot commented Mar 20, 2025 • edited by openshift-ci bot Loading

stephenfin commented Mar 21, 2025

openshift-ci bot commented Mar 24, 2025

openshift-ci bot commented Mar 24, 2025

stephenfin commented Dec 10, 2024 •

edited

Loading

openshift-ci-robot commented Dec 10, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 10, 2025 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Mar 10, 2025 •

edited by openshift-ci bot

Loading

rlobillo commented Mar 20, 2025 •

edited

Loading

openshift-ci-robot commented Mar 20, 2025 •

edited by openshift-ci bot

Loading