> On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote: > > src/common/resources.cpp, lines 1263-1266 > > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1263> > > > > Could you clarify under what conditions this can happen? > > > > It seems surprising to silently drop information explicitly given by > > users, and I believe rejecting the operation would be clearer. If > > situations can occur where a user has more recent information than the > > current master (e.g., an allocation info not yet known to the master) we > > should fix failover behavior.
There are two intake paths for operations: (1) When a framework accepts an offer, and (2) When an operator uses an endpoint to apply an operation (e.g. reserve, create volume, etc). In case (1) we have to apply the operation to the allocated resources. This will fail if the allocation info is incorrect, and if it's absent we will inject it for them prior to applying it (this at least needs to be done for old-style frameworks, but it is done for MULTI_ROLE frameworks as well since it seemed like an easier upgrade path for schedulers since they don't need to ensure they carry or set the allocation info into the operations when they accept). However, we then need to update our internal state for the agent's total and allocated resources. The former has no allocation info and the latter has a variety of allocation infos. Because we have to apply the same operation to both, we either need to handle this within `Resources::apply()` or each call site needs to strip the allocation info prior to applying to the total. I went with the former approach because it seemed easier on the callers, but given the confusion here, perhaps it is clearer to impose the stripping on the callers. I'll add a TODO. Make sense? In case (2) this doesn't occur. > On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote: > > src/common/resources.cpp, line 1288 > > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1288> > > > > This looks like an expensive computation. Should we precompute it at > > function scope and capture it here by ref? Agreed, the issue here is that the current implementation of allocations() CHECK fails if the resources are unallocated, so calling it blindly will crash. For now I'll put a TODO. > On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote: > > src/tests/resources_utils.hpp, line 40 > > <https://reviews.apache.org/r/55828/diff/1/?file=1613179#file1613179line40> > > > > This class seems confusing. It e.g., is not a wrapper since when > > constructing an `AllocatedResources` from some unallocated `Resources` an > > assertion like `EXPECT_EQ(resources, allocated)` is not true (i.e., it > > doesn't just wrap some `Resources` and some role). > > > > What about making this a function for now, e.g., > > > > Resources allocatedResources( > > const Resources& resources, > > const string& role); I'm not sure I followed the point about the EXPECT_EQ, but the intention was to signal to the reader that we may want a type for our flavors of resoures (allocated vs unallocated) in order to prevent invalid operations and the mixing of unallocated and allocated resources. I'll make it a function for now since we don't have a use for the class. > On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote: > > src/common/resources.cpp, line 1272 > > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1272> > > > > Did you intend to make this `const`? If not, the following appears more > > straight-forward to me, > > > > bool isAllocated = false; > > foreach(const Resource& resource, resources) { > > if (resource.has_allocation_info()) { > > isAllocated = true; > > break; > > } > > } > > > > If you'd went for the optimization suggested by Guangya this does > > become a "one-liner" though, > > > > const bool isAllocated = > > resources.resources().empty() > > ? false > > : resources.resources(0).has_allocation_info(); Yeah, all of these sound fine to me, however I was looking to signal to the reader that isAllocated is a function applied to Resources. If we had better enforcement of the invariant that there is no mixing of resources then the optimization would sound good to me. I'll make it const. > On Jan. 30, 2017, 4:40 p.m., Benjamin Bannier wrote: > > src/common/resources.cpp, lines 1268-1270 > > <https://reviews.apache.org/r/55828/diff/1/?file=1613177#file1613177line1268> > > > > `..., see MESOS-6636.` to allow better tracking of the status of this. Hm.. MESOS-6636 is about enforcing that executors and tasks do not mix resources within themselves, which will be enforced at the validation layer. What I'm saying here is that our `Resources` class itself doesn't enforce the invariant of no mixing but our code generally does not do any mixing and likely should not need any mixing because of it being error prone. - Benjamin ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/55828/#review163503 ----------------------------------------------------------- On Jan. 23, 2017, 10:47 p.m., Benjamin Mahler wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/55828/ > ----------------------------------------------------------- > > (Updated Jan. 23, 2017, 10:47 p.m.) > > > Review request for mesos, Benjamin Bannier, Jay Guo, Guangya Liu, and Michael > Park. > > > Bugs: MESOS-6967 > https://issues.apache.org/jira/browse/MESOS-6967 > > > Repository: mesos > > > Description > ------- > > Previously, `Resource` did not contain `AllocationInfo`. So for > backwards compatibility with old schedulers and tooling, we must > allow operations to contain `Resource`s without an allocation role. > The two interesting cases for adjusting the operation's resource are: > > (1) The operation `Resource` does not contain an `AllocationInfo` > but is being applied to an allocated `Resources`. We allow this > only if the operation is unambiguous, that is, the allocated > `Resources` are only allocated to a single role. > > (2) The operation `Resource` contains an `AllocationInfo` but is > being applied to an unallocated `Resources`. In this case we > simply ignore the `AllocationInfo` of the `Resource`. > > Note that we assume no `Resources` store a mix of allocated and > unallocated resources. This is a brittle assumption that we should > have enforcement for. > > > Diffs > ----- > > src/common/resources.cpp be9bca2063e9f0e60c5faa0142077bea56272e45 > src/tests/resources_tests.cpp 8dfb1be35d9f9c6ff69139d055c6b3d3ec475e68 > src/tests/resources_utils.hpp 18dcca7f171102df8fe88f10785f70c5d1cf5b32 > src/v1/resources.cpp da4701c03020ff9c33ef995cd0af437d8827c267 > > Diff: https://reviews.apache.org/r/55828/diff/ > > > Testing > ------- > > Added a test. > > > Thanks, > > Benjamin Mahler > >
