From 6c5eb9c7989b392f45496cfdf3d08a5bdd348438 Mon Sep 17 00:00:00 2001 From: Stefan Bueringer Date: Thu, 12 Jan 2023 19:24:36 +0100 Subject: [PATCH] test/framework: WaitForControlPlaneToBeReady now considers replicas MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Stefan Büringer buringerst@vmware.com --- internal/contract/controlplane.go | 3 +- .../topology/cluster/desired_state_test.go | 92 ++++++++++--------- .../topology/cluster/reconcile_state_test.go | 36 ++++---- test/e2e/clusterctl_upgrade.go | 1 + test/e2e/self_hosted.go | 9 +- test/framework/controlplane_helpers.go | 31 +++++-- 6 files changed, 104 insertions(+), 68 deletions(-) diff --git a/internal/contract/controlplane.go b/internal/contract/controlplane.go index 18c957d62a58..3148477af309 100644 --- a/internal/contract/controlplane.go +++ b/internal/contract/controlplane.go @@ -207,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 { @@ -247,7 +248,7 @@ func (c *ControlPlaneContract) IsScaling(obj *unstructured.Unstructured) (bool, } unavailableReplicas, err := c.UnavailableReplicas().Get(obj) - if err != nil && !errors.Is(err, errNotFound) { + if err != nil { if !errors.Is(err, errNotFound) { return false, errors.Wrap(err, "failed to get control plane status unavailableReplicas") } 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.