Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🌱 ClusterClass & test/framework: consider replicas for control plane readiness #7914

Merged
merged 2 commits into from
Jan 16, 2023
Merged

🌱 ClusterClass & test/framework: consider replicas for control plane readiness #7914

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e499366
ClusterClass: IsScaling now considers unavailableReplicas
sbueringer Jan 12, 2023
6c5eb9c
test/framework: WaitForControlPlaneToBeReady now considers replicas
sbueringer Jan 12, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion internal/contract/controlplane.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
)
Expand Down Expand Up @@ -206,6 +207,7 @@ func (c *ControlPlaneContract) IsUpgrading(obj *unstructured.Unstructured) (bool
// - spec.replicas != status.replicas.
// - spec.replicas != status.updatedReplicas.
// - spec.replicas != status.readyReplicas.
// - status.unavailableReplicas > 0.
func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool, error) {
desiredReplicas, err := c.Replicas().Get(obj)
if err != nil {
Expand Down Expand Up @@ -245,9 +247,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 {
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
Expand Down
95 changes: 80 additions & 15 deletions internal/contract/controlplane_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand Down
92 changes: 51 additions & 41 deletions internal/controllers/topology/cluster/desired_state_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -611,7 +611,7 @@ func TestComputeControlPlaneVersion(t *testing.T) {
defer utilfeature.SetFeatureGateDuringTest(t, feature.Gates, feature.RuntimeSDK, true)()

// Note: the version used by the machine deployments does
// not affect how we determining the control plane version.
// not affect how we're determining the control plane version.
// We only want to know if the machine deployments are stable.
//
// A machine deployment is considered stable if all the following are true:
Expand Down Expand Up @@ -706,10 +706,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build(),
expectedVersion: "v1.2.3",
Expand Down Expand Up @@ -739,10 +740,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(1),
"status.updatedReplicas": int64(1),
"status.readyReplicas": int64(1),
"status.version": "v1.2.2",
"status.replicas": int64(1),
"status.updatedReplicas": int64(1),
"status.readyReplicas": int64(1),
"status.unavailableReplicas": int64(0),
}).
Build(),
expectedVersion: "v1.2.2",
Expand All @@ -756,10 +758,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build(),
machineDeploymentsState: scope.MachineDeploymentsStateMap{
Expand All @@ -778,10 +781,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build(),
machineDeploymentsState: scope.MachineDeploymentsStateMap{
Expand All @@ -800,10 +804,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build(),
machineDeploymentsState: scope.MachineDeploymentsStateMap{
Expand All @@ -822,10 +827,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build(),
machineDeploymentsState: scope.MachineDeploymentsStateMap{
Expand Down Expand Up @@ -1211,10 +1217,11 @@ func TestComputeControlPlaneVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build()

Expand Down Expand Up @@ -1697,10 +1704,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.2",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build()
controlPlaneStable123 := builder.ControlPlane("test1", "cp1").
Expand All @@ -1709,10 +1717,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.3",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.version": "v1.2.3",
"status.replicas": int64(2),
"status.updatedReplicas": int64(2),
"status.readyReplicas": int64(2),
"status.unavailableReplicas": int64(0),
}).
Build()
controlPlaneUpgrading := builder.ControlPlane("test1", "cp1").
Expand All @@ -1729,10 +1738,11 @@ func TestComputeMachineDeploymentVersion(t *testing.T) {
"spec.replicas": int64(2),
}).
WithStatusFields(map[string]interface{}{
"status.version": "v1.2.3",
"status.replicas": int64(1),
"status.updatedReplicas": int64(1),
"status.readyReplicas": int64(1),
"status.version": "v1.2.3",
"status.replicas": int64(1),
"status.updatedReplicas": int64(1),
"status.readyReplicas": int64(1),
"status.unavailableReplicas": int64(0),
}).
Build()
controlPlaneDesired := builder.ControlPlane("test1", "cp1").
Expand Down
Loading