----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/45961/#review129838 -----------------------------------------------------------
Looks like the SHARE and UNSHARE operations are not in this patch? include/mesos/resources.hpp (lines 424 - 430) <https://reviews.apache.org/r/45961/#comment196776> See my comment at the definition. include/mesos/resources.hpp (lines 513 - 518) <https://reviews.apache.org/r/45961/#comment196777> See my comment at the definition. include/mesos/v1/resources.hpp (lines 424 - 431) <https://reviews.apache.org/r/45961/#comment196778> Ditto. include/mesos/v1/resources.hpp (lines 513 - 518) <https://reviews.apache.org/r/45961/#comment196779> Ditto. src/common/resources.cpp (line 1169) <https://reviews.apache.org/r/45961/#comment196780> Chatted offline but I think we do need to `clear_shared()` and `clear_disk()` for shared resources here. The problem here is that only quantities can be aggregated across slaves and if `clear_shared()` and `clear_disk()` are not called, the resources have identities and cannot be aggregated across slaves. e.g., different slaves can have persistent volumes with the same size and ID but are actually different. src/common/resources.cpp (lines 1311 - 1316) <https://reviews.apache.org/r/45961/#comment196782> Chatted about removing `destroyAllowed` method but here an additional point is that: determining if a shared resource is no longer used and thus can be destroyed is the responsiblity of the master (more specifically the valication.cpp file in it and not the Resource abstraction. Here the concern should be: if the resource is in there, destroy it, return Error otherwise. Therefore the original code is still correct. src/common/resources.cpp (line 1328) <https://reviews.apache.org/r/45961/#comment196784> Should we require persistent volumes to be unshared before we destroy them? Here the concern is that we are clearing things in DESTROY that are not added by its pairing operation CREATE. src/common/resources.cpp (lines 1464 - 1496) <https://reviews.apache.org/r/45961/#comment196785> This concept of weightedGet tightly couples with the sorter logic which is not used elsewhere so it shouldn't be pulled out. Here to have something that's more easily explainable in a generic way, perhaps we can introduce something like ``` // Returns the count of the target Resource in the Resources. size_t Resources::count(const Resource& target) const { // Search the list of resources in the collection. // For shared resources, return the Resource_::sharedCount but because this // is strictly an internal optimzation, the caller just needs to refer to this // as "the count". // For nonshared resources, given how we collapse addable resources, the result // is at most 1. } ``` I feel API is easiler to explain in a general sense for the callers and the implementation is easy to explain for readers of the cpp file without going to through the DRF logic. Chatted with Anindya offline and we are still debating whether we can avoid even having `count` altogether but I am writing down my thoughts here. As to whether we are leaking internal information, I don't feel like this is the case: we are not asking the caller to carefully manipulating the internal state in order to use share resource arithmetics correctly but rather this provides additional information for logic that doesn't deal with resource arithmetic. Therefore I don't think we are breaking abstractions but merely exposing a const `count` method. src/common/resources.cpp (lines 1613 - 1635) <https://reviews.apache.org/r/45961/#comment196786> If we don't need `weightedGet` we don't need this either. src/common/resources.cpp (line 1980) <https://reviews.apache.org/r/45961/#comment196757> I understand that there are cases where we call `<<` on the Resource directly so the `Resource_` version doesn't get called, but perhaps `<SHARED>` pairs better with the shared count `<6>`? src/common/resources_utils.hpp (lines 41 - 45) <https://reviews.apache.org/r/45961/#comment196787> The comments at the definitions. src/common/resources_utils.cpp (lines 71 - 74) <https://reviews.apache.org/r/45961/#comment196789> Do we need `Resources::isShared(resource)`? If we don't need that method, here we can actually just `stripped.clear_shared()`. src/common/resources_utils.cpp (lines 88 - 119) <https://reviews.apache.org/r/45961/#comment196790> In the most recent iteration we can just -= from the call sites right? src/common/resources_utils.cpp (lines 122 - 139) <https://reviews.apache.org/r/45961/#comment196791> This method is basically `Resources& Resources::operator+=(const Resources& that)` except that it doesn't increment the shared count. The method name doesn't suggest this so it could be confusing but moreover it looks like the same logic can be achieved at the call sites using `+=` but "not add stuff if it is already in there". See the comments at the call site in the allocation. src/master/allocator/mesos/hierarchical.cpp (line 445) <https://reviews.apache.org/r/45961/#comment195432> Shouldn't `Resources::sum(used)` already include shared resources? Also, shared resources in `total` are not all `allocated`. Ideally we should only have one copy of the shared resources here even though multiple tasks are using it. Instead of using `sum`, we can just add the resources one by one and check if `slaves[slaveId].allocated.contains(resource)` before adding it. src/master/allocator/mesos/hierarchical.cpp (line 890) <https://reviews.apache.org/r/45961/#comment196800> We can keep using `slaves[slaveId].allocated -= resources;` right? src/master/allocator/mesos/hierarchical.cpp (lines 1267 - 1270) <https://reviews.apache.org/r/45961/#comment195953> Instead of stating nonshared resources are accounted for here and attach shared resources later, we should explain the algorithm here. For shared resources if we just take it from `slaves[slaveId].total`, then `available` becomes: ``` Resources available = (slaves[slaveId].total - slaves[slaveId].allocated).nonshared() + slaves[slaveId].total.shared(); ``` Right? src/master/allocator/mesos/hierarchical.cpp (line 1307) <https://reviews.apache.org/r/45961/#comment195923> According to my comment above, shared resources can be handled together with nonshared resources, i.e., this line above ``` Resources available = slaves[slaveId].total - slaves[slaveId].allocated; ``` is sufficient. Plus there are a few issues if we do it here: 1. Yes currently shared resources happen to be all reserved but it's not the allocator's concern here so we don't need to limit it to reserved(role) for futureproofness and that we don't need to explain this here. 2. Of course we don't want to use resources reserevd for other roles but that's captured in a line above: `Resources resources = (available.unreserved() + available.reserved(role)).nonRevocable();` 3. We do want `allocatable` and `isFiltered` to work with the shared resources as well. (e.g., the framework has filtered the shared resources because it doesn't want it) src/master/allocator/mesos/hierarchical.cpp (lines 1317 - 1326) <https://reviews.apache.org/r/45961/#comment196801> I understand that we should only add shared resources if it is not already in here but we don't need to call a separate helper whose purpose is hard to describe out of the context here. We can just do this: ``` // Remove the shared resources already in 'allocated' first so there's // no duplicates in the result. slaves[slaveId].allocated -= resources.shared(); slaves[slaveId].allocated += resources; ``` src/master/allocator/mesos/hierarchical.cpp (lines 1402 - 1405) <https://reviews.apache.org/r/45961/#comment196129> Ditto to comments on the quota stage. Note that here if we do ``` Resources available = (slaves[slaveId].total - slaves[slaveId].allocated).nonshared() + slaves[slaveId].total.shared(); ``` The same shared resources are added to available again and could be offered to another framework. This is in line with the design that a different offer can go to a different framework in the second stage. Frameworks outside of the role will not see the shared volumes so that's not a concern either. src/master/allocator/mesos/hierarchical.cpp (line 1405) <https://reviews.apache.org/r/45961/#comment196132> Use two space indentation to continue after a `=`. src/master/allocator/mesos/hierarchical.cpp (lines 1470 - 1472) <https://reviews.apache.org/r/45961/#comment196130> Ditto to my comments on the quota stage. src/master/allocator/mesos/hierarchical.cpp (line 1483) <https://reviews.apache.org/r/45961/#comment196131> Ditto to my comments on the quota stage. src/master/allocator/sorter/drf/sorter.hpp (line 22) <https://reviews.apache.org/r/45961/#comment196802> Not needed? src/master/allocator/sorter/drf/sorter.cpp (lines 19 - 25) <https://reviews.apache.org/r/45961/#comment196873> No longer needed? src/master/allocator/sorter/drf/sorter.cpp (line 130) <https://reviews.apache.org/r/45961/#comment196525> Similar to the allocator code: ``` // Remove the shared resources already in 'allocated' first so there's // no duplicates in the result. allocations[name].resources[slaveId] -= resources.shared(); allocations[name].resources[slaveId] += resources; ``` src/master/allocator/sorter/drf/sorter.cpp (lines 132 - 133) <https://reviews.apache.org/r/45961/#comment196526> We should clearly document this in the comments for `Allocation::scalarQuantites`: it only includes nonshared resources because shared resources can only be aggregated during according to the total during `calculateShare()`. src/master/allocator/sorter/drf/sorter.cpp (line 250) <https://reviews.apache.org/r/45961/#comment196869> Compared to `allocate()` this is even simpler here: `allocations[name].resources[slaveId] -= resources;` should just work right? src/master/allocator/sorter/drf/sorter.cpp (lines 252 - 254) <https://reviews.apache.org/r/45961/#comment196870> What does "included in non shareable resources already." mean? src/master/allocator/sorter/drf/sorter.cpp (lines 271 - 274) <https://reviews.apache.org/r/45961/#comment196871> Here we should actually add shared resources into `total_.scalarQuantities` because `total_.scalarQuantities` doesn't change with allocations and we need `total_.scalarQuantities.names()` to return all names of resources in the pool. src/master/allocator/sorter/drf/sorter.cpp (lines 290 - 296) <https://reviews.apache.org/r/45961/#comment196872> Here we should actually remove shared resources from `total_.scalarQuantities`. src/master/allocator/sorter/drf/sorter.cpp (lines 394 - 402) <https://reviews.apache.org/r/45961/#comment196940> `allocations[(*it).name].scalarQuantities` has only nonshared resources. src/master/allocator/sorter/drf/sorter.cpp (line 411) <https://reviews.apache.org/r/45961/#comment196939> It's convenient here that `total_.scalarQuantities` still contains both shared and nonshared quantities. src/master/allocator/sorter/drf/sorter.cpp (lines 427 - 432) <https://reviews.apache.org/r/45961/#comment196876> Since `scalarQuantities` doesn't include any shared resources, this is in fact the allocation for nonshared resources. To add shared resource allocation on top of this one way discussed is to use `contains`, something like: ``` foreachpair (const SlaveID& slaveId, const Resources& resources, allocations[name].resources) { foreach (const Resource& resource, resources.shared()) { unsigned count = 0; foreach (const Client& client, clients) { if (allocations[client.name].resources.contains(slaveId) && allocations[client.name].resources[slaveId].contains(resource)) { ++count; } } CHECK(count > 0); allocation += resource.scalar() / count; } } ``` It feels like it's not optimal that we need to go through all slaves here if very few/no slaves have shared resources on them. Alternatively if we maintain a separate `hashmap<SlaveID, Resources> sharedResources` in `DRFSorter::Allocation` and `DRFSorter::Total` and add a `count` method (described in another comment) in Resources we could do this: ``` foreachpair (const SlaveID& slaveId, const Resources& resources, allocations[name].shareResources) { foreach (const Resource& resource, resources) { size_t count = total_.shareResources[slaveId].count(resource); CHECK(count > 0); allocation += resource.scalar() / count; } } ``` This requires `allocated` and `unallocated` to update `total_.shareResources` though. What do you think? src/master/master.hpp (lines 163 - 183) <https://reviews.apache.org/r/45961/#comment196792> We don't need this in the most recent revision right? src/master/master.cpp (line 3309) <https://reviews.apache.org/r/45961/#comment196793> We should no longer need to pull this up. src/master/master.cpp (line 3327) <https://reviews.apache.org/r/45961/#comment195372> s/incase/in case/ src/master/master.cpp (lines 3334 - 3350) <https://reviews.apache.org/r/45961/#comment195382> I see that you want `offeredResources` to include the most update-to-date shared count in it but instead of handling it specially here can we can always look at the usage in `slave.usedResources` explicitly wherever that information is actually needed. src/master/master.cpp (lines 3789 - 3790) <https://reviews.apache.org/r/45961/#comment195469> We should validate of the operation here, as ``` Option<Error> error = validation::operation::validate( - operation.destroy(), slave->checkpointedResources); + operation.destroy(), + slave->checkpointedResources, + slave->usedResources); ``` src/master/master.cpp (line 3806) <https://reviews.apache.org/r/45961/#comment195567> Good eye :) src/master/master.cpp (line 3807) <https://reviews.apache.org/r/45961/#comment195387> This is probably due to rebasing but the slave->agent renaming is already happening so don't change it back. src/master/master.cpp (lines 3909 - 3912) <https://reviews.apache.org/r/45961/#comment195390> It's not intuitive that you have to -= on part of the resources and += on others. Plus `_offeredResources` is what the master has offered to the framework and in this method we are subtracting what the task is using from the offered resources. Here let's keep the semantics unchanged. If so, then if a task is using a shared volume from an offer, it should still be taken out of the resources the same way as nonshared resources (i.e., -=). Note that when you call `Framework::removeOffer()` and `Slave::removeOffer`, shared resources operators should make sure `Framework::offeredResources`, `Framework::totalOfferedResources` and `Slave::offeredResources` are adjusted correctly for shared resources. src/master/master.cpp (line 3918) <https://reviews.apache.org/r/45961/#comment195410> `task_.resources()` should still be correct even in the presence of shared resources. src/master/master.cpp (lines 3952 - 3961) <https://reviews.apache.org/r/45961/#comment195422> Here I think this semantics is more straightforward: Return the shared resource to the allocator only when the frameowrk is no longer using a shared resource. i.e., ``` Resources recovered = _offeredResources.nonShared(); foreach (const Resource& resource, _offeredResources.shared()) { if (!framework.totalUsedResources.contains(resource) && !framework.totalOfferedResources.contains(resource) { recovered += resource; } } ``` src/master/master.cpp (lines 6757 - 6768) <https://reviews.apache.org/r/45961/#comment196795> Handle it the same way as in `_accept()`. src/master/master.cpp (line 6954) <https://reviews.apache.org/r/45961/#comment195427> No need for the empty line. src/master/validation.cpp (lines 825 - 827) <https://reviews.apache.org/r/45961/#comment195467> This is where we should validate the destroy operation: ``` Option<Error> validate( const Offer::Operation::Destroy& destroy, - const Resources& checkpointedResources) + const Resources& checkpointedResources, + const Resources& usedResources) ``` src/master/validation.cpp (line 842) <https://reviews.apache.org/r/45961/#comment195468> Here ``` if (usedResources.contains(destroy.volumes())) { return Error("Persistent volumes busy"); } ``` - Jiang Yan Xu On April 28, 2016, 5:16 p.m., Anindya Sinha wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/45961/ > ----------------------------------------------------------- > > (Updated April 28, 2016, 5:16 p.m.) > > > Review request for mesos and Jiang Yan Xu. > > > Bugs: MESOS-4431 > https://issues.apache.org/jira/browse/MESOS-4431 > > > Repository: mesos > > > Description > ------- > > Number of consumers for each shared resource is tracked. The count is > incremented on task launch, and decremented on task termination whenever > the task uses a shared resource. > Allow DESTROY for shared volumes only if reference count is 0. > Since shared resources are available to multiple frameworks of the > same role simultaneously, we normalize it with a weight equivalent > to the number of frameworks to which this shared resource has been > allocated to in the sorter. > > > Diffs > ----- > > include/mesos/resources.hpp a557e97c65194d4aad879fb88d8edefd1c95b8d8 > include/mesos/v1/resources.hpp a5ba8fec4c9c3643646308f75a4b28cefe0b3df3 > src/common/resources.cpp f6ff92b591c15bc8e93fd85e1896349c3a7bb968 > src/common/resources_utils.hpp 2840f459288bbe8440eda08119d4f86a8be5a291 > src/common/resources_utils.cpp 8e881a09500b0966b577e514077460f724868a8d > src/master/allocator/mesos/hierarchical.cpp > 0de03c7347e01fde2b42f5ec38a34a62edf642a1 > src/master/allocator/sorter/drf/sorter.hpp > 05d5205d29ad74e01e07c508d88b6f8371541513 > src/master/allocator/sorter/drf/sorter.cpp > 4306973277b9d32356eed31ceabac09fb2a03e6c > src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 > src/master/master.cpp ff41da3d077b65b44277e1bbae88c61b7bb88a3d > src/master/validation.cpp f458100d22ec1f9f10921c1c91b6931a5671e28f > src/v1/resources.cpp 8c3f2d1c1529915a59d47fe37bb3fc7a3267079a > > Diff: https://reviews.apache.org/r/45961/diff/ > > > Testing > ------- > > Tests successful. > > > Thanks, > > Anindya Sinha > >
