Skip to content

Commit 86c8608

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #1688 from vincepri/md-ms-match-labels
✨Auto-select on cluster-name label in MachineDeployment and MachineSet
2 parents 685e7dc + 36cf145 commit 86c8608

5 files changed

+67
-64
lines changed

‎controllers/machinedeployment_controller.go

+19-32
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,6 @@ package controllers
1919
import (
2020
"context"
2121
"fmt"
22-
"reflect"
2322

2423
"github.com/go-logr/logr"
2524
"github.com/pkg/errors"
@@ -109,49 +108,26 @@ func (r *MachineDeploymentReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
109108

110109
func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv1.MachineDeployment) (ctrl.Result, error) {
111110
logger := r.Log.WithValues("machinedeployment", d.Name, "namespace", d.Namespace)
111+
logger.V(4).Info("Reconcile MachineDeployment")
112112

113+
// Reconcile defaults.
113114
clusterv1.PopulateDefaultsMachineDeployment(d)
114115

115-
// Test for an empty LabelSelector and short circuit if that is the case
116-
// TODO: When we have validation webhooks, we should likely reject on an empty LabelSelector
117-
everything := metav1.LabelSelector{}
118-
if reflect.DeepEqual(d.Spec.Selector, &everything) {
119-
if d.Status.ObservedGeneration < d.Generation {
120-
patch := client.MergeFrom(d.DeepCopy())
121-
d.Status.ObservedGeneration = d.Generation
122-
if err := r.Client.Status().Patch(ctx, d, patch); err != nil {
123-
logger.Error(err, "Failed to patch status")
124-
return ctrl.Result{}, err
125-
}
126-
}
127-
return ctrl.Result{}, nil
128-
}
129-
130-
// Make sure that label selector can match the template's labels.
131-
// TODO(vincepri): Move to a validation (admission) webhook when supported.
132-
selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
133-
if err != nil {
134-
return ctrl.Result{}, errors.Wrapf(err, "failed to parse MachineDeployment %q label selector", d.Name)
135-
}
136-
137-
if !selector.Matches(labels.Set(d.Spec.Template.Labels)) {
138-
return ctrl.Result{}, errors.Errorf("failed validation on MachineDeployment %q label selector, cannot match Machine template labels", d.Name)
139-
}
140-
141-
// Copy label selector to its status counterpart in string format.
142-
// This is necessary for CRDs including scale subresources.
143-
d.Status.Selector = selector.String()
144-
145116
// Reconcile and retrieve the Cluster object.
146117
if d.Labels == nil {
147118
d.Labels = make(map[string]string)
148119
}
149120
d.Labels[clusterv1.ClusterLabelName] = d.Spec.ClusterName
150121

151-
// Add MachineDeploymentLabelName label to machines
122+
// Make sure selector and template to be in the same cluster.
123+
d.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = d.Spec.ClusterName
124+
d.Spec.Template.Labels[clusterv1.ClusterLabelName] = d.Spec.ClusterName
125+
126+
// Add label and selector based on the MachineDeployment's name.
152127
d.Spec.Selector.MatchLabels[clusterv1.MachineDeploymentLabelName] = d.Name
153128
d.Spec.Template.Labels[clusterv1.MachineDeploymentLabelName] = d.Name
154129

130+
// Check for Cluster ownership.
155131
cluster, err := util.GetClusterByName(ctx, r.Client, d.ObjectMeta.Namespace, d.Spec.ClusterName)
156132
if err != nil {
157133
return ctrl.Result{}, err
@@ -172,6 +148,17 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv
172148
}
173149
}
174150

151+
// Make sure that label selector can match the template's labels.
152+
// TODO(vincepri): Move to a validation (admission) webhook when supported.
153+
selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
154+
if err != nil {
155+
return ctrl.Result{}, errors.Wrapf(err, "failed to parse MachineDeployment %q label selector", d.Name)
156+
}
157+
158+
if !selector.Matches(labels.Set(d.Spec.Template.Labels)) {
159+
return ctrl.Result{}, errors.Errorf("failed validation on MachineDeployment %q label selector, cannot match Machine template labels", d.Name)
160+
}
161+
175162
msList, err := r.getMachineSetsForDeployment(d)
176163
if err != nil {
177164
return ctrl.Result{}, err

‎controllers/machinedeployment_controller_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -327,6 +327,9 @@ var _ = Describe("MachineDeployment Reconciler", func() {
327327

328328
return len(machineSets.Items)
329329
}, timeout*5).Should(BeEquivalentTo(0))
330+
331+
// Validate that the controller set the cluster name label in selector.
332+
Expect(deployment.Status.Selector).To(ContainSubstring(testCluster.Name))
330333
})
331334
})
332335

‎controllers/machinedeployment_sync.go

+4
Original file line numberDiff line numberDiff line change
@@ -362,9 +362,13 @@ func calculateStatus(allMSs []*clusterv1.MachineSet, newMS *clusterv1.MachineSet
362362
unavailableReplicas = 0
363363
}
364364

365+
// Calculate the label selector. We check the error in the MD reconcile function, ignore here.
366+
selector, _ := metav1.LabelSelectorAsSelector(&deployment.Spec.Selector) //nolint
367+
365368
status := clusterv1.MachineDeploymentStatus{
366369
// TODO: Ensure that if we start retrying status updates, we won't pick up a new Generation value.
367370
ObservedGeneration: deployment.Generation,
371+
Selector: selector.String(),
368372
Replicas: mdutil.GetActualReplicaCountForMachineSets(allMSs),
369373
UpdatedReplicas: mdutil.GetActualReplicaCountForMachineSets([]*clusterv1.MachineSet{newMS}),
370374
ReadyReplicas: mdutil.GetReadyReplicaCountForMachineSets(allMSs),

‎controllers/machineset_controller.go

+38-32
Original file line numberDiff line numberDiff line change
@@ -121,9 +121,46 @@ func (r *MachineSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
121121

122122
func (r *MachineSetReconciler) reconcile(ctx context.Context, machineSet *clusterv1.MachineSet) (ctrl.Result, error) {
123123
logger := r.Log.WithValues("machineset", machineSet.Name, "namespace", machineSet.Namespace)
124-
125124
logger.V(4).Info("Reconcile MachineSet")
126125

126+
// Reconcile and retrieve the Cluster object.
127+
if machineSet.Labels == nil {
128+
machineSet.Labels = make(map[string]string)
129+
}
130+
machineSet.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
131+
132+
cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)
133+
if err != nil {
134+
return ctrl.Result{}, err
135+
}
136+
137+
if r.shouldAdopt(machineSet) {
138+
patch := client.MergeFrom(machineSet.DeepCopy())
139+
machineSet.OwnerReferences = util.EnsureOwnerRef(machineSet.OwnerReferences, metav1.OwnerReference{
140+
APIVersion: clusterv1.GroupVersion.String(),
141+
Kind: "Cluster",
142+
Name: cluster.Name,
143+
UID: cluster.UID,
144+
})
145+
// Patch using a deep copy to avoid overwriting any unexpected Status changes from the returned result
146+
if err := r.Client.Patch(ctx, machineSet.DeepCopy(), patch); err != nil {
147+
return ctrl.Result{}, errors.Wrapf(
148+
err,
149+
"failed to add OwnerReference to MachineSet %s/%s",
150+
machineSet.Namespace,
151+
machineSet.Name,
152+
)
153+
}
154+
}
155+
156+
// Make sure selector and template to be in the same cluster.
157+
machineSet.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
158+
machineSet.Spec.Template.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
159+
160+
// Add label and selector based on the MachineSet's name.
161+
machineSet.Spec.Selector.MatchLabels[clusterv1.MachineSetLabelName] = machineSet.Name
162+
machineSet.Spec.Template.Labels[clusterv1.MachineSetLabelName] = machineSet.Name
163+
127164
// Make sure that label selector can match template's labels.
128165
// TODO(vincepri): Move to a validation (admission) webhook when supported.
129166
selector, err := metav1.LabelSelectorAsSelector(&machineSet.Spec.Selector)
@@ -155,36 +192,6 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, machineSet *cluste
155192
return ctrl.Result{}, errors.Wrap(err, "failed to list machines")
156193
}
157194

158-
// Reconcile and retrieve the Cluster object.
159-
if machineSet.Labels == nil {
160-
machineSet.Labels = make(map[string]string)
161-
}
162-
machineSet.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
163-
164-
cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)
165-
if err != nil {
166-
return ctrl.Result{}, err
167-
}
168-
169-
if r.shouldAdopt(machineSet) {
170-
patch := client.MergeFrom(machineSet.DeepCopy())
171-
machineSet.OwnerReferences = util.EnsureOwnerRef(machineSet.OwnerReferences, metav1.OwnerReference{
172-
APIVersion: clusterv1.GroupVersion.String(),
173-
Kind: "Cluster",
174-
Name: cluster.Name,
175-
UID: cluster.UID,
176-
})
177-
// Patch using a deep copy to avoid overwriting any unexpected Status changes from the returned result
178-
if err := r.Client.Patch(ctx, machineSet.DeepCopy(), patch); err != nil {
179-
return ctrl.Result{}, errors.Wrapf(
180-
err,
181-
"failed to add OwnerReference to MachineSet %s/%s",
182-
machineSet.Namespace,
183-
machineSet.Name,
184-
)
185-
}
186-
}
187-
188195
// Filter out irrelevant machines (deleting/mismatch labels) and claim orphaned machines.
189196
filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items))
190197
for idx := range allMachines.Items {
@@ -395,7 +402,6 @@ func (r *MachineSetReconciler) getNewMachine(machineSet *clusterv1.MachineSet) *
395402
if machine.Labels == nil {
396403
machine.Labels = make(map[string]string)
397404
}
398-
machine.Labels[clusterv1.MachineSetLabelName] = machineSet.Name
399405
return machine
400406
}
401407

‎controllers/machineset_controller_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -201,6 +201,9 @@ var _ = Describe("MachineSet Reconciler", func() {
201201
}
202202
return instance.Status.AvailableReplicas
203203
}, timeout).Should(BeEquivalentTo(replicas))
204+
205+
// Validate that the controller set the cluster name label in selector.
206+
Expect(instance.Status.Selector).To(ContainSubstring(testCluster.Name))
204207
})
205208
})
206209

0 commit comments

Comments
 (0)