Skip to content

Commit b41e37d

Browse files
fabriziopandinifabriziopandini
committed
Use MS and MD from scope instead of reading them again
1 parent ad12ab5 commit b41e37d

File tree

3 files changed

+53
-122
lines changed

3 files changed

+53
-122
lines changed

‎internal/controllers/machine/machine_controller_noderef.go‎

Lines changed: 14 additions & 45 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@ import (
2424

2525
"github.com/pkg/errors"
2626
corev1 "k8s.io/api/core/v1"
27-
apierrors "k8s.io/apimachinery/pkg/api/errors"
2827
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2928
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
3029
"k8s.io/apimachinery/pkg/runtime/schema"
@@ -143,7 +142,7 @@ func (r *Reconciler) reconcileNode(ctx context.Context, s *scope) (ctrl.Result,
143142
_, nodeHadInterruptibleLabel := s.node.Labels[clusterv1.InterruptibleLabel]
144143

145144
// Reconcile node taints
146-
if err := r.patchNode(ctx, remoteClient, s.node, nodeLabels, nodeAnnotations, machine); err != nil {
145+
if err := r.patchNode(ctx, remoteClient, s.node, nodeLabels, nodeAnnotations, s.owningMachineSet, s.owningMachineDeployment); err != nil {
147146
return ctrl.Result{}, errors.Wrapf(err, "failed to reconcile Node %s", klog.KObj(s.node))
148147
}
149148
if !nodeHadInterruptibleLabel && interruptible {
@@ -246,7 +245,7 @@ func (r *Reconciler) getNode(ctx context.Context, c client.Reader, providerID st
246245

247246
// PatchNode is required to workaround an issue on Node.Status.Address which is incorrectly annotated as patchStrategy=merge
248247
// and this causes SSA patch to fail in case there are two addresses with the same key https://github.com/kubernetes-sigs/cluster-api/issues/8417
249-
func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client, node *corev1.Node, newLabels, newAnnotations map[string]string, m *clusterv1.Machine) error {
248+
func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client, node *corev1.Node, newLabels, newAnnotations map[string]string, ms *clusterv1.MachineSet, md *clusterv1.MachineDeployment) error {
250249
newNode := node.DeepCopy()
251250

252251
// Adds the annotations from the Machine.
@@ -336,20 +335,14 @@ func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client,
336335
hasTaintChanges := taints.RemoveNodeTaint(newNode, clusterv1.NodeUninitializedTaint)
337336

338337
// Set Taint to a node in an old MachineSet and unset Taint from a node in a new MachineSet
339-
isOutdated, notFound, err := shouldNodeHaveOutdatedTaint(ctx, r.Client, m)
338+
isOutdated, err := shouldNodeHaveOutdatedTaint(ms, md)
340339
if err != nil {
341340
return errors.Wrapf(err, "failed to check if Node %s is outdated", klog.KRef("", node.Name))
342341
}
343-
344-
// It is only possible to identify if we have to set or remove the NodeOutdatedRevisionTaint if shouldNodeHaveOutdatedTaint
345-
// found all relevant objects.
346-
// Example: when the MachineDeployment or Machineset can't be found due to a background deletion of objects.
347-
if !notFound {
348-
if isOutdated {
349-
hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
350-
} else {
351-
hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
352-
}
342+
if isOutdated {
343+
hasTaintChanges = taints.EnsureNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
344+
} else {
345+
hasTaintChanges = taints.RemoveNodeTaint(newNode, clusterv1.NodeOutdatedRevisionTaint) || hasTaintChanges
353346
}
354347

355348
if !hasAnnotationChanges && !hasLabelChanges && !hasTaintChanges {
@@ -362,47 +355,23 @@ func (r *Reconciler) patchNode(ctx context.Context, remoteClient client.Client,
362355
// shouldNodeHaveOutdatedTaint tries to compare the revision of the owning MachineSet to the MachineDeployment.
363356
// It returns notFound = true if the OwnerReference is not set or the APIServer returns NotFound for the MachineSet or MachineDeployment.
364357
// Note: This three cases could happen during background deletion of objects.
365-
func shouldNodeHaveOutdatedTaint(ctx context.Context, c client.Client, m *clusterv1.Machine) (outdated bool, notFound bool, err error) {
366-
if _, hasLabel := m.Labels[clusterv1.MachineDeploymentNameLabel]; !hasLabel {
367-
return false, false, nil
358+
func shouldNodeHaveOutdatedTaint(ms *clusterv1.MachineSet, md *clusterv1.MachineDeployment) (outdated bool, err error) {
359+
if ms == nil || md == nil {
360+
return false, nil
368361
}
369362

370-
// Resolve the MachineSet name via owner references because the label value
371-
// could also be a hash.
372-
objKey, notFound, err := getOwnerMachineSetObjectKey(m.ObjectMeta)
373-
if err != nil || notFound {
374-
return false, notFound, err
375-
}
376-
ms := &clusterv1.MachineSet{}
377-
if err := c.Get(ctx, *objKey, ms); err != nil {
378-
if apierrors.IsNotFound(err) {
379-
return false, true, nil
380-
}
381-
return false, false, err
382-
}
383-
md := &clusterv1.MachineDeployment{}
384-
objKey = &client.ObjectKey{
385-
Namespace: m.Namespace,
386-
Name: m.Labels[clusterv1.MachineDeploymentNameLabel],
387-
}
388-
if err := c.Get(ctx, *objKey, md); err != nil {
389-
if apierrors.IsNotFound(err) {
390-
return false, true, nil
391-
}
392-
return false, false, err
393-
}
394363
msRev, err := mdutil.Revision(ms)
395364
if err != nil {
396-
return false, false, err
365+
return false, err
397366
}
398367
mdRev, err := mdutil.Revision(md)
399368
if err != nil {
400-
return false, false, err
369+
return false, err
401370
}
402371
if msRev < mdRev {
403-
return true, false, nil
372+
return true, nil
404373
}
405-
return false, false, nil
374+
return false, nil
406375
}
407376

408377
func getOwnerMachineSetObjectKey(obj metav1.ObjectMeta) (*client.ObjectKey, bool, error) {

‎internal/controllers/machine/machine_controller_noderef_test.go‎

Lines changed: 29 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1199,7 +1199,7 @@ func TestPatchNode(t *testing.T) {
11991199
_ = env.CleanupAndWait(ctx, oldNode, machine, ms, md)
12001200
})
12011201

1202-
err := r.patchNode(ctx, env, oldNode, tc.newLabels, tc.newAnnotations, tc.machine)
1202+
err := r.patchNode(ctx, env, oldNode, tc.newLabels, tc.newAnnotations, ms, md)
12031203
g.Expect(err).ToNot(HaveOccurred())
12041204

12051205
g.Eventually(func(g Gomega) {
@@ -1304,7 +1304,7 @@ func TestMultiplePatchNode(t *testing.T) {
13041304
_ = env.CleanupAndWait(ctx, oldNode, machine)
13051305
})
13061306

1307-
err := r.patchNode(ctx, env, oldNode, labels, tc.newAnnotations, machine)
1307+
err := r.patchNode(ctx, env, oldNode, labels, tc.newAnnotations, nil, nil)
13081308
g.Expect(err).ToNot(HaveOccurred())
13091309

13101310
newNode := &corev1.Node{}
@@ -1318,7 +1318,7 @@ func TestMultiplePatchNode(t *testing.T) {
13181318
}, 10*time.Second).Should(Succeed())
13191319

13201320
// Re-reconcile with the same metadata
1321-
err = r.patchNode(ctx, env, newNode, labels, tc.newAnnotations, machine)
1321+
err = r.patchNode(ctx, env, newNode, labels, tc.newAnnotations, nil, nil)
13221322
g.Expect(err).ToNot(HaveOccurred())
13231323

13241324
g.Eventually(func(g Gomega) {
@@ -1391,7 +1391,6 @@ func newFakeMachineDeployment(namespace, clusterName string) *clusterv1.MachineD
13911391

13921392
func Test_shouldNodeHaveOutdatedTaint(t *testing.T) {
13931393
namespaceName := "test"
1394-
namespace := &corev1.Namespace{ObjectMeta: metav1.ObjectMeta{Name: namespaceName}}
13951394

13961395
testMachineDeployment := builder.MachineDeployment(namespaceName, "my-md").
13971396
WithAnnotations(map[string]string{clusterv1.RevisionAnnotation: "1"}).
@@ -1404,88 +1403,52 @@ func Test_shouldNodeHaveOutdatedTaint(t *testing.T) {
14041403
Build()
14051404
testMachineSet.Annotations = map[string]string{clusterv1.RevisionAnnotation: "1"}
14061405

1407-
labels := map[string]string{
1408-
clusterv1.MachineDeploymentNameLabel: "my-md",
1409-
}
1410-
testMachine := builder.Machine(namespaceName, "my-machine").WithLabels(labels).Build()
1411-
testMachine.SetOwnerReferences([]metav1.OwnerReference{*ownerrefs.OwnerReferenceTo(testMachineSet, clusterv1.GroupVersion.WithKind("MachineSet"))})
1412-
14131406
tests := []struct {
1414-
name string
1415-
machine *clusterv1.Machine
1416-
objects []client.Object
1417-
wantOutdated bool
1418-
wantNotFound bool
1419-
wantErr bool
1407+
name string
1408+
machineSet *clusterv1.MachineSet
1409+
machineDeployment *clusterv1.MachineDeployment
1410+
wantOutdated bool
1411+
wantErr bool
14201412
}{
14211413
{
1422-
name: "Machineset not outdated",
1423-
machine: testMachine,
1424-
objects: []client.Object{testMachineSet, testMachineDeployment},
1425-
wantOutdated: false,
1426-
wantNotFound: false,
1427-
wantErr: false,
1428-
},
1429-
{
1430-
name: "Machineset outdated",
1431-
machine: testMachine,
1432-
objects: []client.Object{testMachineSet, testMachineDeploymentNew},
1433-
wantOutdated: true,
1434-
wantNotFound: false,
1435-
wantErr: false,
1414+
name: "Machineset not outdated",
1415+
machineSet: testMachineSet,
1416+
machineDeployment: testMachineDeployment,
1417+
wantOutdated: false,
1418+
wantErr: false,
14361419
},
14371420
{
1438-
name: "Machine without MachineDeployment label",
1439-
machine: builder.Machine(namespaceName, "no-deploy").Build(),
1440-
objects: nil,
1441-
wantOutdated: false,
1442-
wantNotFound: false,
1443-
wantErr: false,
1421+
name: "Machineset outdated",
1422+
machineSet: testMachineSet,
1423+
machineDeployment: testMachineDeploymentNew,
1424+
wantOutdated: true,
1425+
wantErr: false,
14441426
},
14451427
{
1446-
name: "Machine without OwnerReference",
1447-
machine: builder.Machine(namespaceName, "no-ownerref").WithLabels(labels).Build(),
1448-
objects: nil,
1449-
wantOutdated: false,
1450-
wantNotFound: true,
1451-
wantErr: false,
1428+
name: "Stand-alone machine",
1429+
machineSet: nil,
1430+
machineDeployment: nil,
1431+
wantOutdated: false,
1432+
wantErr: false,
14521433
},
14531434
{
1454-
name: "Machine without existing MachineSet",
1455-
machine: testMachine,
1456-
objects: nil,
1457-
wantOutdated: false,
1458-
wantNotFound: true,
1459-
wantErr: false,
1460-
},
1461-
{
1462-
name: "Machine without existing MachineDeployment",
1463-
machine: testMachine,
1464-
objects: []client.Object{testMachineSet},
1465-
wantOutdated: false,
1466-
wantNotFound: true,
1467-
wantErr: false,
1435+
name: "Stand-alone machine set",
1436+
machineSet: testMachineSet,
1437+
machineDeployment: nil,
1438+
wantOutdated: false,
1439+
wantErr: false,
14681440
},
14691441
}
14701442
for _, tt := range tests {
14711443
t.Run(tt.name, func(t *testing.T) {
1472-
objects := []client.Object{namespace}
1473-
objects = append(objects, tt.machine)
1474-
objects = append(objects, tt.objects...)
1475-
c := fake.NewClientBuilder().
1476-
WithObjects(objects...).Build()
1477-
1478-
gotOutdated, gotNotFound, err := shouldNodeHaveOutdatedTaint(ctx, c, tt.machine)
1444+
gotOutdated, err := shouldNodeHaveOutdatedTaint(tt.machineSet, tt.machineDeployment)
14791445
if (err != nil) != tt.wantErr {
14801446
t.Errorf("shouldNodeHaveOutdatedTaint() error = %v, wantErr %v", err, tt.wantErr)
14811447
return
14821448
}
14831449
if gotOutdated != tt.wantOutdated {
14841450
t.Errorf("shouldNodeHaveOutdatedTaint() = %v, want %v", gotOutdated, tt.wantOutdated)
14851451
}
1486-
if gotNotFound != tt.wantNotFound {
1487-
t.Errorf("shouldNodeHaveOutdatedTaint() = %v, want %v", gotNotFound, tt.wantNotFound)
1488-
}
14891452
})
14901453
}
14911454
}

‎internal/controllers/machineset/machineset_controller_test.go‎

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -1229,7 +1229,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
12291229
},
12301230
},
12311231
}
1232-
g.Expect(env.Create(ctx, cluster)).To(Succeed())
1232+
g.Expect(env.CreateAndWait(ctx, cluster)).To(Succeed())
12331233

12341234
t.Log("Creating the Cluster Kubeconfig Secret")
12351235
g.Expect(env.CreateKubeconfigSecret(ctx, cluster)).To(Succeed())
@@ -1258,8 +1258,8 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
12581258
Name: "ms-1",
12591259
Namespace: namespace.Name,
12601260
Labels: map[string]string{
1261-
"label-1": "true",
1262-
clusterv1.MachineDeploymentNameLabel: "md-1",
1261+
"label-1": "true",
1262+
// Note: not adding the clusterv1.MachineDeploymentNameLabel to keep the test simple.
12631263
},
12641264
},
12651265
Spec: clusterv1.MachineSetSpec{
@@ -1471,7 +1471,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
14711471
Manager: machineSetManagerName,
14721472
Operation: metav1.ManagedFieldsOperationApply,
14731473
APIVersion: clusterv1.GroupVersion.String(),
1474-
FieldsV1: fmt.Sprintf("{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:finalizers\":{\"v:\\\"machine.cluster.x-k8s.io\\\"\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/deployment-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}},\"f:ownerReferences\":{\"k:{\\\"uid\\\":\\\"%s\\\"}\":{}}},\"f:spec\":{\"f:bootstrap\":{\"f:configRef\":{\"f:apiGroup\":{},\"f:kind\":{},\"f:name\":{}}},\"f:clusterName\":{},\"f:infrastructureRef\":{\"f:apiGroup\":{},\"f:kind\":{},\"f:name\":{}}}}", ms.UID),
1474+
FieldsV1: fmt.Sprintf("{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:finalizers\":{\"v:\\\"machine.cluster.x-k8s.io\\\"\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/cluster-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}},\"f:ownerReferences\":{\"k:{\\\"uid\\\":\\\"%s\\\"}\":{}}},\"f:spec\":{\"f:bootstrap\":{\"f:configRef\":{\"f:apiGroup\":{},\"f:kind\":{},\"f:name\":{}}},\"f:clusterName\":{},\"f:infrastructureRef\":{\"f:apiGroup\":{},\"f:kind\":{},\"f:name\":{}}}}", ms.UID),
14751475
}, {
14761476
// manager owns the finalizer.
14771477
Manager: "manager",
@@ -1497,7 +1497,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
14971497
Manager: machineSetMetadataManagerName,
14981498
Operation: metav1.ManagedFieldsOperationApply,
14991499
APIVersion: updatedInfraMachine.GetAPIVersion(),
1500-
FieldsV1: "{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/deployment-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}}}}",
1500+
FieldsV1: "{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/cluster-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}}}}",
15011501
}, {
15021502
// capi-machineset owns spec.
15031503
Manager: machineSetManagerName,
@@ -1521,7 +1521,7 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
15211521
Manager: machineSetMetadataManagerName,
15221522
Operation: metav1.ManagedFieldsOperationApply,
15231523
APIVersion: updatedBootstrapConfig.GetAPIVersion(),
1524-
FieldsV1: "{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/deployment-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}}}}",
1524+
FieldsV1: "{\"f:metadata\":{\"f:annotations\":{\"f:dropped-annotation\":{},\"f:modified-annotation\":{},\"f:preserved-annotation\":{}},\"f:labels\":{\"f:cluster.x-k8s.io/cluster-name\":{},\"f:cluster.x-k8s.io/set-name\":{},\"f:dropped-label\":{},\"f:modified-label\":{},\"f:preserved-label\":{}}}}",
15251525
}, {
15261526
// capi-machineset owns spec.
15271527
Manager: machineSetManagerName,
@@ -1548,11 +1548,10 @@ func TestMachineSetReconciler_syncMachines(t *testing.T) {
15481548
// Drop "dropped-label"
15491549
}
15501550
expectedLabels := map[string]string{
1551-
"preserved-label": "preserved-value",
1552-
"modified-label": "modified-value-2",
1553-
clusterv1.MachineSetNameLabel: ms.Name,
1554-
clusterv1.MachineDeploymentNameLabel: "md-1",
1555-
clusterv1.ClusterNameLabel: testClusterName, // This label is added by the Machine controller.
1551+
"preserved-label": "preserved-value",
1552+
"modified-label": "modified-value-2",
1553+
clusterv1.MachineSetNameLabel: ms.Name,
1554+
clusterv1.ClusterNameLabel: testClusterName, // This label is added by the Machine controller.
15561555
}
15571556
ms.Spec.Template.Annotations = map[string]string{
15581557
"preserved-annotation": "preserved-value", // Keep the annotation and value as is

0 commit comments

Comments
 (0)