----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52473/#review151184 -----------------------------------------------------------
src/tests/containerizer/mesos_containerizer_tests.cpp (line 111) <https://reviews.apache.org/r/52473/#comment219462> Can we set local == true consistently in tests if possible. It's much easier to debug if test fails. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 48) <https://reviews.apache.org/r/52473/#comment219464> We no longer use 'using namspace' in our code base. Please use 'using process::XXX' explictly. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 426) <https://reviews.apache.org/r/52473/#comment219476> dash (default shell on ubuntu) does not support double digit file descriptor. This is test may be flaky on some linux distributions. We should force to use 'bash' which does not have this issue. https://bugs.launchpad.net/ubuntu/+source/dash/+bug/249620 (BTW, we ran into this issue during debugging and I spent on like 4 hours debugging this issue) To force to use 'bash', you need to use the non shell version of CommandInfo: ``` shell=false value=bash arguments=['bash', '-c', 'read key <&x'] ``` src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 510 - 516) <https://reviews.apache.org/r/52473/#comment219477> Aha, this is correct :) I think we may want to factor out this into createCommandInfo helper to support non shell version. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (line 662) <https://reviews.apache.org/r/52473/#comment219478> Why reset here given you'll reset below? Please fix all other occurances. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 750 - 752) <https://reviews.apache.org/r/52473/#comment219480> I don't like the current way of simulating a launcher orphan. First, it's linux launcher specific. It uses some internal impl. details in the linux launcher (painful to update when that layout changes). In the future, I think we should still parameterize the tests in this file to use both posix launcher or linux launcher. To simulate a launcher orphan, i think we should launch container, and then wipe the runtime dir. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 942 - 943) <https://reviews.apache.org/r/52473/#comment219536> I'd prefer getting `containerizer->wait` to the next line. src/tests/containerizer/nested_mesos_containerizer_tests.cpp (lines 955 - 956) <https://reviews.apache.org/r/52473/#comment219537> Instead of calling 'status' to test if a container has gone or not. I'd actually prefer we call 'containers' and test if containerId belongs to the returned hashset. This is because 'status' can fail in different ways, not just because the container does not exist. - Jie Yu On Oct. 3, 2016, 3:28 a.m., Benjamin Hindman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52473/ > ----------------------------------------------------------- > > (Updated Oct. 3, 2016, 3:28 a.m.) > > > Review request for mesos, Gilbert Song, Jie Yu, and Kevin Klues. > > > Repository: mesos > > > Description > ------- > > Added nested MesosContainerizer tests. > > > Diffs > ----- > > src/Makefile.am c897d863bde284de41c99ed50ccbbdfd2dcad23b > src/tests/containerizer/mesos_containerizer_tests.cpp > 8b5a19e121ef74eaf99b39682f8fd170605bf56d > src/tests/containerizer/nested_container_tests.cpp > 8430823d54d132bc3e9c3e0781f027072999683e > src/tests/containerizer/nested_mesos_containerizer_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/52473/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Benjamin Hindman > >
