Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 3 commits into from
Apr 30, 2024
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d754ad5
clusterctl: always run crd migration if possible to reduce conversion…
chrischdi Apr 25, 2024
4a7acb0
review fixes
chrischdi Apr 26, 2024
b46a303
review fixes
chrischdi Apr 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 28 additions & 15 deletions cmd/clusterctl/client/cluster/crd_migration.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down Expand Up @@ -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.
Expand All @@ -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 {
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
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.
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
// 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
Expand All @@ -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())
Expand All @@ -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 {
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
return errors.Wrapf(err, "failed to migrate %s/%s", obj.GetNamespace(), obj.GetName())
Expand Down Expand Up @@ -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,
}
}
73 changes: 6 additions & 67 deletions cmd/clusterctl/client/cluster/crd_migration_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand All @@ -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{
Expand All @@ -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
},
},
},
Expand Down Expand Up @@ -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{}{
Expand Down Expand Up @@ -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) {
Expand Down
Loading