implement GoMemLimitModificationEnabled feature flag (#387)

mercari · Mar 21, 2024 · e3c41cf · e3c41cf
1 parent 690e0f9
commit e3c41cf
Show file tree

Hide file tree

Showing 3 changed files with 26 additions and 31 deletions.
diff --git a/pkg/features/features.go b/pkg/features/features.go
@@ -10,11 +10,8 @@ const (
 	VerticalScalingBasedOnPreferredMaxReplicas FeatureFlag = "VerticalScalingBasedOnPreferredMaxReplicas"
 
 	// Stage: alpha (default: disabled)
-	// Description: Enable the feature to modify GOMAXPROCS/GOMEMLIMIT based on the resource requests in the Pod mutating webhook.
-	// Tracked at:
-	// - https://github.com/mercari/tortoise/issues/319
-	// - https://github.com/mercari/tortoise/issues/320
-	GolangEnvModification FeatureFlag = "GolangEnvModification"
+	// Description: Enable the feature to modify GOMEMLIMIT based on the memory request in the Pod mutating webhook.
+	GoMemLimitModificationEnabled FeatureFlag = "GoMemLimitModificationEnabled"
 )
 
 func Contains(flags []FeatureFlag, flag FeatureFlag) bool {

diff --git a/pkg/pod/pod.go b/pkg/pod/pod.go
@@ -18,10 +18,10 @@ import (
 
 type Service struct {
 	// For example, if it's 3 and Pod's resource request is 100m, the limit will be changed to 300m.
-	resourceLimitMultiplier      map[string]int64
-	minimumCPULimit              resource.Quantity
-	controllerFetcher            controllerfetcher.ControllerFetcher
-	golangEnvModificationEnabled bool
+	resourceLimitMultiplier       map[string]int64
+	minimumCPULimit               resource.Quantity
+	controllerFetcher             controllerfetcher.ControllerFetcher
+	goMemLimitModificationEnabled bool
 }
 
 func New(
@@ -35,10 +35,10 @@ func New(
 	}
 	minCPULim := resource.MustParse(minimumCPULimit)
 	return &Service{
-		resourceLimitMultiplier:      resourceLimitMultiplier,
-		minimumCPULimit:              minCPULim,
-		controllerFetcher:            cf,
-		golangEnvModificationEnabled: features.Contains(featureFlags, features.GolangEnvModification),
+		resourceLimitMultiplier:       resourceLimitMultiplier,
+		minimumCPULimit:               minCPULim,
+		controllerFetcher:             cf,
+		goMemLimitModificationEnabled: features.Contains(featureFlags, features.GoMemLimitModificationEnabled),
 	}, nil
 }
 
@@ -102,10 +102,6 @@ func (s *Service) ModifyPodResource(pod *v1.Pod, t *v1beta3.Tortoise) {
 		}
 	}
 
-	if !s.golangEnvModificationEnabled {
-		return
-	}
-
 	// Update GOMEMLIMIT and GOMAXPROCS
 	for i, container := range pod.Spec.Containers {
 		for j, env := range container.Env {
@@ -134,6 +130,11 @@ func (s *Service) ModifyPodResource(pod *v1.Pod, t *v1beta3.Tortoise) {
 
 			}
 
+			if !s.goMemLimitModificationEnabled {
+				// don't modify GOMEMLIMIT
+				continue
+			}
+
 			if env.Name == "GOMEMLIMIT" {
 				changeRatio, ok := requestChangeRatio[containerNameAndResource{
 					containerName: container.Name,

diff --git a/pkg/pod/pod_test.go b/pkg/pod/pod_test.go
@@ -617,7 +617,7 @@ func TestService_ModifyPodResource(t *testing.T) {
 		{
 			name: "Tortoise is Auto; GOMEMLIMIT and GOMAXPROCS are updated based on the recommendation",
 			fields: fields{
-				featureFlags: []features.FeatureFlag{features.GolangEnvModification},
+				featureFlags: []features.FeatureFlag{features.GoMemLimitModificationEnabled},
 			},
 			args: args{
 				pod: &v1.Pod{
@@ -753,10 +753,7 @@ func TestService_ModifyPodResource(t *testing.T) {
 			},
 		},
 		{
-			name: "Tortoise is Auto; GOMEMLIMIT and GOMAXPROCS are ignored when no feature flag is enabled",
-			fields: fields{
-				featureFlags: []features.FeatureFlag{},
-			},
+			name: "Tortoise is Auto; GOMEMLIMIT is ignored if no feature flag",
 			args: args{
 				pod: &v1.Pod{
 					Spec: v1.PodSpec{
@@ -770,7 +767,7 @@ func TestService_ModifyPodResource(t *testing.T) {
 									},
 									{
 										Name:  "GOMEMLIMIT",
-										Value: "100Mi",
+										Value: "100MiB",
 									},
 								},
 								Resources: v1.ResourceRequirements{
@@ -793,7 +790,7 @@ func TestService_ModifyPodResource(t *testing.T) {
 									},
 									{
 										Name:  "GOMEMLIMIT",
-										Value: "100Mi",
+										Value: "100MiB",
 									},
 								},
 								Resources: v1.ResourceRequirements{
@@ -829,7 +826,7 @@ func TestService_ModifyPodResource(t *testing.T) {
 									ContainerName: "istio-proxy",
 									Resource: v1.ResourceList{
 										v1.ResourceCPU:    resource.MustParse("50m"),
-										v1.ResourceMemory: resource.MustParse("200Mi"),
+										v1.ResourceMemory: resource.MustParse("2000Mi"),
 									},
 								},
 							},
@@ -845,11 +842,11 @@ func TestService_ModifyPodResource(t *testing.T) {
 							Env: []v1.EnvVar{
 								{
 									Name:  "GOMAXPROCS",
-									Value: "1",
+									Value: "2",
 								},
 								{
 									Name:  "GOMEMLIMIT",
-									Value: "100Mi",
+									Value: "100MiB",
 								},
 							},
 							Resources: v1.ResourceRequirements{
@@ -868,21 +865,21 @@ func TestService_ModifyPodResource(t *testing.T) {
 							Env: []v1.EnvVar{
 								{
 									Name:  "GOMAXPROCS",
-									Value: "1",
+									Value: "1", // It wants to be 0.5, but GOMAXPROCS should be an integer. So, we ceil it to 1.
 								},
 								{
 									Name:  "GOMEMLIMIT",
-									Value: "100Mi",
+									Value: "100MiB",
 								},
 							},
 							Resources: v1.ResourceRequirements{
 								Requests: v1.ResourceList{
 									v1.ResourceCPU:    resource.MustParse("50m"),
-									v1.ResourceMemory: resource.MustParse("200Mi"),
+									v1.ResourceMemory: resource.MustParse("2000Mi"),
 								},
 								Limits: v1.ResourceList{
 									v1.ResourceCPU:    resource.MustParse("200m"),
-									v1.ResourceMemory: resource.MustParse("400Mi"),
+									v1.ResourceMemory: resource.MustParse("4000Mi"),
 								},
 							},
 						},