Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

> On July 28, 2016, 12:01 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 486
> > 
> >
> > Would love to understand why test will cause this issue.
> 
> Qian Zhang wrote:
> It seems that comment was introduced by this ticket: 
> https://issues.apache.org/jira/browse/MESOS-1438 , and in the test code here: 
> https://github.com/apache/mesos/blob/1.0.0/src/tests/cluster.cpp#L552:L559 , 
> I see each container created in the test will be cleaned up, and we have 
> already initiated cleanup for those containers in the test code, that might 
> be where the race occurs.
> 
> haosdent huang wrote:
> Thx @qian!
> 
> Jie Yu wrote:
> Calling `destroy` on Containerizer multiple times should not cause 
> `isolator->destroy` being called multiple times. I still don't get it

Hi, @jieyu. I check the code again. We have 

```
  if (container->state == DESTROYING) {
VLOG(1) << "Destroy has already been initiated for '" << containerId << "'";
return;
  }
```

so multiple `MesosContainerizer::destroy` should be safe.

But we still need this for these 2 cases:

1. `MesosContainerizer::prepare` are running serially. Suppose we failed in 
other isolator preparation and have not performed `CgroupsIsolator::prepare`. 
`MesosContainerizer::destroy` would be run in Mesos Agent. And then it would 
run `CgroupsIsolator::destroy` as well.
2. Suppose we start Mesos Agent with `--isolation=cgroups/cpu,cgroups/mem` and 
start some containers. And restart Mesos Agent with a new isolation flag 
`--isolation=cgroups/cpu,cgroups/mem,cgroups/perf_event`. Then when recover 
those exists containers, only `cpu`, `cpuacct`, `memory` subsystems could 
recovered successfully. But when call `CgroupsIsolator::destroy`, we would try 
to cleanup all known subsystems `cpu`, `cpuacct`, `memory`, `perf_event`. This 
is what the test case 
`MesosContainerizerSlaveRecoveryTest.CGROUPS_ROOT_PERF_RollForward` do exactly.


- haosdent


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


On July 31, 2016, 5:47 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 31, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 422b4653fc3cb3d14c94b93ff456625fc59fbb27 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> bd20631a9650cf84e99c6489b2e92bc40ed764ca 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Jie Yu]

> On July 28, 2016, 12:01 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 486
> > 
> >
> > Would love to understand why test will cause this issue.
> 
> Qian Zhang wrote:
> It seems that comment was introduced by this ticket: 
> https://issues.apache.org/jira/browse/MESOS-1438 , and in the test code here: 
> https://github.com/apache/mesos/blob/1.0.0/src/tests/cluster.cpp#L552:L559 , 
> I see each container created in the test will be cleaned up, and we have 
> already initiated cleanup for those containers in the test code, that might 
> be where the race occurs.
> 
> haosdent huang wrote:
> Thx @qian!

Calling `destroy` on Containerizer multiple times should not cause 
`isolator->destroy` being called multiple times. I still don't get it


- Jie


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


On July 31, 2016, 5:47 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 31, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 422b4653fc3cb3d14c94b93ff456625fc59fbb27 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> bd20631a9650cf84e99c6489b2e92bc40ed764ca 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Jie Yu]

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


Ship it!




I made some adjustments while commit.

- Jie Yu


On July 31, 2016, 5:47 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 31, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 422b4653fc3cb3d14c94b93ff456625fc59fbb27 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> bd20631a9650cf84e99c6489b2e92bc40ed764ca 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 31, 2016, 5:47 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
422b4653fc3cb3d14c94b93ff456625fc59fbb27 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
bd20631a9650cf84e99c6489b2e92bc40ed764ca 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On July 31, 2016, 12:24 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 31, 2016, 12:24 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 422b4653fc3cb3d14c94b93ff456625fc59fbb27 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> bd20631a9650cf84e99c6489b2e92bc40ed764ca 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

> On July 28, 2016, 12:01 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 500
> > 
> >
> > I'd prefer we use await() so that we can get the error message for each 
> > subsystem. I feel like we should consistently use await in this file 
> > (instead of collect). Can you follow up with patches to do that?

The follow up patch which replace exist `collect`s is 
https://reviews.apache.org/r/50627/


- haosdent


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


On July 30, 2016, 4:24 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 30, 2016, 4:24 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 422b4653fc3cb3d14c94b93ff456625fc59fbb27 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> bd20631a9650cf84e99c6489b2e92bc40ed764ca 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 30, 2016, 4:24 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
422b4653fc3cb3d14c94b93ff456625fc59fbb27 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
bd20631a9650cf84e99c6489b2e92bc40ed764ca 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 30, 2016, 3:57 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
422b4653fc3cb3d14c94b93ff456625fc59fbb27 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
bd20631a9650cf84e99c6489b2e92bc40ed764ca 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 30, 2016, 3:47 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @jieyu's comments.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
422b4653fc3cb3d14c94b93ff456625fc59fbb27 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
bd20631a9650cf84e99c6489b2e92bc40ed764ca 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

> On July 28, 2016, 12:01 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp, line 486
> > 
> >
> > Would love to understand why test will cause this issue.
> 
> Qian Zhang wrote:
> It seems that comment was introduced by this ticket: 
> https://issues.apache.org/jira/browse/MESOS-1438 , and in the test code here: 
> https://github.com/apache/mesos/blob/1.0.0/src/tests/cluster.cpp#L552:L559 , 
> I see each container created in the test will be cleaned up, and we have 
> already initiated cleanup for those containers in the test code, that might 
> be where the race occurs.

Thx @qian!


- haosdent


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


On July 26, 2016, 5:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 26, 2016, 5:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 348f105f9c3109a02f1dde0649f1b829cb9ddd04 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 30, 2016, 3:06 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
422b4653fc3cb3d14c94b93ff456625fc59fbb27 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
bd20631a9650cf84e99c6489b2e92bc40ed764ca 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 486)


Would love to understand why test will cause this issue.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 496 - 497)


cleanups.push_back(subsystem->cleanup(...));



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 500)


I'd prefer we use await() so that we can get the error message for each 
subsystem. I feel like we should consistently use await in this file (instead 
of collect). Can you follow up with patches to do that?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 533 - 537)


This should be a CHECK?


- Jie Yu


On July 26, 2016, 5:11 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 26, 2016, 5:11 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 348f105f9c3109a02f1dde0649f1b829cb9ddd04 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 26, 2016, 5:11 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
348f105f9c3109a02f1dde0649f1b829cb9ddd04 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 24, 2016, 3:13 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @qianzhang's comments.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 540 - 546)


I do not think we need this `onAny()` call.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 551)


I do not think you need this parameter.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 557)


Ditto.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 560)


Suggest to add a newline before this line.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 570 - 575)


I would suggest to change these code to:
```
  return collect(futures)
.then(defer(PID(this),
::__cleanup,
containerId))
```
And to make the above code work, we also need to change the return value of 
`__cleanup` from `void` to `Future`. You may take a look at the 
following code as a reference:

https://github.com/apache/mesos/blob/1.0.0-rc4/src/slave/containerizer/mesos/isolators/cgroups/net_cls.cpp#L627:L634


- Qian Zhang


On July 22, 2016, 3:11 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 22, 2016, 3:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 21, 2016, 7:11 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 21, 2016, 7:07 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address @qianzhang's comments.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp (lines 134 - 138)


Why are they virtual methods?



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 581 - 585)


I think you need to call `onAny()` rather than `then()`, so these codes 
should be changed to:
```
  return collect(cleanups)
.onAny(defer(
PID(this),
::_cleanup,
containerId,
lambda::_1));
```
You can take a look at the following code as a reference:

https://github.com/apache/mesos/blob/1.0.0-rc2/src/linux/cgroups.cpp#L1648:L1649



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 593)


I think you need to check `!isReady()` rather than `isFailed()`.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 615 - 617)


Can we merge these 3 lines into the code below?
```
.then([]() { return Nothing(); });
```



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 632 - 635)


I think these code should be formated to:
```
return Failure(
"Failed to clean up container '" + stringify(containerId) +
"': " + (future.isFailed() ? future.failure() : "discarded"));
```


- Qian Zhang


On July 15, 2016, 11:33 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 15, 2016, 11:33 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 15, 2016, 3:33 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 15, 2016, 3 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Address comments


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 620)


Add a blank line below.


- haosdent huang


On July 14, 2016, 5:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 14, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.
> 
> 
> Bugs: MESOS-5041
> https://issues.apache.org/jira/browse/MESOS-5041
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> c57baec88437f68886702a40ec8a6a6458546119 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4a9f55bf3b217405bf90943f27a976422877a99e 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 14, 2016, 5:18 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Qian Zhang, and Zhengju Sha.


Changes
---

Rebase.


Bugs: MESOS-5041
https://issues.apache.org/jira/browse/MESOS-5041


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
c57baec88437f68886702a40ec8a6a6458546119 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
4a9f55bf3b217405bf90943f27a976422877a99e 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 614)


Show `containerId` in this log.


- haosdent huang


On July 9, 2016, 10:17 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49827/
> ---
> 
> (Updated July 9, 2016, 10:17 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented `CgroupsIsolatorProcess::cleanup`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49827/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 49827: Implemented `CgroupsIsolatorProcess::cleanup`.

[haosdent huang]

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

(Updated July 9, 2016, 10:17 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Changes
---

Rebase.


Repository: mesos


Description
---

Implemented `CgroupsIsolatorProcess::cleanup`.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang