mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 61387: Made the rlimits isolator work with nested containers.
Date Fri, 04 Aug 2017 08:58:50 GMT

-----------------------------------------------------------
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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message