----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51031/#review153817 -----------------------------------------------------------
src/tests/containerizer/cgroups_tests.cpp (lines 135 - 150) <https://reviews.apache.org/r/51031/#comment223279> We might need to cleanup this part. It's likely that `baseHierarchy` is not the parent of all the subsystems. Imagine a subsystem is not mounted initially. And we'll use TEST_CGROUPS_HIERARCHY as the base. However, if all other subsystems are mounted under /sys/fs/cgroup, then this test fixture will fail. We can address that in the separate patch, but I'd like to cleanup this tech debt. src/tests/containerizer/cgroups_tests.cpp (lines 169 - 177) <https://reviews.apache.org/r/51031/#comment223281> Looks like we can just call cgroups::destroy(TEST_CGROUPS_ROOT) here which recursively kills cgroups? src/tests/containerizer/cgroups_tests.cpp (lines 184 - 197) <https://reviews.apache.org/r/51031/#comment223280> Looks like no one will try to cleanup TEST_CGROUPS_HIERARCHY? Again, I feel that we should fix this test fixture. src/tests/containerizer/cgroups_tests.cpp (lines 187 - 195) <https://reviews.apache.org/r/51031/#comment223282> Ditto. Is this just recursive cgroups::destroy? src/tests/containerizer/cgroups_tests.cpp (line 386) <https://reviews.apache.org/r/51031/#comment223284> I'd prefer we declare them at the top: ``` const string cgroup1 = path::join(...); const string cgroup2 = path::join(...); const string cgroup3 = path::join(...); ... ASSERT_SOME(cgroups::create(hierarchy, cgroup1, true)); ... ``` src/tests/containerizer/cgroups_tests.cpp (lines 406 - 440) <https://reviews.apache.org/r/51031/#comment223283> I'd prefer we convert cgroups into a hashset and use cgroups.contains here. - Jie Yu On Oct. 24, 2016, 2:39 p.m., haosdent huang wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51031/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2016, 2:39 p.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, Zhengju Sha, and > Jiang Yan Xu. > > > Bugs: MESOS-6035 > https://issues.apache.org/jira/browse/MESOS-6035 > > > Repository: mesos > > > Description > ------- > > In some cases, we just want to get the children cgroups instead of > retrieve descendant cgroups recursively. We added an argument to > `cgroups::get` to indicate whether to retrieve cgroups recursively but > made recursive retrieve the default behaviour. This patch fixed some > incorrect `TEST_CGROUPS_ROOT` checks as well. > > > Diffs > ----- > > src/linux/cgroups.hpp cfce09cb57501f2c988a8d997d7c6150280ed53d > src/linux/cgroups.cpp 630e2ec2d0eac2bb35488d0416883df1601224c8 > src/tests/containerizer/cgroups_tests.cpp > 0afaec6ae948cabf1472bf01103210d8f9809cb1 > > Diff: https://reviews.apache.org/r/51031/diff/ > > > Testing > ------- > > ``` > sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*" --verbose > ``` > > > Thanks, > > haosdent huang > >
