Remove the experimental ContainerV2 flag (#466)

Remove the experimental `ContainerV2` flag

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=``

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

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) {

integration_test/testdata/launcher-custom-dangerous-disable-container-support.yml

@@ -3,5 +3,3 @@ configVersion: 1
 jvmOpts:
   - '-Xmx1g'
 dangerousDisableContainerSupport: true
-experimental:
-  containerV2: false

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

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

integration_test/testdata/launcher-custom-max-ram-override.yml

@@ -2,5 +2,3 @@ configType: java
 configVersion: 1
 jvmOpts:
   - '-XX:MaxRAM=1001'
-experimental:
-  containerV2: false

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

integration_test/testdata/launcher-custom-multiprocess-long-sub-process.yml

@@ -9,5 +9,3 @@ subProcesses:
       SLEEP_TIME: "200"
     jvmOpts:
       - '-Xmx1g'
-experimental:
-  containerV2: false

integration_test/testdata/launcher-custom-multiprocess.yml

@@ -7,5 +7,3 @@ subProcesses:
     configType: java
     jvmOpts:
       - '-Xmx1g'
-experimental:
-  containerV2: false

integration_test/testdata/launcher-custom.yml

@@ -2,5 +2,3 @@ configType: java
 configVersion: 1
 jvmOpts:
   - '-Xmx1g'
-experimental:
-  containerV2: false

integration_test/testdata/launcher-static-bad-java-home.yml

@@ -9,5 +9,3 @@ jvmOpts:
 args:
   - arg1
 javaHome: /foo/bar
-experimental:
-  containerV2: false

integration_test/testdata/launcher-static-multiprocess.yml

@@ -16,5 +16,3 @@ subProcesses:
       - ./testdata/
     jvmOpts:
       - '-Xmx4M'
-experimental:
-  containerV2: false

integration_test/testdata/launcher-static-with-dirs.yml

@@ -11,5 +11,3 @@ args:
 dirs:
   - foo
   - bar/baz
-experimental:
-  containerV2: false

integration_test/testdata/launcher-static.yml

@@ -8,5 +8,3 @@ jvmOpts:
   - '-Xmx4M'
 args:
   - arg1
-experimental:
-  containerV2: false

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
 }
 

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{},
 				},
 			},
 		},

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")