add more parameters of blkio subsystem to command line. #1004

hopkings2008 · 2016-08-27T09:32:31Z

hopkings2008 · 2016-08-27T09:43:33Z

Dear maintainers,
Please help to review this PR, and this PR is to add more blkio subsystem parameters according to oci spec.
And seems that the test cannot be started due to "java.io.IOException: remote file operation failed: /home/ubuntu/workspace/runc-PRs at hudson.remoting.Channel@3bdba927:ubuntu (aufs) (i-7ca51ce9): java.io.IOException: Remote call on ubuntu (aufs) (i-7ca51ce9) failed"

hopkings2008 · 2016-08-29T01:46:21Z

Dear maintainers,
I triggered the jenkins test again, this time, all the tests are passed. So, please help to review this PR, thanks a lot.

hqhq · 2016-08-29T12:44:41Z

Code looks good, can you add some test cases in tests/integration/cgroups.bats and tests/integration/update.bats?

mlaventure · 2016-08-29T15:29:42Z

update.go

+		},
+		cli.StringSliceFlag{
+			Name:  "blkio-weight-device",
+			Usage: "Weight per cgroup per device, can override BlkioWeight.",


Can you add the expected input format in there (and the one below)?

hopkings2008 · 2016-08-30T08:49:26Z

@mlaventure , thanks for your suggestion, i have added the input style example according to your suggestion.
@hqhq , thanks for your suggestion, i have added both unit tests and integration test for this PR. But the integration tests failed at '# process_linux.go:291: setting cgroup config for ready process caused "failed to write 8:0 100 to blkio.weight_device: write /sys/fs/cgroup/blkio/runc-update-integration-test/blkio.weight_device: invalid argument"'
After i checked this problem, it seems that it is related to the problem found at '#140'

hqhq · 2016-08-30T09:24:45Z

@hopkings2008 Then I think you can test it on your local machine make sure they all passed, and add check for io scheduler and skip the test case if it's not cfq.

mlaventure · 2016-08-30T18:25:29Z

@hopkings2008 I think in the flag description something more generic should be used, for instance:

<MAJOR>:<MINOR> <WEIGHT> [LEAF_WEIGHT] instead of 8:0 800 [200]

wdyt?

hopkings2008 · 2016-08-31T06:04:25Z

@mlaventure thanks for your good suggestion. I have amended the description of them, please check it again.
@hqhq i have added unit tests for this PR, and due to the lack of cfq device in CI machine, the tests cannot be added into integration currently. I will added them later.

hqhq · 2016-08-31T07:51:46Z

update.go

+		ctds = append(ctds, td)
+	}
+	return ctds
+}


You can use configs.NewWeightDevice and configs.NewThrottleDevice instead of these.

@hqhq Good suggestion, I will amend this. Thanks.

hopkings2008 · 2016-08-31T08:43:54Z

@hqhq i have amended the code according to your suggestion, please check it again. thanks.

hqhq · 2016-09-01T02:49:36Z

update.go

+func convertSpecWeightDevices(devices []specs.WeightDevice) []*configs.WeightDevice {
+	var cwds []*configs.WeightDevice
+	for _, d := range devices {
+		wd := configs.NewWeightDevice(d.Major, d.Minor, *d.Weight, *d.LeafWeight)


According to getBlkioWeightDeviceFromString(), d.LeafWeight could be null, you have to check null condition here.

@hqhq thanks for finding this problem. I have fixed it by checking the nil pointer value. By default, the leafweight will be 0 which does nothing. Please help to check this PR again.

hqhq · 2016-09-01T07:02:40Z

Sorry I should've mentioned before, man/runc-update.8.md and contrib/completions/bash/runc should also be altered.

hopkings2008 · 2016-09-01T08:14:15Z

@hqhq i missed those two related things, and thanks for your reminder. And i have added the missed changes. Please check it again. :)

hqhq · 2016-09-01T08:41:47Z

LGTM, thanks.

hopkings2008 · 2016-09-02T01:06:19Z

@mlaventure could you please help to check this PR again? and welcome your suggestions, thanks a lot.

hopkings2008 · 2016-09-02T09:20:55Z

Dear maintainers, Please help to check this PR and after this can be merged, i will submit the PR for related change of containerd.

mlaventure · 2016-09-02T14:38:57Z

man/runc-update.8.md

+   --blkio-weight value                        Specifies per cgroup weight, range is from 10 to 1000 (default: 0)
+   --blkio-leaf-weight value                   Specifies tasks' weight in the given cgroup while competing with the cgroup's child cgroups, range is from 10 to 1000, cfq scheduler only 
+   --blkio-weight-device value                 Weight per cgroup per device, can override blkio-weight. For example, "<MAJOR>:<MINOR> <WEIGHT> [LEAF_WEIGHT]"
+   --blkio-throttle-readbps-device value       IO read rate limit per cgroup per device, bytes per second. For example, "<MAJOR>:<MINOR> <RATE>"


nitpick: s/For example/Argument must be of the form/ or something similar

@mlaventure thanks very much for your good suggestion. I have amended that description and please check it again. thanks

hopkings2008 · 2016-09-06T02:18:21Z

@mlaventure could you please help to review this PR again? i have amended the help description message according to your suggestion. Thanks a lot.

hopkings2008 · 2016-09-07T02:53:54Z

Dear maintainers, Could you please check this PR so that i can send my PR for related changes for containerd, and welcome your suggestions. thanks in advance.

hopkings2008 · 2016-09-07T03:02:53Z

@crosbymichael , could you please help to review this PR? and weclome your suggestions. :)

hqhq · 2016-09-07T03:03:00Z

LGTM

hopkings2008 · 2016-09-20T04:57:34Z

@mlaventure , could you please take some time to check this pr again? it has taken some long time. :)

cyphar

See my comments.

cyphar · 2016-09-20T10:36:19Z

man/runc-update.8.md

+   --blkio-throttle-readbps-device value       IO read rate limit per cgroup per device, bytes per second. Argument must be of the form "<MAJOR>:<MINOR> <RATE>"
+   --blkio-throttle-writebps-device value      IO write rate limit per cgroup per divice, bytes per second. Argument must be of the form "<MAJOR>:<MINOR> <RATE>"
+   --blkio-throttle-readiops-device value      IO read rate limit per cgroup per device, IO per second. Argument must be of the form "<MAJOR>:<MINOR> <RATE>"
+   --blkio-throttle-writeiops-device value     IO write rate limit per cgroup per device, IO per second. Argument must be of the form "<MAJOR>:<MINOR> <RATE>"


You've added all of these flags ...

cyphar · 2016-09-20T10:36:49Z

tests/integration/update.bats


+    # update blkio-leaf-weight
+    runc update test_update --blkio-leaf-weight 500
+    check_cgroup_value $CGROUP_BLKIO "blkio.leaf_weight" 500


... but only added a test for leaf weight. Is this intentional (meaning, is it possible for us to test the other options as well here)?

@cyphar thanks for checking this PR. For other parameters of blkio, we cannot test them in current CI machine, because it is lack of cfq device in CI machine. What is your suggestion about the device limitation of CI machine?

I would add the tests as "optional" if possible (at SUSE we run the test suite on SUSE machines, so the tests are useful outside of the CI setup for this repo). But if we're not testing it in the CI then it's not as important (and could wait for a follow-up PR).

@cyphar , currently, the CI tests are wrote with shell framework. If i add the new flags, some tests are not passed due to the "device" problem. I have tried to add the new flags to CI tests by using 'conditional' judgement, but if do so, the code seems very ugly. For example, if i add new flags into 'DATA; section, i will double the 'old flags' twice. I think this is not acceptable. Can I add those new flags into tests later in another PR if the test framework become more flexible?
Welcome your suggestions.

Yeah, we can handle this later I guess. We need to do the test framework improvements for the memory cgroup anyway.

cyphar · 2016-09-20T10:37:50Z

update.go

 		},
+		cli.IntFlag{
+			Name:  "blkio-leaf-weight",
+			Usage: "Specifies tasks' weight in the given cgroup while competing with the cgroup's child cgroups, range is from 10 to 1000, cfq scheduler only",


IMO we should be specifying "ranges" as part of the help docs. Shouldn't these be all listed inside the runtime-spec?

EDIT: I meant to say "shouldn't"

@cyphar , thanks for your checking. I have amended this and uses 'ranges from' instead of "range is from". Please check it again. Thanks.

That's not what I meant. What I meant was that the actual range should not be in the help docs. The ranges are defined by the runtime-spec, not by us.

@cyphar thanks for your explanation, i have amended this pr and removed 'range' information from help messages. Please check it again. Thanks a lot. :)

cyphar · 2016-09-20T10:39:34Z

update.go

 func u16Ptr(i uint16) *uint16 { return &i }

+var regBlkioWeightDevice = regexp.MustCompile(`([0-9]+):([0-9]+) ([0-9]+)(?: ([0-9]+))?`)
+var regBlkioThrottleDevice = regexp.MustCompile(`([0-9]+):([0-9]+) ([0-9]+)`)


Is this format defined in the spec (my guess is yes), and if so please at least comment that it comes from the spec or something so it's a bit more clear. In addition, if it comes from the spec maybe all of these conversion functions should go inside libcontainer/specconv (@hqhq what do you think?).

Signed-off-by: Yu Zou <zouyu7@huawei.com>

AkihiroSuda · 2020-03-09T16:27:32Z

Needs rebase

kolyshkin · 2025-09-17T02:49:56Z

Per-device IOPS and weights are supported now, see PR #4775. We did not want to introduce more CLI flags, this can be handled via JSON.

GordonTheTurtle added the status/0-triage label Aug 27, 2016

hopkings2008 force-pushed the blkio branch from f7df340 to a642b4e Compare August 27, 2016 09:41

hopkings2008 force-pushed the blkio branch from a642b4e to 4a9d453 Compare August 29, 2016 00:57

hopkings2008 force-pushed the blkio branch from 4a9d453 to d163a0f Compare August 29, 2016 02:05

mlaventure reviewed Aug 29, 2016
View reviewed changes

hopkings2008 force-pushed the blkio branch 8 times, most recently from 29c8d04 to 4e1570d Compare August 30, 2016 08:40

hopkings2008 force-pushed the blkio branch 8 times, most recently from 1ab9cb1 to 5a004d9 Compare August 31, 2016 05:11

hqhq reviewed Aug 31, 2016
View reviewed changes

hopkings2008 force-pushed the blkio branch from 5a004d9 to faa8ec1 Compare August 31, 2016 08:40

hqhq reviewed Sep 1, 2016
View reviewed changes

hopkings2008 force-pushed the blkio branch from faa8ec1 to 0e3caf4 Compare September 1, 2016 06:14

hopkings2008 force-pushed the blkio branch from 0e3caf4 to 6f6735f Compare September 1, 2016 08:01

mlaventure reviewed Sep 2, 2016
View reviewed changes

hopkings2008 force-pushed the blkio branch from 6f6735f to a24d368 Compare September 5, 2016 01:02

cyphar requested changes Sep 20, 2016

View reviewed changes

hopkings2008 force-pushed the blkio branch 2 times, most recently from da28752 to 54d1412 Compare September 26, 2016 07:55

add more parameters of blkio subsystem to command line.

aa60df2

Signed-off-by: Yu Zou <zouyu7@huawei.com>

hopkings2008 force-pushed the blkio branch from 54d1412 to aa60df2 Compare September 26, 2016 08:01

kolyshkin closed this Sep 17, 2025

add more parameters of blkio subsystem to command line. #1004

add more parameters of blkio subsystem to command line. #1004

Uh oh!

Conversation

hopkings2008 commented Aug 27, 2016

Uh oh!

hopkings2008 commented Aug 27, 2016

Uh oh!

hopkings2008 commented Aug 29, 2016

Uh oh!

hqhq commented Aug 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hopkings2008 commented Aug 30, 2016

Uh oh!

hqhq commented Aug 30, 2016

Uh oh!

mlaventure commented Aug 30, 2016

Uh oh!

hopkings2008 commented Aug 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hopkings2008 commented Aug 31, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hqhq commented Sep 1, 2016

Uh oh!

hopkings2008 commented Sep 1, 2016

Uh oh!

hqhq commented Sep 1, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hopkings2008 commented Sep 2, 2016

Uh oh!

hopkings2008 commented Sep 2, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hopkings2008 commented Sep 6, 2016

Uh oh!

hopkings2008 commented Sep 7, 2016

Uh oh!

hopkings2008 commented Sep 7, 2016

Uh oh!

hqhq commented Sep 7, 2016 • edited by caniszczyk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hopkings2008 commented Sep 20, 2016

Uh oh!

cyphar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cyphar Sep 20, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

hqhq commented Sep 1, 2016 •

edited by caniszczyk

Loading

hqhq commented Sep 7, 2016 •

edited by caniszczyk

Loading

cyphar Sep 20, 2016 •

edited

Loading