Skip to content

Commit eec306b

Browse files
sbueringersbueringer
committed
Fix review findings
1 parent 79c8a90 commit eec306b

File tree

4 files changed

+57
-7
lines changed

4 files changed

+57
-7
lines changed

‎internal/controllers/machineset/machineset_controller.go‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1558,9 +1558,9 @@ func (r *Reconciler) createBootstrapConfig(ctx context.Context, ms *clusterv1.Ma
15581558
return nil, clusterv1.ContractVersionedObjectReference{}, errors.Wrapf(err, "failed to create BootstrapConfig")
15591559
}
15601560

1561-
// Create the full object with capi-kubeadmcontrolplane.
1561+
// Create the full object with capi-machineset.
15621562
// Below ssa.RemoveManagedFieldsForLabelsAndAnnotations will drop ownership for labels and annotations
1563-
// so that in a subsequent syncMachines call capi-kubeadmcontrolplane-metadata can take ownership for them.
1563+
// so that in a subsequent syncMachines call capi-machineset-metadata can take ownership for them.
15641564
// Note: This is done in way that it does not rely on managedFields being stored in the cache, so we can optimize
15651565
// memory usage by dropping managedFields before storing objects in the cache.
15661566
if err := ssa.Patch(ctx, r.Client, machineSetManagerName, bootstrapConfig); err != nil {
@@ -1628,9 +1628,9 @@ func (r *Reconciler) createInfraMachine(ctx context.Context, ms *clusterv1.Machi
16281628
return nil, clusterv1.ContractVersionedObjectReference{}, errors.Wrapf(err, "failed to create InfraMachine")
16291629
}
16301630

1631-
// Create the full object with capi-kubeadmcontrolplane.
1631+
// Create the full object with capi-machineset.
16321632
// Below ssa.RemoveManagedFieldsForLabelsAndAnnotations will drop ownership for labels and annotations
1633-
// so that in a subsequent syncMachines call capi-kubeadmcontrolplane-metadata can take ownership for them.
1633+
// so that in a subsequent syncMachines call capi-machineset-metadata can take ownership for them.
16341634
// Note: This is done in way that it does not rely on managedFields being stored in the cache, so we can optimize
16351635
// memory usage by dropping managedFields before storing objects in the cache.
16361636
if err := ssa.Patch(ctx, r.Client, machineSetManagerName, infraMachine); err != nil {

‎internal/controllers/machineset/machineset_controller_test.go‎

Lines changed: 14 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -121,7 +121,8 @@ func TestMachineSetReconciler(t *testing.T) {
121121
machineTemplateSpec := clusterv1.MachineTemplateSpec{
122122
ObjectMeta: clusterv1.ObjectMeta{
123123
Labels: map[string]string{
124-
"label-1": "true",
124+
"label-1": "true",
125+
"precedence": "MachineSet",
125126
},
126127
Annotations: map[string]string{
127128
"annotation-1": "true",
@@ -208,6 +209,9 @@ func TestMachineSetReconciler(t *testing.T) {
208209
"annotations": map[string]interface{}{
209210
"precedence": "GenericBootstrapConfig",
210211
},
212+
"labels": map[string]interface{}{
213+
"precedence": "GenericBootstrapConfig",
214+
},
211215
},
212216
"spec": map[string]interface{}{
213217
"format": "cloud-init",
@@ -234,6 +238,9 @@ func TestMachineSetReconciler(t *testing.T) {
234238
"annotations": map[string]interface{}{
235239
"precedence": "GenericInfrastructureMachineTemplate",
236240
},
241+
"labels": map[string]interface{}{
242+
"precedence": "GenericInfrastructureMachineTemplate",
243+
},
237244
},
238245
"spec": map[string]interface{}{
239246
"size": "3xlarge",
@@ -314,6 +321,7 @@ func TestMachineSetReconciler(t *testing.T) {
314321
g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("annotation-1", "true"), "have annotations of MachineTemplate applied")
315322
g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("precedence", "MachineSet"), "the annotations from the MachineSpec template to overwrite the infrastructure template ones")
316323
g.Expect(im.GetLabels()).To(HaveKeyWithValue("label-1", "true"), "have labels of MachineTemplate applied")
324+
g.Expect(im.GetLabels()).To(HaveKeyWithValue("precedence", "MachineSet"), "the labels from the MachineSpec template to overwrite the infrastructure template ones")
317325
g.Expect(cleanupTime(im.GetManagedFields())).To(ConsistOf(toManagedFields([]managedFieldEntry{{
318326
// capi-machineset-metadata owns labels and annotations.
319327
APIVersion: im.GetAPIVersion(),
@@ -329,7 +337,8 @@ func TestMachineSetReconciler(t *testing.T) {
329337
"f:cluster.x-k8s.io/cluster-name":{},
330338
"f:cluster.x-k8s.io/deployment-name":{},
331339
"f:cluster.x-k8s.io/set-name":{},
332-
"f:label-1":{}
340+
"f:label-1":{},
341+
"f:precedence":{}
333342
}
334343
}}`,
335344
}, {
@@ -385,6 +394,7 @@ func TestMachineSetReconciler(t *testing.T) {
385394
g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("annotation-1", "true"), "have annotations of MachineTemplate applied")
386395
g.Expect(im.GetAnnotations()).To(HaveKeyWithValue("precedence", "MachineSet"), "the annotations from the MachineSpec template to overwrite the bootstrap config template ones")
387396
g.Expect(im.GetLabels()).To(HaveKeyWithValue("label-1", "true"), "have labels of MachineTemplate applied")
397+
g.Expect(im.GetLabels()).To(HaveKeyWithValue("precedence", "MachineSet"), "the labels from the MachineSpec template to overwrite the bootstrap config template ones")
388398
g.Expect(cleanupTime(im.GetManagedFields())).To(ConsistOf(toManagedFields([]managedFieldEntry{{
389399
// capi-machineset-metadata owns labels and annotations.
390400
APIVersion: im.GetAPIVersion(),
@@ -400,7 +410,8 @@ func TestMachineSetReconciler(t *testing.T) {
400410
"f:cluster.x-k8s.io/cluster-name":{},
401411
"f:cluster.x-k8s.io/deployment-name":{},
402412
"f:cluster.x-k8s.io/set-name":{},
403-
"f:label-1":{}
413+
"f:label-1":{},
414+
"f:precedence":{}
404415
}
405416
}}`,
406417
}, {

‎internal/util/ssa/managedfields.go‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ limitations under the License.
1818
package ssa
1919

2020
import (
21+
"bytes"
2122
"context"
2223
"encoding/json"
2324
"time"
@@ -43,6 +44,8 @@ const classicManager = "manager"
4344
// * First a full object is created with fieldManager.
4445
// * Then this func is called to drop ownership for labels and annotations
4546
// * And then in a subsequent syncMachines call a metadataFieldManager can take ownership for labels and annotations.
47+
// Note: This is done so that this func does not rely on managedFields being stored in the cache, so we can optimize
48+
// memory usage by dropping managedFields before storing objects in the cache.
4649
func RemoveManagedFieldsForLabelsAndAnnotations(ctx context.Context, c client.Client, apiReader client.Reader, object client.Object, fieldManager string) error {
4750
objectKey := client.ObjectKeyFromObject(object)
4851
objectGVK, err := apiutil.GVKForObject(object, c.Scheme())
@@ -78,6 +81,11 @@ func RemoveManagedFieldsForLabelsAndAnnotations(ctx context.Context, c client.Cl
7881
if managedField.Manager == fieldManager &&
7982
managedField.Operation == metav1.ManagedFieldsOperationApply &&
8083
managedField.Subresource == "" {
84+
// If fieldManager does not own labels and annotations there's nothing to do.
85+
if !bytes.Contains(managedField.FieldsV1.Raw, []byte("f:metadata")) {
86+
return nil
87+
}
88+
8189
// Unmarshal the managed fields into a map[string]interface{}
8290
fieldsV1 := map[string]interface{}{}
8391
if err := json.Unmarshal(managedField.FieldsV1.Raw, &fieldsV1); err != nil {
@@ -125,6 +133,7 @@ func RemoveManagedFieldsForLabelsAndAnnotations(ctx context.Context, c client.Cl
125133
// MigrateManagedFields migrates managedFields.
126134
// ManagedFields are only migrated if fieldManager owns labels+annotations
127135
// The migration deletes all non-status managedField entries for fieldManager:Apply and manager:Update.
136+
// Additionally it adds a seed entry for metadataFieldManager.
128137
// Note: We have to call this func for every Machine created with CAPI <= v1.11 once.
129138
// Given that this was introduced in CAPI v1.12 and our n-3 upgrade policy this can
130139
// be removed with CAPI v1.15.
@@ -198,6 +207,11 @@ func needsMigration(object client.Object, fieldManager string) (bool, error) {
198207
if managedField.Manager == fieldManager &&
199208
managedField.Operation == metav1.ManagedFieldsOperationApply &&
200209
managedField.Subresource == "" {
210+
// If fieldManager does not own the cluster-name label migration is not needed
211+
if !bytes.Contains(managedField.FieldsV1.Raw, []byte("f:cluster.x-k8s.io/cluster-name")) {
212+
return false, nil
213+
}
214+
201215
// Unmarshal the managed fields into a map[string]interface{}
202216
fieldsV1 := map[string]interface{}{}
203217
if err := json.Unmarshal(managedField.FieldsV1.Raw, &fieldsV1); err != nil {

‎internal/util/ssa/managedfields_test.go‎

Lines changed: 25 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,8 @@ func TestRemoveManagedFieldsForLabelsAndAnnotations(t *testing.T) {
4949
Format: bootstrapv1.CloudConfig,
5050
},
5151
}
52+
kubeadmConfigWithoutFinalizer := kubeadmConfig.DeepCopy()
53+
kubeadmConfigWithoutFinalizer.Finalizers = nil
5254
kubeadmConfigWithLabelsAndAnnotations := kubeadmConfig.DeepCopy()
5355
kubeadmConfigWithLabelsAndAnnotations.Labels = map[string]string{
5456
"label-1": "value-1",
@@ -93,6 +95,29 @@ func TestRemoveManagedFieldsForLabelsAndAnnotations(t *testing.T) {
9395
"v:\"test.com/finalizer\"":{}
9496
}
9597
},
98+
"f:spec":{
99+
"f:format":{}
100+
}}`,
101+
}},
102+
},
103+
{
104+
name: "no-op if there are no managedFields for labels and annotations (not even metadata)",
105+
// Note: This case should never happen, but using it to test the no-op code path.
106+
kubeadmConfig: kubeadmConfigWithoutFinalizer.DeepCopy(),
107+
// Note: After create testFieldManager should own all fields.
108+
expectedManagedFieldsAfterCreate: []managedFieldEntry{{
109+
Manager: testFieldManager,
110+
Operation: metav1.ManagedFieldsOperationApply,
111+
FieldsV1: `{
112+
"f:spec":{
113+
"f:format":{}
114+
}}`,
115+
}},
116+
// Note: Expect no change.
117+
expectedManagedFieldsAfterRemoval: []managedFieldEntry{{
118+
Manager: testFieldManager,
119+
Operation: metav1.ManagedFieldsOperationApply,
120+
FieldsV1: `{
96121
"f:spec":{
97122
"f:format":{}
98123
}}`,

0 commit comments

Comments
 (0)