From b1a206c8a59fd90ea8fcb27d7696535303176df3 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Thu, 16 Nov 2023 12:18:05 -0500 Subject: [PATCH] Add the `containerV2` flag to compute initial heap percentage using processor count, and disable setting the `ActiveProcessorCount` JVM option (#361) When the experimental `containerV2` is set, 1) compute the heap percentage as 75% of the heap minus 3mb per processor, with a minimum value of 50%, and 2) don't set the `-XX:ActiveProcessorCount` JVM option. --- README.md | 11 ++- changelog/@unreleased/pr-361.v2.yml | 7 ++ .../go_java_launcher_integration_test.go | 86 +++++++++++++++-- ...ntainer-v2-with-initial-ram-percentage.yml | 6 ++ ...l-container-v2-with-max-ram-percentage.yml | 6 ++ ...imental-container-v2-with-xms-and-xmx.yml} | 3 +- ...ncher-custom-experimental-container-v2.yml | 5 + launchlib/config.go | 4 +- launchlib/launcher.go | 55 ++++++++++- launchlib/memory.go | 71 ++++++++++++++ launchlib/memory_test.go | 95 +++++++++++++++++++ 11 files changed, 334 insertions(+), 15 deletions(-) create mode 100644 changelog/@unreleased/pr-361.v2.yml create mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml create mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml rename integration_test/testdata/{launcher-custom-experimental-processor-count.yml => launcher-custom-experimental-container-v2-with-xms-and-xmx.yml} (65%) create mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2.yml create mode 100644 launchlib/memory.go create mode 100644 launchlib/memory_test.go diff --git a/README.md b/README.md index fc7e215c..d0a366bd 100644 --- a/README.md +++ b/README.md @@ -148,7 +148,7 @@ All output from `go-java-launcher` itself, and from the launch of all processes ## Java heap and container support -By default, when starting a java process inside a container (as indicated by the presence of ``CONTAINER`` env +By _default_, when starting a java process inside a container (as indicated by the presence of ``CONTAINER`` env variable): 1. If `-XX:ActiveProcessorCount` is unset, it will be set based on the discovered cgroup configurations and host @@ -162,6 +162,15 @@ variable): This will cause the JVM 11+ to discover the ``MaxRAM`` value using Linux cgroups, and calculate the heap sizes as the specified percentage of ``MaxRAM`` value, e.g. ``max-heap-size = MaxRAM * MaxRamPercentage``. +If the experimental flag `containerV2` is set: +1. The `-XX:ActiveProcessorCount` is unset, it will remain unset. +1. Args with prefix``-Xmx|-Xms`` in both static and custom jvm opts will be filtered out. If neither + ``-XX:MaxRAMPercentage=`` nor ``-XX:InitialRAMPercentage=`` prefixes are present in either static or custom jvm opts: + - if we can obtain the cgroups memory limit ``-Xmx|-Xms`` will both be set to be 75% of the cgroups memory limit + minus 3mb per processor, with a minimum value of 50% of the heap. + - if we cannot obtain the cgroups memory limit, both RAM percentage values will be set to ``75.0`` (i.e. + ``-XX:InitialRAMPercentage=75.0 -XX:MaxRAMPercentage=75.0 `` will be appended after all the other jvm opts) + ### Overriding default values Developers can override the heap percentage in containers by specifying both ``-XX:MaxRAMPercentage=`` diff --git a/changelog/@unreleased/pr-361.v2.yml b/changelog/@unreleased/pr-361.v2.yml new file mode 100644 index 00000000..c721722f --- /dev/null +++ b/changelog/@unreleased/pr-361.v2.yml @@ -0,0 +1,7 @@ +type: improvement +improvement: + description: When the experimental `containerV2` is set, 1) compute the heap percentage + as 75% of the heap minus 3mb per processor, with a minimum value of 50%, and 2) + don't set the -XX:ActiveProcessorCount JVM option. + links: + - https://github.com/palantir/go-java-launcher/pull/361 diff --git a/integration_test/go_java_launcher_integration_test.go b/integration_test/go_java_launcher_integration_test.go index 215b8603..9bee79de 100644 --- a/integration_test/go_java_launcher_integration_test.go +++ b/integration_test/go_java_launcher_integration_test.go @@ -45,9 +45,10 @@ func TestMainMethod(t *testing.T) { func TestMainMethodContainerSupportEnabled(t *testing.T) { for _, tc := range []struct { - name string - launcherCustom string - expectedJVMArgs string + name string + launcherCustom string + expectedJVMArgs string + expectedJVMArgKeys []string }{ { name: "sets defaults", @@ -69,9 +70,33 @@ func TestMainMethodContainerSupportEnabled(t *testing.T) { launcherCustom: "testdata/launcher-custom-initial-and-max-ram-percentage-override.yml", expectedJVMArgs: "-XX\\:InitialRAMPercentage=79.9 -XX\\:MaxRAMPercentage=80.9 -XX\\:ActiveProcessorCount=2", }, + { + name: "using containerV2 sets Xms and Xmx and does not set ActiveProcessorCount", + launcherCustom: "testdata/launcher-custom-experimental-container-v2.yml", + expectedJVMArgs: "", + expectedJVMArgKeys: []string{"-Xmx", "-Xms"}, + }, + { + name: "using containerV2 with InitialRAMPercentage does not set Xms, Xmx, or " + + "ActiveProcessorCount", + launcherCustom: "testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml", + expectedJVMArgs: "-XX\\:InitialRAMPercentage=70.0", + }, + { + name: "using containerV2 with MaxRAMPercentage does not set Xms, Xmx, or " + + "ActiveProcessorCount", + launcherCustom: "testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml", + expectedJVMArgs: "-XX\\:MaxRAMPercentage=70.0", + }, + { + name: "using containerV2 does not use user-provided Xms or Xmx", + launcherCustom: "testdata/launcher-custom-experimental-container-v2.yml", + expectedJVMArgs: "", + expectedJVMArgKeys: []string{"-Xmx", "-Xms"}, + }, } { t.Run(tc.name, func(t *testing.T) { - testContainerSupportEnabled(t, tc.launcherCustom, tc.expectedJVMArgs) + testContainerSupportEnabled(t, tc.launcherCustom, tc.expectedJVMArgs, tc.expectedJVMArgKeys) }) } } @@ -97,7 +122,7 @@ func TestMainMethodContainerSupportDisabled(t *testing.T) { }, } { t.Run(tc.name, func(t *testing.T) { - testInContainer(t, tc.launcherCustom, tc.containerSupportMessage, tc.expectedJVMArgs) + testInContainer(t, tc.launcherCustom, tc.containerSupportMessage, tc.expectedJVMArgs, []string{}) }) } } @@ -119,7 +144,7 @@ func TestMainMethodWithoutCustomConfig(t *testing.T) { } func TestMainMethodContainerWithoutCustomConfig(t *testing.T) { - output := testContainerSupportEnabled(t, "foo", "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2") + output := testContainerSupportEnabled(t, "foo", "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2", []string{}) assert.Regexp(t, `Failed to read custom config file, assuming no custom config: foo`, output) } @@ -178,6 +203,42 @@ func TestSubProcessesParsedMonitorSignals(t *testing.T) { assert.Len(t, trapped.FindAll(output.Bytes(), -1), 2, "expect two messages that SIGPOLL was caught") } +func TestComputeJVMHeapSize(t *testing.T) { + for _, tc := range []struct { + name string + numHostProcessors int + memoryLimit uint64 + expectedMaxHeapSize uint64 + }{ + { + name: "at least 50% of heap", + numHostProcessors: 1, + memoryLimit: 10 * launchlib.BytesInMebibyte, + // 75% of heap - 3mb*processors = 4.5mb + expectedMaxHeapSize: 5 * launchlib.BytesInMebibyte, + }, + { + name: "computes 75% of heap minus 3mb per processor", + numHostProcessors: 1, + memoryLimit: 16 * launchlib.BytesInMebibyte, + // 75% of heap - 3mb*processors = 9mb + expectedMaxHeapSize: 9 * launchlib.BytesInMebibyte, + }, + { + name: "multiple processors", + numHostProcessors: 3, + memoryLimit: 120 * launchlib.BytesInMebibyte, + // 75% of heap - 3mb*processors = 81mb + expectedMaxHeapSize: 81 * launchlib.BytesInMebibyte, + }, + } { + t.Run(tc.name, func(t *testing.T) { + heapSizeInBytes := launchlib.ComputeJVMHeapSizeInBytes(tc.numHostProcessors, tc.memoryLimit) + assert.Equal(t, heapSizeInBytes, tc.expectedMaxHeapSize) + }) + } +} + func runMainWithArgs(t *testing.T, staticConfigFile, customConfigFile string, env ...string) (string, error) { jdkDir := "jdk" javaHome, err := filepath.Abs(jdkDir) @@ -226,18 +287,23 @@ func runMultiProcess(t *testing.T, cmd *exec.Cmd) map[string]int { return children } -func testContainerSupportEnabled(t *testing.T, launcherCustom string, expectedJvmArgs string) string { - return testInContainer(t, launcherCustom, "Container support enabled", expectedJvmArgs) +func testContainerSupportEnabled(t *testing.T, launcherCustom string, expectedJvmArgs string, expectedJvmArgKeys []string) string { + return testInContainer(t, launcherCustom, "Container support enabled", expectedJvmArgs, expectedJvmArgKeys) } -func testInContainer(t *testing.T, launcherCustom string, containerSupportMessage string, jvmArgs string) string { +func testInContainer(t *testing.T, launcherCustom string, containerSupportMessage string, jvmArgs string, jvmArgKeys []string) string { output, err := runMainWithArgs(t, "testdata/launcher-static.yml", launcherCustom, "CONTAINER=") require.NoError(t, err, "failed: %s", output) // part of expected output from launcher - assert.Regexp(t, `Argument list to executable binary: \[.+/bin/java `+jvmArgs+` -classpath .+/go-java-launcher/integration_test/testdata Main arg1\]`, output) + if jvmArgs != "" { + assert.Regexp(t, `Argument list to executable binary: \[.+/bin/java `+jvmArgs+` -classpath .+/go-java-launcher/integration_test/testdata Main arg1\]`, output) + } // container support detected and running inside container assert.Regexp(t, containerSupportMessage, output) + for _, key := range jvmArgKeys { + assert.Regexp(t, key, output) + } // expected output of Java program assert.Regexp(t, `\nmain method\n`, output) return output diff --git a/integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml b/integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml new file mode 100644 index 00000000..f2ff4319 --- /dev/null +++ b/integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml @@ -0,0 +1,6 @@ +configType: java +configVersion: 1 +jvmOpts: + - '-XX:InitialRAMPercentage=70.0' +experimental: + containerV2: true diff --git a/integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml b/integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml new file mode 100644 index 00000000..e5d905f1 --- /dev/null +++ b/integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml @@ -0,0 +1,6 @@ +configType: java +configVersion: 1 +jvmOpts: + - '-XX:MaxRAMPercentage=70.0' +experimental: + containerV2: true diff --git a/integration_test/testdata/launcher-custom-experimental-processor-count.yml b/integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml similarity index 65% rename from integration_test/testdata/launcher-custom-experimental-processor-count.yml rename to integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml index c2457679..299a15bf 100644 --- a/integration_test/testdata/launcher-custom-experimental-processor-count.yml +++ b/integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml @@ -1,6 +1,7 @@ configType: java configVersion: 1 jvmOpts: + - '-Xms1g' - '-Xmx1g' experimental: - overrideActiveProcessorCount: true + containerV2: true diff --git a/integration_test/testdata/launcher-custom-experimental-container-v2.yml b/integration_test/testdata/launcher-custom-experimental-container-v2.yml new file mode 100644 index 00000000..c5d70615 --- /dev/null +++ b/integration_test/testdata/launcher-custom-experimental-container-v2.yml @@ -0,0 +1,5 @@ +configType: java +configVersion: 1 +jvmOpts: [] +experimental: + containerV2: true diff --git a/launchlib/config.go b/launchlib/config.go index f1f6c5d6..6b1538eb 100644 --- a/launchlib/config.go +++ b/launchlib/config.go @@ -71,7 +71,9 @@ type CustomLauncherConfig struct { DisableContainerSupport bool `yaml:"dangerousDisableContainerSupport"` } -type ExperimentalLauncherConfig struct{} +type ExperimentalLauncherConfig struct { + ContainerV2 bool `yaml:"containerV2"` +} type PrimaryCustomLauncherConfig struct { VersionedConfig `yaml:",inline"` diff --git a/launchlib/launcher.go b/launchlib/launcher.go index 8ab8829c..1dde27e2 100644 --- a/launchlib/launcher.go +++ b/launchlib/launcher.go @@ -21,6 +21,7 @@ import ( "os/exec" "path" "regexp" + "runtime" "strings" "github.com/pkg/errors" @@ -31,6 +32,7 @@ const ( TemplateDelimsClose = "}}" // ExecPathBlackListRegex matches characters disallowed in paths we allow to be passed to exec() ExecPathBlackListRegex = `[^\w.\/_\-]` + BytesInMebibyte = 1048576 ) type ServiceCmds struct { @@ -279,8 +281,20 @@ func delim(str string) string { func createJvmOpts(combinedJvmOpts []string, customConfig *CustomLauncherConfig, logger io.WriteCloser) []string { if isEnvVarSet("CONTAINER") && !customConfig.DisableContainerSupport && !hasMaxRAMOverride(combinedJvmOpts) { _, _ = fmt.Fprintln(logger, "Container support enabled") - combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts) - combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger) + if customConfig.Experimental.ContainerV2 { + jvmOptsWithUpdatedHeapSizeArgs, err := filterHeapSizeArgsV2(combinedJvmOpts) + if err != nil { + // When we fail to get the memory limit from the cgroups files, fallback to using percentage-based heap + // sizing. While this method doesn't take into account the per-processor memory offset, it is supported + // by all platforms using Java. + combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts) + } else { + combinedJvmOpts = jvmOptsWithUpdatedHeapSizeArgs + } + } else { + combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts) + combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger) + } return combinedJvmOpts } @@ -317,6 +331,34 @@ func filterHeapSizeArgs(args []string) []string { return filtered } +// Used when the containerV2 flag is set +func filterHeapSizeArgsV2(args []string) ([]string, error) { + var filtered []string + var hasMaxRAMPercentage, hasInitialRAMPercentage bool + for _, arg := range args { + if !isHeapSizeArg(arg) { + filtered = append(filtered, arg) + } + + if isMaxRAMPercentage(arg) { + hasMaxRAMPercentage = true + } else if isInitialRAMPercentage(arg) { + hasInitialRAMPercentage = true + } + } + + if !hasInitialRAMPercentage && !hasMaxRAMPercentage { + cgroupMemoryLimitInBytes, err := DefaultMemoryLimit.MemoryLimitInBytes() + if err != nil { + return filtered, errors.Wrap(err, "failed to get cgroup memory limit") + } + jvmHeapSizeInBytes := ComputeJVMHeapSizeInBytes(runtime.NumCPU(), cgroupMemoryLimitInBytes) + filtered = append(filtered, fmt.Sprintf("-Xms%d", jvmHeapSizeInBytes)) + filtered = append(filtered, fmt.Sprintf("-Xmx%d", jvmHeapSizeInBytes)) + } + return filtered, nil +} + func ensureActiveProcessorCount(args []string, logger io.Writer) []string { filtered := make([]string, 0, len(args)+1) @@ -376,3 +418,12 @@ func isMaxRAMPercentage(arg string) bool { func isInitialRAMPercentage(arg string) bool { return strings.HasPrefix(arg, "-XX:InitialRAMPercentage=") } + +// ComputeJVMHeapSizeInBytes If the experimental `ContainerV2` is set, compute the heap size to be 75% of +// the heap minus 3mb per processor, with a minimum value of 50% of the heap. +func ComputeJVMHeapSizeInBytes(hostProcessorCount int, cgroupMemoryLimitInBytes uint64) uint64 { + var memoryLimit = float64(cgroupMemoryLimitInBytes) + var processorAdjustment = 3 * BytesInMebibyte * float64(hostProcessorCount) + var computedHeapSize = max(0.5*memoryLimit, 0.75*memoryLimit-processorAdjustment) + return uint64(computedHeapSize) +} diff --git a/launchlib/memory.go b/launchlib/memory.go new file mode 100644 index 00000000..a0b756fc --- /dev/null +++ b/launchlib/memory.go @@ -0,0 +1,71 @@ +// Copyright 2023 Palantir Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package launchlib + +import ( + "io" + "io/fs" + "os" + "path/filepath" + "strconv" + "strings" + + "github.com/pkg/errors" +) + +const ( + memGroupName = "memory" + memLimitName = "memory.limit_in_bytes" +) + +type MemoryLimit interface { + MemoryLimitInBytes() (uint64, error) +} + +var DefaultMemoryLimit = NewCGroupMemoryLimit(os.DirFS("/")) + +type CGroupMemoryLimit struct { + pather CGroupPather + fs fs.FS +} + +func NewCGroupMemoryLimit(filesystem fs.FS) MemoryLimit { + return CGroupMemoryLimit{ + pather: NewCGroupV1Pather(filesystem), + fs: filesystem, + } +} + +func (c CGroupMemoryLimit) MemoryLimitInBytes() (uint64, error) { + memoryCGroupPath, err := c.pather.Path(memGroupName) + if err != nil { + return 0, errors.Wrap(err, "failed to get memory cgroup path") + } + + memLimitFilepath := filepath.Join(memoryCGroupPath, memLimitName) + memLimitFile, err := c.fs.Open(convertToFSPath(memLimitFilepath)) + if err != nil { + return 0, errors.Wrapf(err, "unable to open memory.limit_in_bytes at expected location: %s", memLimitFilepath) + } + memLimitBytes, err := io.ReadAll(memLimitFile) + if err != nil { + return 0, errors.Wrapf(err, "unable to read memory.limit_in_bytes") + } + memLimit, err := strconv.Atoi(strings.TrimSpace(string(memLimitBytes))) + if err != nil { + return 0, errors.New("unable to convert memory.limit_in_bytes value to expected type") + } + return uint64(memLimit), nil +} diff --git a/launchlib/memory_test.go b/launchlib/memory_test.go new file mode 100644 index 00000000..1e1527b7 --- /dev/null +++ b/launchlib/memory_test.go @@ -0,0 +1,95 @@ +// Copyright 2023 Palantir Technologies, Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package launchlib_test + +import ( + "io/fs" + "testing" + "testing/fstest" + + "github.com/palantir/go-java-launcher/launchlib" + "github.com/pkg/errors" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var ( + memoryLimitContent = []byte("2147483648\n") + badMemoryLimitContent = []byte(``) +) + +func TestMemoryLimit_DefaultMemoryLimit(t *testing.T) { + for _, test := range []struct { + name string + filesystem fs.FS + expectedMemoryLimit uint64 + expectedError error + }{ + { + name: "fails when unable to read memory.limit_in_bytes", + filesystem: fstest.MapFS{ + "proc/self/cgroup": &fstest.MapFile{ + Data: CGroupContent, + }, + "proc/self/mountinfo": &fstest.MapFile{ + Data: MountInfoContent, + }, + }, + expectedError: errors.New("unable to open memory.limit_in_bytes at expected location"), + }, + { + name: "fails when unable to parse memory.limit_in_bytes", + filesystem: fstest.MapFS{ + "proc/self/cgroup": &fstest.MapFile{ + Data: CGroupContent, + }, + "proc/self/mountinfo": &fstest.MapFile{ + Data: MountInfoContent, + }, + "sys/fs/cgroup/memory/memory.limit_in_bytes": &fstest.MapFile{ + Data: badMemoryLimitContent, + }, + }, + expectedError: errors.New("unable to convert memory.limit_in_bytes value to expected type"), + }, + { + name: "returns expected RAM percentage when memory.limit_in_bytes under 2 GiB", + filesystem: fstest.MapFS{ + "proc/self/cgroup": &fstest.MapFile{ + Data: CGroupContent, + }, + "proc/self/mountinfo": &fstest.MapFile{ + Data: MountInfoContent, + }, + "sys/fs/cgroup/memory/memory.limit_in_bytes": &fstest.MapFile{ + Data: memoryLimitContent, + }, + }, + expectedMemoryLimit: 1 << 31, + }, + } { + t.Run(test.name, func(t *testing.T) { + limit := launchlib.NewCGroupMemoryLimit(test.filesystem) + memoryLimit, err := limit.MemoryLimitInBytes() + if test.expectedError != nil { + require.Error(t, err) + assert.Contains(t, err.Error(), test.expectedError.Error()) + return + } + assert.NoError(t, err) + assert.Equal(t, test.expectedMemoryLimit, memoryLimit) + }) + } +}