Merge pull request #28 from appuio/fix/ignore-non-running-pods

Only consider running pods when computing ratio
appuio · May 24, 2022 · 5ef8b94 · 5ef8b94
2 parents b871994 + 3682317
commit 5ef8b94
Show file tree

Hide file tree

Showing 6 changed files with 164 additions and 95 deletions.
diff --git a/controllers/ratio_controller_test.go b/controllers/ratio_controller_test.go
@@ -192,4 +192,7 @@ var testPod = &corev1.Pod{
 		Name:      "pod",
 		Namespace: "foo",
 	},
+	Status: corev1.PodStatus{
+		Phase: corev1.PodRunning,
+	},
 }
diff --git a/ratio/fetcher_test.go b/ratio/fetcher_test.go
@@ -7,6 +7,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	corev1 "k8s.io/api/core/v1"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/resource"
 	utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -17,6 +18,24 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client/fake"
 )
 
+var (
+	fooPod = podFromResources("foo1", "foo", podResource{
+		containers: []containerResources{
+			{cpu: "1", memory: "1Gi"},
+			{cpu: "2", memory: "1Gi"},
+		},
+		phase: corev1.PodRunning,
+	})
+	foo2Pod = podFromResources("foo2", "foo", podResource{
+		containers: []containerResources{{memory: "1Gi"}},
+		phase:      corev1.PodRunning,
+	})
+	foobarPod = podFromResources("foo", "bar", podResource{
+		containers: []containerResources{{memory: "1337Gi"}},
+		phase:      corev1.PodRunning,
+	})
+)
+
 func TestRatioValidator_Handle(t *testing.T) {
 	ctx := context.Background()
 	tests := map[string]struct {
@@ -36,16 +55,9 @@ func TestRatioValidator_Handle(t *testing.T) {
 		"Fetch_Namespace": {
 			namespace: "foo",
 			objects: []client.Object{
-				podFromResources("foo1", "foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "bar", []containerResources{
-					{memory: "1337Gi"},
-				}),
+				fooPod,
+				foo2Pod,
+				foobarPod,
 			},
 			memory: "3Gi",
 			cpu:    "3",
@@ -63,32 +75,23 @@ func TestRatioValidator_Handle(t *testing.T) {
 		"Fetch_OtherNamespace": {
 			namespace: "bar",
 			objects: []client.Object{
-				podFromResources("foo1", "foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "bar", []containerResources{
-					{memory: "1337Gi"},
-				}),
+				fooPod,
+				foo2Pod,
+				foobarPod,
 			},
 			memory: "1337Gi",
 			cpu:    "0",
 		},
 		"Fetch_WronglyDisabledNamespace": {
 			namespace: "notdisabled-bar",
 			objects: []client.Object{
-				podFromResources("foo1", "foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "notdisabled-bar", []containerResources{
-					{memory: "1337Gi"},
+				fooPod,
+				foo2Pod,
+				podFromResources("foo", "notdisabled-bar", podResource{
+					containers: []containerResources{
+						{memory: "1337Gi"},
+					},
+					phase: corev1.PodRunning,
 				}),
 			},
 			memory: "1337Gi",
@@ -98,31 +101,33 @@ func TestRatioValidator_Handle(t *testing.T) {
 		"Fetch_DisabledNamespace": {
 			namespace: "disabled-bar",
 			objects: []client.Object{
-				podFromResources("foo1", "foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "disabled-bar", []containerResources{
-					{memory: "1337Gi"},
+				fooPod,
+				foo2Pod,
+				podFromResources("foo", "disabled-bar", podResource{
+					containers: []containerResources{
+						{memory: "1337Gi"},
+					},
+					phase: corev1.PodRunning,
 				}),
 			},
 			err: ErrorDisabled,
 		},
 		"Fetch_OtherDisabledNamespace": {
 			namespace: "disabled-foo",
 			objects: []client.Object{
-				podFromResources("foo1", "disabled-foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "disabled-bar", []containerResources{
-					{memory: "1337Gi"},
+				podFromResources("foo1", "disabled-foo", podResource{
+					containers: []containerResources{
+						{cpu: "1", memory: "1Gi"},
+						{cpu: "2", memory: "1Gi"},
+					},
+					phase: corev1.PodRunning,
+				}),
+				foo2Pod,
+				podFromResources("foo", "disabled-bar", podResource{
+					containers: []containerResources{
+						{memory: "1337Gi"},
+					},
+					phase: corev1.PodRunning,
 				}),
 			},
 			err: ErrorDisabled,
@@ -131,15 +136,13 @@ func TestRatioValidator_Handle(t *testing.T) {
 			namespace: "foo",
 			orgLabel:  "appuio.io/org",
 			objects: []client.Object{
-				podFromResources("foo1", "foo", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "foo", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "disabled-bar", []containerResources{
-					{memory: "1337Gi"},
+				fooPod,
+				foo2Pod,
+				podFromResources("foo", "disabled-bar", podResource{
+					containers: []containerResources{
+						{memory: "1337Gi"},
+					},
+					phase: corev1.PodRunning,
 				}),
 			},
 			err: ErrorDisabled,
@@ -148,16 +151,20 @@ func TestRatioValidator_Handle(t *testing.T) {
 			namespace: "org",
 			orgLabel:  "appuio.io/org",
 			objects: []client.Object{
-				podFromResources("foo1", "org", []containerResources{
-					{cpu: "1", memory: "1Gi"},
-					{cpu: "2", memory: "1Gi"},
-				}),
-				podFromResources("foo2", "org", []containerResources{
-					{memory: "1Gi"},
-				}),
-				podFromResources("foo", "bar", []containerResources{
-					{memory: "1337Gi"},
-				}),
+				podFromResources("foo1", "org", podResource{
+					containers: []containerResources{
+						{cpu: "1", memory: "1Gi"},
+						{cpu: "2", memory: "1Gi"},
+					},
+					phase: corev1.PodRunning,
+				}),
+				podFromResources("foo2", "org", podResource{
+					containers: []containerResources{
+						{memory: "1Gi"},
+					},
+					phase: corev1.PodRunning,
+				}),
+				foobarPod,
 			},
 			memory: "3Gi",
 			cpu:    "3",

diff --git a/ratio/ratio.go b/ratio/ratio.go
@@ -38,10 +38,13 @@ func (r *Ratio) recordReplicatedPodSpec(replicas int32, spec corev1.PodSpec) *Ra
 	return r
 }
 
-// RecordPod collects all requests in the given Pod(s) and adds it to the ratio
+// RecordPod collects all requests in the given Pod(s), and adds it to the ratio
+// The function only considers pods in phase `Running`.
 func (r *Ratio) RecordPod(pods ...corev1.Pod) *Ratio {
 	for _, pod := range pods {
-		r.recordReplicatedPodSpec(1, pod.Spec)
+		if pod.Status.Phase == corev1.PodRunning {
+			r.recordReplicatedPodSpec(1, pod.Spec)
+		}
 	}
 	return r
 }

diff --git a/ratio/ratio_test.go b/ratio/ratio_test.go
@@ -21,10 +21,13 @@ func TestRatio_Record(t *testing.T) {
 		"single container": {
 			pods: []podResource{
 				{
-					{
-						cpu:    "1",
-						memory: "4Gi",
+					containers: []containerResources{
+						{
+							cpu:    "1",
+							memory: "4Gi",
+						},
 					},
+					phase: corev1.PodRunning,
 				},
 			},
 			cpuSum:    "1",
@@ -33,14 +36,17 @@ func TestRatio_Record(t *testing.T) {
 		"multi container": {
 			pods: []podResource{
 				{
-					{
-						cpu:    "500m",
-						memory: "4Gi",
-					},
-					{
-						cpu:    "701m",
-						memory: "1Gi",
+					containers: []containerResources{
+						{
+							cpu:    "500m",
+							memory: "4Gi",
+						},
+						{
+							cpu:    "701m",
+							memory: "1Gi",
+						},
 					},
+					phase: corev1.PodRunning,
 				},
 			},
 			cpuSum:    "1201m",
@@ -49,28 +55,65 @@ func TestRatio_Record(t *testing.T) {
 		"multi pod": {
 			pods: []podResource{
 				{
-					{
-						cpu:    "500m",
-						memory: "4Gi",
-					},
-					{
-						cpu:    "101m",
-						memory: "1Gi",
+					containers: []containerResources{
+						{
+							cpu:    "500m",
+							memory: "4Gi",
+						},
+						{
+							cpu:    "101m",
+							memory: "1Gi",
+						},
 					},
+					phase: corev1.PodRunning,
 				},
 				{
-					{
-						memory: "1Gi",
-					},
-					{
-						cpu:    "101m",
-						memory: "101Mi",
+					containers: []containerResources{
+						{
+							memory: "1Gi",
+						},
+						{
+							cpu:    "101m",
+							memory: "101Mi",
+						},
 					},
+					phase: corev1.PodRunning,
 				},
 			},
 			cpuSum:    "702m",
 			memorySum: "6245Mi",
 		},
+		"running+completed pod": {
+			pods: []podResource{
+				{
+					containers: []containerResources{
+						{
+							cpu:    "500m",
+							memory: "4Gi",
+						},
+						{
+							cpu:    "101m",
+							memory: "1Gi",
+						},
+					},
+					phase: corev1.PodRunning,
+				},
+				{
+					containers: []containerResources{
+						{
+							memory: "1Gi",
+						},
+						{
+							cpu:    "101m",
+							memory: "101Mi",
+						},
+					},
+					phase: corev1.PodSucceeded,
+				},
+			},
+			cpuSum:    "601m",
+			memorySum: "5Gi",
+		},
 		"deployments": {
 			deployments: []deployResource{
 				{
@@ -134,8 +177,12 @@ func TestRatio_Record(t *testing.T) {
 
 			r := NewRatio()
 			for _, pr := range tc.pods {
-				pod := corev1.Pod{}
-				pod.Spec.Containers = newTestContainers(pr)
+				pod := corev1.Pod{
+					Status: corev1.PodStatus{
+						Phase: pr.phase,
+					},
+				}
+				pod.Spec.Containers = newTestContainers(pr.containers)
 				r.RecordPod(pod)
 			}
 

diff --git a/ratio/suite_test.go b/ratio/suite_test.go
@@ -72,8 +72,11 @@ func podFromResources(name, namespace string, res podResource) *corev1.Pod {
 			Name:      name,
 			Namespace: namespace,
 		},
+		Status: corev1.PodStatus{
+			Phase: res.phase,
+		},
 	}
-	for i, cr := range res {
+	for i, cr := range res.containers {
 		c := corev1.Container{
 			Name: fmt.Sprintf("container-%d", i),
 			Resources: corev1.ResourceRequirements{
@@ -95,7 +98,10 @@ type deployResource struct {
 	containers []containerResources
 	replicas   int32
 }
-type podResource []containerResources
+type podResource struct {
+	containers []containerResources
+	phase      corev1.PodPhase
+}
 type containerResources struct {
 	cpu    string
 	memory string

diff --git a/webhooks/ratio_validator_test.go b/webhooks/ratio_validator_test.go
@@ -367,6 +367,9 @@ func podFromResources(name, namespace string, res podResource) *corev1.Pod {
 			Name:      name,
 			Namespace: namespace,
 		},
+		Status: corev1.PodStatus{
+			Phase: corev1.PodRunning,
+		},
 	}
 	for i, cr := range res {
 		c := corev1.Container{