libcontainer: cgroups: deal with unlimited case for pids.max

[cyphar]

Make sure we don't error out collecting statistics for cases where
pids.max == "max". In that case, we can use a limit of 0 which means
"unlimited".

Signed-off-by: Aleksa Sarai [email protected]

Sorry, I forgot about this case in #640. This should fix it.

/cc @hqhq @mrunalp

[crosbymichael]

Instead of -1 is there like maxint that can be used? Is there a limit in the kernel for number of pids?

[crosbymichael]

Should we use something like /proc/sys/kernel/pid_max or since this can change maybe -1 is a better solution?

[cyphar]

My main reason for using -1 is because it mirrors the spec for setting the limit. Admittedly, I'd prefer if it was a uint64 (because that's what the type in the kernel /actually is/), but it probably makes the API more intuitive when it's -1. As you said, I'm not really comfortable about basing it on /proc/sys/kernel/pid_max because it can be changed.

[hqhq]

What about *uint64? nil means unlimited. But I'm OK with both *uint64 and -1.

[cyphar]

@hqhq Heh, I feel like we're reliving the discussion in #58. Since none of the other stats structs I can see use pointers (and they don't make sense if you contrast the use with opencontainers/runtime-spec#233), I feel like we should just stick with < 0 being "max" (even though we should probably consider whether 0 should be "max" as well).

[crosbymichael]

Maybe 0 is better since its the default value then things can check for pids > 0 ??

[cyphar]

If you're fine with 0, then I am too. At the end of the day, the value we pick is arbitrary. One last thing to consider is the fact that the PIDs cgroup is heirarchical, so we have to make a decision about what value to return in this situation:

% cat /sys/fs/cgroup/pids/some/container/pids.max
1024
% cat /sys/fs/cgroup/pids/some/pids.max
512

Should we return 512 because that's the "effective" limit? Or 1024 because it's the limit that's been set on the container?

[mrunalp]

I think that we should return the value set for the container. Anything outside should be properly managed by the admin or higher orchestration layer.

[cyphar]

I've updated the following:

• Changed "max" to be represented as 0, not -1.
• Update the relevant tests.
• Switch the stats attribute name to Limit (to mirror the name we use to /set/ the limit).

/cc @mrunalp @crosbymichael

[hqhq]

LGTM

[jessfraz]

fixes my problem in #664 :)

[mrunalp]

LGTM