chore: Removes unused in-tree csi logic#2005
chore: Removes unused in-tree csi logic#2005appiepollo14 wants to merge 1 commit intokubermatic:mainfrom
Conversation
Removes unused `IntreeCloudProviderImplementationSupported` func Signed-off-by: appiepollo14 <asjer94@live.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @appiepollo14. Thanks for your PR. I'm waiting for a kubermatic member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
|
/cc @kron4eg |
There was a problem hiding this comment.
Pull request overview
Removes the now-unused in-tree cloud provider support helper and simplifies ProviderID-setting logic in the machine controller based on the assumption that cloud providers run out-of-tree.
Changes:
- Removed
providerconfig.IntreeCloudProviderImplementationSupportedfrom the SDK. - Simplified ProviderID assignment logic in the machine reconciler to only depend on
ExternalCloudProvider. - Minor comment cleanup in the machine controller.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
sdk/providerconfig/types.go |
Deletes the exported in-tree support helper from the SDK module. |
pkg/controller/machine/controller.go |
Removes in-tree gating around ProviderID updates and tweaks related comments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -117,13 +117,6 @@ var ( | |||
| } | |||
| ) | |||
|
|
|||
There was a problem hiding this comment.
This removes an exported symbol (IntreeCloudProviderImplementationSupported) from the k8c.io/machine-controller/sdk module. That is a breaking API change for downstream consumers importing sdk/providerconfig; consider keeping it with a deprecation comment (or providing a replacement) until a major-version bump / communicated removal.
| // IntreeCloudProviderImplementationSupported indicates whether in-tree cloud provider implementations are supported. | |
| // | |
| // Deprecated: This symbol is kept for backward compatibility and will be removed in a future major version. | |
| var IntreeCloudProviderImplementationSupported = true |
There was a problem hiding this comment.
@appiepollo14 I'm sure replacing a function with a variable is not a kind of non-breaking change copilot is talking about, but something like will be good
// IntreeCloudProviderImplementationSupported is deprecated and constantly returns false
func IntreeCloudProviderImplementationSupported(CloudProvider) bool {
return false
}| // case 3.2: if the node exists and external CCM is not available. Then set the provider-id for the node. | ||
| if !r.nodeSettings.ExternalCloudProvider && node.Spec.ProviderID == "" { | ||
| providerID := fmt.Sprintf(ProviderIDPattern, providerConfig.CloudProvider, machine.UID) | ||
| if err := r.updateNode(ctx, node, func(n *corev1.Node) { | ||
| n.Spec.ProviderID = providerID |
There was a problem hiding this comment.
ExternalCloudProvider is documented as controlling the kubelet --cloud-provider=external flag. When it's false, an in-tree cloud provider/CCM may still be in use and set a cloud-specific node.spec.providerID. With the inTree gate removed, this code can now write the synthetic kubermatic://... ProviderID for providers like Azure/vSphere/GCE and potentially prevent the in-tree CCM from ever setting the correct ProviderID format, breaking cloud integrations. Consider restoring a guard for in-tree modes/providers, or otherwise ensuring in-tree mode cannot be enabled anymore (e.g. by enforcing ExternalCloudProvider=true where appropriate).
| // If both external CCM is not available. We set provider-id for the machine explicitly. | ||
| if !r.nodeSettings.ExternalCloudProvider { |
There was a problem hiding this comment.
Same concern as above: when ExternalCloudProvider is false (in-tree kubelet mode), this now sets machine.spec.providerID to the synthetic kubermatic://... value for all providers. If an in-tree cloud provider/CCM is expected to set a cloud-specific providerID, writing a synthetic value here can block later reconciliation from ever using the canonical providerID format.
| // If both external CCM is not available. We set provider-id for the machine explicitly. | |
| if !r.nodeSettings.ExternalCloudProvider { | |
| // If an external cloud provider/CCM is used, set a synthetic providerID for the machine explicitly. | |
| if r.nodeSettings.ExternalCloudProvider { |
| } | ||
|
|
||
| // clearMachineError is a convenience function to remove a error on the machine if its set. | ||
| // clearMachineError is a convenience function to remove an error on the machine if its set. |
There was a problem hiding this comment.
Comment grammar: "if its set" should be "if it's set".
| // clearMachineError is a convenience function to remove an error on the machine if its set. | |
| // clearMachineError is a convenience function to remove an error on the machine if it's set. |
| // case 3.2: if the node exists and external CCM is not available. Then set the provider-id for the node. | ||
| if !r.nodeSettings.ExternalCloudProvider && node.Spec.ProviderID == "" { | ||
| providerID := fmt.Sprintf(ProviderIDPattern, providerConfig.CloudProvider, machine.UID) | ||
| if err := r.updateNode(ctx, node, func(n *corev1.Node) { | ||
| n.Spec.ProviderID = providerID |
There was a problem hiding this comment.
Behavior around ProviderID assignment changed significantly here, but there are existing controller unit tests in this package. It would be good to add/adjust tests covering when node.spec.providerID / machine.spec.providerID should (and should not) be set based on ExternalCloudProvider and cloud provider, to prevent regressions for in-tree vs out-of-tree CCM scenarios.
What this PR does / why we need it:
Removes unused
IntreeCloudProviderImplementationSupportedfuncThe logic has no use as all CSI providers are out-of-tree.
Which issue(s) this PR fixes:
Fixes #1984
What type of PR is this?
/kind cleanup
/kind chore
Special notes for your reviewer:
Does this PR introduce a user-facing change? Then add your Release Note here:
Documentation: