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 Wed, 11 Mar 2020 18:30:55 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.
> 
> 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
> 
>


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