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




src/slave/containerizer/composing.cpp (lines 276 - 280)
<https://reviews.apache.org/r/47707/#comment199210>

    How about "managed" and "manage" instead of "enumerated" and "enumerate" 
here? It seems like the important thing to understand in this comment is that 
each containerizer is trying to manage resources?



src/slave/containerizer/composing.cpp (lines 304 - 306)
<https://reviews.apache.org/r/47707/#comment199212>

    Hm.. how would this read?
    
    Failed to merge 'cpus' managed by the composed containerizers: one 
containerizer is managing 2cpus and another is managing 3cpus
    
    You could print this by stringifying the filtered resources.



src/slave/containerizer/containerizer.hpp (lines 64 - 68)
<https://reviews.apache.org/r/47707/#comment199217>

    Hm.. it seems more like the default set of resources would not include the 
flags.
    
    The current function is a little more than just the default resources, it 
is the flag provided resources along with defaults injected in for any that 
were omitted.
    
    Could we instead provide just the defaults (per the suggested signature 
above) and have containerizer implementations inject defaults with the help of 
this function?
    
    As an example:
    
    ```
    // Provides a means for containerizer implementation to use a 
    // consistent set of default resources when resources are not
    // specified via the '--resources' flag. The containerizer may
    // choose its own default value by probing the system, however
    // this helper provides a standard set of defaults for:
    //
    //     ["cpus", "mem", "disk", "ports"]
    //
    // A containerizer is free to choose alternative defaults for these
    // resources, but this increases the likelihood that containerizers
    // disagree about the amount of resources to manage when used within
    // the composing containerizer.
    static Try<Resources> defaultResources();
    ```
    
    This means we could augment the Containerizer::resources and 
Isolator::resources to do two things:
    
    (1) Get informed about the resources provided via flags, and
    (2) Return additional resources that the containerizer or isolator wishes 
to manage.
    
    Something like:
    
    ```
    // Informs the containerizer / isolator about the resources
    // that are explicitly specified. The containerizer / isolator
    // returns a subset of the resources that it will manage. The
    // containerizer / isolator may also return additional resources
    // that it wishes to manage if they were not specified.
    // For example, a GPU isolator, seeing that "gpus" is unspecified
    // may probe the system and decide to manage some GPUs (i.e. it
    // will return these GPUs from this function).
    Future<Resources> manage(const Resources& provided);
    ```



src/slave/containerizer/containerizer.cpp (lines 57 - 61)
<https://reviews.apache.org/r/47707/#comment199216>

    Why this comment in the .cpp in addition to the one in the .hpp?



src/slave/containerizer/mesos/containerizer.cpp (lines 479 - 480)
<https://reviews.apache.org/r/47707/#comment199218>

    Ditto on "manage" vs "enumerate" here.



src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp (line 
242)
<https://reviews.apache.org/r/47707/#comment199209>

    It's a bit implicit that this condition ensures that 
`resources.gpus().isSome()`, could we store the return value of 
`resources.gpus()` and use it in both the condition and the validation?
    
    ```
    Option<double> gpus = resources.gpus();
    
    if (gpus.isSome()) {
      ...
      long long millis = static_cast<long long>(gpus.get() * 1000);
      ...
    }
    ```
    
    This seems clearer?



src/slave/slave.cpp (lines 476 - 478)
<https://reviews.apache.org/r/47707/#comment199208>

    Would you mind adding a TODO to avoid waiting forever? I'm just thinking of 
the debugability when there's a inifitely pending future coming out of an 
isolator module.



src/slave/slave.cpp (lines 481 - 484)
<https://reviews.apache.org/r/47707/#comment199206>

    Could you handle the discarded case? We don't expect a discarded result 
since we do not request a discard on this future, but it would be nice to avoid 
the call to .get() without validating this:
    
    ```
    CHECK_READY(resources);
    ```
    
    Or modify the error handling:
    
    ```
      if (!resources.isReady()) {
        EXIT(EXIT_FAILURE)
          << "Failed to determine agent resources: " << resources.isFailed() ? 
resources.failure() : "discarded";
      }
    ```


- Benjamin Mahler


On May 23, 2016, 12:45 a.m., Kevin Klues wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47707/
> -----------------------------------------------------------
> 
> (Updated May 23, 2016, 12:45 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jie Yu.
> 
> 
> Bugs: MESOS-5256
>     https://issues.apache.org/jira/browse/MESOS-5256
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Previously, the `Containerizer` class contained a static `resources()`
> function for enumerating all of the resources available on an agent.
> This made it hard to add new resources to this enumeration that were,
> for example, containerizer-specific or tied to a new isolator module
> loaded at runtime.
> 
> This commit refactors the resource enumeration logic to allow each
> containerizer to enumerate resources itself. It does this by adding a
> new virtual `resources()` function, whose default implementation
> includes the same logic as the old static version of `resources()`.
> Individual containerizers simply overwrite this function to do their
> own enumeration if they want to.
> 
> Similar logic exists to push resource enumeration down into isolators
> as well. As such, the logic for the Nvidia GPU resource enumeration has
> been moved down into the Nvidia GPU isolator instead of being
> hard-coded into the mesos containerizer itself.
> 
> 
> Diffs
> -----
> 
>   include/mesos/slave/isolator.hpp 4be8c2bb409052e2e07138483408209384f41e23 
>   src/slave/containerizer/composing.hpp 
> f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 
>   src/slave/containerizer/composing.cpp 
> 15d059f0bbda4e8cb93c65c09327dde1e34d3e7b 
>   src/slave/containerizer/containerizer.hpp 
> ff78b4d0fd4a3b862f6019fc757c16b7367cd3cf 
>   src/slave/containerizer/containerizer.cpp 
> faa0c789dda8a6f36fdb6217b0bae270b6b8f2e2 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/isolator.hpp 
> bacd86af42d16cb7c9b6622dfb298dcaa7007b75 
>   src/slave/containerizer/mesos/isolator.cpp 
> 253ff3cea8aff3e7a3051fb5a763cc081f455f18 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.hpp 
> 502204650192d5ea44aa631eac8eb37e051843f0 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> 8f81cb79c10261670efc9eaa8614751854f53806 
>   src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 
> 
> Diff: https://reviews.apache.org/r/47707/diff/
> 
> 
> Testing
> -------
> 
> make -j check ( which fails on one test -- see 
> https://reviews.apache.org/r/47708/ )
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Reply via email to