> On March 9, 2017, 9:16 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/containerizer.cpp > > Lines 2391 (patched) > > <https://reviews.apache.org/r/57384/diff/3/?file=1660152#file1660152line2391> > > > > Instead of CHECK, I would return a Failure here. We should avoid CHECK > > if possible.
This depends on whether it's an internal or external invariant. If we say this function _must_ be called for nested containers only (that's what the comment says now), `CHECK` is fine I'd say. It is the caller's responsibility then to ensure this function is called for a nested container. > On March 9, 2017, 9:16 p.m., Jie Yu wrote: > > src/slave/containerizer/mesos/containerizer.cpp > > Lines 2401-2406 (patched) > > <https://reviews.apache.org/r/57384/diff/3/?file=1660152#file1660152line2401> > > > > I would return an Error here instead. The reason is because the caller > > expects the sandbox associated with the nested container is gone if this > > call returns a success. The code here will break this invariant. > > > > I would also adjust the comments here. Agree with Jie. We should either return an error (and say that in the comment) or ensure this container's artifacts have already been revomed before returning here. - Alexander ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/57384/#review168515 ----------------------------------------------------------- On March 9, 2017, 1:46 p.m., Gastón Kleiman wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/57384/ > ----------------------------------------------------------- > > (Updated March 9, 2017, 1:46 p.m.) > > > Review request for mesos, Alexander Rukletsov, Jie Yu, Kevin Klues, and Vinod > Kone. > > > Bugs: MESOS-7120 > https://issues.apache.org/jira/browse/MESOS-7120 > > > Repository: mesos > > > Description > ------- > > This new method cleans up the sandbox and runtime directories of a > terminated nested container. > > > Diffs > ----- > > src/slave/containerizer/containerizer.hpp > f65a9b9761fc254bd0778bf13aac9b256497b22f > src/slave/containerizer/mesos/containerizer.hpp > 09f94ccb3224c14a9324961b789455b119ec2257 > src/slave/containerizer/mesos/containerizer.cpp > b001d0265ec73822193eaac74c631930acce90c0 > src/tests/containerizer/nested_mesos_containerizer_tests.cpp > ea01fe55a28d17105157004d8cf0976202a49b7c > > > Diff: https://reviews.apache.org/r/57384/diff/3/ > > > Testing > ------- > > Added a test and verified that it works on Linux. > > > Thanks, > > Gastón Kleiman > >
