----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/51879/#review150615 -----------------------------------------------------------
Partial review. Will continue after discussion. src/common/resources.cpp (lines 473 - 474) <https://reviews.apache.org/r/51879/#comment218636> src/common/resources.cpp (lines 583 - 614) <https://reviews.apache.org/r/51879/#comment218728> 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())); } ``` src/common/resources.cpp (lines 651 - 658) <https://reviews.apache.org/r/51879/#comment218657> Moving it to /r/51999/ makes the review chain more coherent. src/common/resources.cpp (lines 694 - 702) <https://reviews.apache.org/r/51879/#comment218665> `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". src/common/resources.cpp (lines 876 - 880) <https://reviews.apache.org/r/51879/#comment218731> This will make an invalid Resource "not empty" (which is against the assumptions of the current usage). The caller needs to use `validate()` to make sure the resource is valid. We can improve the docuementation to make it clear rather than checking it here. src/common/resources.cpp (lines 1986 - 2003) <https://reviews.apache.org/r/51879/#comment218732> No need for this. These calls return the default value if not set. src/slave/containerizer/containerizer.cpp (lines 135 - 138) <https://reviews.apache.org/r/51879/#comment218805> 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()`. src/slave/containerizer/containerizer.cpp (lines 185 - 189) <https://reviews.apache.org/r/51879/#comment218832> As discussed offline, for simplicity let's uniformly process all disk types. There are already many more things that we don't check other than "it doesn't make sense give PATH disks the full size of the partition" (which actually is not strictly true, they can be carved up but they can still use the whole disk). Since the main reasons for auto-detection here is so we don't have to separately manage each host's disk sizes, we should probably trust the operator to specify PATH disks reasonably. src/slave/containerizer/containerizer.cpp (lines 234 - 255) <https://reviews.apache.org/r/51879/#comment218736> 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()); } ``` src/slave/containerizer/containerizer.cpp (line 286) <https://reviews.apache.org/r/51879/#comment218806> 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. src/slave/containerizer/containerizer.cpp (line 289) <https://reviews.apache.org/r/51879/#comment218733> s/auto detect/auto-detect/ src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 141 - 154) <https://reviews.apache.org/r/51879/#comment218831> 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. - Jiang Yan Xu On Sept. 28, 2016, 12:25 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/51879/ > ----------------------------------------------------------- > > (Updated Sept. 28, 2016, 12:25 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. 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 > c1a87e9e5c07529bc1d077f68477108a40506806 > src/v1/resources.cpp 62a644ebbd13cfc0862bd118ba16c43e0f6aaf90 > > Diff: https://reviews.apache.org/r/51879/diff/ > > > Testing > ------- > > Tests passed. > > > Thanks, > > Anindya Sinha > >
