Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 14, 2015, 5:04 p.m.) Review request for mesos and Timothy Chen. Changes --- reverted back to using std namespace Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 9a7a951a72390b38f5dda3a3d50c4b35a6c784fd src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f src/tests/docker_containerizer_tests.cpp a3da786c1b733e9dd4abf1d02d5dae051393d921 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91647 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 14, 2015, 5:04 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 14, 2015, 5:04 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 9a7a951a72390b38f5dda3a3d50c4b35a6c784fd src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f src/tests/docker_containerizer_tests.cpp a3da786c1b733e9dd4abf1d02d5dae051393d921 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 14, 2015, 4:09 a.m.) Review request for mesos and Timothy Chen. Changes --- build fix. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 9a7a951a72390b38f5dda3a3d50c4b35a6c784fd src/slave/containerizer/docker.cpp cfb60177fe48ec0eeab12ff392c6c9f89634b92f src/tests/docker_containerizer_tests.cpp a3da786c1b733e9dd4abf1d02d5dae051393d921 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91372 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 10, 2015, 11:03 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91376 --- src/slave/containerizer/docker.cpp (line 1325) https://reviews.apache.org/r/36326/#comment144747 We also have a unit test in docker_containerizer_tests testing the usage call, can you see if it makes sense to add something to test the rss data as well? - Timothy Chen On July 10, 2015, 11:03 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1272 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1272 If you like to a leave a comment why we called this function we should probably comment at the place we call it not in the function (before docker-inspect). also s/didnt/didn't/g Wanted to emphasize the logic of why we need to update the pid there (and not say in collectUSage). On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1342 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1342 This seems too verbose being LOG(INFO). And what value do you think we have here logging this? I can change it to LOG(DEBUG). Wanted to log there to help us debug. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 10, 2015, 8:50 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1266 https://reviews.apache.org/r/36326/diff/4/?file=1004334#file1004334line1266 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. Jojy Varghese wrote: The thought was as follows: - We called inspect to get the pid. So first we check for a pid. Then we set the pid for the container (if the container exists). Then we call collectUsage to collect stats. - The check for DESTROYING is already being centralized in the collectUsage lambda. wanted to point out that we are not skipping the check for contanier at all. We pick the pid from the Docker::Container object being passed in by the then continuation. We still check the existence of container in the collection (member variable collection_). - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91342 --- src/slave/containerizer/docker.cpp (line 1216) https://reviews.apache.org/r/36326/#comment144687 let's spell out cgStats to cgroupStats src/slave/containerizer/docker.cpp (line 1219) https://reviews.apache.org/r/36326/#comment144688 Error getting cgroups statistics for container ' + stringify(containerId) + ': + cgroupStats.error() And why the extra LOG(WARNING) here? src/slave/containerizer/docker.cpp (line 1254) https://reviews.apache.org/r/36326/#comment144689 I think you reordered things here that used to be in _usage. We used to first check the containers map, then check is it destroying, that look at the pid and finally passing that on. Here the order is now pid check, and some reason skip the map check to get it but then checking it later on. I think we should go back to the exact same order as before unless you have reasons to do this change. src/slave/containerizer/docker.cpp (line 1260) https://reviews.apache.org/r/36326/#comment144690 If you like to a leave a comment why we called this function we should probably comment at the place we call it not in the function (before docker-inspect). also s/didnt/didn't/g src/slave/containerizer/docker.cpp (line 1302) https://reviews.apache.org/r/36326/#comment144692 Although it seems obvious from the right hand side you're getting the stat of cgroup, it wasn't obvious what type are you getting back, and more so about it's actually a TryStat If you look at our style guide we only try to use auto in places the right hand side completely capture the type information. I think we should spell out the type here. src/slave/containerizer/docker.cpp (line 1327) https://reviews.apache.org/r/36326/#comment144694 This seems too verbose being LOG(INFO). And what value do you think we have here logging this? - Timothy Chen On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 10:47 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 10, 2015, 11:03 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description (updated) --- Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Changes --- review comments @tim Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.hpp, line 230 https://reviews.apache.org/r/36326/diff/3/?file=1002973#file1002973line230 I dont' think we need to expose this. Since the method is a bit long, kept it but replaced other usage methods with lamdas. On July 8, 2015, 9:57 p.m., Timothy Chen wrote: src/slave/containerizer/docker.cpp, line 1239 https://reviews.apache.org/r/36326/diff/3/?file=1002974#file1002974line1239 It's probably not going to be used outside of usage, so I think it's safe to inline this in _usage, and if you do want a seperate method this probably don't need to be exposed too. same comments as above. - Jojy --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91005 --- On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91224 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 9, 2015, 8:38 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 9, 2015, 8:38 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:40 p.m.) Review request for mesos and Timothy Chen. Changes --- error message fix Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:02 p.m.) Review request for mesos and Timothy Chen. Changes --- minor formatting changes Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs (updated) - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91005 --- src/slave/containerizer/docker.hpp (line 230) https://reviews.apache.org/r/36326/#comment144203 I dont' think we need to expose this. src/slave/containerizer/docker.cpp (line 1239) https://reviews.apache.org/r/36326/#comment144201 It's probably not going to be used outside of usage, so I think it's safe to inline this in _usage, and if you do want a seperate method this probably don't need to be exposed too. src/slave/containerizer/isolators/cgroups/cpushare.cpp https://reviews.apache.org/r/36326/#comment144202 Let's try to make a seperate rb for touching cpushare, and another rb for just updating docker containerizer to use this. - Timothy Chen On July 8, 2015, 9:40 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:40 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
Re: Review Request 36326: containerizer: added cgroups based statistics.
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/#review91051 --- Patch looks great! Reviews applied: [36106, 36326] All tests passed. - Mesos ReviewBot On July 8, 2015, 9:40 p.m., Jojy Varghese wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/36326/ --- (Updated July 8, 2015, 9:40 p.m.) Review request for mesos and Timothy Chen. Bugs: MESOS-2951 https://issues.apache.org/jira/browse/MESOS-2951 Repository: mesos Description --- WIP Adding cgroups cpustat and memory statistics as preferred way to get usage statistics for containerizers. Jira: MESOS-2951 Diffs - src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba src/slave/containerizer/isolators/cgroups/cpushare.cpp f56e97dcf91a6f5c9a8abe4afe9dc1a1d47df330 Diff: https://reviews.apache.org/r/36326/diff/ Testing --- make check Thanks, Jojy Varghese
