Skip to content

chore: Removes unused in-tree csi logic #2005

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
appiepollo14 wants to merge 1 commit into
base: main
Choose a base branch
from
+5 −15
Open

chore: Removes unused in-tree csi logic #2005

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 5 additions & 8 deletions pkg/controller/machine/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ import (
"github.com/heptiolabs/healthcheck"
"github.com/prometheus/client_golang/prometheus"
"go.uber.org/zap"

"k8c.io/machine-controller/pkg/cloudprovider"
cloudprovidererrors "k8c.io/machine-controller/pkg/cloudprovider/errors"
"k8c.io/machine-controller/pkg/cloudprovider/instance"
Expand Down Expand Up @@ -287,7 +286,7 @@ func enqueueRequestsForNodes(ctx context.Context, log *zap.SugaredLogger, mgr ma
})
}

// 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.
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

Comment grammar: "if its set" should be "if it's set".

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

The updated comment still contains a grammatical issue: "if its set" should be "if it's set" (or "if it is set"). Since this line was touched, it’s a good chance to fix the remaining typo.

Suggested change
// 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.

Copilot uses AI. Check for mistakes.
// It does not return an error as it's used around the sync handler.
func (r *Reconciler) clearMachineError(machine *clusterv1alpha1.Machine) {
if machine.Status.ErrorMessage != nil || machine.Status.ErrorReason != nil {
Expand Down Expand Up @@ -456,9 +455,8 @@ func (r *Reconciler) reconcile(ctx context.Context, log *zap.SugaredLogger, mach
return r.ensureInstanceExistsForMachine(ctx, log, prov, machine, providerConfig)
}

// case 3.2: if the node exists and both external and internal CCM are not available. Then set the provider-id for the node.
inTree := providerconfig.IntreeCloudProviderImplementationSupported(providerConfig.CloudProvider)
if !inTree && !r.nodeSettings.ExternalCloudProvider && node.Spec.ProviderID == "" {
// 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
Comment on lines +458 to 462
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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).

Copilot uses AI. Check for mistakes.
Comment on lines +458 to 462
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -899,9 +897,8 @@ func (r *Reconciler) ensureInstanceExistsForMachine(

var providerID string
if machine.Spec.ProviderID == nil {
inTree := providerconfig.IntreeCloudProviderImplementationSupported(providerConfig.CloudProvider)
// If both external and internal CCM are not available. We set provider-id for the machine explicitly.
if !inTree && !r.nodeSettings.ExternalCloudProvider {
// If both external CCM is not available. We set provider-id for the machine explicitly.
if !r.nodeSettings.ExternalCloudProvider {
Comment on lines +900 to +901
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// 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 {

Copilot uses AI. Check for mistakes.
providerID = fmt.Sprintf(ProviderIDPattern, providerConfig.CloudProvider, machine.UID)
}
Comment on lines 898 to 903
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

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

Similarly, machine.Spec.ProviderID is now set whenever ExternalCloudProvider is false, using the kubermatic://... pattern. If a CCM/in-tree cloud provider is expected to manage provider IDs for some configurations, pre-populating this field can lead to inconsistent providerID semantics across clusters. Consider clarifying the supported CCM modes or gating this behavior to only the configurations where a CCM will not set providerID.

Copilot uses AI. Check for mistakes.
}
Expand Down
7 changes: 0 additions & 7 deletions sdk/providerconfig/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,13 +117,6 @@ var (
}
)

Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
// 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

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Member

@kron4eg kron4eg Mar 24, 2026

Choose a reason for hiding this comment

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

@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
}

func IntreeCloudProviderImplementationSupported(cloudProvider CloudProvider) (inTree bool) {
if cloudProvider == CloudProviderAzure || cloudProvider == CloudProviderVsphere || cloudProvider == CloudProviderGoogle {
return true
}
return false
}

// DNSConfig contains a machine's DNS configuration.
type DNSConfig struct {
Servers []string `json:"servers"`
Expand Down
Loading