Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for blkio read/write bps device #14466

Merged
merged 1 commit into from Dec 4, 2015
Merged

Add support for blkio read/write bps device #14466

merged 1 commit into from Dec 4, 2015

Conversation

Mashimiao
Copy link
Contributor

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

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 8 times, most recently from e5f9b19 to e883cdf Compare July 9, 2015 02:52
@LK4D4
Copy link
Contributor

LK4D4 commented Jul 9, 2015

@Mashimiao don't know why, but this continue to fail on jenkins. Could you try to rebase against master pls?

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 2 times, most recently from 8bacfda to 3cc1693 Compare July 12, 2015 15:05
@Mashimiao
Copy link
Contributor Author

@LK4D4, in daemon file I have imported syscall. But, why when making windows binary it always says
daemon\daemon.go:937: undefined: syscall.Stat_t daemon\daemon.go:941: undefined: syscall.Stat daemon\daemon.go:949: undefined: syscall.Stat

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 2 times, most recently from 0ed2fdd to 4fa457e Compare July 20, 2015 06:08
@tiborvass
Copy link
Contributor

For reference, same problem as in #14006 (comment) we need to have a policy on what to do with these cgroups flags.

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 8 times, most recently from 0f8e734 to 9dca02a Compare August 4, 2015 09:47
tiborvass
tiborvass reviewed Aug 6, 2015
View reviewed changes
contrib/completion/bash/docker
@@ -1027,6 +1027,7 @@ _docker_run() {
--net
--pid
--publish -p
--read-bps-device
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Mashimiao could this be device-bps-read for lexicographical order?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @tiborvass --read-bps-device comes from cgroup file blkio.throttle.read_bps_device. I think it's better to keep the same format with cgroup. If familiar with cgroup, it's easier for people to understand what they are going to set. But if you insisted, I think I'm OK to change it.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @tiborvass here.
Most of the cgroup names are pretty sane, but this one is a bit backwards to me. I'd prefer to provide a better name, especially since this will be shared by other platforms.
This is just a nit...

We could probably even go a bit further and say device-read-rate and have it accept a user-friendly value (e.g. 50mbps or something instead of 52428800).

@tiborvass
Copy link
Contributor

For the same reasons described in #14006 (comment) I think we should accept these flags.

@cpuguy83
Copy link
Member

cpuguy83 commented Aug 6, 2015

@tiborvass Should we go ahead and move to code-review?

@tiborvass
Copy link
Contributor

@cpuguy83 was thinking we could wait for OP to answer to see if he disagrees.

@moxiegirl
Copy link
Contributor

@jfrazelle I don't see a test in here either...you mentioned having one earlier. You OK without it?

@jessfraz
Copy link
Contributor

jessfraz commented Dec 2, 2015

would be niceee

On Tue, Dec 1, 2015 at 8:44 AM, moxiegirl notifications@github.com wrote:

@jfrazelle https://github.com/jfrazelle I don't see a test in here
either...you mentioned having one earlier. You OK without it?

—
Reply to this email directly or view it on GitHub
#14466 (comment).

@jessfraz
Copy link
Contributor

jessfraz commented Dec 2, 2015

I actually really think we need one

On Tue, Dec 1, 2015 at 4:41 PM, Jessie Frazelle jess@docker.com wrote:

would be niceee

On Tue, Dec 1, 2015 at 8:44 AM, moxiegirl notifications@github.com
wrote:

@jfrazelle https://github.com/jfrazelle I don't see a test in here
either...you mentioned having one earlier. You OK without it?

—
Reply to this email directly or view it on GitHub
#14466 (comment).

@icecrime
Copy link
Contributor

icecrime commented Dec 2, 2015

@jfrazelle Can somebody take it in a followup PR? This has been opened for quite some months already :-(

@jessfraz
Copy link
Contributor

jessfraz commented Dec 2, 2015

Ya as long as we make an issue so we don't forget I can make a test

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 3 times, most recently from 0eaa5ae to bfd2d73 Compare December 3, 2015 08:02
@Mashimiao
Copy link
Contributor Author

rebased and fixed.

@runcom
Copy link
Member

runcom commented Dec 3, 2015

is this good to merge (given the lack of tests which will be addressed in another PR, I can take care of it also)? ping @jfrazelle

thaJeztah
thaJeztah reviewed Dec 3, 2015
View reviewed changes
contrib/completion/zsh/_docker
@@ -462,6 +462,8 @@ __docker_subcommand() {
"($help)*--cap-drop=[Drop Linux capabilities]:capability: "
"($help)--cidfile=[Write the container ID to the file]:CID file:_files"
"($help)*--device=[Add a host device to the container]:device:_files"
"($help)--device-read-bps=[Limit the read rate (bytes per second) from a device]:device:IO rate: "
"($help)--device-write-bps=[Limit the write rate (bytes per second) to a device]:device:IO rate: "
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just saw @sdurrheimer comment in your other PR (yay, I'm learning!) these should probably have a a * in front of --, so "($help)*--device...

@thaJeztah
Copy link
Member

Thanks @Mashimiao, I left some comments, but we're almost there!

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch 2 times, most recently from c6639b4 to b2ca41f Compare December 4, 2015 01:17
Add support for blkio read/write bps device
3f15a05 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_bps branch from b2ca41f to 3f15a05 Compare December 4, 2015 01:28
@thaJeztah
Copy link
Member

Thanks @Mashimiao LGTM

@runcom will work on a test; I saw some tiny issues with the docs, but I'll create a follow up PR to fix those. Thank you for contributing!

thaJeztah added a commit that referenced this pull request Dec 4, 2015
@thaJeztah
Merge pull request #14466 from Mashimiao/add-support-blkio_throtte_bps
cb6a1a6 
Add support for blkio read/write bps device
@thaJeztah thaJeztah merged commit cb6a1a6 into moby:master Dec 4, 2015
@thaJeztah thaJeztah mentioned this pull request Dec 16, 2015
Merged
@thaJeztah thaJeztah added this to the 1.10 milestone Dec 18, 2015
@moylop260
Copy link

Is #20077 a expected behaivor?

@wurstbrot
Copy link

Is it possible to use the parameter device-read-bps/device-read-bps for docker run in combination with docker-compose some how?

@yunzhu-li yunzhu-li mentioned this pull request May 24, 2016
Merged
@thaJeztah thaJeztah mentioned this pull request Oct 6, 2021
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@jessfraz jessfraz

Labels
impact/api impact/changelog impact/cli status/3-docs-review
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

None yet