blkiodev: limit blkio device's read/write Bps/IOps #2563

houstar · 2018-12-15T23:50:15Z

Signed-off-by: Leno Hou lenohou@gmail.com

Ⅰ. Describe what this PR did

blkiodev: limit blkio device's read/write Bps/IOps
testcase: add blkio device bps/iops testcase

Ⅱ. Does this pull request fix one issue?

fix #2509

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

To be added later

Ⅳ. Describe how to verify it

Create Container with:

pouch run -it --device /dev/sda:/dev/sda --device-read-bps=/dev/sda:100M busybox sh
Check inside Container's Cgroup filesystem
#cat /sys/fs/cgroup/blkio/blkio.throttle.read_bps_device
8:0 104857600

Ⅴ. Special notes for reviews

codecov · 2018-12-15T23:50:17Z

Codecov Report

Merging #2563 into master will decrease coverage by 0.26%.
The diff coverage is 47.22%.

@@            Coverage Diff             @@
##           master    #2563      +/-   ##
==========================================
- Coverage   69.17%   68.91%   -0.27%     
==========================================
  Files         278      278              
  Lines       18623    18659      +36     
==========================================
- Hits        12883    12859      -24     
- Misses       4268     4310      +42     
- Partials     1472     1490      +18

Flag	Coverage Δ
#criv1alpha1test	`31.26% <0%> (-0.02%)`	⬇️
#criv1alpha2test	`35.42% <0%> (-0.07%)`	⬇️
#integrationtest	`40.53% <47.22%> (-0.14%)`	⬇️
#nodee2etest	`32.47% <0%> (-0.19%)`	⬇️
#unittest	`26.7% <0%> (-0.06%)`	⬇️

Impacted Files	Coverage Δ
ctrd/utils.go	`82.97% <44.82%> (-17.03%)`	⬇️
daemon/mgr/container.go	`58.29% <57.14%> (-0.71%)`	⬇️
cri/ocicni/cni_manager.go	`58.82% <0%> (-11.77%)`	⬇️
daemon/mgr/system.go	`67.93% <0%> (-5.35%)`	⬇️
ctrd/watch.go	`80.28% <0%> (-4.23%)`	⬇️
daemon/mgr/events.go	`96.29% <0%> (-3.71%)`	⬇️
ctrd/client.go	`66.92% <0%> (-2.31%)`	⬇️
ctrd/image.go	`76.75% <0%> (-2.2%)`	⬇️
ctrd/container.go	`58.01% <0%> (-0.8%)`	⬇️
daemon/mgr/container_utils.go	`83.33% <0%> (-0.6%)`	⬇️
... and 2 more

apis/types/auth_config.go

apis/swagger.yml

cli/update.go

allencloud · 2018-12-17T08:11:53Z

I am afraid that we must add integration test case for this pull request. @houstar 😄

houstar · 2018-12-17T08:59:31Z

I am afraid that we must add integration test case for this pull request. @houstar 😄

OK. I'll update the unit test later

allencloud · 2018-12-17T11:24:54Z

apis/types/resources.go

@@ -122,7 +122,7 @@ type Resources struct {
 // Total memory limit (memory + swap). Set as `-1` to enable unlimited swap.
 MemorySwap int64 `json:"MemorySwap"`

- // Tune a container's memory swappiness behavior. Accepts an integer between 0 and 100.
+ // Tune a container's memory swappiness behavior. Accepts an integer between 0 and 100. -1 is also accepted, as a legacy alias of 0.


Good catch. I am afraid the added sentence is due to #2474.
While I have no idea why pr #2474 has not update this file? @zhuangqh

allencloud · 2018-12-17T11:29:02Z

Could you add some TestUpdateSuite test case in file test/cli_update_test.go? Thanks a lot. @houstar

houstar · 2018-12-17T12:19:12Z

Could you add some TestUpdateSuite test case in file test/cli_update_test.go? Thanks a lot. @houstar

@allencloud OK. Will be update at tomorrow

houstar · 2018-12-18T05:23:48Z

@allencloud

I'm afraid to mis-understand that the pouch update and pouch run has same code path.
so I removed the testcase I've wrote inside test/cli_run_blkio_test.go
This patch will update the container metadata and send these contents to RUNC runtime but runc
don't support update blkio write/read bps/iops. So the updated Container configuration will not make sense immediately as same as RestartPolicy now.
https://github.com/opencontainers/runc/blob/master/update.go#L251-L265:

		// Update the value
		config.Cgroups.Resources.BlkioWeight = *r.BlockIO.Weight
		config.Cgroups.Resources.CpuPeriod = *r.CPU.Period
		config.Cgroups.Resources.CpuQuota = *r.CPU.Quota
		config.Cgroups.Resources.CpuShares = *r.CPU.Shares
		config.Cgroups.Resources.CpuRtPeriod = *r.CPU.RealtimePeriod
		config.Cgroups.Resources.CpuRtRuntime = *r.CPU.RealtimeRuntime
		config.Cgroups.Resources.CpusetCpus = r.CPU.Cpus
		config.Cgroups.Resources.CpusetMems = r.CPU.Mems
		config.Cgroups.Resources.KernelMemory = *r.Memory.Kernel
		config.Cgroups.Resources.KernelMemoryTCP = *r.Memory.KernelTCP
		config.Cgroups.Resources.Memory = *r.Memory.Limit
		config.Cgroups.Resources.MemoryReservation = *r.Memory.Reservation
		config.Cgroups.Resources.MemorySwap = *r.Memory.Swap
		config.Cgroups.Resources.PidsLimit = r.Pids.Limit

zjumoon01 · 2018-12-18T09:16:33Z

2. This patch will update the container metadata and send these contents to RUNC runtime but runc
don't support update blkio write/read bps/iops. So the updated Container configuration will not make sense immediately as same as RestartPolicy now.

Yes, runc should update its resource.BlockIO. I'm coding in runc now and will push it in few days.

pouchrobot · 2019-02-18T02:08:10Z

ping @houstar
Conflict happens after merging a previous commit.
Please rebase the branch against master and push it back again. Thanks a lot.

houstar · 2019-02-19T02:34:44Z

Close this PR since this was resolved.

pouchrobot added the size/XL label Dec 15, 2018

houstar force-pushed the blkio-quota branch 3 times, most recently from 2c30f10 to 3ee9e7d Compare December 16, 2018 02:18

fengzixu suggested changes Dec 17, 2018

View reviewed changes

apis/types/auth_config.go Outdated Show resolved Hide resolved

allencloud requested changes Dec 17, 2018

View reviewed changes

apis/swagger.yml Outdated Show resolved Hide resolved

cli/update.go Outdated Show resolved Hide resolved

cli/update.go Outdated Show resolved Hide resolved

cli/update.go Outdated Show resolved Hide resolved

cli/update.go Outdated Show resolved Hide resolved

houstar force-pushed the blkio-quota branch from 3ee9e7d to ddee409 Compare December 17, 2018 10:09

pouchrobot added size/L and removed size/XL labels Dec 17, 2018

houstar force-pushed the blkio-quota branch from ddee409 to d5a8944 Compare December 17, 2018 10:20

pouchrobot added size/XL and removed size/L labels Dec 17, 2018

houstar force-pushed the blkio-quota branch 3 times, most recently from 972a144 to 1304f10 Compare December 17, 2018 10:41

pouchrobot added size/L and removed size/XL labels Dec 17, 2018

houstar force-pushed the blkio-quota branch 2 times, most recently from 096436c to f61d2bc Compare December 17, 2018 11:00

allencloud reviewed Dec 17, 2018

View reviewed changes

houstar force-pushed the blkio-quota branch from f61d2bc to f7c1b01 Compare December 18, 2018 05:15

pouchrobot added size/XL and removed size/L labels Dec 18, 2018

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Jan 28, 2019

weekly-digest bot mentioned this pull request Feb 3, 2019

Weekly Digest (27 January, 2019 - 3 February, 2019) #2705

Closed

zjumoon01 mentioned this pull request Feb 11, 2019

feature: update blkio device's read/write Bps/IOps #2708

Merged

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Feb 14, 2019

weekly-digest bot mentioned this pull request Feb 17, 2019

Weekly Digest (10 February, 2019 - 17 February, 2019) #2714

Closed

pouchrobot added conflict/needs-rebase and removed conflict/needs-rebase labels Feb 18, 2019

houstar closed this Feb 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

blkiodev: limit blkio device's read/write Bps/IOps #2563

blkiodev: limit blkio device's read/write Bps/IOps #2563

houstar commented Dec 15, 2018 •

edited

Loading

codecov bot commented Dec 15, 2018 •

edited

Loading

allencloud commented Dec 17, 2018

houstar commented Dec 17, 2018

allencloud Dec 17, 2018

zhuangqh Dec 18, 2018

allencloud commented Dec 17, 2018

houstar commented Dec 17, 2018

houstar commented Dec 18, 2018

zjumoon01 commented Dec 18, 2018

pouchrobot commented Feb 18, 2019

houstar commented Feb 19, 2019

blkiodev: limit blkio device's read/write Bps/IOps #2563

blkiodev: limit blkio device's read/write Bps/IOps #2563

Conversation

houstar commented Dec 15, 2018 • edited Loading

Ⅰ. Describe what this PR did

Ⅱ. Does this pull request fix one issue?

Ⅲ. Why don't you add test cases (unit test/integration test)? (你真的觉得不需要加测试吗？)

Ⅳ. Describe how to verify it

Ⅴ. Special notes for reviews

codecov bot commented Dec 15, 2018 • edited Loading

Codecov Report

allencloud commented Dec 17, 2018

houstar commented Dec 17, 2018

allencloud Dec 17, 2018

Choose a reason for hiding this comment

zhuangqh Dec 18, 2018

Choose a reason for hiding this comment

allencloud commented Dec 17, 2018

houstar commented Dec 17, 2018

houstar commented Dec 18, 2018

zjumoon01 commented Dec 18, 2018

pouchrobot commented Feb 18, 2019

houstar commented Feb 19, 2019

houstar commented Dec 15, 2018 •

edited

Loading

codecov bot commented Dec 15, 2018 •

edited

Loading