Merge pull request #633 from kgrygiel/master

Support for Conditions in VPA.

vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1/types.go

@@ -88,6 +88,8 @@ type VerticalPodAutoscalerStatus struct {
 type VerticalPodAutoscalerConditionType string
 
 var (
+	// Configured indicates whether the VPA recommender was able to load a valid VPA spec.
+	Configured VerticalPodAutoscalerConditionType = "Configured"
 	// RecommendationProvided indicates whether the VPA recommender was able to calculate a recommendation.
 	RecommendationProvided VerticalPodAutoscalerConditionType = "RecommendationProvided"
 )

vertical-pod-autoscaler/pkg/utils/vpa/api.go

@@ -18,6 +18,7 @@ package api
 
 import (
 	"encoding/json"
+	"fmt"
 	"time"
 
 	"github.com/golang/glog"
@@ -30,6 +31,7 @@ import (
 	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/poc.autoscaling.k8s.io/v1alpha1"
 	vpa_lister "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/client/listers/poc.autoscaling.k8s.io/v1alpha1"
+	"k8s.io/autoscaler/vertical-pod-autoscaler/recommender/model"
 	"k8s.io/client-go/tools/cache"
 )
 
@@ -49,32 +51,49 @@ func patchVpa(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpaName string,
 	return vpaClient.Patch(vpaName, types.JSONPatchType, bytes)
 }
 
-// InitVpaStatus inserts into VPA CRD object an empty VerticalPodAutoscalerStatus object, so later it can be filled with data.
-func InitVpaStatus(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpaName string) (result *vpa_types.VerticalPodAutoscaler, err error) {
-	return patchVpa(vpaClient, vpaName, []patchRecord{{
-		Op:    "add",
-		Path:  "/status",
-		Value: vpa_types.VerticalPodAutoscalerStatus{},
-	},
-	})
-}
+// UpdateVpaStatus updates the status field of the VPA API object.
+// It prevents race conditions by verifying that the lastUpdateTime of the
+// API object and its model representation are equal.
+func UpdateVpaStatus(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpa *model.Vpa) (result *vpa_types.VerticalPodAutoscaler, err error) {
+	newUpdateTime := time.Now()
+	if !newUpdateTime.After(vpa.LastUpdateTime) {
+		return nil, fmt.Errorf(
+			"Local time (%v) is behind lastUpdateTime (%v)",
+			newUpdateTime, vpa.LastUpdateTime)
+	}
+	status := vpa_types.VerticalPodAutoscalerStatus{
+		LastUpdateTime: metav1.NewTime(newUpdateTime),
+		Conditions:     vpa.Conditions.AsList(),
+	}
+	if vpa.Recommendation != nil {
+		status.Recommendation = *vpa.Recommendation
+	}
+
+	patches := make([]patchRecord, 0)
 
-// UpdateVpaRecommendation updates VPA's status/recommendation object and status/lastUpdateTime.
-func UpdateVpaRecommendation(vpaClient vpa_api.VerticalPodAutoscalerInterface, vpaName string, recommendation vpa_types.RecommendedPodResources) (result *vpa_types.VerticalPodAutoscaler, err error) {
-	patches := []patchRecord{
-		{
-			Op:    "add",
+	// Verify that Status was not updated in the meantime to avoid a race
+	// condition.
+	if !vpa.LastUpdateTime.IsZero() {
+		patches = append(patches, patchRecord{
+			Op:    "test",
 			Path:  "/status/lastUpdateTime",
-			Value: metav1.Time{time.Now()},
-		},
-		{
-			Op:    "add",
-			Path:  "/status/recommendation",
-			Value: recommendation,
-		},
+			Value: metav1.NewTime(vpa.LastUpdateTime),
+		})
+	} else {
+		// Status field not set yet.
+		patches = append(patches, patchRecord{
+			Op:    "test",
+			Path:  "/status",
+			Value: nil,
+		})
 	}
-	return patchVpa(vpaClient, vpaName, patches)
+	patches = append(patches, patchRecord{
+		Op:    "add",
+		Path:  "/status",
+		Value: status,
+	})
 
+	return patchVpa(vpaClient, (*vpa).ID.VpaName, patches)
 }
 
 // NewAllVpasLister returns VerticalPodAutoscalerLister configured to fetch all VPA objects.

vertical-pod-autoscaler/pkg/utils/vpa/api_test.go

@@ -17,7 +17,6 @@ limitations under the License.
 package api
 
 import (
-	"fmt"
 	"testing"
 	"time"
 
@@ -57,7 +56,6 @@ func TestPodMatchesVPA(t *testing.T) {
 
 	for _, tc := range testCases {
 		actual := PodMatchesVPA(tc.pod, tc.vpa)
-		fmt.Printf("Matching pod %v against vpa %v", tc.pod, tc.vpa)
 		assert.Equal(t, tc.result, actual)
 	}
 }

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

@@ -17,7 +17,11 @@ limitations under the License.
 package model
 
 import (
+	"fmt"
+	apiv1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	labels "k8s.io/apimachinery/pkg/labels"
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
 )
 
 // ClusterState holds all runtime information about the cluster required for the
@@ -146,9 +150,24 @@ func (cluster *ClusterState) AddSample(sample *ContainerUsageSampleWithKey) erro
 // didn't yet exist. If the VPA already existed but had a different pod
 // selector, the pod selector is updated. Updates the links between the VPA and
 // all pods it matches.
-func (cluster *ClusterState) AddOrUpdateVpa(vpaID VpaID, podSelectorStr string) error {
+func (cluster *ClusterState) AddOrUpdateVpa(apiObject *vpa_types.VerticalPodAutoscaler) error {
+	vpaID := VpaID{Namespace: apiObject.Namespace, VpaName: apiObject.Name}
+	conditionsMap := make(vpaConditionsMap)
+	for _, condition := range apiObject.Status.Conditions {
+		conditionsMap[condition.Type] = condition
+	}
+	var currentRecommendation *vpa_types.RecommendedPodResources
+	if conditionsMap[vpa_types.RecommendationProvided].Status == apiv1.ConditionTrue {
+		currentRecommendation = &apiObject.Status.Recommendation
+	}
+	selector, err := metav1.LabelSelectorAsSelector(apiObject.Spec.Selector)
+	if err != nil {
+		errMsg := fmt.Sprintf("couldn't convert selector into a corresponding internal selector object: %v", err)
+		conditionsMap.Set(vpa_types.Configured, false, "InvalidSelector", errMsg)
+	}
+
 	vpa, vpaExists := cluster.Vpas[vpaID]
-	if vpaExists && vpa.PodSelectorStr != podSelectorStr {
+	if vpaExists && (err != nil || vpa.PodSelector.String() != selector.String()) {
 		// Pod selector was changed. Delete the VPA object and recreate
 		// it with the new selector.
 		if err := cluster.DeleteVpa(vpaID); err != nil {
@@ -157,15 +176,15 @@ func (cluster *ClusterState) AddOrUpdateVpa(vpaID VpaID, podSelectorStr string)
 		vpaExists = false
 	}
 	if !vpaExists {
-		vpa, err := NewVpa(vpaID, podSelectorStr)
-		if err != nil {
-			return err
-		}
+		vpa = NewVpa(vpaID, selector)
 		cluster.Vpas[vpaID] = vpa
 		for _, pod := range cluster.Pods {
 			vpa.UpdatePodLink(pod)
 		}
 	}
+	vpa.Conditions = conditionsMap
+	vpa.Recommendation = currentRecommendation
+	vpa.LastUpdateTime = apiObject.Status.LastUpdateTime.Time
 	return nil
 }
 
@@ -176,7 +195,7 @@ func (cluster *ClusterState) DeleteVpa(vpaID VpaID) error {
 		return NewKeyError(vpaID)
 	}
 	// Change the selector to not match any pod and detach all pods.
-	vpa.SetPodSelectorStr("0=1")
+	vpa.PodSelector = nil
 	for _, pod := range vpa.Pods {
 		vpa.UpdatePodLink(pod)
 	}

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

@@ -20,7 +20,10 @@ import (
 	"testing"
 	"time"
 
+	"github.com/golang/glog"
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
 )
 
 var (
@@ -62,14 +65,25 @@ func TestMissingKeys(t *testing.T) {
 	assert.EqualError(t, err, "KeyError: {namespace-1 pod-1}")
 }
 
-func addTestPod(cluster *ClusterState) *PodState {
-	cluster.AddOrUpdatePod(testPodID, testLabels)
-	return cluster.Pods[testPodID]
+func addVpa(cluster *ClusterState, id VpaID, selector string) *Vpa {
+	var apiObject vpa_types.VerticalPodAutoscaler
+	apiObject.Namespace = id.Namespace
+	apiObject.Name = id.VpaName
+	apiObject.Spec.Selector, _ = metav1.ParseToLabelSelector(selector)
+	err := cluster.AddOrUpdateVpa(&apiObject)
+	if err != nil {
+		glog.Fatalf("AddOrUpdateVpa() failed: %v", err)
+	}
+	return cluster.Vpas[id]
 }
 
 func addTestVpa(cluster *ClusterState) *Vpa {
-	cluster.AddOrUpdateVpa(testVpaID, testSelectorStr)
-	return cluster.Vpas[testVpaID]
+	return addVpa(cluster, testVpaID, testSelectorStr)
+}
+
+func addTestPod(cluster *ClusterState) *PodState {
+	cluster.AddOrUpdatePod(testPodID, testLabels)
+	return cluster.Pods[testPodID]
 }
 
 // Creates a VPA followed by a matching pod. Verifies that the links between the
@@ -136,20 +150,17 @@ func TestUpdatePodSelector(t *testing.T) {
 	pod := addTestPod(cluster)
 
 	// Update the VPA selector such that it still matches the Pod.
-	assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 in (value-1,value-2)"))
-	vpa = cluster.Vpas[testVpaID]
+	vpa = addVpa(cluster, testVpaID, "label-1 in (value-1,value-2)")
 	assert.Equal(t, pod, vpa.Pods[testPodID])
 	assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas)
 
 	// Update the VPA selector to no longer match the Pod.
-	assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 = value-2"))
-	vpa = cluster.Vpas[testVpaID]
+	vpa = addVpa(cluster, testVpaID, "label-1 = value-2")
 	assert.Empty(t, vpa.Pods)
 	assert.Empty(t, pod.MatchingVpas)
 
 	// Update the VPA selector to match the Pod again.
-	assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, "label-1 = value-1"))
-	vpa = cluster.Vpas[testVpaID]
+	vpa = addVpa(cluster, testVpaID, "label-1 = value-1")
 	assert.Equal(t, pod, vpa.Pods[testPodID])
 	assert.Equal(t, map[VpaID]*Vpa{testVpaID: vpa}, pod.MatchingVpas)
 }
@@ -161,9 +172,9 @@ func TestUpdatePodSelector(t *testing.T) {
 // longer controlled by any VPA.
 func TestTwoVpasForPod(t *testing.T) {
 	cluster := NewClusterState()
-	cluster.AddOrUpdateVpa(VpaID{"namespace-1", "vpa-1"}, "label-1 = value-1")
+	addVpa(cluster, VpaID{"namespace-1", "vpa-1"}, "label-1 = value-1")
 	pod := addTestPod(cluster)
-	cluster.AddOrUpdateVpa(VpaID{"namespace-1", "vpa-2"}, "label-1 in (value-1,value-2)")
+	addVpa(cluster, VpaID{"namespace-1", "vpa-2"}, "label-1 in (value-1,value-2)")
 	assert.Equal(t, map[VpaID]*Vpa{
 		{"namespace-1", "vpa-1"}: cluster.Vpas[VpaID{"namespace-1", "vpa-1"}],
 		{"namespace-1", "vpa-2"}: cluster.Vpas[VpaID{"namespace-1", "vpa-2"}]},
@@ -184,7 +195,7 @@ func TestTwoVpasForPod(t *testing.T) {
 func TestEmptySelector(t *testing.T) {
 	cluster := NewClusterState()
 	// Create a VPA with an empty selector (matching all pods).
-	assert.NoError(t, cluster.AddOrUpdateVpa(testVpaID, ""))
+	addVpa(cluster, testVpaID, "")
 	// Create a pod with labels.
 	cluster.AddOrUpdatePod(testPodID, testLabels)
 	// Create a pod without labels.

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

@@ -17,55 +17,81 @@ limitations under the License.
 package model
 
 import (
+	apiv1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	vpa_types "k8s.io/autoscaler/vertical-pod-autoscaler/pkg/apis/poc.autoscaling.k8s.io/v1alpha1"
+	"time"
 )
 
+// Map from VPA condition type to condition.
+type vpaConditionsMap map[vpa_types.VerticalPodAutoscalerConditionType]vpa_types.VerticalPodAutoscalerCondition
+
+func (conditionsMap *vpaConditionsMap) Set(
+	conditionType vpa_types.VerticalPodAutoscalerConditionType,
+	status bool, reason string, message string) *vpaConditionsMap {
+	oldCondition, alreadyPresent := (*conditionsMap)[conditionType]
+	condition := vpa_types.VerticalPodAutoscalerCondition{
+		Type:    conditionType,
+		Reason:  reason,
+		Message: message,
+	}
+	if status {
+		condition.Status = apiv1.ConditionTrue
+	} else {
+		condition.Status = apiv1.ConditionFalse
+	}
+	if alreadyPresent && oldCondition.Status == condition.Status {
+		condition.LastTransitionTime = oldCondition.LastTransitionTime
+	} else {
+		condition.LastTransitionTime = metav1.Now()
+	}
+	(*conditionsMap)[conditionType] = condition
+	return conditionsMap
+}
+
+func (conditionsMap *vpaConditionsMap) AsList() []vpa_types.VerticalPodAutoscalerCondition {
+	conditions := make([]vpa_types.VerticalPodAutoscalerCondition, 0, len(*conditionsMap))
+	for _, condition := range *conditionsMap {
+		conditions = append(conditions, condition)
+	}
+	return conditions
+}
+
 // Vpa (Vertical Pod Autoscaler) object is responsible for vertical scaling of
 // Pods matching a given label selector.
 type Vpa struct {
 	ID VpaID
 	// Labels selector that determines which Pods are controlled by this VPA
 	// object. Can be nil, in which case no Pod is matched.
 	PodSelector labels.Selector
-	// Original (string) representation of the PodSelector.
-	PodSelectorStr string
+	// Map of the status conditions (keys are condition types).
+	Conditions vpaConditionsMap
+	// Most recently computed recommendation. Can be nil.
+	Recommendation *vpa_types.RecommendedPodResources
 	// Pods controlled by this VPA object.
 	Pods map[PodID]*PodState
+	// Value of the Status.LastUpdateTime fetched from the VPA API object.
+	LastUpdateTime time.Time
 }
 
 // NewVpa returns a new Vpa with a given ID and pod selector. Doesn't set the
 // links to the matched pods.
-func NewVpa(id VpaID, podSelectorStr string) (*Vpa, error) {
+func NewVpa(id VpaID, selector labels.Selector) *Vpa {
 	vpa := &Vpa{
-		id, nil, "",
-		make(map[PodID]*PodState), // Empty pods map.
-	}
-	if err := vpa.SetPodSelectorStr(podSelectorStr); err != nil {
-		return nil, err
-	}
-	return vpa, nil
-}
-
-// SetPodSelectorStr sets the pod selector of the VPA to the given value, passed
-// in the text format (see apimachinery/pkg/labels for the syntax). It returns
-// an error if the string cannot be parsed. Doesn't update the links to the
-// matched pods.
-func (vpa *Vpa) SetPodSelectorStr(podSelectorStr string) error {
-	podSelector, err := labels.Parse(podSelectorStr)
-	if err != nil {
-		return err
+		ID:          id,
+		PodSelector: selector,
+		Pods:        make(map[PodID]*PodState), // Empty pods map.
 	}
-	vpa.PodSelectorStr = podSelectorStr
-	vpa.PodSelector = podSelector
-	return nil
+	return vpa
 }
 
 // MatchesPod returns true iff a given pod is matched by the Vpa pod selector.
 func (vpa *Vpa) MatchesPod(pod *PodState) bool {
 	if vpa.ID.Namespace != pod.ID.Namespace {
 		return false
 	}
-	return pod.Labels != nil && vpa.PodSelector.Matches(pod.Labels)
+	return vpa.PodSelector != nil && pod.Labels != nil && vpa.PodSelector.Matches(pod.Labels)
 }
 
 // UpdatePodLink marks the Pod as controlled or not-controlled by the VPA