Skip to content

Commit 6a82514

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes-sigs#7914 from sbueringer/pr-cc-improve-kcp-readiness-checks
🌱 ClusterClass & test/framework: consider replicas for control plane readiness
2 parents 059eb76 + 6c5eb9c commit 6a82514

File tree

7 files changed

+205
-83
lines changed

7 files changed

+205
-83
lines changed

‎internal/contract/controlplane.go‎

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ import (
2222
"github.com/blang/semver"
2323
"github.com/pkg/errors"
2424
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
25+
"k8s.io/utils/pointer"
2526

2627
"sigs.k8s.io/cluster-api/util/version"
2728
)
@@ -206,6 +207,7 @@ func (c *ControlPlaneContract) IsUpgrading(obj *unstructured.Unstructured) (bool
206207
// - spec.replicas != status.replicas.
207208
// - spec.replicas != status.updatedReplicas.
208209
// - spec.replicas != status.readyReplicas.
210+
// - status.unavailableReplicas > 0.
209211
func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool, error) {
210212
desiredReplicas, err := c.Replicas().Get(obj)
211213
if err != nil {
@@ -245,9 +247,29 @@ func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool,
245247
return false, errors.Wrap(err, "failed to get control plane status readyReplicas")
246248
}
247249

250+
unavailableReplicas, err := c.UnavailableReplicas().Get(obj)
251+
if err != nil {
252+
if !errors.Is(err, errNotFound) {
253+
return false, errors.Wrap(err, "failed to get control plane status unavailableReplicas")
254+
}
255+
// If unavailableReplicas is not set on the control plane we assume it is 0.
256+
// We have to do this as the following happens after clusterctl move with KCP:
257+
// * clusterctl move creates the KCP object without status
258+
// * the KCP controller won't patch the field to 0 if it doesn't exist
259+
// * This is because the patchHelper marshals before/after object to JSON to calculate a diff
260+
// and as the unavailableReplicas field is not a pointer, not set and 0 are both rendered as 0.
261+
// If before/after of the field is the same (i.e. 0), there is no diff and thus also no patch to set it to 0.
262+
unavailableReplicas = pointer.Int64(0)
263+
}
264+
265+
// Control plane is still scaling if:
266+
// * .spec.replicas, .status.replicas, .status.updatedReplicas,
267+
// .status.readyReplicas are not equal and
268+
// * unavailableReplicas > 0
248269
if *statusReplicas != *desiredReplicas ||
249270
*updatedReplicas != *desiredReplicas ||
250-
*readyReplicas != *desiredReplicas {
271+
*readyReplicas != *desiredReplicas ||
272+
*unavailableReplicas > 0 {
251273
return true, nil
252274
}
253275
return false, nil

‎internal/contract/controlplane_test.go‎

Lines changed: 80 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -353,9 +353,10 @@ func TestControlPlaneIsScaling(t *testing.T) {
353353
"replicas": int64(2),
354354
},
355355
"status": map[string]interface{}{
356-
"replicas": int64(2),
357-
"updatedReplicas": int64(2),
358-
"readyReplicas": int64(2),
356+
"replicas": int64(2),
357+
"updatedReplicas": int64(2),
358+
"readyReplicas": int64(2),
359+
"unavailableReplicas": int64(0),
359360
},
360361
}},
361362
wantScaling: false,
@@ -370,53 +371,117 @@ func TestControlPlaneIsScaling(t *testing.T) {
370371
wantScaling: true,
371372
},
372373
{
373-
name: "should return true if status.replicas is not set on control plane",
374+
name: "should return true if status replicas is not set on control plane",
374375
obj: &unstructured.Unstructured{Object: map[string]interface{}{
375376
"spec": map[string]interface{}{
376377
"replicas": int64(2),
377378
},
378-
"status": map[string]interface{}{},
379+
"status": map[string]interface{}{
380+
"updatedReplicas": int64(2),
381+
"readyReplicas": int64(2),
382+
"unavailableReplicas": int64(0),
383+
},
379384
}},
380385
wantScaling: true,
381386
},
382387
{
383-
name: "should return true if spec and status replicas do not match",
388+
name: "should return true if spec replicas and status replicas do not match",
384389
obj: &unstructured.Unstructured{Object: map[string]interface{}{
385390
"spec": map[string]interface{}{
386391
"replicas": int64(2),
387392
},
388393
"status": map[string]interface{}{
389-
"replicas": int64(1),
390-
"updatedReplicas": int64(2),
391-
"readyReplicas": int64(2),
394+
"replicas": int64(1),
395+
"updatedReplicas": int64(2),
396+
"readyReplicas": int64(2),
397+
"unavailableReplicas": int64(0),
392398
},
393399
}},
394400
wantScaling: true,
395401
},
396402
{
397-
name: "should return true if spec and status updatedReplicas do not match",
403+
name: "should return true if status updatedReplicas is not set on control plane",
398404
obj: &unstructured.Unstructured{Object: map[string]interface{}{
399405
"spec": map[string]interface{}{
400406
"replicas": int64(2),
401407
},
402408
"status": map[string]interface{}{
403-
"replicas": int64(2),
404-
"updatedReplicas": int64(1),
405-
"readyReplicas": int64(2),
409+
"replicas": int64(2),
410+
"readyReplicas": int64(2),
411+
"unavailableReplicas": int64(0),
412+
},
413+
}},
414+
wantScaling: true,
415+
},
416+
{
417+
name: "should return true if spec replicas and status updatedReplicas do not match",
418+
obj: &unstructured.Unstructured{Object: map[string]interface{}{
419+
"spec": map[string]interface{}{
420+
"replicas": int64(2),
421+
},
422+
"status": map[string]interface{}{
423+
"replicas": int64(2),
424+
"updatedReplicas": int64(1),
425+
"readyReplicas": int64(2),
426+
"unavailableReplicas": int64(0),
406427
},
407428
}},
408429
wantScaling: true,
409430
},
410431
{
411-
name: "should return true if spec and status readyReplicas do not match",
432+
name: "should return true if status readyReplicas is not set on control plane",
433+
obj: &unstructured.Unstructured{Object: map[string]interface{}{
434+
"spec": map[string]interface{}{
435+
"replicas": int64(2),
436+
},
437+
"status": map[string]interface{}{
438+
"replicas": int64(2),
439+
"updatedReplicas": int64(2),
440+
"unavailableReplicas": int64(0),
441+
},
442+
}},
443+
wantScaling: true,
444+
},
445+
{
446+
name: "should return true if spec replicas and status readyReplicas do not match",
447+
obj: &unstructured.Unstructured{Object: map[string]interface{}{
448+
"spec": map[string]interface{}{
449+
"replicas": int64(2),
450+
},
451+
"status": map[string]interface{}{
452+
"replicas": int64(2),
453+
"updatedReplicas": int64(2),
454+
"readyReplicas": int64(1),
455+
"unavailableReplicas": int64(0),
456+
},
457+
}},
458+
wantScaling: true,
459+
},
460+
{
461+
name: "should return false if status unavailableReplicas is not set on control plane",
412462
obj: &unstructured.Unstructured{Object: map[string]interface{}{
413463
"spec": map[string]interface{}{
414464
"replicas": int64(2),
415465
},
416466
"status": map[string]interface{}{
417467
"replicas": int64(2),
418468
"updatedReplicas": int64(2),
419-
"readyReplicas": int64(1),
469+
"readyReplicas": int64(2),
470+
},
471+
}},
472+
wantScaling: false,
473+
},
474+
{
475+
name: "should return true if status unavailableReplicas is > 0",
476+
obj: &unstructured.Unstructured{Object: map[string]interface{}{
477+
"spec": map[string]interface{}{
478+
"replicas": int64(2),
479+
},
480+
"status": map[string]interface{}{
481+
"replicas": int64(2),
482+
"updatedReplicas": int64(2),
483+
"readyReplicas": int64(2),
484+
"unavailableReplicas": int64(1),
420485
},
421486
}},
422487
wantScaling: true,

‎internal/controllers/topology/cluster/desired_state_test.go‎

Lines changed: 51 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -611,7 +611,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
611611
defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()
612612

613613
// Note: the version used by the machine deployments does
614-
// not affect how we determining the control plane version.
614+
// not affect how we're determining the control plane version.
615615
// We only want to know if the machine deployments are stable.
616616
//
617617
// A machine deployment is considered stable if all the following are true:
@@ -706,10 +706,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
706706
"spec.replicas": int64(2),
707707
}).
708708
WithStatusFields(map[string]interface{}{
709-
"status.version": "v1.2.2",
710-
"status.replicas": int64(2),
711-
"status.updatedReplicas": int64(2),
712-
"status.readyReplicas": int64(2),
709+
"status.version": "v1.2.2",
710+
"status.replicas": int64(2),
711+
"status.updatedReplicas": int64(2),
712+
"status.readyReplicas": int64(2),
713+
"status.unavailableReplicas": int64(0),
713714
}).
714715
Build(),
715716
expectedVersion: "v1.2.3",
@@ -739,10 +740,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
739740
"spec.replicas": int64(2),
740741
}).
741742
WithStatusFields(map[string]interface{}{
742-
"status.version": "v1.2.2",
743-
"status.replicas": int64(1),
744-
"status.updatedReplicas": int64(1),
745-
"status.readyReplicas": int64(1),
743+
"status.version": "v1.2.2",
744+
"status.replicas": int64(1),
745+
"status.updatedReplicas": int64(1),
746+
"status.readyReplicas": int64(1),
747+
"status.unavailableReplicas": int64(0),
746748
}).
747749
Build(),
748750
expectedVersion: "v1.2.2",
@@ -756,10 +758,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
756758
"spec.replicas": int64(2),
757759
}).
758760
WithStatusFields(map[string]interface{}{
759-
"status.version": "v1.2.2",
760-
"status.replicas": int64(2),
761-
"status.updatedReplicas": int64(2),
762-
"status.readyReplicas": int64(2),
761+
"status.version": "v1.2.2",
762+
"status.replicas": int64(2),
763+
"status.updatedReplicas": int64(2),
764+
"status.readyReplicas": int64(2),
765+
"status.unavailableReplicas": int64(0),
763766
}).
764767
Build(),
765768
machineDeploymentsState: scope.MachineDeploymentsStateMap{
@@ -778,10 +781,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
778781
"spec.replicas": int64(2),
779782
}).
780783
WithStatusFields(map[string]interface{}{
781-
"status.version": "v1.2.2",
782-
"status.replicas": int64(2),
783-
"status.updatedReplicas": int64(2),
784-
"status.readyReplicas": int64(2),
784+
"status.version": "v1.2.2",
785+
"status.replicas": int64(2),
786+
"status.updatedReplicas": int64(2),
787+
"status.readyReplicas": int64(2),
788+
"status.unavailableReplicas": int64(0),
785789
}).
786790
Build(),
787791
machineDeploymentsState: scope.MachineDeploymentsStateMap{
@@ -800,10 +804,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
800804
"spec.replicas": int64(2),
801805
}).
802806
WithStatusFields(map[string]interface{}{
803-
"status.version": "v1.2.2",
804-
"status.replicas": int64(2),
805-
"status.updatedReplicas": int64(2),
806-
"status.readyReplicas": int64(2),
807+
"status.version": "v1.2.2",
808+
"status.replicas": int64(2),
809+
"status.updatedReplicas": int64(2),
810+
"status.readyReplicas": int64(2),
811+
"status.unavailableReplicas": int64(0),
807812
}).
808813
Build(),
809814
machineDeploymentsState: scope.MachineDeploymentsStateMap{
@@ -822,10 +827,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
822827
"spec.replicas": int64(2),
823828
}).
824829
WithStatusFields(map[string]interface{}{
825-
"status.version": "v1.2.2",
826-
"status.replicas": int64(2),
827-
"status.updatedReplicas": int64(2),
828-
"status.readyReplicas": int64(2),
830+
"status.version": "v1.2.2",
831+
"status.replicas": int64(2),
832+
"status.updatedReplicas": int64(2),
833+
"status.readyReplicas": int64(2),
834+
"status.unavailableReplicas": int64(0),
829835
}).
830836
Build(),
831837
machineDeploymentsState: scope.MachineDeploymentsStateMap{
@@ -1211,10 +1217,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
12111217
"spec.replicas": int64(2),
12121218
}).
12131219
WithStatusFields(map[string]interface{}{
1214-
"status.version": "v1.2.2",
1215-
"status.replicas": int64(2),
1216-
"status.updatedReplicas": int64(2),
1217-
"status.readyReplicas": int64(2),
1220+
"status.version": "v1.2.2",
1221+
"status.replicas": int64(2),
1222+
"status.updatedReplicas": int64(2),
1223+
"status.readyReplicas": int64(2),
1224+
"status.unavailableReplicas": int64(0),
12181225
}).
12191226
Build()
12201227

@@ -1697,10 +1704,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
16971704
"spec.replicas": int64(2),
16981705
}).
16991706
WithStatusFields(map[string]interface{}{
1700-
"status.version": "v1.2.2",
1701-
"status.replicas": int64(2),
1702-
"status.updatedReplicas": int64(2),
1703-
"status.readyReplicas": int64(2),
1707+
"status.version": "v1.2.2",
1708+
"status.replicas": int64(2),
1709+
"status.updatedReplicas": int64(2),
1710+
"status.readyReplicas": int64(2),
1711+
"status.unavailableReplicas": int64(0),
17041712
}).
17051713
Build()
17061714
controlPlaneStable123 := builder.ControlPlane("test1", "cp1").
@@ -1709,10 +1717,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
17091717
"spec.replicas": int64(2),
17101718
}).
17111719
WithStatusFields(map[string]interface{}{
1712-
"status.version": "v1.2.3",
1713-
"status.replicas": int64(2),
1714-
"status.updatedReplicas": int64(2),
1715-
"status.readyReplicas": int64(2),
1720+
"status.version": "v1.2.3",
1721+
"status.replicas": int64(2),
1722+
"status.updatedReplicas": int64(2),
1723+
"status.readyReplicas": int64(2),
1724+
"status.unavailableReplicas": int64(0),
17161725
}).
17171726
Build()
17181727
controlPlaneUpgrading := builder.ControlPlane("test1", "cp1").
@@ -1729,10 +1738,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
17291738
"spec.replicas": int64(2),
17301739
}).
17311740
WithStatusFields(map[string]interface{}{
1732-
"status.version": "v1.2.3",
1733-
"status.replicas": int64(1),
1734-
"status.updatedReplicas": int64(1),
1735-
"status.readyReplicas": int64(1),
1741+
"status.version": "v1.2.3",
1742+
"status.replicas": int64(1),
1743+
"status.updatedReplicas": int64(1),
1744+
"status.readyReplicas": int64(1),
1745+
"status.unavailableReplicas": int64(0),
17361746
}).
17371747
Build()
17381748
controlPlaneDesired := builder.ControlPlane("test1", "cp1").

0 commit comments

Comments
 (0)