fix vpa condition RecommendationApplied

kubewharf · May 6, 2023 · 0147b67 · 0147b67
1 parent 4937825
commit 0147b67
Show file tree

Hide file tree

Showing 5 changed files with 54 additions and 28 deletions.
diff --git a/pkg/controller/vpa/util/api.go b/pkg/controller/vpa/util/api.go
@@ -40,6 +40,8 @@ const (
 const (
 	VPAConditionReasonUpdated           = "Updated"
 	VPAConditionReasonCalculatedIllegal = "Illegal"
+	VPAConditionReasonPodSpecUpdated    = "PodSpecUpdated"
+	VPAConditionReasonPodSpecNoUpdate   = "PodSpecNoUpdate"
 )
 
 // SetVPAConditions is used to set conditions for vpa in local vpa

diff --git a/pkg/controller/vpa/vpa_status.go b/pkg/controller/vpa/vpa_status.go
@@ -298,6 +298,14 @@ func (vs *vpaStatusController) syncVPA(key string) error {
 	vpaNew.Status.PodResources = vpaPodResources
 	vpaNew.Status.ContainerResources = vpaContainerResources
 
+	// set RecommendationApplied condition, based on whether all pods for this vpa
+	// are updated to the expected resources in their annotations
+	err = vs.setRecommendationAppliedCondition(vpaNew, pods)
+	if err != nil {
+		klog.Errorf("[vpa-status] set recommendation applied condition failed: %v", err)
+		return err
+	}
+
 	// skip to update status if no change happened
 	if apiequality.Semantic.DeepEqual(vpa.Status, vpaNew.Status) {
 		return nil
@@ -311,3 +319,29 @@ func (vs *vpaStatusController) syncVPA(key string) error {
 
 	return nil
 }
+
+// setRecommendationAppliedCondition set vpa recommendation applied condition by checking all pods whether
+// are updated to the expected resources in their annotations
+func (vs *vpaStatusController) setRecommendationAppliedCondition(vpa *apis.KatalystVerticalPodAutoscaler, pods []*v1.Pod) error {
+	allPodSpecUpdated := func() bool {
+		for _, pod := range pods {
+			if !katalystutil.CheckPodSpecUpdated(pod) {
+				return false
+			}
+		}
+		return true
+	}()
+
+	if allPodSpecUpdated {
+		err := util.SetVPAConditions(vpa, apis.RecommendationApplied, v1.ConditionTrue, util.VPAConditionReasonPodSpecUpdated, "")
+		if err != nil {
+			return err
+		}
+	} else {
+		err := util.SetVPAConditions(vpa, apis.RecommendationApplied, v1.ConditionFalse, util.VPAConditionReasonPodSpecNoUpdate, "")
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}
diff --git a/pkg/controller/vpa/vparec.go b/pkg/controller/vpa/vparec.go
@@ -446,25 +446,15 @@ func (rec *VerticalPodAutoScaleRecommendationController) updateVPAStatus(vpa *ap
 	vpaNew.Status.PodResources = vpaPodResources
 	vpaNew.Status.ContainerResources = vpaContainerResources
 	if apiequality.Semantic.DeepEqual(vpaNew.Status, vpa.Status) {
-		for _, condition := range vpaNew.Status.Conditions {
-			if condition.Type == apis.RecommendationUpdated && condition.Status == core.ConditionTrue {
-				return nil
-			}
-		}
-
-		return util.PatchVPAConditions(rec.ctx, rec.vpaUpdater, vpa, apis.RecommendationApplied, core.ConditionTrue, util.VPARecConditionReasonUpdated, "")
+		return nil
 	}
 
-	updated, err := rec.vpaUpdater.PatchVPAStatus(rec.ctx, vpa, vpaNew)
+	_, err := rec.vpaUpdater.PatchVPAStatus(rec.ctx, vpa, vpaNew)
 	if err != nil {
 		klog.Errorf("[vpa-rec] failed to set vpa %v status: %v", vpa.Name, err)
-		if err := util.PatchVPAConditions(rec.ctx, rec.vpaUpdater, vpa, apis.RecommendationApplied, core.ConditionFalse, util.VPAConditionReasonCalculatedIllegal, err.Error()); err != nil {
-			return err
-		}
-
 		return err
 	}
-	return util.PatchVPAConditions(rec.ctx, rec.vpaUpdater, updated, apis.RecommendationApplied, core.ConditionTrue, util.VPARecConditionReasonUpdated, "")
+	return nil
 }
 
 // updateVPAStatus is used to set status for vpaRec

diff --git a/pkg/controller/vpa/vparec_test.go b/pkg/controller/vpa/vparec_test.go
@@ -122,13 +122,6 @@ func TestVPARecControllerSyncVPA(t *testing.T) {
 							},
 						},
 					},
-					Conditions: []apis.VerticalPodAutoscalerCondition{
-						{
-							Type:   apis.RecommendationApplied,
-							Status: v1.ConditionTrue,
-							Reason: util.VPAConditionReasonUpdated,
-						},
-					},
 				},
 			},
 			vparecs: []*apis.VerticalPodAutoscalerRecommendation{
@@ -255,13 +248,6 @@ func TestVPARecControllerSyncVPA(t *testing.T) {
 							},
 						},
 					},
-					Conditions: []apis.VerticalPodAutoscalerCondition{
-						{
-							Type:   apis.RecommendationApplied,
-							Status: v1.ConditionTrue,
-							Reason: util.VPAConditionReasonUpdated,
-						},
-					},
 				},
 			},
 			vparecs: []*apis.VerticalPodAutoscalerRecommendation{
@@ -361,7 +347,6 @@ func TestVPARecControllerSyncVPA(t *testing.T) {
 			vpa, err := controlCtx.Client.InternalClient.AutoscalingV1alpha1().
 				KatalystVerticalPodAutoscalers(tc.vpaNew.Namespace).Get(context.TODO(), tc.vpaNew.Name, metav1.GetOptions{})
 			assert.NoError(t, err)
-			vpa.Status.Conditions[0].LastTransitionTime = metav1.Time{}
 			assert.Equal(t, tc.vpaNew, vpa)
 		})
 	}

diff --git a/pkg/util/vpa.go b/pkg/util/vpa.go
@@ -21,6 +21,7 @@ import (
 
 	"github.com/pkg/errors"
 	core "k8s.io/api/core/v1"
+	apiequality "k8s.io/apimachinery/pkg/api/equality"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/api/meta"
 	"k8s.io/apimachinery/pkg/api/resource"
@@ -418,3 +419,17 @@ func CheckVPAStatusLegal(vpa *apis.KatalystVerticalPodAutoscaler, pods []*core.P
 	}
 	return true, "", nil
 }
+
+// CheckPodSpecUpdated checks pod spec whether is updated to expected resize resource in annotation
+func CheckPodSpecUpdated(pod *core.Pod) bool {
+	podCopy := &core.Pod{}
+	podCopy.Spec.Containers = native.DeepCopyPodContainers(pod)
+	annotationResource, err := GenerateVPAPodResizeResourceAnnotations(pod, nil, nil)
+	if err != nil {
+		klog.Errorf("failed to exact pod %v resize resource annotation from container resource: %v", pod.Name, err)
+		return false
+	}
+
+	native.ApplyPodResources(annotationResource, podCopy)
+	return apiequality.Semantic.DeepEqual(pod.Spec.Containers, podCopy.Spec.Containers)
+}