fix review comments

kubernetes-sigs · Mar 28, 2023 · 206d5d4 · 206d5d4
1 parent 695f01b
commit 206d5d4
Show file tree

Hide file tree

Showing 2 changed files with 24 additions and 26 deletions.
diff --git a/test/integration/podgroup_controller_test.go b/test/integration/podgroup_controller_test.go
@@ -127,12 +127,12 @@ func TestPodGroupController(t *testing.T) {
 	// TODO: Update the number of scheduled pods when changing the Reconcile logic.
 	// PostBind is not running in this test, so the number of Scheduled pods in PodGroup is 0.
 	for _, tt := range []struct {
-		name         string
-		podGroups    []*v1alpha1.PodGroup
-		existingPods []*v1.Pod
-		status       []*v1alpha1.PodGroup
-		incomingPods []*v1.Pod
-		want         []*v1alpha1.PodGroup
+		name                string
+		podGroups           []*v1alpha1.PodGroup
+		existingPods        []*v1.Pod
+		intermediatePGState []*v1alpha1.PodGroup
+		incomingPods        []*v1.Pod
+		expectedPGState     []*v1alpha1.PodGroup
 	}{
 		{
 			name: "Statuses of all pods change from pending to running",
@@ -147,7 +147,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Obj(),
 			},
-			status: []*v1alpha1.PodGroup{
+			intermediatePGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "PreScheduling", "", 0, 0, 0, 0),
 			},
 			incomingPods: []*v1.Pod{
@@ -158,7 +158,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodRunning).Obj(),
 			},
-			want: []*v1alpha1.PodGroup{
+			expectedPGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Running", "", 0, 3, 0, 0),
 			},
 		},
@@ -175,7 +175,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodRunning).Obj(),
 			},
-			status: []*v1alpha1.PodGroup{
+			intermediatePGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Running", "", 0, 3, 0, 0),
 			},
 			incomingPods: []*v1.Pod{
@@ -186,7 +186,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodSucceeded).Obj(),
 			},
-			want: []*v1alpha1.PodGroup{
+			expectedPGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Finished", "", 0, 0, 3, 0),
 			},
 		},
@@ -203,14 +203,14 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodRunning).Obj(),
 			},
-			status: []*v1alpha1.PodGroup{
+			intermediatePGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Running", "", 0, 3, 0, 0),
 			},
 			incomingPods: []*v1.Pod{
 				st.MakePod().Namespace(ns).Name("t1-p1-1").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodSucceeded).Obj(),
 			},
-			want: []*v1alpha1.PodGroup{
+			expectedPGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Running", "", 0, 2, 1, 0),
 			},
 		},
@@ -227,14 +227,14 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodRunning).Obj(),
 			},
-			status: []*v1alpha1.PodGroup{
+			intermediatePGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Running", "", 0, 3, 0, 0),
 			},
 			incomingPods: []*v1.Pod{
 				st.MakePod().Namespace(ns).Name("t1-p1-1").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Label(v1alpha1.PodGroupLabel, "pg1-1").Node(nodeName).Phase(v1.PodFailed).Obj(),
 			},
-			want: []*v1alpha1.PodGroup{
+			expectedPGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Failed", "", 0, 2, 0, 1),
 			},
 		},
@@ -251,7 +251,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Node(nodeName).Obj(),
 			},
-			status: []*v1alpha1.PodGroup{
+			intermediatePGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Pending", "", 0, 0, 0, 0),
 			},
 			incomingPods: []*v1.Pod{
@@ -262,7 +262,7 @@ func TestPodGroupController(t *testing.T) {
 				st.MakePod().Namespace(ns).Name("t1-p1-3").Req(map[v1.ResourceName]string{v1.ResourceMemory: "50"}).Priority(
 					midPriority).Node(nodeName).Phase(v1.PodRunning).Obj(),
 			},
-			want: []*v1alpha1.PodGroup{
+			expectedPGState: []*v1alpha1.PodGroup{
 				util.UpdatePGStatus(util.MakePG("pg1-1", ns, 3, nil, nil), "Pending", "", 0, 0, 0, 0),
 			},
 		},
@@ -279,13 +279,11 @@ func TestPodGroupController(t *testing.T) {
 			// create Pods
 			for _, pod := range tt.existingPods {
 				klog.InfoS("Creating pod ", "podName", pod.Name)
-				_, err := cs.CoreV1().Pods(pod.Namespace).Create(testCtx.Ctx, pod, metav1.CreateOptions{})
-				if err != nil {
+				if _, err := cs.CoreV1().Pods(pod.Namespace).Create(testCtx.Ctx, pod, metav1.CreateOptions{}); err != nil {
 					t.Fatalf("Failed to create Pod %q: %v", pod.Name, err)
 				}
 				if pod.Status.Phase == v1.PodRunning {
-					_, err = cs.CoreV1().Pods(pod.Namespace).UpdateStatus(testCtx.Ctx, pod, metav1.UpdateOptions{})
-					if err != nil {
+					if _, err := cs.CoreV1().Pods(pod.Namespace).UpdateStatus(testCtx.Ctx, pod, metav1.UpdateOptions{}); err != nil {
 						t.Fatalf("Failed to update Pod status %q: %v", pod.Name, err)
 					}
 				}
@@ -302,7 +300,7 @@ func TestPodGroupController(t *testing.T) {
 			}
 
 			if err := wait.Poll(time.Millisecond*200, 10*time.Second, func() (bool, error) {
-				for _, v := range tt.status {
+				for _, v := range tt.intermediatePGState {
 					pg, err := extClient.SchedulingV1alpha1().PodGroups(v.Namespace).Get(testCtx.Ctx, v.Name, metav1.GetOptions{})
 					if err != nil {
 						// This could be a connection error so we want to retry.
@@ -326,18 +324,19 @@ func TestPodGroupController(t *testing.T) {
 				}
 			}
 			if err := wait.Poll(time.Millisecond*200, 10*time.Second, func() (bool, error) {
+				// wait for all incomingPods to be scheduled
 				for _, pod := range tt.incomingPods {
 					if !podScheduled(cs, pod.Namespace, pod.Name) {
 						return false, nil
 					}
 				}
 				return true, nil
 			}); err != nil {
-				t.Fatalf("%v Waiting nextPods update status error: %v", tt.name, err.Error())
+				t.Fatalf("%v Waiting incomingPods scheduled error: %v", tt.name, err.Error())
 			}
 
 			if err := wait.Poll(time.Millisecond*200, 10*time.Second, func() (bool, error) {
-				for _, v := range tt.want {
+				for _, v := range tt.expectedPGState {
 					pg, err := extClient.SchedulingV1alpha1().PodGroups(v.Namespace).Get(testCtx.Ctx, v.Name, metav1.GetOptions{})
 					if err != nil {
 						// This could be a connection error so we want to retry.
@@ -352,7 +351,7 @@ func TestPodGroupController(t *testing.T) {
 				}
 				return true, nil
 			}); err != nil {
-				t.Fatalf("%v Waiting next PodGroup error: %v", tt.name, err.Error())
+				t.Fatalf("%v Waiting PodGroup status update error: %v", tt.name, err.Error())
 			}
 			t.Logf("Case %v finished", tt.name)
 		})

diff --git a/test/util/utils.go b/test/util/utils.go
@@ -90,8 +90,7 @@ func MakePG(name, namespace string, min int32, creationTime *time.Time, minResou
 	return pg
 }
 
-func UpdatePGStatus(pg *v1alpha1.PodGroup, phase v1alpha1.PodGroupPhase, occupiedBy string, scheduled int32, running int32, succeeded int32,
-	failed int32) *v1alpha1.PodGroup {
+func UpdatePGStatus(pg *v1alpha1.PodGroup, phase v1alpha1.PodGroupPhase, occupiedBy string, scheduled int32, running int32, succeeded int32, failed int32) *v1alpha1.PodGroup {
 	pg.Status = v1alpha1.PodGroupStatus{
 		Phase:      phase,
 		OccupiedBy: occupiedBy,