mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Bannier <benjamin.bann...@mesosphere.io>
Subject Re: Review Request 46798: Introduced linux capabilities support for mesos containerizer.
Date Mon, 11 Jul 2016 17:10:13 GMT

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




src/launcher/executor.cpp (line 90)
<https://reviews.apache.org/r/46798/#comment207006>

    I would much prefer this to not force each and every caller to deal with different function
signatures.
    
    We should probably try to find a way to use identical signatures on every platform, and
then possible make the underlying implementation do what is actually supported.



src/launcher/executor.cpp (line 376)
<https://reviews.apache.org/r/46798/#comment207007>

    It seems we should be able to do away with conditional compilation here if we'd push the
check for platform support into the capabilities implementation.



src/launcher/executor.cpp (line 397)
<https://reviews.apache.org/r/46798/#comment207008>

    Same here, we should try to remove the conditional compilation and instead push the checks
for platform support into the capabilities infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 270)
<https://reviews.apache.org/r/46798/#comment207019>

    This could again be simplified by pushing the check for platform support into the capabilities
infrastructure.



src/slave/containerizer/mesos/containerizer.cpp (line 271)
<https://reviews.apache.org/r/46798/#comment207020>

    I think moving the capabilities check out of the loop would improve readibilty of this
loop.



src/slave/containerizer/mesos/containerizer.cpp (line 1299)
<https://reviews.apache.org/r/46798/#comment207021>

    Remove conditional compilation once member exists for all platforms.



src/slave/containerizer/mesos/launch.cpp (line 31)
<https://reviews.apache.org/r/46798/#comment207015>

    Sort alphabetically.



src/slave/containerizer/mesos/launch.cpp (line 49)
<https://reviews.apache.org/r/46798/#comment207046>

    As currently written this needs to be compiled conditionally as `Capabilities` is not
defined on non-Linux platforms.



src/slave/containerizer/mesos/launch.cpp (line 280)
<https://reviews.apache.org/r/46798/#comment207017>

    Why do we need to require a shell command here?



src/slave/containerizer/mesos/launch.cpp (line 281)
<https://reviews.apache.org/r/46798/#comment207018>

    The nesting of these preprocessor checks (where one checks a negative and the other for
a positive) on top of a already deeply control flow makes this really hard to read.
    
    I believe this could again be simplified by pushing the check for platform support into
the capabilities infrastructure.


- Benjamin Bannier


On May 26, 2016, 5:03 p.m., Jojy Varghese wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46798/
> -----------------------------------------------------------
> 
> (Updated May 26, 2016, 5:03 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> This change introduces linux capability based security for unified
> containerizer. A new agent flag \`allowed_capabilities\` has been
> introduced to override the default capabilities of the user or the
> capabilities requested by the user.
> 
> This feature is only available on linux.
> 
> 
> Diffs
> -----
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
>   src/launcher/executor.cpp 7d111e668e0a139a98bdeb959997843180b40452 
>   src/slave/containerizer/mesos/containerizer.hpp a1a00020668f6da8d611f26e5637afffc87d09ba

>   src/slave/containerizer/mesos/containerizer.cpp 75e5a32a3e70ec60a6800e21a621673184ea0956

>   src/slave/containerizer/mesos/launch.hpp c716e0396736d1f2f60ec31540f12f4f7597d081 
>   src/slave/containerizer/mesos/launch.cpp e22106b014c871e2184a15c2ab154a0674874e47 
>   src/slave/flags.hpp 80ba2887448e91c40ae68fc2d9f0c0bee1a49f48 
>   src/slave/flags.cpp b7df8f760d0f75459f1e80e3d8e18d49a3995df8 
>   src/tests/container_logger_tests.cpp efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp c524f42743bf08ee54f1cbb083d0d3c85a8b70c9

>   src/tests/containerizer/filesystem_isolator_tests.cpp 4293416ac8434e9eb7e80724480a54936a2fe24a

>   src/tests/containerizer/mesos_containerizer_tests.cpp 09742ff21513dc2570684d384b257868dd57a9ce

> 
> Diff: https://reviews.apache.org/r/46798/diff/
> 
> 
> Testing
> -------
> 
> make check; used mesos cli to test end to end functionality.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>


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