return code changed to ErrNotExist #1190
Conversation
|
This PR is part of fix for #487 |
|
Rejected. This is not right, some files' absence is totally legal, such as different kernel configurations, for blkio, some files could only be seem on CFQ enabled kernels, so you can't just return error for not finding some files. Some subsystems don't have this issue, so we are having this inconsistency issue. This PR doesn't look good to me, but I don't deny there can be some improvements to soften this issue, so #486 could still be valid. You'll be welcome to keep digging on this, and thanks for your contribution :) . |
I Agree. The GetStats() function already take cares about CFQ enabled/disabled configuration. My fix simply checks, if CFQ enabled, then in case some files are missing, it reports |
Your PR will break CFQ enabled/disabled check. Fix that could make this PR reasonable. |
getBlkioStat was returning nil instead of ErrNotExist, if any file in /sys/fs/cgroup/blkio/ was not found. Correcting this error also, force to fix test cases, which were not verifying if the files were really missing. Signed-off-by: Kunal Kushwaha <kushwaha_kunal_v7@lab.ntt.co.jp>
kunalkushwaha
force-pushed
the
consistent-getstats
branch
from
f7ba098
to
b4483a7
Compare
|
@hqhq I have added a new function Also, I found, in |
| @@ -79,6 +79,8 @@ type BlkioStats struct { | |||
| IoMergedRecursive []BlkioStatEntry `json:"io_merged_recursive,omitempty"` | |||
| IoTimeRecursive []BlkioStatEntry `json:"io_time_recursive,omitempty"` | |||
| SectorsRecursive []BlkioStatEntry `json:"sectors_recursive,omitempty"` | |||
| ThrottleServiceBytes []BlkioStatEntry `json:"io_service_bytes,omitempty"` | |||
There was a problem hiding this comment.
I'd prefer the name to be ThrottleIoServiceBytes (and I'd expect we have a follow up PR to s/Io/IO for all blkio entry names to keep consistency with other places) and json name to be throttle_io_service_bytes, and I think adding these two new fields should be in a separated commit.
Same with ThrottleServiced.
There was a problem hiding this comment.
Thanks for review comments. Will Incorporate this.
| @@ -226,12 +230,15 @@ func getStats(path string, stats *cgroups.Stats) error { | |||
| if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_service_bytes")); err != nil { | |||
| return err | |||
| } | |||
| stats.BlkioStats.IoServiceBytesRecursive = blkioStats | |||
| // stats.BlkioStats.IoServiceBytesRecursive = blkioStats | |||
There was a problem hiding this comment.
After a second thought, I think we might reuse IoServiceBytesRecursive for some reasons, it's been there since the old libcontainer days, maybe upper level monitor tools don't have to care if bandwidth or throttle is used by blkio. And your change will break these tools.
There was a problem hiding this comment.
I digged a bit and found the PR added this code: docker-archive/libcontainer#167
Looks like it's intentional.
There was a problem hiding this comment.
Actually, while testing my code I realized testcase TestNonCFQBlkioStats doesn’t executes for non-CFQ condition as blkio.io_serviced_recursive attribute is present. As result, It always executes CFQ case only.
If some tools depends on IoServiceBytesRecursive and do not honour ThrottleIoServiceBytes , I can re-test the results with them.
|
|
||
| if blkioStats, err = getBlkioStat(filepath.Join(path, "blkio.throttle.io_serviced")); err != nil { | ||
| return err | ||
| } | ||
| stats.BlkioStats.IoServicedRecursive = blkioStats | ||
| // stats.BlkioStats.IoServicedRecursive = blkioStats |
|
Feel free to update and reopen if you still think this PR should get in and you have time. Thanks! |
getBlkioStat was returning nil instead of ErrNotExist,
if any file in /sys/fs/cgroup/blkio/ was not found.
Correcting this error also, force to fix test cases,
which were not verifying if the files were really missing.
Signed-off-by: Kunal Kushwaha kushwaha_kunal_v7@lab.ntt.co.jp