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 Thu, 12 Mar 2020 04:21:39 GMT


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


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