Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 1:09 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 674-677
> > 
> >
> > I see here we call `lambda::bind()`, but in the original `cgroups/mem` 
> > isolator, we call defer():
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L605:L609
> > 
> > That means `MemorySubsystem::oomWaited()` will be executed in a 
> > separate thread. So my concern is there may be some multi-thread race 
> > condition issue for your way, e.g., what if `MemorySubsystem::oomWaited()` 
> > is triggered after the container exits (i.e, we have cleaned it up)? I 
> > think in the original `cgroups/mem` isolator, we have already considered 
> > this issue, please refer to this comment for details:
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L634:L637
> 
> haosdent huang wrote:
> For
> ```
> // It is likely that process exited is executed before this
> // function (e.g.  The kill and OOM events happen at the same
> // time, and the process exit event arrives first.) Therefore, we
> // should not report a fatal error here.
> ```
> It is because eventfd would be triggered when hierarchy destruction. It 
> should be fixed in https://reviews.apache.org/r/46299/
> 
> haosdent huang wrote:
> I try to avoid to make `Subsystem` extends from `Process` before, but I 
> think it should be necessary, let me update here.
> 
> Qian Zhang wrote:
> I think we need to handle this multi-thread issue anyway (even 
> https://reviews.apache.org/r/46299/ is fixed), because when container uses 
> the memory more than the limit, it will be killed (so 
> `CgroupsMemIsolatorProcess::cleanup()` will be called) and at the same time 
> OOM event will happen (so `CgroupsMemIsolatorProcess::oom()` will be called), 
> but the order can not be guaranteed, if `CgroupsMemIsolatorProcess::oom()` is 
> called after `CgroupsMemIsolatorProcess::cleanup()` which will delete `info`, 
> then we will not find `info` by the containerId. I think that's why we have 
> the following code in the `cgroups/mem` isolator.
> ```
>   if (!infos.contains(containerId)) {
> // It is likely that process exited is executed before this
> // function (e.g.  The kill and OOM events happen at the same
> // time, and the process exit event arrives first.) Therefore, we
> // should not report a fatal error here.
> LOG(INFO) << "OOM detected for an already terminated executor";
> return;
>   }
> ```

Yes, need extends from `Process` and use `defer` to avoid threads competition.


- haosdent


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


On June 19, 2016, 10:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

> On June 15, 2016, 4:18 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, line 505
> > 
> >
> > I can see two issues here:
> > 1. `updated` is a field in subsystem level (`MemorySubsystem` class) 
> > rather than container level (`Info` class), that means once we set it to 
> > `true` (e.g., when we update resources for the first container), it will be 
> > `true` for all the subsequent container, I do not think that is what we 
> > expect. Actually what we need is a flag in container level with which we 
> > can know whether it is the first time that we update resources for this 
> > container. I think that's the purpose of 
> > `CgroupsMemIsolatorProcess::Info::pid` in the original `cgroups/mem` 
> > isolator.
> > 2. How to recover this `updated` field when agent is restarted? I think 
> > when agent is restarted, this field will be always set back to `false` 
> > though it may be `true` before agent is restarted. Actually I see the same 
> > issue in the original `cgroups/mem` isolator, in that isolator, I do not 
> > see we recover `CgroupsMemIsolatorProcess::Info::pid` in 
> > `CgroupsMemIsolatorProcess::recover`. So it is a bug in the `cgroups/mem` 
> > isolator?
> > 
> > We probably need to follow the way of the `cgroups/mem` isolator (use 
> > `CgroupsMemIsolatorProcess::Info::pid` as the flag) and recover 
> > `CgroupsMemIsolatorProcess::Info::pid` during recovery (I think we can get 
> > pid from `ContainerState`).
> 
> haosdent huang wrote:
> For first issue, because different containers have different 
> MemorySubsystem objects, so it would not be an issue.

Agree the first one is not an issue, thanks haosdent!


- Qian


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


On June 19, 2016, 6:31 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

> On June 12, 2016, 9:09 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 674-677
> > 
> >
> > I see here we call `lambda::bind()`, but in the original `cgroups/mem` 
> > isolator, we call defer():
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L605:L609
> > 
> > That means `MemorySubsystem::oomWaited()` will be executed in a 
> > separate thread. So my concern is there may be some multi-thread race 
> > condition issue for your way, e.g., what if `MemorySubsystem::oomWaited()` 
> > is triggered after the container exits (i.e, we have cleaned it up)? I 
> > think in the original `cgroups/mem` isolator, we have already considered 
> > this issue, please refer to this comment for details:
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L634:L637
> 
> haosdent huang wrote:
> For
> ```
> // It is likely that process exited is executed before this
> // function (e.g.  The kill and OOM events happen at the same
> // time, and the process exit event arrives first.) Therefore, we
> // should not report a fatal error here.
> ```
> It is because eventfd would be triggered when hierarchy destruction. It 
> should be fixed in https://reviews.apache.org/r/46299/
> 
> haosdent huang wrote:
> I try to avoid to make `Subsystem` extends from `Process` before, but I 
> think it should be necessary, let me update here.

I think we need to handle this multi-thread issue anyway (even 
https://reviews.apache.org/r/46299/ is fixed), because when container uses the 
memory more than the limit, it will be killed (so 
`CgroupsMemIsolatorProcess::cleanup()` will be called) and at the same time OOM 
event will happen (so `CgroupsMemIsolatorProcess::oom()` will be called), but 
the order can not be guaranteed, if `CgroupsMemIsolatorProcess::oom()` is 
called after `CgroupsMemIsolatorProcess::cleanup()` which will delete `info`, 
then we will not find `info` by the containerId. I think that's why we have the 
following code in the `cgroups/mem` isolator.
```
  if (!infos.contains(containerId)) {
// It is likely that process exited is executed before this
// function (e.g.  The kill and OOM events happen at the same
// time, and the process exit event arrives first.) Therefore, we
// should not report a fatal error here.
LOG(INFO) << "OOM detected for an already terminated executor";
return;
  }
```


- Qian


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


On June 19, 2016, 6:31 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

> On June 11, 2016, 2:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 650-656
> > 
> >
> > I see this method is different from the original one: 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L66:L69
> > 
> > Can you please let me know why you want to do this change?
> 
> haosdent huang wrote:
> I think it looks same?
> ```
> static const vector levels()
> {
>   return {Level::LOW, Level::MEDIUM, Level::CRITICAL};
> }
> ```
> I move the scope to `MemorySubsystem` because we put all `XXXSubsystem` 
> in `subsystem.cpp` currently. Define a global `levels()` looks not exactly.
> 
> Qian Zhang wrote:
> Agree to move it into `MemorySubsystem`. But inside of `levels()` method, 
> do we really need to define the variable `levels` as static? What about just 
> `return {Level::LOW, Level::MEDIUM, Level::CRITICAL};` as before?

Oh, I got your idea. Could return directly.


- haosdent


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


On June 19, 2016, 10:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

> On June 11, 2016, 10:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 650-656
> > 
> >
> > I see this method is different from the original one: 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L66:L69
> > 
> > Can you please let me know why you want to do this change?
> 
> haosdent huang wrote:
> I think it looks same?
> ```
> static const vector levels()
> {
>   return {Level::LOW, Level::MEDIUM, Level::CRITICAL};
> }
> ```
> I move the scope to `MemorySubsystem` because we put all `XXXSubsystem` 
> in `subsystem.cpp` currently. Define a global `levels()` looks not exactly.

Agree to move it into `MemorySubsystem`. But inside of `levels()` method, do we 
really need to define the variable `levels` as static? What about just `return 
{Level::LOW, Level::MEDIUM, Level::CRITICAL};` as before?


- Qian


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


On June 19, 2016, 6:31 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 6:31 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

> On June 12, 2016, 1:09 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 674-677
> > 
> >
> > I see here we call `lambda::bind()`, but in the original `cgroups/mem` 
> > isolator, we call defer():
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L605:L609
> > 
> > That means `MemorySubsystem::oomWaited()` will be executed in a 
> > separate thread. So my concern is there may be some multi-thread race 
> > condition issue for your way, e.g., what if `MemorySubsystem::oomWaited()` 
> > is triggered after the container exits (i.e, we have cleaned it up)? I 
> > think in the original `cgroups/mem` isolator, we have already considered 
> > this issue, please refer to this comment for details:
> > 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L634:L637

For
```
// It is likely that process exited is executed before this
// function (e.g.  The kill and OOM events happen at the same
// time, and the process exit event arrives first.) Therefore, we
// should not report a fatal error here.
```
It is because eventfd would be triggered when hierarchy destruction. It should 
be fixed in https://reviews.apache.org/r/46299/


- haosdent


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


On June 19, 2016, 10:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

> On June 15, 2016, 8:18 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, line 505
> > 
> >
> > I can see two issues here:
> > 1. `updated` is a field in subsystem level (`MemorySubsystem` class) 
> > rather than container level (`Info` class), that means once we set it to 
> > `true` (e.g., when we update resources for the first container), it will be 
> > `true` for all the subsequent container, I do not think that is what we 
> > expect. Actually what we need is a flag in container level with which we 
> > can know whether it is the first time that we update resources for this 
> > container. I think that's the purpose of 
> > `CgroupsMemIsolatorProcess::Info::pid` in the original `cgroups/mem` 
> > isolator.
> > 2. How to recover this `updated` field when agent is restarted? I think 
> > when agent is restarted, this field will be always set back to `false` 
> > though it may be `true` before agent is restarted. Actually I see the same 
> > issue in the original `cgroups/mem` isolator, in that isolator, I do not 
> > see we recover `CgroupsMemIsolatorProcess::Info::pid` in 
> > `CgroupsMemIsolatorProcess::recover`. So it is a bug in the `cgroups/mem` 
> > isolator?
> > 
> > We probably need to follow the way of the `cgroups/mem` isolator (use 
> > `CgroupsMemIsolatorProcess::Info::pid` as the flag) and recover 
> > `CgroupsMemIsolatorProcess::Info::pid` during recovery (I think we can get 
> > pid from `ContainerState`).

For first issue, because different containers have different MemorySubsystem 
objects, so it would not be an issue.


- haosdent


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


On June 19, 2016, 10:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

> On June 11, 2016, 2:12 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp, lines 650-656
> > 
> >
> > I see this method is different from the original one: 
> > https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L66:L69
> > 
> > Can you please let me know why you want to do this change?

I think it looks same?
```
static const vector levels()
{
  return {Level::LOW, Level::MEDIUM, Level::CRITICAL};
}
```
I move the scope to `MemorySubsystem` because we put all `XXXSubsystem` in 
`subsystem.cpp` currently. Define a global `levels()` looks not exactly.


- haosdent


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


On June 19, 2016, 10:31 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated June 19, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, 
> Kevin Klues, and Qian Zhang.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated June 19, 2016, 10:31 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, Kevin 
Klues, and Qian Zhang.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

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




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


I can see two issues here:
1. `updated` is a field in subsystem level (`MemorySubsystem` class) rather 
than container level (`Info` class), that means once we set it to `true` (e.g., 
when we update resources for the first container), it will be `true` for all 
the subsequent container, I do not think that is what we expect. Actually what 
we need is a flag in container level with which we can know whether it is the 
first time that we update resources for this container. I think that's the 
purpose of `CgroupsMemIsolatorProcess::Info::pid` in the original `cgroups/mem` 
isolator.
2. How to recover this `updated` field when agent is restarted? I think 
when agent is restarted, this field will be always set back to `false` though 
it may be `true` before agent is restarted. Actually I see the same issue in 
the original `cgroups/mem` isolator, in that isolator, I do not see we recover 
`CgroupsMemIsolatorProcess::Info::pid` in `CgroupsMemIsolatorProcess::recover`. 
So it is a bug in the `cgroups/mem` isolator?

We probably need to follow the way of the `cgroups/mem` isolator (use 
`CgroupsMemIsolatorProcess::Info::pid` as the flag) and recover 
`CgroupsMemIsolatorProcess::Info::pid` during recovery (I think we can get pid 
from `ContainerState`).


- Qian Zhang


On April 16, 2016, 6:22 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated April 16, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp (lines 674 - 677)


I see here we call `lambda::bind()`, but in the original `cgroups/mem` 
isolator, we call defer():

https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L605:L609

That means `MemorySubsystem::oomWaited()` will be executed in a separate 
thread. So my concern is there may be some multi-thread race condition issue 
for your way, e.g., what if `MemorySubsystem::oomWaited()` is triggered after 
the container exits (i.e, we have cleaned it up)? I think in the original 
`cgroups/mem` isolator, we have already considered this issue, please refer to 
this comment for details:

https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L634:L637


- Qian Zhang


On April 16, 2016, 6:22 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated April 16, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[Qian Zhang]

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




src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp (lines 650 - 656)


I see this method is different from the original one: 
https://github.com/apache/mesos/blob/0.28.2/src/slave/containerizer/mesos/isolators/cgroups/mem.cpp#L66:L69

Can you please let me know why you want to do this change?


- Qian Zhang


On April 16, 2016, 6:22 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45363/
> ---
> 
> (Updated April 16, 2016, 6:22 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-5045
> https://issues.apache.org/jira/browse/MESOS-5045
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `MemorySubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45363/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 16, 2016, 10:22 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 16, 2016, 10:16 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 13, 2016, 6:47 p.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 7, 2016, 10:37 a.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 4, 2016, 5:21 p.m.)


Review request for mesos, Gilbert Song, Guangya Liu, Ian Downes, Jie Yu, and 
Kevin Klues.


Changes
---

Rebase and update reviewers.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 1, 2016, 2:08 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated March 30, 2016, 6:26 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


Summary (updated)
-

Add `MemorySubsystem` for cgroups unified isolator.


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


Repository: mesos


Description (updated)
---

Add `MemorySubsystem` for cgroups unified isolator.


Diffs (updated)
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45363: Add `MemorySubsystem ` for cgroups unified isolator.

[haosdent huang]

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

(Updated March 27, 2016, 4:17 p.m.)


Review request for mesos and Jie Yu.


Summary (updated)
-

Add `MemorySubsystem ` for cgroups unified isolator.


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


Repository: mesos


Description (updated)
---

Add `MemorySubsystem ` for cgroups unified isolator.


Diffs
-

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

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


Testing
---


Thanks,

haosdent huang