> On May 10, 2018, 9:23 p.m., Greg Mann wrote: > > src/slave/containerizer/composing.cpp > > Lines 488-502 (original), 493-512 (patched) > > <https://reviews.apache.org/r/66669/diff/2/?file=2012421#file2012421line493> > > > > I'm wondering if, instead of also adding a callback for this in > > `destroy()`, perhaps we should unconditionally remove the container from > > the `containers_` map in the callbacks which are registered on `wait()`? > > > > i.e. we would do the following here: > > ``` > > if (launchResult == Containerizer::LaunchResult::SUCCESS) { > > // Note that we don't update the state if a destroy is in progress. > > if (container->state == LAUNCHING) { > > container->state = LAUNCHED; > > } > > > > // This is needed for eventually removing the given container from > > // the list of active containers. > > container->containerizer->wait(containerId) > > .onAny(defer(self(), [=](const > > Future<Option<ContainerTermination>>&) { > > if (containers_.contains(containerId)) { > > delete containers_.at(containerId); > > containers_.erase(containerId); > > } > > })); > > > > // Note that the return value is not impacted > > // by whether a destroy is currently in progress. > > return Containerizer::LaunchResult::SUCCESS; > > } > > ``` > > > > and NOT register the callback in `destroy()`. > > > > This might also mean that we could remove `if > > (containers_.contains(containerId))` and instead do > > `CHECK(containers_.contains(containerId));`? > > > > WDYT?
Composing c'zer adds a container to the `containers_` map and then calls `launch()` on underlying c'zer. However, composing c'zer doesn't handle the case when an underlying containerizer returns a failure during the launch, so the container will not be removed from the `containers_` map. If we want to call `destroy()` without the callback, then there is no chance to get the container removed from the `containers_` map in this case. To check that, you can remove the callback from `destroy()` (and optionally copy-paste `containerizer->wait(containerId).onAny(...)` to `_recover()`), then run tests and you'll get a few tests failing, including `ParentChildContainerTypeAndContentType/AgentContainerAPITest.NestedContainerFailLaunch/0`. We could address the issue by handling the launch failures, but there are 3 places where we should handle it. That change would make composing c'zer more complex IMO. - Andrei ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/66669/#review202868 ----------------------------------------------------------- On April 17, 2018, 3:23 p.m., Andrei Budnik wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/66669/ > ----------------------------------------------------------- > > (Updated April 17, 2018, 3:23 p.m.) > > > Review request for mesos, Alexander Rukletsov, Greg Mann, Jie Yu, and Qian > Zhang. > > > Bugs: MESOS-8714 > https://issues.apache.org/jira/browse/MESOS-8714 > > > Repository: mesos > > > Description > ------- > > This patch adds callbacks on `wait()` and `destroy()` in composing > containerizer to remove a container from the `containers_` map after > the container's termination. > > > Diffs > ----- > > src/slave/containerizer/composing.cpp > 186102c66d373dcd799cadd9fed7d1c8cb894971 > > > Diff: https://reviews.apache.org/r/66669/diff/2/ > > > Testing > ------- > > internal CI > > > Thanks, > > Andrei Budnik > >
