From 3eb5bf87175ae8b14c6ef36e9a947d860e0e74b8 Mon Sep 17 00:00:00 2001 From: David Benque Date: Wed, 5 Oct 2022 16:25:59 +0200 Subject: [PATCH 1/5] [vpa] introduce recommendation post processor --- .../pkg/recommender/logic/recommender.go | 30 +++++++ .../pkg/recommender/main.go | 7 +- .../routines/capping_post_processor.go | 41 +++++++++ .../routines/recommendation_post_processor.go | 83 +++++++++++++++++++ .../pkg/recommender/routines/recommender.go | 71 +++++----------- .../recommender/routines/recommender_test.go | 2 +- 6 files changed, 184 insertions(+), 50 deletions(-) create mode 100644 vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go create mode 100644 vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go diff --git a/vertical-pod-autoscaler/pkg/recommender/logic/recommender.go b/vertical-pod-autoscaler/pkg/recommender/logic/recommender.go index 39861aa01b..9a7ad8e51b 100644 --- a/vertical-pod-autoscaler/pkg/recommender/logic/recommender.go +++ b/vertical-pod-autoscaler/pkg/recommender/logic/recommender.go @@ -18,7 +18,9 @@ package logic import ( "flag" + "sort" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" ) @@ -147,3 +149,31 @@ func CreatePodResourceRecommender() PodResourceRecommender { lowerBoundEstimator, upperBoundEstimator} } + +// MapToListOfRecommendedContainerResources converts the map of RecommendedContainerResources into a stable sorted list +// This can be used to get a stable sequence while ranging on the data +func MapToListOfRecommendedContainerResources(resources RecommendedPodResources) *vpa_types.RecommendedPodResources { + containerResources := make([]vpa_types.RecommendedContainerResources, 0, len(resources)) + // Sort the container names from the map. This is because maps are an + // unordered data structure, and iterating through the map will return + // a different order on every call. + containerNames := make([]string, 0, len(resources)) + for containerName := range resources { + containerNames = append(containerNames, containerName) + } + sort.Strings(containerNames) + // Create the list of recommendations for each container. + for _, name := range containerNames { + containerResources = append(containerResources, vpa_types.RecommendedContainerResources{ + ContainerName: name, + Target: model.ResourcesAsResourceList(resources[name].Target), + LowerBound: model.ResourcesAsResourceList(resources[name].LowerBound), + UpperBound: model.ResourcesAsResourceList(resources[name].UpperBound), + UncappedTarget: model.ResourcesAsResourceList(resources[name].Target), + }) + } + recommendation := &vpa_types.RecommendedPodResources{ + ContainerRecommendations: containerResources, + } + return recommendation +} diff --git a/vertical-pod-autoscaler/pkg/recommender/main.go b/vertical-pod-autoscaler/pkg/recommender/main.go index e7400c0109..838340d4f9 100644 --- a/vertical-pod-autoscaler/pkg/recommender/main.go +++ b/vertical-pod-autoscaler/pkg/recommender/main.go @@ -57,6 +57,7 @@ var ( ctrPodNameLabel = flag.String("container-pod-name-label", "pod_name", `Label name to look for container pod names`) ctrNameLabel = flag.String("container-name-label", "name", `Label name to look for container names`) vpaObjectNamespace = flag.String("vpa-object-namespace", apiv1.NamespaceAll, "Namespace to search for VPA objects and pod stats. Empty means all namespaces will be used.") + postProcessors = flag.String("recommendation-post-processors", "capping", "Coma separated list of post processor names.") ) // Aggregation configuration flags @@ -82,7 +83,11 @@ func main() { metrics_quality.Register() useCheckpoints := *storage != "prometheus" - recommender := routines.NewRecommender(config, *checkpointsGCInterval, useCheckpoints, *vpaObjectNamespace, *recommenderName) + validatedPostProcessors, err := routines.ParsePostProcessors(*postProcessors, true) + if err != nil { + klog.Fatalf("Failed to build post processors: %v", err) + } + recommender := routines.NewRecommender(config, *checkpointsGCInterval, useCheckpoints, *vpaObjectNamespace, *recommenderName, validatedPostProcessors) promQueryTimeout, err := time.ParseDuration(*queryTimeout) if err != nil { diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go new file mode 100644 index 0000000000..b4c2055947 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go @@ -0,0 +1,41 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package routines + +import ( + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" + vpa_utils "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" + "k8s.io/klog/v2" +) + +// cappingPostProcessor ensure that the policy is applied to recommendation +// it applies policy for fields: MinAllowed and MaxAllowed +type cappingPostProcessor struct{} + +var _ RecommendationPostProcessor = &cappingPostProcessor{} + +// Process apply the capping post-processing to the recommendation. (use to be function getCappedRecommendation) +func (c cappingPostProcessor) Process(vpa *model.Vpa, recommendation *vpa_types.RecommendedPodResources, policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources { + // TODO: maybe rename the vpa_utils.ApplyVPAPolicy to something that mention that it is doing capping only + cappedRecommendation, err := vpa_utils.ApplyVPAPolicy(recommendation, policy) + if err != nil { + klog.Errorf("Failed to apply policy for VPA %v/%v: %v", vpa.ID.Namespace, vpa.ID.VpaName, err) + return recommendation + } + return cappedRecommendation +} diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go new file mode 100644 index 0000000000..a4a813c9c9 --- /dev/null +++ b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go @@ -0,0 +1,83 @@ +/* +Copyright 2017 The Kubernetes Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package routines + +import ( + "fmt" + vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" + "strings" +) + +// KnownPostProcessors represent the names of the known post-processors +type KnownPostProcessors string + +const ( + // Capping is post-processor name to ensure that recommendation stays within [MinAllowed-MaxAllowed] range + Capping KnownPostProcessors = "capping" +) + +// RecommendationPostProcessor can amend the recommendation according to the defined policies +type RecommendationPostProcessor interface { + Process(vpa *model.Vpa, recommendation *vpa_types.RecommendedPodResources, + policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources +} + +// RecommendationPostProcessorsBuilder helps for the creation of the pre-processors list +type RecommendationPostProcessorsBuilder interface { + // Build returns the list of post processors or an error + Build() ([]RecommendationPostProcessor, error) +} + +// RecommendationPostProcessorFactory helps to build processors by their name +// using a struct and not a simple function to hold parameters that might be necessary to create some post processors in the future +type RecommendationPostProcessorFactory struct { + names []KnownPostProcessors +} + +// Build returns the list of post processors or an error +// implements interface RecommendationPostProcessorsBuilder +func (f *RecommendationPostProcessorFactory) Build() ([]RecommendationPostProcessor, error) { + var processors []RecommendationPostProcessor + for _, name := range f.names { + switch name { + case Capping: + processors = append(processors, &cappingPostProcessor{}) + default: + return nil, fmt.Errorf("unknown Post Processor: %s", name) + } + } + return processors, nil +} + +// ParsePostProcessors parses a comma separated list of post processor names, validate it, +// returns the list of post-processors or an error. +// if `endsWithCapping` is set to true, the parse will automatically append the `capping` post-processor +// at the end of the list if it is not already there. +func ParsePostProcessors(postProcessorNames string, endsWithCapping bool) ([]RecommendationPostProcessor, error) { + var names []KnownPostProcessors + cappingLast := false + for _, name := range strings.Split(postProcessorNames, ",") { + names = append(names, KnownPostProcessors(name)) + cappingLast = name == string(Capping) + } + if endsWithCapping && !cappingLast { + names = append(names, Capping) + } + b := RecommendationPostProcessorFactory{names: names} + return b.Build() +} diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommender.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommender.go index dab5170bb7..46a167f04d 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/recommender.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/recommender.go @@ -19,10 +19,8 @@ package routines import ( "context" "flag" - "sort" "time" - vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" vpa_clientset "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned" vpa_api "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/clientset/versioned/typed/autoscaling.k8s.io/v1" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/checkpoint" @@ -80,6 +78,7 @@ type recommender struct { podResourceRecommender logic.PodResourceRecommender useCheckpoints bool lastAggregateContainerStateGC time.Time + recommendationPostProcessor []RecommendationPostProcessor } func (r *recommender) GetClusterState() *model.ClusterState { @@ -107,7 +106,14 @@ func (r *recommender) UpdateVPAs() { } resources := r.podResourceRecommender.GetRecommendedPodResources(GetContainerNameToAggregateStateMap(vpa)) had := vpa.HasRecommendation() - vpa.UpdateRecommendation(getCappedRecommendation(vpa.ID, resources, observedVpa.Spec.ResourcePolicy)) + + listOfResourceRecommendation := logic.MapToListOfRecommendedContainerResources(resources) + + for _, postProcessor := range r.recommendationPostProcessor { + listOfResourceRecommendation = postProcessor.Process(vpa, listOfResourceRecommendation, observedVpa.Spec.ResourcePolicy) + } + + vpa.UpdateRecommendation(listOfResourceRecommendation) if vpa.HasRecommendation() && !had { metrics_recommender.ObserveRecommendationLatency(vpa.Created) } @@ -138,42 +144,6 @@ func (r *recommender) UpdateVPAs() { } } -// getCappedRecommendation creates a recommendation based on recommended pod -// resources, setting the UncappedTarget to the calculated recommended target -// and if necessary, capping the Target, LowerBound and UpperBound according -// to the ResourcePolicy. -func getCappedRecommendation(vpaID model.VpaID, resources logic.RecommendedPodResources, - policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources { - containerResources := make([]vpa_types.RecommendedContainerResources, 0, len(resources)) - // Sort the container names from the map. This is because maps are an - // unordered data structure, and iterating through the map will return - // a different order on every call. - containerNames := make([]string, 0, len(resources)) - for containerName := range resources { - containerNames = append(containerNames, containerName) - } - sort.Strings(containerNames) - // Create the list of recommendations for each container. - for _, name := range containerNames { - containerResources = append(containerResources, vpa_types.RecommendedContainerResources{ - ContainerName: name, - Target: model.ResourcesAsResourceList(resources[name].Target), - LowerBound: model.ResourcesAsResourceList(resources[name].LowerBound), - UpperBound: model.ResourcesAsResourceList(resources[name].UpperBound), - UncappedTarget: model.ResourcesAsResourceList(resources[name].Target), - }) - } - recommendation := &vpa_types.RecommendedPodResources{ - ContainerRecommendations: containerResources, - } - cappedRecommendation, err := vpa_utils.ApplyVPAPolicy(recommendation, policy) - if err != nil { - klog.Errorf("Failed to apply policy for VPA %v/%v: %v", vpaID.Namespace, vpaID.VpaName, err) - return recommendation - } - return cappedRecommendation -} - func (r *recommender) MaintainCheckpoints(ctx context.Context, minCheckpointsPerRun int) { now := time.Now() if r.useCheckpoints { @@ -228,6 +198,8 @@ type RecommenderFactory struct { PodResourceRecommender logic.PodResourceRecommender VpaClient vpa_api.VerticalPodAutoscalersGetter + RecommendationPostProcessors []RecommendationPostProcessor + CheckpointsGCInterval time.Duration UseCheckpoints bool } @@ -244,6 +216,7 @@ func (c RecommenderFactory) Make() Recommender { useCheckpoints: c.UseCheckpoints, vpaClient: c.VpaClient, podResourceRecommender: c.PodResourceRecommender, + recommendationPostProcessor: c.RecommendationPostProcessors, lastAggregateContainerStateGC: time.Now(), lastCheckpointGC: time.Now(), } @@ -254,19 +227,21 @@ func (c RecommenderFactory) Make() Recommender { // NewRecommender creates a new recommender instance. // Dependencies are created automatically. // Deprecated; use RecommenderFactory instead. -func NewRecommender(config *rest.Config, checkpointsGCInterval time.Duration, useCheckpoints bool, namespace string, recommenderName string) Recommender { +func NewRecommender(config *rest.Config, checkpointsGCInterval time.Duration, useCheckpoints bool, namespace string, recommenderName string, recommendationPostProcessors []RecommendationPostProcessor) Recommender { clusterState := model.NewClusterState(AggregateContainerStateGCInterval) kubeClient := kube_client.NewForConfigOrDie(config) factory := informers.NewSharedInformerFactoryWithOptions(kubeClient, defaultResyncPeriod, informers.WithNamespace(namespace)) controllerFetcher := controllerfetcher.NewControllerFetcher(config, kubeClient, factory, scaleCacheEntryFreshnessTime, scaleCacheEntryLifetime, scaleCacheEntryJitterFactor) + return RecommenderFactory{ - ClusterState: clusterState, - ClusterStateFeeder: input.NewClusterStateFeeder(config, clusterState, *memorySaver, namespace, "default-metrics-client", recommenderName), - ControllerFetcher: controllerFetcher, - CheckpointWriter: checkpoint.NewCheckpointWriter(clusterState, vpa_clientset.NewForConfigOrDie(config).AutoscalingV1()), - VpaClient: vpa_clientset.NewForConfigOrDie(config).AutoscalingV1(), - PodResourceRecommender: logic.CreatePodResourceRecommender(), - CheckpointsGCInterval: checkpointsGCInterval, - UseCheckpoints: useCheckpoints, + ClusterState: clusterState, + ClusterStateFeeder: input.NewClusterStateFeeder(config, clusterState, *memorySaver, namespace, "default-metrics-client", recommenderName), + ControllerFetcher: controllerFetcher, + CheckpointWriter: checkpoint.NewCheckpointWriter(clusterState, vpa_clientset.NewForConfigOrDie(config).AutoscalingV1()), + VpaClient: vpa_clientset.NewForConfigOrDie(config).AutoscalingV1(), + PodResourceRecommender: logic.CreatePodResourceRecommender(), + RecommendationPostProcessors: recommendationPostProcessors, + CheckpointsGCInterval: checkpointsGCInterval, + UseCheckpoints: useCheckpoints, }.Make() } diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go index 9b8dfd1839..27e867b1dd 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go @@ -68,7 +68,7 @@ func TestSortedRecommendation(t *testing.T) { t.Run(tc.name, func(t *testing.T) { namespace := "test-namespace" vpa := model.NewVpa(model.VpaID{Namespace: namespace, VpaName: "my-vpa"}, labels.Nothing(), time.Unix(0, 0)) - vpa.UpdateRecommendation(getCappedRecommendation(vpa.ID, tc.resources, nil)) + vpa.UpdateRecommendation(logic.MapToListOfRecommendedContainerResources(tc.resources)) // Check that the slice is in the correct order. for i := range vpa.Recommendation.ContainerRecommendations { assert.Equal(t, tc.expectedLast[i], vpa.Recommendation.ContainerRecommendations[i].ContainerName) From fcdf42d3bc2f833cf2b9c48a0d624a819acc1994 Mon Sep 17 00:00:00 2001 From: David Benque Date: Tue, 11 Oct 2022 14:19:24 +0200 Subject: [PATCH 2/5] code review, move flag to boolean for post processor --- .../pkg/recommender/logic/recommender_test.go | 52 +++++++++++++ .../pkg/recommender/main.go | 14 +++- .../routines/capping_post_processor.go | 2 +- .../routines/recommendation_post_processor.go | 29 ++----- .../recommender/routines/recommender_test.go | 78 ------------------- 5 files changed, 70 insertions(+), 105 deletions(-) delete mode 100644 vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go diff --git a/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go b/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go index c19398559b..ba9d981504 100644 --- a/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go @@ -17,7 +17,9 @@ limitations under the License. package logic import ( + "k8s.io/apimachinery/pkg/labels" "testing" + "time" "github.com/stretchr/testify/assert" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" @@ -115,3 +117,53 @@ func TestControlledResourcesFilteredDefault(t *testing.T) { assert.Contains(t, recommendedResources[containerName].LowerBound, model.ResourceCPU) assert.Contains(t, recommendedResources[containerName].UpperBound, model.ResourceCPU) } + +func TestMapToListOfRecommendedContainerResources(t *testing.T) { + cases := []struct { + name string + resources RecommendedPodResources + expectedLast []string + }{ + { + name: "All recommendations sorted", + resources: RecommendedPodResources{ + "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + }, + expectedLast: []string{ + "a-container", + "b-container", + "c-container", + "d-container", + }, + }, + { + name: "All recommendations unsorted", + resources: RecommendedPodResources{ + "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + }, + expectedLast: []string{ + "a-container", + "b-container", + "c-container", + "d-container", + }, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + namespace := "test-namespace" + vpa := model.NewVpa(model.VpaID{Namespace: namespace, VpaName: "my-vpa"}, labels.Nothing(), time.Unix(0, 0)) + vpa.UpdateRecommendation(MapToListOfRecommendedContainerResources(tc.resources)) + // Check that the slice is in the correct order. + for i := range vpa.Recommendation.ContainerRecommendations { + assert.Equal(t, tc.expectedLast[i], vpa.Recommendation.ContainerRecommendations[i].ContainerName) + } + }) + } +} diff --git a/vertical-pod-autoscaler/pkg/recommender/main.go b/vertical-pod-autoscaler/pkg/recommender/main.go index 838340d4f9..f0cc250919 100644 --- a/vertical-pod-autoscaler/pkg/recommender/main.go +++ b/vertical-pod-autoscaler/pkg/recommender/main.go @@ -57,7 +57,9 @@ var ( ctrPodNameLabel = flag.String("container-pod-name-label", "pod_name", `Label name to look for container pod names`) ctrNameLabel = flag.String("container-name-label", "name", `Label name to look for container names`) vpaObjectNamespace = flag.String("vpa-object-namespace", apiv1.NamespaceAll, "Namespace to search for VPA objects and pod stats. Empty means all namespaces will be used.") - postProcessors = flag.String("recommendation-post-processors", "capping", "Coma separated list of post processor names.") + + // list of post processors flag + postProcessorCapping = flag.Bool("post-processor-capping", true, "Enable 'capping' post processor: apply minAllowed/maxAllowed range for the recommendation") ) // Aggregation configuration flags @@ -83,11 +85,17 @@ func main() { metrics_quality.Register() useCheckpoints := *storage != "prometheus" - validatedPostProcessors, err := routines.ParsePostProcessors(*postProcessors, true) + + var postProcessorsNames []routines.KnownPostProcessors + if *postProcessorCapping { + postProcessorsNames = append(postProcessorsNames, routines.Capping) + } + postProcessorFactory := routines.RecommendationPostProcessorFactory{PostProcessorsNames: postProcessorsNames} + postProcessors, err := postProcessorFactory.Build() if err != nil { klog.Fatalf("Failed to build post processors: %v", err) } - recommender := routines.NewRecommender(config, *checkpointsGCInterval, useCheckpoints, *vpaObjectNamespace, *recommenderName, validatedPostProcessors) + recommender := routines.NewRecommender(config, *checkpointsGCInterval, useCheckpoints, *vpaObjectNamespace, *recommenderName, postProcessors) promQueryTimeout, err := time.ParseDuration(*queryTimeout) if err != nil { diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go index b4c2055947..d9a20923e0 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2022 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go index a4a813c9c9..239ab5c554 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go @@ -1,5 +1,5 @@ /* -Copyright 2017 The Kubernetes Authors. +Copyright 2022 The Kubernetes Authors. Licensed under the Apache License, Version 2.0 (the "License"); you may not use this file except in compliance with the License. @@ -20,10 +20,10 @@ import ( "fmt" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" - "strings" + "k8s.io/klog/v2" ) -// KnownPostProcessors represent the names of the known post-processors +// KnownPostProcessors represent the PostProcessorsNames of the known post-processors type KnownPostProcessors string const ( @@ -46,14 +46,14 @@ type RecommendationPostProcessorsBuilder interface { // RecommendationPostProcessorFactory helps to build processors by their name // using a struct and not a simple function to hold parameters that might be necessary to create some post processors in the future type RecommendationPostProcessorFactory struct { - names []KnownPostProcessors + PostProcessorsNames []KnownPostProcessors } // Build returns the list of post processors or an error // implements interface RecommendationPostProcessorsBuilder func (f *RecommendationPostProcessorFactory) Build() ([]RecommendationPostProcessor, error) { var processors []RecommendationPostProcessor - for _, name := range f.names { + for _, name := range f.PostProcessorsNames { switch name { case Capping: processors = append(processors, &cappingPostProcessor{}) @@ -61,23 +61,6 @@ func (f *RecommendationPostProcessorFactory) Build() ([]RecommendationPostProces return nil, fmt.Errorf("unknown Post Processor: %s", name) } } + klog.Infof("List of recommendation post-processors: %v", processors) return processors, nil } - -// ParsePostProcessors parses a comma separated list of post processor names, validate it, -// returns the list of post-processors or an error. -// if `endsWithCapping` is set to true, the parse will automatically append the `capping` post-processor -// at the end of the list if it is not already there. -func ParsePostProcessors(postProcessorNames string, endsWithCapping bool) ([]RecommendationPostProcessor, error) { - var names []KnownPostProcessors - cappingLast := false - for _, name := range strings.Split(postProcessorNames, ",") { - names = append(names, KnownPostProcessors(name)) - cappingLast = name == string(Capping) - } - if endsWithCapping && !cappingLast { - names = append(names, Capping) - } - b := RecommendationPostProcessorFactory{names: names} - return b.Build() -} diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go deleted file mode 100644 index 27e867b1dd..0000000000 --- a/vertical-pod-autoscaler/pkg/recommender/routines/recommender_test.go +++ /dev/null @@ -1,78 +0,0 @@ -/* -Copyright 2022 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package routines - -import ( - "testing" - "time" - - labels "k8s.io/apimachinery/pkg/labels" - "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/logic" - "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" - - "github.com/stretchr/testify/assert" -) - -func TestSortedRecommendation(t *testing.T) { - cases := []struct { - name string - resources logic.RecommendedPodResources - expectedLast []string - }{ - { - name: "All recommendations sorted", - resources: logic.RecommendedPodResources{ - "a-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "b-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "c-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "d-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - }, - expectedLast: []string{ - "a-container", - "b-container", - "c-container", - "d-container", - }, - }, - { - name: "All recommendations unsorted", - resources: logic.RecommendedPodResources{ - "b-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "a-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "d-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "c-container": logic.RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - }, - expectedLast: []string{ - "a-container", - "b-container", - "c-container", - "d-container", - }, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - namespace := "test-namespace" - vpa := model.NewVpa(model.VpaID{Namespace: namespace, VpaName: "my-vpa"}, labels.Nothing(), time.Unix(0, 0)) - vpa.UpdateRecommendation(logic.MapToListOfRecommendedContainerResources(tc.resources)) - // Check that the slice is in the correct order. - for i := range vpa.Recommendation.ContainerRecommendations { - assert.Equal(t, tc.expectedLast[i], vpa.Recommendation.ContainerRecommendations[i].ContainerName) - } - }) - } -} From 95698da1a8640c82c10e5c2b4bc037b986fa5625 Mon Sep 17 00:00:00 2001 From: David Benque Date: Fri, 14 Oct 2022 16:22:18 +0200 Subject: [PATCH 3/5] remove the flag for Capping post-processor --- vertical-pod-autoscaler/pkg/recommender/main.go | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/recommender/main.go b/vertical-pod-autoscaler/pkg/recommender/main.go index f0cc250919..92ec1325b8 100644 --- a/vertical-pod-autoscaler/pkg/recommender/main.go +++ b/vertical-pod-autoscaler/pkg/recommender/main.go @@ -57,9 +57,6 @@ var ( ctrPodNameLabel = flag.String("container-pod-name-label", "pod_name", `Label name to look for container pod names`) ctrNameLabel = flag.String("container-name-label", "name", `Label name to look for container names`) vpaObjectNamespace = flag.String("vpa-object-namespace", apiv1.NamespaceAll, "Namespace to search for VPA objects and pod stats. Empty means all namespaces will be used.") - - // list of post processors flag - postProcessorCapping = flag.Bool("post-processor-capping", true, "Enable 'capping' post processor: apply minAllowed/maxAllowed range for the recommendation") ) // Aggregation configuration flags @@ -87,9 +84,8 @@ func main() { useCheckpoints := *storage != "prometheus" var postProcessorsNames []routines.KnownPostProcessors - if *postProcessorCapping { - postProcessorsNames = append(postProcessorsNames, routines.Capping) - } + postProcessorsNames = append(postProcessorsNames, routines.Capping) + postProcessorFactory := routines.RecommendationPostProcessorFactory{PostProcessorsNames: postProcessorsNames} postProcessors, err := postProcessorFactory.Build() if err != nil { From 398e68e936f09e8018125c3d154299447336403c Mon Sep 17 00:00:00 2001 From: David Benque Date: Mon, 31 Oct 2022 15:34:33 +0100 Subject: [PATCH 4/5] remove post-processor factory --- .../pkg/recommender/main.go | 9 +---- .../routines/capping_post_processor.go | 8 ++-- .../routines/recommendation_post_processor.go | 38 ------------------- 3 files changed, 6 insertions(+), 49 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/recommender/main.go b/vertical-pod-autoscaler/pkg/recommender/main.go index 92ec1325b8..80743fd7cf 100644 --- a/vertical-pod-autoscaler/pkg/recommender/main.go +++ b/vertical-pod-autoscaler/pkg/recommender/main.go @@ -83,13 +83,8 @@ func main() { useCheckpoints := *storage != "prometheus" - var postProcessorsNames []routines.KnownPostProcessors - postProcessorsNames = append(postProcessorsNames, routines.Capping) - - postProcessorFactory := routines.RecommendationPostProcessorFactory{PostProcessorsNames: postProcessorsNames} - postProcessors, err := postProcessorFactory.Build() - if err != nil { - klog.Fatalf("Failed to build post processors: %v", err) + postProcessors := []routines.RecommendationPostProcessor{ + &routines.CappingPostProcessor{}, } recommender := routines.NewRecommender(config, *checkpointsGCInterval, useCheckpoints, *vpaObjectNamespace, *recommenderName, postProcessors) diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go index d9a20923e0..ad05ee7b59 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/capping_post_processor.go @@ -23,14 +23,14 @@ import ( "k8s.io/klog/v2" ) -// cappingPostProcessor ensure that the policy is applied to recommendation +// CappingPostProcessor ensure that the policy is applied to recommendation // it applies policy for fields: MinAllowed and MaxAllowed -type cappingPostProcessor struct{} +type CappingPostProcessor struct{} -var _ RecommendationPostProcessor = &cappingPostProcessor{} +var _ RecommendationPostProcessor = &CappingPostProcessor{} // Process apply the capping post-processing to the recommendation. (use to be function getCappedRecommendation) -func (c cappingPostProcessor) Process(vpa *model.Vpa, recommendation *vpa_types.RecommendedPodResources, policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources { +func (c CappingPostProcessor) Process(vpa *model.Vpa, recommendation *vpa_types.RecommendedPodResources, policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources { // TODO: maybe rename the vpa_utils.ApplyVPAPolicy to something that mention that it is doing capping only cappedRecommendation, err := vpa_utils.ApplyVPAPolicy(recommendation, policy) if err != nil { diff --git a/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go index 239ab5c554..f73cbb0ecc 100644 --- a/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go +++ b/vertical-pod-autoscaler/pkg/recommender/routines/recommendation_post_processor.go @@ -17,18 +17,8 @@ limitations under the License. package routines import ( - "fmt" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" - "k8s.io/klog/v2" -) - -// KnownPostProcessors represent the PostProcessorsNames of the known post-processors -type KnownPostProcessors string - -const ( - // Capping is post-processor name to ensure that recommendation stays within [MinAllowed-MaxAllowed] range - Capping KnownPostProcessors = "capping" ) // RecommendationPostProcessor can amend the recommendation according to the defined policies @@ -36,31 +26,3 @@ type RecommendationPostProcessor interface { Process(vpa *model.Vpa, recommendation *vpa_types.RecommendedPodResources, policy *vpa_types.PodResourcePolicy) *vpa_types.RecommendedPodResources } - -// RecommendationPostProcessorsBuilder helps for the creation of the pre-processors list -type RecommendationPostProcessorsBuilder interface { - // Build returns the list of post processors or an error - Build() ([]RecommendationPostProcessor, error) -} - -// RecommendationPostProcessorFactory helps to build processors by their name -// using a struct and not a simple function to hold parameters that might be necessary to create some post processors in the future -type RecommendationPostProcessorFactory struct { - PostProcessorsNames []KnownPostProcessors -} - -// Build returns the list of post processors or an error -// implements interface RecommendationPostProcessorsBuilder -func (f *RecommendationPostProcessorFactory) Build() ([]RecommendationPostProcessor, error) { - var processors []RecommendationPostProcessor - for _, name := range f.PostProcessorsNames { - switch name { - case Capping: - processors = append(processors, &cappingPostProcessor{}) - default: - return nil, fmt.Errorf("unknown Post Processor: %s", name) - } - } - klog.Infof("List of recommendation post-processors: %v", processors) - return processors, nil -} From cacd6b66e1ae18dbb42143ac25c14ad0d5d898dc Mon Sep 17 00:00:00 2001 From: David Benque Date: Mon, 31 Oct 2022 16:46:20 +0100 Subject: [PATCH 5/5] update test for MapToListOfRecommendedContainerResources --- .../pkg/recommender/logic/recommender_test.go | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go b/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go index ba9d981504..6e7e788dc3 100644 --- a/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/logic/recommender_test.go @@ -17,12 +17,9 @@ limitations under the License. package logic import ( - "k8s.io/apimachinery/pkg/labels" - "testing" - "time" - "github.com/stretchr/testify/assert" "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/recommender/model" + "testing" ) func TestMinResourcesApplied(t *testing.T) { @@ -127,10 +124,10 @@ func TestMapToListOfRecommendedContainerResources(t *testing.T) { { name: "All recommendations sorted", resources: RecommendedPodResources{ - "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1e6)}}, + "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(2), model.ResourceMemory: model.MemoryAmountFromBytes(2e6)}}, + "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(3), model.ResourceMemory: model.MemoryAmountFromBytes(3e6)}}, + "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(4), model.ResourceMemory: model.MemoryAmountFromBytes(4e6)}}, }, expectedLast: []string{ "a-container", @@ -142,10 +139,10 @@ func TestMapToListOfRecommendedContainerResources(t *testing.T) { { name: "All recommendations unsorted", resources: RecommendedPodResources{ - "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, - "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1000)}}, + "b-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(1), model.ResourceMemory: model.MemoryAmountFromBytes(1e6)}}, + "a-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(2), model.ResourceMemory: model.MemoryAmountFromBytes(2e6)}}, + "d-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(3), model.ResourceMemory: model.MemoryAmountFromBytes(3e6)}}, + "c-container": RecommendedContainerResources{Target: model.Resources{model.ResourceCPU: model.CPUAmountFromCores(4), model.ResourceMemory: model.MemoryAmountFromBytes(4e6)}}, }, expectedLast: []string{ "a-container", @@ -157,12 +154,13 @@ func TestMapToListOfRecommendedContainerResources(t *testing.T) { } for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { - namespace := "test-namespace" - vpa := model.NewVpa(model.VpaID{Namespace: namespace, VpaName: "my-vpa"}, labels.Nothing(), time.Unix(0, 0)) - vpa.UpdateRecommendation(MapToListOfRecommendedContainerResources(tc.resources)) - // Check that the slice is in the correct order. - for i := range vpa.Recommendation.ContainerRecommendations { - assert.Equal(t, tc.expectedLast[i], vpa.Recommendation.ContainerRecommendations[i].ContainerName) + outRecommendations := MapToListOfRecommendedContainerResources(tc.resources) + for i, outRecommendation := range outRecommendations.ContainerRecommendations { + containerName := tc.expectedLast[i] + assert.Equal(t, containerName, outRecommendation.ContainerName) + // also check that the recommendation is not changed + assert.Equal(t, int64(tc.resources[containerName].Target[model.ResourceCPU]), outRecommendation.Target.Cpu().MilliValue()) + assert.Equal(t, int64(tc.resources[containerName].Target[model.ResourceMemory]), outRecommendation.Target.Memory().Value()) } }) }