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

🌱 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 2 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
37 changes: 25 additions & 12 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,9 +112,9 @@ 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.
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
}
Expand All @@ -129,9 +126,8 @@ func (m *crdMigrator) run(ctx context.Context, newCRD *apiextensionsv1.CustomRes
// 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), ","))
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 mutationwebhooks may be unavailable and cause a failure.
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
func newCRDMigrationBackoff() wait.Backoff {
// Return a exponential backoff configuration which returns durations for a total time of ~1m30s.
sbueringer marked this conversation as resolved.
Show resolved Hide resolved
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
// 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,
}
}
28 changes: 25 additions & 3 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 @@ -111,6 +111,28 @@ func Test_CRDMigrator(t *testing.T) {
},
wantIsMigrated: false,
},
{
name: "No-op if new CRD adds a new versions and unserves the old",
currentCRD: &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
{Name: "v1alpha1", Storage: true, Served: true},
},
},
Status: apiextensionsv1.CustomResourceDefinitionStatus{StoredVersions: []string{"v1alpha1"}},
},
newCRD: &apiextensionsv1.CustomResourceDefinition{
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
{Name: "v1beta1", Storage: true, Served: true}, // v1beta1 is being added
{Name: "v1alpha1", Served: false}, // v1alpha1 still exists but is not served anymore
},
},
},
wantIsMigrated: false,
},
{
name: "Fails if new CRD drops current storage version",
chrischdi marked this conversation as resolved.
Show resolved Hide resolved
currentCRD: &apiextensionsv1.CustomResourceDefinition{
Expand Down Expand Up @@ -185,7 +207,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
},
},
Expand Down Expand Up @@ -246,8 +268,8 @@ 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: "v1alpha1", Served: false}, // v1alpha1 is no longer being served (required for migration)
{Name: "v1beta1", Served: true}, // v1beta1 still there (required for future migration)
{Name: "v1alpha1", Served: false}, // v1alpha1 is no longer being served
},
},
},
Expand Down
Loading