Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Gastón Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/
---

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


Testing
---

Added a test and verified that it works on Linux.


Thanks,

Gastón Kleiman

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Kevin Klues]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168222
---




src/slave/containerizer/containerizer.hpp
Lines 155 (patched)


s/Destroy/Cleanup/



src/slave/containerizer/containerizer.hpp
Lines 157 (patched)


s/destroy/cleanup/

Maybe rewrite the comment to say:
```
// NOTE: You can only cleanup artifacts from a
// nested container that has been fully destroyed.
```



src/slave/containerizer/mesos/containerizer.cpp
Lines 2482 (patched)


It's probably worth putting a TODO here that this function should check 
that recovery has completed before continuing.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2486-2489 (patched)


`s/The nested container/Nested Container/`

`s/hasn't/has not/`

Also, (I know it's not consistent elsewhere) but can you put the 
containerID inside single quotes? Same for the error messages below too.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2494 (patched)


s/terminated/destroyed/



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2037 (patched)


Can you just have the command here be `true`?
Then you don't need the explicit destroy below -- the container will exit 
very quickly by itself.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2052-2054 (patched)


These two comments seem redundant.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2052-2056 (patched)


If you follow the suggestion above, you should expect an exit status of 0 
here.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2068 (patched)


Kill this newline



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2071-2075 (patched)


I think this check is unnecessary and it desitracts from the flow of what 
the test is trying to verify.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2083 (patched)


kill this newline



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2089 (patched)


Can you pull this wait out of the AWAIT_READY() call in order to be 
consistent with the pattern above?


- Kevin Klues


On March 7, 2017, 5:13 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 7, 2017, 5:13 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/1/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---




src/slave/containerizer/containerizer.hpp
Lines 158 (patched)


can we call it 'delete'? This is more consistent with 
`DeleteNestedContainer` call?


- Jie Yu


On March 7, 2017, 5:13 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 7, 2017, 5:13 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/1/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Gastón Kleiman]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/
---

(Updated March 8, 2017, 10:50 a.m.)


Review request for mesos, Alexander Rukletsov, Jie Yu, Kevin Klues, and Vinod 
Kone.


Changes
---

Addressed Kevin's feedback.


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 (updated)
-

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

Changes: https://reviews.apache.org/r/57384/diff/1-2/


Testing
---

Added a test and verified that it works on Linux.


Thanks,

Gastón Kleiman

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Gastón Kleiman]

> On March 8, 2017, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 158 (patched)
> > 
> >
> > can we call it 'delete'? This is more consistent with 
> > `DeleteNestedContainer` call?

What do you think about `deleteArtifacts`? `delete` is a reserved keyword, I 
might be able to name a method `delete`, but it is not very explicit... and it 
confuses vim ;-).


- Gastón


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Vinod Kone]

> On March 8, 2017, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 158 (patched)
> > 
> >
> > can we call it 'delete'? This is more consistent with 
> > `DeleteNestedContainer` call?
> 
> Gastón Kleiman wrote:
> What do you think about `deleteArtifacts`? `delete` is a reserved 
> keyword, I might be able to name a method `delete`, but it is not very 
> explicit... and it confuses vim ;-).

Yea, I don't like `cleanupArtifacts` either because it's not clear this is 
called by `DeleteNestedContainer`. We have a similar issue with 
`KillNestedContainer` where it calls `Containerizer::destroy()`. Sigh.

What about `RemoveNestedContainer` and `Containerizer::remove()`? `remove` and 
`delete` are synonyms afterall and docker uses it as well. Looks like k8s uses 
`delete`.


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Vinod Kone]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168385
---




src/slave/containerizer/mesos/containerizer.cpp
Lines 2483 (patched)


s/check/Check/



src/slave/containerizer/mesos/containerizer.cpp
Lines 2496 (patched)


Does a parent container's destruction imply removal of sandbox and runtime 
directories of itself and/or all its children?



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2077 (patched)


Can you add a test where you call delete of nested container after parent 
container has exited?


- Vinod Kone


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Vinod Kone]

> On March 8, 2017, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 158 (patched)
> > 
> >
> > can we call it 'delete'? This is more consistent with 
> > `DeleteNestedContainer` call?
> 
> Gastón Kleiman wrote:
> What do you think about `deleteArtifacts`? `delete` is a reserved 
> keyword, I might be able to name a method `delete`, but it is not very 
> explicit... and it confuses vim ;-).
> 
> Vinod Kone wrote:
> Yea, I don't like `cleanupArtifacts` either because it's not clear this 
> is called by `DeleteNestedContainer`. We have a similar issue with 
> `KillNestedContainer` where it calls `Containerizer::destroy()`. Sigh.
> 
> What about `RemoveNestedContainer` and `Containerizer::remove()`? 
> `remove` and `delete` are synonyms afterall and docker uses it as well. Looks 
> like k8s uses `delete`.

Also this needs to be moved up next to `destroy()`


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Vinod Kone]

> On March 8, 2017, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 158 (patched)
> > 
> >
> > can we call it 'delete'? This is more consistent with 
> > `DeleteNestedContainer` call?
> 
> Gastón Kleiman wrote:
> What do you think about `deleteArtifacts`? `delete` is a reserved 
> keyword, I might be able to name a method `delete`, but it is not very 
> explicit... and it confuses vim ;-).
> 
> Vinod Kone wrote:
> Yea, I don't like `cleanupArtifacts` either because it's not clear this 
> is called by `DeleteNestedContainer`. We have a similar issue with 
> `KillNestedContainer` where it calls `Containerizer::destroy()`. Sigh.
> 
> What about `RemoveNestedContainer` and `Containerizer::remove()`? 
> `remove` and `delete` are synonyms afterall and docker uses it as well. Looks 
> like k8s uses `delete`.
> 
> Vinod Kone wrote:
> Also this needs to be moved up next to `destroy()`

Another option is `GCNestedContainer` and `Containerizer::gc()`


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Vinod Kone]

> On March 8, 2017, 12:54 a.m., Jie Yu wrote:
> > src/slave/containerizer/containerizer.hpp
> > Lines 158 (patched)
> > 
> >
> > can we call it 'delete'? This is more consistent with 
> > `DeleteNestedContainer` call?
> 
> Gastón Kleiman wrote:
> What do you think about `deleteArtifacts`? `delete` is a reserved 
> keyword, I might be able to name a method `delete`, but it is not very 
> explicit... and it confuses vim ;-).
> 
> Vinod Kone wrote:
> Yea, I don't like `cleanupArtifacts` either because it's not clear this 
> is called by `DeleteNestedContainer`. We have a similar issue with 
> `KillNestedContainer` where it calls `Containerizer::destroy()`. Sigh.
> 
> What about `RemoveNestedContainer` and `Containerizer::remove()`? 
> `remove` and `delete` are synonyms afterall and docker uses it as well. Looks 
> like k8s uses `delete`.
> 
> Vinod Kone wrote:
> Also this needs to be moved up next to `destroy()`
> 
> Vinod Kone wrote:
> Another option is `GCNestedContainer` and `Containerizer::gc()`

Looks like `delete` is better than `remove` because the former connotes 
permanence. 
http://english.stackexchange.com/questions/52508/difference-between-delete-and-remove


- Vinod


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168226
---


On March 8, 2017, 10:50 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57384/
> ---
> 
> (Updated March 8, 2017, 10:50 a.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/2/
> 
> 
> Testing
> ---
> 
> Added a test and verified that it works on Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Gastón Kleiman]

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


Changes
---

Addressed Vinod's feedback (modulo naming).


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 (updated)
-

  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/

Changes: https://reviews.apache.org/r/57384/diff/2-3/


Testing
---

Added a test and verified that it works on Linux.


Thanks,

Gastón Kleiman

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Gastón Kleiman]

> On March 9, 2017, 1:01 a.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 2496 (patched)
> > 
> >
> > Does a parent container's destruction imply removal of sandbox and 
> > runtime directories of itself and/or all its children?

AFAIK the garbage collection of a container's sandbox and runtime directories 
is scheduled when the container is destroyed. These directories contain the 
sandbox/runtime directories of the container's children, so they will 
eventually also be removed. I added a comment explaining this.


- Gastón


---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168385
---


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

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Jie Yu]

---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/57384/#review168515
---




src/slave/containerizer/mesos/containerizer.cpp
Lines 2386 (patched)


s/cleanupArtifacts/remove/

It's not just about artifacts. This interface will also remove the metadata 
associated with the container.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2391 (patched)


Instead of CHECK, I would return a Failure here. We should avoid CHECK if 
possible.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2395-2396 (patched)


Error message here should not include 'containerId' because that's the 
responsibility of the caller.

Ditto elsewhere.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2401-2406 (patched)


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.



src/slave/containerizer/mesos/containerizer.cpp
Lines 2413 (patched)


I'll kill this line



src/slave/containerizer/mesos/containerizer.cpp
Lines 2426 (patched)


I'll kill this line



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 1990 (patched)


I'll adjust the name of this test according to my comments above.



src/tests/containerizer/nested_mesos_containerizer_tests.cpp
Lines 2157-2159 (patched)


you may want to adjust here.


- Jie Yu


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

Re: Review Request 57384: Added `Containerizer::cleanupArtifacts`.

[Alexander Rukletsov]

> On March 9, 2017, 9:16 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/containerizer.cpp
> > Lines 2391 (patched)
> > 
> >
> > 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)
> > 
> >
> > 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
> 
>