----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/26486/#review57306 -----------------------------------------------------------
IIUC, the crux of the problem is that we're not properly forwarding 'destroy' calls to the right containerizer. The extra semantics introduced in this review therefore seem unnecessary, and definitely more complicated. Why not make sure we forward all calls to the right containerizer for whatever containerizer we're currently trying to use to launch the container? On top of that, we can keep around a hashset of ContainerID's that have been destroyed so that we don't retry to launch if one containerizer fails. I've made some comments inline to try and capture this. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment97971> I think we can get away with two extra hashsets here instead of the LaunchStatus struct. One hashset for ContainerIDs that are currently being launched and one hashset for ContainerIDs that have been asked to be destroyed. See more below. Also, always try and match the instance variable naming per class please. In this case the _ suffix was used. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment97974> This is introducing new semantics. Without the composing containerizer the slave would get back whatever it got back from the containerizer, and it seems like we should respect that and pass back whatever the result was, even if a destroy happens midway, which we just need to make sure we forward. src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment97972> So rather than do this here, let's do it at the begnning, in any of the 'launch' functions. We can update the comment around 'containers_' to reflect that these are containers that are launching or launched. Now, all other calls should properly get forwarded to the right containerizer! The one slightly wierd thing here is actually forwarding a call to a containerizer that maybe is going to decide it can't launch that container, which could result in a Failure being propagated back to the slave which causes unnice things to happen. To truely solve that problem what we should really be doing is creating a Promise that all subsequent calls hang off of until a containerizer has actually been found (or we return a failure because no containerizer would take the container). src/slave/containerizer/composing.cpp <https://reviews.apache.org/r/26486/#comment97973> Having to make the same call in two different places in this function (and the same is true in 'destroy' below too) is code smell to me. - Benjamin Hindman On Oct. 17, 2014, 7:56 p.m., Timothy Chen wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/26486/ > ----------------------------------------------------------- > > (Updated Oct. 17, 2014, 7:56 p.m.) > > > Review request for mesos and Benjamin Hindman. > > > Bugs: MESOS-1884 and MESOS-1915 > https://issues.apache.org/jira/browse/MESOS-1884 > https://issues.apache.org/jira/browse/MESOS-1915 > > > Repository: mesos-git > > > Description > ------- > > Review: https://reviews.apache.org/r/26486 > > > Diffs > ----- > > src/Makefile.am c44a9ad47d6e1262949b9049f4ae25b049440d99 > src/slave/containerizer/composing.cpp > 9022700b628d9746a6a8a17c9fbf1b1988da6fca > src/tests/composing_containerizer_tests.cpp PRE-CREATION > > Diff: https://reviews.apache.org/r/26486/diff/ > > > Testing > ------- > > make check > > > Thanks, > > Timothy Chen > >
