Skip to content

Commit 837bfa7

Browse files
vinceprivincepri
committed
Auto-select on cluster-name label in MachineDeployment and MachineSet
Signed-off-by: Vince Prignano <[email protected]>
1 parent 24a1f81 commit 837bfa7

5 files changed

+76
-61
lines changed

‎controllers/machinedeployment_controller.go

+32-30
Original file line numberDiff line numberDiff line change
@@ -104,39 +104,11 @@ func (r *MachineDeploymentReconciler) Reconcile(req ctrl.Request) (ctrl.Result,
104104

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

109+
// Reconcile defaults.
108110
clusterv1.PopulateDefaultsMachineDeployment(d)
109111

110-
// Test for an empty LabelSelector and short circuit if that is the case
111-
// TODO: When we have validation webhooks, we should likely reject on an empty LabelSelector
112-
everything := metav1.LabelSelector{}
113-
if reflect.DeepEqual(d.Spec.Selector, &everything) {
114-
if d.Status.ObservedGeneration < d.Generation {
115-
patch := client.MergeFrom(d.DeepCopy())
116-
d.Status.ObservedGeneration = d.Generation
117-
if err := r.Client.Status().Patch(ctx, d, patch); err != nil {
118-
logger.Error(err, "Failed to patch status")
119-
return ctrl.Result{}, err
120-
}
121-
}
122-
return ctrl.Result{}, nil
123-
}
124-
125-
// Make sure that label selector can match the template's labels.
126-
// TODO(vincepri): Move to a validation (admission) webhook when supported.
127-
selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
128-
if err != nil {
129-
return ctrl.Result{}, errors.Wrapf(err, "failed to parse MachineDeployment %q label selector", d.Name)
130-
}
131-
132-
if !selector.Matches(labels.Set(d.Spec.Template.Labels)) {
133-
return ctrl.Result{}, errors.Errorf("failed validation on MachineDeployment %q label selector, cannot match Machine template labels", d.Name)
134-
}
135-
136-
// Copy label selector to its status counterpart in string format.
137-
// This is necessary for CRDs including scale subresources.
138-
d.Status.Selector = selector.String()
139-
140112
// Reconcile and retrieve the Cluster object.
141113
if d.Labels == nil {
142114
d.Labels = make(map[string]string)
@@ -163,6 +135,36 @@ func (r *MachineDeploymentReconciler) reconcile(ctx context.Context, d *clusterv
163135
}
164136
}
165137

138+
// Test for an empty LabelSelector and short circuit if that is the case
139+
// TODO: When we have validation webhooks, we should likely reject on an empty LabelSelector
140+
everything := metav1.LabelSelector{}
141+
if reflect.DeepEqual(d.Spec.Selector, &everything) {
142+
if d.Status.ObservedGeneration < d.Generation {
143+
patch := client.MergeFrom(d.DeepCopy())
144+
d.Status.ObservedGeneration = d.Generation
145+
if err := r.Client.Status().Patch(ctx, d, patch); err != nil {
146+
logger.Error(err, "Failed to patch status")
147+
return ctrl.Result{}, err
148+
}
149+
}
150+
return ctrl.Result{}, nil
151+
}
152+
153+
// Make sure selector and template to be in the same cluster.
154+
d.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = d.Spec.ClusterName
155+
d.Spec.Template.Labels[clusterv1.ClusterLabelName] = d.Spec.ClusterName
156+
157+
// Make sure that label selector can match the template's labels.
158+
// TODO(vincepri): Move to a validation (admission) webhook when supported.
159+
selector, err := metav1.LabelSelectorAsSelector(&d.Spec.Selector)
160+
if err != nil {
161+
return ctrl.Result{}, errors.Wrapf(err, "failed to parse MachineDeployment %q label selector", d.Name)
162+
}
163+
164+
if !selector.Matches(labels.Set(d.Spec.Template.Labels)) {
165+
return ctrl.Result{}, errors.Errorf("failed validation on MachineDeployment %q label selector, cannot match Machine template labels", d.Name)
166+
}
167+
166168
msList, err := r.getMachineSetsForDeployment(d)
167169
if err != nil {
168170
return ctrl.Result{}, err

‎controllers/machinedeployment_controller_test.go

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

311311
return len(machineSets.Items)
312312
}, timeout*5).Should(BeEquivalentTo(0))
313+
314+
// Validate that the controller set the cluster name label in selector.
315+
Expect(deployment.Status.Selector).To(ContainSubstring(testCluster.Name))
313316
})
314317
})
315318

‎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

+34-31
Original file line numberDiff line numberDiff line change
@@ -113,9 +113,42 @@ func (r *MachineSetReconciler) Reconcile(req ctrl.Request) (ctrl.Result, error)
113113

114114
func (r *MachineSetReconciler) reconcile(ctx context.Context, machineSet *clusterv1.MachineSet) (ctrl.Result, error) {
115115
logger := r.Log.WithValues("machineset", machineSet.Name, "namespace", machineSet.Namespace)
116-
117116
logger.V(4).Info("Reconcile MachineSet")
118117

118+
// Reconcile and retrieve the Cluster object.
119+
if machineSet.Labels == nil {
120+
machineSet.Labels = make(map[string]string)
121+
}
122+
machineSet.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
123+
124+
cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)
125+
if err != nil {
126+
return ctrl.Result{}, err
127+
}
128+
129+
if r.shouldAdopt(machineSet) {
130+
patch := client.MergeFrom(machineSet.DeepCopy())
131+
machineSet.OwnerReferences = util.EnsureOwnerRef(machineSet.OwnerReferences, metav1.OwnerReference{
132+
APIVersion: clusterv1.GroupVersion.String(),
133+
Kind: "Cluster",
134+
Name: cluster.Name,
135+
UID: cluster.UID,
136+
})
137+
// Patch using a deep copy to avoid overwriting any unexpected Status changes from the returned result
138+
if err := r.Client.Patch(ctx, machineSet.DeepCopy(), patch); err != nil {
139+
return ctrl.Result{}, errors.Wrapf(
140+
err,
141+
"failed to add OwnerReference to MachineSet %s/%s",
142+
machineSet.Namespace,
143+
machineSet.Name,
144+
)
145+
}
146+
}
147+
148+
// Make sure selector and template to be in the same cluster.
149+
machineSet.Spec.Selector.MatchLabels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
150+
machineSet.Spec.Template.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
151+
119152
// Make sure that label selector can match template's labels.
120153
// TODO(vincepri): Move to a validation (admission) webhook when supported.
121154
selector, err := metav1.LabelSelectorAsSelector(&machineSet.Spec.Selector)
@@ -147,36 +180,6 @@ func (r *MachineSetReconciler) reconcile(ctx context.Context, machineSet *cluste
147180
return ctrl.Result{}, errors.Wrap(err, "failed to list machines")
148181
}
149182

150-
// Reconcile and retrieve the Cluster object.
151-
if machineSet.Labels == nil {
152-
machineSet.Labels = make(map[string]string)
153-
}
154-
machineSet.Labels[clusterv1.ClusterLabelName] = machineSet.Spec.ClusterName
155-
156-
cluster, err := util.GetClusterByName(ctx, r.Client, machineSet.ObjectMeta.Namespace, machineSet.Spec.ClusterName)
157-
if err != nil {
158-
return ctrl.Result{}, err
159-
}
160-
161-
if r.shouldAdopt(machineSet) {
162-
patch := client.MergeFrom(machineSet.DeepCopy())
163-
machineSet.OwnerReferences = util.EnsureOwnerRef(machineSet.OwnerReferences, metav1.OwnerReference{
164-
APIVersion: clusterv1.GroupVersion.String(),
165-
Kind: "Cluster",
166-
Name: cluster.Name,
167-
UID: cluster.UID,
168-
})
169-
// Patch using a deep copy to avoid overwriting any unexpected Status changes from the returned result
170-
if err := r.Client.Patch(ctx, machineSet.DeepCopy(), patch); err != nil {
171-
return ctrl.Result{}, errors.Wrapf(
172-
err,
173-
"failed to add OwnerReference to MachineSet %s/%s",
174-
machineSet.Namespace,
175-
machineSet.Name,
176-
)
177-
}
178-
}
179-
180183
// Filter out irrelevant machines (deleting/mismatch labels) and claim orphaned machines.
181184
filteredMachines := make([]*clusterv1.Machine, 0, len(allMachines.Items))
182185
for idx := range allMachines.Items {

‎controllers/machineset_controller_test.go

+3
Original file line numberDiff line numberDiff line change
@@ -196,6 +196,9 @@ var _ = Describe("MachineSet Reconciler", func() {
196196
}
197197
return instance.Status.AvailableReplicas
198198
}, timeout).Should(BeEquivalentTo(replicas))
199+
200+
// Validate that the controller set the cluster name label in selector.
201+
Expect(instance.Status.Selector).To(ContainSubstring(testCluster.Name))
199202
})
200203
})
201204

0 commit comments

Comments
 (0)