Merge pull request #29935 from janetkuo/deployment-scale-rollout-test

Automatic merge from submit-queue

Deployment status validity should be checked in scaled rollout e2e test

Follow up #29889 

@kubernetes/deployment

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.kubernetes.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.kubernetes.io/reviews/kubernetes/kubernetes/29935)
<!-- Reviewable:end -->

test/e2e/deployment.go

@@ -1042,8 +1042,14 @@ func testScaledRolloutDeployment(f *framework.Framework) {
 	deployment, err = c.Extensions().Deployments(ns).Get(deploymentName)
 	Expect(err).NotTo(HaveOccurred())
 
-	By(fmt.Sprintf("Waiting for deployment status to sync (current available: %d, minimum available: %d)", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment)))
+	// Verify that the required pods have come up.
-	err = framework.WaitForDeploymentStatusValid(c, deployment, true)
+	By("Waiting for all required pods to come up")
+	err = framework.VerifyPods(f.Client, ns, nginxImageName, false, deployment.Spec.Replicas)
+	if err != nil {
+		framework.Logf("error in waiting for pods to come up: %s", err)
+		Expect(err).NotTo(HaveOccurred())
+	}
+	err = framework.WaitForDeploymentStatus(c, deployment)
 	Expect(err).NotTo(HaveOccurred())
 
 	first, err := deploymentutil.GetNewReplicaSet(deployment, c)
@@ -1063,9 +1069,9 @@ func testScaledRolloutDeployment(f *framework.Framework) {
 	deployment, err = c.Extensions().Deployments(ns).Get(deploymentName)
 	Expect(err).NotTo(HaveOccurred())
 
-	By(fmt.Sprintf("Waiting for deployment status to sync (current available: %d, minimum available: %d)", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment)))
+	if deployment.Status.AvailableReplicas < deploymentutil.MinAvailable(deployment) {
-	err = framework.WaitForDeploymentStatusValid(c, deployment, false)
+		Expect(fmt.Errorf("Observed %d available replicas, less than min required %d", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment))).NotTo(HaveOccurred())
-	Expect(err).NotTo(HaveOccurred())
+	}
 
 	By(fmt.Sprintf("Checking that the replica sets for %q are synced", deploymentName))
 	second, err := deploymentutil.GetNewReplicaSet(deployment, c)
@@ -1105,7 +1111,9 @@ func testScaledRolloutDeployment(f *framework.Framework) {
 	}
 
 	By(fmt.Sprintf("Waiting for deployment status to sync (current available: %d, minimum available: %d)", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment)))
-	err = framework.WaitForDeploymentStatusValid(c, deployment, true)
+	err = framework.WaitForDeploymentStatusValid(c, deployment)
+	Expect(err).NotTo(HaveOccurred())
+	err = framework.WaitForDeploymentStatus(c, deployment)
 	Expect(err).NotTo(HaveOccurred())
 
 	// Update the deployment with a non-existent image so that the new replica set will be blocked.
@@ -1122,9 +1130,9 @@ func testScaledRolloutDeployment(f *framework.Framework) {
 	deployment, err = c.Extensions().Deployments(ns).Get(deploymentName)
 	Expect(err).NotTo(HaveOccurred())
 
-	By(fmt.Sprintf("Waiting for deployment status to sync (current available: %d, minimum available: %d)", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment)))
+	if deployment.Status.AvailableReplicas < deploymentutil.MinAvailable(deployment) {
-	err = framework.WaitForDeploymentStatusValid(c, deployment, false)
+		Expect(fmt.Errorf("Observed %d available replicas, less than min required %d", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment))).NotTo(HaveOccurred())
-	Expect(err).NotTo(HaveOccurred())
+	}
 
 	By(fmt.Sprintf("Checking that the replica sets for %q are synced", deploymentName))
 	oldRs, err := c.Extensions().ReplicaSets(rs.Namespace).Get(rs.Name)
@@ -1164,6 +1172,8 @@ func testScaledRolloutDeployment(f *framework.Framework) {
 	}
 
 	By(fmt.Sprintf("Waiting for deployment status to sync (current available: %d, minimum available: %d)", deployment.Status.AvailableReplicas, deploymentutil.MinAvailable(deployment)))
-	err = framework.WaitForDeploymentStatusValid(c, deployment, true)
+	err = framework.WaitForDeploymentStatusValid(c, deployment)
+	Expect(err).NotTo(HaveOccurred())
+	err = framework.WaitForDeploymentStatus(c, deployment)
 	Expect(err).NotTo(HaveOccurred())
 }

test/e2e/framework/util.go

@@ -3200,9 +3200,10 @@ func waitForReplicaSetPodsGone(c *client.Client, rs *extensions.ReplicaSet) erro
 	})
 }
 
-// Waits for the deployment status to sync (i.e. max unavailable and max surge aren't violated anymore).
+// Waits for the deployment status to become valid (i.e. max unavailable and max surge aren't violated anymore).
-// If expectComplete, wait until all its replicas become up-to-date.
+// Note that the status should stay valid at all times unless shortly after a scaling event or the deployment is just created.
-func WaitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deployment, expectComplete bool) error {
+// To verify that the deployment status is valid and wait for the rollout to finish, use WaitForDeploymentStatus instead.
+func WaitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deployment) error {
 	var (
 		oldRSs, allOldRSs, allRSs []*extensions.ReplicaSet
 		newRS                     *extensions.ReplicaSet
@@ -3252,20 +3253,7 @@ func WaitForDeploymentStatusValid(c clientset.Interface, d *extensions.Deploymen
 			Logf(reason)
 			return false, nil
 		}
-
+		return true, nil
-		if !expectComplete {
-			return true, nil
-		}
-
-		// When the deployment status and its underlying resources reach the desired state, we're done
-		if deployment.Status.Replicas == deployment.Spec.Replicas &&
-			deployment.Status.UpdatedReplicas == deployment.Spec.Replicas &&
-			deploymentutil.GetReplicaCountForReplicaSets(oldRSs) == 0 &&
-			deploymentutil.GetReplicaCountForReplicaSets([]*extensions.ReplicaSet{newRS}) == deployment.Spec.Replicas {
-			return true, nil
-		}
-		reason = fmt.Sprintf("deployment %q is yet to complete: %#v", deployment.Name, deployment.Status)
-		return false, nil
 	})
 
 	if err == wait.ErrWaitTimeout {