✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration #11889

sbueringer · 2025-02-21T16:03:05Z

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #11894

sbueringer · 2025-02-24T10:50:21Z

/test pull-cluster-api-e2e-main

sbueringer · 2025-02-24T15:15:56Z

/test pull-cluster-api-e2e-main

JoelSpeed

There are quite a few new files with the license stating 2024 as the year they were introduced, not sure if you wanted to update those, added a nit for one, left the others

JoelSpeed · 2025-02-24T17:19:04Z

bootstrap/kubeadm/main.go

@@ -140,6 +144,9 @@ func InitFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&kubeadmConfigConcurrency, "kubeadmconfig-concurrency", 10,
 		"Number of kubeadm configs to process simultaneously")

+	fs.StringVar(&skipCRDMigrationPhases, "skip-crd-migration-phases", "",


Have you considered making this a string slice and allowing it to be specified with multiple values? Would all us to drop the All and mean that if we add a third phase at some point this won't become awkward

cc @fabriziopandini As you maybe have an additional opinion on this flag :)

I mostly added the All to allow folks to entire disable the CRD migrator without worrying about the phases.

The current flag supports a comma-separated list, so in general All is also not required at the moment.

If it can accept CSV then the string slice part of my comment isn't so important

I can see the argument for All, though, do we want users to be conscious of new phases that we discover and acknowledge that they are skipping them?

I'm not entirely sure why anyone would skip the phases, they are an important part of the lifecycle

I'm not entirely sure why anyone would skip the phases, they are an important part of the lifecycle

I think this could be interesting if:

folks want to use the storage version migrator from upstream k/k instead of our implementation here

have another way to handle migration between apiVersions (also in general thinking about folks that don't run conversion webhooks, even though I don't exactly know how they handle all of this :))

I'm ok if we want make it a slice
I also think it is acceptable to ask users to be explicit in choosing what they want skip and thus dropping All (we also have only two phases, so it will not be a long list)

Sounds good. Made it a slice and dropped All

controllers/crdmigrator/suite_test.go

controllers/crdmigrator/crd_migrator_test.go

chrischdi

not yet done with the review, but first finding

bootstrap/kubeadm/config/rbac/role.yaml

k8s-ci-robot · 2025-02-26T15:51:54Z

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

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

sbueringer · 2025-02-26T15:56:48Z

/test pull-cluster-api-e2e-main

sbueringer · 2025-02-26T19:28:00Z

/test pull-cluster-api-e2e-main

chrischdi

just one nit and one future idea, otherwise lgtm.

controllers/crdmigrator/crd_migrator.go

controlplane/kubeadm/main.go

chrischdi · 2025-02-27T07:59:26Z

Flake:

  [FAILED] Timed out after 120.001s.
  The function passed to Eventually failed at /home/prow/go/src/sigs.k8s.io/cluster-api/test/framework/crdmigration_helpers.go:61 with:
  CustomResourceDefinition clusterclasses.cluster.x-k8s.io should have observed generation annotation with correct value
  Expected
      <map[string]string | len:2>: {
          "controller-gen.kubebuilder.io/version": "v0.17.2",
          "cert-manager.io/inject-ca-from": "capi-system/capi-serving-cert",
      }
  to have {key: value}
      <map[interface {}]interface {} | len:1>: {
          <string>"crd-migration.cluster.x-k8s.io/observed-generation": <string>"3",
      }
  In [It] at: /home/prow/go/src/sigs.k8s.io/cluster-api/test/framework/crdmigration_helpers.go:78 @ 02/26/25 20:32:05.997
  Full Stack Trace
    sigs.k8s.io/cluster-api/test/framework.ValidateCRDMigration({0x3108588, 0xc000565ea0}, {0x3120df0, 0xc000039880}, {0xc003bdd470, 0x12}, {0xc00136df20?, 0xc0016a2480?}, 0x2dfd9e8, 0xc0016042d0)
    	/home/prow/go/src/sigs.k8s.io/cluster-api/test/framework/crdmigration_helpers.go:78 +0x153
    sigs.k8s.io/cluster-api/test/e2e.init.func11.1.1({0x3120df0, 0xc000039880}, {0xc003bdd470, 0x12}, {0xc0014e4930, 0x22})
    	/home/prow/go/src/sigs.k8s.io/cluster-api/test/e2e/clusterctl_upgrade_test.go:103 +0xb7
    sigs.k8s.io/cluster-api/test/e2e.ClusterctlUpgradeSpec.func2()
    	/home/prow/go/src/sigs.k8s.io/cluster-api/test/e2e/clusterctl_upgrade.go:705 +0x463f

sbueringer · 2025-02-27T13:14:58Z

Not sure if it's a flake. Had the same error yesterday, improved the error message slightly. Now going to debug it though :)

sbueringer · 2025-02-27T13:48:11Z

test/e2e/clusterctl_upgrade_test.go

+						framework.ValidateCRDMigration(ctx, proxy, namespace, clusterName,
+							func(crd apiextensionsv1.CustomResourceDefinition) bool {
+								return strings.HasSuffix(crd.Name, ".cluster.x-k8s.io") &&
+									crd.Name != "providers.clusterctl.cluster.x-k8s.io" &&


Not sure what we want to do with providers. At the moment it doesn't matter because it's v1alpha3 and we don't touch it at all

maybe make it a overwritable func?

But also good enough to fixup once we start using it in a provider e2e

This whole thing is already overridable / configurable if a infra provider runs this test :)

What I meant with providers is the providers CRD. Currently I. did not configure any CRDMigrator to migrate the providers CRD (literally the CRD with the name: providers.clusterctl.cluster.x-k8s.io)

sbueringer · 2025-02-27T13:48:26Z

Okay that was an easy fix

									// ClusterTopology feature is disabled via the CLUSTER_TOPOLOGY variable below,
									// so we can't expect the CRD migrator to migrate the ClusterClass CRD.
									crd.Name != "clusterclasses.cluster.x-k8s.io"

sbueringer · 2025-02-27T13:48:37Z

/test pull-cluster-api-e2e-main

fabriziopandini

Awesome work, the integration test alone is a amazing.

docs/book/src/developer/providers/migrations/v1.9-to-v1.10.md

internal/util/ssa/managedfields.go

fabriziopandini · 2025-02-25T16:25:17Z

bootstrap/kubeadm/main.go

@@ -140,6 +144,9 @@ func InitFlags(fs *pflag.FlagSet) {
 	fs.IntVar(&kubeadmConfigConcurrency, "kubeadmconfig-concurrency", 10,
 		"Number of kubeadm configs to process simultaneously")

+	fs.StringVar(&skipCRDMigrationPhases, "skip-crd-migration-phases", "",


I'm ok if we want make it a slice
I also think it is acceptable to ask users to be explicit in choosing what they want skip and thus dropping All (we also have only two phases, so it will not be a long list)

main.go

controllers/crdmigrator/crd_migrator.go

test/e2e/clusterctl_upgrade_test.go

controllers/crdmigrator/crd_migrator.go

sbueringer · 2025-02-27T16:28:35Z

Findings should be all addressed. Thx everyone for the reviews!

I'll run the e2e test locally to see what's going on there

sbueringer · 2025-02-27T16:57:28Z

Should be hopefully green now

/test pull-cluster-api-e2e-main

sbueringer · 2025-02-28T10:20:58Z

@JoelSpeed @fabriziopandini @chrischdi

e2e tests are now also all green. Findings should be addressed, PTAL :)

JoelSpeed

I wonder if we want to try and condense the CRD rbac by setting the resource name multiple times. It appears the RBAC generator doesn't do any sort of condensing of duplicate rules that only differ by resource name entries

bootstrap/kubeadm/main.go

test/framework/crdmigration_helpers.go

sbueringer · 2025-02-28T11:26:18Z

/test pull-cluster-api-e2e-main

sbueringer force-pushed the pr-crd-migration branch 2 times, most recently from 645df3d to 3187ed5 Compare February 24, 2025 09:44

sbueringer changed the title ~~[WIP] ✨ [DNR][POC] Add CRD migrator~~ [WIP] ✨ [DNR][POC] Add CRD migrator, deprecate clusterctl CRD migration Feb 24, 2025

sbueringer force-pushed the pr-crd-migration branch from 3187ed5 to 917e826 Compare February 24, 2025 10:49

sbueringer changed the title ~~[WIP] ✨ [DNR][POC] Add CRD migrator, deprecate clusterctl CRD migration~~ [WIP] ✨ Add CRD migrator, deprecate clusterctl CRD migration Feb 24, 2025

sbueringer added the area/util Issues or PRs related to utils label Feb 24, 2025

k8s-ci-robot removed the do-not-merge/needs-area PR is missing an area label label Feb 24, 2025

sbueringer mentioned this pull request Feb 24, 2025

Removal of old apiVersions in CRDs #11894

Open

3 tasks

sbueringer force-pushed the pr-crd-migration branch from 917e826 to f1be095 Compare February 24, 2025 15:14

Add CRD migrator

919c9a6

sbueringer force-pushed the pr-crd-migration branch from f1be095 to 919c9a6 Compare February 24, 2025 15:15

sbueringer changed the title ~~[WIP] ✨ Add CRD migrator, deprecate clusterctl CRD migration~~ ✨ Add CRD migrator, deprecate clusterctl CRD storage version migration Feb 24, 2025

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 24, 2025

sbueringer changed the title ~~✨ Add CRD migrator, deprecate clusterctl CRD storage version migration~~ ✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration Feb 24, 2025

JoelSpeed reviewed Feb 24, 2025

View reviewed changes

sbueringer added 2 commits February 25, 2025 14:07

Fixes

29ba9e1

Mark test helper correctly

5b83bae

chrischdi reviewed Feb 25, 2025

View reviewed changes

bootstrap/kubeadm/config/rbac/role.yaml Outdated Show resolved Hide resolved

Reduce CRD RBAC

667da65

sbueringer force-pushed the pr-crd-migration branch from b313d99 to d80ed62 Compare February 26, 2025 15:56

Add e2e test coverage

4030bf0

sbueringer force-pushed the pr-crd-migration branch from d80ed62 to 4030bf0 Compare February 26, 2025 19:23

chrischdi reviewed Feb 27, 2025

View reviewed changes

controllers/crdmigrator/crd_migrator.go Show resolved Hide resolved

controlplane/kubeadm/main.go Outdated Show resolved Hide resolved

Fixup

25dd3a2

sbueringer commented Feb 27, 2025

View reviewed changes

fabriziopandini reviewed Feb 27, 2025

View reviewed changes

Fixups

74fd647

sbueringer added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Feb 27, 2025

Fixup e2e test

bb33361

JoelSpeed reviewed Feb 28, 2025

View reviewed changes

bootstrap/kubeadm/main.go Outdated Show resolved Hide resolved

test/framework/crdmigration_helpers.go Outdated Show resolved Hide resolved

Condense RBAC, fix copyright

6ccf183

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration #11889

✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration #11889

sbueringer commented Feb 21, 2025 •

edited

Loading

sbueringer commented Feb 24, 2025

sbueringer commented Feb 24, 2025

JoelSpeed left a comment

JoelSpeed Feb 24, 2025

sbueringer Feb 24, 2025

sbueringer Feb 24, 2025

JoelSpeed Feb 25, 2025

sbueringer Feb 25, 2025 •

edited

Loading

fabriziopandini Feb 25, 2025

sbueringer Feb 27, 2025 •

edited

Loading

chrischdi left a comment

k8s-ci-robot commented Feb 26, 2025

sbueringer commented Feb 26, 2025

sbueringer commented Feb 26, 2025

chrischdi left a comment

chrischdi commented Feb 27, 2025

sbueringer commented Feb 27, 2025

sbueringer Feb 27, 2025

chrischdi Feb 27, 2025

sbueringer Feb 27, 2025

sbueringer commented Feb 27, 2025

sbueringer commented Feb 27, 2025

fabriziopandini left a comment

fabriziopandini Feb 25, 2025

sbueringer commented Feb 27, 2025 •

edited

Loading

sbueringer commented Feb 27, 2025

sbueringer commented Feb 28, 2025

JoelSpeed left a comment

sbueringer commented Feb 28, 2025

✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration #11889

Are you sure you want to change the base?

✨ Add CRD migrator, deprecate clusterctl upgrade CRD storage version migration #11889

Conversation

sbueringer commented Feb 21, 2025 • edited Loading

sbueringer commented Feb 24, 2025

sbueringer commented Feb 24, 2025

JoelSpeed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Feb 25, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer Feb 27, 2025 • edited Loading

Choose a reason for hiding this comment

chrischdi left a comment

Choose a reason for hiding this comment

k8s-ci-robot commented Feb 26, 2025

sbueringer commented Feb 26, 2025

sbueringer commented Feb 26, 2025

chrischdi left a comment

Choose a reason for hiding this comment

chrischdi commented Feb 27, 2025

sbueringer commented Feb 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Feb 27, 2025

sbueringer commented Feb 27, 2025

fabriziopandini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sbueringer commented Feb 27, 2025 • edited Loading

sbueringer commented Feb 27, 2025

sbueringer commented Feb 28, 2025

JoelSpeed left a comment

Choose a reason for hiding this comment

sbueringer commented Feb 28, 2025

sbueringer commented Feb 21, 2025 •

edited

Loading

sbueringer Feb 25, 2025 •

edited

Loading

sbueringer Feb 27, 2025 •

edited

Loading

sbueringer commented Feb 27, 2025 •

edited

Loading