diff --git a/cmd/clusterctl/client/cluster/crd_migration.go b/cmd/clusterctl/client/cluster/crd_migration.go index 17870b0017b9..98098f911461 100644 --- a/cmd/clusterctl/client/cluster/crd_migration.go +++ b/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, + } +} diff --git a/cmd/clusterctl/client/cluster/crd_migration_test.go b/cmd/clusterctl/client/cluster/crd_migration_test.go index 950e5150b875..95efc6c31488 100644 --- a/cmd/clusterctl/client/cluster/crd_migration_test.go +++ b/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,7 +185,7 @@ 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 }, }, @@ -193,67 +193,6 @@ func Test_CRDMigrator(t *testing.T) { 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) {