migrate mcm-provider from openstack to stackit by aniruddha2000 · Pull Request #272 · stackitcloud/gardener-extension-provider-stackit

aniruddha2000 · 2026-08-26T09:25:34Z

How to categorize this PR?

/kind enhancement

What this PR does / why we need it:

This PR replaces mcm provider from openstack to stackit. Replaces openstack provider ID with stackit provider ID and add stackit.cloud/migrated-machine: true annotation to machines and puts annotation stackit.cloud/machine-controller-manager-migrated: true to workers once the migration is done.

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Breaking changes:

ske-prow · 2026-08-26T09:25: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 aniruddha2000 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

aniruddha2000 · 2026-08-31T13:22:09Z

/retest-required

aniruddha2000 · 2026-08-31T13:22:25Z

/retest

aniruddha2000 · 2026-08-31T13:23:04Z

/retest

aniruddha2000 · 2026-08-31T13:25:25Z

/retest

aniruddha2000 · 2026-08-31T20:49:16Z

/retest

aniruddha2000 · 2026-08-31T20:50:00Z

/test pull-gardener-extension-provider-stackit-dev-artifacts

aniruddha2000 · 2026-08-31T20:50:41Z

/test all

aniruddha2000 · 2026-09-07T15:04:55Z

/test pull-gardener-extension-provider-stackit-integration-tests

# Conflicts: # pkg/controller/worker/machines.go

stackit-ske-bot

SKE Code Review

Architectural Feedback

Inverted Migration Transaction Flow & Redundant Patch Overhead: In pkg/controller/worker/machines.go#L456-L499, each machine undergoes three separate Kubernetes API Patch calls (shouldMigrateMachineAnnotation + migratedMachineAnnotation, then Spec.ProviderID, then removing shouldMigrateMachineAnnotation). This transaction complexity and ephemeral annotation were introduced because Spec.ProviderID is updated in Kubernetes before confirming that the STACKIT IaaS UpdateServer call succeeded. Reversing the sequence—executing UpdateServer (an idempotent operation) on STACKIT IaaS first, and then executing a single atomic Kubernetes Patch updating both Spec.ProviderID and migratedMachineAnnotation—eliminates the ephemeral shouldMigrateMachineAnnotation, reduces Kubernetes API patch traffic by 66% during node migrations, and avoids leaving machine resources in a state where Spec.ProviderID points to STACKIT while the remote server labels were never updated.
Unconditional IaaS Client Initialization in WorkerDelegate: In pkg/controller/worker/actuator.go#L76-L80, stackitClient.IaaS(ctx, d.seedClient, worker.Spec.SecretRef) is called unconditionally on every Worker reconciliation. Because w.iaaSClient is exclusively used by migrateMachines (which only executes when feature.MigrateStackitMachineControllerManager(cluster) is enabled and the worker has not yet migrated), Shoot clusters that do not use MCM migration or OpenStack clusters whose secret lacks STACKIT service account credentials (saKeyJSON) will fail secret credential parsing in stackit.GetCredentialsFromSecretRef, breaking Worker reconciliation. The IaaS client should only be instantiated when feature.MigrateStackitMachineControllerManager(cluster) && worker.Annotations[workerMigratedAnnotation] != "true", or lazily on demand inside migrateMachines.

Findings & Feedback

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

Verdict

Comment

breuerfelix · 2026-09-17T08:37:10Z

-			if err != nil {
-				return fmt.Errorf("migrateMachines: %w", err)
-			}
+		if m.Spec.ProviderID == "" {


we cannot just error out here.
we have to answer the following questions:
can the provider id even be empty? if yes, in which cases? what is the state of the server if the id is empty?
what happens when we return an error here if the id is empty, this will result in a "failed migration" right? is this the expected behaviour?

@aniruddha2000 why have you resolved this comment?
as discussed, we should no error out here, just remove these 3 lines.
if the providerID is empty, the machine will get cleaned up by the mcm anyways after some time

:) whats wrong with my git, i deleted it and pushed a commit bellow this comment, but somehow it did not! sorrry! :(

commit also says "revert provider id is empty review"

breuerfelix · 2026-09-18T09:40:31Z

+		err = w.migrateMachines(ctx)
+		if err != nil {
+			return err
+		}


Suggested change

err = w.migrateMachines(ctx)

if err != nil {

return err

}

if err := w.migrateMachines(ctx); err != nil {

return err

}

breuerfelix · 2026-09-18T10:12:16Z

-			if err != nil {
-				return fmt.Errorf("migrateMachines: %w", err)
-			}
+		if m.Spec.ProviderID == "" {


@aniruddha2000 why have you resolved this comment?
as discussed, we should no error out here, just remove these 3 lines.
if the providerID is empty, the machine will get cleaned up by the mcm anyways after some time

breuerfelix · 2026-09-18T10:13:12Z

+			return fmt.Errorf("cannot migrate machine %s: providerID is empty (provisioning in progress)", m.Name)
+		}
+
+		if m.Spec.ProviderID != "" {


add a comment above on why empty providerID is also fine (this is stated in the ADR), so the next person who reads that code knows that we intentionally skip them

breuerfelix · 2026-09-18T10:13:38Z

+			m.Annotations = make(map[string]string)
+		}
+		m.Annotations[shouldMigrateMachineAnnotation] = "true"
+		m.Annotations[migratedMachineAnnotation] = "true"


add a comment what this annotation does, and that we need it for cleaning up the nics (stated in the ADR)

breuerfelix · 2026-09-18T10:20:02Z

+					// TODO refine labels
+					"mcm.gardener.cloud/machine":      m.Name,
+					"mcm.gardener.cloud/machineclass": m.Spec.Class.Name,
+					"mcm.gardener.cloud/role":         "node",


Suggested change

// TODO refine labels

"mcm.gardener.cloud/machine": m.Name,

"mcm.gardener.cloud/machineclass": m.Spec.Class.Name,

"mcm.gardener.cloud/role": "node",

"kubernetes.io/machine": m.Name,

"kubernetes.io/machineclass": m.Spec.Class.Name,

"kubernetes.io/role": "node",

AFAIK these are the labels that the stackit-mcm are using.
double check that, when using the stackit mcm, creating a new machine, what labels are present on this machine?

ske-prow Bot added the kind/enhancement Enhancement, improvement, extension label Aug 26, 2026

ske-prow Bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 26, 2026

aniruddha2000 force-pushed the migrate-mcm branch from 67e72b1 to dcf82cb Compare August 26, 2026 09:26

ske-prow Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 28, 2026

aniruddha2000 force-pushed the migrate-mcm branch from b696375 to 25fa93d Compare August 31, 2026 13:14

ske-prow Bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 31, 2026

aniruddha2000 force-pushed the migrate-mcm branch from 81f51f4 to af6bcea Compare September 7, 2026 11:52

nschad reviewed Sep 9, 2026

View reviewed changes

nschad requested changes Sep 10, 2026

View reviewed changes

Comment thread pkg/controller/worker/machines.go Outdated

Comment thread pkg/controller/worker/machines.go Outdated

Comment thread pkg/controller/worker/machines.go Outdated

Comment thread pkg/controller/worker/machines.go Outdated

Comment thread pkg/controller/worker/machines.go Outdated

aniruddha2000 added 12 commits September 10, 2026 15:47

migrate mcm provider from openstack to stackit

8b86252

# Conflicts: # pkg/controller/worker/machines.go

add feature gates

01a1b39

add worker migrated annotation

1837d0f

add small check to not trigger migration when done

c47646a

minor nits

504056b

add diagnosis

462effd

trigger build

952006b

run ci

6a753b1

add nil stackit client to the tests

a67922d

remove diagnosis

8f4dd16

add poc test for the machine migration

5241e32

fix mocks in the tests

b32b095

aniruddha2000 added 9 commits September 10, 2026 15:47

add improvements to the test

a73bc4b

fix the test and its running

385baa8

add iaas failure tolerance testing

51b27e6

make fmt

320d8bf

make check fix<

ec219a7

make check fix

0733734

fix test

95619d8

fix test

2b1a268

resolve review

a712745

aniruddha2000 force-pushed the migrate-mcm branch from b636923 to a712745 Compare September 10, 2026 13:47

aniruddha2000 added 2 commits September 10, 2026 15:48

fix test

c44e976

use named capture groups in the regex

f66ecdc

nschad reviewed Sep 11, 2026

View reviewed changes

Comment thread pkg/controller/worker/machines.go

move regex compile to init func

84dd68b

aniruddha2000 requested a review from stackit-ske-bot September 16, 2026 10:47

stackit-ske-bot reviewed Sep 16, 2026

View reviewed changes

Comment thread pkg/controller/worker/machines.go Outdated

Comment thread pkg/controller/worker/machines.go

Comment thread pkg/controller/worker/machines.go

Comment thread pkg/controller/worker/machines.go Outdated

resolve AI suggestions

49091b3

breuerfelix reviewed Sep 17, 2026

View reviewed changes

Comment thread pkg/controller/worker/machines.go

resolve AI suggestions

1624513

breuerfelix reviewed Sep 17, 2026

View reviewed changes

revert provider id is empty review

84af27b

breuerfelix reviewed Sep 18, 2026

View reviewed changes

aniruddha2000 added 2 commits September 18, 2026 15:09

update labels and make nits

88d9ba2

fix test

183cd32

Conversation

aniruddha2000 commented Aug 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ske-prow Bot commented Aug 26, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Aug 31, 2026

Uh oh!

aniruddha2000 commented Sep 7, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stackit-ske-bot left a comment

Choose a reason for hiding this comment

SKE Code Review

Architectural Feedback

Findings & Feedback

Verdict

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aniruddha2000 Sep 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aniruddha2000 commented Aug 26, 2026 •

edited

Loading

aniruddha2000 Sep 18, 2026 •

edited

Loading