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 2016/12/10 01:19:26 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 Dec. 10, 2016, 1:19 a.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 (updated)
-------
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 except "gpus" (i.e. "cpus", "mem", "disk" and "ports") when
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 f569c931ff7db8d51dfd7c96f4f2addab05df85d
include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7
src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f
src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9
Diff: https://reviews.apache.org/r/51879/diff/
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
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 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 Joris Van Remoortere <jo...@gmail.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review167528
-----------------------------------------------------------
Closing this review due to inactivity. Please see our [guidelines](https://github.com/apache/mesos/blob/master/docs/reopening-reviews.md) for reopening reviews.
- Joris Van Remoortere
On Dec. 18, 2016, 8:38 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
>
> (Updated Dec. 18, 2016, 8:38 a.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 c2e6d6680d9687f0bbfb73e84ffb1457072c4412
> include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
> src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226
> src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
> src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
> src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833
>
>
> Diff: https://reviews.apache.org/r/51879/diff/12/
>
>
> 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 Dec. 18, 2016, 8:38 a.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 (updated)
-------
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 c2e6d6680d9687f0bbfb73e84ffb1457072c4412
include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1
src/common/resources.cpp 7dbeefc8fc159f28dc1542ad6a761213c65ff226
src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
src/v1/resources.cpp 859f7388035b855bf0cb60d5cbe746e6871bf833
Diff: https://reviews.apache.org/r/51879/diff/
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>.
> On Dec. 15, 2016, 11:58 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, line 142
> > <https://reviews.apache.org/r/51879/diff/11/?file=1581274#file1581274line142>
> >
> > Remove the `.`.
Haha this formatting look odd. I meant s/path./path/
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review159311
-----------------------------------------------------------
On Dec. 9, 2016, 5:19 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 5:19 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 except "gpus" (i.e. "cpus", "mem", "disk" and "ports") when
> 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 f569c931ff7db8d51dfd7c96f4f2addab05df85d
> include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7
> src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f
> src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
> src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
> src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9
>
> Diff: https://reviews.apache.org/r/51879/diff/
>
>
> 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 Dec. 16, 2016, 7:58 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 327-344
> > <https://reviews.apache.org/r/51879/diff/11/?file=1581274#file1581274line327>
> >
> > Here what I found to be a bit awkward:
> >
> > We already know here that we want to process "cpus", "mem", "disk" and "ports". We don't do it directly but we call a helper method which has to check that the input are precisely what this method provides.
> >
> > ```
> > if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") {
> > return Error(
> > "Auto-detection of resource type '" + name + "' not supported");
> > }
> > ```
> >
> > and from this method we have the check the output.
> >
> > This makes the helper not a natrual abstraction but rather a tightly coupled blocked to code to avoid duplicating logic for all predefined resources. If that's the objective, then we can just do:
> >
> > ```
> > const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
> >
> > foreach (const string& name, predefined) {
> > #ifdef __linux__
> > // GPUS are handled separately.
> > if (name == "gpus") {
> > Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
> > if (gpus.isError()) {
> > return Error("Failed to obtain GPU resources: " + gpus.error());
> > }
> >
> > foreach(const Resource& gpu, gpus.get()) {
> > result.push_back(gpu);
> > }
> > }
> > #endif
> >
> > // Content of `detect()` except we can just add stuff to `result` without returning.
> > }
> >
> > // Custom resources.
> > foreach(const Resource& resource, parsed.get()) {
> > if (!predefined.contains(resource.name())) {
> > result.push_back(resource);
> > }
> > }
> > ```
> >
> > The result should be less amount of code without any increase in code duplication.
I removed the following block from `Try<vector<Resource>> detect()` since if the `name` does not match the resources that cannot be auto-detected, it would be a no-op, ie. returns the resources with no modifications.
```
if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") {
return Error(
"Auto-detection of resource type '" + name + "' not supported");
}
```
I think I would still want to keep the `detect()` function separate. Seems cleaner to me, otherwise we would have to have bunch of `if` conditions within this `foreach` for each of the predefined resource types. How does this look instead?
```
const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"});
foreach (const string& name, predefined) {
#ifdef __linux__
// GPU resource is handled separately.
if (name == "gpus") {
Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
if (gpus.isError()) {
return Error("Failed to obtain GPU resources: " + gpus.error());
}
foreach (const Resource& gpu, gpus.get()) {
result.push_back(gpu);
}
}
#endif
if (name != "gpus") {
Try<vector<Resource>> _resources = detect(name, flags, parsed.get());
if (_resources.isError()) {
return Error(
"Failed to obtain " + name + " resources: " + _resources.error());
}
result.insert(
result.end(), _resources.get().begin(), _resources.get().end());
}
}
// Custom resources.
foreach (const Resource& resource, parsed.get()) {
if (!predefined.contains(resource.name())) {
result.push_back(resource);
}
}
```
> On Dec. 16, 2016, 7:58 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp, lines 192-200
> > <https://reviews.apache.org/r/51879/diff/11/?file=1581275#file1581275line192>
> >
> > It's the same logic to support GPUs that are missing values right? Can we add it (fine to be in a separate review)? Otherwise we have to separately document that we don't support GPUs and the inconsistency looks bad.
Seems to be a bit more complicated. When we iterate through `parsed` to retrieve `gpus` and we find a `Resource` with no value, we can add a `Resource` object (of type `gpus`) with a size of `available` retrieved from `nvml::deviceGetCount()`. But if there are additional `gpus` resources (with or without value), then we will fail here eventually:
`if (resources.gpus().get() > available.get())`
I think a better approach might be to account for all `gpus` resources which have values, and then give the `gpus` resources with no value an amount equivalent to `available - (sum of all other gpus resources)`. In case there are more than 1 `gpus` resources with no value, we will still fail at the same point (unless we equally divide the remaining amount of `gpus` resources amongst n of such `gpus` resources having no value.
Thoughts?
- Anindya
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review159311
-----------------------------------------------------------
On Dec. 10, 2016, 1:19 a.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
>
> (Updated Dec. 10, 2016, 1:19 a.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 except "gpus" (i.e. "cpus", "mem", "disk" and "ports") when
> 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 f569c931ff7db8d51dfd7c96f4f2addab05df85d
> include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7
> src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f
> src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
> src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
> src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9
>
> Diff: https://reviews.apache.org/r/51879/diff/
>
>
> 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>.
> On Dec. 15, 2016, 11:58 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/containerizer.cpp
> > Lines 165-169 (original), 288-305 (patched)
> > <https://reviews.apache.org/r/51879/diff/11/?file=1581274#file1581274line327>
> >
> > Here what I found to be a bit awkward:
> >
> > We already know here that we want to process "cpus", "mem", "disk" and "ports". We don't do it directly but we call a helper method which has to check that the input are precisely what this method provides.
> >
> > ```
> > if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") {
> > return Error(
> > "Auto-detection of resource type '" + name + "' not supported");
> > }
> > ```
> >
> > and from this method we have the check the output.
> >
> > This makes the helper not a natrual abstraction but rather a tightly coupled blocked to code to avoid duplicating logic for all predefined resources. If that's the objective, then we can just do:
> >
> > ```
> > const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
> >
> > foreach (const string& name, predefined) {
> > #ifdef __linux__
> > // GPUS are handled separately.
> > if (name == "gpus") {
> > Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
> > if (gpus.isError()) {
> > return Error("Failed to obtain GPU resources: " + gpus.error());
> > }
> >
> > foreach(const Resource& gpu, gpus.get()) {
> > result.push_back(gpu);
> > }
> > }
> > #endif
> >
> > // Content of `detect()` except we can just add stuff to `result` without returning.
> > }
> >
> > // Custom resources.
> > foreach(const Resource& resource, parsed.get()) {
> > if (!predefined.contains(resource.name())) {
> > result.push_back(resource);
> > }
> > }
> > ```
> >
> > The result should be less amount of code without any increase in code duplication.
>
> Anindya Sinha wrote:
> I removed the following block from `Try<vector<Resource>> detect()` since if the `name` does not match the resources that cannot be auto-detected, it would be a no-op, ie. returns the resources with no modifications.
>
> ```
> if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") {
> return Error(
> "Auto-detection of resource type '" + name + "' not supported");
> }
> ```
>
> I think I would still want to keep the `detect()` function separate. Seems cleaner to me, otherwise we would have to have bunch of `if` conditions within this `foreach` for each of the predefined resource types. How does this look instead?
>
> ```
> const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"});
>
> foreach (const string& name, predefined) {
> #ifdef __linux__
> // GPU resource is handled separately.
> if (name == "gpus") {
> Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
> if (gpus.isError()) {
> return Error("Failed to obtain GPU resources: " + gpus.error());
> }
>
> foreach (const Resource& gpu, gpus.get()) {
> result.push_back(gpu);
> }
> }
> #endif
>
> if (name != "gpus") {
> Try<vector<Resource>> _resources = detect(name, flags, parsed.get());
> if (_resources.isError()) {
> return Error(
> "Failed to obtain " + name + " resources: " + _resources.error());
> }
>
> result.insert(
> result.end(), _resources.get().begin(), _resources.get().end());
> }
> }
>
> // Custom resources.
> foreach (const Resource& resource, parsed.get()) {
> if (!predefined.contains(resource.name())) {
> result.push_back(resource);
> }
> }
> ```
This is a reiteration of the comment above and I am inclined to get this shipped first and then work on a refactor.
But for posterity, to summarize in a short example, I don't think this is a good pattern.
```
void genericHelper(condition)
{
// common code.
switch (something) {
case A:
customHelperA();
case B:
customHelperB();
}
}
void highLevelMethod()
{
switch (condition) {
case A:
genericHelper(c1);
case B:
genericHelper(c2);
}
}
```
In this case the generic helper is not really generic. In the high level method if we already have to enumerate specific cases, we should be able to directly call the custom helpers.
To address of the concern of "code duplication" we should be able to just pull out the common code into the high-level method. I think we can do something similar to: https://github.com/apache/mesos/blob/0bc502292d51c7d78c01de5764c5280b3fc0e3df/src/slave/containerizer/mesos/containerizer.cpp#L289
But again, it can be a follow-up.
- Jiang Yan
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review159311
-----------------------------------------------------------
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 Jiang Yan Xu <ya...@jxu.me>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/51879/#review159311
-----------------------------------------------------------
LGTM! Just some minor comments.
src/common/resources.cpp (lines 513 - 514)
<https://reviews.apache.org/r/51879/#comment230287>
If we do change this, I think using "with" and ": " looks better?
```
"Bad type for resource '" + name + "' with value '" + value +
"': " + Value::Type_Name(_value.type()));
```
- ":" because type is already mentioned in the beginning of the sentense.
- Not quoting type because it's not mixed in the sentense anymore and it doesn't contain spaces.
Also the other error in the same method still uses a different style, change both for consistency?
src/common/resources.cpp (lines 604 - 606)
<https://reviews.apache.org/r/51879/#comment230289>
The `Resource` class doesn't know anything about auto-detection so the comment here is not too relevant?
We already have comments elsewhere to explain this.
src/slave/containerizer/containerizer.cpp (line 61)
<https://reviews.apache.org/r/51879/#comment230293>
We acutally aren't leaving any headroom for cpus.
src/slave/containerizer/containerizer.cpp (line 104)
<https://reviews.apache.org/r/51879/#comment230296>
Remove the `.`.
src/slave/containerizer/containerizer.cpp (line 173)
<https://reviews.apache.org/r/51879/#comment230412>
`type` here is ambiguous. In this context there's already a `type` that means SCALAR/RANGES/SET.
Quoted `Resources` means the `Resources` object which we don't return here.
How about
```
Return a vector of `Resource` objects for predefined resources, viz. "cpus", "mem", "disk" and "ports". The amount of resources are detected if unspecified.
```
src/slave/containerizer/containerizer.cpp (line 174)
<https://reviews.apache.org/r/51879/#comment230297>
s/amount/the amount/
src/slave/containerizer/containerizer.cpp (line 183)
<https://reviews.apache.org/r/51879/#comment230413>
s/type //
src/slave/containerizer/containerizer.cpp (line 188)
<https://reviews.apache.org/r/51879/#comment230414>
s/resource/resources/
src/slave/containerizer/containerizer.cpp (lines 206 - 256)
<https://reviews.apache.org/r/51879/#comment230415>
I suggested this previously:
```
vector<Resource> result;
foreach (Resource& resource, resources) {
Resource _resource = resource;
if (name == "cpus" && !resource.has_scalar()) {
_resource = Resources::parse(
name,
stringify(systemCpusAmount()),
_resource.role()).get();
// The following is fine too.
// _resource.set_type(Value::SCALAR);
// _resource.mutable_scalar()->set_value(systemCpusAmount());
} else if (name == "mem" && !resource.has_scalar()) {
_resource = Resources::parse(
name,
stringify(systemMemAmount().megabytes()),
_resource.role()).get();
} else if (name == "disk" && !resource.has_scalar()) {
Try<Bytes> amount = diskAmount(resource, flags);
if (amount.isError()) {
return Error("Failed to retrieve disk size: " + _size.error());
}
_resource.set_type(Value::SCALAR);
_resource.mutable_scalar()->set_value(amount.megabytes());
} else if (name == "ports" && !resource.has_ranges())) {
_resource = Resources::parse(
name,
stringify(DEFAULT_PORTS),
_resource.role()).get();
}
result.push_back(_resource);
}
```
The advantage is that this
- avoids scattering special handling of each kind of resource in multiple places.
- avoids explaining the "defaultSize" (which would be clearer if named `detected`)
- avoids `internal::values::parse()`.
src/slave/containerizer/containerizer.cpp (lines 271 - 273)
<https://reviews.apache.org/r/51879/#comment230397>
My bad in suggesting this indentation style, looks like the commonly used style is:
```
Try<vector<Resource>> parsed = json.isSome()
? Resources::fromJSON(json.get(), flags.default_role)
: Resources::fromSimpleString(text, flags.default_role);
```
src/slave/containerizer/containerizer.cpp (line 281)
<https://reviews.apache.org/r/51879/#comment230408>
Nit: s/resources/resource/ since singular is used below?
src/slave/containerizer/containerizer.cpp (lines 282 - 284)
<https://reviews.apache.org/r/51879/#comment230402>
We do auto-detect 'missing' cpus in the simple string representation as well so this is inaccurate.
How about
```
We auto-detect "cpus" when:
(1) no cpus is specified; or
(2) cpus is specified with no value.
Note (2) is currently only possible when JSON input is used with `--resources` because the parsing logic requires the resource's `type` to be specified (as it's a required protobuf field).
```
src/slave/containerizer/containerizer.cpp (line 287)
<https://reviews.apache.org/r/51879/#comment230403>
"We" (Mesos) do auto-detect "gpus": https://github.com/apache/mesos/blob/ead829bfff3256793185dbeb53906016874c48d7/src/slave/containerizer/mesos/isolators/gpu/allocator.cpp#L241
How about:
```
The same logic applies for the other known resource types. Note that "gpus" auto-detection is handled in `NvidiaGpuAllocator::resources()`.
```
src/slave/containerizer/containerizer.cpp (lines 288 - 305)
<https://reviews.apache.org/r/51879/#comment230434>
Here what I found to be a bit awkward:
We already know here that we want to process "cpus", "mem", "disk" and "ports". We don't do it directly but we call a helper method which has to check that the input are precisely what this method provides.
```
if (name != "cpus" && name != "mem" && name != "disk" && name != "ports") {
return Error(
"Auto-detection of resource type '" + name + "' not supported");
}
```
and from this method we have the check the output.
This makes the helper not a natrual abstraction but rather a tightly coupled blocked to code to avoid duplicating logic for all predefined resources. If that's the objective, then we can just do:
```
const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
foreach (const string& name, predefined) {
#ifdef __linux__
// GPUS are handled separately.
if (name == "gpus") {
Try<Resources> gpus = NvidiaGpuAllocator::resources(flags);
if (gpus.isError()) {
return Error("Failed to obtain GPU resources: " + gpus.error());
}
foreach(const Resource& gpu, gpus.get()) {
result.push_back(gpu);
}
}
#endif
// Content of `detect()` except we can just add stuff to `result` without returning.
}
// Custom resources.
foreach(const Resource& resource, parsed.get()) {
if (!predefined.contains(resource.name())) {
result.push_back(resource);
}
}
```
The result should be less amount of code without any increase in code duplication.
src/slave/containerizer/containerizer.cpp (line 314)
<https://reviews.apache.org/r/51879/#comment230410>
Space after `foreach`.
src/slave/containerizer/containerizer.cpp (lines 321 - 323)
<https://reviews.apache.org/r/51879/#comment230299>
The following more elegant?
```
const hashset<string> predefined = ({"cpus", "mem", "disk", "ports", "gpus"};
...
foreach(const Resource& resource, parsed.get()) {
if (!predefined.contains(resource.name())) {
result.push_back(resource);
}
}
```
src/slave/containerizer/containerizer.cpp (line 328)
<https://reviews.apache.org/r/51879/#comment230416>
`+=` validates the individual 'Resource' objects too. Here I guess we want to highlight that:
```
// Explicit validation because we want to propagte the error.
```
src/slave/containerizer/containerizer.cpp (line 330)
<https://reviews.apache.org/r/51879/#comment230411>
Space after `foreach`.
src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 158 - 160)
<https://reviews.apache.org/r/51879/#comment230422>
Ditto about style.
src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (line 167)
<https://reviews.apache.org/r/51879/#comment230421>
Space after `foreach`.
src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 192 - 196)
<https://reviews.apache.org/r/51879/#comment230429>
It's the same logic to support GPUs that are missing values right? Can we add it (fine to be in a separate review)? Otherwise we have to separately document that we don't support GPUs and the inconsistency looks bad.
- Jiang Yan Xu
On Dec. 9, 2016, 5:19 p.m., Anindya Sinha wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
>
> (Updated Dec. 9, 2016, 5:19 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 except "gpus" (i.e. "cpus", "mem", "disk" and "ports") when
> 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 f569c931ff7db8d51dfd7c96f4f2addab05df85d
> include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7
> src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f
> src/slave/containerizer/containerizer.cpp d46882baa904fd439bffb23c324828b777228f1c
> src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 2e722691475c84afae14009014ea70cc0fdd0e65
> src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9
>
> Diff: https://reviews.apache.org/r/51879/diff/
>
>
> Testing
> -------
>
> Tests passed.
>
>
> Thanks,
>
> Anindya Sinha
>
>