mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexander Rukletsov <ruklet...@gmail.com>
Subject Re: Review Request 68257: Fixed incorrect `mnt` namespace detection of command executor's task.
Date Thu, 09 Aug 2018 09:57:18 GMT


> On Aug. 7, 2018, 4:03 p.m., Vinod Kone wrote:
> > src/slave/containerizer/mesos/utils.cpp
> > Line 102 (original), 105 (patched)
> > <https://reviews.apache.org/r/68257/diff/1/?file=2069850#file2069850line108>
> >
> >     Are we guaranteed that there are no short-lived processes, other than the task
process, at the 2nd level? If not, we will have the same issue right?
> >     
> >     Modulo the above question, the change LGTM.
> 
> Andrei Budnik wrote:
>     There are two types of 2nd-level processes:
>     1) the command executor's task
>     2) the nested container's task
>     
>     E.g. the process tree can be like the following:
>     0. mesos-containerizer (`init`)
>        1. mesos-executor (command executor)
>           2. sleep 1000 (command executor's task)
>        1. mesos-containerizer (`init` of a nested container) <- enters `mnt` namespace
of command executor's task before forking a task
>           2. echo "echo" (nested container's task)
>     
>     Since we skip 1st-level processes whose `mnt` namespace is not the same as `init`
process (PID 1), the algorithm doesn't iterate over their 2nd-level processes. That gives
a gurarantee that we only detect command executor's task, but not the short-lived processes.

Let's make it explicit to the reader. I suggest to:
1. Collect _candidates_ (2nd level children) first.
2. Assert there are >= 1 candidates.
3. Filter all candidates except those whose (`mnt` differs from root's `mnt` && parent's
`mnt` equals to root's `mnt`).
4. Assert there are now 0 or 1 candidates left.
5. Return pid of that single candidate or root's pid if there are none.


- Alexander


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/68257/#review206947
-----------------------------------------------------------


On Aug. 7, 2018, 1:46 p.m., Andrei Budnik wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/68257/
> -----------------------------------------------------------
> 
> (Updated Aug. 7, 2018, 1:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Gilbert Song, Jie Yu, and Kevin Klues.
> 
> 
> Bugs: MESOS-9116
>     https://issues.apache.org/jira/browse/MESOS-9116
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, we were walking the process tree from the container's
> `init` process to find the first process along the way whose `mnt`
> namespace differs from the `init` process. We expected this algorithm
> to always return the PID of the command executor's task. However, if
> someone launches multiple nested containers within the process tree,
> the algorithm might detect the PID of the nested container instead of
> the command executor's task. The detected PID might belong to a
> short-lived container, so the container's process might terminate at
> the moment the containerizer launcher (aka `nanny`) process tries to
> enter its `mnt` namespace. This patch fixes the detection algorithm
> so that it always returns PID of the command executor's task.
> 
> 
> Diffs
> -----
> 
>   src/slave/containerizer/mesos/utils.cpp 30e76d1d91651975033078f5450e45f5f2fd8ba0 
> 
> 
> Diff: https://reviews.apache.org/r/68257/diff/1/
> 
> 
> Testing
> -------
> 
> 1) Internal CI with disabled `ROOT_CGROUPS_LaunchNestedContainerSessionsInParallel` test
(see previous patch).
> 2) Fedora 25: `./src/mesos-tests --gtest_filter=*AgentAPITest.LaunchNestedContainerSessionInParallel*
--gtest_break_on_failure --gtest_repeat=100 --verbose`
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>


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