From b9a362d54e2a85787aa872dc14e89a5d39189208 Mon Sep 17 00:00:00 2001 From: Pritham Marupaka Date: Fri, 13 Sep 2024 16:08:01 -0400 Subject: [PATCH] Remove the experimental `ContainerV2` flag (#466) Remove the experimental `ContainerV2` flag --- README.md | 11 +--- changelog/@unreleased/pr-466.v2.yml | 5 ++ .../go_java_launcher_integration_test.go | 37 ++--------- ...om-dangerous-disable-container-support.yml | 2 - ...ntainer-v2-with-initial-ram-percentage.yml | 4 -- ...l-container-v2-with-max-ram-percentage.yml | 4 -- ...rimental-container-v2-with-xms-and-xmx.yml | 5 -- ...ncher-custom-experimental-container-v2.yml | 3 - ...nitial-and-max-ram-percentage-override.yml | 2 - ...custom-initial-ram-percentage-override.yml | 2 - .../launcher-custom-max-ram-override.yml | 2 - ...her-custom-max-ram-percentage-override.yml | 2 - ...r-custom-multiprocess-long-sub-process.yml | 2 - .../testdata/launcher-custom-multiprocess.yml | 2 - integration_test/testdata/launcher-custom.yml | 2 - .../launcher-static-bad-java-home.yml | 2 - .../testdata/launcher-static-multiprocess.yml | 2 - .../testdata/launcher-static-with-dirs.yml | 2 - integration_test/testdata/launcher-static.yml | 2 - launchlib/config.go | 5 -- launchlib/config_test.go | 14 ++-- launchlib/launcher.go | 65 ++++--------------- 22 files changed, 30 insertions(+), 147 deletions(-) create mode 100644 changelog/@unreleased/pr-466.v2.yml delete mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml delete mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml delete mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml delete mode 100644 integration_test/testdata/launcher-custom-experimental-container-v2.yml diff --git a/README.md b/README.md index 0d1b9cea..da3af604 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 +When starting a java process inside a container (as indicated by the presence of ``CONTAINER`` env variable): 1. If 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 @@ -159,15 +159,6 @@ 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 to `false`, the behavior will be as follows: -1. If `-XX:ActiveProcessorCount` is unset, it will be set based on the discovered cgroup configurations and host - information to a value between 2 and the number of processors reported by the runtime. You can read more about the - reasoning behind this [here](https://github.com/palantir/go-java-launcher/issues/313). -1. Args with prefix``-Xmx|-Xms`` in both static and custom jvm opts will be filtered out. -1. If neither ``-XX:MaxRAMPercentage=`` nor ``-XX:InitialRAMPercentage=`` prefixes are present in either static or - custom jvm opts, both 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-466.v2.yml b/changelog/@unreleased/pr-466.v2.yml new file mode 100644 index 00000000..3b9c8b38 --- /dev/null +++ b/changelog/@unreleased/pr-466.v2.yml @@ -0,0 +1,5 @@ +type: improvement +improvement: + description: Remove the experimental `ContainerV2` flag + links: + - https://github.com/palantir/go-java-launcher/pull/466 diff --git a/integration_test/go_java_launcher_integration_test.go b/integration_test/go_java_launcher_integration_test.go index fd7cf83d..bc3907ad 100644 --- a/integration_test/go_java_launcher_integration_test.go +++ b/integration_test/go_java_launcher_integration_test.go @@ -51,48 +51,25 @@ func TestMainMethodContainerSupportEnabled(t *testing.T) { expectedJVMArgKeys []string }{ { - name: "sets defaults", - launcherCustom: "testdata/launcher-custom.yml", - expectedJVMArgs: "-XX\\:InitialRAMPercentage=75.0 -XX\\:MaxRAMPercentage=75.0 -XX\\:ActiveProcessorCount=2", + name: "sets defaults", + launcherCustom: "testdata/launcher-custom.yml", + expectedJVMArgs: "", + expectedJVMArgKeys: []string{"-Xmx", "-Xms"}, }, { name: "does not set defaults if InitialRAMPercentage override is present", launcherCustom: "testdata/launcher-custom-initial-ram-percentage-override.yml", - expectedJVMArgs: "-XX\\:InitialRAMPercentage=79.9 -XX\\:ActiveProcessorCount=2", + expectedJVMArgs: "-XX\\:InitialRAMPercentage=79.9", }, { name: "does not set defaults if MaxRAMPercentage override is present", launcherCustom: "testdata/launcher-custom-max-ram-percentage-override.yml", - expectedJVMArgs: "-XX\\:MaxRAMPercentage=79.9 -XX\\:ActiveProcessorCount=2", + expectedJVMArgs: "-XX\\:MaxRAMPercentage=79.9", }, { name: "does not set defaults if InitialRAMPercentage and MaxRAMPercentage overrides are present", 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"}, + expectedJVMArgs: "-XX\\:InitialRAMPercentage=79.9 -XX\\:MaxRAMPercentage=80.9", }, } { t.Run(tc.name, func(t *testing.T) { diff --git a/integration_test/testdata/launcher-custom-dangerous-disable-container-support.yml b/integration_test/testdata/launcher-custom-dangerous-disable-container-support.yml index cd01d9c0..7c7e9f71 100644 --- a/integration_test/testdata/launcher-custom-dangerous-disable-container-support.yml +++ b/integration_test/testdata/launcher-custom-dangerous-disable-container-support.yml @@ -3,5 +3,3 @@ configVersion: 1 jvmOpts: - '-Xmx1g' dangerousDisableContainerSupport: true -experimental: - containerV2: false 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 deleted file mode 100644 index 445aadf8..00000000 --- a/integration_test/testdata/launcher-custom-experimental-container-v2-with-initial-ram-percentage.yml +++ /dev/null @@ -1,4 +0,0 @@ -configType: java -configVersion: 1 -jvmOpts: - - '-XX:InitialRAMPercentage=70.0' 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 deleted file mode 100644 index 04344178..00000000 --- a/integration_test/testdata/launcher-custom-experimental-container-v2-with-max-ram-percentage.yml +++ /dev/null @@ -1,4 +0,0 @@ -configType: java -configVersion: 1 -jvmOpts: - - '-XX:MaxRAMPercentage=70.0' diff --git a/integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml b/integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml deleted file mode 100644 index 3275e56a..00000000 --- a/integration_test/testdata/launcher-custom-experimental-container-v2-with-xms-and-xmx.yml +++ /dev/null @@ -1,5 +0,0 @@ -configType: java -configVersion: 1 -jvmOpts: - - '-Xms1g' - - '-Xmx1g' diff --git a/integration_test/testdata/launcher-custom-experimental-container-v2.yml b/integration_test/testdata/launcher-custom-experimental-container-v2.yml deleted file mode 100644 index 9218ad75..00000000 --- a/integration_test/testdata/launcher-custom-experimental-container-v2.yml +++ /dev/null @@ -1,3 +0,0 @@ -configType: java -configVersion: 1 -jvmOpts: [] diff --git a/integration_test/testdata/launcher-custom-initial-and-max-ram-percentage-override.yml b/integration_test/testdata/launcher-custom-initial-and-max-ram-percentage-override.yml index f7428b66..54d0c47e 100644 --- a/integration_test/testdata/launcher-custom-initial-and-max-ram-percentage-override.yml +++ b/integration_test/testdata/launcher-custom-initial-and-max-ram-percentage-override.yml @@ -4,5 +4,3 @@ jvmOpts: - '-Xmx1g' - '-XX:InitialRAMPercentage=79.9' - '-XX:MaxRAMPercentage=80.9' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom-initial-ram-percentage-override.yml b/integration_test/testdata/launcher-custom-initial-ram-percentage-override.yml index 7a48c70c..7b6f405c 100644 --- a/integration_test/testdata/launcher-custom-initial-ram-percentage-override.yml +++ b/integration_test/testdata/launcher-custom-initial-ram-percentage-override.yml @@ -3,5 +3,3 @@ configVersion: 1 jvmOpts: - '-Xmx1g' - '-XX:InitialRAMPercentage=79.9' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom-max-ram-override.yml b/integration_test/testdata/launcher-custom-max-ram-override.yml index a44c822d..eeb8f22a 100644 --- a/integration_test/testdata/launcher-custom-max-ram-override.yml +++ b/integration_test/testdata/launcher-custom-max-ram-override.yml @@ -2,5 +2,3 @@ configType: java configVersion: 1 jvmOpts: - '-XX:MaxRAM=1001' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom-max-ram-percentage-override.yml b/integration_test/testdata/launcher-custom-max-ram-percentage-override.yml index c884af10..55e748c5 100644 --- a/integration_test/testdata/launcher-custom-max-ram-percentage-override.yml +++ b/integration_test/testdata/launcher-custom-max-ram-percentage-override.yml @@ -3,5 +3,3 @@ configVersion: 1 jvmOpts: - '-Xmx1g' - '-XX:MaxRAMPercentage=79.9' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom-multiprocess-long-sub-process.yml b/integration_test/testdata/launcher-custom-multiprocess-long-sub-process.yml index 43da4664..6e9ad3f0 100644 --- a/integration_test/testdata/launcher-custom-multiprocess-long-sub-process.yml +++ b/integration_test/testdata/launcher-custom-multiprocess-long-sub-process.yml @@ -9,5 +9,3 @@ subProcesses: SLEEP_TIME: "200" jvmOpts: - '-Xmx1g' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom-multiprocess.yml b/integration_test/testdata/launcher-custom-multiprocess.yml index b2eb948d..e0994ce8 100644 --- a/integration_test/testdata/launcher-custom-multiprocess.yml +++ b/integration_test/testdata/launcher-custom-multiprocess.yml @@ -7,5 +7,3 @@ subProcesses: configType: java jvmOpts: - '-Xmx1g' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-custom.yml b/integration_test/testdata/launcher-custom.yml index 55382e26..e6ade620 100644 --- a/integration_test/testdata/launcher-custom.yml +++ b/integration_test/testdata/launcher-custom.yml @@ -2,5 +2,3 @@ configType: java configVersion: 1 jvmOpts: - '-Xmx1g' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-static-bad-java-home.yml b/integration_test/testdata/launcher-static-bad-java-home.yml index 97ae0c7f..adb52326 100644 --- a/integration_test/testdata/launcher-static-bad-java-home.yml +++ b/integration_test/testdata/launcher-static-bad-java-home.yml @@ -9,5 +9,3 @@ jvmOpts: args: - arg1 javaHome: /foo/bar -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-static-multiprocess.yml b/integration_test/testdata/launcher-static-multiprocess.yml index 60eb3e41..0c0828d8 100644 --- a/integration_test/testdata/launcher-static-multiprocess.yml +++ b/integration_test/testdata/launcher-static-multiprocess.yml @@ -16,5 +16,3 @@ subProcesses: - ./testdata/ jvmOpts: - '-Xmx4M' -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-static-with-dirs.yml b/integration_test/testdata/launcher-static-with-dirs.yml index 66970293..b55dd77f 100644 --- a/integration_test/testdata/launcher-static-with-dirs.yml +++ b/integration_test/testdata/launcher-static-with-dirs.yml @@ -11,5 +11,3 @@ args: dirs: - foo - bar/baz -experimental: - containerV2: false diff --git a/integration_test/testdata/launcher-static.yml b/integration_test/testdata/launcher-static.yml index 0f3fdb45..e02728b3 100644 --- a/integration_test/testdata/launcher-static.yml +++ b/integration_test/testdata/launcher-static.yml @@ -8,5 +8,3 @@ jvmOpts: - '-Xmx4M' args: - arg1 -experimental: - containerV2: false diff --git a/launchlib/config.go b/launchlib/config.go index f66c4529..80d915bb 100644 --- a/launchlib/config.go +++ b/launchlib/config.go @@ -72,7 +72,6 @@ type CustomLauncherConfig struct { } type ExperimentalLauncherConfig struct { - ContainerV2 *bool `yaml:"containerV2"` } type PrimaryCustomLauncherConfig struct { @@ -267,10 +266,6 @@ func parseCustomConfig(yamlString []byte) (PrimaryCustomLauncherConfig, error) { "subProcess config %s", name) } } - if config.Experimental.ContainerV2 == nil { - var trueVal = true - config.Experimental.ContainerV2 = &trueVal - } return config, nil } diff --git a/launchlib/config_test.go b/launchlib/config_test.go index c934c636..4eb796c2 100644 --- a/launchlib/config_test.go +++ b/launchlib/config_test.go @@ -20,8 +20,6 @@ import ( "github.com/stretchr/testify/assert" ) -var trueValue = true - func TestParseStaticConfig(t *testing.T) { for i, currCase := range []struct { name string @@ -190,7 +188,7 @@ jvmOpts: }, JvmOpts: []string{"jvmOpt1", "jvmOpt2"}, DisableContainerSupport: false, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, }, }, @@ -212,7 +210,7 @@ jvmOpts: Type: "java", }, JvmOpts: []string{"jvmOpt1", "jvmOpt2"}, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, }, }, @@ -239,7 +237,7 @@ jvmOpts: "SOME_ENV_VAR": "{{CWD}}/etc/profile", }, JvmOpts: []string{"jvmOpt1", "jvmOpt2"}, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, }, }, @@ -278,7 +276,7 @@ subProcesses: "SOME_ENV_VAR": "{{CWD}}/etc/profile", }, JvmOpts: []string{"jvmOpt1", "jvmOpt2"}, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, SubProcesses: map[string]CustomLauncherConfig{ "envoy": { @@ -312,7 +310,7 @@ dangerousDisableContainerSupport: true }, JvmOpts: []string{"jvmOpt1", "jvmOpt2"}, DisableContainerSupport: true, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, }, }, @@ -337,7 +335,7 @@ env: "SOME_ENV_VAR": "/etc/profile", "OTHER_ENV_VAR": "/etc/redhat-release", }, - Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue}, + Experimental: ExperimentalLauncherConfig{}, }, }, }, diff --git a/launchlib/launcher.go b/launchlib/launcher.go index 05dd2266..00dcd785 100644 --- a/launchlib/launcher.go +++ b/launchlib/launcher.go @@ -281,24 +281,17 @@ 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") - // If the containerV2 field is nil, there was a failure reading the custom config file, and we use the default - // behavior of enabling containerV2 behavior. - if customConfig.Experimental.ContainerV2 != nil && !*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. + // Also, when the memory limit is unusually high (defined to be over 1TB), we revert to the + // percentage-based heap sizing. This is to handle the edge case where the cgroups memory limit is set + // to be an arbitrary large value. combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts) - combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger) } else { - 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. - // Also, when the memory limit is unusually high (defined to be over 1TB), we revert to the - // percentage-based heap sizing. This is to handle the edge case where the cgroups memory limit is set - // to be an arbitrary large value. - combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts) - } else { - combinedJvmOpts = jvmOptsWithUpdatedHeapSizeArgs - } + combinedJvmOpts = jvmOptsWithUpdatedHeapSizeArgs } return combinedJvmOpts } @@ -336,7 +329,6 @@ func filterHeapSizeArgs(args []string) []string { return filtered } -// Used when the containerV2 flag is set to `true`. This is the default behavior. func filterHeapSizeArgsV2(args []string) ([]string, error) { var filtered []string var hasMaxRAMPercentage, hasInitialRAMPercentage bool @@ -367,29 +359,6 @@ func filterHeapSizeArgsV2(args []string) ([]string, error) { return filtered, nil } -func ensureActiveProcessorCount(args []string, logger io.Writer) []string { - filtered := make([]string, 0, len(args)+1) - - var hasActiveProcessorCount bool - for _, arg := range args { - if isActiveProcessorCount(arg) { - hasActiveProcessorCount = true - } - filtered = append(filtered, arg) - } - - if !hasActiveProcessorCount { - processorCountArg, err := getActiveProcessorCountArg(logger) - if err == nil { - filtered = append(filtered, processorCountArg) - } else { - _, _ = fmt.Fprintln(logger, "Failed to detect cgroup CPU configuration, not setting active processor count", err.Error()) - } - } - - return filtered -} - func hasMaxRAMOverride(args []string) bool { for _, arg := range args { if isMaxRAM(arg) { @@ -399,18 +368,6 @@ func hasMaxRAMOverride(args []string) bool { return false } -func getActiveProcessorCountArg(logger io.Writer) (string, error) { - cgroupProcessorCount, err := DefaultCGroupV1ProcessorCounter.ProcessorCount() - if err != nil { - return "", errors.Wrap(err, "failed to get cgroup processor count") - } - return fmt.Sprintf("-XX:ActiveProcessorCount=%d", cgroupProcessorCount), nil -} - -func isActiveProcessorCount(arg string) bool { - return strings.HasPrefix(arg, "-XX:ActiveProcessorCount=") -} - func isMaxRAM(arg string) bool { return strings.HasPrefix(arg, "-XX:MaxRAM=") } @@ -427,8 +384,8 @@ func isInitialRAMPercentage(arg string) bool { return strings.HasPrefix(arg, "-XX:InitialRAMPercentage=") } -// ComputeJVMHeapSizeInBytes If the experimental `ContainerV2` is set to `true` (which it is by default), compute the -// heap size to be 75% of the heap minus 3mb per processor, with a minimum value of 50% of the heap. +// ComputeJVMHeapSizeInBytes 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, error) { if cgroupMemoryLimitInBytes > 1_000_000*BytesInMebibyte { return 0, errors.New("cgroups memory limit is unusually high. Not computing JVM heap size")