Propagate scaling mode to containers. Correctly update ResourcePolicy…

… and UpdatePolicy on update.
kubernetes · Jan 17, 2020 · 10ca9c0 · 10ca9c0
1 parent 76bc551
commit 10ca9c0
Show file tree

Hide file tree

Showing 7 changed files with 491 additions and 64 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
@@ -97,6 +97,7 @@ type AggregateContainerState struct {
 	LastRecommendation corev1.ResourceList
 	IsUnderVPA         bool
 	UpdateMode         *vpa_types.UpdateMode
+	scalingMode        *vpa_types.ContainerScalingMode
 }
 
 // GetLastRecommendation returns last recorded recommendation.
@@ -106,7 +107,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 +116,19 @@ func (a *AggregateContainerState) GetUpdateMode() *vpa_types.UpdateMode {
 	return a.UpdateMode
 }
 
+// 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 contorled 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 +249,17 @@ func (a *AggregateContainerState) isEmpty() bool {
 	return a.TotalSamplesCount == 0
 }
 
+// UpdateFromPolicy updates container state scaling mode based on reasource
+// 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 +320,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
@@ -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 - dafault to Auto",
+			policy:   &vpa_types.ContainerResourcePolicy{},
+			expected: &scalingModeAuto,
+		}, {
+			name:     "Nil policy - dafault 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
@@ -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
@@ -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,120 @@ 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)
+	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
+	}{
+		{
+			name:                "New VPA",
+			oldVpa:              nil,
+			newVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
+			expectedScalingMode: &scalingModeAuto,
+		}, {
+			name:   "New VPA with default scaling mode Off",
+			oldVpa: nil,
+			newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
+			resourcePolicy: &vpa_types.PodResourcePolicy{
+				ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+					{
+						ContainerName: "*",
+						Mode:          &scalingModeOff,
+					},
+				},
+			},
+			expectedScalingMode: &scalingModeOff,
+		}, {
+			name:   "New VPA with explicit scaling mode 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,
+		}, {
+			name:                "UpdateMode from Off to Auto",
+			oldVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
+			newVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
+			expectedScalingMode: &scalingModeAuto,
+		}, {
+			name:                "UpdateMode from Auto to Off",
+			oldVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
+			newVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
+			expectedScalingMode: &scalingModeAuto,
+		}, {
+			name:   "Scaling mode to default Off",
+			oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
+			newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
+			resourcePolicy: &vpa_types.PodResourcePolicy{
+				ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+					{
+						ContainerName: "*",
+						Mode:          &scalingModeOff,
+					},
+				},
+			},
+			expectedScalingMode: &scalingModeOff,
+		}, {
+			name:   "Scaling mode to explicit Off",
+			oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
+			newVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
+			resourcePolicy: &vpa_types.PodResourcePolicy{
+				ContainerPolicies: []vpa_types.ContainerResourcePolicy{
+					{
+						ContainerName: testContainerID.ContainerName,
+						Mode:          &scalingModeOff,
+					},
+				},
+			},
+			expectedScalingMode: &scalingModeOff,
+		},
+	}
+	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()
+			}
+			expectedUpdateMode := tc.newVpa.Spec.UpdatePolicy.UpdateMode
+			assert.Equal(t, vpa.UpdateMode, expectedUpdateMode)
+			assert.Len(t, vpa.aggregateContainerStates, 1, "Expected one container aggregation in VPA %v", testVpaID)
+			for containerName, aggregation := range vpa.aggregateContainerStates {
+				assert.Equal(t, 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
@@ -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.