mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jiang Yan Xu <...@jxu.me>
Subject Re: Review Request 51879: Autodetect value of resource when not specified in static resources.
Date Thu, 01 Jun 2017 00:42:40 GMT

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



Sorry this patch had to go through this long delay. We couldn't resolve a few open issues
last time and I still think we need some follow-ups but I think we can get this patch into
a mergable state and iterate on it. :)


src/common/resources.cpp
Line 617 (original), 617-622 (patched)
<https://reviews.apache.org/r/51879/#comment249776>

    This is exatctly the content of `Resources::fromString` (which was added after we introduced
`Resources::fromSimpleString`).



src/slave/containerizer/containerizer.cpp
Lines 113 (patched)
<https://reviews.apache.org/r/51879/#comment249928>

    Sorry I know you had https://reviews.apache.org/r/52002 which was originally intended
for all disk types and later evolved into a single `bool isRootDisk()` but given this is the
only use of the helper can we just simply do the following here?
    
    ```
    bool isRootDisk = !resource.has_disk();
    ```
    
    Now I think a proper follow up would be to actually to give the root disk a type which
is set by default.
    
    e.g.,
    
    ```
        message Source {
          enum Type {
            UNKNOWN = 0;
            PATH = 1;
            MOUNT = 2;
            ROOT = 3;
          }
          ...
          optional Source source = 3 [default = ROOT];
        }
    ```
    
    Which would have made things much simpler in many places.



src/slave/containerizer/containerizer.cpp
Lines 141 (patched)
<https://reviews.apache.org/r/51879/#comment249934>

    The awkward situation here is that the code here is in fact reachable. This is the result
of the us doing auto-detection before some minimal validation. Therefore a json input with
a UNKNOWN type could crash the agent here.
    
    To fix this particular case we can return an Error here but the general situation that
we "detect without for minimal validation" is a bit concerning.
    
    Let's just return an error right now. I think a proper follow-up would be to split `Resources::validate()`
into composable pieces, similar to how we introduced `Resources::from*`.



src/slave/containerizer/containerizer.cpp
Lines 205 (patched)
<https://reviews.apache.org/r/51879/#comment249908>

    You can take a copy by removing the `const&` above.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 153-154 (original), 153-160 (patched)
<https://reviews.apache.org/r/51879/#comment249909>

    This is covered by `Resources::fromString`.



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 160-163 (original), 166-173 (patched)
<https://reviews.apache.org/r/51879/#comment249910>

    s/gpus/_resources/
    
    Otherwise gpus vs. resources looks odd. (They are the same thing of different types)



src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
Lines 182-190 (original), 192-196 (patched)
<https://reviews.apache.org/r/51879/#comment249919>

    So if we are not auot-detecting for "gpus:" (which I think is fine for now as an incremental
step), are the changes in "src/slave/containerizer/mesos/isolators/gpu/allocator.cpp" just
a follow-up for https://reviews.apache.org/r/55761/?
    
    Then can we separate it out to simplify this patch?



src/tests/persistent_volume_endpoints_tests.cpp
Lines 2180-2195 (patched)
<https://reviews.apache.org/r/51879/#comment249920>

    Are these just reordering? Why do it?
    
    Same for other changes in this file?



src/tests/reservation_endpoints_tests.cpp
Lines 1784-1791 (original)
<https://reviews.apache.org/r/51879/#comment249922>

    Ditto. Why reordering?



src/v1/resources.cpp
Line 619 (original), 619-624 (patched)
<https://reviews.apache.org/r/51879/#comment249923>

    This is exatctly the content of `Resources::fromString` (which was added after we introduced
`Resources::fromSimpleString`).


- Jiang Yan Xu


On May 24, 2017, 12:56 p.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
> 
> (Updated May 24, 2017, 12:56 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
>     https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When static resources indicate resources with a positive size, we use
> that for the resources on the agent. However, --resources can include
> resources with no size, which indicates that mesos agent determine the
> size of those resources from the agent and uses that information. Note
> that auto-detection of resources is allowed for all known resource
> types represented in JSON format only. Auto-detection is not done when
> the resources are represented in text format.
> 
> With this change, JSON representation for disk resources that do not
> specify any value would not result in an error, but those resources
> will not be accounted for until a valid size is determined for such
> resources. A scalar value of -1 still results in an invalid resource.
> 
> 
> Diffs
> -----
> 
>   include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
>   include/mesos/v1/resources.hpp 132ef3eb03d335774ba13ecb39045128d0476954 
>   src/common/resources.cpp 1d07f1a049ba3bb3f5d78f05031f33ba97e07e8c 
>   src/slave/containerizer/containerizer.cpp 15ae0b33a5ea8f73a9a84081d9c310c1d59c3141

>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp c288ad634b856702483b9751f41445308babd0c9

>   src/tests/persistent_volume_endpoints_tests.cpp 8c54372b7f6d94f0335561086f2a8cb90373e285

>   src/tests/reservation_endpoints_tests.cpp 8c195eb7d788fbca8722d66d81c77d399702160a

>   src/v1/resources.cpp 1bb5d0741c9f9ea59f34e92159a335661019444d 
> 
> 
> Diff: https://reviews.apache.org/r/51879/diff/13/
> 
> 
> Testing
> -------
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>


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