Skip to content

Update blkio GetStats#2407

Merged
kolyshkin merged 2 commits intoopencontainers:masterfrom
piowag:2365-blkio-getstat-update
Dec 3, 2020
Merged

Update blkio GetStats#2407
kolyshkin merged 2 commits intoopencontainers:masterfrom
piowag:2365-blkio-getstat-update

Conversation

@piowag
Copy link
Copy Markdown
Contributor

@piowag piowag commented May 15, 2020

Updates blkio GetStats, as proposed in 2365

Signed-off-by: Piotr Wagner piotr.wagner@intel.com

@piowag
Copy link
Copy Markdown
Contributor Author

piowag commented May 26, 2020

What do you think about changes introduced in this pull request?

@piowag piowag force-pushed the 2365-blkio-getstat-update branch from 6090f95 to b30e696 Compare July 31, 2020 06:36
@piowag piowag requested a review from kolyshkin July 31, 2020 07:55
@piowag
Copy link
Copy Markdown
Contributor Author

piowag commented Aug 5, 2020

@AkihiroSuda Could you take a look at this PR?

@piowag
Copy link
Copy Markdown
Contributor Author

piowag commented Aug 19, 2020

@crosbymichael, @mrunalp, @dqminh, @hqhq, @cyphar Could you take a look at this pull request?

@crosbymichael
Copy link
Copy Markdown
Member

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.

@piowag
Copy link
Copy Markdown
Contributor Author

piowag commented Aug 24, 2020

Is this close to what you had in mind @crosbymichael ?

@piowag piowag force-pushed the 2365-blkio-getstat-update branch from 067d4fe to d1c507e Compare September 1, 2020 07:58
@Creatone
Copy link
Copy Markdown

Creatone commented Sep 3, 2020

lgtm

@piowag
Copy link
Copy Markdown
Contributor Author

piowag commented Sep 3, 2020

@kolyshkin, @crosbymichael PTAL

@harche
Copy link
Copy Markdown

harche commented Oct 21, 2020

I ran into similar issue where blkio.throttle.io_service_bytes_recursive and blkio.throttle.io_serviced_recursive contained accurate information compared to blkio.throttle.io_service_bytes and blkio.throttle.io_serviced respectively.

This PR solves that issue by reading blkio.throttle.io_service_bytes_recursive and blkio.throttle.io_serviced_recursive over blkio.throttle.io_service_bytes and blkio.throttle.io_serviced respectively.

@piowag thanks for this PR. :)

@harche
Copy link
Copy Markdown

harche commented Oct 21, 2020

Just to add, I have verified the proposed fix of preferring to use _recursive file by this rudimentary diff,

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.

@Creatone Creatone force-pushed the 2365-blkio-getstat-update branch from d1c507e to d614e67 Compare November 9, 2020 11:15
@Creatone
Copy link
Copy Markdown

Creatone commented Nov 9, 2020

@kolyshkin @AkihiroSuda

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 {
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin Nov 17, 2020

Choose a reason for hiding this comment

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

Suggested change
if moveToNextMethod, err := getBFQDebugStats(path, stats); moveToNextMethod != true {
if moveToNextMethod, err := getBFQDebugStats(path, stats); !moveToNextMethod {

return err
}
// Try to read BFQ stats with debug files disabled
if moveToNextMethod, err := getBFQStats(path, stats); moveToNextMethod != true {
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.

ditto

Comment on lines +234 to +238
if _, err = getBFQStats(path, stats); err != nil {
return moveToNextMethod, err
}

return moveToNextMethod, nil
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.

Suggested change
if _, err = getBFQStats(path, stats); err != nil {
return moveToNextMethod, err
}
return moveToNextMethod, nil
_, err = getBFQStats(path, stats)
return moveToNextMethod, err

Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

Left a few nits

@kolyshkin kolyshkin added this to the 1.0.0-rc93 milestone Nov 17, 2020
@kolyshkin
Copy link
Copy Markdown
Contributor

@piowag PTAL

@Creatone
Copy link
Copy Markdown

@kolyshkin you gave comments to outdated code

@Creatone Creatone force-pushed the 2365-blkio-getstat-update branch from d614e67 to af89b9c Compare November 23, 2020 10:43
@Creatone
Copy link
Copy Markdown

@kolyshkin you gave comments to outdated code

Ok, now I understand. Nits fixed.

@AkihiroSuda
Copy link
Copy Markdown
Member

@kolyshkin LGTY?

AkihiroSuda
AkihiroSuda previously approved these changes Nov 30, 2020
kolyshkin
kolyshkin previously approved these changes Dec 3, 2020
Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

Signed-off-by: Piotr Wagner <piotr.wagner@intel.com>
Signed-off-by: Piotr Wagner <piotr.wagner@intel.com>
@kolyshkin kolyshkin dismissed stale reviews from AkihiroSuda and themself via 31b0151 December 3, 2020 16:19
@kolyshkin kolyshkin force-pushed the 2365-blkio-getstat-update branch from af89b9c to 31b0151 Compare December 3, 2020 16:19
@kolyshkin
Copy link
Copy Markdown
Contributor

rebased to include latest CI

Copy link
Copy Markdown
Contributor

@kolyshkin kolyshkin left a comment

Choose a reason for hiding this comment

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

LGTM

@ahus1
Copy link
Copy Markdown

ahus1 commented Oct 7, 2022

This PR references issue #2365 which is still open.Is the issue resolved as this PR has been merged? Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants