mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrei Budnik <abud...@mesosphere.com>
Subject Re: Review Request 71965: Cgroups isolator: added support for nested cgroups during launch.
Date Thu, 13 Feb 2020 15:18:50 GMT


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


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