Change the rlimit type to string instead of int

[mrunalp]

Signed-off-by: Mrunal Patel mrunalp@gmail.com

[vbatts]

should we set some const for these? (RLIMIT_CPU, RLIMIT_AS, ...)

[mrunalp]

@vbatts I think for this case, we could let the runtime map from string to the constants just like for capabilities. That way we don't have to track changes as newer constants are added.

[wking]

Touching this line is probably a good enough excuse to reformat with one line per-sentence.

[wking]

I'd rather drop the leading RLIMIT_. See 1 in #109 and 2 in #160.

[vbatts]

@wking but that is less clear?

[wking]

On Tue, Sep 08, 2015 at 01:42:00PM -0700, Vincent Batts wrote:

@wking but that is less clear?

Why is it less clear? The context (linux.rlimits) should be enough to
make it clear that these are rlimit values. For example, may
discussions of kernel config parameters drop the leading CONFIG_. If
there's a strong feeling that the RLIMIT_-less version is unclear,
then I'll survive adding it in, but it feels redundant to me ;).

[vbatts]

Those dropped-prefixes are idioms elected for those projects internal
references. Externally their reference is the full name. Doing this is to
propose that we impose special prefix logic those that implement.
On Sep 8, 2015 4:47 PM, "W. Trevor King" notifications@github.com wrote:

On Tue, Sep 08, 2015 at 01:42:00PM -0700, Vincent Batts wrote:

@wking but that is less clear?

Why is it less clear? The context (linux.rlimits) should be enough to
make it clear that these are rlimit values. For example, may
discussions of kernel config parameters drop the leading CONFIG_. If
there's a strong feeling that the RLIMIT_-less version is unclear,
then I'll survive adding it in, but it feels redundant to me ;).

—
Reply to this email directly or view it on GitHub
#159 (comment).

[wking]

On Tue, Sep 08, 2015 at 02:20:01PM -0700, Vincent Batts wrote:

Those dropped-prefixes are idioms elected for those projects
internal references. Externally their reference is the full
name.

That's not just because C doesn't namespace definitions? Since we get
namespacing via the config's JSON keys, it seems redundant to also
have namespace prefixes on the values themselves. Are you worried
about folks seeing something like ‘CPU’ out of context and not
realizing that it's a resource-limit at all? Or are you worried that
it will be hard to grep for resource-limit-specific occurences of CPU
without also hitting other occurences of CPU (e.g. if there was a
CAP_CPU)?

[vbatts]

Context is something to be aware of. Honestly, I'm not sure whether you're
arguing for actual technical reasons and simplistic design, or just to
derive another idiom for implementation. if the capability has CAP_ in
front, then have CAP in front. We do not control whether one day that
prefix changes, and now the semantic pattern is broken.

On Tue, Sep 8, 2015 at 5:31 PM, W. Trevor King notifications@github.com
wrote:

On Tue, Sep 08, 2015 at 02:20:01PM -0700, Vincent Batts wrote:

Those dropped-prefixes are idioms elected for those projects
internal references. Externally their reference is the full
name.

That's not just because C doesn't namespace definitions? Since we get
namespacing via the config's JSON keys, it seems redundant to also
have namespace prefixes on the values themselves. Are you worried
about folks seeing something like ‘CPU’ out of context and not
realizing that it's a resource-limit at all? Or are you worried that
it will be hard to grep for resource-limit-specific occurences of CPU
without also hitting other occurences of CPU (e.g. if there was a
CAP_CPU)?

—
Reply to this email directly or view it on GitHub
#159 (comment).

[wking]

On Tue, Sep 08, 2015 at 03:39:11PM -0700, Vincent Batts wrote:

Context is something to be aware of. Honestly, I'm not sure whether
you're arguing for actual technical reasons and simplistic design,
or just to derive another idiom for implementation. if the
capability has CAP_ in front, then have CAP in front. We do not
control whether one day that prefix changes, and now the semantic
pattern is broken.

I think it's unlikely that the prefix will change ;). But there's no
harm other than redundancy for keeping the CAP_ around, and I guess a
little redundancy and extra typing isn't a big deal :p.

[mrunalp]

One could argue both ways but I think it is better to be explicit here. Also, some tooling could benefit by directly using these definitions from their header files.

[wking]

On Tue, Sep 08, 2015 at 04:03:58PM -0700, Mrunal Patel wrote:

One could argue both ways but I think it is better to be explicit
here.

This seems fair.

Also, some tooling could benefit by directly using these definitions
from their header files.

Adding the prefix back in before searching headers seems easy enough,
so I'm not buying the tool-support argument ;).

But “the redunancy reduction from removing the namespacing prefix is
not useful enough to be worth trimming the upstream identifier” seems
sufficiently reasonable to carry this PR's approach without appealing
to tool-support.

[wking]

On Tue, Sep 08, 2015 at 03:39:11PM -0700, Vincent Batts wrote:

Honestly, I'm not sure whether you're arguing for actual technical
reasons and simplistic design…

And I think everyone who's chimed in is on board with prefixed strings
now, but the seed of my initial resistance was preserving the current
capabilities deprefixing (which is going away with #160). And we've
had that since the ancient days of 77d44b1 (Update runtime.md,
2015-06-16). Unfortunately that commit landed in the pre-PR days, so
there's no motivation for its use of deprefixed capabilities strings.
Since that stuff was mostly from libcontainer, I looked there, and the
deprefixed form landed with docker-archive/libcontainer@6415e8be (Initial
commit of libcontainer, 2014-02-18). I haven't dug into the
provenance of that code, so I'm not sure what the original
motivation was for the deprefixed approach (maybe it was like my
preference, since sidelined, for avoiding redundancy).

[philips]

Seems fine, needs a rebase.

[mrunalp]

Rebased.

[philips]

lgtm

[vbatts]

LGTM