ClusterClass: IsScaling now considers unavailableReplicas

Signed-off-by: Stefan Büringer [email protected]
kubernetes-sigs · Jan 13, 2023 · e499366 · e499366
1 parent b3665e0
commit e499366
Show file tree

Hide file tree

Showing 2 changed files with 102 additions and 16 deletions.
diff --git a/internal/contract/controlplane.go b/internal/contract/controlplane.go
@@ -22,6 +22,7 @@ import (
 	"github.com/blang/semver"
 	"github.com/pkg/errors"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"k8s.io/utils/pointer"
 
 	"sigs.k8s.io/cluster-api/util/version"
 )
@@ -245,9 +246,29 @@ func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool,
 		return false, errors.Wrap(err, "failed to get control plane status readyReplicas")
 	}
 
+	unavailableReplicas, err := c.UnavailableReplicas().Get(obj)
+	if err != nil && !errors.Is(err, errNotFound) {
+		if !errors.Is(err, errNotFound) {
+			return false, errors.Wrap(err, "failed to get control plane status unavailableReplicas")
+		}
+		// If unavailableReplicas is not set on the control plane we assume it is 0.
+		// We have to do this as the following happens after clusterctl move with KCP:
+		// * clusterctl move creates the KCP object without status
+		// * the KCP controller won't patch the field to 0 if it doesn't exist
+		//   * This is because the patchHelper marshals before/after object to JSON to calculate a diff
+		//     and as the unavailableReplicas field is not a pointer, not set and 0 are both rendered as 0.
+		//     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.
+		unavailableReplicas = pointer.Int64(0)
+	}
+
+	// Control plane is still scaling if:
+	// * .spec.replicas, .status.replicas, .status.updatedReplicas,
+	//   .status.readyReplicas are not equal and
+	// * unavailableReplicas > 0
 	if *statusReplicas != *desiredReplicas ||
 		*updatedReplicas != *desiredReplicas ||
-		*readyReplicas != *desiredReplicas {
+		*readyReplicas != *desiredReplicas ||
+		*unavailableReplicas > 0 {
 		return true, nil
 	}
 	return false, nil

diff --git a/internal/contract/controlplane_test.go b/internal/contract/controlplane_test.go
@@ -353,9 +353,10 @@ func TestControlPlaneIsScaling(t *testing.T) {
 					"replicas": int64(2),
 				},
 				"status": map[string]interface{}{
-					"replicas":        int64(2),
-					"updatedReplicas": int64(2),
-					"readyReplicas":   int64(2),
+					"replicas":            int64(2),
+					"updatedReplicas":     int64(2),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(0),
 				},
 			}},
 			wantScaling: false,
@@ -370,53 +371,117 @@ func TestControlPlaneIsScaling(t *testing.T) {
 			wantScaling: true,
 		},
 		{
-			name: "should return true if status.replicas is not set on control plane",
+			name: "should return true if status replicas is not set on control plane",
 			obj: &unstructured.Unstructured{Object: map[string]interface{}{
 				"spec": map[string]interface{}{
 					"replicas": int64(2),
 				},
-				"status": map[string]interface{}{},
+				"status": map[string]interface{}{
+					"updatedReplicas":     int64(2),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(0),
+				},
 			}},
 			wantScaling: true,
 		},
 		{
-			name: "should return true if spec and status replicas do not match",
+			name: "should return true if spec replicas and status replicas do not match",
 			obj: &unstructured.Unstructured{Object: map[string]interface{}{
 				"spec": map[string]interface{}{
 					"replicas": int64(2),
 				},
 				"status": map[string]interface{}{
-					"replicas":        int64(1),
-					"updatedReplicas": int64(2),
-					"readyReplicas":   int64(2),
+					"replicas":            int64(1),
+					"updatedReplicas":     int64(2),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(0),
 				},
 			}},
 			wantScaling: true,
 		},
 		{
-			name: "should return true if spec and status updatedReplicas do not match",
+			name: "should return true if status updatedReplicas is not set on control plane",
 			obj: &unstructured.Unstructured{Object: map[string]interface{}{
 				"spec": map[string]interface{}{
 					"replicas": int64(2),
 				},
 				"status": map[string]interface{}{
-					"replicas":        int64(2),
-					"updatedReplicas": int64(1),
-					"readyReplicas":   int64(2),
+					"replicas":            int64(2),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(0),
+				},
+			}},
+			wantScaling: true,
+		},
+		{
+			name: "should return true if spec replicas and status updatedReplicas do not match",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"replicas": int64(2),
+				},
+				"status": map[string]interface{}{
+					"replicas":            int64(2),
+					"updatedReplicas":     int64(1),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(0),
 				},
 			}},
 			wantScaling: true,
 		},
 		{
-			name: "should return true if spec and status readyReplicas do not match",
+			name: "should return true if status readyReplicas is not set on control plane",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"replicas": int64(2),
+				},
+				"status": map[string]interface{}{
+					"replicas":            int64(2),
+					"updatedReplicas":     int64(2),
+					"unavailableReplicas": int64(0),
+				},
+			}},
+			wantScaling: true,
+		},
+		{
+			name: "should return true if spec replicas and status readyReplicas do not match",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"replicas": int64(2),
+				},
+				"status": map[string]interface{}{
+					"replicas":            int64(2),
+					"updatedReplicas":     int64(2),
+					"readyReplicas":       int64(1),
+					"unavailableReplicas": int64(0),
+				},
+			}},
+			wantScaling: true,
+		},
+		{
+			name: "should return false if status unavailableReplicas is not set on control plane",
 			obj: &unstructured.Unstructured{Object: map[string]interface{}{
 				"spec": map[string]interface{}{
 					"replicas": int64(2),
 				},
 				"status": map[string]interface{}{
 					"replicas":        int64(2),
 					"updatedReplicas": int64(2),
-					"readyReplicas":   int64(1),
+					"readyReplicas":   int64(2),
+				},
+			}},
+			wantScaling: false,
+		},
+		{
+			name: "should return true if status unavailableReplicas is > 0",
+			obj: &unstructured.Unstructured{Object: map[string]interface{}{
+				"spec": map[string]interface{}{
+					"replicas": int64(2),
+				},
+				"status": map[string]interface{}{
+					"replicas":            int64(2),
+					"updatedReplicas":     int64(2),
+					"readyReplicas":       int64(2),
+					"unavailableReplicas": int64(1),
 				},
 			}},
 			wantScaling: true,