Skip to content

Drop support for OpenStack in-tree plugin#1443

Merged
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:drop-openstack-intree
Dec 13, 2022
Merged

Drop support for OpenStack in-tree plugin#1443
openshift-merge-robot merged 1 commit intoopenshift:masterfrom
shiftstack:drop-openstack-intree

Conversation

@mandre
Copy link
Copy Markdown
Member

@mandre mandre commented Dec 13, 2022

The in-tree openstack provider is gone from kube 1.26.

@JoelSpeed
Copy link
Copy Markdown
Contributor

This makes sense for the issue we are trying to solve, but I wonder where else this might be used, have we done a code search to work out where this is used to check this change isn't going to break other consumers?

@mandre
Copy link
Copy Markdown
Member Author

mandre commented Dec 13, 2022

This makes sense for the issue we are trying to solve, but I wonder where else this might be used, have we done a code search to work out where this is used to check this change isn't going to break other consumers?

Yes, it seems to me this code is only used in KCMO, where I want to use it.

https://github.com/search?q=org%3Aopenshift+GetPlatformName&type=code

@mandre
Copy link
Copy Markdown
Member Author

mandre commented Dec 13, 2022

Hold on, I notice we're also making a call to GetPlatformName() in ObserveCloudProviderNames(), but I think it's fine. Would appreciate a second pair of eyes though.

Perhaps I should also remove reference to openstack being a valid cloud provider.

The in-tree openstack provider is gone from kube 1.26.
@mandre mandre force-pushed the drop-openstack-intree branch from 5ee382b to b3ee2de Compare December 13, 2022 14:30
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 13, 2022

@mandre: all tests passed!

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.

@JoelSpeed
Copy link
Copy Markdown
Contributor

So this prevents the KCMO from getting the platform name for openstack, in turn preventing it setting the external cloud volume plugin for openstack which is desired.

The second effect here is that the ObserveCloudProviderNames historically has been responsible for copying the configmap from the openshift-config namespace to the operator namespace, seems like that's ok for the KCMO/KASO, is anything else using that observer? I know CCMO does the copy manually so that's ok, what does the storage operator do?

@mandre
Copy link
Copy Markdown
Member Author

mandre commented Dec 13, 2022

The second effect here is that the ObserveCloudProviderNames historically has been responsible for copying the configmap from the openshift-config namespace to the operator namespace, seems like that's ok for the KCMO/KASO, is anything else using that observer? I know CCMO does the copy manually so that's ok, what does the storage operator do?

According to this github code search, it's called only from KAS and KCM operators. I would imagine KAS no longer needs access (although this could break until openshift/kubernetes#1389 lands), I'm less sure about KCM.

@JoelSpeed
Copy link
Copy Markdown
Contributor

Ack, if that's the case and it's only used by those two, then I'm happy with the change, KCM won't need the configmap anymore

/lgtm

@openshift-ci openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Dec 13, 2022
Copy link
Copy Markdown

@soltysh soltysh left a comment

Choose a reason for hiding this comment

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

/approve

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 13, 2022

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed, mandre, soltysh

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:

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

@openshift-ci openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 13, 2022
@openshift-merge-robot openshift-merge-robot merged commit eaa3941 into openshift:master Dec 13, 2022
mandre added a commit to shiftstack/cluster-kube-controller-manager-operator that referenced this pull request Dec 14, 2022
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.

4 participants