runtime: config: linux: Edit BlockIO struct

[runcom]

WeightDevice, ThrottleReadBpsDevice, ThrottleWriteBpsDevice, ThrottleReadIOpsDevice, ThrottleWriteIOpsDevice are now slices to well defined structs to allow setting multiple devices in their respective blkio file. By using a string to represents those values it wasn't possible to set correct values when multiple devices were passed in the config (either newline separated or comma separated).

I don't know how this worked before to define multiple devices for these cgroups resources. Writing something like 8:0 500\n8:16 300 isn't working (there's a Docker PR moby/moby#13959 which passes that format to libcontainer and by passing that format to current master runc isn't working as well (neither using comma) because it just writes it to cgroup file which doesn't accept newlines/commas).
8:0 500\n8:16 300 will only set the first device before the \n when written to blkio.weight_device for instance.

I'm not fond of blockIODevice embedded struct and specifying major:minor for this. I think it will suffice specifying just a Path and do the syscall.Stat to retrieve major:minor where it needs to (I'm thinking about libcontainer as it's currently doing for Rlimits for instance).
By just specifing a Path this will help also for example to just receive a string like /dev/sda and do the major:minor convert in the implementation. It's useful to have the plain string like for checking which scheduler the device is using (/sys/block/sda/queue/scheduler w/o converting back major:minor to path in implementation).

I'll open an issue in runc to track this and update runc/libcontainer if this patch is correct and lands.

Signed-off-by: Antonio Murdaca [email protected]

[wking]

There doesn't seem to be much point in having these examples if they're all null. Maybe drop some of them completely, and then set the remaining ones to something more useful to show the new syntax?

[runcom]

I left it as it was but yes makes sense I'll update the example

[runcom]

Fixed

[wking]

On Wed, Sep 16, 2015 at 07:51:53AM -0700, Antonio Murdaca wrote:

I'm not fond of blockIODevice embedded struct and specifying
major:minor for this. I think it will suffice specifying just a
Path and do the syscall.Stat to retrieve major:minor where it
needs to (I'm thinking about libcontainer as it's currently doing
for Rlimits for instance).

I'm fine with blockIODevice. One benefit is that we don't need to
specify ordering between mounting, mknod calls, and these resource
limits, because we can set major:minor limits before the path exists.

[runcom]

On Wed, Sep 16, 2015 at 07:51:53AM -0700, Antonio Murdaca wrote:
I'm not fond of blockIODevice embedded struct and specifying
major:minor for this. I think it will suffice specifying just a
Path and do the syscall.Stat to retrieve major:minor where it
needs to (I'm thinking about libcontainer as it's currently doing
for Rlimits for instance).

I'm fine with blockIODevice. One benefit is that we don't need to
specify ordering between mounting, mknod calls, and these resource
limits, because we can set major:minor limits before the path exists.

fine, I guess /proc/partitions or something could be used to map device numbers back to path but shouldn't be a problem now

[wking]

89bed76 looks good to me, although I'm a bit surprised that all we
have to update are the Go types and examples. Can we add a longer
example linking to 1 and sketching out the intended mapping between
our docs and that interface? A list of options for each type with
their intended type, the fact that they're required, and a one-line
summary of their interpretation would help me anyway. For example:

• major (int64, required) number for the devices that will be limited.
• minor (int64, required) number for the devices that will be limited.
• weight (int16, required) in CFQ IO scheduling

where I've tried to use the “whole thing reads like a sentence”
phrasing 2.

[runcom]

89bed76 looks good to me, although I'm a bit surprised that all we
have to update are the Go types and examples. Can we add a longer
example linking to 1 and sketching out the intended mapping between
our docs and that interface? A list of options for each type with
their intended type, the fact that they're required, and a one-line
summary of their interpretation would help me anyway. For example:

• major (int64, required) number for the devices that will be limited.
• minor (int64, required) number for the devices that will be limited.
• weight (int16, required) in CFQ IO scheduling

where I've tried to use the “whole thing reads like a sentence”
phrasing 2.

Sounds good to have these docs around, I'll update.
Also, I guess the other resources fields need a bit of doc bump as well, I'll look into this as well in another PR.

[wking]

On Wed, Sep 16, 2015 at 12:51:13PM -0700, W. Trevor King wrote:

…
[1]: https://www.kernel.org/doc/Documentation/cgroups/blkio-controller.txt

These also talk about leaf_weight. While we're formalizing these
entries, do we want to expose that too?

Maybe with:

• weight (int16, optional) when competing within this cgroup in
  CFQ IO scheduling

• leafWeight (int16, optional) when competing with cgroup children
  in CFQ IO scheduling

  You must specify at least one of weight or leafWeight in a given
  entry, and can specify both.

Or we can have separate entries (like you're currently doing for
weight and throttle) for handling weight and leaf_weight. Or we can
punt for now, and come back and look at leaf_weight when someone asks
for it with a particular use-case in mind.

[wking]

On Wed, Sep 16, 2015 at 12:56:08PM -0700, Antonio Murdaca wrote:

sounds good to have these docs around, I'll update

I had some other cgroups docs stubbing in #99 if you want to borrow
from there (although there hasn't been discussion about explicitly
handling device-access cgroups, so probably leave that section out ;).

[runcom]

Or we can have separate entries (like you're currently doing for
weight and throttle) for handling weight and leaf_weight.

these are separate cause one is dealing with weight and the other with rate

Or we can
punt for now, and come back and look at leaf_weight when someone asks
for it with a particular use-case in mind.

It shoudln't be so much hard to put leaf_weight into account. And I like your proposed struct with both weight and leaf_weight.

I had some other cgroups docs stubbing in #99 if you want to borrow
from there (although there hasn't been discussion about explicitly
handling device-access cgroups, so probably leave that section out ;).

Thanks!

[hqhq]

I'm not fond of blockIODevice embedded struct and specifying major:minor for this. I think it will suffice specifying just a Path and do the syscall.Stat to retrieve major:minor where it needs to (I'm thinking about libcontainer as it's currently doing for Rlimits for instance).

Though systemd takes device path instead of major minor number, but for struct, I think is't better to store just major minor numbers.

And +1 for this change.

[runcom]

Though systemd takes device path instead of major minor number, but for struct, I think is't better to store just major minor numbers.

systemd was one of the reason I thought having Path would have been good. But whatever, I agree having device numbers is better.

[runcom]

ping @vbatts @mrunalp @LK4D4 @wking @hqhq

I've updated the structure of the cgroups section here by splitting the example into per resource section and filled the bllkio one as per current PR topic. If you're happy with this path/format I can do a follow up PR updating the rest of the resources (I'd leave this PR as is now w/o going any further with other modifications if you're ok with this)

[wking]

I think our other examples use four-space indents for JSON, so it's probably better to stick with that and skip the tabs.

[runcom]

Sure, missed that

[wking]

On Thu, Sep 17, 2015 at 05:55:38AM -0700, Antonio Murdaca wrote:

I've updated the structure of the cgroups section here by splitting
the example into per resource section and filled the bllkio one as
per current PR topic.

Looks good to me :). I've left a few copy-edit suggestions, but I'm
on board with all the significant changes in a89565b. I'd be fine if
it landed as-is, with follow up PRs to accept/reject my copy-edit
suggestions (but I'd rather they got rerolled in or rejected here ;).

[crosbymichael]

LGTM

[mrunalp]

typo: blkioThrottleReadIOPSDevicei

[runcom]

fixed

[mrunalp]

LGTM

[runcom]

Thanks, I'll create a PR in runc to update libcontainer

[mrunalp]

@runcom Sounds good!