Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Gilbert Song]

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

(Updated Sept. 26, 2016, 5:34 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Changes
---

remove an empty line.


Repository: mesos


Description
---

This patch makes the mesos containerizer destroy to be recursive.
To destroy a container with children nested under it, all its
children will be destroyed first, recursively. Then, the container
will get destroyed after all children destroy finish.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
cf0ec671c1af7450c2eb44a899c98dbf403a76b1 
  src/slave/containerizer/mesos/containerizer.cpp 
a88e4039da7319bc5b6c4003b67ef7c23ad5c320 

Diff: https://reviews.apache.org/r/51674/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 27, 2016, 12:13 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51674/
> ---
> 
> (Updated Sept. 27, 2016, 12:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the mesos containerizer destroy to be recursive.
> To destroy a container with children nested under it, all its
> children will be destroyed first, recursively. Then, the container
> will get destroyed after all children destroy finish.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> cf0ec671c1af7450c2eb44a899c98dbf403a76b1 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a88e4039da7319bc5b6c4003b67ef7c23ad5c320 
> 
> Diff: https://reviews.apache.org/r/51674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Gilbert Song]

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

(Updated Sept. 26, 2016, 5:13 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Repository: mesos


Description
---

This patch makes the mesos containerizer destroy to be recursive.
To destroy a container with children nested under it, all its
children will be destroyed first, recursively. Then, the container
will get destroyed after all children destroy finish.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
cf0ec671c1af7450c2eb44a899c98dbf403a76b1 
  src/slave/containerizer/mesos/containerizer.cpp 
a88e4039da7319bc5b6c4003b67ef7c23ad5c320 

Diff: https://reviews.apache.org/r/51674/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Jie Yu]

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




src/slave/containerizer/mesos/containerizer.hpp (line 234)


typo?



src/slave/containerizer/mesos/containerizer.hpp (line 246)


of the container.



src/slave/containerizer/mesos/containerizer.cpp (lines 1707 - 1710)


Now that 'destroy' returns a Future, we can simply do:
```
list> destroys;
foreach (const ContainerID& child, container->children) {
  LOG(INFO) << "...";
  
  destroys.push_back(destroy(child));
}

return collect(destroys)
  .then(defer(self(), &Self::_destroy, containerId));
```



src/slave/containerizer/mesos/containerizer.cpp (line 1713)


You should not use 'await' here because if any of the destroy on child 
container fails, we should not continue destroy the parent container because 
that breaks our invariant.



src/slave/containerizer/mesos/containerizer.cpp (lines 1945 - 1964)


Per our discussion, this is not necessary. Please update it.


- 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/51674/
> ---
> 
> (Updated Sept. 24, 2016, 6:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch makes the mesos containerizer destroy to be recursive.
> To destroy a container with children nested under it, all its
> children will be destroyed first, recursively. Then, the container
> will get destroyed after all children destroy finish.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 16f9e3e92e90fe7f8a0ebd24e567800e1f285bc9 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 144b0db501d40d4e0bba12672723616bedd76e7e 
> 
> Diff: https://reviews.apache.org/r/51674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Gilbert Song]

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

(Updated Sept. 24, 2016, 11:50 a.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Repository: mesos


Description (updated)
---

This patch makes the mesos containerizer destroy to be recursive.
To destroy a container with children nested under it, all its
children will be destroyed first, recursively. Then, the container
will get destroyed after all children destroy finish.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
16f9e3e92e90fe7f8a0ebd24e567800e1f285bc9 
  src/slave/containerizer/mesos/containerizer.cpp 
144b0db501d40d4e0bba12672723616bedd76e7e 

Diff: https://reviews.apache.org/r/51674/diff/


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Qian Zhang]

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




src/slave/containerizer/mesos/containerizer.cpp (line 1669)


I think you need to put the code below this line into a helper method, and 
call the helper method in `await(subContainers).then(helper)`. Otherwise, we 
will destroy parent container and its nested containers in parallel.


- Qian Zhang


On Sept. 7, 2016, 5:53 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51674/
> ---
> 
> (Updated Sept. 7, 2016, 5:53 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported mesos containerizer destroy to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Jie Yu]

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



This patch does not look correct.

I think we need a top level destroy which is nesting aware. We might want to 
rename the existing destory to `_destroy` which is not nesting aware. THe top 
level destroy will recursively call destroy for its children and calling `wait` 
for them to finish before calling `_destory` for itself.


src/slave/containerizer/mesos/containerizer.cpp (line 1634)


why this change there? The original sentense sounds fine to me



src/slave/containerizer/mesos/containerizer.cpp (line 1645)


Again, why this change?



src/slave/containerizer/mesos/containerizer.cpp (lines 1652 - 1657)


This is not correct. 'destroy' simply initiate the destroy. You need to 
call 'wait' to wait for it to finish.


- Jie Yu


On Sept. 6, 2016, 9:53 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51674/
> ---
> 
> (Updated Sept. 6, 2016, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
> Wu, and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported mesos containerizer destroy to be nested aware.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 89b7e8db38916d69d9b2d4fe305d4397b0859a10 
> 
> Diff: https://reviews.apache.org/r/51674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 51674: Supported mesos containerizer destroy to be nested aware.

[Gilbert Song]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, Joseph 
Wu, and Kevin Klues.


Repository: mesos


Description
---

Supported mesos containerizer destroy to be nested aware.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
89b7e8db38916d69d9b2d4fe305d4397b0859a10 

Diff: https://reviews.apache.org/r/51674/diff/


Testing
---

make check


Thanks,

Gilbert Song