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.

Sign up for GitHub

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

Jump to bottom

Add OomScoreAdj #16277

Merged
merged 1 commit into from
Dec 2, 2015
Merged

Add OomScoreAdj #16277

merged 1 commit into from
Dec 2, 2015

Conversation

runcom
Copy link
Member

@runcom runcom commented Sep 14, 2015

libcontainer v0.0.4 introduces setting /proc/self/oom_score_adj to
better tune oom killing preferences for container process. This patch
simply integrates OomScoreAdj libcontainer's config option and adjust
the cli with this new option.

close #13116

TODOs:

  • Docs
  • Tests

Signed-off-by: Antonio Murdaca runcom@linux.com

@runcom runcom changed the title Add --oom-score-adj [WIP] Add --oom-score-adj Sep 14, 2015
@runcom runcom force-pushed the add-oom-score-adj branch 2 times, most recently from 427de1e to 2bc25ae Compare September 14, 2015 11:15
@runcom runcom changed the title [WIP] Add --oom-score-adj [WIP] Add OomScoreAdj Sep 14, 2015
@runcom runcom force-pushed the add-oom-score-adj branch 2 times, most recently from 91e1cc3 to 0a49ad8 Compare September 14, 2015 16:23
RichardScothern
RichardScothern reviewed Sep 14, 2015
View reviewed changes
runconfig/parse.go
@@ -71,6 +71,7 @@ func Parse(cmd *flag.FlagSet, args []string) (*Config, *HostConfig, *flag.FlagSe
flStdin = cmd.Bool([]string{"i", "-interactive"}, false, "Keep STDIN open even if not attached")
flTty = cmd.Bool([]string{"t", "-tty"}, false, "Allocate a pseudo-TTY")
flOomKillDisable = cmd.Bool([]string{"-oom-kill-disable"}, false, "Disable OOM Killer")
flOomScoreAdj = cmd.Int([]string{"-oom-score-adj"}, 0, "Tune container OOM score adjustment")
Copy link

Choose a reason for hiding this comment

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

Should this description contain the accepted range (-1000, 1000)?

@runcom runcom force-pushed the add-oom-score-adj branch 2 times, most recently from acc21cc to 909e519 Compare September 14, 2015 19:01
@runcom runcom force-pushed the add-oom-score-adj branch 9 times, most recently from 899b7af to dfeacbf Compare September 18, 2015 18:23
@jessfraz jessfraz changed the title [WIP] Add OomScoreAdj Add OomScoreAdj Oct 8, 2015
@icecrime
Copy link
Contributor

icecrime commented Oct 8, 2015

Design OK

@icecrime
Copy link
Contributor

icecrime commented Oct 8, 2015

Ping @runcom for rebase before code review!

@runcom runcom force-pushed the add-oom-score-adj branch from dfeacbf to 4ff44c7 Compare October 8, 2015 19:01
@runcom
Copy link
Member Author

runcom commented Oct 8, 2015

@icecrime rebased

@vdemeester
Copy link
Member

@runcom looks like the vet is not happy 😅

thaJeztah
thaJeztah reviewed Nov 26, 2015
View reviewed changes
contrib/completion/zsh/_docker
@@ -482,6 +482,7 @@ __docker_subcommand() {
"($help)--name=[Container name]:name: "
"($help)--net=[Connect a container to a network]:network mode:(bridge none container host)"
"($help)--oom-kill-disable[Disable OOM Killer]"
"($help)--oom-score-adj[Tune OOM preferences for container (-1000 to 1000)]"
Copy link
Member

Choose a reason for hiding this comment

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

Can you use the same text here as well?

@thaJeztah
Copy link
Member

@moxiegirl I see we're inconsistent with describing the options; Tunes ... something v.s. Tune ... something. I think we should pick one direction, so either "(this option) does something", or "(use this option to) do something"; wdyt?

@moxiegirl
Copy link
Contributor

@thaJeztah Yes, I noticed that. I think that is a good idea but the problem is beyond the scope of this PR. It is really something we should do in another PR. WDYT?

@thaJeztah
Copy link
Member

@moxiegirl I think so far we used Tune... but your suggestions here were Tunes.., that was my reason for mentioning it. But if there are other inconsistencies, I'm perfectly fine changing it in a separate PR

@moxiegirl
Copy link
Contributor

Ah, I see. Yes, you are right Tune is more consistent with the other options.

@runcom
Copy link
Member Author

runcom commented Nov 30, 2015

@moxiegirl @thaJeztah updated (I'll leave what's not consistent to a follow up PR as you agreed on)

@thaJeztah
Copy link
Member

@runcom looks like I overlooked that changes to the man-pages are missing, can you add the flag to the man-pages for create and run? 😊

@runcom
Copy link
Member Author

runcom commented Nov 30, 2015

@thaJeztah added

@thaJeztah
Copy link
Member

Thanks! And thanks for being patient with us 👍

LGTM

ping @moxiegirl for a final look

@runcom
Add OomScoreAdj to configure container oom killer preferences
d3af7f2 
libcontainer v0.0.4 introduces setting `/proc/self/oom_score_adj` to
better tune oom killing preferences for container process. This patch
simply integrates OomScoreAdj libcontainer's config option and adjust
the cli with this new option.

Signed-off-by: Antonio Murdaca <amurdaca@redhat.com>
Signed-off-by: Antonio Murdaca <runcom@redhat.com>
@runcom
Copy link
Member Author

runcom commented Dec 2, 2015

ping @moxiegirl O:)

@moxiegirl
Copy link
Contributor

@runcom LGTM to you! :-D Thank you. @thaJeztah for the merge

icecrime pushed a commit that referenced this pull request Dec 2, 2015
Merge pull request #16277 from runcom/add-oom-score-adj
8f1f53f 
Add OomScoreAdj
@icecrime icecrime merged commit 8f1f53f into moby:master Dec 2, 2015
@runcom runcom deleted the add-oom-score-adj branch December 2, 2015 19:50
@thaJeztah thaJeztah added this to the 1.10.0 milestone Jan 15, 2016
This was referenced Jan 25, 2016
Closed
Closed
@resouer resouer mentioned this pull request Jan 27, 2016
Merged
@awasilyev awasilyev mentioned this pull request Apr 7, 2016
Closed
@Knetic Knetic mentioned this pull request Jun 8, 2016
Closed
@thaJeztah thaJeztah mentioned this pull request Mar 24, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
impact/api impact/changelog impact/cli status/4-merge
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

Support setting OOM score