config-linux.md: part fix of blkio spec #825

Mashimiao · 2017-05-14T15:36:45Z

blkioWeightDevice is not direct for bindwidth rate limit, actually
used for weight division.
bpsdeivce limits are different from IOpsdevice, they are limit
rate in bytes, say used for bandwidth limit will be better.

Signed-off-by: Ma Shimiao mashimiao.fnst@cn.fujitsu.com

wking · 2017-05-15T18:11:11Z

On Sun, May 14, 2017 at 08:36:46AM -0700, Ma Shimiao wrote: blkioWeightDevice is not direct for bindwidth rate limit, actually used for weight division.

Can we punt more directly to the backing kernel cgroup API? For example, see the in-flight #745 where I'm trying to tie `disableOOMKiller` to the `memory.oom_control` file. If the spec is clear about how the property is mapped to the kernel API, our local handwaving about what the value means is less important (and folks interested in the details know what to look for in the kernel docs/code).

Mashimiao · 2017-05-16T07:36:02Z

Maybe value description is not so important in spec, but we should not give descriptions that seems not correct.

wking · 2017-05-16T18:33:49Z

On Tue, May 16, 2017 at 12:36:03AM -0700, Ma Shimiao wrote: Maybe value description is not so important in spec, but we should not give descriptions that seems not correct.

Agreed. But if you require the runtime to write the `blkioWeightDevice` value to the `blkio.weight_device` file and link the kernel docs, you don't have to explain what the property means. Readers have enough information to find the meaning in the kernel docs without us having to echo those kernel docs here.

crosbymichael · 2017-06-01T18:40:47Z

@Mashimiao can you please rebase this?

Mashimiao · 2017-06-02T01:42:49Z

rebase, PTAL @opencontainers/runtime-spec-maintainers

wking · 2017-06-02T05:02:09Z

config-linux.md

+    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device. More info in [mknod(1)][mknod.1] man page.
+    * **`rate`** *(uint64, REQUIRED)* - bandwidth rate limit in bytes per second for the device
+
+* **`blkioThrottleReadIOPSDevice`**, **`blkioThrottleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be IO rate limited.
    The following parameters can be specified per-device:
    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device. More info in [mknod(1)][mknod.1] man page.
    * **`rate`** *(uint64, REQUIRED)* - IO rate limit for the device


If we are not going to require these to be implemented by blkio.throttle.read_iops_device and blkio.throttle.write_iops_device, I suggest we at least inline the kernel's docs for those settings:

Specifies upper limit on read (or write, for blkioThrottleWriteIOPSDevice) rate to the device. IO rate is specified in io per second. Rules are per device.

Although I'm not sure how much we can legally copy from the GPLv2 kernel docs into this Apache-2 project. I'd much rather require these to have the same semantics as blkio.throttle.read_iops_device and blkio.throttle.write_iops_device, link to the kernel docs, and leave it at that.

wking · 2017-06-21T19:05:34Z

config-linux.md


    You MUST specify at least one of `weight` or `leafWeight` in a given entry, and MAY specify both.

-* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`**, **`throttleReadIOPSDevice`**, **`throttleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be IO rate limited.
+* **`blkioThrottleReadBpsDevice`**, **`blkioThrottleWriteBpsDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be bandwidth rate limited.


This is re-introducing the blkio prefix which we removed in #860.

Mashimiao · 2017-06-22T01:32:09Z

rebased. PTAL

dqminh · 2017-07-03T23:32:15Z

LGTM

tianon · 2017-07-05T15:11:07Z

config-linux.md

@@ -330,14 +330,19 @@ The following parameters can be specified to set up the controller:

 * **`weight`** *(uint16, OPTIONAL)* - specifies per-cgroup weight. This is default weight of the group on all devices until and unless overridden by per-device rules.
 * **`leafWeight`** *(uint16, OPTIONAL)* - equivalents of `weight` for the purpose of deciding how much weight tasks in the given cgroup has while competing with the cgroup's child cgroups.
-* **`weightDevice`** *(array of objects, OPTIONAL)* - specifies the list of devices which will be bandwidth rate limited. The following parameters can be specified per-device:
+* **`weightDevice`** *(array of objects, OPTIONAL)* - specifies the list of devices which will be weight division of bandwidth. The following parameters can be specified per-device:


specifies the list of devices which will be weight division of bandwidth

This reads a bit odd to me -- perhaps "specifies the list of devices which will be bandwidth-limited by weight" ?

I don't understand why we're fixating on "weight division of bandwidth" here -- the relevant kernel doc (https://www.kernel.org/doc/Documentation/cgroup-v1/blkio-controller.txt) only uses that phrase once.

@tianon How about "specifies the list of devices which has its own specific weight and leafWeight"?

As you know, "weight" and "leafWeight" are used as bandwidth-limit in default. weightDevice is used to specify per-device rules.

How about "specifies the list of devices which has its own specific weight and leafWeight"?

The “and” portion of this isn't necessarily true, since you can have just one or the other as well. And this is less about limiting and more about weighting. Also, JSON uses “array” instead of ”list”. So how about:

weightDevice (array of objects, OPTIONAL) - an array of per-device bandwidth weights. Each entry has the following structure:

The each-entry line follows the current precedent:

$ git --no-pager grep -n 'entry.*following' v1.0.0-rc6 v1.0.0-rc6:config-linux.md:82:Each entry has the following structure: v1.0.0-rc6:config-linux.md:115:Each entry has the following structure: v1.0.0-rc6:config-linux.md:221:Each entry has the following structure: v1.0.0-rc6:config-linux.md:394:Each entry has the following structure: v1.0.0-rc6:config-linux.md:540: Each entry has the following structure: v1.0.0-rc6:config-linux.md:555: Each entry has the following structure: v1.0.0-rc6:config.md:165: Each entry has the following structure:

I like this one, updated

wking · 2017-07-11T06:18:09Z

config-linux.md

@@ -337,14 +337,19 @@ The following parameters can be specified to set up the controller:

 * **`weight`** *(uint16, OPTIONAL)* - specifies per-cgroup weight. This is default weight of the group on all devices until and unless overridden by per-device rules.
 * **`leafWeight`** *(uint16, OPTIONAL)* - equivalents of `weight` for the purpose of deciding how much weight tasks in the given cgroup has while competing with the cgroup's child cgroups.
-* **`weightDevice`** *(array of objects, OPTIONAL)* - specifies the list of devices which will be bandwidth rate limited. The following parameters can be specified per-device:
+* **`weightDevice`** *(array of objects, OPTIONAL)* - an array of per-device bandwidth weights. Each entry has the following structure:


The “Each entry…” sentence should be on it's own line like it is here.

wking · 2017-07-11T06:18:28Z

config-linux.md

    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device. More info in [mknod(1)][mknod.1] man page.
-    * **`weight`** *(uint16, OPTIONAL)* - bandwidth rate for the device.
-    * **`leafWeight`** *(uint16, OPTIONAL)* - bandwidth rate for the device while competing with the cgroup's child cgroups, CFQ scheduler only
+    * **`weight`** *(uint16, OPTIONAL)* - bandwidth wight for the device.


“wight” → “weight”

wking · 2017-07-11T06:22:16Z

config-linux.md

-* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`**, **`throttleReadIOPSDevice`**, **`throttleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be IO rate limited.
+* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be bandwidth rate limited.
+    The following parameters can be specified per-device:
+    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device. More info in [mknod(1)][mknod.1] man page.


The “More info” sentence should be on it's own line.

I'd also like to see it rephrased to be a more complete sentence. Following this pattern, it would be:

For more information, see the [`mknod(1)`][mknod.1] man page.

wking · 2017-07-11T06:26:09Z

config-linux.md

+    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device. More info in [mknod(1)][mknod.1] man page.
+    * **`rate`** *(uint64, REQUIRED)* - bandwidth rate limit in bytes per second for the device
+
+* **`throttleReadIOPSDevice`**, **`throttleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be IO rate limited.


For consistency (at least within lines touched by this PR), I'd rather follow weightDevice and use something like:

throttleReadIOPSDevice, throttleWriteIOPSDevice (array of objects, OPTIONAL) - an array of per-device IO rate limits. Each entry has the following structure:

With “Each entry has the following structure”, you could drop the following line (“The following parameters can be specified per-device”).

wking · 2017-07-11T06:27:19Z

config-linux.md


    You MUST specify at least one of `weight` or `leafWeight` in a given entry, and MAY specify both.

-* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`**, **`throttleReadIOPSDevice`**, **`throttleWriteIOPSDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be IO rate limited.
+* **`throttleReadBpsDevice`**, **`throttleWriteBpsDevice`** *(array of objects, OPTIONAL)* - specify the list of devices which will be bandwidth rate limited.
+    The following parameters can be specified per-device:


For consistency (at least within lines touched by this PR), I'd rather follow weightDevice and use something like:

throttleReadBpsDevice, throttleWriteBpsDevice (array of objects, OPTIONAL) - an array of per-device bandwidth rate limits. Each entry has the following structure:

weightDevice is not direct for bindwidth rate limit, actually used for weight division. bpsdeivce limits are different from IOpsdevice, they are limit rate in bytes, say used for bandwidth limit will be better. Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>

Mashimiao · 2017-07-11T08:42:55Z

updated, PTAL

wking

b27348d looks good to me. There's the backtick nit and my preference for clearly punting to kernel APIs, but neither of those seem like blockers for this PR.

wking · 2017-07-11T09:37:20Z

config-linux.md

+* **`weightDevice`** *(array of objects, OPTIONAL)* - an array of per-device bandwidth weights.
+    Each entry has the following structure:
+    * **`major, minor`** *(int64, REQUIRED)* - major, minor numbers for device.
+        For more information, see the [mknod(1)][mknod.1] man page.


I prefer backticks around man page references (e.g. here and here), but that's clearly not a big deal.

TomSweeneyRedHat · 2017-07-11T11:32:11Z

LGTM

Mashimiao · 2017-08-17T05:57:03Z

ping @opencontainers/runtime-spec-maintainers

crosbymichael · 2017-08-17T13:44:24Z

LGTM

Mashimiao · 2017-08-22T09:40:58Z

ping @opencontainers/runtime-spec-maintainers

hqhq · 2017-08-22T11:23:44Z

LGTM

Mashimiao force-pushed the fix-blkio-spec branch from cb65723 to d8e17dd Compare May 16, 2017 07:40

Mashimiao force-pushed the fix-blkio-spec branch from d8e17dd to 76b50f3 Compare June 2, 2017 01:42

wking reviewed Jun 2, 2017

View reviewed changes

wking reviewed Jun 21, 2017

View reviewed changes

Mashimiao force-pushed the fix-blkio-spec branch 2 times, most recently from 6f4146b to f63941a Compare June 22, 2017 01:30

tianon reviewed Jul 5, 2017

View reviewed changes

Mashimiao force-pushed the fix-blkio-spec branch from f63941a to c447eb3 Compare July 11, 2017 05:24

wking reviewed Jul 11, 2017

View reviewed changes

Mashimiao force-pushed the fix-blkio-spec branch from c447eb3 to b27348d Compare July 11, 2017 08:42

wking approved these changes Jul 11, 2017

View reviewed changes

hqhq merged commit 3294695 into opencontainers:master Aug 22, 2017

vbatts mentioned this pull request Sep 8, 2017

Release 1.0.1 #919

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config-linux.md: part fix of blkio spec #825

config-linux.md: part fix of blkio spec #825

Mashimiao commented May 14, 2017

wking commented May 15, 2017 via email

Mashimiao commented May 16, 2017

wking commented May 16, 2017 via email

crosbymichael commented Jun 1, 2017

Mashimiao commented Jun 2, 2017

wking Jun 2, 2017

wking Jun 21, 2017

Mashimiao commented Jun 22, 2017

dqminh commented Jul 3, 2017 •

edited by caniszczyk

Loading

tianon Jul 5, 2017

Mashimiao Jul 6, 2017

Mashimiao Jul 6, 2017

wking Jul 10, 2017

Mashimiao Jul 11, 2017

wking Jul 11, 2017

wking Jul 11, 2017

wking Jul 11, 2017

wking Jul 11, 2017

wking Jul 11, 2017

Mashimiao commented Jul 11, 2017

wking left a comment

wking Jul 11, 2017

TomSweeneyRedHat commented Jul 11, 2017

Mashimiao commented Aug 17, 2017

crosbymichael commented Aug 17, 2017 •

edited by caniszczyk

Loading

Mashimiao commented Aug 22, 2017

hqhq commented Aug 22, 2017 •

edited by caniszczyk

Loading

config-linux.md: part fix of blkio spec #825

config-linux.md: part fix of blkio spec #825

Conversation

Mashimiao commented May 14, 2017

wking commented May 15, 2017 via email

Mashimiao commented May 16, 2017

wking commented May 16, 2017 via email

crosbymichael commented Jun 1, 2017

Mashimiao commented Jun 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Jun 22, 2017

dqminh commented Jul 3, 2017 • edited by caniszczyk Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mashimiao commented Jul 11, 2017

wking left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TomSweeneyRedHat commented Jul 11, 2017

Mashimiao commented Aug 17, 2017

crosbymichael commented Aug 17, 2017 • edited by caniszczyk Loading

Mashimiao commented Aug 22, 2017

hqhq commented Aug 22, 2017 • edited by caniszczyk Loading

dqminh commented Jul 3, 2017 •

edited by caniszczyk

Loading

crosbymichael commented Aug 17, 2017 •

edited by caniszczyk

Loading

hqhq commented Aug 22, 2017 •

edited by caniszczyk

Loading