Repository: mercari/tortoise (go)

Issue:
Tortoise shouldn't generate any recommendation when VPA has 0 recommendation
When VPA cannot calculate the recommendation, it set `RecommendationProvided` `False` in VPA condition.
If Tortoise see such VPAs, it should ignore them.

Pull request:
validate VPA's recommendation
<!--  
Thanks for sending a pull request!  
Please read the CLA carefully before submitting your contribution to Mercari. 
Under any circumstances, by submitting your contribution, you are deemed to accept and agree to be bound by the terms and conditions of the CLA.
https://www.mercari.com/cla/

Also, please read the contributor guide, which contains some general rules and suggestions:
https://github.com/mercari/tortoise/blob/main/docs/contributor-guide.md
-->

#### What this PR does / why we need it:

validate the VPA's recommendation so that Tortoise generates the recommendation only when VPA is valid.

#### Which issue(s) this PR fixes:

<!--
Before implementing any feature changes, please raise the issue and discuss what you propose with maintainers.
-->

Fixes #331

#### Special notes for your reviewer:


Patch:
diff --git a/pkg/recommender/recommender.go b/pkg/recommender/recommender.go
index 601bed2..34ddc8a 100644
--- a/pkg/recommender/recommender.go
+++ b/pkg/recommender/recommender.go
@@ -221,7 +221,7 @@ func (s *Service) calculateBestNewSize(ctx context.Context, p v1beta3.Autoscalin
 
 	if !isMultipleContainersPod {
 		// nothing else to do for a single container Pod.
-		return s.justifyNewSizeByMaxMin(resourceRequest.MilliValue(), k, minAllocatedResources, containerName), "", nil
+		return s.justifyNewSizeByMaxMin(resourceRequest.MilliValue(), k, minAllocatedResources, containerName), "nothing to do", nil
 	}
 
 	targetUtilizationValue, err := hpaservice.GetHPATargetValue(ctx, hpa, containerName, k)
@@ -240,12 +240,12 @@ func (s *Service) calculateBestNewSize(ctx context.Context, p v1beta3.Autoscalin
 		// so that the upper usage will be the target usage.
 		newSize := int64(float64(recommendedResourceRequest.MilliValue()) * 100.0 / float64(targetUtilizationValue))
 		jastified := s.justifyNewSizeByMaxMin(newSize, k, minAllocatedResources, containerName)
-		return jastified, fmt.Sprintf("the current resource utilization (%v) is too small and it's due to unbalanced container size, so make %v request (%v) smaller (%v → %v) based on VPA's recommendation and HPA target utilization %v%%", int(upperUtilization), k, containerName, resourceRequest.MilliValue(), jastified, targetUtilizationValue), nil
+		return jastified, fmt.Sprintf("the current resource usage (%v, %v%%) is too small and it's due to unbalanced container size, so make %v request (%v) smaller (%v → %v) based on VPA's recommendation and HPA target utilization %v%%", recommendedResourceRequest.MilliValue(), int(upperUtilization), k, containerName, resourceRequest.MilliValue(), jastified, targetUtilizationValue), nil
 	}
 
 	// Just keep the current resource request.
 	// Only do justification.
-	return s.justifyNewSizeByMaxMin(resourceRequest.MilliValue(), k, minAllocatedResources, containerName), "", nil
+	return s.justifyNewSizeByMaxMin(resourceRequest.MilliValue(), k, minAllocatedResources, containerName), "nothing to do", nil
 }
 
 func (s *Service) getGlobalMinResourceSize(k corev1.ResourceName, containerName string) resource.Quantity {
diff --git a/pkg/vpa/service.go b/pkg/vpa/service.go
index 1fc128a..c6cd071 100644
--- a/pkg/vpa/service.go
+++ b/pkg/vpa/service.go
@@ -434,13 +434,36 @@ func (c *Service) GetTortoiseMonitorVPA(ctx context.Context, tortoise *autoscali
 		return nil, false, fmt.Errorf("failed to get updater vpa on tortoise: %w", err)
 	}
 
+	return vpa, isMonitorVPAReady(vpa, tortoise), nil
+}
+
+func isMonitorVPAReady(vpa *v1.VerticalPodAutoscaler, tortoise *autoscalingv1beta3.Tortoise) bool {
+	provided := false
 	for _, c := range vpa.Status.Conditions {
 		if c.Type == v1.RecommendationProvided && c.Status == corev1.ConditionTrue {
-			return vpa, true, nil
+			provided = true
+		}
+	}
+	if !provided {
+		return false
+	}
+
+	// Check if VPA has the recommendation for all the containers registered in the tortoise.
+	containerInTortoise := sets.New[string]()
+	for _, p := range tortoise.Status.AutoscalingPolicy {
+		containerInTortoise.Insert(p.ContainerName)
+	}
+
+	containerInVPA := sets.New[string]()
+	for _, c := range vpa.Status.Recommendation.ContainerRecommendations {
+		containerInVPA.Insert(c.ContainerName)
+		if c.Target.Cpu().IsZero() || c.Target.Memory().IsZero() {
+			// something wrong with the recommendation.
+			return false
 		}
 	}
 
-	return vpa, false, nil
+	return containerInTortoise.Equal(containerInVPA)
 }
 
 func SetAllVerticalContainerResourcePhaseWorking(tortoise *autoscalingv1beta3.Tortoise, now time.Time) *autoscalingv1beta3.Tortoise {


Test patch:
diff --git a/pkg/vpa/service_test.go b/pkg/vpa/service_test.go
index 47670a0..57aa661 100644
--- a/pkg/vpa/service_test.go
+++ b/pkg/vpa/service_test.go
@@ -1542,3 +1542,168 @@ func TestService_UpdateVPAFromTortoiseRecommendation(t *testing.T) {
 		})
 	}
 }
+
+func Test_isMonitorVPAReady(t *testing.T) {
+	type args struct {
+		vpa      *vpav1.VerticalPodAutoscaler
+		tortoise *autoscalingv1beta3.Tortoise
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{
+			name: "VPA is not ready",
+			args: args{
+				vpa: &vpav1.VerticalPodAutoscaler{
+					Status: vpav1.VerticalPodAutoscalerStatus{
+						Conditions: []vpav1.VerticalPodAutoscalerCondition{
+							{
+								Type:   vpav1.RecommendationProvided,
+								Status: v1.ConditionFalse,
+							},
+						},
+					},
+				},
+			},
+			want: false,
+		},
+		{
+			name: "VPA doesn't have a recommendation for a container",
+			args: args{
+				vpa: &vpav1.VerticalPodAutoscaler{
+					Status: vpav1.VerticalPodAutoscalerStatus{
+						Conditions: []vpav1.VerticalPodAutoscalerCondition{
+							{
+								Type:   vpav1.RecommendationProvided,
+								Status: v1.ConditionFalse,
+							},
+						},
+						Recommendation: &vpav1.RecommendedPodResources{
+							ContainerRecommendations: []vpav1.RecommendedContainerResources{
+								{
+									ContainerName: "app",
+									Target: v1.ResourceList{
+										v1.ResourceMemory: resource.MustParse("1Gi"),
+										v1.ResourceCPU:    resource.MustParse("1"),
+									},
+								},
+								// no istio
+							},
+						},
+					},
+				},
+				tortoise: &autoscalingv1beta3.Tortoise{
+					Status: autoscalingv1beta3.TortoiseStatus{
+						AutoscalingPolicy: []autoscalingv1beta3.ContainerAutoscalingPolicy{
+							{
+								ContainerName: "app",
+							},
+							{
+								ContainerName: "istio",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "VPA has a invalid recommendation for a container",
+			args: args{
+				vpa: &vpav1.VerticalPodAutoscaler{
+					Status: vpav1.VerticalPodAutoscalerStatus{
+						Conditions: []vpav1.VerticalPodAutoscalerCondition{
+							{
+								Type:   vpav1.RecommendationProvided,
+								Status: v1.ConditionFalse,
+							},
+						},
+						Recommendation: &vpav1.RecommendedPodResources{
+							ContainerRecommendations: []vpav1.RecommendedContainerResources{
+								{
+									ContainerName: "app",
+									Target: v1.ResourceList{
+										v1.ResourceMemory: resource.MustParse("1Gi"),
+										v1.ResourceCPU:    resource.MustParse("0"), // wrong
+									},
+								},
+								{
+									ContainerName: "istio",
+									Target: v1.ResourceList{
+										v1.ResourceMemory: resource.MustParse("1Gi"),
+										v1.ResourceCPU:    resource.MustParse("1"),
+									},
+								},
+							},
+						},
+					},
+				},
+				tortoise: &autoscalingv1beta3.Tortoise{
+					Status: autoscalingv1beta3.TortoiseStatus{
+						AutoscalingPolicy: []autoscalingv1beta3.ContainerAutoscalingPolicy{
+							{
+								ContainerName: "app",
+							},
+							{
+								ContainerName: "istio",
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			name: "VPA is ready",
+			args: args{
+				vpa: &vpav1.VerticalPodAutoscaler{
+					Status: vpav1.VerticalPodAutoscalerStatus{
+						Conditions: []vpav1.VerticalPodAutoscalerCondition{
+							{
+								Type:   vpav1.RecommendationProvided,
+								Status: v1.ConditionFalse,
+							},
+						},
+						Recommendation: &vpav1.RecommendedPodResources{
+							ContainerRecommendations: []vpav1.RecommendedContainerResources{
+								{
+									ContainerName: "app",
+									Target: v1.ResourceList{
+										v1.ResourceMemory: resource.MustParse("1Gi"),
+										v1.ResourceCPU:    resource.MustParse("1"), // wrong
+									},
+								},
+								{
+									ContainerName: "istio",
+									Target: v1.ResourceList{
+										v1.ResourceMemory: resource.MustParse("1Gi"),
+										v1.ResourceCPU:    resource.MustParse("1"),
+									},
+								},
+							},
+						},
+					},
+				},
+				tortoise: &autoscalingv1beta3.Tortoise{
+					Status: autoscalingv1beta3.TortoiseStatus{
+						AutoscalingPolicy: []autoscalingv1beta3.ContainerAutoscalingPolicy{
+							{
+								ContainerName: "app",
+							},
+							{
+								ContainerName: "istio",
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			if got := isMonitorVPAReady(tt.args.vpa, tt.args.tortoise); got != tt.want {
+				t.Errorf("isMonitorVPAReady() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}