Mailing-List: contact dev-help@mesos.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@mesos.apache.org
Content-Type: multipart/alternative;
 boundary="===============4841732685637016291=="
MIME-Version: 1.0
Subject: Re: Review Request 25865: Pid namespace isolator for the
 MesosContainerizer.
From: "Vinod Kone" <vinodkone@gmail.com>
To: "Vinod Kone" <vinodkone@gmail.com>, "Jie Yu" <yujie.jay@gmail.com>
Cc: "mesos" <dev@mesos.apache.org>, "Ian Downes" <ian.downes@gmail.com>
Date: Fri, 26 Sep 2014 01:17:54 -0000
Message-ID: <20140926011754.15006.8647@reviews.apache.org>
Auto-Submitted: auto-generated
Sender: "Vinod Kone" <noreply@reviews.apache.org>
References: <20140923233927.15006.24902@reviews.apache.org>
In-Reply-To: <20140923233927.15006.24902@reviews.apache.org>
Reply-To: "Vinod Kone" <vinodkone@gmail.com>

--===============4841732685637016291==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit


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


src/slave/containerizer/isolators/namespaces/pid.hpp
<https://reviews.apache.org/r/25865/#comment94869>

    s/NamespacesPid/PidNamespace/ ?


src/slave/containerizer/isolators/namespaces/pid.hpp
<https://reviews.apache.org/r/25865/#comment94870>

    kill new line.


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94871>

    Comment?


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94873>

    As mentioned in the previous review, instead of requiring users/operators to know this dependency, we should just automatically use fileystem/shared isoator when using pid or network isolation.


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94877>

    Who is calling this method?


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94882>

    // Cleanup orphans. ?


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94880>

    if you use hashset, you can just do !containerers.contain().


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94883>

    Why not just call cleanup() here?


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94881>

    Can you also say why in the comment? Presumably because you dont want containers to see other containers runninng in the system?


src/slave/containerizer/isolators/namespaces/pid.cpp
<https://reviews.apache.org/r/25865/#comment94884>

    Add a comment that you are doing this for the ability to cleanup orphans during recovery?
    
    Also, what is the need for manual cleanup or orphans?


src/slave/containerizer/linux_launcher.cpp
<https://reviews.apache.org/r/25865/#comment94867>

    why is this pulled out?


src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/25865/#comment94885>

    s/NamespacesPidIsolatorTest/PidNamespaceIsolatorTest/


src/tests/isolator_tests.cpp
<https://reviews.apache.org/r/25865/#comment94886>

    you are writing to files, not stdout and stderr right?


- Vinod Kone


On Sept. 23, 2014, 11:39 p.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25865/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 11:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Vinod Kone.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> Add namespaces/pid to --isolation slave flag. Places executor into a pid namespace so it and all descendants will be contained in the namespace. Requires the filesystem/shared isolator so /proc and /sys are remounted to reflect the different namespace.
> 
> 
> Diffs
> -----
> 
>   src/Makefile.am 9b973e5503e30180045e270220987ba647da8038 
>   src/slave/containerizer/isolators/filesystem/shared.cpp PRE-CREATION 
>   src/slave/containerizer/isolators/namespaces/pid.hpp PRE-CREATION 
>   src/slave/containerizer/isolators/namespaces/pid.cpp PRE-CREATION 
>   src/slave/containerizer/linux_launcher.cpp f7bc894830a7ca3f55465dacc7b653cdc2d7758b 
>   src/slave/containerizer/mesos/containerizer.cpp 9d083294caa5c5a47ba3ceaa1b57346144cb795c 
>   src/tests/isolator_tests.cpp c38f87632cb6984543cb3767dbd656cde7459610 
> 
> Diff: https://reviews.apache.org/r/25865/diff/
> 
> 
> Testing
> -------
> 
> Added test that command in pid namespaced container is in a different namespace and that the command is 'init' (verifies remount of /proc).
> 
> 
> Thanks,
> 
> Ian Downes
> 
>


--===============4841732685637016291==--