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