Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable containersV2 by default #369

Merged
merged 5 commits into from
Jan 10, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -150,7 +150,16 @@ All output from `go-java-launcher` itself, and from the launch of all processes

By _default_, 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
``-XX:MaxRAMPercentage=`` nor ``-XX:InitialRAMPercentage=`` prefixes are present in either static or custom jvm opts
``-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.

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).
Expand All @@ -159,16 +168,6 @@ variable):
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).

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

### Overriding default values

Developers can override the heap percentage in containers by specifying both ``-XX:MaxRAMPercentage=``
Expand Down
5 changes: 5 additions & 0 deletions changelog/@unreleased/pr-369.v2.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
type: improvement
improvement:
description: Enable `containersV2` by default.
links:
- https://github.com/palantir/go-java-launcher/pull/369
2 changes: 1 addition & 1 deletion integration_test/go_java_launcher_integration_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -144,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", []string{})
output := testContainerSupportEnabled(t, "foo", "", []string{"-Xmx", "-Xms"})
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is a no custom config, or there is a failure to read the custom config, we use the new default behavior, hence this change.

assert.Regexp(t, `Failed to read custom config file, assuming no custom config: foo`, output)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ configVersion: 1
jvmOpts:
- '-Xmx1g'
dangerousDisableContainerSupport: true
experimental:
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Setting the flag to false to retain tests for old behavior for now.

containerV2: false
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,3 @@ configType: java
configVersion: 1
jvmOpts:
- '-XX:InitialRAMPercentage=70.0'
experimental:
containerV2: true
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,3 @@ configType: java
configVersion: 1
jvmOpts:
- '-XX:MaxRAMPercentage=70.0'
experimental:
containerV2: true
Original file line number Diff line number Diff line change
Expand Up @@ -3,5 +3,3 @@ configVersion: 1
jvmOpts:
- '-Xms1g'
- '-Xmx1g'
experimental:
containerV2: true
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
configType: java
configVersion: 1
jvmOpts: []
experimental:
containerV2: true
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,5 @@ jvmOpts:
- '-Xmx1g'
- '-XX:InitialRAMPercentage=79.9'
- '-XX:MaxRAMPercentage=80.9'
experimental:
containerV2: false
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ configVersion: 1
jvmOpts:
- '-Xmx1g'
- '-XX:InitialRAMPercentage=79.9'
experimental:
containerV2: false
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ configType: java
configVersion: 1
jvmOpts:
- '-XX:MaxRAM=1001'
experimental:
containerV2: false
Original file line number Diff line number Diff line change
Expand Up @@ -3,3 +3,5 @@ configVersion: 1
jvmOpts:
- '-Xmx1g'
- '-XX:MaxRAMPercentage=79.9'
experimental:
containerV2: false
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ subProcesses:
SLEEP_TIME: "200"
jvmOpts:
- '-Xmx1g'
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-custom-multiprocess.yml
Original file line number Diff line number Diff line change
Expand Up @@ -7,3 +7,5 @@ subProcesses:
configType: java
jvmOpts:
- '-Xmx1g'
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-custom.yml
Original file line number Diff line number Diff line change
Expand Up @@ -2,3 +2,5 @@ configType: java
configVersion: 1
jvmOpts:
- '-Xmx1g'
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-static-bad-java-home.yml
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,5 @@ jvmOpts:
args:
- arg1
javaHome: /foo/bar
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-static-multiprocess.yml
Original file line number Diff line number Diff line change
Expand Up @@ -16,3 +16,5 @@ subProcesses:
- ./testdata/
jvmOpts:
- '-Xmx4M'
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-static-with-dirs.yml
Original file line number Diff line number Diff line change
Expand Up @@ -11,3 +11,5 @@ args:
dirs:
- foo
- bar/baz
experimental:
containerV2: false
2 changes: 2 additions & 0 deletions integration_test/testdata/launcher-static.yml
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,5 @@ jvmOpts:
- '-Xmx4M'
args:
- arg1
experimental:
containerV2: false
6 changes: 5 additions & 1 deletion launchlib/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ type CustomLauncherConfig struct {
}

type ExperimentalLauncherConfig struct {
ContainerV2 bool `yaml:"containerV2"`
ContainerV2 *bool `yaml:"containerV2"`
}

type PrimaryCustomLauncherConfig struct {
Expand Down Expand Up @@ -267,6 +267,10 @@ func parseCustomConfig(yamlString []byte) (PrimaryCustomLauncherConfig, error) {
"subProcess config %s", name)
}
}
if config.Experimental.ContainerV2 == nil {
var trueVal = true
config.Experimental.ContainerV2 = &trueVal
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've seen this pattern elsewhere, but it would be nice if I could make this one line.

Copy link
Contributor

@andybradshaw andybradshaw Jan 9, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there's anything in the standard library for it :( but there are some pretty small deps for doing something like this:

package main

import (
    "fmt"
    "k8s.io/utils/ptr"
)

func main() {
    a := ptr.To(false)
    fmt.Printf("%T %v", a, *a)
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I'm leaning towards not adding the dep since we use this in one place in the implementation (and also once in the tests).

}
return config, nil
}

Expand Down
14 changes: 11 additions & 3 deletions launchlib/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ import (
"github.com/stretchr/testify/assert"
)

var trueValue = true

func TestParseStaticConfig(t *testing.T) {
for i, currCase := range []struct {
name string
Expand Down Expand Up @@ -188,6 +190,7 @@ jvmOpts:
},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
DisableContainerSupport: false,
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
},
},
Expand All @@ -208,7 +211,8 @@ jvmOpts:
TypedConfig: TypedConfig{
Type: "java",
},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
},
},
Expand All @@ -234,7 +238,8 @@ jvmOpts:
Env: map[string]string{
"SOME_ENV_VAR": "{{CWD}}/etc/profile",
},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
},
},
Expand Down Expand Up @@ -272,7 +277,8 @@ subProcesses:
Env: map[string]string{
"SOME_ENV_VAR": "{{CWD}}/etc/profile",
},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
SubProcesses: map[string]CustomLauncherConfig{
"envoy": {
Expand Down Expand Up @@ -306,6 +312,7 @@ dangerousDisableContainerSupport: true
},
JvmOpts: []string{"jvmOpt1", "jvmOpt2"},
DisableContainerSupport: true,
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
},
},
Expand All @@ -330,6 +337,7 @@ env:
"SOME_ENV_VAR": "/etc/profile",
"OTHER_ENV_VAR": "/etc/redhat-release",
},
Experimental: ExperimentalLauncherConfig{ContainerV2: &trueValue},
},
},
},
Expand Down
16 changes: 9 additions & 7 deletions launchlib/launcher.go
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,12 @@ 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 customConfig.Experimental.ContainerV2 {
// 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 {
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
Expand All @@ -294,9 +299,6 @@ func createJvmOpts(combinedJvmOpts []string, customConfig *CustomLauncherConfig,
} else {
combinedJvmOpts = jvmOptsWithUpdatedHeapSizeArgs
}
} else {
combinedJvmOpts = filterHeapSizeArgs(combinedJvmOpts)
combinedJvmOpts = ensureActiveProcessorCount(combinedJvmOpts, logger)
}
return combinedJvmOpts
}
Expand Down Expand Up @@ -334,7 +336,7 @@ func filterHeapSizeArgs(args []string) []string {
return filtered
}

// Used when the containerV2 flag is set
// 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
Expand Down Expand Up @@ -425,8 +427,8 @@ 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.
// 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.
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")
Expand Down