You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Gastón Kleiman <ga...@mesosphere.io> on 2017/03/10 17:58:53 UTC
Re: Review Request 57384: Added `Containerizer::remove`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/
-----------------------------------------------------------
(Updated March 10, 2017, 5:58 p.m.)
Review request for mesos, Alexander Rukletsov, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Renamed the method + rebased + addressed feedback.
Summary (updated)
-----------------
Added `Containerizer::remove`.
Bugs: MESOS-7120
https://issues.apache.org/jira/browse/MESOS-7120
Repository: mesos
Description (updated)
-------
This new method removes the sandbox and runtime directories of a
terminated nested container.
Diffs (updated)
-----
src/slave/containerizer/containerizer.hpp f65a9b9761fc254bd0778bf13aac9b256497b22f
src/slave/containerizer/mesos/containerizer.hpp 09f94ccb3224c14a9324961b789455b119ec2257
src/slave/containerizer/mesos/containerizer.cpp 7676a4d8a35f2239c7373b0a9e76fa343b322f62
src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
Diff: https://reviews.apache.org/r/57384/diff/4/
Changes: https://reviews.apache.org/r/57384/diff/3-4/
Testing
-------
Added a test and verified that it works on Linux.
Thanks,
Gast�n Kleiman
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168760
-----------------------------------------------------------
Ship it!
Ship It!
- Vinod Kone
On March 13, 2017, 12:32 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 12:32 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 removes 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 7676a4d8a35f2239c7373b0a9e76fa343b322f62
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
>
>
> Diff: https://reviews.apache.org/r/57384/diff/5/
>
>
> Testing
> -------
>
> Added a test and verified that it works on Linux.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Alexander Rukletsov <ru...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168797
-----------------------------------------------------------
Ship it!
Ship It!
- Alexander Rukletsov
On March 13, 2017, 2:25 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> -----------------------------------------------------------
>
> (Updated March 13, 2017, 2:25 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 removes 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 7676a4d8a35f2239c7373b0a9e76fa343b322f62
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
>
>
> Diff: https://reviews.apache.org/r/57384/diff/6/
>
>
> Testing
> -------
>
> Added a test and verified that it works on Linux.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/
-----------------------------------------------------------
(Updated March 13, 2017, 2:25 p.m.)
Review request for mesos, Alexander Rukletsov, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Removed `containerId` from the failures.
Bugs: MESOS-7120
https://issues.apache.org/jira/browse/MESOS-7120
Repository: mesos
Description
-------
This new method removes the sandbox and runtime directories of a
terminated nested container.
Diffs (updated)
-----
src/slave/containerizer/containerizer.hpp f65a9b9761fc254bd0778bf13aac9b256497b22f
src/slave/containerizer/mesos/containerizer.hpp 09f94ccb3224c14a9324961b789455b119ec2257
src/slave/containerizer/mesos/containerizer.cpp 7676a4d8a35f2239c7373b0a9e76fa343b322f62
src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
Diff: https://reviews.apache.org/r/57384/diff/6/
Changes: https://reviews.apache.org/r/57384/diff/5-6/
Testing
-------
Added a test and verified that it works on Linux.
Thanks,
Gast�n Kleiman
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/
-----------------------------------------------------------
(Updated March 13, 2017, 12:32 p.m.)
Review request for mesos, Alexander Rukletsov, Jie Yu, Kevin Klues, and Vinod Kone.
Changes
-------
Improved comment.
Bugs: MESOS-7120
https://issues.apache.org/jira/browse/MESOS-7120
Repository: mesos
Description
-------
This new method removes the sandbox and runtime directories of a
terminated nested container.
Diffs (updated)
-----
src/slave/containerizer/containerizer.hpp f65a9b9761fc254bd0778bf13aac9b256497b22f
src/slave/containerizer/mesos/containerizer.hpp 09f94ccb3224c14a9324961b789455b119ec2257
src/slave/containerizer/mesos/containerizer.cpp 7676a4d8a35f2239c7373b0a9e76fa343b322f62
src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
Diff: https://reviews.apache.org/r/57384/diff/5/
Changes: https://reviews.apache.org/r/57384/diff/4-5/
Testing
-------
Added a test and verified that it works on Linux.
Thanks,
Gast�n Kleiman
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Gastón Kleiman <ga...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168618
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Lines 2391 (patched)
<https://reviews.apache.org/r/57384/#comment240869>
I am new to the containerizer, so I tried to follow the current patterns.
The following methods have similar CHECKs:
`MesosContainerizerProcess::update`: https://github.com/apache/mesos/blob/487c667260ec89127e18c77b9e8c8c243cf6ec57/src/slave/containerizer/mesos/containerizer.cpp#L1877
`MesosContainerizerProcess::launch`: https://github.com/apache/mesos/blob/487c667260ec89127e18c77b9e8c8c243cf6ec57/src/slave/containerizer/mesos/containerizer.cpp#L964
In the case of this new method, the caller (`slave/http.cpp`) is responsible for validating that the `ContainerID` belongs to a nested container.
Having said this, I am also fine with returning a `Failure` here, but I'd also keep the validation in the agent, in order to be able to return a 400 - Bad Request instead of a 500 - Internal Server Error.
src/slave/containerizer/mesos/containerizer.cpp
Lines 2395-2396 (patched)
<https://reviews.apache.org/r/57384/#comment240870>
I followed this example: https://github.com/apache/mesos/blob/487c667260ec89127e18c77b9e8c8c243cf6ec57/src/slave/containerizer/mesos/containerizer.cpp#L1740-L1743
Should I create a patch updating that failure?
- Gast�n Kleiman
On March 10, 2017, 5:58 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:58 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 removes 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 7676a4d8a35f2239c7373b0a9e76fa343b322f62
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
>
>
> Diff: https://reviews.apache.org/r/57384/diff/4/
>
>
> Testing
> -------
>
> Added a test and verified that it works on Linux.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>
Re: Review Request 57384: Added `Containerizer::remove`.
Posted by Vinod Kone <vi...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168650
-----------------------------------------------------------
src/slave/containerizer/mesos/containerizer.cpp
Lines 2403 (patched)
<https://reviews.apache.org/r/57384/#comment240889>
I think this behaviour should be called out in the comments in the header because it's not intuitive. Ideally we should be able to definitively say whether its sandbox directory is gone, but I guess it's not easy. Runtime directory existence check seems straightforward.
- Vinod Kone
On March 10, 2017, 5:58 p.m., Gast�n Kleiman wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> -----------------------------------------------------------
>
> (Updated March 10, 2017, 5:58 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 removes 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 7676a4d8a35f2239c7373b0a9e76fa343b322f62
> src/tests/containerizer/nested_mesos_containerizer_tests.cpp ea01fe55a28d17105157004d8cf0976202a49b7c
>
>
> Diff: https://reviews.apache.org/r/57384/diff/4/
>
>
> Testing
> -------
>
> Added a test and verified that it works on Linux.
>
>
> Thanks,
>
> Gast�n Kleiman
>
>