mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Qian Zhang <zhq527...@gmail.com>
Subject Re: Review Request 71965: Cgroups isolator: added support for nested cgroups during launch.
Date Tue, 21 Jan 2020 07:51:03 GMT


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


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message