Repository: mercari/tortoise
Pull request #249 (2023-12-28 10:13:33)

draft fix to make VPA min resource mutable
<!--  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/
-->

#### What this PR does / why we need it:


#### Which issue(s) this PR fixes:

<!--
Before implementing any feature changes, please raise the issue and discuss what you propose with maintainers.
-->

Fixes #

#### Special notes for your reviewer:



Patch:
diff --git a/controllers/tortoise_controller.go b/controllers/tortoise_controller.go
index d115c24..783d86a 100644
--- a/controllers/tortoise_controller.go
+++ b/controllers/tortoise_controller.go
@@ -195,6 +195,12 @@ func (r *TortoiseReconciler) Reconcile(ctx context.Context, req ctrl.Request) (_
 		return ctrl.Result{RequeueAfter: r.Interval}, nil
 	}
 
+	_, err = r.VpaService.UpdateVPAContainerResourcePolicy(ctx, tortoise, monitorvpa)
+	if err != nil {
+		logger.Error(err, "update VPA Container Resource Policy", "tortoise", req.NamespacedName)
+		return ctrl.Result{}, err
+	}
+
 	// VPA is ready, we mark all Vertical scaling resources as Running.
 	tortoise = vpa.SetAllVerticalContainerResourcePhaseWorking(tortoise, now)
 
diff --git a/pkg/vpa/service.go b/pkg/vpa/service.go
index 2c69c5b..8dbd627 100644
--- a/pkg/vpa/service.go
+++ b/pkg/vpa/service.go
@@ -153,6 +153,31 @@ func (c *Service) CreateTortoiseUpdaterVPA(ctx context.Context, tortoise *autosc
 	return vpa, tortoise, nil
 }
 
+// UpdateVPAContainerResourcePolicy is update VPAs to have appropriate container policies based on tortoises' resource policy.
+func (c *Service) UpdateVPAContainerResourcePolicy(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise, vpa *v1.VerticalPodAutoscaler) (*v1.VerticalPodAutoscaler, error) {
+	retVPA := &v1.VerticalPodAutoscaler{}
+	var err error
+
+	updateFn := func() error {
+		crp := make([]v1.ContainerResourcePolicy, 0, len(tortoise.Spec.ResourcePolicy))
+		for _, c := range tortoise.Spec.ResourcePolicy {
+			crp = append(crp, v1.ContainerResourcePolicy{
+				ContainerName: c.ContainerName,
+				MinAllowed:    c.MinAllocatedResources,
+			})
+		}
+		vpa.Spec.ResourcePolicy = &v1.PodResourcePolicy{ContainerPolicies: crp}
+		retVPA, err = c.c.AutoscalingV1().VerticalPodAutoscalers(vpa.Namespace).Update(ctx, vpa, metav1.UpdateOptions{})
+		return err
+	}
+
+	if err := retry.RetryOnConflict(retry.DefaultRetry, updateFn); err != nil {
+		return retVPA, fmt.Errorf("update VPA ContainerResourcePolicy status: %w", err)
+	}
+
+	return retVPA, nil
+}
+
 func (c *Service) CreateTortoiseMonitorVPA(ctx context.Context, tortoise *autoscalingv1beta3.Tortoise) (*v1.VerticalPodAutoscaler, *autoscalingv1beta3.Tortoise, error) {
 	off := v1.UpdateModeOff
 	vpa := &v1.VerticalPodAutoscaler{


Test patch:
diff --git a/pkg/vpa/service_test.go b/pkg/vpa/service_test.go
index 07685e0..9844706 100644
--- a/pkg/vpa/service_test.go
+++ b/pkg/vpa/service_test.go
@@ -141,6 +141,235 @@ func TestMakeAllVerticalContainerResourcePhaseRunning(t *testing.T) {
 	}
 }
 
+func TestVPAContainerResourcePolicy(t *testing.T) {
+	now := metav1.NewTime(time.Date(2022, 1, 1, 1, 1, 1, 1, time.UTC))
+
+	type args struct {
+		ctx          context.Context
+		initTortoise *autoscalingv1beta3.Tortoise
+		tortoise     *autoscalingv1beta3.Tortoise
+		now          time.Time
+	}
+	tests := []struct {
+		name       string
+		args       args
+		initialVPA *vpav1.VerticalPodAutoscaler
+		want       *vpav1.VerticalPodAutoscaler
+		wantErr    bool
+	}{
+		{
+			name: "modified correctly",
+			args: args{
+				ctx: context.Background(),
+				initTortoise: &autoscalingv1beta3.Tortoise{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "tortoise",
+						Namespace: "default",
+					},
+					Status: autoscalingv1beta3.TortoiseStatus{
+						AutoscalingPolicy: []autoscalingv1beta3.ContainerAutoscalingPolicy{
+							{
+								ContainerName: "app",
+								Policy: map[v1.ResourceName]v1beta3.AutoscalingType{
+									v1.ResourceMemory: v1beta3.AutoscalingTypeVertical,
+									v1.ResourceCPU:    v1beta3.AutoscalingTypeHorizontal,
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								Policy: map[v1.ResourceName]v1beta3.AutoscalingType{
+									v1.ResourceMemory: v1beta3.AutoscalingTypeVertical,
+									v1.ResourceCPU:    v1beta3.AutoscalingTypeHorizontal,
+								},
+							},
+						},
+						ContainerResourcePhases: []autoscalingv1beta3.ContainerResourcePhases{
+							{
+								ContainerName: "app",
+								ResourcePhases: map[v1.ResourceName]autoscalingv1beta3.ResourcePhase{
+									v1.ResourceCPU: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+									v1.ResourceMemory: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								ResourcePhases: map[v1.ResourceName]autoscalingv1beta3.ResourcePhase{
+									v1.ResourceCPU: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+									v1.ResourceMemory: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+								},
+							},
+						},
+					},
+					Spec: autoscalingv1beta3.TortoiseSpec{
+						ResourcePolicy: []autoscalingv1beta3.ContainerResourcePolicy{
+							{
+								ContainerName: "app",
+								MinAllocatedResources: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("0.5Gi"),
+									v1.ResourceCPU:    resource.MustParse("0.5"),
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								MinAllocatedResources: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("0.5Gi"),
+									v1.ResourceCPU:    resource.MustParse("0.5"),
+								},
+							},
+						},
+					},
+				},
+				tortoise: &autoscalingv1beta3.Tortoise{
+					ObjectMeta: metav1.ObjectMeta{
+						Name:      "tortoise",
+						Namespace: "default",
+					},
+					Status: autoscalingv1beta3.TortoiseStatus{
+						AutoscalingPolicy: []autoscalingv1beta3.ContainerAutoscalingPolicy{
+							{
+								ContainerName: "app",
+								Policy: map[v1.ResourceName]v1beta3.AutoscalingType{
+									v1.ResourceMemory: v1beta3.AutoscalingTypeVertical,
+									v1.ResourceCPU:    v1beta3.AutoscalingTypeHorizontal,
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								Policy: map[v1.ResourceName]v1beta3.AutoscalingType{
+									v1.ResourceMemory: v1beta3.AutoscalingTypeVertical,
+									v1.ResourceCPU:    v1beta3.AutoscalingTypeHorizontal,
+								},
+							},
+						},
+						ContainerResourcePhases: []autoscalingv1beta3.ContainerResourcePhases{
+							{
+								ContainerName: "app",
+								ResourcePhases: map[v1.ResourceName]autoscalingv1beta3.ResourcePhase{
+									v1.ResourceCPU: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+									v1.ResourceMemory: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								ResourcePhases: map[v1.ResourceName]autoscalingv1beta3.ResourcePhase{
+									v1.ResourceCPU: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+									v1.ResourceMemory: {
+										Phase: autoscalingv1beta3.ContainerResourcePhaseGatheringData,
+									},
+								},
+							},
+						},
+					},
+					Spec: autoscalingv1beta3.TortoiseSpec{
+						ResourcePolicy: []autoscalingv1beta3.ContainerResourcePolicy{
+							{
+								ContainerName: "app",
+								MinAllocatedResources: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("1Gi"),
+									v1.ResourceCPU:    resource.MustParse("1"),
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								MinAllocatedResources: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("1Gi"),
+									v1.ResourceCPU:    resource.MustParse("1"),
+								},
+							},
+						},
+					},
+				},
+				now: now.Time,
+			},
+			initialVPA: &vpav1.VerticalPodAutoscaler{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "tortoise-updater-tortoise",
+					Namespace: "default",
+				},
+				Spec: vpav1.VerticalPodAutoscalerSpec{
+					ResourcePolicy: &vpav1.PodResourcePolicy{
+						ContainerPolicies: []vpav1.ContainerResourcePolicy{
+							{
+								ContainerName: "app",
+								MinAllowed: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("0.5Gi"),
+									v1.ResourceCPU:    resource.MustParse("0.5"),
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								MinAllowed: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("0.5Gi"),
+									v1.ResourceCPU:    resource.MustParse("0.5"),
+								},
+							},
+						},
+					},
+				},
+			},
+			want: &vpav1.VerticalPodAutoscaler{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      "tortoise-updater-tortoise",
+					Namespace: "default",
+				},
+				Spec: vpav1.VerticalPodAutoscalerSpec{
+					ResourcePolicy: &vpav1.PodResourcePolicy{
+						ContainerPolicies: []vpav1.ContainerResourcePolicy{
+							{
+								ContainerName: "app",
+								MinAllowed: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("1Gi"),
+									v1.ResourceCPU:    resource.MustParse("1"),
+								},
+							},
+							{
+								ContainerName: "istio-proxy",
+								MinAllowed: v1.ResourceList{
+									v1.ResourceMemory: resource.MustParse("1Gi"),
+									v1.ResourceCPU:    resource.MustParse("1"),
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			c := &Service{
+				c:        fake.NewSimpleClientset(tt.initialVPA),
+				recorder: record.NewFakeRecorder(10),
+			}
+
+			got, err := c.UpdateVPAContainerResourcePolicy(tt.args.ctx, tt.args.tortoise, tt.initialVPA)
+
+			if (err != nil) != tt.wantErr {
+				t.Errorf("UpdateVPAContainerResourcePolicy error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+
+			if d := cmp.Diff(tt.want.Spec, got.Spec); d != "" {
+				t.Errorf("UpdateVPAContainerResourcePolicy vpa diff = %v", d)
+			}
+		})
+	}
+}
+
 func TestService_UpdateVPAFromTortoiseRecommendation(t *testing.T) {
 	tests := []struct {
 		name       string