improve validation of cpu-shares, and migrate TestRunInvalidCPUShares#49180
Merged
thaJeztah merged 3 commits intomoby:masterfrom Jan 9, 2025
Merged
improve validation of cpu-shares, and migrate TestRunInvalidCPUShares#49180thaJeztah merged 3 commits intomoby:masterfrom
thaJeztah merged 3 commits intomoby:masterfrom
Conversation
thaJeztah
added
status/2-code-review
kind/enhancement
thaJeztah
force-pushed
the
improve_cpushares_validation
branch
from
3f48ddc to
fdaee58
Compare
thaJeztah
commented
Jan 3, 2025
Comment on lines
138
to
141
| if config.CPUShares < 0 { | ||
| return nil, fmt.Errorf("shares: invalid argument") | ||
| return nil, fmt.Errorf("invalid CPU shares (%d): value must be a positive integer", config.CPUShares) | ||
| } | ||
| if config.CPUShares > 0 { |
Member
Author
There was a problem hiding this comment.
Actually, let me combine these 2 branches.
thaJeztah
force-pushed
the
improve_cpushares_validation
branch
2 times, most recently
from
3624b61 to
0ea952e
Compare
thaJeztah
commented
Jan 6, 2025
| } | ||
| // We're only producing an error when supported to preserve old behavior. | ||
| // The OCI runtime may still reject the config though. | ||
| if si.CPUShares { |
Member
Author
There was a problem hiding this comment.
This comment is written a bit poorly; "when supported" should probably be something like "if CPU-shares are supported"; let me touch it up.
thaJeztah
force-pushed
the
improve_cpushares_validation
branch
from
0ea952e to
c6e9db9
Compare
vvoland
approved these changes
Jan 9, 2025
Make sure we're testing for the right condition. Signed-off-by: Sebastiaan van Stijn <[email protected]>
- rewrite to use subtests - check for actual error returned Signed-off-by: Sebastiaan van Stijn <[email protected]>
This test was testing errors produced by runc; both the "maximum" and "minimum" values originate from the OCI runtime; https://github.com/opencontainers/runc/blob/d48d9cfefcdc13862ade35fd2df428cf9798bf22/libcontainer/cgroups/fs/cpu.go#L66-L83 docker run --cpu-shares=1 alpine docker: Error response from daemon: failed to create shim task: OCI runtime create failed: runc create failed: unable to start container process: error during container init: error setting cgroup config for procHooks process: the minimum allowed cpu-shares is 2: unknown. Happy path for this setting is covered by TestRunWithCPUShares, and various other tests, so we validate that the options take effect; https://github.com/moby/moby/blob/f5af46d4d5db591da5ce330c9936baecd1df5d9c/integration-cli/docker_cli_run_unix_test.go#L494-L503 This patch: - removes the test and migrates it to an integration test - removes the checks for errors that might be produced by runc - updates our validation for invalid (negative) values to happen when creating the contaienr; the existing check that happened when creating the OCI spec is preserved, so that configs of existing containers are still validated. - updates validateResources to return the correct error-type - updated unit-test to validate With this patch: make TEST_FILTER='TestCreateInvalidHostConfig' TEST_SKIP_INTEGRATION_CLI=1 test-integration --- PASS: TestCreateInvalidHostConfig (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_IpcMode (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_CPUShares (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_PidMode (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_PidMode_without_container_ID (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_Annotations (0.00s) --- PASS: TestCreateInvalidHostConfig/invalid_UTSMode (0.00s) Signed-off-by: Sebastiaan van Stijn <[email protected]>
thaJeztah
force-pushed
the
improve_cpushares_validation
branch
from
c6e9db9 to
6d24a21
Compare
thaJeztah
commented
Jan 9, 2025
| // CPU-shares on a system that doesn't support it instead of silently | ||
| // ignoring. | ||
| if hc.Resources.CPUShares < 0 { | ||
| return validationError(fmt.Sprintf("invalid CPU shares (%d): value must be a positive integer", hc.Resources.CPUShares)) |
Member
Author
There was a problem hiding this comment.
While rebasing, changed this to validationError (I used errdefs.InvalidParameter, but we have this error that we used above, so might as well be consistent).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
runconfig: TestValidateResources: fix duplicate test-case
Make sure we're testing for the right condition.
runconfig: TestValidateResources: use subtests
improve validation of cpu-shares, and migrate TestRunInvalidCPUShares
This test was testing errors produced by runc; both the "maximum" and
"minimum" values originate from the OCI runtime;
https://github.com/opencontainers/runc/blob/d48d9cfefcdc13862ade35fd2df428cf9798bf22/libcontainer/cgroups/fs/cpu.go#L66-L83
Happy path for this setting is covered by TestRunWithCPUShares, and
various other tests, so we validate that the options take effect;
moby/integration-cli/docker_cli_run_unix_test.go
Lines 494 to 503 in f5af46d
This patch:
when creating the contaienr; the existing check that happened when
creating the OCI spec is preserved, so that configs of existing containers
are still validated.
With this patch:
- What I did
- How I did it
- How to verify it
- Description for the changelog
- A picture of a cute animal (not mandatory but encouraged)