Merge pull request #21550 from mwielgus/hpa-pending

Don't include pending pods in metrics calculations

pkg/controller/podautoscaler/metrics/metrics_client.go

@@ -123,6 +123,11 @@ func (h *HeapsterMetricsClient) GetCpuConsumptionAndRequestInMillis(namespace st
 	requestSum := int64(0)
 	missing := false
 	for _, pod := range podList.Items {
+		if pod.Status.Phase == api.PodPending {
+			// Skip pending pods.
+			continue
+		}
+
 		podNames = append(podNames, pod.Name)
 		for _, container := range pod.Spec.Containers {
 			containerRequest := container.Resources.Requests[api.ResourceCPU]
@@ -133,6 +138,9 @@ func (h *HeapsterMetricsClient) GetCpuConsumptionAndRequestInMillis(namespace st
 			}
 		}
 	}
+	if len(podNames) == 0 && len(podList.Items) > 0 {
+		return 0, 0, time.Time{}, fmt.Errorf("no running pods")
+	}
 	if missing || requestSum == 0 {
 		return 0, 0, time.Time{}, fmt.Errorf("some pods do not have request for cpu")
 	}
@@ -159,8 +167,15 @@ func (h *HeapsterMetricsClient) GetCustomMetric(customMetricName string, namespa
 	}
 	podNames := []string{}
 	for _, pod := range podList.Items {
+		if pod.Status.Phase == api.PodPending {
+			// Skip pending pods.
+			continue
+		}
 		podNames = append(podNames, pod.Name)
 	}
+	if len(podNames) == 0 && len(podList.Items) > 0 {
+		return nil, time.Time{}, fmt.Errorf("no running pods")
+	}
 
 	value, timestamp, err := h.getForPods(metricSpec, namespace, podNames)
 	if err != nil {

pkg/controller/podautoscaler/metrics/metrics_client_test.go

@@ -68,6 +68,7 @@ type testCase struct {
 	targetTimestamp       int
 	reportedMetricsPoints [][]metricPoint
 	namespace             string
+	podListOverride       *api.PodList
 	selector              map[string]string
 }
 
@@ -81,30 +82,13 @@ func (tc *testCase) prepareTestClient(t *testing.T) *fake.Clientset {
 	fakeClient := &fake.Clientset{}
 
 	fakeClient.AddReactor("list", "pods", func(action core.Action) (handled bool, ret runtime.Object, err error) {
+		if tc.podListOverride != nil {
+			return true, tc.podListOverride, nil
+		}
 		obj := &api.PodList{}
 		for i := 0; i < tc.replicas; i++ {
 			podName := fmt.Sprintf("%s-%d", podNamePrefix, i)
-			pod := api.Pod{
-				ObjectMeta: api.ObjectMeta{
-					Name:      podName,
-					Namespace: namespace,
-					Labels:    selector,
-				},
-				Spec: api.PodSpec{
-					Containers: []api.Container{
-						{
-							Resources: api.ResourceRequirements{
-								Requests: api.ResourceList{
-									api.ResourceCPU: resource.MustParse("10"),
-								},
-							},
-						},
-					},
-				},
-				Status: api.PodStatus{
-					Phase: api.PodRunning,
-				},
-			}
+			pod := buildPod(namespace, podName, selector, api.PodRunning)
 			obj.Items = append(obj.Items, pod)
 		}
 		return true, obj, nil
@@ -136,6 +120,30 @@ func (tc *testCase) prepareTestClient(t *testing.T) *fake.Clientset {
 	return fakeClient
 }
 
+func buildPod(namespace, podName string, selector map[string]string, phase api.PodPhase) api.Pod {
+	return api.Pod{
+		ObjectMeta: api.ObjectMeta{
+			Name:      podName,
+			Namespace: namespace,
+			Labels:    selector,
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Resources: api.ResourceRequirements{
+						Requests: api.ResourceList{
+							api.ResourceCPU: resource.MustParse("10"),
+						},
+					},
+				},
+			},
+		},
+		Status: api.PodStatus{
+			Phase: phase,
+		},
+	}
+}
+
 func (tc *testCase) verifyResults(t *testing.T, val *float64, timestamp time.Time, err error) {
 	assert.Equal(t, tc.desiredError, err)
 	if tc.desiredError != nil {
@@ -173,6 +181,50 @@ func TestCPU(t *testing.T) {
 	tc.runTest(t)
 }
 
+func TestCPUPending(t *testing.T) {
+	tc := testCase{
+		replicas:              4,
+		desiredValue:          5000,
+		targetResource:        "cpu-usage",
+		targetTimestamp:       1,
+		reportedMetricsPoints: [][]metricPoint{{{5000, 1}}, {{5000, 1}}, {{5000, 1}}},
+		podListOverride:       &api.PodList{},
+	}
+
+	namespace := "test-namespace"
+	podNamePrefix := "test-pod"
+	selector := map[string]string{"name": podNamePrefix}
+	for i := 0; i < tc.replicas; i++ {
+		podName := fmt.Sprintf("%s-%d", podNamePrefix, i)
+		pod := buildPod(namespace, podName, selector, api.PodRunning)
+		tc.podListOverride.Items = append(tc.podListOverride.Items, pod)
+	}
+	tc.podListOverride.Items[0].Status.Phase = api.PodPending
+
+	tc.runTest(t)
+}
+
+func TestCPUAllPending(t *testing.T) {
+	tc := testCase{
+		replicas:              4,
+		targetResource:        "cpu-usage",
+		targetTimestamp:       1,
+		reportedMetricsPoints: [][]metricPoint{},
+		podListOverride:       &api.PodList{},
+		desiredError:          fmt.Errorf("no running pods"),
+	}
+
+	namespace := "test-namespace"
+	podNamePrefix := "test-pod"
+	selector := map[string]string{"name": podNamePrefix}
+	for i := 0; i < tc.replicas; i++ {
+		podName := fmt.Sprintf("%s-%d", podNamePrefix, i)
+		pod := buildPod(namespace, podName, selector, api.PodPending)
+		tc.podListOverride.Items = append(tc.podListOverride.Items, pod)
+	}
+	tc.runTest(t)
+}
+
 func TestQPS(t *testing.T) {
 	tc := testCase{
 		replicas:              3,
@@ -184,6 +236,50 @@ func TestQPS(t *testing.T) {
 	tc.runTest(t)
 }
 
+func TestQPSPending(t *testing.T) {
+	tc := testCase{
+		replicas:              4,
+		desiredValue:          13.33333,
+		targetResource:        "qps",
+		targetTimestamp:       1,
+		reportedMetricsPoints: [][]metricPoint{{{10, 1}}, {{20, 1}}, {{10, 1}}},
+		podListOverride:       &api.PodList{},
+	}
+
+	namespace := "test-namespace"
+	podNamePrefix := "test-pod"
+	selector := map[string]string{"name": podNamePrefix}
+	for i := 0; i < tc.replicas; i++ {
+		podName := fmt.Sprintf("%s-%d", podNamePrefix, i)
+		pod := buildPod(namespace, podName, selector, api.PodRunning)
+		tc.podListOverride.Items = append(tc.podListOverride.Items, pod)
+	}
+	tc.podListOverride.Items[0].Status.Phase = api.PodPending
+	tc.runTest(t)
+}
+
+func TestQPSAllPending(t *testing.T) {
+	tc := testCase{
+		replicas:              4,
+		desiredError:          fmt.Errorf("no running pods"),
+		targetResource:        "qps",
+		targetTimestamp:       1,
+		reportedMetricsPoints: [][]metricPoint{},
+		podListOverride:       &api.PodList{},
+	}
+
+	namespace := "test-namespace"
+	podNamePrefix := "test-pod"
+	selector := map[string]string{"name": podNamePrefix}
+	for i := 0; i < tc.replicas; i++ {
+		podName := fmt.Sprintf("%s-%d", podNamePrefix, i)
+		pod := buildPod(namespace, podName, selector, api.PodPending)
+		tc.podListOverride.Items = append(tc.podListOverride.Items, pod)
+	}
+	tc.podListOverride.Items[0].Status.Phase = api.PodPending
+	tc.runTest(t)
+}
+
 func TestCPUSumEqualZero(t *testing.T) {
 	tc := testCase{
 		replicas:              3,