You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by GitBox <gi...@apache.org> on 2022/08/27 09:28:19 UTC
[GitHub] [mesos] cf-natali opened a new pull request, #446: Blkio isolator: handled missing CFQ statistics.
cf-natali opened a new pull request, #446:
URL: https://github.com/apache/mesos/pull/446
CFQ was removed from 5.x kernel, so we need to silently ignore missing
CFQ statistics.
Before:
```
[----------] 1 test from CgroupsIsolatorTest [ RUN ]
[...]
CgroupsIsolatorTest.ROOT_CGROUPS_BlkioUsage W0824 21:58:36.604790 308667
cgroups.cpp:932] Skipping resource statistic for container
b2d67073-85fa-4a0b-84a3-790791047eac because: Failed to read from
'blkio.time': No such file or directory
../../src/tests/containerizer/cgroups_isolator_tests.cpp:2656: Failure
Value of: usage->has_blkio_statistics() Actual: false Expected: true [
[...]
FAILED ] CgroupsIsolatorTest.ROOT_CGROUPS_BlkioUsage (1888 ms)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r962313764
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
Is there a way to detect whether CFQ is support or not? If it is not supported, then maybe we can skip calling the following `cfq::` methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r1027276119
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,230 +108,240 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
- Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
- if (time.isError()) {
- return Failure(time.error());
+ // Get CFQ statistics, if available - CFQ was removed from kernel 5.0.
Review Comment:
Can you please add https://github.com/torvalds/linux/commit/f382fb0bcef4c37dc049e9f6963e3baf204d815c in this comment for more context?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r1027276222
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
Thanks for the update.
Do we need to update any tests accordingly?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r966529371
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
If we know the specific kernel version where CFQ was removed, can we use `kernelVersion()` to detect it like [this code](https://github.com/apache/mesos/blob/96339efb53f7cdf1126ead7755d2b83b435e3263/src/slave/containerizer/mesos/isolators/linux/nnp.cpp#L38)?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r962313764
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
Is there a way to detect whether CFQ is supported or not? If it is not supported, then maybe we can skip calling the following `cfq::` methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r990721931
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
@cf-natali Any comments?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r1027270645
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
Sorry for the delay, I've been busy lately.
It was removed from kernel 5.0: https://github.com/torvalds/linux/commit/f382fb0bcef4c37dc049e9f6963e3baf204d815c
I've updated the change to explicitly change the kernel version - gitlab renders the diff in a sub-optimal way, but the change is actually very small (adding the version check).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r1027279343
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
No, the test that was breaking are now passing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r1027279611
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,230 +108,240 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
- Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
- if (time.isError()) {
- return Failure(time.error());
+ // Get CFQ statistics, if available - CFQ was removed from kernel 5.0.
Review Comment:
Done.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] qianzhangxa merged pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
qianzhangxa merged PR #446:
URL: https://github.com/apache/mesos/pull/446
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r966451142
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
OK to merge?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [mesos] cf-natali commented on a diff in pull request #446: Blkio isolator: handled missing CFQ statistics.
Posted by GitBox <gi...@apache.org>.
cf-natali commented on code in PR #446:
URL: https://github.com/apache/mesos/pull/446#discussion_r964046502
##########
src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp:
##########
@@ -107,226 +107,205 @@ Future<ResourceStatistics> BlkioSubsystemProcess::usage(
CgroupInfo::Blkio::CFQ::Statistics totalCfqRecursive;
CgroupInfo::Blkio::Throttling::Statistics totalThrottling;
- // Get CFQ statistics.
+ // CFQ was removed from kernel 5.x, so be ready to handle missing statistics.
+
Try<vector<cgroups::blkio::Value>> time = cfq::time(hierarchy, cgroup);
Review Comment:
Hm, not really.
The only way would be maybe to look at all block devices `/sys/block/*/queue/scheduler` and see if any of those files contains `cfq`, but I think it's more fragile.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: reviews-unsubscribe@mesos.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org