Updated the allocator to track allocations via a single code path. A helper was introduced for tracking allocated resources in the sorters. This updates the code to allow the other two copies of this code to use the function.
This also documents some of the addFramework/addSlave cases. Review: https://reviews.apache.org/r/64238 Project: http://git-wip-us.apache.org/repos/asf/mesos/repo Commit: http://git-wip-us.apache.org/repos/asf/mesos/commit/cd450bc4 Tree: http://git-wip-us.apache.org/repos/asf/mesos/tree/cd450bc4 Diff: http://git-wip-us.apache.org/repos/asf/mesos/diff/cd450bc4 Branch: refs/heads/1.4.x Commit: cd450bc41384532811a3a5213cf90d07214d7998 Parents: e2d52d3 Author: Benjamin Mahler <[email protected]> Authored: Thu Nov 30 16:36:42 2017 -0800 Committer: Benjamin Mahler <[email protected]> Committed: Wed May 2 16:43:25 2018 -0700 ---------------------------------------------------------------------- src/master/allocator/mesos/hierarchical.cpp | 117 ++++++++++++----------- src/master/allocator/mesos/hierarchical.hpp | 3 +- 2 files changed, 64 insertions(+), 56 deletions(-) ---------------------------------------------------------------------- http://git-wip-us.apache.org/repos/asf/mesos/blob/cd450bc4/src/master/allocator/mesos/hierarchical.cpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index dc6f27f..640f9ce 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -280,11 +280,16 @@ void HierarchicalAllocatorProcess::addFramework( // Update the allocation for this framework. foreachpair (const SlaveID& slaveId, const Resources& resources, used) { + // TODO(bmahler): The master won't tell us about resources + // allocated to agents that have not yet been added, consider + // CHECKing this case. if (!slaves.contains(slaveId)) { continue; } - trackAllocatedResources(slaveId, {{frameworkId, resources}}); + // The slave struct will already be aware of the allocated + // resources, so we only need to track them in the sorters. + trackAllocatedResources(slaveId, frameworkId, resources); } LOG(INFO) << "Added framework " << frameworkId; @@ -498,13 +503,6 @@ void HierarchicalAllocatorProcess::addSlave( CHECK(!slaves.contains(slaveId)); CHECK(!paused || expectedAgentCount.isSome()); - roleSorter->add(slaveId, total); - - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->add(slaveId, total.nonRevocable()); - - trackAllocatedResources(slaveId, used); - slaves[slaveId] = Slave(); Slave& slave = slaves.at(slaveId); @@ -525,6 +523,35 @@ void HierarchicalAllocatorProcess::addSlave( slave.maintenance = Slave::Maintenance(unavailability.get()); } + roleSorter->add(slaveId, total); + + // See comment at `quotaRoleSorter` declaration regarding non-revocable. + quotaRoleSorter->add(slaveId, total.nonRevocable()); + + foreachpair (const FrameworkID& frameworkId, + const Resources& allocation, + used) { + // There are two cases here: + // + // (1) The framework has already been added to the allocator. + // In this case, we track the allocation in the sorters. + // + // (2) The framework has not yet been added to the allocator. + // The master will imminently add the framework using + // the `FrameworkInfo` recovered from the agent, and in + // the interim we do not track the resources allocated to + // this framework. This leaves a small window where the + // role sorting will under-account for the roles belonging + // to this framework. + // + // TODO(bmahler): Fix the issue outlined in (2). + if (!frameworks.contains(frameworkId)) { + continue; + } + + trackAllocatedResources(slaveId, frameworkId, allocation); + } + // If we have just a number of recovered agents, we cannot distinguish // between "old" agents from the registry and "new" ones joined after // recovery has started. Because we do not persist enough information @@ -1643,14 +1670,7 @@ void HierarchicalAllocatorProcess::__allocate() slave.allocated += resources; - // Resources allocated as part of the quota count towards the - // role's and the framework's fair share. - // - // NOTE: Revocable resources have already been excluded. - frameworkSorter->add(slaveId, resources); - frameworkSorter->allocated(frameworkId_, slaveId, resources); - roleSorter->allocated(role, slaveId, resources); - quotaRoleSorter->allocated(role, slaveId, resources); + trackAllocatedResources(slaveId, frameworkId, resources); } } } @@ -1861,15 +1881,7 @@ void HierarchicalAllocatorProcess::__allocate() slave.allocated += resources; - frameworkSorter->add(slaveId, resources); - frameworkSorter->allocated(frameworkId_, slaveId, resources); - roleSorter->allocated(role, slaveId, resources); - - if (quotas.contains(role)) { - // See comment at `quotaRoleSorter` declaration regarding - // non-revocable. - quotaRoleSorter->allocated(role, slaveId, resources.nonRevocable()); - } + trackAllocatedResources(slaveId, frameworkId, resources); } } } @@ -2373,41 +2385,36 @@ bool HierarchicalAllocatorProcess::isRemoteSlave(const Slave& slave) const void HierarchicalAllocatorProcess::trackAllocatedResources( const SlaveID& slaveId, - const hashmap<FrameworkID, Resources>& used) + const FrameworkID& frameworkId, + const Resources& allocated) { - // Update the allocation for each framework. - foreachpair (const FrameworkID& frameworkId, - const Resources& used_, - used) { - if (!frameworks.contains(frameworkId)) { - continue; - } + CHECK(slaves.contains(slaveId)); + CHECK(frameworks.contains(frameworkId)); - foreachpair (const string& role, - const Resources& allocated, - used_.allocations()) { - // The framework has resources allocated to this role but it may - // or may not be subscribed to the role. Either way, we need to - // track the framework under the role. - if (!isFrameworkTrackedUnderRole(frameworkId, role)) { - trackFrameworkUnderRole(frameworkId, role); - } + // TODO(bmahler): Calling allocations() is expensive since it has + // to construct a map. Avoid this. + foreachpair (const string& role, + const Resources& allocation, + allocated.allocations()) { + // The framework has resources allocated to this role but it may + // or may not be subscribed to the role. Either way, we need to + // track the framework under the role. + if (!isFrameworkTrackedUnderRole(frameworkId, role)) { + trackFrameworkUnderRole(frameworkId, role); + } - // TODO(bmahler): Validate that the reserved resources have the - // framework's role. - CHECK(roleSorter->contains(role)); - CHECK(frameworkSorters.contains(role)); - CHECK(frameworkSorters.at(role)->contains(frameworkId.value())); + CHECK(roleSorter->contains(role)); + CHECK(frameworkSorters.contains(role)); + CHECK(frameworkSorters.at(role)->contains(frameworkId.value())); - roleSorter->allocated(role, slaveId, allocated); - frameworkSorters.at(role)->add(slaveId, allocated); - frameworkSorters.at(role)->allocated( - frameworkId.value(), slaveId, allocated); + roleSorter->allocated(role, slaveId, allocation); + frameworkSorters.at(role)->add(slaveId, allocation); + frameworkSorters.at(role)->allocated( + frameworkId.value(), slaveId, allocation); - if (quotas.contains(role)) { - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->allocated(role, slaveId, allocated.nonRevocable()); - } + if (quotas.contains(role)) { + // See comment at `quotaRoleSorter` declaration regarding non-revocable. + quotaRoleSorter->allocated(role, slaveId, allocation.nonRevocable()); } } } http://git-wip-us.apache.org/repos/asf/mesos/blob/cd450bc4/src/master/allocator/mesos/hierarchical.hpp ---------------------------------------------------------------------- diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp index ececf31..a53a952 100644 --- a/src/master/allocator/mesos/hierarchical.hpp +++ b/src/master/allocator/mesos/hierarchical.hpp @@ -542,7 +542,8 @@ private: // Helper to track used resources on an agent. void trackAllocatedResources( const SlaveID& slaveId, - const hashmap<FrameworkID, Resources>& used); + const FrameworkID& frameworkId, + const Resources& allocated); };
