-
Notifications
You must be signed in to change notification settings - Fork 950
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
feature: add oom flag #934
Conversation
Codecov Report
@@ Coverage Diff @@
## master #934 +/- ##
==========================================
- Coverage 13.12% 13.09% -0.04%
==========================================
Files 123 123
Lines 8197 8219 +22
==========================================
Hits 1076 1076
- Misses 7025 7047 +22
Partials 96 96
Continue to review full report at Codecov.
|
|
||
resp, err := request.Post("/containers/create", query, body) | ||
c.Assert(err, check.IsNil) | ||
CheckRespStatus(c, resp, 201) |
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.
It tested that the container creation works. Please test further to check if the ContainerMeta contains this field with the inspect API.
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.
this will be test in test/cli_create_test.go
test/cli_create_test.go
Outdated
c.Errorf("failed to decode inspect output: %v", err) | ||
} | ||
c.Assert(*result.HostConfig.OomScoreAdj, check.Equals, int64(100)) | ||
c.Assert(*result.HostConfig.OomKillDisable, check.Equals, true) |
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.
Can we tested an oom container with some way? For example, create a container with less memory, but the application in the container needs more memory than the limit.
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.
maybe it hard to write test like this in ci test.
cli/container.go
Outdated
@@ -415,3 +423,12 @@ func parseNetwork(network string) (string, string, string, error) { | |||
|
|||
return name, parameter, mode, nil | |||
} | |||
|
|||
// validate oom score | |||
func validOOMScore(score int64) error { |
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.
s/validOOMScore/validateOOMScore?
apis/swagger.yml
Outdated
The range is in [-1000, 1000]. | ||
type: "integer" | ||
format: "int" | ||
x-nullable: true |
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.
I do not think this should be x-nullable: true
, since it would be a little bit incompatible with moby 1.12.6's API.
So it should be x-nullable: false
@allencloud , updated. |
Signed-off-by: Ace-Tang <aceapril@126.com>
LGTM |
Ⅰ. Describe what this PR did
add oom-score-adj and oom-kill-disable flag
Ⅱ. Does this pull request fix one issue?
Ⅲ. Describe how you did it
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews