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



See my comment around the CNI isolator. Need more discussion on this one


include/mesos/slave/isolator.hpp
Lines 57 (patched)
<https://reviews.apache.org/r/65932/#comment279063>

    why returning `Option<bool>` instead of `bool`?
    
    What does `None()` mean here?



src/slave/containerizer/mesos/containerizer.cpp
Lines 468 (patched)
<https://reviews.apache.org/r/65932/#comment279061>

    Should we use `geteuid` here (so that mesos agent can be a setuid program)?



src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp
Line 56 (original), 56 (patched)
<https://reviews.apache.org/r/65932/#comment279066>

    not yours, can you follow up with patch to make those `const` as well (just 
to be consistent)



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Line 226 (original), 222 (patched)
<https://reviews.apache.org/r/65932/#comment279065>

    2 lines apart



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp
Lines 229-232 (patched)
<https://reviews.apache.org/r/65932/#comment279067>

    Looks like this override is not necessary. I'd suggest remove this 
override. That also allows us to perform optimization (fine grind capabilities 
check) in the future.



src/slave/containerizer/mesos/isolators/filesystem/posix.hpp
Lines 37 (patched)
<https://reviews.apache.org/r/65932/#comment279068>

    `{` to the next line



src/slave/containerizer/mesos/isolators/filesystem/posix.hpp
Lines 38 (patched)
<https://reviews.apache.org/r/65932/#comment279069>

    hum, why posix filesystem isolator requires root?



src/slave/containerizer/mesos/isolators/filesystem/shared.cpp
Lines 221-224 (patched)
<https://reviews.apache.org/r/65932/#comment279070>

    Kill



src/slave/containerizer/mesos/isolators/gpu/isolator.cpp
Lines 239 (patched)
<https://reviews.apache.org/r/65932/#comment279072>

    gpu isolator requires mount namespaces so that the injected volumes won't 
show up on the host.



src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp
Lines 70-73 (patched)
<https://reviews.apache.org/r/65932/#comment279071>

    kill



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 88-101 (original), 88-96 (patched)
<https://reviews.apache.org/r/65932/#comment279077>

    CNI isolator is a bit tricky here. We do enable that currently when the 
agent is running not under 'root' so that it can reject container launch 
requesting joining a named CNI network.
    
    So this change will break it.
    
    I was thinking maybe we should make `root` permission check a per container 
check during launch time (rather than an isolator level global check). Of 
course, the downside is that we cannot fail fast.
    
    Also, the namespace requirements are different for each container. If a 
container joins host network and does not define container image, no namespace 
is required.
    
    If we really want to add an isolator level check, that'll probably check 
the "minimal" requirement.


- Jie Yu


On March 6, 2018, 6:18 p.m., James Peach wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65932/
> -----------------------------------------------------------
> 
> (Updated March 6, 2018, 6:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Jie Yu.
> 
> 
> Bugs: MESOS-6555
>     https://issues.apache.org/jira/browse/MESOS-6555
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Many isolators require root privileges and support for specific
> Linux namespaces. By adding some additional virtual functions
> to the `Isolator` interface, we can let isolators declare
> their needs and perform the checks in a containerizer single
> pass when we create the isolators.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/isolator.hpp f682e3037523b1c210b5ba36246556518f261e1b 
>   src/linux/ns.cpp 64722c785cecce5b861efbc3f3f2027a61f666d4 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a61ed5bb4f60ec63aebd6bdabbc9a4b01235623e 
>   src/slave/containerizer/mesos/isolator.hpp 
> b7af3946155e28d80013a650973ae230de7d01c5 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> 5763c9880728f02e44116fd50e62b592a8ef69b6 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 4431ce13d28035b0c5c037b2848ae03aeaf65562 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.hpp 
> 0e1761d44f7dce51bef138c5eda2ac3d8eb6ea21 
>   src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
> a6f05a8cf7eab2b2cc4c2142efdf3125462ec68e 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.hpp 
> d933af4e5a605f63d3c4bc584e82de7f3f80da55 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> f1d377eb911fb6a0ca3a4749a0e00d5166f35129 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> 794b6e5990db5f8eb21a6535872f284ca02e0553 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.hpp 
> 414682ba8380e8960ff93666799e2f0f674022c1 
>   src/slave/containerizer/mesos/isolators/filesystem/shared.cpp 
> 118f39871fd76ca00b92e4fa80c698a2a8f02f19 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.hpp 
> 5d1bc7bc637fb266018ba89e668910ef7f89d5e8 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.cpp 
> f5e8860cc3ce3dcd6d4d15ccfd1cc690484b1b57 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp 
> 73e26a0c78ccfa43f533d6811f0a58ebe1bc4403 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
> 2e8ad930df489777845928fb24239365c043f618 
>   src/slave/containerizer/mesos/isolators/namespaces/ipc.hpp 
> b3815ae21dd3d31c2c0b94305e262f167fb95030 
>   src/slave/containerizer/mesos/isolators/namespaces/ipc.cpp 
> 6c8e8eed69ed355094f7d9b54bde25a8a9bc63f7 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.hpp 
> 5ed4a6eceb0a7bd340a85482eab013a157c56cff 
>   src/slave/containerizer/mesos/isolators/namespaces/pid.cpp 
> 73e09636808081ce411c022bf2bcb2a157a3ad25 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 1d01915c2db66e54ed68a3dbaa12ea061ca5f6b2 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> c60c23f74f2abf6bef8dd32cc2e47e33bf666169 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
> 6ff3820de6af544c6ec92b0e76fd934715b87a96 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 96e1a2b4133171e7a204e5d234c28ed81913261c 
>   src/slave/containerizer/mesos/isolators/posix/rlimits.hpp 
> 2d95fb2af0283ed5201cb33b653e6dfb9c35d532 
>   src/slave/containerizer/mesos/isolators/posix/rlimits.cpp 
> 9aa886fc4b8a1eddaf96a93b4385dd99681a8588 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.hpp 
> 20d5b32fb7ada1a17a40bf1a2438db4d85cf1063 
>   src/slave/containerizer/mesos/isolators/volume/sandbox_path.cpp 
> 5801977e93bcb8f463a2635f73e763098f2aa97d 
>   src/slave/containerizer/mesos/isolators/volume/secret.hpp 
> 2680345c974c5c20bef1f2715677b37f6bead27f 
>   src/slave/containerizer/mesos/isolators/volume/secret.cpp 
> 8071e4ee808bc825b13a6291767778d6ce3c2746 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp 
> 07e68a777aefba4dd35066f2eb207bba7f199d83 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> 8d9f8f846866f9de377c59cb7fb311041283ba70 
>   src/tests/containerizer/ns_tests.cpp 
> fa4349e29b975550e05b00a1f848a24cd8e4f0de 
> 
> 
> Diff: https://reviews.apache.org/r/65932/diff/1/
> 
> 
> Testing
> -------
> 
> make check (Fedora 27)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Reply via email to