Cleanup NICs when migrate from openstack MCM

[dergeberl]

How to categorize this PR?

/kind enhancement

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

This PR involves migration of machines from OpenStack to STACKIT provider. There are mainly two controller involved in the change:

Creation
The newly created machine is created with STACKIT provider ID (stackit://project-id/server-id), and for the migrated machines with stackit.cloud/migrated-machine: true annotaion the creation will not work, to be sure not to mess with half created machines.

Delete
While deleting machine it needs to list and delete all ports if a machine with the stackit.cloud/migrated-machine annotation gets deleted.

NOTE: There is a fallback to get the server by name in case there is no providerID during deletion. Normally this is done with a label containing the machine name and a label selector. In case of a migrated machine with the stackit.cloud/migrated-machine annotation the deletion needs to get all servers and filters internally.

Special notes for your reviewer:

Breaking changes:

[ske-prow]

[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]

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]

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]

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]

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

/reopen

[ske-prow]

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

/remove-lifecycle rotten

[stackit-ske]

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]

/remove-lifecycle stale

[nschad]

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

[stackit-ske-bot]

SKE Code Review

Architectural Feedback

• Server Deletion Synchronization & NIC Attachment Race:
  In pkg/provider/delete.go#L170-L183, WaitUntilServerDeleted is retained but is no longer invoked anywhere in the codebase (previously called after DeleteServer). As a consequence, p.client.DeleteServer initiates asynchronous VM termination, but deleteMachineNICs is called immediately afterwards. In STACKIT IaaS, deleting a NIC while its server is still shutting down / terminating will fail because the network interface remains bound to the active instance. Re-evaluating whether WaitUntilServerDeleted should be awaited prior to NIC cleanup prevents race conditions with STACKIT infrastructure during migrated machine teardown.
• Simplification of Deletion Control Flow & Dead Code Elimination:
  In pkg/provider/delete.go#L53-L75, both deleteServers and deleteMachineNICs return (bool, error). However, both functions return (true, nil) whenever allErrors == nil. Because the boolean is always true on success, the conditional returns if serverAlreadyDeleted && !migrated and if nicAlreadyDeleted ensure that lines 72–74 (pkg/provider/delete.go#L72-L74) are completely unreachable. Refactoring deleteServers and deleteMachineNICs to return solely error simplifies the control flow, eliminates misleading variable naming (*AlreadyDeleted vs successful deletion), and allows completion logging to execute as intended.
• Defensive Guard on providerSpec.Networking:
  In pkg/provider/delete.go#L64, providerSpec.Networking.NetworkID is dereferenced directly. In DeleteMachine, req.Secret is not validated through ValidateProviderSpecNSecret. If providerSpec.Networking is nil or specifies NICIDs instead of NetworkID, this causes a nil pointer dereference or an invalid STACKIT API call with an empty path parameter. Guarding this invocation with if migrated && providerSpec.Networking != nil && providerSpec.Networking.NetworkID != "" ensures safe teardown.

Findings & Feedback

• All findings and concrete recommendations have been provided as inline code suggestions above.

Verdict

Comments with suggestions and architectural feedback.

[stackit-ske-bot]

SKE Code Review

Findings & Feedback

All findings and concrete recommendations have been provided as inline code suggestions above.

Verdict

Comments provided with inline suggestions for nil-pointer protection, dead code cleanup, error status code wrapping, and robust network ID handling.