You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrei Budnik <ab...@mesosphere.com> on 2020/01/07 17:06:34 UTC
Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/
-----------------------------------------------------------
Review request for mesos, Greg Mann and Qian Zhang.
Bugs: MESOS-10076
https://issues.apache.org/jira/browse/MESOS-10076
Repository: mesos
Description
-------
This patch adds support for nested cgroups for nested containers.
Nested cgroups are created only for a nested container with explicitly
enabled `share_cgroups` flag. The cgroups isolator stores info about
nested cgroups in the `infos` class variable, which is used to
determine whether a nested container has its nested cgroup.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
Diff: https://reviews.apache.org/r/71965/diff/1/
Testing
-------
Thanks,
Andrei Budnik
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Март 8, 2020, 2:29 п.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 743-748 (original), 758-762 (patched)
> > <https://reviews.apache.org/r/71965/diff/4/?file=2212806#file2212806line764>
> >
> > I think this is for the case: Agent is started with a cgroup subsystem specified (like `--isolation=cgroups/cpu`) and a default executor is launched to run a task group with `share_cgroups==true`, now agent is restarted with two cgroup subsystems (like `--isolation=cgroups/cpu,cgroups/mem`) and another task group with `share_cgroups==true` is launched by the same default executor. For the nested containers corresponding to the second task group, we should NOT assign their pids for the subsystem `cgroups/mem` since the default executor does not have cgroup created under it.
> >
> > So I think we should not change the log message here.
After introducing the support for nested cgroups, a `cgroup` variable might refer to a nested container's cgroup rather than a root container's cgroup. If a nested cgroup is lost for some reason, we shouldn't claim that a parent cgroup doesn't have the cgroup hierarchy because it can actually exist. In fact, a nested cgroup might be missing by 2 reasons: a) parent cgroups is absent (due to the reason you've described) b) the parent cgroup is here, but a nested cgroup is absent. I decided to update the log message in order to make it more generic.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 13, 2020, 5:19 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 8, 2020, 2:29 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 411 (original), 412 (patched)
> > <https://reviews.apache.org/r/71965/diff/4/?file=2212806#file2212806line412>
> >
> > s/with/from/
>
> Greg Mann wrote:
> I think "with" is actually the preferred language here.
>
> Qian Zhang wrote:
> I think it should be parent container shares its cgroups with nested container, and nested container shares cgroups from parent container, right?
I think the phrase "share with" works well in both cases you mentioned, and generally sounds more natural than "share from".
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
On Feb. 13, 2020, 5:19 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2020, 5:19 p.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Greg Mann <gr...@mesosphere.io>.
> On March 8, 2020, 2:29 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 411 (original), 412 (patched)
> > <https://reviews.apache.org/r/71965/diff/4/?file=2212806#file2212806line412>
> >
> > s/with/from/
I think "with" is actually the preferred language here.
- Greg
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
On Feb. 13, 2020, 5:19 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2020, 5:19 p.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 8, 2020, 10:29 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 411 (original), 412 (patched)
> > <https://reviews.apache.org/r/71965/diff/4/?file=2212806#file2212806line412>
> >
> > s/with/from/
>
> Greg Mann wrote:
> I think "with" is actually the preferred language here.
I think it should be parent container shares its cgroups with nested container, and nested container shares cgroups from parent container, right?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
> On March 8, 2020, 10:29 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 743-748 (original), 758-762 (patched)
> > <https://reviews.apache.org/r/71965/diff/4/?file=2212806#file2212806line764>
> >
> > I think this is for the case: Agent is started with a cgroup subsystem specified (like `--isolation=cgroups/cpu`) and a default executor is launched to run a task group with `share_cgroups==true`, now agent is restarted with two cgroup subsystems (like `--isolation=cgroups/cpu,cgroups/mem`) and another task group with `share_cgroups==true` is launched by the same default executor. For the nested containers corresponding to the second task group, we should NOT assign their pids for the subsystem `cgroups/mem` since the default executor does not have cgroup created under it.
> >
> > So I think we should not change the log message here.
>
> Andrei Budnik wrote:
> After introducing the support for nested cgroups, a `cgroup` variable might refer to a nested container's cgroup rather than a root container's cgroup. If a nested cgroup is lost for some reason, we shouldn't claim that a parent cgroup doesn't have the cgroup hierarchy because it can actually exist. In fact, a nested cgroup might be missing by 2 reasons: a) parent cgroups is absent (due to the reason you've described) b) the parent cgroup is here, but a nested cgroup is absent. I decided to update the log message in order to make it more generic.
> In fact, a nested cgroup might be missing by 2 reasons: a) parent cgroups is absent (due to the reason you've described) b) the parent cgroup is here, but a nested cgroup is absent.
For a) it is correct to log an INFO message just like what we did before, but for b) is it OK just log an INFO message? In this case there must be something wrong, i.e., we can find the `Info` struct for the nested container, but its own cgroup does not exist somehow, we should return an Error in this case, right?
So basically in this method we still need to know if the nested container has `share_cgroup` as true or false, if it is true but its parent container's cgroup does not exist, that is a), we should just log an INFO message exactly like before. And for the other cases (the nested container whose `share_cgroup` as false and the top-level container), we should just go ahead with `cgroups::assign()`, if the cgroup does not exist, `cgroups::assign()` will give us reasonable error.
Maybe we could change
```
if (containerId.has_parent() && !cgroups::exists(hierarchy, info->cgroup)) {
```
to:
```
if (containerId.has_parent() && containerId != info->containerId && !cgroups::exists(hierarchy, info->cgroup)) {
```
In the above way, we know it is a nested container sharing its parent container's cgroups but the parent container's cgroup does not exist for the reason that I described above, so we just log the original INFO message. HDYT?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219836
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 411 (original), 412 (patched)
<https://reviews.apache.org/r/71965/#comment308078>
s/with/from/
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 735 (original)
<https://reviews.apache.org/r/71965/#comment308081>
Why do you remove this? Can we change it to `const string& cgroup = info->cgroup;`?
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 743-748 (original), 758-762 (patched)
<https://reviews.apache.org/r/71965/#comment308082>
I think this is for the case: Agent is started with a cgroup subsystem specified (like `--isolation=cgroups/cpu`) and a default executor is launched to run a task group with `share_cgroups==true`, now agent is restarted with two cgroup subsystems (like `--isolation=cgroups/cpu,cgroups/mem`) and another task group with `share_cgroups==true` is launched by the same default executor. For the nested containers corresponding to the second task group, we should NOT assign their pids for the subsystem `cgroups/mem` since the default executor does not have cgroup created under it.
So I think we should not change the log message here.
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 1058-1072 (patched)
<https://reviews.apache.org/r/71965/#comment308080>
It seems the `while` loop will be an endless loop if the input `containerId` is an ID of an unknown container (because `current.isSome()` is always true).
- Qian Zhang
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219830
-----------------------------------------------------------
In the description: should be "explicitly disabled `share_cgroups` flag"
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 411 (original), 412 (patched)
<https://reviews.apache.org/r/71965/#comment308088>
I think "with" is actually the preferred language here.
- Greg Mann
On Feb. 13, 2020, 5:19 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2020, 5:19 p.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219921
-----------------------------------------------------------
Fix it, then Ship it!
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 741-745 (original), 762-765 (patched)
<https://reviews.apache.org/r/71965/#comment308156>
I think we should not change this message, the original message is more accurate.
- Qian Zhang
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/6/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219843
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp
Line 149 (original), 149 (patched)
<https://reviews.apache.org/r/71965/#comment308090>
I just tried to apply this patch on master and got a conflict, looks like it needs a rebase.
- Greg Mann
On Feb. 13, 2020, 5:19 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 13, 2020, 5:19 p.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219908
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 1069 (patched)
<https://reviews.apache.org/r/71965/#comment308147>
Will we have the issue mentioned here: https://github.com/apache/mesos/blob/1.9.0/src/common/protobuf_utils.cpp#L1058:L1063 ?
- Qian Zhang
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219915
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 415-416 (original), 423-425 (patched)
<https://reviews.apache.org/r/71965/#comment308154>
If there are already a top-level container and a 1st level nested container (`share_cgroup==false`) running, now a 2nd level nested container (`share_cgroup==true`) with resource requests/limits specified is launched, I think here we need to increase the resource limits of the 1st level nested cgroup to ensure it includes the 2nd level nested container's resources. Similarly when that 2nd level nested container terminates, we should decrease the 1st level nested cgroup's limits.
Or in MVP, we can just disallow launching 2nd level nested container with resource requests/limits specified? I.e., if executor wants to launch a 2nd level nested container, that container's `share_cgroups` can only be true and it can not have resource requests/limits specified.
- Qian Zhang
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219908
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 1069 (patched)
<https://reviews.apache.org/r/71965/#comment308147>
Will we have the issue mentioned here: https://github.com/apache/mesos/blob/1.9.0/src/common/protobuf_utils.cpp#L1058:L1063 ?
- Qian Zhang
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/
-----------------------------------------------------------
(Updated Фев. 13, 2020, 5:19 п.п.)
Review request for mesos, Greg Mann and Qian Zhang.
Bugs: MESOS-10076
https://issues.apache.org/jira/browse/MESOS-10076
Repository: mesos
Description
-------
This patch adds support for nested cgroups for nested containers.
Nested cgroups are created only for a nested container with explicitly
enabled `share_cgroups` flag. The cgroups isolator stores info about
nested cgroups in the `infos` class variable, which is used to
determine whether a nested container has its nested cgroup.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
Diff: https://reviews.apache.org/r/71965/diff/3/
Changes: https://reviews.apache.org/r/71965/diff/2-3/
Testing
-------
Thanks,
Andrei Budnik
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/
-----------------------------------------------------------
(Updated Фев. 12, 2020, 3:47 п.п.)
Review request for mesos, Greg Mann and Qian Zhang.
Bugs: MESOS-10076
https://issues.apache.org/jira/browse/MESOS-10076
Repository: mesos
Description
-------
This patch adds support for nested cgroups for nested containers.
Nested cgroups are created only for a nested container with explicitly
enabled `share_cgroups` flag. The cgroups isolator stores info about
nested cgroups in the `infos` class variable, which is used to
determine whether a nested container has its nested cgroup.
Diffs (updated)
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
Diff: https://reviews.apache.org/r/71965/diff/2/
Changes: https://reviews.apache.org/r/71965/diff/1-2/
Testing
-------
Thanks,
Andrei Budnik
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 840-841 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line855>
> >
> > How do we know this is a nested container **with shared cgroups**?
> >
> > And I have the same comment for the methods `usage`, `status` and `cleanup`.
> How do we know this is a nested container with shared cgroups?
`update()` and `usage()` case.
Since there is no way to distinguish between a nested container with shared cgroups and a non-existent container, we should return the error message describing the situation: `Unknown container`. This message tells us that the container is unknown to the cgroups isolator. It doesn't state that this container does not exist, nor it states that this is a nested container with shared cgroups.
`cleanup()` case.
I removed the misleading comment. Also, I made it print VLOG message in any case as it's more accurate.
`status()` case.
If we are a nested container unknown to the isolator, we try to find the status of its ancestor. This preserves the old behavior.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 12, 2020, 3:47 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 12, 2020, 3:47 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 411 (original), 425 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line425>
> >
> > s/root/parent/, see the definition of the field `share_cgroups`: https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3335:L3348 .
I have to update `CgroupsIsolatorProcess::__prepare` method to add a search for the first ancestor who has `share_cgroups=false`. Does it make sense?
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 435 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line435>
> >
> > Here I think we need a special handling for 2nd level nested containers, i.e., for such containers the `share_cgroups` field will be ignored and they should always share cgroups from its parent container, see https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3340:L3343 .
Is this still actual after our discussion?
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 610-614 (original), 634 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line634>
> >
> > Linux launcher creates freezer and systemd cgroups with `mesos` as separator, but here you use `""` as separator, will that be a problem? Or you want to change Linux launcher's code to make it consistent with the code here?
Empty separator shouldn't be a problem.
I'd prefer making it consistent by not using a separator in the Linux launcher, but it seems that it's a non-goal for the vertical bursting feature.
I can file a ticket for that and add a comment here, which refers to the ticket.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
That's a good point!
Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
We can keep `Info` for all existing containers and add `share_cgroup` field to it.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Янв. 9, 2020, 9:03 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Янв. 9, 2020, 9:03 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
>
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` field to it.
>
> Andrei Budnik wrote:
> On second thought, we won't be able to create `Info` for nested containers with shared cgroups during the recovery. They're invisible to the cgroups isolator.
```
This logic is correct for root container since we will return an Unknown container failure for it if infos does not contain it, but it seems not correct for the nested container whose share_cgroup is false, for such container, if infos does not contain it, we will always return a pending future but never return the unknown container failure, that might hide some errors in our code.
```
The previous version was prone to the problem you have described above: we could return a pending future for a non-existent nested container. This code change neither introduces new problems nor fixes the existing problem.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 12, 2020, 3:47 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 12, 2020, 3:47 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 435 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line435>
> >
> > Here I think we need a special handling for 2nd level nested containers, i.e., for such containers the `share_cgroups` field will be ignored and they should always share cgroups from its parent container, see https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3340:L3343 .
>
> Andrei Budnik wrote:
> Is this still actual after our discussion?
>
> Qian Zhang wrote:
> Instead of ignoring debug containers, I think here we should add a validation to ensure that debug containers do not set share_cgroups=false, and ignore debug containers when we do the validation that the value of share_cgroups for all the nested containers under a root container must be same.
>
> Andrei Budnik wrote:
> Added a check for `DEBUG` containers in `CgroupsIsolatorProcess::prepare`.
Could you please post a patch to update the comments of the `share_cgroups` field (https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3342:L3348) according to our latest discussion?
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
>
> Andrei Budnik wrote:
> Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
>
> Qian Zhang wrote:
> Actually we are not able to disallow that, it is up to the task itself, if the task wants to create a child cgroup under its own cgroup, then I think it can just do it. But yes I agree that may bring trouble to recovery, maybe we should use a `mesos` separator so that the cgroups created by the tasks will not affect our recovery, how do you think?
>
> Andrei Budnik wrote:
> After enabling `mesos` separator between cgroups, launching a nested container with `share_cgroup=false` fails with "No space left on device" error. The root cause of this failure is described in the comment to our function `cloneCpusetCpusMems` and in this SO article: https://stackoverflow.com/questions/28348627/echo-tasks-gives-no-space-left-on-device-when-trying-to-use-cpuset
>
>
> `CgroupsIsolatorProcess::prepare()` calls `cgroups::create()` which calls `cloneCpusetCpusMems()` function.
>
>
> ```
> // Copies the value of 'cpuset.cpus' and 'cpuset.mems' from a parent
> // cgroup to a child cgroup so the child cgroup can actually run tasks
> // (otherwise it gets the error 'Device or resource busy').
> ...
> static Try<Nothing> cloneCpusetCpusMems(
> ```
>
>
> When we call `cgroups::create()` for a nested cgroup, `cloneCpusetCpusMems()` tries to copy those “cpuset” files from a parent container that has a suffix `<path to parent container>/mesos` to a child container with suffix `<path to parent container>/mesos/childContainerId`. Since those "cpuset" files are not filled/initialized for the parent that ends with `/mesos` suffix, `cloneCpusetCpusMems()` copies those empty files to the child cgroup of `/mesos/childContainerId`. Therefore, the cgroups isolator fails to assign a PID to the cgroup for the child container during `isolate()` for the `cpuset` cgroup (for the reason described in the beggining of this comment).
>
> I'm not sure about the best way to fix/workaround this issue. I decided to get rid of a separator between cgroups. AFAIK, k8s does not use any separator between containers within the POD.
>
> Do you happen to know any better approach to the problem described above?
>
> Qian Zhang wrote:
> Good to know!
>
> It seems a bug of `cgroups::create()` to me, I mean if `cgroups::create()` is used to create a cgroup recursively (like: `mesos/childContainerId`), it should clone `cpuset.cpus` and `cpuset.mems` recursively too (both `mesos` and `childContainerId`) rather than just clone them for the leaf cgroup (`childContainerId`), right?
>
> If we do not want to change the code of `cgroups::create()`, then a workaround could be calling `cgroups::create()` non-recursively, i.e., call this function twice, one for `mesos` and another for `mesos/childContainerId`. But I would suggest to fix `cgroups::create()` if possible.
>
> Andrei Budnik wrote:
> Updated the comment.
Thanks! And I see you have posted a patch to fix (https://reviews.apache.org/r/72122) `cgroups::create()`.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
>
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` field to it.
>
> Andrei Budnik wrote:
> On second thought, we won't be able to create `Info` for nested containers with shared cgroups during the recovery. They're invisible to the cgroups isolator.
>
> Andrei Budnik wrote:
> ```
> This logic is correct for root container since we will return an Unknown container failure for it if infos does not contain it, but it seems not correct for the nested container whose share_cgroup is false, for such container, if infos does not contain it, we will always return a pending future but never return the unknown container failure, that might hide some errors in our code.
> ```
>
> The previous version was prone to the problem you have described above: we could return a pending future for a non-existent nested container. This code change neither introduces new problems nor fixes the existing problem.
Could you please add a `TODO` in this code to mention that ideally we should return an unknown container failure for the nested container whose `share_cgroups` is false but `infos` does not contain it?
Or how about we add a new parameter `const ContainerConfig& containerConfig` to the `watch()` method of the isolator interface, and with such parameter in `watch()` method we will clearly know if the container's `share_cgroups` is true or false, if it is false and the container does not exist in `infos`, we can just return an unknown container failure.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 840-841 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line855>
> >
> > How do we know this is a nested container **with shared cgroups**?
> >
> > And I have the same comment for the methods `usage`, `status` and `cleanup`.
>
> Andrei Budnik wrote:
> > How do we know this is a nested container with shared cgroups?
>
> `update()` and `usage()` case.
> Since there is no way to distinguish between a nested container with shared cgroups and a non-existent container, we should return the error message describing the situation: `Unknown container`. This message tells us that the container is unknown to the cgroups isolator. It doesn't state that this container does not exist, nor it states that this is a nested container with shared cgroups.
>
> `cleanup()` case.
> I removed the misleading comment. Also, I made it print VLOG message in any case as it's more accurate.
>
> `status()` case.
> If we are a nested container unknown to the isolator, we try to find the status of its ancestor. This preserves the old behavior.
> status() case.
> If we are a nested container unknown to the isolator, we try to find the status of its ancestor. This preserves the old behavior.
I think here we have the similar issue with `watch()`: For the nested container whose `share_cgroups` is true but somehow it does not exist in `infos`, ideally we should return an unknown container failure, but not just return its parent container's status which means we may return wrong status for a nested container.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Feb. 14, 2020, 1:19 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Feb. 14, 2020, 1:19 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
>
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` field to it.
On second thought, we won't be able to create `Info` for nested containers with shared cgroups during the recovery. They're invisible to the cgroups isolator.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Янв. 9, 2020, 9:03 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Янв. 9, 2020, 9:03 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 435 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line435>
> >
> > Here I think we need a special handling for 2nd level nested containers, i.e., for such containers the `share_cgroups` field will be ignored and they should always share cgroups from its parent container, see https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3340:L3343 .
>
> Andrei Budnik wrote:
> Is this still actual after our discussion?
>
> Qian Zhang wrote:
> Instead of ignoring debug containers, I think here we should add a validation to ensure that debug containers do not set share_cgroups=false, and ignore debug containers when we do the validation that the value of share_cgroups for all the nested containers under a root container must be same.
Added a check for `DEBUG` containers in `CgroupsIsolatorProcess::prepare`.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 13, 2020, 5:19 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/4/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
>
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` field to it.
>
> Andrei Budnik wrote:
> On second thought, we won't be able to create `Info` for nested containers with shared cgroups during the recovery. They're invisible to the cgroups isolator.
>
> Andrei Budnik wrote:
> ```
> This logic is correct for root container since we will return an Unknown container failure for it if infos does not contain it, but it seems not correct for the nested container whose share_cgroup is false, for such container, if infos does not contain it, we will always return a pending future but never return the unknown container failure, that might hide some errors in our code.
> ```
>
> The previous version was prone to the problem you have described above: we could return a pending future for a non-existent nested container. This code change neither introduces new problems nor fixes the existing problem.
>
> Qian Zhang wrote:
> Could you please add a `TODO` in this code to mention that ideally we should return an unknown container failure for the nested container whose `share_cgroups` is false but `infos` does not contain it?
>
> Or how about we add a new parameter `const ContainerConfig& containerConfig` to the `watch()` method of the isolator interface, and with such parameter in `watch()` method we will clearly know if the container's `share_cgroups` is true or false, if it is false and the container does not exist in `infos`, we can just return an unknown container failure.
If a container shares cgroups with its parent, then the cgroups isolator should not be in charge of this container. So, returning "Unknown" seems totally fine for me, and I don't think we need a comment either.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 840-841 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line855>
> >
> > How do we know this is a nested container **with shared cgroups**?
> >
> > And I have the same comment for the methods `usage`, `status` and `cleanup`.
>
> Andrei Budnik wrote:
> > How do we know this is a nested container with shared cgroups?
>
> `update()` and `usage()` case.
> Since there is no way to distinguish between a nested container with shared cgroups and a non-existent container, we should return the error message describing the situation: `Unknown container`. This message tells us that the container is unknown to the cgroups isolator. It doesn't state that this container does not exist, nor it states that this is a nested container with shared cgroups.
>
> `cleanup()` case.
> I removed the misleading comment. Also, I made it print VLOG message in any case as it's more accurate.
>
> `status()` case.
> If we are a nested container unknown to the isolator, we try to find the status of its ancestor. This preserves the old behavior.
>
> Qian Zhang wrote:
> > status() case.
> > If we are a nested container unknown to the isolator, we try to find the status of its ancestor. This preserves the old behavior.
>
> I think here we have the similar issue with `watch()`: For the nested container whose `share_cgroups` is true but somehow it does not exist in `infos`, ideally we should return an unknown container failure, but not just return its parent container's status which means we may return wrong status for a nested container.
Previously, we could return the status of a parent container for a nested container that had never been launched. Now, we're dealing with the following alternatives for a nested container: 1) it's unknown 2) it shares cgroups with its parent. Since we can't resolve this ambiguity, we can either a) return a failure, or b) return a status of a parent container. I decided to choose option (b) as it preserves the old behaviour. Do you think we should change it?
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 13, 2020, 5:19 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
>
> Andrei Budnik wrote:
> Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
>
> Qian Zhang wrote:
> Actually we are not able to disallow that, it is up to the task itself, if the task wants to create a child cgroup under its own cgroup, then I think it can just do it. But yes I agree that may bring trouble to recovery, maybe we should use a `mesos` separator so that the cgroups created by the tasks will not affect our recovery, how do you think?
>
> Andrei Budnik wrote:
> After enabling `mesos` separator between cgroups, launching a nested container with `share_cgroup=false` fails with "No space left on device" error. The root cause of this failure is described in the comment to our function `cloneCpusetCpusMems` and in this SO article: https://stackoverflow.com/questions/28348627/echo-tasks-gives-no-space-left-on-device-when-trying-to-use-cpuset
>
>
> `CgroupsIsolatorProcess::prepare()` calls `cgroups::create()` which calls `cloneCpusetCpusMems()` function.
>
>
> ```
> // Copies the value of 'cpuset.cpus' and 'cpuset.mems' from a parent
> // cgroup to a child cgroup so the child cgroup can actually run tasks
> // (otherwise it gets the error 'Device or resource busy').
> ...
> static Try<Nothing> cloneCpusetCpusMems(
> ```
>
>
> When we call `cgroups::create()` for a nested cgroup, `cloneCpusetCpusMems()` tries to copy those “cpuset” files from a parent container that has a suffix `<path to parent container>/mesos` to a child container with suffix `<path to parent container>/mesos/childContainerId`. Since those "cpuset" files are not filled/initialized for the parent that ends with `/mesos` suffix, `cloneCpusetCpusMems()` copies those empty files to the child cgroup of `/mesos/childContainerId`. Therefore, the cgroups isolator fails to assign a PID to the cgroup for the child container during `isolate()` for the `cpuset` cgroup (for the reason described in the beggining of this comment).
>
> I'm not sure about the best way to fix/workaround this issue. I decided to get rid of a separator between cgroups. AFAIK, k8s does not use any separator between containers within the POD.
>
> Do you happen to know any better approach to the problem described above?
Good to know!
It seems a bug of `cgroups::create()` to me, I mean if `cgroups::create()` is used to create a cgroup recursively (like: `mesos/childContainerId`), it should clone `cpuset.cpus` and `cpuset.mems` recursively too (both `mesos` and `childContainerId`) rather than just clone them for the leaf cgroup (`childContainerId`), right?
If we do not want to change the code of `cgroups::create()`, then a workaround could be calling `cgroups::create()` non-recursively, i.e., call this function twice, one for `mesos` and another for `mesos/childContainerId`. But I would suggest to fix `cgroups::create()` if possible.
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Jan. 10, 2020, 5:03 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 5:03 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
>
> Andrei Budnik wrote:
> Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
>
> Qian Zhang wrote:
> Actually we are not able to disallow that, it is up to the task itself, if the task wants to create a child cgroup under its own cgroup, then I think it can just do it. But yes I agree that may bring trouble to recovery, maybe we should use a `mesos` separator so that the cgroups created by the tasks will not affect our recovery, how do you think?
After enabling `mesos` separator between cgroups, launching a nested container with `share_cgroup=false` fails with "No space left on device" error. The root cause of this failure is described in the comment to our function `cloneCpusetCpusMems` and in this SO article: https://stackoverflow.com/questions/28348627/echo-tasks-gives-no-space-left-on-device-when-trying-to-use-cpuset
`CgroupsIsolatorProcess::prepare()` calls `cgroups::create()` which calls `cloneCpusetCpusMems()` function.
```
// Copies the value of 'cpuset.cpus' and 'cpuset.mems' from a parent
// cgroup to a child cgroup so the child cgroup can actually run tasks
// (otherwise it gets the error 'Device or resource busy').
...
static Try<Nothing> cloneCpusetCpusMems(
```
When we call `cgroups::create()` for a nested cgroup, `cloneCpusetCpusMems()` tries to copy those “cpuset” files from a parent container that has a suffix `<path to parent container>/mesos` to a child container with suffix `<path to parent container>/mesos/childContainerId`. Since those "cpuset" files are not filled/initialized for the parent that ends with `/mesos` suffix, `cloneCpusetCpusMems()` copies those empty files to the child cgroup of `/mesos/childContainerId`. Therefore, the cgroups isolator fails to assign a PID to the cgroup for the child container during `isolate()` for the `cpuset` cgroup (for the reason described in the beggining of this comment).
I'm not sure about the best way to fix/workaround this issue. I decided to get rid of a separator between cgroups. AFAIK, k8s does not use any separator between containers within the POD.
Do you happen to know any better approach to the problem described above?
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Янв. 9, 2020, 9:03 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Янв. 9, 2020, 9:03 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 779-785 (original), 797-803 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line806>
> >
> > This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
>
> Andrei Budnik wrote:
> That's a good point!
> Unfortunately, `share_cgroup` flag is unknown for terminated containers. A terminated nested container with `share_cgroup=false` is indistinguishable from a running container with `share_cgroup=true`, because they both are not presented in the `infos`.
> We can keep `Info` for all existing containers and add `share_cgroup` field to it.
>
> Andrei Budnik wrote:
> On second thought, we won't be able to create `Info` for nested containers with shared cgroups during the recovery. They're invisible to the cgroups isolator.
>
> Andrei Budnik wrote:
> ```
> This logic is correct for root container since we will return an Unknown container failure for it if infos does not contain it, but it seems not correct for the nested container whose share_cgroup is false, for such container, if infos does not contain it, we will always return a pending future but never return the unknown container failure, that might hide some errors in our code.
> ```
>
> The previous version was prone to the problem you have described above: we could return a pending future for a non-existent nested container. This code change neither introduces new problems nor fixes the existing problem.
>
> Qian Zhang wrote:
> Could you please add a `TODO` in this code to mention that ideally we should return an unknown container failure for the nested container whose `share_cgroups` is false but `infos` does not contain it?
>
> Or how about we add a new parameter `const ContainerConfig& containerConfig` to the `watch()` method of the isolator interface, and with such parameter in `watch()` method we will clearly know if the container's `share_cgroups` is true or false, if it is false and the container does not exist in `infos`, we can just return an unknown container failure.
>
> Andrei Budnik wrote:
> If a container shares cgroups with its parent, then the cgroups isolator should not be in charge of this container. So, returning "Unknown" seems totally fine for me, and I don't think we need a comment either.
> ideally we should return an unknown container failure for the nested container whose share_cgroups is false but infos does not contain it
I think the only reason for this to happen is a bug in our code. If this happens, then many other methods will be affected as well. For instance, `usage()` will return a failure if a running nested container with `share_cgroups=false` is unknown to the cgroups isolator. So I'm not sure if we should detect or think about the potential bug in this particular method.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 13, 2020, 5:19 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> disabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/5/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 411 (original), 425 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line425>
> >
> > s/root/parent/, see the definition of the field `share_cgroups`: https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3335:L3348 .
>
> Andrei Budnik wrote:
> I have to update `CgroupsIsolatorProcess::__prepare` method to add a search for the first ancestor who has `share_cgroups=false`. Does it make sense?
Yes. Actually we did the similar thing for nested container's shared memory: if a nested container wants to share its parent container's shared memory, we will search for the first ancenstor who has private shared memory, see: https://github.com/apache/mesos/blob/1.9.0/src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp#L213:L215 .
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 435 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line435>
> >
> > Here I think we need a special handling for 2nd level nested containers, i.e., for such containers the `share_cgroups` field will be ignored and they should always share cgroups from its parent container, see https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3340:L3343 .
>
> Andrei Budnik wrote:
> Is this still actual after our discussion?
Instead of ignoring debug containers, I think here we should add a validation to ensure that debug containers do not set share_cgroups=false, and ignore debug containers when we do the validation that the value of share_cgroups for all the nested containers under a root container must be same.
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
>
> Andrei Budnik wrote:
> Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
Actually we are not able to disallow that, it is up to the task itself, if the task wants to create a child cgroup under its own cgroup, then I think it can just do it. But yes I agree that may bring trouble to recovery, maybe we should use a `mesos` separator so that the cgroups created by the tasks will not affect our recovery, how do you think?
> On Jan. 13, 2020, 11:21 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 610-614 (original), 634 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line634>
> >
> > Linux launcher creates freezer and systemd cgroups with `mesos` as separator, but here you use `""` as separator, will that be a problem? Or you want to change Linux launcher's code to make it consistent with the code here?
>
> Andrei Budnik wrote:
> Empty separator shouldn't be a problem.
> I'd prefer making it consistent by not using a separator in the Linux launcher, but it seems that it's a non-goal for the vertical bursting feature.
>
> I can file a ticket for that and add a comment here, which refers to the ticket.
> Empty separator shouldn't be a problem.
Can you please elaborate why? Here we try to mount the freezer and systemd cgroups created by Linux launcher into container's mount namespace, Linux launches uses `mesos` separator, but here you do not use it, so won't the mount fail due to cannot find source of the mount?
- Qian
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Jan. 10, 2020, 5:03 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 5:03 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Line 459 (original), 480 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line480>
> >
> > I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
>
> Andrei Budnik wrote:
> Do we allow creating cgroups under nested cgroups of those nested containers? Since we do not use a `mesos` separator, the cgroups isolator may pick up those underlying cgroups during recovery, which is not what expected.
>
> Qian Zhang wrote:
> Actually we are not able to disallow that, it is up to the task itself, if the task wants to create a child cgroup under its own cgroup, then I think it can just do it. But yes I agree that may bring trouble to recovery, maybe we should use a `mesos` separator so that the cgroups created by the tasks will not affect our recovery, how do you think?
>
> Andrei Budnik wrote:
> After enabling `mesos` separator between cgroups, launching a nested container with `share_cgroup=false` fails with "No space left on device" error. The root cause of this failure is described in the comment to our function `cloneCpusetCpusMems` and in this SO article: https://stackoverflow.com/questions/28348627/echo-tasks-gives-no-space-left-on-device-when-trying-to-use-cpuset
>
>
> `CgroupsIsolatorProcess::prepare()` calls `cgroups::create()` which calls `cloneCpusetCpusMems()` function.
>
>
> ```
> // Copies the value of 'cpuset.cpus' and 'cpuset.mems' from a parent
> // cgroup to a child cgroup so the child cgroup can actually run tasks
> // (otherwise it gets the error 'Device or resource busy').
> ...
> static Try<Nothing> cloneCpusetCpusMems(
> ```
>
>
> When we call `cgroups::create()` for a nested cgroup, `cloneCpusetCpusMems()` tries to copy those “cpuset” files from a parent container that has a suffix `<path to parent container>/mesos` to a child container with suffix `<path to parent container>/mesos/childContainerId`. Since those "cpuset" files are not filled/initialized for the parent that ends with `/mesos` suffix, `cloneCpusetCpusMems()` copies those empty files to the child cgroup of `/mesos/childContainerId`. Therefore, the cgroups isolator fails to assign a PID to the cgroup for the child container during `isolate()` for the `cpuset` cgroup (for the reason described in the beggining of this comment).
>
> I'm not sure about the best way to fix/workaround this issue. I decided to get rid of a separator between cgroups. AFAIK, k8s does not use any separator between containers within the POD.
>
> Do you happen to know any better approach to the problem described above?
>
> Qian Zhang wrote:
> Good to know!
>
> It seems a bug of `cgroups::create()` to me, I mean if `cgroups::create()` is used to create a cgroup recursively (like: `mesos/childContainerId`), it should clone `cpuset.cpus` and `cpuset.mems` recursively too (both `mesos` and `childContainerId`) rather than just clone them for the leaf cgroup (`childContainerId`), right?
>
> If we do not want to change the code of `cgroups::create()`, then a workaround could be calling `cgroups::create()` non-recursively, i.e., call this function twice, one for `mesos` and another for `mesos/childContainerId`. But I would suggest to fix `cgroups::create()` if possible.
Updated the comment.
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
On Фев. 12, 2020, 3:47 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 12, 2020, 3:47 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/2/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219227
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 351 (patched)
<https://reviews.apache.org/r/71965/#comment307385>
I think we can directly call `cgroup()` instead of `CgroupsIsolatorProcess::cgroup()` here, right? Same to the other places where we call this method.
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 411 (original), 425 (patched)
<https://reviews.apache.org/r/71965/#comment307387>
s/root/parent/, see the definition of the field `share_cgroups`: https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3335:L3348 .
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 434-435 (patched)
<https://reviews.apache.org/r/71965/#comment307386>
A newline between.
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 435 (patched)
<https://reviews.apache.org/r/71965/#comment307388>
Here I think we need a special handling for 2nd level nested containers, i.e., for such containers the `share_cgroups` field will be ignored and they should always share cgroups from its parent container, see https://github.com/apache/mesos/blob/master/include/mesos/v1/mesos.proto#L3340:L3343 .
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 459 (original), 480 (patched)
<https://reviews.apache.org/r/71965/#comment307394>
I think now we need to do this for nested containers whose `share_cgroups` is false, because they may need to create cgroups under their own nested cgroups.
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 610-614 (original), 634 (patched)
<https://reviews.apache.org/r/71965/#comment307389>
Linux launcher creates freezer and systemd cgroups with `mesos` as separator, but here you use `""` as separator, will that be a problem? Or you want to change Linux launcher's code to make it consistent with the code here?
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 779-785 (original), 797-803 (patched)
<https://reviews.apache.org/r/71965/#comment307390>
This logic is correct for root container since we will return an `Unknown container` failure for it if `infos` does not contain it, but it seems not correct for the nested container whose `share_cgroup` is false, for such container, if `infos` does not contain it, we will always return a pending future but never return the `unknown container` failure, that might hide some errors in our code. I think we should also return an `Unknown container` failure for the nested container whose `share_cgroup` is false and somehow `infos` does not contain it. HDYT?
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 840-841 (patched)
<https://reviews.apache.org/r/71965/#comment307391>
How do we know this is a nested container **with shared cgroups**?
And I have the same comment for the methods `usage`, `status` and `cleanup`.
- Qian Zhang
On Jan. 10, 2020, 5:03 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 5:03 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
> On Янв. 13, 2020, 6:49 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 189 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line189>
> >
> > So we changed our cgroups layout from flat to nested, I am wondering if we can work correctly after upgrade (a mixed env with old and new layout), could you please write a unit test to verify it?
Please see these 2 patches:
https://reviews.apache.org/r/72189/
https://reviews.apache.org/r/72190/
- Andrei
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219231
-----------------------------------------------------------
On Фев. 13, 2020, 5:19 п.п., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Фев. 13, 2020, 5:19 п.п.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/3/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219231
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 189 (patched)
<https://reviews.apache.org/r/71965/#comment307398>
So we changed our cgroups layout from flat to nested, I am wondering if we can work correctly after upgrade (a mixed env with old and new layout), could you please write a unit test to verify it?
- Qian Zhang
On Jan. 10, 2020, 5:03 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 5:03 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Greg Mann <gr...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219217
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 414 (original), 429 (patched)
<https://reviews.apache.org/r/71965/#comment307369>
s/share_cgroups/shareCgroups/
- Greg Mann
On Jan. 9, 2020, 9:03 p.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 9, 2020, 9:03 p.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Qian Zhang <zh...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/#review219339
-----------------------------------------------------------
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Lines 607-609 (patched)
<https://reviews.apache.org/r/71965/#comment307484>
I do not think we should use `rootContainerId` here, instead we should use the first ancestor who has share_cgroups=false.
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
Line 735 (original), 752-754 (patched)
<https://reviews.apache.org/r/71965/#comment307485>
Ditto.
- Qian Zhang
On Jan. 10, 2020, 5:03 a.m., Andrei Budnik wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/71965/
> -----------------------------------------------------------
>
> (Updated Jan. 10, 2020, 5:03 a.m.)
>
>
> Review request for mesos, Greg Mann and Qian Zhang.
>
>
> Bugs: MESOS-10076
> https://issues.apache.org/jira/browse/MESOS-10076
>
>
> Repository: mesos
>
>
> Description
> -------
>
> This patch adds support for nested cgroups for nested containers.
> Nested cgroups are created only for a nested container with explicitly
> enabled `share_cgroups` flag. The cgroups isolator stores info about
> nested cgroups in the `infos` class variable, which is used to
> determine whether a nested container has its nested cgroup.
>
>
> Diffs
> -----
>
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
> src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
>
>
> Diff: https://reviews.apache.org/r/71965/diff/1/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Andrei Budnik
>
>
Re: Review Request 71965: Cgroups isolator: added support for nested
cgroups during launch.
Posted by Andrei Budnik <ab...@mesosphere.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/71965/
-----------------------------------------------------------
(Updated Jan. 9, 2020, 9:03 p.m.)
Review request for mesos, Greg Mann and Qian Zhang.
Bugs: MESOS-10076
https://issues.apache.org/jira/browse/MESOS-10076
Repository: mesos
Description
-------
This patch adds support for nested cgroups for nested containers.
Nested cgroups are created only for a nested container with explicitly
enabled `share_cgroups` flag. The cgroups isolator stores info about
nested cgroups in the `infos` class variable, which is used to
determine whether a nested container has its nested cgroup.
Diffs
-----
src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 4bd3d6dad37dee031660c15e957cc36f63e21fcb
src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp b12b73d8e0161d448075378765e77867521de04e
Diff: https://reviews.apache.org/r/71965/diff/1/
Testing
-------
Thanks,
Andrei Budnik