mesos-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Adam B" <a...@mesosphere.io>
Subject Re: Review Request 16147: Containerizer (part 1)
Date Fri, 24 Jan 2014 01:59:05 GMT

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


Finally got around to reviewing the new containerizer files. Looks great, only some minor
comments.


src/slave/container/containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61582>

    Comment what the map<string, string> return value represents, please? Looks like
environment variables mapped to their values.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61583>

    Do you need to use "process::" since you're "using namespace process" above?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61594>

    Lost some TODOs in the move that might be useful:
    // TODO(benh): Move this computation into Flags as the "default".
    // TODO(vinod): Move some of this computation into Resources.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61585>

    If resources = parsed.get(), and !resources.cpus().isSome(), then how can parsed.get().cpus().isSome()
be true?
    Maybe I'm missing something here about Resources and Try<>, but it looks like unreachable
code to me. Ditto for mem.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61589>

    Lost the old comment, including BenH's TODO:
    // Leave 1 GB free if we have more than 1 GB, otherwise, use all!		
    // TODO(benh): Have better default scheme (e.g., % of mem not		
    // greater than 1 GB?)
    Shouldn't we put it back in?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61597>

    resources += Resources::parse(...? Like you did for cpu/mem. Same applies to 'ports' below.
    I'd like to see the style consistent for each of these four resources.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61600>

    Could include the default translation (e.g. process -> "posix/cpu,posix/mem") in log/warn
message, so a user knows what to replace their deprecated isolation options with.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61613>

    Will isolator.error() include type in its string? Seems useful.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61601>

    PosixLauncher can be used on Linux too, right?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61615>

    Might want to give a different error message from below, to indicate an unrecognized-file-extension
error as opposed to the command failing.



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61616>

    s/htfp/hftp/



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61617>

    from <uri> to <path>?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61620>

    Shouldn't path::join be "making it: <local>" and not uri (the original relative
path)?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61619>

    More specific error message?



src/slave/container/containerizer.cpp
<https://reviews.apache.org/r/16147/#comment61621>

    s/uri/local/ ?



src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61623>

    And if state.isNone(), it terminates/cleans-up all executors? Or recovers all?



src/slave/container/mesos_containerizer.hpp
<https://reviews.apache.org/r/16147/#comment61624>

    There were talks (with BenH) of moving the Reaper into the launcher, but that can be a
later review/change


- Adam B


On Jan. 21, 2014, 11:36 a.m., Ian Downes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/16147/
> -----------------------------------------------------------
> 
> (Updated Jan. 21, 2014, 11:36 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Ben Mahler, Chi Zhang, Niklas Nielsen, samya,
and Jason Dusek.
> 
> 
> Repository: mesos-git
> 
> 
> Description
> -------
> 
> The proposed Containerizer interface is to replace the existing Isolator. 
> 
> One ContainerizerProcess has been written:
> MesosContainerizerProcess - implements containerizeration internally using a Launcher
and one or more Isolators (following review)
> 
> The intent is to also support a generic ExternalContainerizerProcess that can delegate
containerizeration by making external calls. Other Containerizers could interface with specific
external containerization techniques such as Docker or LXC.
> 
> 
> Diffs
> -----
> 
>   include/mesos/mesos.proto 655f86757487ddbe551fdcf53eb793e773ecdd34 
>   src/Makefile.am cf0c8c66e8dd21c2f5a2da22e5d5adb056353454 
>   src/common/type_utils.hpp 3b05751a9a27055dd19e4abb79197f53e5554fe1 
>   src/launcher/launcher.hpp 104fe812ddd2b75b14ec0b40d192f58cdba3603a 
>   src/launcher/launcher.cpp d5ab66704429a95eeb8eda5188e33d8e691221af 
>   src/launcher/main.cpp de64609905ee63096c0173fe7e64a1eafea5d6bf 
>   src/local/local.cpp 83a7f913afb1441e9137c7fcec8fd81815714905 
>   src/slave/cgroups_isolator.hpp e86062e9abaaa263c32c55e9dbfefd700f605886 
>   src/slave/cgroups_isolator.cpp 80155a3f0cd684a6aad9a37f61c5204d86032735 
>   src/slave/container/containerizer.hpp PRE-CREATION 
>   src/slave/container/containerizer.cpp PRE-CREATION 
>   src/slave/container/external_containerizer.hpp PRE-CREATION 
>   src/slave/container/mesos_containerizer.hpp PRE-CREATION 
>   src/slave/container/mesos_containerizer.cpp PRE-CREATION 
>   src/slave/flags.hpp 827b2d0d6dc8fa279f3187a09e5dc6d4799d17cd 
>   src/slave/http.cpp c8357e214d2adf2cd712072f58d07b07badb79dc 
>   src/slave/isolator.hpp 9634535d8c746597b4bb6e278587a1b9ca8f1608 
>   src/slave/isolator.cpp c9643cf9c5657bc142482a71fb161233bffb3b9f 
>   src/slave/main.cpp e0cae7b205c2599e05c4db990cc9c8e9e3673c37 
>   src/slave/monitor.hpp 52568ad8ec566f7cf36c249c76d798d44eacb578 
>   src/slave/monitor.cpp a931c4f35a8793c66ee03de82f0e0a21b92f8ffa 
>   src/slave/paths.hpp b1a56a346ca84e7e71c31386602264c5097ef1cf 
>   src/slave/process_isolator.hpp 4ae093fe65775a2b9bec42071961dd58aa0c3d8b 
>   src/slave/process_isolator.cpp 0bc698f04f7c8eaad166dc9d646e13310129dd01 
>   src/slave/slave.hpp 38ba7b6d6c35f0668d2f021a1285363cbf4367f4 
>   src/slave/slave.cpp 8b83dacad802a5434bf76ca075b2b8113ed14a84 
>   src/slave/state.hpp 78b20ffae1f2f629d851784302648e3bf5349321 
>   src/slave/state.cpp 93cde0c2efbbe5387448631a02e64b20a3d021c1 
>   src/slave/status_update_manager.hpp 06ea4659cdd24cb1b82f389f404566ba14a663fb 
>   src/slave/status_update_manager.cpp 03f5eafefd6ed748bfd4511d654c23c7b460db66 
> 
> Diff: https://reviews.apache.org/r/16147/diff/
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Ian Downes
> 
>


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