mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.
Date Fri, 10 Mar 2017 12:48:02 GMT


> 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
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message