Update blkio GetStats#2407
Conversation
|
What do you think about changes introduced in this pull request? |
6090f95 to
b30e696
Compare
|
@AkihiroSuda Could you take a look at this PR? |
|
@crosbymichael, @mrunalp, @dqminh, @hqhq, @cyphar Could you take a look at this pull request? |
|
Overall, I think the ability to handle the correct blkio stats is good. It would be good to update the code in this PR to handle these various sources in a loop. Also the whole move next logic could be replaced by a typed error, which is the common way to handle this in Go. But I think you can get away from all of that error handling by just trying to read all the sources in a loop and just continuing on error. |
|
Is this close to what you had in mind @crosbymichael ? |
067d4fe to
d1c507e
Compare
|
lgtm |
|
@kolyshkin, @crosbymichael PTAL |
|
I ran into similar issue where This PR solves that issue by reading @piowag thanks for this PR. :) |
|
Just to add, I have verified the proposed fix of preferring to use diff --git a/vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs/blkio.go b/vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs/blkio.go
index 52c118d68c1..eb25d4af61b 100644
--- a/vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs/blkio.go
+++ b/vendor/github.com/opencontainers/runc/libcontainer/cgroups/fs/blkio.go
@@ -224,15 +224,42 @@ func getStats(path string, stats *cgroups.Stats) error {
var blkioStats []cgroups.BlkioStatEntry
var err error
- if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_service_bytes")); err != nil {
+ ioServiceBytesRecursiveFileName, err := getRecursiveFileName(filepath.Join(path, "blkio.throttle.io_service_bytes"))
+ if err != nil {
+ return err
+ }
+
+ if blkioStats, err = getBlkioStat(ioServiceBytesRecursiveFileName); err != nil {
return err
}
stats.BlkioStats.IoServiceBytesRecursive = blkioStats
- if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_serviced")); err != nil {
+ ioServicedRecursiveFileName, err := getRecursiveFileName(filepath.Join(path, "blkio.throttle.io_serviced"))
+ if err != nil {
+ return err
+ }
+
+ if blkioStats, err = getBlkioStat(ioServicedRecursiveFileName); err != nil {
return err
}
stats.BlkioStats.IoServicedRecursive = blkioStats
return nil
}
+
+func getRecursiveFileName(path string) (string, error) {
+ var cgroupFile string
+
+ recursiveFileName := filepath.Join(path, "_recursive")
+ if _, err := os.Stat(recursiveFileName); err == nil {
+ cgroupFile = recursiveFileName
+
+ } else if os.IsNotExist(err) {
+ cgroupFile = path
+
+ } else {
+ return "", err
+ }
+
+ return cgroupFile, nil
+}but I think this PR handles it more gracefully. |
d1c507e to
d614e67
Compare
libcontainer/cgroups/fs/blkio.go
Outdated
| if blkioStats, err := getBlkioStat(path, "blkio.io_serviced_recursive"); err == nil && blkioStats != nil { | ||
| return getCFQStats(path, stats) | ||
| // Try to read BFQ stats with debug files enabled first | ||
| if moveToNextMethod, err := getBFQDebugStats(path, stats); moveToNextMethod != true { |
There was a problem hiding this comment.
| if moveToNextMethod, err := getBFQDebugStats(path, stats); moveToNextMethod != true { | |
| if moveToNextMethod, err := getBFQDebugStats(path, stats); !moveToNextMethod { |
libcontainer/cgroups/fs/blkio.go
Outdated
| return err | ||
| } | ||
| // Try to read BFQ stats with debug files disabled | ||
| if moveToNextMethod, err := getBFQStats(path, stats); moveToNextMethod != true { |
libcontainer/cgroups/fs/blkio.go
Outdated
| if _, err = getBFQStats(path, stats); err != nil { | ||
| return moveToNextMethod, err | ||
| } | ||
|
|
||
| return moveToNextMethod, nil |
There was a problem hiding this comment.
| if _, err = getBFQStats(path, stats); err != nil { | |
| return moveToNextMethod, err | |
| } | |
| return moveToNextMethod, nil | |
| _, err = getBFQStats(path, stats) | |
| return moveToNextMethod, err |
|
@piowag PTAL |
|
@kolyshkin you gave comments to outdated code |
d614e67 to
af89b9c
Compare
Ok, now I understand. Nits fixed. |
|
@kolyshkin LGTY? |
Signed-off-by: Piotr Wagner <piotr.wagner@intel.com>
Signed-off-by: Piotr Wagner <piotr.wagner@intel.com>
af89b9c to
31b0151
Compare
|
rebased to include latest CI |
|
This PR references issue #2365 which is still open.Is the issue resolved as this PR has been merged? Thanks! |
Updates blkio GetStats, as proposed in 2365
Signed-off-by: Piotr Wagner piotr.wagner@intel.com