Merge pull request #10513 from chrischdi/pr-prevent-unnecessary-crd-migration

🌱 clusterctl: always run crd migration if possible to reduce conversion webhook usage

Author: k8s-ci-robot

cmd/clusterctl/client/cluster/crd_migration.go

@@ -29,6 +29,7 @@ import (
 	"k8s.io/apimachinery/pkg/runtime/schema"
 	"k8s.io/apimachinery/pkg/util/rand"
 	"k8s.io/apimachinery/pkg/util/sets"
+	"k8s.io/apimachinery/pkg/util/wait"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"sigs.k8s.io/cluster-api/cmd/clusterctl/internal/scheme"
@@ -83,12 +84,8 @@ func (m *crdMigrator) run(ctx context.Context, newCRD *apiextensionsv1.CustomRes
 
 	// Gets the list of version supported by the new CRD
 	newVersions := sets.Set[string]{}
-	servedVersions := sets.Set[string]{}
 	for _, version := range newCRD.Spec.Versions {
 		newVersions.Insert(version.Name)
-		if version.Served {
-			servedVersions.Insert(version.Name)
-		}
 	}
 
 	// Get the current CRD.
@@ -115,23 +112,22 @@ func (m *crdMigrator) run(ctx context.Context, newCRD *apiextensionsv1.CustomRes
 	}
 
 	currentStatusStoredVersions := sets.Set[string]{}.Insert(currentCRD.Status.StoredVersions...)
-	// If the new CRD still contains all current stored versions, nothing to do
-	// as no previous storage version will be dropped.
-	if servedVersions.HasAll(currentStatusStoredVersions.UnsortedList()...) {
+	// If the old CRD only contains its current storageVersion as storedVersion,
+	// nothing to do as all objects are already on the current storageVersion.
+	// Note: We want to migrate objects to new storage versions as soon as possible
+	// to prevent unnecessary conversion webhook calls.
+	if currentStatusStoredVersions.Len() == 1 && currentCRD.Status.StoredVersions[0] == currentStorageVersion {
 		log.V(2).Info("CRD migration check passed", "name", newCRD.Name)
 		return false, nil
 	}
 
-	// Otherwise a version that has been used as storage version will be dropped, so it is necessary to migrate all the
-	// objects and drop the storage version from the current CRD status before installing the new CRD.
-	// Ref https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
 	// Note: We are simply migrating all CR objects independent of the version in which they are actually stored in etcd.
 	// This way we can make sure that all CR objects are now stored in the current storage version.
 	// Alternatively, we would have to figure out which objects are stored in which version but this information is not
 	// exposed by the apiserver.
-	storedVersionsToDelete := currentStatusStoredVersions.Difference(servedVersions)
-	storedVersionsToPreserve := currentStatusStoredVersions.Intersection(servedVersions)
-	log.Info("CR migration required", "kind", newCRD.Spec.Names.Kind, "storedVersionsToDelete", strings.Join(sets.List(storedVersionsToDelete), ","), "storedVersionsToPreserve", strings.Join(sets.List(storedVersionsToPreserve), ","))
+	// Ref https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
+	storedVersionsToDelete := currentStatusStoredVersions.Delete(currentStorageVersion)
+	log.Info("CR migration required", "kind", newCRD.Spec.Names.Kind, "storedVersionsToDelete", strings.Join(sets.List(storedVersionsToDelete), ","), "storedVersionToPreserve", currentStorageVersion)
 
 	if err := m.migrateResourcesForCRD(ctx, currentCRD, currentStorageVersion); err != nil {
 		return false, err
@@ -157,7 +153,7 @@ func (m *crdMigrator) migrateResourcesForCRD(ctx context.Context, crd *apiextens
 
 	var i int
 	for {
-		if err := retryWithExponentialBackoff(ctx, newReadBackoff(), func(ctx context.Context) error {
+		if err := retryWithExponentialBackoff(ctx, newCRDMigrationBackoff(), func(ctx context.Context) error {
 			return m.Client.List(ctx, list, client.Continue(list.GetContinue()))
 		}); err != nil {
 			return errors.Wrapf(err, "failed to list %q", list.GetKind())
@@ -167,7 +163,7 @@ func (m *crdMigrator) migrateResourcesForCRD(ctx context.Context, crd *apiextens
 			obj := list.Items[i]
 
 			log.V(5).Info("Migrating", logf.UnstructuredToValues(obj)...)
-			if err := retryWithExponentialBackoff(ctx, newWriteBackoff(), func(ctx context.Context) error {
+			if err := retryWithExponentialBackoff(ctx, newCRDMigrationBackoff(), func(ctx context.Context) error {
 				return handleMigrateErr(m.Client.Update(ctx, &obj))
 			}); err != nil {
 				return errors.Wrapf(err, "failed to migrate %s/%s", obj.GetNamespace(), obj.GetName())
@@ -230,3 +226,20 @@ func storageVersionForCRD(crd *apiextensionsv1.CustomResourceDefinition) (string
 	}
 	return "", errors.Errorf("could not find storage version for CRD %q", crd.Name)
 }
+
+// newCRDMigrationBackoff creates a new API Machinery backoff parameter set suitable for use with crd migration operations.
+// Clusterctl upgrades cert-manager right before doing CRD migration. This may lead to rollout of new certificates.
+// The time between new certificate creation + injection into objects (CRD, Webhooks) and the new secrets getting propagated
+// to the controller can be 60-90s, because the kubelet only periodically syncs secret contents to pods.
+// During this timespan conversion, validating- or mutating-webhooks may be unavailable and cause a failure.
+func newCRDMigrationBackoff() wait.Backoff {
+	// Return a exponential backoff configuration which returns durations for a total time of ~1m30s + some buffer.
+	// Example: 0, .25s, .6s, 1.1s, 1.8s, 2.7s, 4s, 6s, 9s, 12s, 17s, 25s, 35s, 49s, 69s, 97s, 135s
+	// Jitter is added as a random fraction of the duration multiplied by the jitter factor.
+	return wait.Backoff{
+		Duration: 250 * time.Millisecond,
+		Factor:   1.4,
+		Steps:    17,
+		Jitter:   0.1,
+	}
+}

cmd/clusterctl/client/cluster/crd_migration_test.go

@@ -69,7 +69,7 @@ func Test_CRDMigrator(t *testing.T) {
 			wantIsMigrated: false,
 		},
 		{
-			name: "No-op if new CRD supports same versions",
+			name: "No-op if new CRD uses the same storage version",
 			currentCRD: &apiextensionsv1.CustomResourceDefinition{
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
@@ -90,7 +90,7 @@ func Test_CRDMigrator(t *testing.T) {
 			wantIsMigrated: false,
 		},
 		{
-			name: "No-op if new CRD adds a new versions",
+			name: "No-op if new CRD adds a new versions and stored versions is only the old storage version",
 			currentCRD: &apiextensionsv1.CustomResourceDefinition{
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
@@ -104,8 +104,8 @@ func Test_CRDMigrator(t *testing.T) {
 				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
 				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
 					Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
-						{Name: "v1beta1", Storage: true, Served: true}, // v1beta1 is being added
-						{Name: "v1alpha1", Served: true},               // v1alpha1 still exists
+						{Name: "v1beta1", Storage: true, Served: false}, // v1beta1 is being added
+						{Name: "v1alpha1", Served: true},                // v1alpha1 still exists
 					},
 				},
 			},
@@ -133,7 +133,7 @@ func Test_CRDMigrator(t *testing.T) {
 			wantErr: true,
 		},
 		{
-			name: "Migrate CRs if their storage version is removed from the CRD",
+			name: "Migrate CRs if there are stored versions is not only the current storage version",
 			CRs: []unstructured.Unstructured{
 				{
 					Object: map[string]interface{}{
@@ -185,75 +185,14 @@ func Test_CRDMigrator(t *testing.T) {
 					Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Foo", ListKind: "FooList"},
 					Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
 						{Name: "v1", Storage: true, Served: true}, // v1 is being added
-						{Name: "v1beta1", Served: true},           // v1beta1 still there (required for migration)
+						{Name: "v1beta1", Served: true},           // v1beta1 still there
 						// v1alpha1 is being dropped
 					},
 				},
 			},
 			wantStoredVersions: []string{"v1beta1"}, // v1alpha1 should be dropped from current CRD's stored versions
 			wantIsMigrated:     true,
 		},
-		{
-			name: "Migrate the CR if their storage version is no longer served by the CRD",
-			CRs: []unstructured.Unstructured{
-				{
-					Object: map[string]interface{}{
-						"apiVersion": "foo/v1beta1",
-						"kind":       "Foo",
-						"metadata": map[string]interface{}{
-							"name":      "cr1",
-							"namespace": metav1.NamespaceDefault,
-						},
-					},
-				},
-				{
-					Object: map[string]interface{}{
-						"apiVersion": "foo/v1beta1",
-						"kind":       "Foo",
-						"metadata": map[string]interface{}{
-							"name":      "cr2",
-							"namespace": metav1.NamespaceDefault,
-						},
-					},
-				},
-				{
-					Object: map[string]interface{}{
-						"apiVersion": "foo/v1beta1",
-						"kind":       "Foo",
-						"metadata": map[string]interface{}{
-							"name":      "cr3",
-							"namespace": metav1.NamespaceDefault,
-						},
-					},
-				},
-			},
-			currentCRD: &apiextensionsv1.CustomResourceDefinition{
-				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
-				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
-					Group: "foo",
-					Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Foo", ListKind: "FooList"},
-					Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
-						{Name: "v1beta1", Storage: true, Served: true},
-						{Name: "v1alpha1", Served: true},
-					},
-				},
-				Status: apiextensionsv1.CustomResourceDefinitionStatus{StoredVersions: []string{"v1beta1", "v1alpha1"}},
-			},
-			newCRD: &apiextensionsv1.CustomResourceDefinition{
-				ObjectMeta: metav1.ObjectMeta{Name: "foo"},
-				Spec: apiextensionsv1.CustomResourceDefinitionSpec{
-					Group: "foo",
-					Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Foo", ListKind: "FooList"},
-					Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
-						{Name: "v1", Storage: true, Served: true}, // v1 is being added
-						{Name: "v1beta1", Served: true},           // v1beta1 still there (required for migration)
-						{Name: "v1alpha1", Served: false},         // v1alpha1 is no longer being served (required for migration)
-					},
-				},
-			},
-			wantStoredVersions: []string{"v1beta1"}, // v1alpha1 should be dropped from current CRD's stored versions
-			wantIsMigrated:     true,
-		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {