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 Wed, 30 Nov 2016 08:53:07 GMT


> On Sept. 28, 2016, 3:37 p.m., Jiang Yan Xu wrote:
> > src/common/resources.cpp, lines 587-618
> > <https://reviews.apache.org/r/51879/diff/6/?file=1510293#file1510293line587>
> >
> >     No need for the helpers. Just the following is sufficient.
> >     
> >     ```
> >     Resource resource;
> >     resource.set_name(name);
> >     resource.set_role(role);
> >     
> >     // Return a Resource with missing value.
> >     if (_value.isNone()) {
> >       return resource;
> >     }
> >     
> >     Value _value = result.get();
> >     
> >     if (_value.type() == Value::SCALAR) {
> >       resource.set_type(Value::SCALAR);
> >       resource.mutable_scalar()->CopyFrom(_value.scalar());
> >     } else if (_value.type() == Value::RANGES) {
> >       resource.set_type(Value::RANGES);
> >       resource.mutable_ranges()->CopyFrom(_value.ranges());
> >     } else if (_value.type() == Value::SET) {
> >       resource.set_type(Value::SET);
> >       resource.mutable_set()->CopyFrom(_value.set());
> >     } else {
> >       return Error(
> >           "Bad type for resource " + name + " value " + value +
> >           " type " + Value::Type_Name(_value.type()));
> >     }
> >     ```
> 
> Guangya Liu wrote:
>     Seems we still need the `type` for resource when `_value.isNone()`, so the logic
could be:
>     
>     ```
>     Resource resource;
>     resource.set_name(name);
>     resource.set_role(role);
>     
>     // Return a Resource with missing value.
>     if (_value.isNone()) {
>       Try<Value::Type> _type = Resources::valueType(name);
>       if (_type.isError()) {
>         return Error(_type.error());
>       }
>       
>       resource.set_type(_type.get());
>       return resource;
>     }
>     
>     Value _value = result.get();
>     
>     if (_value.type() == Value::SCALAR) {
>       resource.set_type(Value::SCALAR);
>       resource.mutable_scalar()->CopyFrom(_value.scalar());
>     } else if (_value.type() == Value::RANGES) {
>       resource.set_type(Value::RANGES);
>       resource.mutable_ranges()->CopyFrom(_value.ranges());
>     } else if (_value.type() == Value::SET) {
>       resource.set_type(Value::SET);
>       resource.mutable_set()->CopyFrom(_value.set());
>     } else {
>       return Error(
>           "Bad type for resource " + name + " value " + value +
>           " type " + Value::Type_Name(_value.type()));
>     }
>     ```
> 
> Jiang Yan Xu wrote:
>     I don't think so. The type is based on the value. No value, no type. After we detect
the value, we'll have the type, no?
> 
> Anindya Sinha wrote:
>     Firstly, I think type is based on resource name (for non-custom resources), and value
is based on resource name and type.
>     
>     Secondly, the function:
>     `Try<Resource> Resources::parse(const string& name, const string& value,
const string& role)`
>     
>     is always expected to return a valid `Resource` object (if there is no error). By
not setting the `type`, we make the `Resource` object invalid since `type` is a `required`
parameter. But not setting value does not make `Resource` invalid since `scalar`, `ranges`
and `set` are `optional` fields.
>     
>     Finally, we need JSON input to indicate `type` since it is `required` and not specifying
`type` (for resource with value missing in JSON format) would fail `protobuf::parse<RepeatedPtrField<Resource>>(resourcesJSON)`
in `Resources::fromJSON()` since the protobuf parser expects `type` to be there.
>     
>     Hence, I think we should expect all input to indicate type (explicitly in JSON, and
implicitly based on resource name for text input). So, I think we should add the resource
type in `Try<Resource> Resources::parse(const string& name, const string& value,
const string& role)`.

Didn't realize this when I was first commenting but it looks like that the value of calling
`Try<Resource> Resources::parse(const string& name, const string& value, const
string& role)` with an empty value is basically zero... After calling the method which
has to be changed to handle the special case, you have to assert that the resulting Try is
a SOME. You might as well spend 3 lines to just do:

```
Resource resources;
resources.set_name(name);
resources.set_role(role);
```

Can we just do that?

---

To be clear I do agree that Mesos should provide the types for predefined resources but I
think:

Ideally:

- The user shouldn't need to specify types in either JSON or simple string format. 
- If the user does specify the type they should be validated. (We don't do that today).
- The protobuf type of the predefined resource should come out of its own C++ type.

It sucks that:

- The JSON input has to specify the resource type just because protobuf requires it.

---

For now:

The reality is that we don't really have to support simple string auto-detection in the form
of "cpus:" or "cpus"; they don't have feature parity with the JSON definition to begin with.
I now think that it simplifies things a bit if we don't support it. Then we don't need to
make `Try<vector<Resource>> Resources::fromSimpleString(...)` support inputs like
"cpus:" or "cpus" without the `type`.

Longer term:

We can change the `Resources::type` from required to optional in the way similar to MESOS-4997.
In fact, we don't even need to change the public APIs, just the internal APIs.


> On Sept. 28, 2016, 3:37 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/gpu/allocator.cpp, lines 141-161
> > <https://reviews.apache.org/r/51879/diff/6/?file=1510296#file1510296line141>
> >
> >     With the flags, the method here already has all information it needs. Let's
delay the changes to gpus as this review is already large and more work is needed to make
this better.
> 
> Anindya Sinha wrote:
>     If `flags.resources` contains non-gpus resources with no value, then this would fail
here (owing to invalid resources) since the auto-detection logic is contained here but local
to `containerizer.cpp`. Hence, I exposed the optional `Resources` arg for this function. If
present, we just use that here since the auto-detection logic has already been done for the
call-site in that case. If this arg is not provided or is `None()`, we continue with the original
flow.
>     
>     Note that I have not added auto-detection of gpus in this chain.

Ok. However let's still keep the `Try<Resources> NvidiaGpuAllocator::resources(...)`
API because the method is also called from `Try<Containerizer*> Containerizer::create(...)`
and it's awkward to pre-parse the resources there. We can copy over the code that parses `--resources`
flag into a vector.


- Jiang Yan


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


On Oct. 25, 2016, 11:19 a.m., Anindya Sinha wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> -----------------------------------------------------------
> 
> (Updated Oct. 25, 2016, 11: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.
> 
> With this change, JSON or textual 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 in JSON or textual formats
> 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.hpp b2eabfebef99ccebef427d144bb816adc0175ecf

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


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