Skip to content

Commit b46a303

Browse files
chrischdichrischdi
committed
review fixes
1 parent 4a7acb0 commit b46a303

File tree

2 files changed

+10
-93
lines changed

2 files changed

+10
-93
lines changed

‎cmd/clusterctl/client/cluster/crd_migration.go

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -114,18 +114,18 @@ func (m *crdMigrator) run(ctx context.Context, newCRD *apiextensionsv1.CustomRes
114114
currentStatusStoredVersions := sets.Set[string]{}.Insert(currentCRD.Status.StoredVersions...)
115115
// If the old CRD only contains its current storageVersion as storedVersion,
116116
// nothing to do as all objects are already on the current storageVersion.
117+
// Note: We want to migrate objects to new storage versions as soon as possible
118+
// to prevent unnecessary conversion webhook calls.
117119
if currentStatusStoredVersions.Len() == 1 && currentCRD.Status.StoredVersions[0] == currentStorageVersion {
118120
log.V(2).Info("CRD migration check passed", "name", newCRD.Name)
119121
return false, nil
120122
}
121123

122-
// Otherwise a version that has been used as storage version will be dropped, so it is necessary to migrate all the
123-
// objects and drop the storage version from the current CRD status before installing the new CRD.
124-
// Ref https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
125124
// Note: We are simply migrating all CR objects independent of the version in which they are actually stored in etcd.
126125
// This way we can make sure that all CR objects are now stored in the current storage version.
127126
// Alternatively, we would have to figure out which objects are stored in which version but this information is not
128127
// exposed by the apiserver.
128+
// Ref https://kubernetes.io/docs/tasks/extend-kubernetes/custom-resources/custom-resource-definition-versioning/#writing-reading-and-updating-versioned-customresourcedefinition-objects
129129
storedVersionsToDelete := currentStatusStoredVersions.Delete(currentStorageVersion)
130130
log.Info("CR migration required", "kind", newCRD.Spec.Names.Kind, "storedVersionsToDelete", strings.Join(sets.List(storedVersionsToDelete), ","), "storedVersionToPreserve", currentStorageVersion)
131131

@@ -231,9 +231,9 @@ func storageVersionForCRD(crd *apiextensionsv1.CustomResourceDefinition) (string
231231
// Clusterctl upgrades cert-manager right before doing CRD migration. This may lead to rollout of new certificates.
232232
// The time between new certificate creation + injection into objects (CRD, Webhooks) and the new secrets getting propagated
233233
// to the controller can be 60-90s, because the kubelet only periodically syncs secret contents to pods.
234-
// During this timespan conversion, validating- or mutationwebhooks may be unavailable and cause a failure.
234+
// During this timespan conversion, validating- or mutating-webhooks may be unavailable and cause a failure.
235235
func newCRDMigrationBackoff() wait.Backoff {
236-
// Return a exponential backoff configuration which returns durations for a total time of ~1m30s.
236+
// Return a exponential backoff configuration which returns durations for a total time of ~1m30s + some buffer.
237237
// Example: 0, .25s, .6s, 1.1s, 1.8s, 2.7s, 4s, 6s, 9s, 12s, 17s, 25s, 35s, 49s, 69s, 97s, 135s
238238
// Jitter is added as a random fraction of the duration multiplied by the jitter factor.
239239
return wait.Backoff{

‎cmd/clusterctl/client/cluster/crd_migration_test.go

Lines changed: 5 additions & 88 deletions
Original file line numberDiff line numberDiff line change
@@ -69,7 +69,7 @@ func Test_CRDMigrator(t *testing.T) {
6969
wantIsMigrated: false,
7070
},
7171
{
72-
name: "No-op if new CRD supports same versions",
72+
name: "No-op if new CRD uses the same storage version",
7373
currentCRD: &apiextensionsv1.CustomResourceDefinition{
7474
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
7575
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
@@ -90,7 +90,7 @@ func Test_CRDMigrator(t *testing.T) {
9090
wantIsMigrated: false,
9191
},
9292
{
93-
name: "No-op if new CRD adds a new versions",
93+
name: "No-op if new CRD adds a new versions and stored versions is only the old storage version",
9494
currentCRD: &apiextensionsv1.CustomResourceDefinition{
9595
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
9696
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
@@ -104,30 +104,8 @@ func Test_CRDMigrator(t *testing.T) {
104104
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
105105
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
106106
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
107-
{Name: "v1beta1", Storage: true, Served: true}, // v1beta1 is being added
108-
{Name: "v1alpha1", Served: true}, // v1alpha1 still exists
109-
},
110-
},
111-
},
112-
wantIsMigrated: false,
113-
},
114-
{
115-
name: "No-op if new CRD adds a new versions and unserves the old",
116-
currentCRD: &apiextensionsv1.CustomResourceDefinition{
117-
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
118-
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
119-
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
120-
{Name: "v1alpha1", Storage: true, Served: true},
121-
},
122-
},
123-
Status: apiextensionsv1.CustomResourceDefinitionStatus{StoredVersions: []string{"v1alpha1"}},
124-
},
125-
newCRD: &apiextensionsv1.CustomResourceDefinition{
126-
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
127-
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
128-
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
129-
{Name: "v1beta1", Storage: true, Served: true}, // v1beta1 is being added
130-
{Name: "v1alpha1", Served: false}, // v1alpha1 still exists but is not served anymore
107+
{Name: "v1beta1", Storage: true, Served: false}, // v1beta1 is being added
108+
{Name: "v1alpha1", Served: true}, // v1alpha1 still exists
131109
},
132110
},
133111
},
@@ -155,7 +133,7 @@ func Test_CRDMigrator(t *testing.T) {
155133
wantErr: true,
156134
},
157135
{
158-
name: "Migrate CRs if their storage version is removed from the CRD",
136+
name: "Migrate CRs if there are stored versions is not only the current storage version",
159137
CRs: []unstructured.Unstructured{
160138
{
161139
Object: map[string]interface{}{
@@ -215,67 +193,6 @@ func Test_CRDMigrator(t *testing.T) {
215193
wantStoredVersions: []string{"v1beta1"}, // v1alpha1 should be dropped from current CRD's stored versions
216194
wantIsMigrated: true,
217195
},
218-
{
219-
name: "Migrate the CR if their storage version is no longer served by the CRD",
220-
CRs: []unstructured.Unstructured{
221-
{
222-
Object: map[string]interface{}{
223-
"apiVersion": "foo/v1beta1",
224-
"kind": "Foo",
225-
"metadata": map[string]interface{}{
226-
"name": "cr1",
227-
"namespace": metav1.NamespaceDefault,
228-
},
229-
},
230-
},
231-
{
232-
Object: map[string]interface{}{
233-
"apiVersion": "foo/v1beta1",
234-
"kind": "Foo",
235-
"metadata": map[string]interface{}{
236-
"name": "cr2",
237-
"namespace": metav1.NamespaceDefault,
238-
},
239-
},
240-
},
241-
{
242-
Object: map[string]interface{}{
243-
"apiVersion": "foo/v1beta1",
244-
"kind": "Foo",
245-
"metadata": map[string]interface{}{
246-
"name": "cr3",
247-
"namespace": metav1.NamespaceDefault,
248-
},
249-
},
250-
},
251-
},
252-
currentCRD: &apiextensionsv1.CustomResourceDefinition{
253-
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
254-
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
255-
Group: "foo",
256-
Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Foo", ListKind: "FooList"},
257-
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
258-
{Name: "v1beta1", Storage: true, Served: true},
259-
{Name: "v1alpha1", Served: true},
260-
},
261-
},
262-
Status: apiextensionsv1.CustomResourceDefinitionStatus{StoredVersions: []string{"v1beta1", "v1alpha1"}},
263-
},
264-
newCRD: &apiextensionsv1.CustomResourceDefinition{
265-
ObjectMeta: metav1.ObjectMeta{Name: "foo"},
266-
Spec: apiextensionsv1.CustomResourceDefinitionSpec{
267-
Group: "foo",
268-
Names: apiextensionsv1.CustomResourceDefinitionNames{Kind: "Foo", ListKind: "FooList"},
269-
Versions: []apiextensionsv1.CustomResourceDefinitionVersion{
270-
{Name: "v1", Storage: true, Served: true}, // v1 is being added
271-
{Name: "v1beta1", Served: true}, // v1beta1 still there (required for future migration)
272-
{Name: "v1alpha1", Served: false}, // v1alpha1 is no longer being served
273-
},
274-
},
275-
},
276-
wantStoredVersions: []string{"v1beta1"}, // v1alpha1 should be dropped from current CRD's stored versions
277-
wantIsMigrated: true,
278-
},
279196
}
280197
for _, tt := range tests {
281198
t.Run(tt.name, func(t *testing.T) {

0 commit comments

Comments
 (0)