> On Nov. 23, 2018, 5:44 a.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?
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 :)
- Joseph
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/69436/#review210831
-----------------------------------------------------------
On Nov. 22, 2018, 11:43 a.m., Benno Evers wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/69436/
> -----------------------------------------------------------
>
> (Updated Nov. 22, 2018, 11:43 a.m.)
>
>
> Review request for mesos, Andrei Budnik 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
>
>
