mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benno Evers <bev...@mesosphere.com>
Subject Re: Review Request 69436: Fixed flaky check in cluster::Slave destructor.
Date Thu, 13 Dec 2018 14:13:47 GMT


> On Nov. 23, 2018, 1:44 p.m., Andrei Budnik wrote:
> > src/tests/cluster.cpp
> > Lines 699 (patched)
> > <https://reviews.apache.org/r/69436/diff/1/?file=2110098#file2110098line699>
> >
> >     What if there is more than one container whose `Future<ContainerTermination>`
is ready? Should we call `settle()` for each of these containers?
> 
> Benno Evers wrote:
>     At this point we know from the `foreach`-loop above that all `Future<ContainerTermination>`s
are ready, so all calls to `defer()` in `composing.cpp` should have already happened. So I
think a single call to settle should guarantee that all of them will be processed by libprocess
before the clock is settled.
> 
> Andrei Budnik wrote:
>     Can we check this assumption by trying to launch say 1000 containers in the given
test?
> 
> Joseph Wu wrote:
>     This might be a common (among Mesos test writers) misconception of what `Clock::settle()`
actually does.  Operations involving the `Clock` will only affect timers and delayed events.
 Other dispatches and deferals are not affected.
>     
>     ```
>     Clock::pause();
>     
>     // If I delay something to the future here:
>     delay(Seconds(10), somePid, someFunction);
>     
>     // And then I advance the clock to trigger the functions:
>     Clock::advance(Seconds(10));
>     
>     // Checking state mutated by the delayed function will race with this thread (main
thread vs libprocess worker thread).
>     // Unless, we settle the clock.  This explicitly waits for the delayed functions
to finish, on the calling thread.
>     Clock::settle();
>     ```
>     
>     This means adding a `Clock::settle()` in this code is adding more a bit more computation
between terminating the last container, and calling `containerizer->containers()`.  Which
makes the flaky less likely, but still possible.
>     
>     We still have a race between two threads (main/test thread & containerizer thread),
gated on the same future, dispatching onto the composing containerizer.  
>     To completely remove the race, the main thread must dispatch on the Mesos/Docker
containerizer actor, not the composing containerizer actor, because the Mesos/Docker containerizer
actor's thread will be the one `defer`-ing onto the composing containerizer actor.  I think
the following might work:
>     
>     ```
>          foreach (const ContainerID& containerId, containers.get()) {
>            process::Future<Option<ContainerTermination>> termination =
>              containerizer->destroy(containerId);
>      
>            AWAIT(termination);
>      
>            if (!termination.isReady()) {
>              LOG(ERROR) << "...";
>            }
>           
>     +      // Even though we just destroyed the container, fetch the status to
>     +      // synchronize this thread with the containerizer thread(s).
>     +      // This ensures that any continuations triggered by container termination
>     +      // have been dispatched (although not necessarily completed).
>     +      Future<ContainerStatus> status = containerizer->status(containerId);
>     +      AWAIT(status);
>     +      
>     +      // We don't care about the success or failure of the `status` Future.
>     +      // There are three possible responses:
>     +      //   1) Mesos containerizer - This should return a failure since the container
>     +      //      no longer exists.
>     +      //   2) Docker containerizer - This containerizer seems to just return
>     +      //      a mostly-empty ContainerStatus object, regardless of whether the
>     +      //      container exists or not.
>     +      //   3) Composing containerizer - This could return either of the above, or
a
>     +      //      failure originating from the Composing containerizer itself.  This
is
>     +      //      because the Composing containerizer does not update its internal state
>     +      //      immediately upon terminating a container, since termination and state
>     +      //      must be performed on different actors.
>     +      //      If this returns (1) or (2), then we know that the next dispatch onto
>     +      //      the Composing containerizer actor will come after the Composing 
>     +      //      containerizer has had a chance to update its internal state.
>         }
>     ```
>     
>     Feel free to reword the comment if it feels wordy or rambling or confusing.  This
is not a straightforward race to put into text :)

Looking at `Clock::settle()`, it looks to me like it *does* have some guarantees about dispatches,
in particular all dispatched events that are ready to run will have finished:

    // When the clock is paused this returns only after
    //   (1) all expired timers are executed,
    //   (2) no processes are running, and
    //   (3) no processes are ready to run.
    //
    // In other words, this function blocks synchronously until no other
    // execution on any processes will occur unless the clock is
    // advanced.
    //
    // TODO(benh): Move this function elsewhere, for instance, to a
    // top-level function in the 'process' namespace since it deals with
    // both processes and the clock.
    static void settle();
    
So as far as I can see, both the original patch as well as your suggestion will work reliably
and do not contain any more flakiness.

Given that, we should probably choose the version that is easier to understand for a reader
who is not familiar with the code. Intuitively this would probably be Joseph's version, since
it doesn't require an understanding of the clock, but I'm hardly in a position to judge -
Greg, Andrei, any thoughts on this?


- Benno


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69436/#review210831
-----------------------------------------------------------


On Nov. 22, 2018, 7:43 p.m., Benno Evers wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69436/
> -----------------------------------------------------------
> 
> (Updated Nov. 22, 2018, 7:43 p.m.)
> 
> 
> Review request for mesos, Andrei Budnik, Greg Mann, and Joseph Wu.
> 
> 
> Bugs: MESOS-9272
>     https://issues.apache.org/jira/browse/MESOS-9272
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> The destructor of `cluster::Slave` contained an assertion
> that was not safe to assume in the presence of the
> composing containerizer. This commit adds an additional
> `Clock::settle()` to fix the issue.
> 
> 
> Diffs
> -----
> 
>   src/tests/cluster.cpp 2b351ca70d8e80008e49722aa7d46918b5ecd9b0 
> 
> 
> Diff: https://reviews.apache.org/r/69436/diff/1/
> 
> 
> Testing
> -------
> 
> Ran `./src/mesos-tests --gtest_filter="SlaveTest.DefaultExecutorCommandInfo" --verbose
--gtest_repeat=50000 --gtest_throw_on_failure` while simulatenously running `stress-ng --random
64` on the same machine.
> 
> (before the change, `SlaveTest.DefaultExecutorCommandInfo` would fail roughly once every
15000 runs without `stress-ng` and roughly once every 300 runs with `stress-ng`)
> 
> 
> Thanks,
> 
> Benno Evers
> 
>


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