bugfix: fetch the right device number which great than 255 by yyb196 · Pull Request #39212 · moby/moby

yyb196 · 2019-05-14T07:40:56Z

Signed-off-by: frankyang [email protected]

- What I did

fix a bug which cause an error like :
write 4349:48 0 to blkio.throttle.read_bps_device: write /sys/fs/cgroup/blkio/xxx/cid/blkio.throttle.read_bps_device: no such device

the container has config "BlkioDeviceReadBps":[{"Path":"/dev/vdt","Rate":0}]

and I foud stat /dev/vdt and ls -l /dev/vdt has different out of device number, which is correct:


#ls -l /dev/vdt
brw-rw---- 1 root disk 253, 304 May 9 17:03 /dev/vdt

#stat /dev/vdt | grep type
Device: 6h/6d Inode: 56051004	Links: 1	Device type: fd, 130

Someone in docker forum ran into the same problem: https://forums.docker.com/t/docker-run-device-read-bps-option-not-working/68044 in his case is the major number greater than 255.

- How I did it
I correct the way in which we get major and minor device number according to the method new_encode_dev in linux kernel.

- How to verify it
I add two unit test cases.

- Description for the changelog

- A picture of a cute animal (not mandatory but encouraged)

olljanat · 2019-05-14T20:12:29Z

(reserved for my derek commands)

olljanat · 2019-05-14T20:17:55Z

@yyb196 congrats of your first PR to moby 🎂

I'm not familiar this this feature so just to verify that did you test that those unit tests would fail without this fix?

thaJeztah · 2019-05-15T00:43:17Z

ping @kolyshkin PTAL

kolyshkin · 2019-05-15T03:12:31Z

Looks fine, but since x/sys/unix already implemented these, it'll be better to use those. In other words:

import "golang.org/x/sys/unix"
...
d.Major = unix.Major(stat.Rdev)
d.Minor = unix.Minor(stat.Rdev)

@yyb196 can you please update this PR?

codecov · 2019-05-15T08:04:09Z

Codecov Report

❗ No coverage uploaded for pull request base (master@34b5672). Click here to learn what that means.
The diff coverage is 100%.

@@            Coverage Diff            @@
##             master   #39212   +/-   ##
=========================================
  Coverage          ?   37.06%           
=========================================
  Files             ?      612           
  Lines             ?    45494           
  Branches          ?        0           
=========================================
  Hits              ?    16862           
  Misses            ?    26344           
  Partials          ?     2288

yyb196 · 2019-05-15T08:13:51Z

@kolyshkin @olljanat I have change the code and test case according to your advices.

PTALA, thanks a lot.

Using the unix package's methods Major and Minor.

And now the unit test case that I add will fail without this fix.

Because I don't know how to mock system call in go, my test case depends on that we can do unix.Mknode in unit test, then can I mock a temporary device whose major and minor device type great than 255.

kolyshkin · 2019-05-15T18:03:27Z

mknod requires root, so this test case should have something like

if os.Getuid() != 0 { t.Skip("root required") // for mknod }

at the very beginning (so if someone runs go test as non-root, there won't be a fake failure).

Also, make sense to use

t.Parallel()

kolyshkin · 2019-05-15T18:07:48Z

s/mknode/mknod/

kolyshkin · 2019-05-15T18:09:43Z

You can use unix.Mkdev(major, minor) (and define major and minor as constants to not have some "magic numbers" across the code.

kolyshkin · 2019-05-15T18:12:43Z

assert.Check(t, is.Len(weightDevs, 1), "getBlkioWeightDevices")

(you can use assert in other places in your code as well, it makes the code more compact and perhaps more readable)

kolyshkin · 2019-05-15T18:13:56Z

s/"tempDevDirForBlkioThrottleDevices"/t.Name()/

or, if you like it more, "tempDevDir"+t.Name()

kolyshkin · 2019-05-15T18:22:53Z

This second test is the same as the first one, except for function called. Can you please combine the code so it won't repeat twice?

kolyshkin · 2019-05-15T18:23:36Z

@yyb196 thanks so much for the update! I have left some comments

Signed-off-by: frankyang <[email protected]>

yyb196 · 2019-05-16T07:38:43Z

@kolyshkin thanks for your comments, I have updated. PTALA.

kolyshkin

LGTM, thanks for good work!

thaJeztah

LGTM, thanks!

GordonTheTurtle added the status/0-triage label May 14, 2019

derek Bot added the rebuild/z label May 14, 2019

GordonTheTurtle removed the rebuild/z label May 14, 2019

derek Bot added the rebuild/powerpc label May 14, 2019

GordonTheTurtle removed the rebuild/powerpc label May 14, 2019

yyb196 force-pushed the devno branch from d2c8f30 to 5737c06 Compare May 15, 2019 08:04

kolyshkin reviewed May 15, 2019

View reviewed changes

Comment thread daemon/daemon_unix_test.go Outdated

kolyshkin May 15, 2019

Copy link
Copy Markdown

Contributor

Choose a reason for hiding this comment

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

s/mknode/mknod/

kolyshkin reviewed May 15, 2019

View reviewed changes

bugfix: fetch the right device number which great than 255

b9f3191

Signed-off-by: frankyang <[email protected]>

yyb196 force-pushed the devno branch from 5737c06 to b9f3191 Compare May 16, 2019 07:33

kolyshkin approved these changes May 16, 2019

View reviewed changes

thaJeztah added status/2-code-review and removed status/0-triage labels May 20, 2019

thaJeztah approved these changes May 20, 2019

View reviewed changes

thaJeztah added process/cherry-pick rebuild/* status/4-merge and removed status/2-code-review labels May 20, 2019

GordonTheTurtle removed the rebuild/* label May 20, 2019

cpuguy83 merged commit 9808f60 into moby:master May 20, 2019

This was referenced May 21, 2019

[18.09 backport] bugfix: fetch the right device number which great than 255 docker-archive/engine#223

Merged

[19.03 backport] bugfix: fetch the right device number which great than 255 docker-archive/engine#224

Merged

thaJeztah added process/cherry-picked and removed process/cherry-pick labels May 25, 2019

thaJeztah mentioned this pull request Jun 13, 2019

Stat_t on MIPS uses 32-bit fields for Dev and Rdev, so we need to cast these to uint64. #37490

Closed

thaJeztah added this to the 20.03.0 milestone Apr 2, 2020

Uh oh!

Conversation

yyb196 commented May 14, 2019

Uh oh!

olljanat commented May 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

olljanat commented May 14, 2019

Uh oh!

thaJeztah commented May 15, 2019

Uh oh!

kolyshkin commented May 15, 2019

Uh oh!

codecov Bot commented May 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

yyb196 commented May 15, 2019

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin May 15, 2019 • 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!

kolyshkin May 15, 2019

Choose a reason for hiding this comment

Uh oh!

kolyshkin commented May 15, 2019

Uh oh!

yyb196 commented May 16, 2019

Uh oh!

kolyshkin left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

olljanat commented May 14, 2019 •

edited

Loading

codecov Bot commented May 15, 2019 •

edited

Loading

kolyshkin May 15, 2019 •

edited

Loading