----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/61387/#review182189 -----------------------------------------------------------
LGTM. Left mostly style comments. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 354-358 (patched) <https://reviews.apache.org/r/61387/#comment258059> No extra indent because of preprocessor conditional; indent this like e.g., `flags.bounding_capabilities = ...` src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 368-371 (patched) <https://reviews.apache.org/r/61387/#comment258060> Indent by two spaces less. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 380-381 (patched) <https://reviews.apache.org/r/61387/#comment258061> Indent by two spaces less. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 388 (patched) <https://reviews.apache.org/r/61387/#comment258062> `ASSERT_FALSE(offers->empty())`. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 392-396 (patched) <https://reviews.apache.org/r/61387/#comment258066> Currently the declaration and first use are not on the same page for me. Let's move this right above its use, driver.acceptOffers({offer.id()}, {LAUNCH_GROUP(executorInfo, taskGroup)}) src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 402-407 (patched) <https://reviews.apache.org/r/61387/#comment258063> Indent by two spaces less. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 407 (patched) <https://reviews.apache.org/r/61387/#comment258072> Let's set a `TaskID` here to ease debugging in below loop over the status updates. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 416-417 (patched) <https://reviews.apache.org/r/61387/#comment258067> It seems very unlikely that the simple shell script we add every consume 10s of CPU time, but would I wonder if it makes sense to make this number truely unrealistically large, e.g., 1000000 and 2000000 instead of 10 and 20. That way we'd never fail the test because of flakiness but only due to global timeouts. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 423-428 (patched) <https://reviews.apache.org/r/61387/#comment258064> Indent by two spaces less. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 448 (patched) <https://reviews.apache.org/r/61387/#comment258068> Maybe `s/dummy/inSequence/`. It is probably a good idea to add a comment explaining that it is fine that we never use this var. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 449 (patched) <https://reviews.apache.org/r/61387/#comment258069> It would be nice to avoid hardcoding `4` here, e.g., foreach (Future<TaskStatus> update, updates) { EXPECT_CALL(sched, statusUpdate(&driver, _)) .WillOnce(FutureArg<1>(&update)); } src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 459 (patched) <https://reviews.apache.org/r/61387/#comment258075> A comment briefly summarizing the testing strategy (expected transitions, independent tracking of task status) would be great here. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 460 (patched) <https://reviews.apache.org/r/61387/#comment258076> We do not seem to put these on a single line and instead would use enum class Stage { INITIAL, RUNNING, FINISHED }; The only other case where we us a one-liner is in `check_tests.cpp` (also added by you). src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 463-464 (patched) <https://reviews.apache.org/r/61387/#comment258070> This seems to be equivalent to the shorter taskStages[task1.task_id()] = Stage::INITIAL; taskStages[task2.task_id()] = Stage::INITIAL; Here and in all subsequent uses of `hashmap<K,V>::put`. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 466 (patched) <https://reviews.apache.org/r/61387/#comment258065> Stray space before closing paren. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 466-469 (patched) <https://reviews.apache.org/r/61387/#comment258077> Let's avoid hardcoding hardcoding `4` here, e.g., instead foreach (const Future<TaskStatus>& update, updates) { AWAIT_READY(update); // Either bind `taskStatus` like you currently do, or use `update->` directly (would look nicer if `updates` was named `taskStatuses` so we'd have a future `taskStatus` instead of `update`). } src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 476 (patched) <https://reviews.apache.org/r/61387/#comment258074> Let's output the full `taskStatus` when this assert fires. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 483 (patched) <https://reviews.apache.org/r/61387/#comment258073> Let's output the full `taskStatus` when this assert fires. src/tests/containerizer/posix_rlimits_isolator_tests.cpp Lines 490 (patched) <https://reviews.apache.org/r/61387/#comment258071> `s/updates[1]/taskStatus/`. - Benjamin Bannier On Aug. 4, 2017, 12:10 a.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/61387/ > ----------------------------------------------------------- > > (Updated Aug. 4, 2017, 12:10 a.m.) > > > Review request for mesos, Benjamin Bannier and Jie Yu. > > > Bugs: MESOS-7849 > https://issues.apache.org/jira/browse/MESOS-7849 > > > Repository: mesos > > > Description > ------- > > Make the rlimits isolator support nesting. Without this patch rlimits > sets for tasks launched by the DefaultExecutor are silently ignored. > > > Diffs > ----- > > src/slave/containerizer/mesos/isolators/posix/rlimits.hpp > ee36a24ae36179d3ff3141f8c3cdc768b1e399af > src/slave/containerizer/mesos/isolators/posix/rlimits.cpp > 3fc2b3dbd5b382e422c948adae0dc50f4fbcfc49 > src/tests/containerizer/posix_rlimits_isolator_tests.cpp > b7ccc7eeeb7e4d0d8f4fd463d506cfe7157076a4 > > > Diff: https://reviews.apache.org/r/61387/diff/2/ > > > Testing > ------- > > `bin/mesos-tests.sh > --gtest_filter="PosixRLimitsIsolatorTest.NestedContainers" > --gtest_repeat=1000 --gtest_break_on_failure` passed on a machine running > GNU/Linux. > > > Thanks, > > Gastón Kleiman > >
