> On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 64 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line64> > > > > One thing I am thinking is: in cgroups test, it's highly likely that a > > second test will fail during setup if the first test fails due to whatever > > reasons. > > > > For example, there are some processes left in a cgroup due to the > > failure of the first test, causing the second test (and all following > > tests) to fail during setup. > > > > Maybe what we can do is to declare a static flag in class CgroupsTest > > and set this flag if a test fails so that later tests will fail gracefully > > (have a print saying that it's due to the failure of the previous test), > > instead of a random failure.
Sounds good. I updated the output when we try and remove our old cgroup so that a user knows something seriously wrong has happened. > On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 151 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line151> > > > > Compilable? ASSERT_SOME does not return an ostream. I didn't include you on the review for that (https://reviews.apache.org/r/7619), but I refactored ASSERT_SOME to use gtest constructs. Of course this code compiles and all the tests pass! ;) > On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 152 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line152> > > > > It is possible that here "subsystem" is empty. The createHierarchy will > > fail when subsystem is empty. Fixed. > On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 373 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line373> > > > > we should add one more check here: > > > > ASSERT_EQ(2UL, cgroups.get().size()); Fixed. > On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 377 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line377> > > > > "mesos_test" should not be returned here. Thanks! This test obviously fails with my kernel version. ;) > On Oct. 17, 2012, 1:27 a.m., Jie Yu wrote: > > src/tests/cgroups_tests.cpp, line 382 > > <https://reviews.apache.org/r/7622/diff/1/?file=177252#file177252line382> > > > > Why expect cgroups to be empty here? Just a copy-pasta bug when I split this test into it's own NestedCgroup test. Again, my kernel version is too old to run this test. :( - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/7622/#review12489 ----------------------------------------------------------- On Oct. 24, 2012, 4:42 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/7622/ > ----------------------------------------------------------- > > (Updated Oct. 24, 2012, 4:42 a.m.) > > > Review request for mesos, Vinod Kone, Jie Yu, and Ben Mahler. > > > Description > ------- > > * Added new test fixtures: > (1) CgroupsNoHierarchyTest for running tests where we need to create > a hierarchy (and thus, most likely, no existing hierarchy can > exist because it will already have the cpu and or memory > subsystem attached). > (2) CgroupsAnyHierarchyTest and subclasses > CgroupsAnyHierarchyWithCpuMemoryTest and > CgroupsAnyHierarchyWithCpuMemoryFreezerTest for running tests > with any hierarchy provided it has necessary subsystems > attached. > > * Renamed cgroups (from "prof", "stu", etc. to "mesos_test") and > removed nested cgroups by default. The rename was done because we > might run tests inside of an existing hierarchy and we want to avoid > name clashes. The nested cgroups were removed in favor of a test > that explicitly tries to create nested cgroups (since some older > kernels with particular subsystems attached have a hard time with > this, and we'd like to detect that case explicitly). > > * Created an explicit test for nested cgroups (see above). > > * Updated the "write control" test to use a forked process rather than > the test process (to be more conservative in the presence of errors) > > * Updated the "listen event" (i.e., "oom") test to check for the > proper control first (memory.oom_control). > > * Updated the failure mechanism of forked (children) processes to use > 'abort' rather than 'ASSERT_*' and 'FAIL' in order to make test > output more readable upon failures. > > * Updated the notify mechanism from forked (children) processes to > parent processes to correctly distinguish a closed pipe from a value > written (to catch more instances of when the test is actually > failing). > > > Diffs > ----- > > src/tests/cgroups_tests.cpp a5098447081a87a2b69aadc4f1bdbd0aed7ac724 > > Diff: https://reviews.apache.org/r/7622/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
