----------------------------------------------------------- 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 > >
