Changed DRFSorter's representation of inactive clients. DRFSorter previously removed inactive clients from the `clients` collection, and then re-added clients when they were reactivated. This resulted in resetting the allocation count for the client, which is unfortunate. This scheme would also be more difficult to adapt to hierarchical sorting.
This commit changes DRFSorter to continue to store inactive clients in the `clients`; inactive clients are indicated by a new field in the `Client` struct, and are omitted from the return value of `DRFSorter::sort`. Review: https://reviews.apache.org/r/57564 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/93216467 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/93216467 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/93216467 Branch: refs/heads/master Commit: 932164675908bf83224b6d420d61cfc4a2c0e875 Parents: bbe2c6c Author: Neil Conway <[email protected]> Authored: Mon Mar 13 10:18:17 2017 -0700 Committer: Neil Conway <[email protected]> Committed: Wed Apr 26 14:02:04 2017 -0400 ---------------------------------------------------------------------- src/master/allocator/sorter/drf/sorter.cpp | 60 +++++++++++++------------ src/master/allocator/sorter/drf/sorter.hpp | 12 ++--- src/tests/sorter_tests.cpp | 34 ++++++++++---- 3 files changed, 65 insertions(+), 41 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/93216467/src/master/allocator/sorter/drf/sorter.cpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/sorter/drf/sorter.cpp b/src/master/allocator/sorter/drf/sorter.cpp index 85dbff1..9563f58 100644 --- a/src/master/allocator/sorter/drf/sorter.cpp +++ b/src/master/allocator/sorter/drf/sorter.cpp @@ -87,10 +87,8 @@ void DRFSorter::remove(const string& name) CHECK(contains(name)); set<Client, DRFComparator>::iterator it = find(name); - - if (it != clients.end()) { - clients.erase(it); - } + CHECK(it != clients.end()); + clients.erase(it); allocations.erase(name); @@ -105,8 +103,13 @@ void DRFSorter::activate(const string& name) CHECK(contains(name)); set<Client, DRFComparator>::iterator it = find(name); - if (it == clients.end()) { - Client client(name, calculateShare(name), 0); + CHECK(it != clients.end()); + + if (!it->active) { + Client client(*it); + client.active = true; + + clients.erase(it); clients.insert(client); } } @@ -117,13 +120,14 @@ void DRFSorter::deactivate(const string& name) CHECK(contains(name)); set<Client, DRFComparator>::iterator it = find(name); + CHECK(it != clients.end()); + + if (it->active) { + Client client(*it); + client.active = false; - if (it != clients.end()) { - // TODO(benh): Removing the client is an unfortunate strategy - // because we lose information such as the number of allocations - // for this client which means the fairness can be gamed by a - // framework disconnecting and reconnecting. clients.erase(it); + clients.insert(client); } } @@ -145,14 +149,14 @@ void DRFSorter::allocated( { CHECK(contains(name)); - set<Client, DRFComparator>::iterator it = find(name); + // Update the number of allocations that have been made to this + // client. Note that the client might currently be inactive. + // + // TODO(benh): Refactor 'updateShare' to be able to reuse it here. + { + set<Client, DRFComparator>::iterator it = find(name); + CHECK(it != clients.end()); - // The allocator might notify us about an allocation that has been - // made to an inactive sorter client. For example, this happens when - // an agent re-registers that is running tasks for a framework that - // has not yet re-registered. - if (it != clients.end()) { - // TODO(benh): Refactor 'updateShare' to be able to reuse it here. Client client(*it); // Update the 'allocations' to reflect the allocator decision. @@ -409,10 +413,11 @@ vector<string> DRFSorter::sort() } vector<string> result; - result.reserve(clients.size()); foreach (const Client& client, clients) { - result.push_back(client.name); + if (client.active) { + result.push_back(client.name); + } } return result; @@ -434,17 +439,16 @@ int DRFSorter::count() const void DRFSorter::updateShare(const string& name) { set<Client, DRFComparator>::iterator it = find(name); + CHECK(it != clients.end()); - if (it != clients.end()) { - Client client(*it); + Client client(*it); - // Update the 'share' to get proper sorting. - client.share = calculateShare(client.name); + // Update the 'share' to get proper sorting. + client.share = calculateShare(client.name); - // Remove and reinsert it to update the ordering appropriately. - clients.erase(it); - clients.insert(client); - } + // Remove and reinsert it to update the ordering appropriately. + clients.erase(it); + clients.insert(client); } http://git-wip-us.apache.org/repos/asf/mesos/blob/93216467/src/master/allocator/sorter/drf/sorter.hpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/sorter/drf/sorter.hpp b/src/master/allocator/sorter/drf/sorter.hpp index 19fa41b..665eeb0 100644 --- a/src/master/allocator/sorter/drf/sorter.hpp +++ b/src/master/allocator/sorter/drf/sorter.hpp @@ -41,10 +41,11 @@ namespace allocator { struct Client { Client(const std::string& _name, double _share, uint64_t _allocations) - : name(_name), share(_share), allocations(_allocations) {} + : name(_name), share(_share), active(true), allocations(_allocations) {} std::string name; double share; + bool active; // We store the number of times this client has been chosen for // allocation so that we can fairly share the resources across @@ -151,7 +152,7 @@ private: // If true, sort() will recalculate all shares. bool dirty = false; - // The set of active clients (names and shares), sorted by share. + // The set of clients, sorted by share. std::set<Client, DRFComparator> clients; // Maps client names to the weights that should be applied to their shares. @@ -211,9 +212,10 @@ private: hashmap<std::string, Value::Scalar> totals; }; - // Maps client names to the resources they have been allocated. Note - // that `allocations` might contain entries for deactivated clients - // not currently in `clients`. + // Maps client names to the resources they have been allocated. + // + // TODO(neilc): It would be cleaner to store a client's allocation + // in the `Client` struct instead. hashmap<std::string, Allocation> allocations; // Metrics are optionally exposed by the sorter. http://git-wip-us.apache.org/repos/asf/mesos/blob/93216467/src/tests/sorter_tests.cpp ---------------------------------------------------------------------- diff --git a/src/tests/sorter_tests.cpp b/src/tests/sorter_tests.cpp index 8e86e4c..7ca6fca 100644 --- a/src/tests/sorter_tests.cpp +++ b/src/tests/sorter_tests.cpp @@ -227,8 +227,9 @@ TEST(SorterTest, CountAllocations) sorter.add("d"); sorter.add("e"); - // Everyone is allocated the same resources; "c" gets three distinct - // allocations, "d" gets two, and all other clients get one. + // Everyone is allocated the same amount of resources; "c" gets + // three distinct allocations, "d" gets two, and all other clients + // get one. sorter.allocated("a", slaveId, Resources::parse("cpus:3;mem:3").get()); sorter.allocated("b", slaveId, Resources::parse("cpus:3;mem:3").get()); sorter.allocated("c", slaveId, Resources::parse("cpus:1;mem:1").get()); @@ -238,6 +239,7 @@ TEST(SorterTest, CountAllocations) sorter.allocated("d", slaveId, Resources::parse("cpus:1;mem:1").get()); sorter.allocated("e", slaveId, Resources::parse("cpus:3;mem:3").get()); + // Allocation count: {a,b,e} = 1, {d} = 2, {c} = 3. EXPECT_EQ(vector<string>({"a", "b", "e", "d", "c"}), sorter.sort()); // Check that unallocating and re-allocating to a client does not @@ -248,21 +250,37 @@ TEST(SorterTest, CountAllocations) sorter.allocated("c", slaveId, Resources::parse("cpus:3;mem:3").get()); + // Allocation count: {a,b,e} = 1, {d} = 2, {c} = 4. EXPECT_EQ(vector<string>({"a", "b", "e", "d", "c"}), sorter.sort()); - // Deactivating and then re-activating a client currently resets the - // allocation count to zero. - // - // TODO(neilc): Consider changing this behavior. + // Check that deactivating and then re-activating a client does not + // reset the allocation count. sorter.deactivate("c"); sorter.activate("c"); - EXPECT_EQ(vector<string>({"c", "a", "b", "e", "d"}), sorter.sort()); + // Allocation count: {a,b,e} = 1, {d} = 2, {c} = 4. + EXPECT_EQ(vector<string>({"a", "b", "e", "d", "c"}), sorter.sort()); sorter.unallocated("c", slaveId, Resources::parse("cpus:3;mem:3").get()); sorter.allocated("c", slaveId, Resources::parse("cpus:3;mem:3").get()); - EXPECT_EQ(vector<string>({"a", "b", "c", "e", "d"}), sorter.sort()); + // Allocation count: {a,b,e} = 1, {d} = 2, {c} = 5. + EXPECT_EQ(vector<string>({"a", "b", "e", "d", "c"}), sorter.sort()); + + // Check that allocations to an inactive client increase the + // allocation count. + sorter.deactivate("a"); + + sorter.unallocated("a", slaveId, Resources::parse("cpus:1;mem:3").get()); + sorter.allocated("a", slaveId, Resources::parse("cpus:1;mem:3").get()); + + // Allocation count: {b,e} = 1, {d} = 2, {c} = 5. + EXPECT_EQ(vector<string>({"b", "e", "d", "c"}), sorter.sort()); + + sorter.activate("a"); + + // Allocation count: {b,e} = 1, {a,d} = 2, {c} = 5. + EXPECT_EQ(vector<string>({"b", "e", "a", "d", "c"}), sorter.sort()); }
