From 7111717ce8d2179d6bcd610647bfa1dbb1de1816 Mon Sep 17 00:00:00 2001 From: Beata Skiba Date: Wed, 15 Jan 2020 17:03:41 +0100 Subject: [PATCH] Propagate scaling mode to containers. Correctly update ResourcePolicy and UpdatePolicy on update. --- .../model/aggregate_container_state.go | 43 ++- .../model/aggregate_container_state_test.go | 39 +++ .../pkg/recommender/model/cluster.go | 6 +- .../pkg/recommender/model/cluster_test.go | 153 +++++++- .../pkg/recommender/model/vpa.go | 29 ++ .../pkg/recommender/model/vpa_test.go | 330 ++++++++++++++---- .../pkg/utils/test/test_vpa.go | 9 + 7 files changed, 530 insertions(+), 79 deletions(-) diff --git a/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go b/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go index b009e44a436b..c6ff1743e514 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state.go @@ -90,13 +90,20 @@ type AggregateContainerState struct { // each container should add one peak per memory aggregation interval (e.g. once every 24h). AggregateMemoryPeaks util.Histogram // Note: first/last sample timestamps as well as the sample count are based only on CPU samples. - FirstSampleStart time.Time - LastSampleStart time.Time - TotalSamplesCount int - CreationTime time.Time + FirstSampleStart time.Time + LastSampleStart time.Time + TotalSamplesCount int + CreationTime time.Time + + // Following fields are needed to correctly report quality metrics + // for VPA. When we record a new sample in an AggregateContainerState + // we want to know if it needs recommendation, if the recommendation + // is present and if the automatic updates are on (are we able to + // apply the recommendation to the pods). LastRecommendation corev1.ResourceList IsUnderVPA bool UpdateMode *vpa_types.UpdateMode + ScalingMode *vpa_types.ContainerScalingMode } // GetLastRecommendation returns last recorded recommendation. @@ -106,7 +113,7 @@ func (a *AggregateContainerState) GetLastRecommendation() corev1.ResourceList { // NeedsRecommendation returns true if the state should have recommendation calculated. func (a *AggregateContainerState) NeedsRecommendation() bool { - return a.IsUnderVPA + return a.IsUnderVPA && a.ScalingMode != nil && *a.ScalingMode != vpa_types.ContainerScalingModeOff } // GetUpdateMode returns the update mode of VPA controlling this aggregator, @@ -115,12 +122,19 @@ func (a *AggregateContainerState) GetUpdateMode() *vpa_types.UpdateMode { return a.UpdateMode } -// MarkNotAutoscaled registers that this container state is not contorled by +// GetScalingMode returns the container scaling mode of the container +// represented byt his aggregator, nil if aggregator is not autoscaled. +func (a *AggregateContainerState) GetScalingMode() *vpa_types.ContainerScalingMode { + return a.ScalingMode +} + +// MarkNotAutoscaled registers that this container state is not controlled by // a VPA object. func (a *AggregateContainerState) MarkNotAutoscaled() { a.IsUnderVPA = false a.LastRecommendation = nil a.UpdateMode = nil + a.ScalingMode = nil } // MergeContainerState merges two AggregateContainerStates. @@ -241,6 +255,17 @@ func (a *AggregateContainerState) isEmpty() bool { return a.TotalSamplesCount == 0 } +// UpdateFromPolicy updates container state scaling mode based on resource +// policy of the VPA object. +func (a *AggregateContainerState) UpdateFromPolicy(resourcePolicy *vpa_types.ContainerResourcePolicy) { + // ContainerScalingModeAuto is the default scaling mode + scalingModeAuto := vpa_types.ContainerScalingModeAuto + a.ScalingMode = &scalingModeAuto + if resourcePolicy != nil && resourcePolicy.Mode != nil { + a.ScalingMode = resourcePolicy.Mode + } +} + // AggregateStateByContainerName takes a set of AggregateContainerStates and merge them // grouping by the container name. The result is a map from the container name to the aggregation // from all input containers with the given name. @@ -301,3 +326,9 @@ func (p *ContainerStateAggregatorProxy) GetUpdateMode() *vpa_types.UpdateMode { aggregator := p.cluster.findOrCreateAggregateContainerState(p.containerID) return aggregator.GetUpdateMode() } + +// GetScalingMode returns scaling mode of container represented by the aggregator. +func (p *ContainerStateAggregatorProxy) GetScalingMode() *vpa_types.ContainerScalingMode { + aggregator := p.cluster.findOrCreateAggregateContainerState(p.containerID) + return aggregator.GetScalingMode() +} diff --git a/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state_test.go b/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state_test.go index ece2108672dc..2a120ca19d38 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/aggregate_container_state_test.go @@ -208,3 +208,42 @@ func TestAggregateContainerStateIsExpired(t *testing.T) { assert.False(t, csEmpty.isExpired(testTimestamp.Add(7*24*time.Hour))) assert.True(t, csEmpty.isExpired(testTimestamp.Add(8*24*time.Hour))) } + +func TestUpdateFromPolicy(t *testing.T) { + scalingModeAuto := vpa_types.ContainerScalingModeAuto + scalingModeOff := vpa_types.ContainerScalingModeOff + testCases := []struct { + name string + policy *vpa_types.ContainerResourcePolicy + expected *vpa_types.ContainerScalingMode + }{ + { + name: "Explicit auto scaling mode", + policy: &vpa_types.ContainerResourcePolicy{ + Mode: &scalingModeAuto, + }, + expected: &scalingModeAuto, + }, { + name: "Off scaling mode", + policy: &vpa_types.ContainerResourcePolicy{ + Mode: &scalingModeOff, + }, + expected: &scalingModeOff, + }, { + name: "No mode specified - default to Auto", + policy: &vpa_types.ContainerResourcePolicy{}, + expected: &scalingModeAuto, + }, { + name: "Nil policy - default to Auto", + policy: nil, + expected: &scalingModeAuto, + }, + } + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + cs := NewAggregateContainerState() + cs.UpdateFromPolicy(tc.policy) + assert.Equal(t, tc.expected, cs.GetScalingMode()) + }) + } +} diff --git a/vertical-pod-autoscaler/pkg/recommender/model/cluster.go b/vertical-pod-autoscaler/pkg/recommender/model/cluster.go index c01b9285c811..95638dff56dc 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/cluster.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/cluster.go @@ -246,10 +246,8 @@ func (cluster *ClusterState) AddOrUpdateVpa(apiObject *vpa_types.VerticalPodAuto vpa.Annotations = annotationsMap vpa.Conditions = conditionsMap vpa.Recommendation = currentRecommendation - vpa.ResourcePolicy = apiObject.Spec.ResourcePolicy - if apiObject.Spec.UpdatePolicy != nil { - vpa.UpdateMode = apiObject.Spec.UpdatePolicy.UpdateMode - } + vpa.SetUpdateMode(apiObject.Spec.UpdatePolicy) + vpa.SetResourcePolicy(apiObject.Spec.ResourcePolicy) return nil } diff --git a/vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go b/vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go index 94befac6b64e..d585d35530fb 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go @@ -235,13 +235,15 @@ func TestMissingKeys(t *testing.T) { } func addVpa(cluster *ClusterState, id VpaID, annotations vpaAnnotationsMap, selector string) *Vpa { - var apiObject vpa_types.VerticalPodAutoscaler - apiObject.Namespace = id.Namespace - apiObject.Name = id.VpaName - apiObject.Annotations = annotations + apiObject := test.VerticalPodAutoscaler().WithNamespace(id.Namespace). + WithName(id.VpaName).WithContainer(testContainerID.ContainerName).WithAnnotations(annotations).Get() + return addVpaObject(cluster, id, apiObject, selector) +} + +func addVpaObject(cluster *ClusterState, id VpaID, vpa *vpa_types.VerticalPodAutoscaler, selector string) *Vpa { labelSelector, _ := metav1.ParseToLabelSelector(selector) parsedSelector, _ := metav1.LabelSelectorAsSelector(labelSelector) - err := cluster.AddOrUpdateVpa(&apiObject, parsedSelector) + err := cluster.AddOrUpdateVpa(vpa, parsedSelector) if err != nil { klog.Fatalf("AddOrUpdateVpa() failed: %v", err) } @@ -340,6 +342,147 @@ func TestUpdatePodSelector(t *testing.T) { assert.Contains(t, vpa.aggregateContainerStates, cluster.aggregateStateKeyForContainerID(testContainerID)) } +// Test setting ResourcePolicy and UpdatePolicy on adding or updating VPA object +func TestAddOrUpdateVPAPolicies(t *testing.T) { + testVpaBuilder := test.VerticalPodAutoscaler().WithName(testVpaID.VpaName). + WithNamespace(testVpaID.Namespace).WithContainer(testContainerID.ContainerName) + updateModeAuto := vpa_types.UpdateModeAuto + updateModeOff := vpa_types.UpdateModeOff + scalingModeAuto := vpa_types.ContainerScalingModeAuto + scalingModeOff := vpa_types.ContainerScalingModeOff + cases := []struct { + name string + oldVpa *vpa_types.VerticalPodAutoscaler + newVpa *vpa_types.VerticalPodAutoscaler + resourcePolicy *vpa_types.PodResourcePolicy + expectedScalingMode *vpa_types.ContainerScalingMode + expectedUpdateMode *vpa_types.UpdateMode + }{ + { + name: "Defaults to auto", + oldVpa: nil, + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(), + // Container scaling mode is a separate concept from update mode in the VPA object, + // hence the UpdateModeOff does not influence container scaling mode here. + expectedScalingMode: &scalingModeAuto, + expectedUpdateMode: &updateModeOff, + }, { + name: "Default scaling mode set to Off", + oldVpa: nil, + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: &scalingModeOff, + expectedUpdateMode: &updateModeAuto, + }, { + name: "Explicit scaling mode set to Off", + oldVpa: nil, + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: testContainerID.ContainerName, + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: &scalingModeOff, + expectedUpdateMode: &updateModeAuto, + }, { + name: "Other container has explicit scaling mode Off", + oldVpa: nil, + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "other-container", + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: &scalingModeAuto, + expectedUpdateMode: &updateModeAuto, + }, { + name: "Scaling mode to default Off", + oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: &scalingModeOff, + expectedUpdateMode: &updateModeAuto, + }, { + name: "Scaling mode to explicit Off", + oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: testContainerID.ContainerName, + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: &scalingModeOff, + expectedUpdateMode: &updateModeAuto, + }, + // Tests checking changes to UpdateMode. + { + name: "UpdateMode from Off to Auto", + oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(), + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + expectedScalingMode: &scalingModeAuto, + expectedUpdateMode: &updateModeAuto, + }, { + name: "UpdateMode from Auto to Off", + oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(), + newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(), + expectedScalingMode: &scalingModeAuto, + expectedUpdateMode: &updateModeOff, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + cluster := NewClusterState() + addTestPod(cluster) + addTestContainer(cluster) + if tc.oldVpa != nil { + oldVpa := addVpaObject(cluster, testVpaID, tc.oldVpa, testSelectorStr) + if !assert.Contains(t, cluster.Vpas, testVpaID) { + t.FailNow() + } + assert.Len(t, oldVpa.aggregateContainerStates, 1, "Expected one container aggregation in VPA %v", testVpaID) + for containerName, aggregation := range oldVpa.aggregateContainerStates { + assert.Equal(t, &scalingModeAuto, aggregation.GetScalingMode(), "Unexpected scaling mode for container %s", containerName) + } + } + tc.newVpa.Spec.ResourcePolicy = tc.resourcePolicy + addVpaObject(cluster, testVpaID, tc.newVpa, testSelectorStr) + vpa, found := cluster.Vpas[testVpaID] + if !assert.True(t, found, "VPA %+v not found in cluster state.", testVpaID) { + t.FailNow() + } + assert.Equal(t, tc.expectedUpdateMode, vpa.UpdateMode) + assert.Len(t, vpa.aggregateContainerStates, 1, "Expected one container aggregation in VPA %v", testVpaID) + for containerName, aggregation := range vpa.aggregateContainerStates { + assert.Equal(t, tc.expectedUpdateMode, aggregation.UpdateMode, "Unexpected update mode for container %s", containerName) + assert.Equal(t, tc.expectedScalingMode, aggregation.GetScalingMode(), "Unexpected scaling mode for container %s", containerName) + } + }) + } +} + // Verify that two copies of the same AggregateStateKey are equal. func TestEqualAggregateStateKey(t *testing.T) { cluster := NewClusterState() diff --git a/vertical-pod-autoscaler/pkg/recommender/model/vpa.go b/vertical-pod-autoscaler/pkg/recommender/model/vpa.go index 40a23cf4b110..837bc603a514 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/vpa.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/vpa.go @@ -25,6 +25,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/autoscaling.k8s.io/v1" + vpa_api_util "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/utils/vpa" ) // Map from VPA annotation key to value. @@ -135,6 +136,7 @@ func (vpa *Vpa) UseAggregationIfMatching(aggregationKey AggregateStateKey, aggre vpa.aggregateContainerStates[aggregationKey] = aggregation aggregation.IsUnderVPA = true aggregation.UpdateMode = vpa.UpdateMode + aggregation.UpdateFromPolicy(vpa_api_util.GetContainerResourcePolicy(aggregationKey.ContainerName(), vpa.ResourcePolicy)) return true } return false @@ -202,6 +204,33 @@ func (vpa *Vpa) matchesAggregation(aggregationKey AggregateStateKey) bool { return vpa.PodSelector != nil && vpa.PodSelector.Matches(aggregationKey.Labels()) } +// SetResourcePolicy updates the resource policy of the VPA and the scaling +// policies of aggregators under this VPA. +func (vpa *Vpa) SetResourcePolicy(resourcePolicy *vpa_types.PodResourcePolicy) { + if resourcePolicy == vpa.ResourcePolicy { + return + } + vpa.ResourcePolicy = resourcePolicy + for container, state := range vpa.aggregateContainerStates { + state.UpdateFromPolicy(vpa_api_util.GetContainerResourcePolicy(container.ContainerName(), vpa.ResourcePolicy)) + } +} + +// SetUpdateMode updates the update mode of the VPA and aggregators under this VPA. +func (vpa *Vpa) SetUpdateMode(updatePolicy *vpa_types.PodUpdatePolicy) { + if updatePolicy == nil { + vpa.UpdateMode = nil + } else { + if updatePolicy.UpdateMode == vpa.UpdateMode { + return + } + vpa.UpdateMode = updatePolicy.UpdateMode + } + for _, state := range vpa.aggregateContainerStates { + state.UpdateMode = vpa.UpdateMode + } +} + // UpdateConditions updates the conditions of VPA objects based on it's state. // PodsMatched is passed to indicate if there are currently active pods in the // cluster matching this VPA. diff --git a/vertical-pod-autoscaler/pkg/recommender/model/vpa_test.go b/vertical-pod-autoscaler/pkg/recommender/model/vpa_test.go index 2cf3df624710..b9e450d67875 100644 --- a/vertical-pod-autoscaler/pkg/recommender/model/vpa_test.go +++ b/vertical-pod-autoscaler/pkg/recommender/model/vpa_test.go @@ -221,17 +221,19 @@ func TestUpdateRecommendation(t *testing.T) { func TestUseAggregationIfMatching(t *testing.T) { modeOff := vpa_types.UpdateModeOff modeAuto := vpa_types.UpdateModeAuto + scalingModeAuto := vpa_types.ContainerScalingModeAuto + scalingModeOff := vpa_types.ContainerScalingModeOff cases := []struct { name string aggregations []string vpaSelector string + resourcePolicy *vpa_types.PodResourcePolicy updateMode *vpa_types.UpdateMode container string containerLabels map[string]string - isUnderVPA bool - expectedAggregations []string + expectedMatching bool expectedUpdateMode *vpa_types.UpdateMode - expectedNeedsRecommendation bool + expectedNeedsRecommendation map[string]bool }{ { name: "First matching aggregation", @@ -240,9 +242,8 @@ func TestUseAggregationIfMatching(t *testing.T) { updateMode: &modeOff, container: "test-container", containerLabels: testLabels, - isUnderVPA: false, - expectedAggregations: []string{"test-container"}, - expectedNeedsRecommendation: true, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"test-container": true}, expectedUpdateMode: &modeOff, }, { name: "New matching aggregation", @@ -251,9 +252,8 @@ func TestUseAggregationIfMatching(t *testing.T) { updateMode: &modeAuto, container: "second-container", containerLabels: testLabels, - isUnderVPA: false, - expectedAggregations: []string{"test-container", "second-container"}, - expectedNeedsRecommendation: true, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"test-container": true, "second-container": true}, expectedUpdateMode: &modeAuto, }, { name: "Existing matching aggregation", @@ -262,9 +262,8 @@ func TestUseAggregationIfMatching(t *testing.T) { updateMode: &modeOff, container: "test-container", containerLabels: testLabels, - isUnderVPA: true, - expectedAggregations: []string{"test-container"}, - expectedNeedsRecommendation: true, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"test-container": true}, expectedUpdateMode: &modeOff, }, { name: "Aggregation not matching", @@ -273,66 +272,131 @@ func TestUseAggregationIfMatching(t *testing.T) { updateMode: &modeAuto, container: "second-container", containerLabels: map[string]string{"different": "labels"}, - isUnderVPA: false, - expectedAggregations: []string{"test-container"}, - expectedNeedsRecommendation: false, + expectedMatching: false, + expectedNeedsRecommendation: map[string]bool{"test-container": true}, expectedUpdateMode: nil, + }, { + name: "New matching aggregation with scaling mode Off", + aggregations: []string{"test-container"}, + vpaSelector: testSelectorStr, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "second-container", + Mode: &scalingModeOff, + }, + }, + }, + updateMode: &modeAuto, + container: "second-container", + containerLabels: testLabels, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"second-container": false, "test-container": true}, + expectedUpdateMode: &modeAuto, + }, { + name: "New matching aggregation with default scaling mode Off", + aggregations: []string{"test-container"}, + vpaSelector: testSelectorStr, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, + }, + }, + updateMode: &modeAuto, + container: "second-container", + containerLabels: testLabels, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"second-container": false, "test-container": false}, + expectedUpdateMode: &modeAuto, + }, { + name: "New matching aggregation with scaling mode Off with default Auto", + aggregations: []string{"test-container"}, + vpaSelector: testSelectorStr, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeAuto, + }, + { + ContainerName: "second-container", + Mode: &scalingModeOff, + }, + }, + }, + updateMode: &modeAuto, + container: "second-container", + containerLabels: testLabels, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"second-container": false, "test-container": true}, + expectedUpdateMode: &modeAuto, + }, { + name: "New matching aggregation with scaling mode Auto with default Off", + aggregations: []string{"test-container"}, + vpaSelector: testSelectorStr, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, + { + ContainerName: "second-container", + Mode: &scalingModeAuto, + }, + }, + }, + updateMode: &modeAuto, + container: "second-container", + containerLabels: testLabels, + expectedMatching: true, + expectedNeedsRecommendation: map[string]bool{"second-container": true, "test-container": false}, + expectedUpdateMode: &modeAuto, }, } for _, tc := range cases { - if tc.name == "Existing matching aggregation" { - t.Run(tc.name, func(t *testing.T) { - namespace := "test-namespace" - selector, err := labels.Parse(tc.vpaSelector) - if !assert.NoError(t, err) { - t.FailNow() - } - vpa := NewVpa(VpaID{Namespace: namespace, VpaName: "my-favourite-vpa"}, selector, anyTime) - vpa.UpdateMode = tc.updateMode - key := mockAggregateStateKey{ - namespace: namespace, - containerName: tc.container, - labels: labels.Set(tc.containerLabels).String(), - } - aggregation := &AggregateContainerState{ - IsUnderVPA: tc.isUnderVPA, - } - for _, container := range tc.aggregations { - containerKey := mockAggregateStateKey{ - namespace: namespace, - containerName: container, - labels: labels.Set(testLabels).String(), - } - if container == tc.container { - aggregation.UpdateMode = vpa.UpdateMode - vpa.aggregateContainerStates[containerKey] = aggregation - } else { - vpa.aggregateContainerStates[key] = &AggregateContainerState{UpdateMode: vpa.UpdateMode} - } + t.Run(tc.name, func(t *testing.T) { + namespace := "test-namespace" + selector, err := labels.Parse(tc.vpaSelector) + if !assert.NoError(t, err) { + t.FailNow() + } + vpa := NewVpa(VpaID{Namespace: namespace, VpaName: "my-favourite-vpa"}, selector, anyTime) + vpa.UpdateMode = tc.updateMode + key := mockAggregateStateKey{ + namespace: namespace, + containerName: tc.container, + labels: labels.Set(tc.containerLabels).String(), + } + aggregationUnderTest := &AggregateContainerState{} + for _, container := range tc.aggregations { + containerKey, aggregation := testAggregation(vpa, container, labels.Set(testLabels).String()) + vpa.aggregateContainerStates[containerKey] = aggregation + if container == tc.container { + aggregationUnderTest = aggregation } + } + vpa.SetResourcePolicy(tc.resourcePolicy) - vpa.UseAggregationIfMatching(key, aggregation) - assert.Equal(t, len(tc.expectedAggregations), len(vpa.aggregateContainerStates), "AggregateContainerStates has unexpected size") - for _, container := range tc.expectedAggregations { - found := false - for key := range vpa.aggregateContainerStates { - if key.ContainerName() == container { - found = true - break - } - } - assert.True(t, found, "Container %s not found in aggregateContainerStates", container) - } - assert.Equal(t, tc.expectedNeedsRecommendation, aggregation.NeedsRecommendation()) - if tc.expectedUpdateMode == nil { - assert.Nil(t, aggregation.GetUpdateMode()) - } else { - if assert.NotNil(t, aggregation.GetUpdateMode()) { - assert.Equal(t, *tc.expectedUpdateMode, *aggregation.GetUpdateMode()) + matching := vpa.UseAggregationIfMatching(key, aggregationUnderTest) + assert.Equal(t, tc.expectedMatching, matching, "Unexpected assessment of aggregation matching") + assert.Len(t, vpa.aggregateContainerStates, len(tc.expectedNeedsRecommendation), "AggregateContainerStates has unexpected size") + for container, expectedNeedsRecommendation := range tc.expectedNeedsRecommendation { + found := false + for key, state := range vpa.aggregateContainerStates { + if key.ContainerName() == container { + found = true + assert.Equal(t, expectedNeedsRecommendation, state.NeedsRecommendation(), + "Unexpected NeedsRecommendation result for container %s", container) } } - }) - } + assert.True(t, found, "Container %s not found in aggregateContainerStates", container) + } + assert.Equal(t, tc.expectedUpdateMode, aggregationUnderTest.GetUpdateMode()) + }) } } @@ -380,6 +444,144 @@ func TestDeleteAggregation(t *testing.T) { } } +func TestSetResourcePolicy(t *testing.T) { + scalingModeAuto := vpa_types.ContainerScalingModeAuto + scalingModeOff := vpa_types.ContainerScalingModeOff + cases := []struct { + name string + containers []string + resourcePolicy *vpa_types.PodResourcePolicy + expectedScalingMode map[string]*vpa_types.ContainerScalingMode + expectedNeedsRecommendation map[string]bool + }{ + { + name: "Nil policy", + containers: []string{"container1"}, + resourcePolicy: nil, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeAuto, + }, + expectedNeedsRecommendation: map[string]bool{"container1": true}, + }, { + name: "Default policy with no scaling mode", + containers: []string{"container1", "container2"}, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + }, + }, + }, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeAuto, "container2": &scalingModeAuto, + }, + expectedNeedsRecommendation: map[string]bool{ + "container1": true, "container2": true}, + }, { + name: "Default policy with scaling mode auto", + containers: []string{"container1", "container2"}, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeAuto, + }, + }, + }, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeAuto, "container2": &scalingModeAuto, + }, + expectedNeedsRecommendation: map[string]bool{ + "container1": true, "container2": true}, + }, { + name: "Default policy with scaling mode off", + containers: []string{"container1", "container2"}, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeOff, "container2": &scalingModeOff, + }, + expectedNeedsRecommendation: map[string]bool{ + "container1": false, "container2": false}, + }, { + name: "One container has scaling mode Off", + containers: []string{"container1", "container2"}, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "container2", + Mode: &scalingModeOff, + }, + }, + }, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeAuto, "container2": &scalingModeOff, + }, + expectedNeedsRecommendation: map[string]bool{ + "container1": true, "container2": false}, + }, { + name: "One container has scaling mode Auto with default off", + containers: []string{"container1", "container2"}, + resourcePolicy: &vpa_types.PodResourcePolicy{ + ContainerPolicies: []vpa_types.ContainerResourcePolicy{ + { + ContainerName: "*", + Mode: &scalingModeOff, + }, { + ContainerName: "container2", + Mode: &scalingModeAuto, + }, + }, + }, + expectedScalingMode: map[string]*vpa_types.ContainerScalingMode{ + "container1": &scalingModeOff, "container2": &scalingModeAuto, + }, + expectedNeedsRecommendation: map[string]bool{ + "container1": false, "container2": true}, + }, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + selector, err := labels.Parse(testSelectorStr) + if !assert.NoError(t, err) { + t.FailNow() + } + vpa := NewVpa(VpaID{Namespace: "test-namespace", VpaName: "my-favourite-vpa"}, selector, anyTime) + for _, container := range tc.containers { + containerKey, aggregation := testAggregation(vpa, container, labels.Set(testLabels).String()) + vpa.aggregateContainerStates[containerKey] = aggregation + } + vpa.SetResourcePolicy(tc.resourcePolicy) + assert.Equal(t, tc.resourcePolicy, vpa.ResourcePolicy) + for key, state := range vpa.aggregateContainerStates { + containerName := key.ContainerName() + assert.Equal(t, tc.expectedScalingMode[containerName], state.GetScalingMode(), "Unexpected scaling mode for container %s", containerName) + assert.Equal(t, tc.expectedNeedsRecommendation[containerName], state.NeedsRecommendation(), "Unexpected needs recommendation for container %s", containerName) + } + }) + } +} + +func testAggregation(vpa *Vpa, containerName, labels string) (mockAggregateStateKey, *AggregateContainerState) { + scalingModeAuto := vpa_types.ContainerScalingModeAuto + containerKey := mockAggregateStateKey{ + namespace: vpa.ID.Namespace, + containerName: containerName, + labels: labels, + } + aggregation := &AggregateContainerState{} + aggregation.UpdateMode = vpa.UpdateMode + aggregation.IsUnderVPA = true + aggregation.ScalingMode = &scalingModeAuto + return containerKey, aggregation +} + type mockAggregateStateKey struct { namespace string containerName string diff --git a/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go b/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go index 78172ce4cf74..359624804338 100644 --- a/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go +++ b/vertical-pod-autoscaler/pkg/utils/test/test_vpa.go @@ -38,6 +38,7 @@ type VerticalPodAutoscalerBuilder interface { WithLowerBound(cpu, memory string) VerticalPodAutoscalerBuilder WithTargetRef(targetRef *autoscaling.CrossVersionObjectReference) VerticalPodAutoscalerBuilder WithUpperBound(cpu, memory string) VerticalPodAutoscalerBuilder + WithAnnotations(map[string]string) VerticalPodAutoscalerBuilder AppendCondition(conditionType vpa_types.VerticalPodAutoscalerConditionType, status core.ConditionStatus, reason, message string, lastTransitionTime time.Time) VerticalPodAutoscalerBuilder AppendRecommendation(vpa_types.RecommendedContainerResources) VerticalPodAutoscalerBuilder @@ -64,6 +65,7 @@ type verticalPodAutoscalerBuilder struct { maxAllowed core.ResourceList recommendation RecommendationBuilder conditions []vpa_types.VerticalPodAutoscalerCondition + annotations map[string]string targetRef *autoscaling.CrossVersionObjectReference appendedRecommendations []vpa_types.RecommendedContainerResources } @@ -137,6 +139,12 @@ func (b *verticalPodAutoscalerBuilder) WithTargetRef(targetRef *autoscaling.Cros return &c } +func (b *verticalPodAutoscalerBuilder) WithAnnotations(annotations map[string]string) VerticalPodAutoscalerBuilder { + c := *b + c.annotations = annotations + return &c +} + func (b *verticalPodAutoscalerBuilder) AppendCondition(conditionType vpa_types.VerticalPodAutoscalerConditionType, status core.ConditionStatus, reason, message string, lastTransitionTime time.Time) VerticalPodAutoscalerBuilder { c := *b @@ -174,6 +182,7 @@ func (b *verticalPodAutoscalerBuilder) Get() *vpa_types.VerticalPodAutoscaler { ObjectMeta: meta.ObjectMeta{ Name: b.vpaName, Namespace: b.namespace, + Annotations: b.annotations, CreationTimestamp: meta.NewTime(b.creationTimestamp), }, Spec: vpa_types.VerticalPodAutoscalerSpec{