You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Anindya Sinha <an...@apple.com> on 2017/05/24 19:56:44 UTC
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/
-----------------------------------------------------------
(Updated May 24, 2017, 7: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 (updated)
-----
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/
Changes: https://reviews.apache.org/r/51879/diff/12-13/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
Posted by Anindya Sinha <an...@apple.com>.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 113 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731951#file1731951line175>
> >
> > 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.
Dropped the RR https://reviews.apache.org/r/52002, and moved the other change in that RR to https://reviews.apache.org/r/59724/.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp
> > Lines 182-190 (original), 192-196 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731952#file1731952line192>
> >
> > 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?
Moved this part to a separate RR (https://reviews.apache.org/r/59723/) in this chain.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_endpoints_tests.cpp
> > Lines 2180-2195 (patched)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731953#file1731953line2196>
> >
> > Are these just reordering? Why do it?
> >
> > Same for other changes in this file?
In `Try<Resources> Containerizer::resources(const Flags& flags)`, we use a `const hashset<string> predefined = {"cpus", "mem", "disk", "ports", "gpus"}` (since we call a `contains()`). As a result, the order in which `vector<Resource> result` and hence the `resources` returned from this function varies in order from before.
The test uses `JSON::Value` which should be enhanced to do a comparison using `Resources` which we can do later. For now, I changed the order of the `JSON::Value` in the test to satisfy the condition.
> On June 1, 2017, 12:42 a.m., Jiang Yan Xu wrote:
> > src/tests/reservation_endpoints_tests.cpp
> > Lines 1784-1791 (original)
> > <https://reviews.apache.org/r/51879/diff/13/?file=1731954#file1731954line1784>
> >
> > Ditto. Why reordering?
Same reason as in previous comment.
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review176411
-----------------------------------------------------------
On May 24, 2017, 7: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, 7: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
>
>
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
Posted by Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
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
>
>
Re: Review Request 51879: Autodetect value of resource when not
specified in static resources.
Posted by Anindya Sinha <an...@apple.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/
-----------------------------------------------------------
(Updated June 1, 2017, 7:16 p.m.)
Review request for mesos and Jiang Yan Xu.
Changes
-------
Addressed review comments.
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 (updated)
-----
include/mesos/resources.hpp 5d16a80a0d4fc386ad62c7dffe96b68d11ebddb1
include/mesos/v1/resources.hpp e1c35777227dd709f90b934859eb946daffbcd97
src/common/resources.cpp 97c00b204b17fa6b47face6873eea9751978f52c
src/slave/containerizer/containerizer.cpp 15ae0b33a5ea8f73a9a84081d9c310c1d59c3141
src/tests/persistent_volume_endpoints_tests.cpp 9b820b8f4e0d0e2c62b5c9b420b790cdfb59dd0c
src/tests/reservation_endpoints_tests.cpp 505c5421e95378177a7a09f462e5625ffa75cd37
src/v1/resources.cpp 559b048b65cce2da2e75abd740fc857f35280143
Diff: https://reviews.apache.org/r/51879/diff/14/
Changes: https://reviews.apache.org/r/51879/diff/13-14/
Testing
-------
Tests passed.
Thanks,
Anindya Sinha