> On Sept. 28, 2016, 10: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?
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)`. > On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 698-706 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510293#file1510293line698> > > > > `pair[1] = "";` would cause index out of range and leads to undefined > > behavior. > > > > I think you are looking for `pair.add("")`. > > > > However I think it would be more clear to just change the if condition > > to > > > > ``` > > if (pair.size() > 2) { > > return Error("Bad value for resources, extra ':' in " + token); > > } > > ``` > > > > The original error message is actually not accurate I think: you can't > > distinguish between `"cpus:;mem:20"` and `"cpus;mem:20"`. So it's not > > always due to `"missing ':'"` when `pair.size() == 1`. > > > > Not checking `pair.size() == 1` actually works to our benefit as we are > > fine with both `"cpus:;mem:20"` and `"cpus;mem:20"` being valid for > > specifying "no values". Right. Already fixed the `out of range` condition. Also, I think you meant `pair.push_back()` and not `pair.add()`. > On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote: > > src/common/resources.cpp, lines 1994-2011 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510293#file1510293line1994> > > > > No need for this. These calls return the default value if not set. Right. Consider a scalar which has a default value of 0. I want to differentiate between missing value and a explicit value of 0. With the original code, `cpus:` will show up with `cpus:0` which is inaccurate. > On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/containerizer.cpp, lines 273-294 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510294#file1510294line273> > > > > As commented in /r/51999/, the following form avoids redundancy. > > > > ``` > > Try<vector<Resource>> resources = fromJSONString(text, defaultRole); > > > > // Parsing as a simple string if parsing as a JSON string fails. > > if (resources.isError()) { > > resources = fromSimpleString(text, defaultRole); > > } > > > > if (resources.isError()) { > > return Error(resources.error()); > > } > > ``` With this approach, if the input is valid JSON but invalid based on the protobuf (like a `required` parameter missing), we will consider parsing it as a string. I do not think that is correct. I think the steps should be: 1) Parse the input to see if it is a valid JSON. 2) If (1) is true, use `Resources::fromJSON()`. A failure here should mark this input invalid. 3) If (1) is false, use `Resources::fromSimpleString()`. A failure here should mark this input invalid. So, something like: ``` // Try to parse as a JSON Array. Otherwise, parse as a text string. Try<JSON::Array> json = JSON::parse<JSON::Array>(text); Try<vector<Resource>> resources = json.isSome() ? Resources::fromJSON(json.get(), flags.default_role) : Resources::fromSimpleString(text, flags.default_role); ``` > On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/containerizer.cpp, lines 174-177 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510294#file1510294line174> > > > > The need for such a `autoDetect()` method is unclear to me, especially > > when it specially handles each resource kind (which results in a long > > body). We already specially handles each resource kind in > > `Containerizer::resources()` so we shouldn't need to do it again. > > > > See my comments on `Containerizer::resources()`. Refactored as follows: `Containerizer::resources()` calls a static function `parse()` which returns a `Resources` after doing the auto-detection of all known resource types (except gpus). Thereafter, the existing function for gpus, i.e. `NvidiaGpuAllocator::resources(` is called. The reason I moved the auto-detection logic out of `Containerizer::resources()` into a separate static function `parse()` is so that we can use that in `Containerizer::create()` also [we need that since if non-gpus resources in `flags.resources` contains missing value, `NvidiaGpuAllocator::resources()` would fail since auto-detection logic is local to `containerizer.cpp`. Also, a new optional arg `Resources` is added to `NvidiaGpuAllocator::resources()` so that it can use the passed in `Resources` instead of parsing the `flags.resources` in `NvidiaGpuAllocator::resources()`. Or do you think we can take the parsing logic (and the functions it calls) out of `containerizer.cpp` and put them in a common place and let `NvidiaGpuAllocator::resources()` use them and that would not need the additional `Resources` parameter in `NvidiaGpuAllocator::resources()`? > On Sept. 28, 2016, 10:37 p.m., Jiang Yan Xu wrote: > > src/slave/containerizer/containerizer.cpp, line 325 > > <https://reviews.apache.org/r/51879/diff/6/?file=1510294#file1510294line325> > > > > There a couple of things that feel peculiar to me: > > > > 1. The body of Containerizer specially handles each kind of resource, > > which makes sense, but it first calls a generic `parse()`, which internally > > also specially handles each kind of resources. This doesn't reduce the > > complexity for understanding `Containerizer::resources()` as I have to jump > > back-and-forth to understand the workflow. It would be nice if we can > > confine the special processing of each kind of resources to within this > > method. > > 2. `parse()` encapsulates auto-detection but the main body of this > > method is also auto-detection, just from a different source syntactically. > > It would be nice we can do auto-detection from only one method. > > > > > > Would the following workflow be simpler for > > `Containerizer::resources()`? > > > > 1. Parse the flags into a vector of Resource objects. > > 2. For each kind of resources, first figure out the need for > > auto-detection from the two cases, then do auto-detection there. > > 3. Aggregate resources and validate them. > > > > Take 'mem' as an example: > > > > ``` > > // Memory resources. > > vector<Resource> allMem; > > > > // Filter in all mem resources. > > foreach (const Resource& r, resources) { > > if (r.name() == "mem") { > > allMem.push_back(r); > > } > > } > > > > // If no mem is specified, create one with no value for > > auto-detection. > > if (allMem.empty()) { > > allMem.push_back(Resources::parse( > > "mem", > > "", > > flags.default_role).get()); > > } > > > > // Fill in auto-detected mem values. > > foreach (Resource& mem, allMem) { > > if (!mem.has_scalar()) { > > Try<Resource> _mem = > > Resources::parse("mem", systemMemAmount().megabytes(), > > mem.role()); > > CHECK_SOME(_mem); > > mem.CopyFrom(_mem.get()); > > } > > } > > > > resources = allMem.get() + resources.filter( > > [](const Resource& resource) { > > return resource.name() != "mem"; > > }); > > ``` > > > > In addition, it would be great if we can abstract out the common > > structure in the above code block shared by different kinds of resources. > > We can pass in the auto-detection methods like `systemMemAmount()` as an > > argument. > > > > If not, the body of `Resources::resources()` still tells me the > > workflow/algorithm for processing resources with simple helpers for as > > detectors. > > > > Let me know what you think. Refer to my previous comment on the refactor. > On Sept. 28, 2016, 10: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. 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. - Anindya ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/#review150615 ----------------------------------------------------------- On Oct. 6, 2016, 2 a.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51879/ > ----------------------------------------------------------- > > (Updated Oct. 6, 2016, 2 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. For > disk resources, this is not allowed for PATH disks since PATH disks > can be carved into smaller chunks and we cannot assume that the whole > physical disk is available to the PATH disk. > > With this change, JSON or textual representation for disk resources > that specify scalar value of 0 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 3ef8cacee529addc745b4aeb6398d7606c61b749 > include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 > src/common/resources.cpp 0774ff0669e831494d5b12b88e19dfa0a4a3f757 > 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 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 > > Diff: https://reviews.apache.org/r/51879/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
