From e3c41cfd432ff35d08808240504d585db7bf817d Mon Sep 17 00:00:00 2001 From: Kensei Nakada Date: Thu, 21 Mar 2024 18:13:37 +0900 Subject: [PATCH] implement GoMemLimitModificationEnabled feature flag (#387) --- pkg/features/features.go | 7 ++----- pkg/pod/pod.go | 25 +++++++++++++------------ pkg/pod/pod_test.go | 25 +++++++++++-------------- 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/pkg/features/features.go b/pkg/features/features.go index b866c00e..796a6ea5 100644 --- 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 index b5df07e5..07c409ec 100644 --- 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 index 6d44edf6..63b0610e 100644 --- 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"), }, }, },