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 Mon, 09 Mar 2020 17:48:15 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.

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.


> On Янв. 13, 2020, 3:21 д.п., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp
> > Lines 840-841 (patched)
> > <https://reviews.apache.org/r/71965/diff/1/?file=2199231#file2199231line855>
> >
> >     How do we know this is a nested container **with shared cgroups**?
> >     
> >     And I have the same comment for the methods `usage`, `status` and `cleanup`.
> 
> Andrei Budnik wrote:
>     > How do we know this is a nested container with shared cgroups?
>     
>     `update()` and `usage()` case.
>     Since there is no way to distinguish between a nested container with shared cgroups
and a non-existent container, we should return the error message describing the situation:
`Unknown container`. This message tells us that the container is unknown to the cgroups isolator.
It doesn't state that this container does not exist, nor it states that this is a nested container
with shared cgroups.
>     
>     `cleanup()` case.
>     I removed the misleading comment. Also, I made it print VLOG message in any case
as it's more accurate.
>     
>     `status()` case.
>     If we are a nested container unknown to the isolator, we try to find the status of
its ancestor. This preserves the old behavior.
> 
> Qian Zhang wrote:
>     > status() case.
>     > If we are a nested container unknown to the isolator, we try to find the status
of its ancestor. This preserves the old behavior.
>     
>     I think here we have the similar issue with `watch()`: For the nested container whose
`share_cgroups` is true but somehow it does not exist in `infos`, ideally we should return
an unknown container failure, but not just return its parent container's status which means
we may return wrong status for a nested container.

Previously, we could return the status of a parent container for a nested container that had
never been launched. Now, we're dealing with the following alternatives for a nested container:
1) it's unknown 2) it shares cgroups with its parent. Since we can't resolve this ambiguity,
we can either a) return a failure, or b) return a status of a parent container. I decided
to choose option (b) as it preserves the old behaviour. Do you think we should change it?


- 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