Cleanup NICs when migrate from openstack MCM by dergeberl · Pull Request #89 · stackitcloud/machine-controller-manager-provider-stackit

dergeberl · 2026-01-22T09:55:35Z

How to categorize this PR?

/kind enhancement

/hold
wip
What this PR does / why we need it:

This PR clean up NICs associated with servers when deleting machines.

Special notes for your reviewer:

Breaking changes:

ske-prow · 2026-01-22T09:55:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

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

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

Details

Needs approval from an approver in each of these files:

OWNERS

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

stackit-ske · 2026-02-06T16:52:41Z

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Mark this PR as rotten with /lifecycle rotten
Close this PR with /close

/lifecycle stale

stackit-ske · 2026-02-21T17:44:48Z

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle rotten
Close this PR with /close

/lifecycle rotten

stackit-ske · 2026-02-28T18:05:41Z

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen
Mark this PR as fresh with /remove-lifecycle rotten

/close

ske-prow · 2026-02-28T18:05:45Z

@stackit-ske: Closed this PR.

Details

In response to this:

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied

After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied

After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Reopen this PR with /reopen

Mark this PR as fresh with /remove-lifecycle rotten

/close

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.

dergeberl · 2026-03-02T07:35:23Z

/reopen

ske-prow · 2026-03-02T07:35:27Z

@dergeberl: Reopened this PR.

Details

In response to this:

/reopen

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.

dergeberl · 2026-03-02T07:35:39Z

/remove-lifecycle rotten

nschad · 2026-03-02T08:48:31Z

+
+	nics := make([]*NIC, 0)
+	for _, nic := range *res.Items {
+		nics = append(nics, convertSDKNICtoNIC(&nic))


Is there a reason we use these convertSDKNICtoNIC? I know we did this also for servers but IMO there is really no reason, we could just use the SDK Type.

stackit-ske · 2026-03-17T08:56:02Z

Our project currently lacks enough active contributors to adequately respond to all PRs.
This bot triages PRs according to the following rules:

After 15d of inactivity, lifecycle/stale is applied
After 15d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
After 7d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

Mark this PR as fresh with /remove-lifecycle stale
Mark this PR as rotten with /lifecycle rotten
Close this PR with /close

/lifecycle stale

dergeberl · 2026-03-20T10:56:39Z

/remove-lifecycle stale

# Conflicts: # pkg/client/sdk.go # pkg/client/stackit.go # pkg/provider/core.go

nschad · 2026-09-07T18:46:14Z


+	if len(servers) > 1 {
+		klog.Errorf("Multiple servers already exists for this machine %q: %v", req.Machine.Name, err)
+		return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("failed to fetch server: %v", err))


Suggested change

return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("failed to fetch server: %v", err))

return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("multiple servers found: %v", err))

Also err is most likely nil here. Refactor this. We should print here all the ids or so

nschad · 2026-09-07T18:47:36Z

+func (p *Provider) serverIDsForMachine(ctx context.Context, req *driver.DeleteMachineRequest, secretProjectID, region string, migrated bool) (projectID string, serverIDs []string, err error) {
+	projectID, serverIDs = "", nil
+	if providerID := req.Machine.Spec.ProviderID; providerID != "" {
+		if !strings.HasPrefix(providerID, StackitProviderName) {


Suggested change

if !strings.HasPrefix(providerID, StackitProviderName) {

if !strings.HasPrefix(providerID, StackitProviderName + "://") {

This only checks if the prefix is "stackit"

nschad · 2026-09-07T18:53:10Z

+
+	nics := make([]*NIC, 0, len(res.Items))
+	for i := range res.Items {
+		nics = append(nics, convertSDKNICtoNIC(&res.Items[i]))


can be addressed by index since the sice is known, instead of using append()

nschad · 2026-09-07T18:54:11Z

+func getStringValue(s *string) string {
+	if s == nil {
+		return ""
+	}
+	return *s
+}


Suggested change

func getStringValue(s *string) string {

if s == nil {

return ""

}

return *s

}

nschad · 2026-09-07T18:54:22Z

 		AllowedAddresses: addresses,
 		IPv4:             nic.GetIpv4(),
 		IPv6:             nic.GetIpv6(),
+		Name:             getStringValue(nic.Name),


Suggested change

Name: getStringValue(nic.Name),

Name: nic.GetName(),

nschad · 2026-09-07T19:00:56Z

+		return nil, status.Error(codes.Internal, err.Error())
+	}
+
+	migrated, _ := strconv.ParseBool(req.Machine.Annotations[migratedMachineAnnotation])


Should add comment here that missing annotation is to be treated as false

Also the error of strconv.ParseBool() is ignored!

nschad · 2026-09-07T19:01:31Z

+	}
+
+	migrated, _ := strconv.ParseBool(req.Machine.Annotations[migratedMachineAnnotation])
+	projectID, serverIDs, err := p.serverIDsForMachine(ctx, req, projectIDFromSecret, providerSpec.Region, migrated)


Why does this return multiple serverIDs?

nschad · 2026-09-07T19:02:27Z


-		// Parse ProviderID to extract projectID and serverID
-		projectID, serverID, err = parseProviderID(req.Machine.Spec.ProviderID)
+func (p *Provider) serverIDsForMachine(ctx context.Context, req *driver.DeleteMachineRequest, secretProjectID, region string, migrated bool) (projectID string, serverIDs []string, err error) {


Suggested change

func (p *Provider) serverIDsForMachine(ctx context.Context, req *driver.DeleteMachineRequest, secretProjectID, region string, migrated bool) (projectID string, serverIDs []string, err error) {

func (p *Provider) serverIDsForMachine(ctx context.Context, req *driver.DeleteMachineRequest, projectIDFromSecret, region string, migrated bool) (projectID string, serverIDs []string, err error) {

Add massive docstring. Explain in detail what this does

nschad · 2026-09-07T19:03:48Z

-	if serverID == "" {
-		klog.V(2).Infof("Server is already deleted for machine %q", req.Machine.Name)
-		return &driver.DeleteMachineResponse{}, nil
+func (p *Provider) deleteServers(ctx context.Context, projectID, region, machineName string, serverIDs []string) (bool, error) {


Why plural? Also you return out of function instantly as soon as you get a 404, what about the other serverIDs?

nschad · 2026-09-07T19:04:17Z

-	// Call STACKIT API to delete server
-	err = p.client.DeleteServer(ctx, projectID, providerSpec.Region, serverID)
+func (p *Provider) deleteMachineNICs(ctx context.Context, projectID, region, networkID, machineName string) (bool, error) {
+	nics, err := p.client.ListNICs(ctx, projectID, region, networkID)


Same as above.

nschad · 2026-09-07T19:04:54Z

@aniruddha2000 Please add A LOT of documentation explaining this PR and the individual functions

ske-prow Bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/enhancement Enhancement, improvement, extension do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Jan 22, 2026

ske-prow Bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 22, 2026

dergeberl force-pushed the removeNicsOnDeletion branch 3 times, most recently from d542196 to cf02b5c Compare January 22, 2026 11:31

ske-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jan 22, 2026

ske-prow Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Feb 6, 2026

ske-prow Bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Feb 21, 2026

ske-prow Bot closed this Feb 28, 2026

ske-prow Bot reopened this Mar 2, 2026

ske-prow Bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label Mar 2, 2026

nschad reviewed Mar 2, 2026

View reviewed changes

ske-prow Bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 17, 2026

ske-prow Bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Mar 20, 2026

dergeberl added the lifecycle/frozen Indicates that an issue or PR should not be auto-closed due to staleness. label Mar 20, 2026

aniruddha2000 force-pushed the removeNicsOnDeletion branch from cf02b5c to d47b553 Compare August 27, 2026 09:25

ske-prow Bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 27, 2026

aniruddha2000 force-pushed the removeNicsOnDeletion branch from 55364f4 to 1e553c4 Compare August 27, 2026 11:01

ske-prow Bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 27, 2026

aniruddha2000 changed the title ~~WIP: Cleanup NICs when migrate from openstack MCM~~ Cleanup NICs when migrate from openstack MCM Sep 7, 2026

ske-prow Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2026

nschad requested a review from stackit-ske-bot September 7, 2026 11:43

dergeberl and others added 5 commits September 7, 2026 13:52

WIP

7e0a67c

# Conflicts: # pkg/client/sdk.go # pkg/client/stackit.go # pkg/provider/core.go

fix core.go rebase isssues

6be6d0a

add tests for the create and delete machine

6999aa0

fix make verify

fe038e0

fix make verify

17def4f

aniruddha2000 force-pushed the removeNicsOnDeletion branch from 5b85955 to 17def4f Compare September 7, 2026 12:16

nschad requested changes Sep 7, 2026

View reviewed changes

	return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("failed to fetch server: %v", err))
	return nil, status.Error(codes.AlreadyExists, fmt.Sprintf("multiple servers found: %v", err))

	if !strings.HasPrefix(providerID, StackitProviderName) {
	if !strings.HasPrefix(providerID, StackitProviderName + "://") {

	func (p Provider) serverIDsForMachine(ctx context.Context, req driver.DeleteMachineRequest, secretProjectID, region string, migrated bool) (projectID string, serverIDs []string, err error) {
	func (p Provider) serverIDsForMachine(ctx context.Context, req driver.DeleteMachineRequest, projectIDFromSecret, region string, migrated bool) (projectID string, serverIDs []string, err error) {

Uh oh!

Conversation

dergeberl commented Jan 22, 2026 • edited by aniruddha2000 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ske-prow Bot commented Jan 22, 2026

Uh oh!

stackit-ske commented Feb 6, 2026

Uh oh!

stackit-ske commented Feb 21, 2026

Uh oh!

stackit-ske commented Feb 28, 2026

Uh oh!

ske-prow Bot commented Feb 28, 2026

Uh oh!

dergeberl commented Mar 2, 2026

Uh oh!

ske-prow Bot commented Mar 2, 2026

Uh oh!

dergeberl commented Mar 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stackit-ske commented Mar 17, 2026

Uh oh!

dergeberl commented Mar 20, 2026

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!

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!

nschad commented Sep 7, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dergeberl commented Jan 22, 2026 •

edited by aniruddha2000

Loading