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 iops device #15879

Merged
merged 1 commit into from Dec 21, 2015
Merged

Add support for blkio read/write iops device #15879

merged 1 commit into from Dec 21, 2015

Conversation

Mashimiao
Copy link
Contributor

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

@cpuguy83 cpuguy83 mentioned this pull request Aug 28, 2015
Closed
@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch 2 times, most recently from 8c2fad3 to c58fb34 Compare September 1, 2015 07:54
cpuguy83
cpuguy83 reviewed Sep 1, 2015
View reviewed changes
daemon/daemon_unix.go
@@ -38,6 +38,38 @@ const (
platformSupported = true
)

func getBlkioReadIOpsDevice(config *runconfig.HostConfig) ([]string, error) {
Copy link
Member

Choose a reason for hiding this comment

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

This and getBlkioWriteIOpsDevice look exactly the same. Can we instead pass a value instead of runconfig.HostConfig?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cpuguy83 fixed.

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch 5 times, most recently from 73a5588 to 7077586 Compare September 9, 2015 05:28
This was referenced Sep 16, 2015
Merged
Merged
@jessfraz jessfraz self-assigned this Oct 2, 2015
@runcom runcom mentioned this pull request Oct 6, 2015
Merged
@LK4D4
Copy link
Contributor

LK4D4 commented Oct 23, 2015

@Mashimiao need rebase
LGTM

@thaJeztah
Copy link
Member

does this depend on the other PR's? #14466 and #13959?

(looks like this needs a rebase 😢)

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 13, 2015

@Mashimiao Would you mind to answer @thaJeztah question?

@runcom
Copy link
Member

runcom commented Nov 13, 2015

I think this can go ahead now :)

@LK4D4
Copy link
Contributor

LK4D4 commented Nov 18, 2015

@Mashimiao whoa, sorry, need rebase again.

@Mashimiao
Copy link
Contributor Author

This PR is similar to #14466 and has some common codes, I want to rebase and fix this after #14466 merged

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch 5 times, most recently from 4a0988c to b65bcfb Compare December 7, 2015 07:07
@Mashimiao
Copy link
Contributor Author

@LK4D4 @runcom @thaJeztah PR rebased and fixed. I think it failed with unrelated case.

@thaJeztah
Copy link
Member

Thanks @Mashimiao, test failure will be fixed in #18458

@thaJeztah
Copy link
Member

#18458 was merged, so if you rebase, the tests should be green again

@LK4D4
Copy link
Contributor

LK4D4 commented Dec 15, 2015

ping @thaJeztah @moxiegirl

@calavera
Copy link
Contributor

Docs LGTM. Unfortunately, this PR will need to be rebased before merging.

@thaJeztah can you take a look?

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch 2 times, most recently from a015698 to 7cf4d13 Compare December 18, 2015 01:20
@Mashimiao
Copy link
Contributor Author

@calavera thanks, rebased.

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch from 7cf4d13 to 274dad5 Compare December 18, 2015 05:37
thaJeztah
thaJeztah reviewed Dec 18, 2015
View reviewed changes
docs/reference/run.md
@@ -999,6 +1001,21 @@ Both flags take limits in the `<device-path>:<limit>[unit]` format. Both read
and write rates must be a positive integer. You can specify the rate in `kb`
(kilobytes), `mb` (megabytes), or `gb` (gigabytes).

The `--device-read-iops` flag limits read rate (IO per second) from a device.
For example, this command creates a container and limits the read rate to
`1000` per second from `/dev/sda`:
Copy link
Member

Choose a reason for hiding this comment

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

nit: s/1000 per second/1000 IO per second/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@thaJeztah thaJeztah added this to the 1.10 milestone Dec 18, 2015
@thaJeztah
Copy link
Member

Thanks @Mashimiao two nits. Also can you add the new options to the API changelog, for example:

`POST /containers/create` allows you to set a read/write rate
limit for a device (in bytes per second or IO per second).

(We forgot to add "BlkioThrottleReadBpsDevice" and "BlkioThrottleWriteBpsDevice" to the change log in #14466)

@Mashimiao Mashimiao force-pushed the add-support-blkio_throtte_iops branch from 274dad5 to 843084b Compare December 21, 2015 01:14
Add support for blkio read/write iops device
843084b 
Signed-off-by: Ma Shimiao <mashimiao.fnst@cn.fujitsu.com>
@Mashimiao
Copy link
Contributor Author

@thaJeztah added to api Changes

@Mashimiao
Copy link
Contributor Author

jenkins failed with unrelated case

@thaJeztah
Copy link
Member

Thanks @Mashimiao, happy to see this ready to merge!!

LGTM

Documentation is basically the same as for blkioBps, so it should be good to go. Moving to "merge"

I restarted janky and experimental, but the failing tests are not related, probably a time issue;

--- FAIL: TestFormat (0.00s)
    formatter_test.go:152: Expected 
        CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
        containerID1        ubuntu              ""                  45 years ago                                                foobar_baz
        containerID2        ubuntu              ""                  45 years ago                                                foobar_bar
        , got 
        CONTAINER ID        IMAGE               COMMAND             CREATED             STATUS              PORTS               NAMES
        containerID1        ubuntu              ""                  46 years ago                                                foobar_baz
        containerID2        ubuntu              ""                  46 years ago                                                foobar_bar
FAIL

@runcom
Copy link
Member

runcom commented Dec 21, 2015

seems like failure comes from #18820

@thaJeztah
Copy link
Member

@runcom yes I just found that one, to check if an issue was already created for it.

@thaJeztah
Copy link
Member

@Mashimiao can you rebase? Tests should be fixed after that (by #18820)

@thaJeztah
Copy link
Member

It's green! \o/ :tada:

thaJeztah added a commit that referenced this pull request Dec 21, 2015
@thaJeztah
Merge pull request #15879 from Mashimiao/add-support-blkio_throtte_iops
312c826 
Add support for blkio read/write iops device
@thaJeztah thaJeztah merged commit 312c826 into moby:master Dec 21, 2015
@yunzhu-li yunzhu-li mentioned this pull request May 24, 2016
Merged
@AkihiroSuda AkihiroSuda mentioned this pull request Mar 6, 2017
Closed
@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/4-merge
Projects
None yet
Milestone
1.10.0
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@Mashimiao @LK4D4 @thaJeztah @runcom @jessfraz @calavera @cpuguy83 @GordonTheTurtle