Merge pull request #5864 from kwiesmueller/master

Record all vpa api versions in recommender metrics

vertical-pod-autoscaler/pkg/recommender/model/cluster.go

@@ -279,6 +279,7 @@ func (cluster *ClusterState) AddOrUpdateVpa(apiObject *vpa_types.VerticalPodAuto
 	vpa.Recommendation = currentRecommendation
 	vpa.SetUpdateMode(apiObject.Spec.UpdatePolicy)
 	vpa.SetResourcePolicy(apiObject.Spec.ResourcePolicy)
+	vpa.SetAPIVersion(apiObject.GetObjectKind().GroupVersionKind().Version)
 	return nil
 }
 

vertical-pod-autoscaler/pkg/recommender/model/cluster_test.go

@@ -450,6 +450,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 		resourcePolicy      *vpa_types.PodResourcePolicy
 		expectedScalingMode *vpa_types.ContainerScalingMode
 		expectedUpdateMode  *vpa_types.UpdateMode
+		expectedAPIVersion  string
 	}{
 		{
 			name:   "Defaults to auto",
@@ -459,6 +460,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			// hence the UpdateModeOff does not influence container scaling mode here.
 			expectedScalingMode: &scalingModeAuto,
 			expectedUpdateMode:  &updateModeOff,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:   "Default scaling mode set to Off",
 			oldVpa: nil,
@@ -473,6 +475,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			},
 			expectedScalingMode: &scalingModeOff,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:   "Explicit scaling mode set to Off",
 			oldVpa: nil,
@@ -487,6 +490,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			},
 			expectedScalingMode: &scalingModeOff,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:   "Other container has explicit scaling mode Off",
 			oldVpa: nil,
@@ -501,6 +505,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			},
 			expectedScalingMode: &scalingModeAuto,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:   "Scaling mode to default Off",
 			oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
@@ -515,6 +520,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			},
 			expectedScalingMode: &scalingModeOff,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:   "Scaling mode to explicit Off",
 			oldVpa: testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
@@ -529,6 +535,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			},
 			expectedScalingMode: &scalingModeOff,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		},
 		// Tests checking changes to UpdateMode.
 		{
@@ -537,12 +544,49 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 			newVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
 			expectedScalingMode: &scalingModeAuto,
 			expectedUpdateMode:  &updateModeAuto,
+			expectedAPIVersion:  "v1",
 		}, {
 			name:                "UpdateMode from Auto to Off",
 			oldVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeAuto).Get(),
 			newVpa:              testVpaBuilder.WithUpdateMode(vpa_types.UpdateModeOff).Get(),
 			expectedScalingMode: &scalingModeAuto,
 			expectedUpdateMode:  &updateModeOff,
+			expectedAPIVersion:  "v1",
+		},
+		// Test different API versions being recorded.
+		// Note that this path for testing the apiVersions is not actively exercised
+		// in a running recommender. The GroupVersion is cleared before it reaches
+		// the recommenders code. These tests only test the propagation of version
+		// changes. When introducing new api versions that need to be differentiated
+		// in logic and/or metrics a dedicated detection mechanism is needed for
+		// those new versions. We can not get this information from the api request:
+		// https://github.com/kubernetes/kubernetes/pull/59264#issuecomment-362579495
+		{
+			name:                "Record APIVersion v1",
+			oldVpa:              nil,
+			newVpa:              testVpaBuilder.WithGroupVersion(metav1.GroupVersion(vpa_types.SchemeGroupVersion)).Get(),
+			expectedScalingMode: &scalingModeAuto,
+			expectedAPIVersion:  "v1",
+		},
+		{
+			name:   "Record APIVersion v1beta2",
+			oldVpa: nil,
+			newVpa: testVpaBuilder.WithGroupVersion(metav1.GroupVersion{
+				Group:   vpa_types.SchemeGroupVersion.Group,
+				Version: "v1beta2",
+			}).Get(),
+			expectedScalingMode: &scalingModeAuto,
+			expectedAPIVersion:  "v1beta2",
+		},
+		{
+			name:   "Record APIVersion v1beta1",
+			oldVpa: nil,
+			newVpa: testVpaBuilder.WithGroupVersion(metav1.GroupVersion{
+				Group:   vpa_types.SchemeGroupVersion.Group,
+				Version: "v1beta1",
+			}).Get(),
+			expectedScalingMode: &scalingModeAuto,
+			expectedAPIVersion:  "v1beta1",
 		},
 	}
 	for _, tc := range cases {
@@ -572,6 +616,7 @@ func TestAddOrUpdateVPAPolicies(t *testing.T) {
 				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)
 			}
+			assert.Equal(t, tc.expectedAPIVersion, vpa.APIVersion)
 		})
 	}
 }

vertical-pod-autoscaler/pkg/recommender/model/vpa.go

@@ -104,8 +104,8 @@ type Vpa struct {
 	Created time.Time
 	// CheckpointWritten indicates when last checkpoint for the VPA object was stored.
 	CheckpointWritten time.Time
-	// IsV1Beta1API is set to true if VPA object has labelSelector defined as in v1beta1 api.
-	IsV1Beta1API bool
+	// APIVersion of the VPA object.
+	APIVersion string
 	// TargetRef points to the controller managing the set of pods.
 	TargetRef *autoscaling.CrossVersionObjectReference
 	// PodCount contains number of live Pods matching a given VPA object.
@@ -123,12 +123,27 @@ func NewVpa(id VpaID, selector labels.Selector, created time.Time) *Vpa {
 		Created:                         created,
 		Annotations:                     make(vpaAnnotationsMap),
 		Conditions:                      make(vpaConditionsMap),
-		IsV1Beta1API:                    false,
-		PodCount:                        0,
+		// APIVersion defaults to the version of the client used to read resources.
+		// If a new version is introduced that needs to be differentiated beyond the
+		// client conversion, this needs to be done based on the resource content.
+		// The K8s client will not return the resource apiVersion as it's converted
+		// to the version requested by the client server side.
+		APIVersion: vpa_types.SchemeGroupVersion.Version,
+		PodCount:   0,
 	}
 	return vpa
 }
 
+// SetAPIVersion to the version of the VPA API object.
+// Default API Version is the API version of the VPA client.
+// If the provided version is empty, no change is made.
+func (vpa *Vpa) SetAPIVersion(to string) {
+	if to == "" {
+		return
+	}
+	vpa.APIVersion = to
+}
+
 // UseAggregationIfMatching checks if the given aggregation matches (contributes to) this VPA
 // and adds it to the set of VPA's aggregations if that is the case.
 func (vpa *Vpa) UseAggregationIfMatching(aggregationKey AggregateStateKey, aggregation *AggregateContainerState) {

vertical-pod-autoscaler/pkg/utils/metrics/recommender/recommender.go

@@ -33,7 +33,14 @@ const (
 )
 
 var (
-	modes = []string{string(vpa_types.UpdateModeOff), string(vpa_types.UpdateModeInitial), string(vpa_types.UpdateModeRecreate), string(vpa_types.UpdateModeAuto)}
+	// TODO: unify this list with the types defined in the VPA handler to avoid
+	// drift if one file is changed and the other one is missed.
+	modes = []string{
+		string(vpa_types.UpdateModeOff),
+		string(vpa_types.UpdateModeInitial),
+		string(vpa_types.UpdateModeRecreate),
+		string(vpa_types.UpdateModeAuto),
+	}
 )
 
 type apiVersion string
@@ -150,20 +157,15 @@ func NewObjectCounter() *ObjectCounter {
 
 // Add updates the helper state to include the given VPA object
 func (oc *ObjectCounter) Add(vpa *model.Vpa) {
-	mode := string(vpa_types.UpdateModeAuto)
+	mode := vpa_types.UpdateModeAuto
 	if vpa.UpdateMode != nil && string(*vpa.UpdateMode) != "" {
-		mode = string(*vpa.UpdateMode)
-	}
-	// TODO: Maybe report v1 version as well.
-	api := v1beta2
-	if vpa.IsV1Beta1API {
-		api = v1beta1
+		mode = *vpa.UpdateMode
 	}
 
 	key := objectCounterKey{
-		mode:              mode,
+		mode:              string(mode),
 		has:               vpa.HasRecommendation(),
-		apiVersion:        api,
+		apiVersion:        apiVersion(vpa.APIVersion),
 		matchesPods:       vpa.HasMatchedPods(),
 		unsupportedConfig: vpa.Conditions.ConditionActive(vpa_types.ConfigUnsupported),
 	}