Skip to content

Commit 5f50101

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request #5386 from PanSpagetka/rosa-machinepool-autoscaling-fix
🐛 ROSA: Apply CAPI machinepool changes to ROSAMachinePool
2 parents c470da2 + e236600 commit 5f50101

File tree

3 files changed

+205
-26
lines changed

3 files changed

+205
-26
lines changed

‎exp/controllers/rosamachinepool_controller.go‎

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -366,14 +366,23 @@ func (r *ROSAMachinePoolReconciler) reconcileMachinePoolVersion(machinePoolScope
366366
return nil
367367
}
368368

369+
func (r *ROSAMachinePoolReconciler) shouldUpdateRosaReplicas(machinePoolScope *scope.RosaMachinePoolScope, nodePool *cmv1.NodePool) bool {
370+
if machinePoolScope.MachinePool.Spec.Replicas == nil || machinePoolScope.RosaMachinePool.Spec.Autoscaling != nil || annotations.ReplicasManagedByExternalAutoscaler(machinePoolScope.MachinePool) {
371+
return false
372+
}
373+
374+
return nodePool.Replicas() != int(*machinePoolScope.MachinePool.Spec.Replicas)
375+
}
376+
369377
func (r *ROSAMachinePoolReconciler) updateNodePool(machinePoolScope *scope.RosaMachinePoolScope, ocmClient rosa.OCMClient, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) {
370378
machinePool := machinePoolScope.RosaMachinePool.DeepCopy()
371379
// default all fields before comparing, so that nil/unset fields don't cause an unnecessary update call.
372380
machinePool.Default()
373381
desiredSpec := machinePool.Spec
374382

375383
specDiff := computeSpecDiff(desiredSpec, nodePool)
376-
if specDiff == "" {
384+
// Replicas are not part of RosaMachinePoolSpec
385+
if specDiff == "" && !r.shouldUpdateRosaReplicas(machinePoolScope, nodePool) {
377386
// no changes detected.
378387
return nodePool, nil
379388
}

‎exp/controllers/rosamachinepool_controller_test.go‎

Lines changed: 194 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -218,16 +218,18 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
218218
}
219219

220220
tests := []struct {
221-
name string
222-
new *expinfrav1.ROSAMachinePool
223-
old *expinfrav1.ROSAMachinePool
224-
expect func(m *mocks.MockOCMClientMockRecorder)
225-
result reconcile.Result
221+
name string
222+
newROSAMachinePool *expinfrav1.ROSAMachinePool
223+
oldROSAMachinePool *expinfrav1.ROSAMachinePool
224+
machinePool *expclusterv1.MachinePool
225+
expect func(m *mocks.MockOCMClientMockRecorder)
226+
result reconcile.Result
226227
}{
227228
{
228-
name: "create node pool, nodepool doesn't exist",
229-
old: rosaMachinePool(0),
230-
new: &expinfrav1.ROSAMachinePool{
229+
name: "create node pool, nodepool doesn't exist",
230+
machinePool: ownerMachinePool(0),
231+
oldROSAMachinePool: rosaMachinePool(0),
232+
newROSAMachinePool: &expinfrav1.ROSAMachinePool{
231233
ObjectMeta: metav1.ObjectMeta{
232234
Name: "rosa-machinepool",
233235
Namespace: ns.Name,
@@ -259,9 +261,10 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
259261
},
260262
},
261263
{
262-
name: "Nodepool exist, but is not ready",
263-
old: rosaMachinePool(1),
264-
new: &expinfrav1.ROSAMachinePool{
264+
name: "Nodepool exist, but is not ready",
265+
machinePool: ownerMachinePool(1),
266+
oldROSAMachinePool: rosaMachinePool(1),
267+
newROSAMachinePool: &expinfrav1.ROSAMachinePool{
265268
ObjectMeta: metav1.ObjectMeta{
266269
Name: "rosa-machinepool",
267270
Namespace: ns.Name,
@@ -297,9 +300,10 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
297300
},
298301
},
299302
{
300-
name: "Nodepool is ready",
301-
old: rosaMachinePool(2),
302-
new: &expinfrav1.ROSAMachinePool{
303+
name: "Nodepool is ready",
304+
machinePool: ownerMachinePool(2),
305+
oldROSAMachinePool: rosaMachinePool(2),
306+
newROSAMachinePool: &expinfrav1.ROSAMachinePool{
303307
ObjectMeta: metav1.ObjectMeta{
304308
Name: "rosa-machinepool",
305309
Namespace: ns.Name,
@@ -343,6 +347,151 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
343347
m.CreateNodePool(gomock.Any(), gomock.Any()).Times(0)
344348
},
345349
},
350+
{
351+
name: "Create nodepool, replicas are set in MachinePool",
352+
machinePool: &expclusterv1.MachinePool{
353+
ObjectMeta: metav1.ObjectMeta{
354+
Name: ownerMachinePool(3).Name,
355+
Namespace: ns.Name,
356+
Labels: map[string]string{clusterv1.ClusterNameLabel: ownerCluster(3).Name},
357+
UID: types.UID("owner-mp-uid--3"),
358+
},
359+
TypeMeta: metav1.TypeMeta{
360+
Kind: "MachinePool",
361+
APIVersion: clusterv1.GroupVersion.String(),
362+
},
363+
Spec: expclusterv1.MachinePoolSpec{
364+
ClusterName: ownerCluster(3).Name,
365+
Replicas: ptr.To[int32](2),
366+
Template: clusterv1.MachineTemplateSpec{
367+
Spec: clusterv1.MachineSpec{
368+
ClusterName: ownerCluster(3).Name,
369+
InfrastructureRef: corev1.ObjectReference{
370+
UID: rosaMachinePool(3).UID,
371+
Name: rosaMachinePool(3).Name,
372+
Namespace: ns.Namespace,
373+
Kind: "ROSAMachinePool",
374+
APIVersion: expclusterv1.GroupVersion.String(),
375+
},
376+
},
377+
},
378+
},
379+
},
380+
oldROSAMachinePool: rosaMachinePool(3),
381+
newROSAMachinePool: &expinfrav1.ROSAMachinePool{
382+
ObjectMeta: metav1.ObjectMeta{
383+
Name: "rosa-machinepool",
384+
Namespace: ns.Name,
385+
UID: "rosa-machinepool",
386+
},
387+
TypeMeta: metav1.TypeMeta{
388+
Kind: "ROSAMachinePool",
389+
APIVersion: expinfrav1.GroupVersion.String(),
390+
},
391+
Spec: expinfrav1.RosaMachinePoolSpec{
392+
NodePoolName: "test-nodepool",
393+
Version: "4.14.5",
394+
Subnet: "subnet-id",
395+
InstanceType: "m5.large",
396+
},
397+
Status: expinfrav1.RosaMachinePoolStatus{
398+
Ready: false,
399+
ID: rosaMachinePool(3).Spec.NodePoolName,
400+
},
401+
},
402+
result: ctrl.Result{},
403+
expect: func(m *mocks.MockOCMClientMockRecorder) {
404+
m.GetNodePool(gomock.Any(), gomock.Any()).DoAndReturn(func(clusterId string, nodePoolID string) (*cmv1.NodePool, bool, error) {
405+
return nil, false, nil
406+
}).Times(1)
407+
m.CreateNodePool(gomock.Any(), matchesReplicas(2)).DoAndReturn(func(clusterId string, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) {
408+
return nodePool, nil
409+
}).Times(1)
410+
},
411+
},
412+
{
413+
name: "Update nodepool, replicas are updated from MachinePool",
414+
machinePool: &expclusterv1.MachinePool{
415+
ObjectMeta: metav1.ObjectMeta{
416+
Name: ownerMachinePool(4).Name,
417+
Namespace: ns.Name,
418+
Labels: map[string]string{clusterv1.ClusterNameLabel: ownerCluster(4).Name},
419+
UID: types.UID("owner-mp-uid--4"),
420+
},
421+
TypeMeta: metav1.TypeMeta{
422+
Kind: "MachinePool",
423+
APIVersion: clusterv1.GroupVersion.String(),
424+
},
425+
Spec: expclusterv1.MachinePoolSpec{
426+
ClusterName: ownerCluster(4).Name,
427+
Replicas: ptr.To[int32](2),
428+
Template: clusterv1.MachineTemplateSpec{
429+
Spec: clusterv1.MachineSpec{
430+
ClusterName: ownerCluster(4).Name,
431+
InfrastructureRef: corev1.ObjectReference{
432+
UID: rosaMachinePool(4).UID,
433+
Name: rosaMachinePool(4).Name,
434+
Namespace: ns.Namespace,
435+
Kind: "ROSAMachinePool",
436+
APIVersion: expclusterv1.GroupVersion.String(),
437+
},
438+
},
439+
},
440+
},
441+
},
442+
oldROSAMachinePool: rosaMachinePool(4),
443+
newROSAMachinePool: &expinfrav1.ROSAMachinePool{
444+
ObjectMeta: metav1.ObjectMeta{
445+
Name: "rosa-machinepool",
446+
Namespace: ns.Name,
447+
UID: "rosa-machinepool",
448+
},
449+
TypeMeta: metav1.TypeMeta{
450+
Kind: "ROSAMachinePool",
451+
APIVersion: expinfrav1.GroupVersion.String(),
452+
},
453+
Spec: expinfrav1.RosaMachinePoolSpec{
454+
NodePoolName: "test-nodepool",
455+
Version: "4.14.5",
456+
Subnet: "subnet-id",
457+
InstanceType: "m5.large",
458+
},
459+
Status: expinfrav1.RosaMachinePoolStatus{
460+
// Ready: false,
461+
Ready: true,
462+
Replicas: 2,
463+
},
464+
},
465+
result: ctrl.Result{},
466+
expect: func(m *mocks.MockOCMClientMockRecorder) {
467+
m.GetNodePool(gomock.Any(), gomock.Any()).DoAndReturn(func(clusterId string, nodePoolID string) (*cmv1.NodePool, bool, error) {
468+
rosaSpec := rosaMachinePool(4).Spec
469+
rosaSpec.UpdateConfig = &expinfrav1.RosaUpdateConfig{
470+
RollingUpdate: &expinfrav1.RollingUpdate{
471+
MaxSurge: ptr.To(intstr.FromInt32(1)),
472+
MaxUnavailable: ptr.To(intstr.FromInt32(0)),
473+
},
474+
}
475+
nodePoolBuilder := nodePoolBuilder(rosaSpec, ownerMachinePool(4).Spec, rosacontrolplanev1.Stable)
476+
statusBuilder := (&cmv1.NodePoolStatusBuilder{}).CurrentReplicas(1)
477+
nodeGrace := (&cmv1.ValueBuilder{}).Unit("s").Value(0)
478+
nodePool, err := nodePoolBuilder.ID("test-nodepool").Replicas(1).Status(statusBuilder).AutoRepair(true).NodeDrainGracePeriod(nodeGrace).Build()
479+
g.Expect(err).NotTo(HaveOccurred())
480+
481+
return nodePool, true, nil
482+
}).Times(1)
483+
m.UpdateNodePool(gomock.Any(), matchesReplicas(2)).DoAndReturn(func(clusterID string, nodePool *cmv1.NodePool) (*cmv1.NodePool, error) {
484+
statusBuilder := (&cmv1.NodePoolStatusBuilder{}).CurrentReplicas(2)
485+
version := (&cmv1.VersionBuilder{}).RawID("4.14.5")
486+
npBuilder := cmv1.NodePoolBuilder{}
487+
updatedNodePool, err := npBuilder.Copy(nodePool).Status(statusBuilder).Version(version).Build()
488+
g.Expect(err).NotTo(HaveOccurred())
489+
490+
return updatedNodePool, nil
491+
}).Times(1)
492+
m.CreateNodePool(gomock.Any(), gomock.Any()).Times(0)
493+
},
494+
},
346495
}
347496

348497
createObject(g, secret, ns.Name)
@@ -353,7 +502,7 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
353502
for i, test := range tests {
354503
t.Run(test.name, func(t *testing.T) {
355504
// This is set by CAPI MachinePool reconcile
356-
test.old.OwnerReferences = []metav1.OwnerReference{
505+
test.oldROSAMachinePool.OwnerReferences = []metav1.OwnerReference{
357506
{
358507
Name: ownerMachinePool(i).Name,
359508
UID: ownerMachinePool(i).UID,
@@ -362,7 +511,7 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
362511
},
363512
}
364513
cp := rosaControlPlane(i)
365-
objects := []client.Object{ownerCluster(i), ownerMachinePool(i), cp, test.old}
514+
objects := []client.Object{ownerCluster(i), test.machinePool, cp, test.oldROSAMachinePool}
366515

367516
for _, obj := range objects {
368517
createObject(g, obj, ns.Name)
@@ -374,16 +523,16 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
374523
g.Expect(err).ShouldNot(HaveOccurred())
375524

376525
// patch status conditions
377-
rmpPh, err := patch.NewHelper(test.old, testEnv)
378-
test.old.Status.Conditions = clusterv1.Conditions{
526+
rmpPh, err := patch.NewHelper(test.oldROSAMachinePool, testEnv)
527+
test.oldROSAMachinePool.Status.Conditions = clusterv1.Conditions{
379528
{
380529
Type: "Paused",
381530
Status: corev1.ConditionFalse,
382531
Reason: "NotPaused",
383532
},
384533
}
385534

386-
g.Expect(rmpPh.Patch(ctx, test.old)).To(Succeed())
535+
g.Expect(rmpPh.Patch(ctx, test.oldROSAMachinePool)).To(Succeed())
387536
g.Expect(err).ShouldNot(HaveOccurred())
388537

389538
// patching is not reliably synchronous
@@ -410,20 +559,20 @@ func TestRosaMachinePoolReconcile(t *testing.T) {
410559
}
411560

412561
req := ctrl.Request{}
413-
req.NamespacedName = types.NamespacedName{Name: test.old.Name, Namespace: ns.Name}
562+
req.NamespacedName = types.NamespacedName{Name: test.oldROSAMachinePool.Name, Namespace: ns.Name}
414563

415564
result, errReconcile := r.Reconcile(ctx, req)
416565
g.Expect(errReconcile).ToNot(HaveOccurred())
417566
g.Expect(result).To(Equal(test.result))
418567
time.Sleep(50 * time.Millisecond)
419568

420569
m := &expinfrav1.ROSAMachinePool{}
421-
key := client.ObjectKey{Name: test.old.Name, Namespace: ns.Name}
570+
key := client.ObjectKey{Name: test.oldROSAMachinePool.Name, Namespace: ns.Name}
422571
errGet := testEnv.Get(ctx, key, m)
423572
g.Expect(errGet).NotTo(HaveOccurred())
424-
g.Expect(m.Status.Ready).To(Equal(test.new.Status.Ready))
425-
g.Expect(m.Status.Replicas).To(Equal(test.new.Status.Replicas))
426-
g.Expect(m.Status.ID).To(Equal(test.new.Status.ID))
573+
g.Expect(m.Status.Ready).To(Equal(test.newROSAMachinePool.Status.Ready))
574+
g.Expect(m.Status.Replicas).To(Equal(test.newROSAMachinePool.Status.Replicas))
575+
g.Expect(m.Status.ID).To(Equal(test.newROSAMachinePool.Status.ID))
427576

428577
// cleanup
429578
for _, obj := range objects {
@@ -556,3 +705,24 @@ func cleanupObject(g *WithT, obj client.Object) {
556705
g.Expect(testEnv.Cleanup(ctx, obj)).To(Succeed())
557706
}
558707
}
708+
709+
type replicasMatcher struct {
710+
replicas int
711+
}
712+
713+
func (m replicasMatcher) Matches(arg interface{}) bool {
714+
sarg := arg.(*cmv1.NodePool)
715+
if sarg != nil && sarg.Replicas() == m.replicas {
716+
return true
717+
}
718+
return false
719+
}
720+
721+
// Not used here, but satisfies the Matcher interface.
722+
func (m replicasMatcher) String() string {
723+
return fmt.Sprintf("Replicas %v", m.replicas)
724+
}
725+
726+
func matchesReplicas(replicas int) gomock.Matcher {
727+
return replicasMatcher{replicas: replicas}
728+
}

‎templates/cluster-template-rosa-machinepool.yaml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ metadata:
5757
name: "${CLUSTER_NAME}-pool-0"
5858
spec:
5959
clusterName: "${CLUSTER_NAME}"
60-
replicas: 1
60+
replicas: 3
6161
template:
6262
spec:
6363
clusterName: "${CLUSTER_NAME}"

0 commit comments

Comments
 (0)