----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/52235/#review150344 -----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.hpp (line 311) <https://reviews.apache.org/r/52235/#comment218288> I'd adjust the comment here: ``` Sandbox directory for the container. It is optional here because we don't keep track of sandbox directory for orphan containers. ``` src/slave/containerizer/mesos/containerizer.cpp (line 692) <https://reviews.apache.org/r/52235/#comment218292> As I mentioned in the earlier patch, this should return a Try src/slave/containerizer/mesos/containerizer.cpp (lines 695 - 699) <https://reviews.apache.org/r/52235/#comment218291> Instead of else if here, I'd do the following: ``` if (containerIds.isError()) { return Failure(...); } foreach (...) ``` src/slave/containerizer/mesos/containerizer.cpp (line 705) <https://reviews.apache.org/r/52235/#comment218296> Why the extra `!containerId.has_parent` check here? Will the first check implie the second check? src/slave/containerizer/mesos/containerizer.cpp (line 725) <https://reviews.apache.org/r/52235/#comment218295> I suggest we simply set 'directory' for all containers. It should be straightforward to do. src/slave/containerizer/mesos/containerizer.cpp (lines 726 - 729) <https://reviews.apache.org/r/52235/#comment218294> Should we factor out this into paths helper because we need to use the same layout when creating the sandbox for the nested container. ``` // Determine the sandbox if this is a nested container. Option<string> directory; if (containerId.has_parent()) { const ContainerID& rootContainerId = getRootContainerId(containerId); CHECK(containers_.contains(rootContainerId)); if (contaienrs_[rootContainerId]->direcotry.isSome()) { directory = paths::getSandboxPath( containers_[rootCOntainerId]->directory.get(), containerId); } } Owned<Container> container(new Container()); ... containers_[containerId] = container; if (containerId.has_parent()) { CHECK(containers_.contains(containerId.parent())); containers_[containerId.parent()]->children.insert(containerId); } ... ``` src/slave/containerizer/mesos/containerizer.cpp (lines 743 - 748) <https://reviews.apache.org/r/52235/#comment218298> We still need to call 'destroy' for a container that does not have pid to make sure it is properly cleaned up. ``` container->status = pid.isSome() ? reap(containerId, pid.get()) : Future<Option<int>>(None()); container->status->onAny(defer(self(), &Self::reaped, containerId)); ``` src/slave/containerizer/mesos/containerizer.cpp (lines 756 - 759) <https://reviews.apache.org/r/52235/#comment218300> I'd use a CHECK_SOME here given the sandbox logic above. src/slave/containerizer/mesos/containerizer.cpp (line 769) <https://reviews.apache.org/r/52235/#comment218299> kill this line src/slave/containerizer/mesos/containerizer.cpp (line 794) <https://reviews.apache.org/r/52235/#comment218301> Ditto. Make sure we register onAny on 'status' so that this orphan will be cleaned up. src/slave/containerizer/mesos/containerizer.cpp (lines 800 - 812) <https://reviews.apache.org/r/52235/#comment218302> Why having a separate look for extras? Can you just inline into the first loop? Let's keep creating of `Container` and insert into its parent close together. src/slave/containerizer/mesos/containerizer.cpp (lines 880 - 883) <https://reviews.apache.org/r/52235/#comment218303> Can you sync with @joseph? I think we need to make logger nesting aware as well. I would leave a TODO here (making logger nesting aware). Also, I'd prefer the following logic: ``` // TODO: Make logger nesting aware. if (!containerId.has_parent()) { ... logger->recover(...); } ``` src/slave/containerizer/mesos/containerizer.cpp (lines 906 - 909) <https://reviews.apache.org/r/52235/#comment218304> Why the 'await' here? We don't want to wait for the cleanup to finish for orphans. - Jie Yu On Sept. 24, 2016, 6:50 p.m., Gilbert Song wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/52235/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2016, 6:50 p.m.) > > > Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph > Wu, Kevin Klues, and Vinod Kone. > > > Repository: mesos > > > Description > ------- > > Supported mesos containerizer recover to be nested aware. > > > Diffs > ----- > > src/slave/containerizer/mesos/containerizer.hpp > 16f9e3e92e90fe7f8a0ebd24e567800e1f285bc9 > src/slave/containerizer/mesos/containerizer.cpp > 144b0db501d40d4e0bba12672723616bedd76e7e > > Diff: https://reviews.apache.org/r/52235/diff/ > > > Testing > ------- > > > Thanks, > > Gilbert Song > >
