Skip to content

Commit 5ef8b94

Browse files
simusimu
authored
Merge pull request #28 from appuio/fix/ignore-non-running-pods
Only consider running pods when computing ratio
2 parents b871994 + 3682317 commit 5ef8b94

File tree

6 files changed

+164
-95
lines changed

6 files changed

+164
-95
lines changed

‎controllers/ratio_controller_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,4 +192,7 @@ var testPod = &corev1.Pod{
192192
Name: "pod",
193193
Namespace: "foo",
194194
},
195+
Status: corev1.PodStatus{
196+
Phase: corev1.PodRunning,
197+
},
195198
}

‎ratio/fetcher_test.go‎

Lines changed: 73 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ import (
77

88
"github.com/stretchr/testify/assert"
99
"github.com/stretchr/testify/require"
10+
corev1 "k8s.io/api/core/v1"
1011
apierrors "k8s.io/apimachinery/pkg/api/errors"
1112
"k8s.io/apimachinery/pkg/api/resource"
1213
utilruntime "k8s.io/apimachinery/pkg/util/runtime"
@@ -17,6 +18,24 @@ import (
1718
"sigs.k8s.io/controller-runtime/pkg/client/fake"
1819
)
1920

21+
var (
22+
fooPod = podFromResources("foo1", "foo", podResource{
23+
containers: []containerResources{
24+
{cpu: "1", memory: "1Gi"},
25+
{cpu: "2", memory: "1Gi"},
26+
},
27+
phase: corev1.PodRunning,
28+
})
29+
foo2Pod = podFromResources("foo2", "foo", podResource{
30+
containers: []containerResources{{memory: "1Gi"}},
31+
phase: corev1.PodRunning,
32+
})
33+
foobarPod = podFromResources("foo", "bar", podResource{
34+
containers: []containerResources{{memory: "1337Gi"}},
35+
phase: corev1.PodRunning,
36+
})
37+
)
38+
2039
func TestRatioValidator_Handle(t *testing.T) {
2140
ctx := context.Background()
2241
tests := map[string]struct {
@@ -36,16 +55,9 @@ func TestRatioValidator_Handle(t *testing.T) {
3655
"Fetch_Namespace": {
3756
namespace: "foo",
3857
objects: []client.Object{
39-
podFromResources("foo1", "foo", []containerResources{
40-
{cpu: "1", memory: "1Gi"},
41-
{cpu: "2", memory: "1Gi"},
42-
}),
43-
podFromResources("foo2", "foo", []containerResources{
44-
{memory: "1Gi"},
45-
}),
46-
podFromResources("foo", "bar", []containerResources{
47-
{memory: "1337Gi"},
48-
}),
58+
fooPod,
59+
foo2Pod,
60+
foobarPod,
4961
},
5062
memory: "3Gi",
5163
cpu: "3",
@@ -63,32 +75,23 @@ func TestRatioValidator_Handle(t *testing.T) {
6375
"Fetch_OtherNamespace": {
6476
namespace: "bar",
6577
objects: []client.Object{
66-
podFromResources("foo1", "foo", []containerResources{
67-
{cpu: "1", memory: "1Gi"},
68-
{cpu: "2", memory: "1Gi"},
69-
}),
70-
podFromResources("foo2", "foo", []containerResources{
71-
{memory: "1Gi"},
72-
}),
73-
podFromResources("foo", "bar", []containerResources{
74-
{memory: "1337Gi"},
75-
}),
78+
fooPod,
79+
foo2Pod,
80+
foobarPod,
7681
},
7782
memory: "1337Gi",
7883
cpu: "0",
7984
},
8085
"Fetch_WronglyDisabledNamespace": {
8186
namespace: "notdisabled-bar",
8287
objects: []client.Object{
83-
podFromResources("foo1", "foo", []containerResources{
84-
{cpu: "1", memory: "1Gi"},
85-
{cpu: "2", memory: "1Gi"},
86-
}),
87-
podFromResources("foo2", "foo", []containerResources{
88-
{memory: "1Gi"},
89-
}),
90-
podFromResources("foo", "notdisabled-bar", []containerResources{
91-
{memory: "1337Gi"},
88+
fooPod,
89+
foo2Pod,
90+
podFromResources("foo", "notdisabled-bar", podResource{
91+
containers: []containerResources{
92+
{memory: "1337Gi"},
93+
},
94+
phase: corev1.PodRunning,
9295
}),
9396
},
9497
memory: "1337Gi",
@@ -98,31 +101,33 @@ func TestRatioValidator_Handle(t *testing.T) {
98101
"Fetch_DisabledNamespace": {
99102
namespace: "disabled-bar",
100103
objects: []client.Object{
101-
podFromResources("foo1", "foo", []containerResources{
102-
{cpu: "1", memory: "1Gi"},
103-
{cpu: "2", memory: "1Gi"},
104-
}),
105-
podFromResources("foo2", "foo", []containerResources{
106-
{memory: "1Gi"},
107-
}),
108-
podFromResources("foo", "disabled-bar", []containerResources{
109-
{memory: "1337Gi"},
104+
fooPod,
105+
foo2Pod,
106+
podFromResources("foo", "disabled-bar", podResource{
107+
containers: []containerResources{
108+
{memory: "1337Gi"},
109+
},
110+
phase: corev1.PodRunning,
110111
}),
111112
},
112113
err: ErrorDisabled,
113114
},
114115
"Fetch_OtherDisabledNamespace": {
115116
namespace: "disabled-foo",
116117
objects: []client.Object{
117-
podFromResources("foo1", "disabled-foo", []containerResources{
118-
{cpu: "1", memory: "1Gi"},
119-
{cpu: "2", memory: "1Gi"},
120-
}),
121-
podFromResources("foo2", "foo", []containerResources{
122-
{memory: "1Gi"},
123-
}),
124-
podFromResources("foo", "disabled-bar", []containerResources{
125-
{memory: "1337Gi"},
118+
podFromResources("foo1", "disabled-foo", podResource{
119+
containers: []containerResources{
120+
{cpu: "1", memory: "1Gi"},
121+
{cpu: "2", memory: "1Gi"},
122+
},
123+
phase: corev1.PodRunning,
124+
}),
125+
foo2Pod,
126+
podFromResources("foo", "disabled-bar", podResource{
127+
containers: []containerResources{
128+
{memory: "1337Gi"},
129+
},
130+
phase: corev1.PodRunning,
126131
}),
127132
},
128133
err: ErrorDisabled,
@@ -131,15 +136,13 @@ func TestRatioValidator_Handle(t *testing.T) {
131136
namespace: "foo",
132137
orgLabel: "appuio.io/org",
133138
objects: []client.Object{
134-
podFromResources("foo1", "foo", []containerResources{
135-
{cpu: "1", memory: "1Gi"},
136-
{cpu: "2", memory: "1Gi"},
137-
}),
138-
podFromResources("foo2", "foo", []containerResources{
139-
{memory: "1Gi"},
140-
}),
141-
podFromResources("foo", "disabled-bar", []containerResources{
142-
{memory: "1337Gi"},
139+
fooPod,
140+
foo2Pod,
141+
podFromResources("foo", "disabled-bar", podResource{
142+
containers: []containerResources{
143+
{memory: "1337Gi"},
144+
},
145+
phase: corev1.PodRunning,
143146
}),
144147
},
145148
err: ErrorDisabled,
@@ -148,16 +151,20 @@ func TestRatioValidator_Handle(t *testing.T) {
148151
namespace: "org",
149152
orgLabel: "appuio.io/org",
150153
objects: []client.Object{
151-
podFromResources("foo1", "org", []containerResources{
152-
{cpu: "1", memory: "1Gi"},
153-
{cpu: "2", memory: "1Gi"},
154-
}),
155-
podFromResources("foo2", "org", []containerResources{
156-
{memory: "1Gi"},
157-
}),
158-
podFromResources("foo", "bar", []containerResources{
159-
{memory: "1337Gi"},
160-
}),
154+
podFromResources("foo1", "org", podResource{
155+
containers: []containerResources{
156+
{cpu: "1", memory: "1Gi"},
157+
{cpu: "2", memory: "1Gi"},
158+
},
159+
phase: corev1.PodRunning,
160+
}),
161+
podFromResources("foo2", "org", podResource{
162+
containers: []containerResources{
163+
{memory: "1Gi"},
164+
},
165+
phase: corev1.PodRunning,
166+
}),
167+
foobarPod,
161168
},
162169
memory: "3Gi",
163170
cpu: "3",

‎ratio/ratio.go‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -38,10 +38,13 @@ func (r *Ratio) recordReplicatedPodSpec(replicas int32, spec corev1.PodSpec) *Ra
3838
return r
3939
}
4040

41-
// RecordPod collects all requests in the given Pod(s) and adds it to the ratio
41+
// RecordPod collects all requests in the given Pod(s), and adds it to the ratio
42+
// The function only considers pods in phase `Running`.
4243
func (r *Ratio) RecordPod(pods ...corev1.Pod) *Ratio {
4344
for _, pod := range pods {
44-
r.recordReplicatedPodSpec(1, pod.Spec)
45+
if pod.Status.Phase == corev1.PodRunning {
46+
r.recordReplicatedPodSpec(1, pod.Spec)
47+
}
4548
}
4649
return r
4750
}

‎ratio/ratio_test.go‎

Lines changed: 72 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -21,10 +21,13 @@ func TestRatio_Record(t *testing.T) {
2121
"single container": {
2222
pods: []podResource{
2323
{
24-
{
25-
cpu: "1",
26-
memory: "4Gi",
24+
containers: []containerResources{
25+
{
26+
cpu: "1",
27+
memory: "4Gi",
28+
},
2729
},
30+
phase: corev1.PodRunning,
2831
},
2932
},
3033
cpuSum: "1",
@@ -33,14 +36,17 @@ func TestRatio_Record(t *testing.T) {
3336
"multi container": {
3437
pods: []podResource{
3538
{
36-
{
37-
cpu: "500m",
38-
memory: "4Gi",
39-
},
40-
{
41-
cpu: "701m",
42-
memory: "1Gi",
39+
containers: []containerResources{
40+
{
41+
cpu: "500m",
42+
memory: "4Gi",
43+
},
44+
{
45+
cpu: "701m",
46+
memory: "1Gi",
47+
},
4348
},
49+
phase: corev1.PodRunning,
4450
},
4551
},
4652
cpuSum: "1201m",
@@ -49,28 +55,65 @@ func TestRatio_Record(t *testing.T) {
4955
"multi pod": {
5056
pods: []podResource{
5157
{
52-
{
53-
cpu: "500m",
54-
memory: "4Gi",
55-
},
56-
{
57-
cpu: "101m",
58-
memory: "1Gi",
58+
containers: []containerResources{
59+
{
60+
cpu: "500m",
61+
memory: "4Gi",
62+
},
63+
{
64+
cpu: "101m",
65+
memory: "1Gi",
66+
},
5967
},
68+
phase: corev1.PodRunning,
6069
},
6170
{
62-
{
63-
memory: "1Gi",
64-
},
65-
{
66-
cpu: "101m",
67-
memory: "101Mi",
71+
containers: []containerResources{
72+
{
73+
memory: "1Gi",
74+
},
75+
{
76+
cpu: "101m",
77+
memory: "101Mi",
78+
},
6879
},
80+
phase: corev1.PodRunning,
6981
},
7082
},
7183
cpuSum: "702m",
7284
memorySum: "6245Mi",
7385
},
86+
"running+completed pod": {
87+
pods: []podResource{
88+
{
89+
containers: []containerResources{
90+
{
91+
cpu: "500m",
92+
memory: "4Gi",
93+
},
94+
{
95+
cpu: "101m",
96+
memory: "1Gi",
97+
},
98+
},
99+
phase: corev1.PodRunning,
100+
},
101+
{
102+
containers: []containerResources{
103+
{
104+
memory: "1Gi",
105+
},
106+
{
107+
cpu: "101m",
108+
memory: "101Mi",
109+
},
110+
},
111+
phase: corev1.PodSucceeded,
112+
},
113+
},
114+
cpuSum: "601m",
115+
memorySum: "5Gi",
116+
},
74117
"deployments": {
75118
deployments: []deployResource{
76119
{
@@ -134,8 +177,12 @@ func TestRatio_Record(t *testing.T) {
134177

135178
r := NewRatio()
136179
for _, pr := range tc.pods {
137-
pod := corev1.Pod{}
138-
pod.Spec.Containers = newTestContainers(pr)
180+
pod := corev1.Pod{
181+
Status: corev1.PodStatus{
182+
Phase: pr.phase,
183+
},
184+
}
185+
pod.Spec.Containers = newTestContainers(pr.containers)
139186
r.RecordPod(pod)
140187
}
141188

‎ratio/suite_test.go‎

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -72,8 +72,11 @@ func podFromResources(name, namespace string, res podResource) *corev1.Pod {
7272
Name: name,
7373
Namespace: namespace,
7474
},
75+
Status: corev1.PodStatus{
76+
Phase: res.phase,
77+
},
7578
}
76-
for i, cr := range res {
79+
for i, cr := range res.containers {
7780
c := corev1.Container{
7881
Name: fmt.Sprintf("container-%d", i),
7982
Resources: corev1.ResourceRequirements{
@@ -95,7 +98,10 @@ type deployResource struct {
9598
containers []containerResources
9699
replicas int32
97100
}
98-
type podResource []containerResources
101+
type podResource struct {
102+
containers []containerResources
103+
phase corev1.PodPhase
104+
}
99105
type containerResources struct {
100106
cpu string
101107
memory string

‎webhooks/ratio_validator_test.go‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -367,6 +367,9 @@ func podFromResources(name, namespace string, res podResource) *corev1.Pod {
367367
Name: name,
368368
Namespace: namespace,
369369
},
370+
Status: corev1.PodStatus{
371+
Phase: corev1.PodRunning,
372+
},
370373
}
371374
for i, cr := range res {
372375
c := corev1.Container{

0 commit comments

Comments
 (0)