diff --git a/internal/contract/controlplane.go b/internal/contract/controlplane.go index 788e0e47610e..3148477af309 100644 --- 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" ) @@ -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 { @@ -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 diff --git a/internal/contract/controlplane_test.go b/internal/contract/controlplane_test.go index 4080cb837e5b..98863ca310ca 100644 --- 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,45 +371,94 @@ 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), @@ -416,7 +466,22 @@ func TestControlPlaneIsScaling(t *testing.T) { "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, diff --git a/internal/controllers/topology/cluster/desired_state_test.go b/internal/controllers/topology/cluster/desired_state_test.go index 8e66bd85cf05..bee52479d171 100644 --- a/internal/controllers/topology/cluster/desired_state_test.go +++ b/internal/controllers/topology/cluster/desired_state_test.go @@ -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: @@ -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", @@ -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", @@ -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{ @@ -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{ @@ -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{ @@ -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{ @@ -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() @@ -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"). @@ -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"). @@ -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"). diff --git a/internal/controllers/topology/cluster/reconcile_state_test.go b/internal/controllers/topology/cluster/reconcile_state_test.go index 33d6ff49942e..8036e46abe59 100644 --- a/internal/controllers/topology/cluster/reconcile_state_test.go +++ b/internal/controllers/topology/cluster/reconcile_state_test.go @@ -498,10 +498,11 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { "spec.replicas": int64(2), }). WithStatusFields(map[string]interface{}{ - "status.version": topologyVersion, - "status.replicas": int64(2), - "status.updatedReplicas": int64(2), - "status.readyReplicas": int64(2), + "status.version": topologyVersion, + "status.replicas": int64(2), + "status.updatedReplicas": int64(2), + "status.readyReplicas": int64(2), + "status.unavailableReplicas": int64(0), }). Build() controlPlaneStableAtLowerVersion := builder.ControlPlane("test1", "cp1"). @@ -510,10 +511,11 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { "spec.replicas": int64(2), }). WithStatusFields(map[string]interface{}{ - "status.version": lowerVersion, - "status.replicas": int64(2), - "status.updatedReplicas": int64(2), - "status.readyReplicas": int64(2), + "status.version": lowerVersion, + "status.replicas": int64(2), + "status.updatedReplicas": int64(2), + "status.readyReplicas": int64(2), + "status.unavailableReplicas": int64(0), }). Build() controlPlaneUpgrading := builder.ControlPlane("test1", "cp1"). @@ -522,10 +524,11 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { "spec.replicas": int64(2), }). WithStatusFields(map[string]interface{}{ - "status.version": lowerVersion, - "status.replicas": int64(2), - "status.updatedReplicas": int64(2), - "status.readyReplicas": int64(2), + "status.version": lowerVersion, + "status.replicas": int64(2), + "status.updatedReplicas": int64(2), + "status.readyReplicas": int64(2), + "status.unavailableReplicas": int64(0), }). Build() controlPlaneScaling := builder.ControlPlane("test1", "cp1"). @@ -534,10 +537,11 @@ func TestReconcile_callAfterClusterUpgrade(t *testing.T) { "spec.replicas": int64(2), }). WithStatusFields(map[string]interface{}{ - "status.version": topologyVersion, - "status.replicas": int64(1), - "status.updatedReplicas": int64(1), - "status.readyReplicas": int64(1), + "status.version": topologyVersion, + "status.replicas": int64(1), + "status.updatedReplicas": int64(1), + "status.readyReplicas": int64(1), + "status.unavailableReplicas": int64(0), }). Build() diff --git a/test/e2e/clusterctl_upgrade.go b/test/e2e/clusterctl_upgrade.go index 8e85e336b937..981e294467ea 100644 --- a/test/e2e/clusterctl_upgrade.go +++ b/test/e2e/clusterctl_upgrade.go @@ -436,6 +436,7 @@ func ClusterctlUpgradeSpec(ctx context.Context, inputGetter func() ClusterctlUpg } // After the upgrade check that there were no unexpected rollouts. + log.Logf("Verify there are no unexpected rollouts") Consistently(func() bool { postUpgradeMachineList := &unstructured.UnstructuredList{} postUpgradeMachineList.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("MachineList")) diff --git a/test/e2e/self_hosted.go b/test/e2e/self_hosted.go index beec7415a1b9..724980669efc 100644 --- a/test/e2e/self_hosted.go +++ b/test/e2e/self_hosted.go @@ -261,6 +261,7 @@ func SelfHostedSpec(ctx context.Context, inputGetter func() SelfHostedSpecInput) Expect(controlPlane).ToNot(BeNil()) // After the move check that there were no unexpected rollouts. + log.Logf("Verify there are no unexpected rollouts") Consistently(func() bool { postMoveMachineList := &unstructured.UnstructuredList{} postMoveMachineList.SetGroupVersionKind(clusterv1.GroupVersion.WithKind("MachineList")) @@ -279,8 +280,14 @@ func SelfHostedSpec(ctx context.Context, inputGetter func() SelfHostedSpecInput) return } - By("Upgrading the self-hosted Cluster") + log.Logf("Waiting for control plane to be ready") + framework.WaitForControlPlaneAndMachinesReady(ctx, framework.WaitForControlPlaneAndMachinesReadyInput{ + GetLister: selfHostedClusterProxy.GetClient(), + Cluster: clusterResources.Cluster, + ControlPlane: clusterResources.ControlPlane, + }, input.E2EConfig.GetIntervals(specName, "wait-control-plane")...) + By("Upgrading the self-hosted Cluster") if clusterResources.Cluster.Spec.Topology != nil { // Cluster is using ClusterClass, upgrade via topology. By("Upgrading the Cluster topology") diff --git a/test/framework/controlplane_helpers.go b/test/framework/controlplane_helpers.go index d5545c6c964e..4f5e37cc9de9 100644 --- a/test/framework/controlplane_helpers.go +++ b/test/framework/controlplane_helpers.go @@ -22,7 +22,6 @@ import ( . "github.com/onsi/ginkgo/v2" . "github.com/onsi/gomega" - . "github.com/onsi/gomega/gstruct" "github.com/pkg/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -164,20 +163,34 @@ type WaitForControlPlaneToBeReadyInput struct { func WaitForControlPlaneToBeReady(ctx context.Context, input WaitForControlPlaneToBeReadyInput, intervals ...interface{}) { By("Waiting for the control plane to be ready") controlplane := &controlplanev1.KubeadmControlPlane{} - Eventually(func() (controlplanev1.KubeadmControlPlane, error) { + Eventually(func() (bool, error) { key := client.ObjectKey{ Namespace: input.ControlPlane.GetNamespace(), Name: input.ControlPlane.GetName(), } if err := input.Getter.Get(ctx, key, controlplane); err != nil { - return *controlplane, errors.Wrapf(err, "failed to get KCP") + return false, errors.Wrapf(err, "failed to get KCP") + } + + desiredReplicas := controlplane.Spec.Replicas + statusReplicas := controlplane.Status.Replicas + updatedReplicas := controlplane.Status.UpdatedReplicas + readyReplicas := controlplane.Status.ReadyReplicas + unavailableReplicas := controlplane.Status.UnavailableReplicas + + // Control plane is still rolling out (and thus not ready) if: + // * .spec.replicas, .status.replicas, .status.updatedReplicas, + // .status.readyReplicas are not equal and + // * unavailableReplicas > 0 + if statusReplicas != *desiredReplicas || + updatedReplicas != *desiredReplicas || + readyReplicas != *desiredReplicas || + unavailableReplicas > 0 { + return false, nil } - return *controlplane, nil - }, intervals...).Should(MatchFields(IgnoreExtras, Fields{ - "Status": MatchFields(IgnoreExtras, Fields{ - "Ready": BeTrue(), - }), - }), PrettyPrint(controlplane)+"\n") + + return true, nil + }, intervals...).Should(BeTrue(), PrettyPrint(controlplane)+"\n") } // AssertControlPlaneFailureDomainsInput is the input for AssertControlPlaneFailureDomains.