support setting oom_score_adj by haiyanmeng · Pull Request #176 · opencontainers/runtime-tools

haiyanmeng · 2016-08-02T20:39:39Z

Signed-off-by: Haiyan Meng hmeng@redhat.com

haiyanmeng · 2016-08-02T20:39:52Z

wking · 2016-08-02T20:44:28Z

Hmm. Is there a way to clear oom_score_adj with this interface? Does IntFlag support explicitly setting a nil value (e.g. ‘--oom-score-adj=’ with no argument).

mrunalp · 2016-08-02T21:22:08Z

IIRC, =FALSE works for setting nil value.

mrunalp · 2016-08-02T21:25:07Z

@wking nvm, I misunderstood your question.

mrunalp · 2016-08-02T21:26:05Z

LGTM

wking · 2016-08-02T22:07:28Z

On Tue, Aug 02, 2016 at 02:25:07PM -0700, Mrunal Patel wrote:

@wking nvm, I misunderstood your question.

I haven't tested =FALSE, but I think we want to figure out how we
intend to handle clearing values before we dive too deeply into the
IntFlag approach.

Mashimiao · 2016-08-03T01:08:42Z

 	cli.StringSliceFlag{Name: "seccomp-errno", Usage: "specifies syscalls to be added to list that returns an error"},
 	cli.StringFlag{Name: "template", Usage: "base template to use for creating the configuration"},
 	cli.StringSliceFlag{Name: "label", Usage: "add annotations to the configuration e.g. key=value"},
+	cli.IntFlag{Name: "oom-score-adj", Usage: "specify oom_score_adj for the container"},


specify->specifies
I think usage should keep same with man

@Mashimiao , I was struggling with this when I made the change.
The man doc always use specifies, adds, uses. However, most of flags defined in cmd/ocitools/generate.go are using specify,add,use`.
I will first change it to a phrase instead of a verb + a noun.

haiyanmeng · 2016-08-03T14:05:16Z

Is there a way to clear oom_score_adj with this interface?

@wking, I am not sure what do you really want here.
The range of oom_score_adj can be -1000 to 1000.
If you want to increase the badness score of a process, just set a larger value.
If you want to decrease the badness score of a process, just set to a smaller value.
If you do not want to mess up with the badness score at all, just set it to be 0.

Signed-off-by: Haiyan Meng <hmeng@redhat.com>

haiyanmeng · 2016-08-03T14:12:26Z

@mrunalp , @Mashimiao , PTAL.

mrunalp · 2016-08-03T16:41:06Z

Still LGTM

wking · 2016-08-03T16:42:47Z

GitHub seems to have eaten my emailed comment, so re-posting via the web UI:

On Wed, Aug 03, 2016 at 07:36:34AM -0700, hmeng-19 wrote:

Is there a way to clear oom_score_adj with this interface?
@wking, I am not sure what do you really want here.
…
If you do not want to mess up with the badness score at all, just
set it to be 0.

Resources.OOMScoreAdj is an *int, and it's a pointer (like many
other optional settings) because nil is clearly “don't mess with this”
while 0 is sometimes “don't mess with this” and sometimes not. With
your current implementation (1915407):

$ ./ocitools generate --template <(echo '{"linux": {"resources": {"oomScoreAdj": 5}}}') --oom-score-adj 0 | jq .linux
{
"resources": {
"devices": null,
"oomScoreAdj": 0
}
}

But I want a way to set the *int to nil so it doesn't get printed at
all (I've filed opencontainers/runtime-spec#526 to address the useless
‘devices’ entry).

wking · 2016-08-03T16:48:36Z

On Wed, Aug 03, 2016 at 07:36:34AM -0700, hmeng-19 wrote:

Is there a way to clear oom_score_adj with this interface?
@wking, I am not sure what do you really want here.
…
If you do not want to mess up with the badness score at all, just
set it to be 0.

Resources.OOMScoreAdj is an *int, and it's a pointer (like many
other optional settings) because nil is clearly “don't mess with this”
while 0 is sometimes “don't mess with this” and sometimes not. With
your current implementation (1915407):

$ ./ocitools generate --template <(echo '{"linux": {"resources": {"oomScoreAdj": 5}}}') --oom-score-adj 0 | jq .linux
{
"resources": {
"devices": null,
"oomScoreAdj": 0
}
}

But I want a way to set the *int to nil so it doesn't get printed at
all (I've filed opencontainers/runtime-spec#526 to address the useless
‘devices’ entry).

haiyanmeng · 2016-08-03T17:41:30Z

@wking , first I am not sure we want to support the use cases you pointed out or not. If I understand correctly, you want to remove some fields from a template completely. Right?

I would suggest always feed the minimal config.json as the template, and then only add new fields into it.

If you really want to remove fields from a template, then we probably should add some new flags like --remove-oom-score-adj.

haiyanmeng · 2016-08-03T17:48:23Z

GitHub seems to have eaten my emailed comment, so re-posting via the web UI:

@wking , it seems github was slow this morning. I noticed there was a 40 minutes delay before I got gmail notifications for others' comments.

wking · 2016-08-03T18:27:53Z

On Wed, Aug 03, 2016 at 10:56:48AM -0700, hmeng-19 wrote:

I would suggest always feed the minimal config.json as the template,
and then only add new fields into it.

I think ‘ocitools generate --template ...’ is positioning itself to be
a generic tool for configuration edits, although I'm fine also using
jq for this sort of thing. If you are using ocitools to edit an
existing config, the source config may be provided from a third party
who doesn't realize you won't want oom_score_adj.

If you really want to remove fields from a template, then we
probably should add some new flags like --remove-oom-score-adj.

That's one approach, and the implementation might be more
straightforward. I think the command line API would be more
straightforward if it supported ‘--oom-score-adj=’, but that doesn't
work with IntFlag. Because the IntFlag pattern is young (for
ocitools), I think now is a good time to decide:

Do we want to support setting nil for *int-type fields in general?
If we do, do we want the pattern to be ‘--remove-abc’ or ‘--abc=’?

With jq being a convenient, generic solution to config edits, I think
all of the possible choices are defensible. But I'd like to see
explicit policy choices before forging ahead with the implementation
so we stay consistent.

haiyanmeng · 2016-08-03T18:38:56Z

@mrunalp , how do you think about this?

mrunalp · 2016-08-03T18:40:00Z

Do we want to support setting nil for *int-type fields in general?

Yes. But, it shouldn't block PRs while we figure it out as starting with minimal config.json is a workaround.

If we do, do we want the pattern to be ‘--remove-abc’ or ‘--abc=’?

I think it would be good if we avoid adding new flags, but I don't see how --abc= will work for setting to nil unless the library we are using supports it by special treating some text like NIL to mean make it nil.

haiyanmeng · 2016-08-03T18:40:07Z

@wking , I tried to use the --oom-score-adj=, it would fail:

[hmeng@localhost c4]$ ocitools generate --output config.json --oom-score-adj=
FATA[0000] invalid value "" for flag -oom-score-adj: strconv.ParseInt: parsing "": invalid syntax

wking · 2016-08-03T18:50:02Z

On Wed, Aug 03, 2016 at 11:40:01AM -0700, Mrunal Patel wrote:

If we do, do we want the pattern to be ‘--remove-abc’ or ‘--abc=’?
I think it would be good if we avoid adding new flags, but I don't
see how --abc= will work for setting to nil unless the library we
are using supports it by special treating some text like NIL to mean
make it nil.

I think that means we want to use a StringFlag here and add our own
StringFlag → *int, etc. helpers.

haiyanmeng · 2016-08-03T19:02:35Z

I think that means we want to use a StringFlag here and add our own
StringFlag → *int, etc. helpers.

@wking , I would suggest we first merge PR.
Then we can open a new issue to discuss about which flags are going to be converted from IntFlag to StringFlag, or other possible solutions to tackle the problem here.

wking · 2016-08-03T19:09:12Z

On Wed, Aug 03, 2016 at 12:02:36PM -0700, hmeng-19 wrote:

I think that means we want to use a StringFlag here and add our
own StringFlag → *int, etc. helpers.
@wking , I would suggest we first merge PR. Then we can open a new
issue to discuss about which flags are going to be converted from
IntFlag to StringFlag, or other possible solutions to tackle the
problem here.

If you like. I don't see much need to push this through if we already
know we're going to revisit/discuss. It isn't a big deal either way,
it's just more churn to backport to v1.0.0.rc1 if we merge fixup PRs
;).

haiyanmeng · 2016-08-03T19:16:07Z

@mrunalp , I think we can merge this PR for now, so that we can vendor it into ocid to set up oom_score_adj.

mrunalp · 2016-08-03T22:37:04Z

@wking By using StringFlag we lose some of the checks that we get from the CLI library.

wking · 2016-08-03T22:53:33Z

On Wed, Aug 03, 2016 at 03:37:05PM -0700, Mrunal Patel wrote:

@wking By using StringFlag we lose some of the checks that we get
from the CLI library.

So put the checks in our helper? Are the checks really that
complicated?

mrunalp · 2016-08-04T18:41:34Z

@wking Yeah, we could look into that. We can open an issue to track this.

mrunalp · 2016-08-05T16:55:39Z

LGTM (we can figure out the CLI conversion on the side).

Signed-off-by: Haiyan Meng <hmeng@redhat.com> Backported to v1.0.0.rc1 from 1915407opencontainers#176 (cherry-pick applied cleanly). Signed-off-by: W. Trevor King <wking@tremily.us>

* v1.0.0.rc1: man/ocitools.1: Replace "**...(1)**" with "**...**(1)" add namespace check for uid/gid mappings validation: add linux resource check add label manpage and fix help support setting oom_score_adj generate: fix mount-cgroups bug completions: update based on generate help message update urfave/cli package to v1.18.0 generate: optimize namespace setup log and fix manpage change param type of AddProcessAdditionalGid remove unnecessary return error value Modify generate API Add Travis CI badge to README generate: fix capability.List() for cap_last_cap not exist generate: remove unnecessary spec initialization generate: fix tmpfs adding based on manpage Check CAP_LAST_CAP while setting privileged generate: Remove superfluous err check from Save Signed-off-by: W. Trevor King <wking@tremily.us> Conflicts: cmd/ocitools/generate.go man/ocitools-generate.1.md The conflicts are because: * support setting oom_score_adj (opencontainers#176, opencontainers#185) * add label manpage and fix help (opencontainers#189, opencontainers#190) have landed in master and been backported to v1.0.0.rc1 since this branch split from master. They wouldn't have happend if I'd rebased this branch on the current master before merging v1.0.0.rc1, but then I'd have to repeat the initial dance done with eac0762 (Merge commit '30e2ea2', 2016-08-02) and b45bebd (Merge commit '6acca9e', 2016-08-02).

haiyanmeng force-pushed the set_oom_score_adj branch from 930108c to 8e409e5Compare August 2, 2016 20:40

Mashimiao reviewed Aug 3, 2016
View reviewed changes

support setting oom_score_adj
1915407
Signed-off-by: Haiyan Meng <hmeng@redhat.com>

haiyanmeng force-pushed the set_oom_score_adj branch from 8e409e5 to 1915407Compare August 3, 2016 14:11

mrunalp merged commit 03e8b89 into opencontainers:masterAug 5, 2016

haiyanmeng deleted the set_oom_score_adj branch August 5, 2016 17:20

wking mentioned this pull request Aug 5, 2016
[1.0.0.rc1] support setting oom_score_adj #185
Merged

wking mentioned this pull request Sep 14, 2016
generate: add pids limit support #206
Merged

wking mentioned this pull request Sep 11, 2017
Unable to escape colons in option lists #470
Closed

Uh oh!

Conversation

haiyanmeng commented Aug 2, 2016

Uh oh!

haiyanmeng commented Aug 2, 2016

Uh oh!

wking commented Aug 2, 2016 via email

Uh oh!

mrunalp commented Aug 2, 2016

Uh oh!

mrunalp commented Aug 2, 2016

Uh oh!

mrunalp commented Aug 2, 2016

Uh oh!

wking commented Aug 2, 2016

Uh oh!

MashimiaoAug 3, 2016

Choose a reason for hiding this comment

Uh oh!

haiyanmengAug 3, 2016

Choose a reason for hiding this comment

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

mrunalp commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

mrunalp commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016

Uh oh!

haiyanmeng commented Aug 3, 2016

Uh oh!

mrunalp commented Aug 3, 2016

Uh oh!

wking commented Aug 3, 2016

Uh oh!

mrunalp commented Aug 4, 2016

Uh oh!

mrunalp commented Aug 5, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

wking commented Aug 3, 2016•
edited
Loading