Merge pull request #2745 from bskiba/mode-off

Propagate scaling mode to containers. Correctly update ResourcePolicy and UpdatePolicy on update.

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()
+}

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())
+		})
+	}
+}

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
 }
 

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()

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.