Skip to content

Commit 975892c

Browse files
k8s-ci-robotk8s-ci-robot
authored
Merge pull request kubernetes#2915 from bskiba/admission-refactor
Refactor admission controller.
2 parents 4efe655 + d1a3b05 commit 975892c

File tree

7 files changed

+269
-156
lines changed

7 files changed

+269
-156
lines changed

‎vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider.go‎

Lines changed: 17 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -20,40 +20,33 @@ import (
2020
"fmt"
2121

2222
core "k8s.io/api/core/v1"
23-
"k8s.io/apimachinery/pkg/labels"
2423
vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
25-
vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/autoscaling.k8s.io/v1"
26-
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target"
2724
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange"
2825
vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
2926
"k8s.io/klog"
3027
)
3128

3229
// RecommendationProvider gets current recommendation, annotations and vpaName for the given pod.
3330
type RecommendationProvider interface {
34-
GetContainersResourcesForPod(pod *core.Pod) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, string, error)
31+
GetContainersResourcesForPod(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, error)
3532
}
3633

3734
type recommendationProvider struct {
3835
limitsRangeCalculator limitrange.LimitRangeCalculator
3936
recommendationProcessor vpa_api_util.RecommendationProcessor
40-
selectorFetcher target.VpaTargetSelectorFetcher
41-
vpaLister vpa_lister.VerticalPodAutoscalerLister
4237
}
4338

44-
// NewRecommendationProvider constructs the recommendation provider that list VPAs and can be used to determine recommendations for pods.
45-
func NewRecommendationProvider(calculator limitrange.LimitRangeCalculator, recommendationProcessor vpa_api_util.RecommendationProcessor,
46-
selectorFetcher target.VpaTargetSelectorFetcher, vpaLister vpa_lister.VerticalPodAutoscalerLister) *recommendationProvider {
39+
// NewRecommendationProvider constructs the recommendation provider that can be used to determine recommendations for pods.
40+
func NewRecommendationProvider(calculator limitrange.LimitRangeCalculator,
41+
recommendationProcessor vpa_api_util.RecommendationProcessor) *recommendationProvider {
4742
return &recommendationProvider{
4843
limitsRangeCalculator: calculator,
4944
recommendationProcessor: recommendationProcessor,
50-
selectorFetcher: selectorFetcher,
51-
vpaLister: vpaLister,
5245
}
5346
}
5447

5548
// GetContainersResources returns the recommended resources for each container in the given pod in the same order they are specified in the pod.Spec.
56-
func GetContainersResources(pod *core.Pod, podRecommendation vpa_types.RecommendedPodResources, limitRange *core.LimitRangeItem,
49+
func getContainersResources(pod *core.Pod, podRecommendation vpa_types.RecommendedPodResources, limitRange *core.LimitRangeItem,
5750
annotations vpa_api_util.ContainerToAnnotationsMap) []vpa_api_util.ContainerResources {
5851
resources := make([]vpa_api_util.ContainerResources, len(pod.Spec.Containers))
5952
for i, container := range pod.Spec.Containers {
@@ -78,60 +71,30 @@ func GetContainersResources(pod *core.Pod, podRecommendation vpa_types.Recommend
7871
return resources
7972
}
8073

81-
func (p *recommendationProvider) getMatchingVPA(pod *core.Pod) *vpa_types.VerticalPodAutoscaler {
82-
configs, err := p.vpaLister.VerticalPodAutoscalers(pod.Namespace).List(labels.Everything())
83-
if err != nil {
84-
klog.Errorf("failed to get vpa configs: %v", err)
85-
return nil
86-
}
87-
onConfigs := make([]*vpa_api_util.VpaWithSelector, 0)
88-
for _, vpaConfig := range configs {
89-
if vpa_api_util.GetUpdateMode(vpaConfig) == vpa_types.UpdateModeOff {
90-
continue
91-
}
92-
selector, err := p.selectorFetcher.Fetch(vpaConfig)
93-
if err != nil {
94-
klog.V(3).Infof("skipping VPA object %v because we cannot fetch selector: %s", vpaConfig.Name, err)
95-
continue
96-
}
97-
onConfigs = append(onConfigs, &vpa_api_util.VpaWithSelector{
98-
Vpa: vpaConfig,
99-
Selector: selector,
100-
})
101-
}
102-
klog.V(2).Infof("Let's choose from %d configs for pod %s/%s", len(onConfigs), pod.Namespace, pod.Name)
103-
result := vpa_api_util.GetControllingVPAForPod(pod, onConfigs)
104-
if result != nil {
105-
return result.Vpa
106-
}
107-
return nil
108-
}
109-
110-
// GetContainersResourcesForPod returns recommended request for a given pod, annotations and name of controlling VPA.
74+
// GetContainersResourcesForPod returns recommended request for a given pod and associated annotations.
11175
// The returned slice corresponds 1-1 to containers in the Pod.
112-
func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, string, error) {
113-
klog.V(2).Infof("updating requirements for pod %s.", pod.Name)
114-
vpaConfig := p.getMatchingVPA(pod)
115-
if vpaConfig == nil {
116-
klog.V(2).Infof("no matching VPA found for pod %s", pod.Name)
117-
return nil, nil, "", nil
76+
func (p *recommendationProvider) GetContainersResourcesForPod(pod *core.Pod, vpa *vpa_types.VerticalPodAutoscaler) ([]vpa_api_util.ContainerResources, vpa_api_util.ContainerToAnnotationsMap, error) {
77+
if vpa == nil || pod == nil {
78+
klog.V(2).Infof("can't calculate recommendations, one of vpa(%+v), pod(%+v) is nil", vpa, pod)
79+
return nil, nil, nil
11880
}
81+
klog.V(2).Infof("updating requirements for pod %s.", pod.Name)
11982

12083
var annotations vpa_api_util.ContainerToAnnotationsMap
12184
recommendedPodResources := &vpa_types.RecommendedPodResources{}
12285

123-
if vpaConfig.Status.Recommendation != nil {
86+
if vpa.Status.Recommendation != nil {
12487
var err error
125-
recommendedPodResources, annotations, err = p.recommendationProcessor.Apply(vpaConfig.Status.Recommendation, vpaConfig.Spec.ResourcePolicy, vpaConfig.Status.Conditions, pod)
88+
recommendedPodResources, annotations, err = p.recommendationProcessor.Apply(vpa.Status.Recommendation, vpa.Spec.ResourcePolicy, vpa.Status.Conditions, pod)
12689
if err != nil {
12790
klog.V(2).Infof("cannot process recommendation for pod %s", pod.Name)
128-
return nil, annotations, vpaConfig.Name, err
91+
return nil, annotations, err
12992
}
13093
}
13194
containerLimitRange, err := p.limitsRangeCalculator.GetContainerLimitRangeItem(pod.Namespace)
13295
if err != nil {
133-
return nil, nil, "", fmt.Errorf("error getting containerLimitRange: %s", err)
96+
return nil, nil, fmt.Errorf("error getting containerLimitRange: %s", err)
13497
}
135-
containerResources := GetContainersResources(pod, *recommendedPodResources, containerLimitRange, annotations)
136-
return containerResources, annotations, vpaConfig.Name, nil
98+
containerResources := getContainersResources(pod, *recommendedPodResources, containerLimitRange, annotations)
99+
return containerResources, annotations, nil
137100
}

‎vertical-pod-autoscaler/pkg/admission-controller/logic/recommendation_provider_test.go‎

Lines changed: 16 additions & 80 deletions
Original file line numberDiff line numberDiff line change
@@ -23,24 +23,14 @@ import (
2323

2424
apiv1 "k8s.io/api/core/v1"
2525
"k8s.io/apimachinery/pkg/api/resource"
26-
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
27-
"k8s.io/apimachinery/pkg/labels"
2826
vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1"
29-
target_mock "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/target/mock"
3027
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/limitrange"
3128
"k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/test"
3229
vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa"
3330

34-
"github.com/golang/mock/gomock"
3531
"github.com/stretchr/testify/assert"
3632
)
3733

38-
func parseLabelSelector(selector string) labels.Selector {
39-
labelSelector, _ := metav1.ParseToLabelSelector(selector)
40-
parsedSelector, _ := metav1.LabelSelectorAsSelector(labelSelector)
41-
return parsedSelector
42-
}
43-
4434
func mustParseResourcePointer(val string) *resource.Quantity {
4535
q := resource.MustParse(val)
4636
return &q
@@ -105,8 +95,6 @@ func TestUpdateResourceRequests(t *testing.T) {
10595
limitsNoRequestsPod := test.Pod().WithName("test_initialized").
10696
AddContainer(limitsNoRequestsContainer).WithLabels(labels).Get()
10797

108-
offVPA := vpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get()
109-
11098
targetBelowMinVPA := vpaBuilder.WithTarget("3", "150Mi").WithMinAllowed("4", "300Mi").WithMaxAllowed("5", "1Gi").Get()
11199
targetAboveMaxVPA := vpaBuilder.WithTarget("7", "2Gi").WithMinAllowed("4", "300Mi").WithMaxAllowed("5", "1Gi").Get()
112100
vpaWithHighMemory := vpaBuilder.WithTarget("2", "1000Mi").WithMaxAllowed("3", "3Gi").Get()
@@ -120,7 +108,7 @@ func TestUpdateResourceRequests(t *testing.T) {
120108
testCases := []struct {
121109
name string
122110
pod *apiv1.Pod
123-
vpas []*vpa_types.VerticalPodAutoscaler
111+
vpa *vpa_types.VerticalPodAutoscaler
124112
expectedAction bool
125113
expectedError error
126114
expectedMem resource.Quantity
@@ -130,142 +118,107 @@ func TestUpdateResourceRequests(t *testing.T) {
130118
limitRange *apiv1.LimitRangeItem
131119
limitRangeCalcErr error
132120
annotations vpa_api_util.ContainerToAnnotationsMap
133-
labelSelector string
134121
}{
135122
{
136123
name: "uninitialized pod",
137124
pod: uninitialized,
138-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
125+
vpa: vpa,
139126
expectedAction: true,
140127
expectedMem: resource.MustParse("200Mi"),
141128
expectedCPU: resource.MustParse("2"),
142-
labelSelector: "app = testingApp",
143129
},
144130
{
145131
name: "target below min",
146132
pod: uninitialized,
147-
vpas: []*vpa_types.VerticalPodAutoscaler{targetBelowMinVPA},
133+
vpa: targetBelowMinVPA,
148134
expectedAction: true,
149135
expectedMem: resource.MustParse("300Mi"), // MinMemory is expected to be used
150136
expectedCPU: resource.MustParse("4"), // MinCpu is expected to be used
151137
annotations: vpa_api_util.ContainerToAnnotationsMap{
152138
containerName: []string{"cpu capped to minAllowed", "memory capped to minAllowed"},
153139
},
154-
labelSelector: "app = testingApp",
155140
},
156141
{
157142
name: "target above max",
158143
pod: uninitialized,
159-
vpas: []*vpa_types.VerticalPodAutoscaler{targetAboveMaxVPA},
144+
vpa: targetAboveMaxVPA,
160145
expectedAction: true,
161146
expectedMem: resource.MustParse("1Gi"), // MaxMemory is expected to be used
162147
expectedCPU: resource.MustParse("5"), // MaxCpu is expected to be used
163148
annotations: vpa_api_util.ContainerToAnnotationsMap{
164149
containerName: []string{"cpu capped to maxAllowed", "memory capped to maxAllowed"},
165150
},
166-
labelSelector: "app = testingApp",
167151
},
168152
{
169153
name: "initialized pod",
170154
pod: initialized,
171-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
155+
vpa: vpa,
172156
expectedAction: true,
173157
expectedMem: resource.MustParse("200Mi"),
174158
expectedCPU: resource.MustParse("2"),
175-
labelSelector: "app = testingApp",
176159
},
177160
{
178161
name: "high memory",
179162
pod: initialized,
180-
vpas: []*vpa_types.VerticalPodAutoscaler{vpaWithHighMemory},
163+
vpa: vpaWithHighMemory,
181164
expectedAction: true,
182165
expectedMem: resource.MustParse("1000Mi"),
183166
expectedCPU: resource.MustParse("2"),
184-
labelSelector: "app = testingApp",
185-
},
186-
{
187-
name: "not matching selecetor",
188-
pod: uninitialized,
189-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
190-
expectedAction: false,
191-
labelSelector: "app = differentApp",
192-
},
193-
{
194-
name: "off mode",
195-
pod: uninitialized,
196-
vpas: []*vpa_types.VerticalPodAutoscaler{offVPA},
197-
expectedAction: false,
198-
labelSelector: "app = testingApp",
199-
},
200-
{
201-
name: "two vpas one in off mode",
202-
pod: uninitialized,
203-
vpas: []*vpa_types.VerticalPodAutoscaler{offVPA, vpa},
204-
expectedAction: true,
205-
expectedMem: resource.MustParse("200Mi"),
206-
expectedCPU: resource.MustParse("2"),
207-
labelSelector: "app = testingApp",
208167
},
209168
{
210169
name: "empty recommendation",
211170
pod: initialized,
212-
vpas: []*vpa_types.VerticalPodAutoscaler{vpaWithEmptyRecommendation},
171+
vpa: vpaWithEmptyRecommendation,
213172
expectedAction: true,
214173
expectedMem: resource.MustParse("0"),
215174
expectedCPU: resource.MustParse("0"),
216-
labelSelector: "app = testingApp",
217175
},
218176
{
219177
pod: initialized,
220-
vpas: []*vpa_types.VerticalPodAutoscaler{vpaWithNilRecommendation},
178+
vpa: vpaWithNilRecommendation,
221179
expectedAction: true,
222180
expectedMem: resource.MustParse("0"),
223181
expectedCPU: resource.MustParse("0"),
224-
labelSelector: "app = testingApp",
225182
},
226183
{
227184
name: "guaranteed resources",
228185
pod: limitsMatchRequestsPod,
229-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
186+
vpa: vpa,
230187
expectedAction: true,
231188
expectedMem: resource.MustParse("200Mi"),
232189
expectedCPU: resource.MustParse("2"),
233190
expectedCPULimit: mustParseResourcePointer("2"),
234191
expectedMemLimit: mustParseResourcePointer("200Mi"),
235-
labelSelector: "app = testingApp",
236192
},
237193
{
238194
name: "guaranteed resources - no request",
239195
pod: limitsNoRequestsPod,
240-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
196+
vpa: vpa,
241197
expectedAction: true,
242198
expectedMem: resource.MustParse("200Mi"),
243199
expectedCPU: resource.MustParse("2"),
244200
expectedCPULimit: mustParseResourcePointer("2"),
245201
expectedMemLimit: mustParseResourcePointer("200Mi"),
246-
labelSelector: "app = testingApp",
247202
},
248203
{
249204
name: "proportional limit",
250205
pod: podWithDoubleLimit,
251-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
206+
vpa: vpa,
252207
expectedAction: true,
253208
expectedCPU: resource.MustParse("2"),
254209
expectedMem: resource.MustParse("200Mi"),
255210
expectedCPULimit: mustParseResourcePointer("4"),
256211
expectedMemLimit: mustParseResourcePointer("400Mi"),
257-
labelSelector: "app = testingApp",
258212
},
259213
{
260214
name: "limit over int64",
261215
pod: podWithTenfoldLimit,
262-
vpas: []*vpa_types.VerticalPodAutoscaler{vpaWithExabyteRecommendation},
216+
vpa: vpaWithExabyteRecommendation,
263217
expectedAction: true,
264218
expectedCPU: resource.MustParse("1Ei"),
265219
expectedMem: resource.MustParse("1Ei"),
266220
expectedCPULimit: resource.NewMilliQuantity(math.MaxInt64, resource.DecimalExponent),
267221
expectedMemLimit: resource.NewMilliQuantity(math.MaxInt64, resource.DecimalExponent),
268-
labelSelector: "app = testingApp",
269222
annotations: vpa_api_util.ContainerToAnnotationsMap{
270223
containerName: []string{
271224
"cpu: failed to keep limit to request ratio; capping limit to int64",
@@ -276,21 +229,20 @@ func TestUpdateResourceRequests(t *testing.T) {
276229
{
277230
name: "limit range calculation error",
278231
pod: initialized,
279-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
232+
vpa: vpa,
280233
limitRangeCalcErr: fmt.Errorf("oh no"),
281234
expectedAction: false,
282235
expectedError: fmt.Errorf("error getting containerLimitRange: oh no"),
283236
},
284237
{
285238
name: "proportional limit from default",
286239
pod: initialized,
287-
vpas: []*vpa_types.VerticalPodAutoscaler{vpa},
240+
vpa: vpa,
288241
expectedAction: true,
289242
expectedCPU: resource.MustParse("2"),
290243
expectedMem: resource.MustParse("200Mi"),
291244
expectedCPULimit: mustParseResourcePointer("2"),
292245
expectedMemLimit: mustParseResourcePointer("200Mi"),
293-
labelSelector: "app = testingApp",
294246
limitRange: &apiv1.LimitRangeItem{
295247
Type: apiv1.LimitTypeContainer,
296248
Default: apiv1.ResourceList{
@@ -302,34 +254,18 @@ func TestUpdateResourceRequests(t *testing.T) {
302254
}
303255

304256
for _, tc := range testCases {
305-
t.Run(fmt.Sprintf(tc.name), func(t *testing.T) {
306-
ctrl := gomock.NewController(t)
307-
defer ctrl.Finish()
308-
309-
mockSelectorFetcher := target_mock.NewMockVpaTargetSelectorFetcher(ctrl)
310-
311-
vpaNamespaceLister := &test.VerticalPodAutoscalerListerMock{}
312-
vpaNamespaceLister.On("List").Return(tc.vpas, nil)
313-
314-
vpaLister := &test.VerticalPodAutoscalerListerMock{}
315-
vpaLister.On("VerticalPodAutoscalers", "default").Return(vpaNamespaceLister)
316-
317-
mockSelectorFetcher.EXPECT().Fetch(gomock.Any()).AnyTimes().Return(parseLabelSelector(tc.labelSelector), nil)
318-
257+
t.Run(tc.name, func(t *testing.T) {
319258
recommendationProvider := &recommendationProvider{
320-
vpaLister: vpaLister,
321259
recommendationProcessor: vpa_api_util.NewCappingRecommendationProcessor(limitrange.NewNoopLimitsCalculator()),
322-
selectorFetcher: mockSelectorFetcher,
323260
limitsRangeCalculator: &fakeLimitRangeCalculator{
324261
containerLimitRange: tc.limitRange,
325262
containerErr: tc.limitRangeCalcErr,
326263
},
327264
}
328265

329-
resources, annotations, name, err := recommendationProvider.GetContainersResourcesForPod(tc.pod)
266+
resources, annotations, err := recommendationProvider.GetContainersResourcesForPod(tc.pod, tc.vpa)
330267

331268
if tc.expectedAction {
332-
assert.Equal(t, vpaName, name)
333269
assert.Nil(t, err)
334270
if !assert.Equal(t, len(resources), 1) {
335271
return

0 commit comments

Comments
 (0)