-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[extension/cgroupruntime] Be aware of ECS task and CPU limits #36920
base: main
Are you sure you want to change the base?
Changes from 8 commits
e370c56
94716e3
1583f07
9eaed7f
0831dfa
7496221
6212bde
a510187
1b77e4f
12acc97
12db2c1
4f37a61
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
# Use this changelog template to create an entry for release notes. | ||
|
||
# One of 'breaking', 'deprecation', 'new_component', 'enhancement', 'bug_fix' | ||
change_type: enhancement | ||
|
||
# The name of the component, or a single word describing the area of concern, (e.g. filelogreceiver) | ||
component: cgroupruntimeextension | ||
|
||
# A brief description of the change. Surround your text with quotes ("") if it needs to start with a backtick (`). | ||
note: Implement ECS metadata retrieval for cgroupruntime extension. | ||
|
||
# Mandatory: One or more tracking issues related to the change. You can use the PR number here if no issue exists. | ||
issues: [36814] | ||
|
||
# (Optional) One or more lines of additional information to render under the primary note. | ||
# These lines will be padded with 2 spaces and then inserted directly into the document. | ||
# Use pipe (|) for multiline entries. | ||
subtext: | ||
|
||
# If your change doesn't affect end users or the exported elements of any package, | ||
# you should instead start your pull request title with [chore] or use the "Skip Changelog" label. | ||
# Optional: The change log or logs in which this entry should be included. | ||
# e.g. '[user]' or '[user, api]' | ||
# Include 'user' if the change is relevant to end users. | ||
# Include 'api' if there is a change to a library API. | ||
# Default: '[user]' | ||
change_logs: [] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
# Contributing to the Cgroup Go runtime extension | ||
|
||
In order to contribute to this extension, it might be useful to have a working local setup. | ||
|
||
## Testing | ||
|
||
To run the integration tests locally for this extension, you can follow theses steps in a Linux environment. | ||
|
||
Inside the extension folder, start a privileged docker container and share the code with the container | ||
|
||
```bash | ||
cd extension/cgroupruntimeextension | ||
docker run -ti --privileged --cgroupns=host -v $(pwd):/workspace -w /workspace debian:bookworm-slim | ||
``` | ||
|
||
Install Go and gcc to run the integration test | ||
r0mdau marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
```bash | ||
apt update && apt install -y wget sudo gcc && wget https://go.dev/dl/go1.23.4.linux-amd64.tar.gz && tar -C /usr/local -xzf go1.23.4.linux-amd64.tar.gz && export PATH=$PATH:/usr/local/go/bin && go version && rm go1.23.4.linux-amd64.tar.gz | ||
``` | ||
|
||
Run the integration test | ||
|
||
```bash | ||
CGO_ENABLED=1 go test -v -exec sudo -race -timeout 360s -parallel 4 -tags=integration,"" | ||
``` |
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,6 +11,8 @@ import ( | |
"context" | ||
"fmt" | ||
"math" | ||
"net/http" | ||
"net/http/httptest" | ||
"os" | ||
"path" | ||
"path/filepath" | ||
|
@@ -30,6 +32,7 @@ import ( | |
|
||
const ( | ||
defaultCgroup2Path = "/sys/fs/cgroup" | ||
ecsMetadataUri = "ECS_CONTAINER_METADATA_URI_V4" | ||
) | ||
|
||
// checkCgroupSystem skips the test if is not run in a cgroupv2 system | ||
|
@@ -63,6 +66,26 @@ func cgroupMaxCpu(filename string) (quota int64, period uint64, err error) { | |
return quota, period, err | ||
} | ||
|
||
func testServerECSMetadata(t *testing.T, containerCPU, taskCPU int) *httptest.Server { | ||
t.Helper() | ||
|
||
mux := http.NewServeMux() | ||
mux.HandleFunc("/", func(w http.ResponseWriter, _ *http.Request) { | ||
_, err := w.Write([]byte(fmt.Sprintf(`{"Limits":{"CPU":%d},"DockerId":"container-id"}`, containerCPU))) | ||
assert.NoError(t, err) | ||
}) | ||
mux.HandleFunc("/task", func(w http.ResponseWriter, _ *http.Request) { | ||
_, err := w.Write([]byte(fmt.Sprintf( | ||
`{"Containers":[{"DockerId":"container-id","Limits":{"CPU":%d}}],"Limits":{"CPU":%d}}`, | ||
containerCPU, | ||
taskCPU, | ||
))) | ||
assert.NoError(t, err) | ||
}) | ||
|
||
return httptest.NewServer(mux) | ||
} | ||
|
||
func TestCgroupV2SudoIntegration(t *testing.T) { | ||
checkCgroupSystem(t) | ||
pointerInt64 := func(val int64) *int64 { | ||
|
@@ -81,6 +104,7 @@ func TestCgroupV2SudoIntegration(t *testing.T) { | |
config *Config | ||
expectedGoMaxProcs int | ||
expectedGoMemLimit int64 | ||
setECSMetadataURI bool | ||
}{ | ||
{ | ||
name: "90% the max cgroup memory and 12 GOMAXPROCS", | ||
|
@@ -144,6 +168,71 @@ func TestCgroupV2SudoIntegration(t *testing.T) { | |
// 134217728 * 0.1 | ||
expectedGoMemLimit: 13421772, | ||
}, | ||
{ | ||
name: "AWS ECS 90% the max cgroup memory and 12 GOMAXPROCS", | ||
cgroupCpuQuota: pointerInt64(100000), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wdyt of using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussion continuing here: #36920 (comment) |
||
cgroupCpuPeriod: 8000, | ||
// 128 Mb | ||
cgroupMaxMemory: 134217728, | ||
config: &Config{ | ||
GoMaxProcs: GoMaxProcsConfig{ | ||
Enabled: true, | ||
}, | ||
GoMemLimit: GoMemLimitConfig{ | ||
Enabled: true, | ||
Ratio: 0.9, | ||
}, | ||
}, | ||
// 100000 / 8000 | ||
expectedGoMaxProcs: 12, | ||
// 134217728 * 0.9 | ||
expectedGoMemLimit: 120795955, | ||
setECSMetadataURI: true, | ||
}, | ||
{ | ||
name: "AWS ECS 50% of the max cgroup memory and 1 GOMAXPROCS", | ||
cgroupCpuQuota: pointerInt64(100000), | ||
cgroupCpuPeriod: 100000, | ||
// 128 Mb | ||
cgroupMaxMemory: 134217728, | ||
config: &Config{ | ||
GoMaxProcs: GoMaxProcsConfig{ | ||
Enabled: true, | ||
}, | ||
GoMemLimit: GoMemLimitConfig{ | ||
Enabled: true, | ||
Ratio: 0.5, | ||
}, | ||
}, | ||
// 100000 / 100000 | ||
expectedGoMaxProcs: 1, | ||
// 134217728 * 0.5 | ||
expectedGoMemLimit: 67108864, | ||
setECSMetadataURI: true, | ||
}, | ||
{ | ||
name: "AWS ECS 10% of the max cgroup memory, max cpu, default GOMAXPROCS", | ||
cgroupCpuQuota: nil, | ||
cgroupCpuPeriod: 100000, | ||
// 128 Mb | ||
cgroupMaxMemory: 134217728, | ||
config: &Config{ | ||
GoMaxProcs: GoMaxProcsConfig{ | ||
Enabled: true, | ||
}, | ||
GoMemLimit: GoMemLimitConfig{ | ||
Enabled: true, | ||
Ratio: 0.1, | ||
}, | ||
}, | ||
// GOMAXPROCS is set to the value of `cpu.max / cpu.period` | ||
// If cpu.max is set to max, GOMAXPROCS should not be | ||
// modified | ||
expectedGoMaxProcs: runtime.GOMAXPROCS(-1), | ||
// 134217728 * 0.1 | ||
expectedGoMemLimit: 13421772, | ||
setECSMetadataURI: true, | ||
}, | ||
} | ||
|
||
cgroupPath, err := cgroup2.PidGroupPath(os.Getpid()) | ||
|
@@ -198,12 +287,25 @@ func TestCgroupV2SudoIntegration(t *testing.T) { | |
|
||
for _, test := range tests { | ||
t.Run(test.name, func(t *testing.T) { | ||
// if running in ECS environment, set the ECS metedata URI environment variable | ||
// to get the Cgroup CPU quota from the httptest server | ||
cleanECS := func() {} | ||
if test.setECSMetadataURI { | ||
server := testServerECSMetadata(t, test.expectedGoMaxProcs*1024, test.expectedGoMaxProcs*1024) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Related to the comment above, maybe using There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The cgroup2 Because if So I see 3 viable options:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Taking into account that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Proposition: 12db2c1 I moved some redundant code into functions in order to make it cleaner and that's debatable. |
||
os.Setenv(ecsMetadataUri, server.URL) | ||
cleanECS = func() { | ||
server.Close() | ||
os.Unsetenv(ecsMetadataUri) | ||
} | ||
} | ||
|
||
// restore startup cgroup initial resource values | ||
t.Cleanup(func() { | ||
debug.SetMemoryLimit(initialGoMem) | ||
runtime.GOMAXPROCS(initialGoProcs) | ||
memoryCgroupCleanUp() | ||
cpuCgroupCleanUp() | ||
cleanECS() | ||
}) | ||
|
||
err = manager.Update(&cgroup2.Resources{ | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that some Linux distributions already run systemd under cgroupv2, in that case, integration tests can be run without the docker workaround.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Notice added 12acc97