This is an automated email from the ASF dual-hosted git repository. mzhu pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/mesos.git
commit 62a6c29f190dd95077d2076ea03c9719a7b14c5d Author: Meng Zhu <[email protected]> AuthorDate: Wed May 29 12:35:06 2019 +0200 Removed quota role sorter in the allocator. This patch removes the dedicated quota role sorter in favor of using the same sorting between satisfying guarantees and bursting above guarantees up to limits. The dedicated quota role sorter is tech debt from when a "quota role" was considered different from a "non-quota" role. However, they are the same, one just has a default quota. This helps to simplify the logic in the allocator. Benchmark result shows negligible performance change for clusters with small roles (e.g. 30 and 300 roles). For large number of roles (e.g. 3000 roles), a 5% performance degradation is observed. The patch would result in some behavior change if a cluster is using oversubscribed resources with quota under DRF. Previously, in the quota allocation stage, revocable resources are counted towards neither the total resource pool nor a role's allocated resources when sorting with DRF. This is arguably the right behavior. However, after this patch, all resources, both revocable and non-revocable ones, will be counted when calculating DRF shares in the quota allocation stage. This means, for a quota role that also consumes a lot of revocable resources but no-so-much non-revocable ones, previously it would be sorted towards the head of the queue, now it is likely to be sorted towards the tail of the queue. Review: https://reviews.apache.org/r/70750 --- src/master/allocator/mesos/hierarchical.cpp | 68 +++-------------------------- src/master/allocator/mesos/hierarchical.hpp | 36 +++------------ 2 files changed, 13 insertions(+), 91 deletions(-) diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index 40c8363..df0fa43 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -201,11 +201,7 @@ void HierarchicalAllocatorProcess::initialize( BoundedHashMap<FrameworkID, process::Owned<FrameworkMetrics>>( options.maxCompletedFrameworks); - // Resources for quota'ed roles are allocated separately and prior to - // non-quota'ed roles, hence a dedicated sorter for quota'ed roles is - // necessary. roleSorter->initialize(options.fairnessExcludeResourceNames); - quotaRoleSorter->initialize(options.fairnessExcludeResourceNames); VLOG(1) << "Initialized hierarchical allocator process"; @@ -233,7 +229,6 @@ void HierarchicalAllocatorProcess::recover( // Recovery should start before actual allocation starts. CHECK(initialized); CHECK(slaves.empty()); - CHECK_EQ(0u, quotaRoleSorter->count()); CHECK(_expectedAgentCount >= 0); // If there is no quota, recovery is a no-op. Otherwise, we need @@ -253,7 +248,6 @@ void HierarchicalAllocatorProcess::recover( return; } - // NOTE: `quotaRoleSorter` is updated implicitly in `setQuota()`. foreachpair (const string& role, const Quota& quota, quotas) { setQuota(role, quota); } @@ -595,9 +589,6 @@ void HierarchicalAllocatorProcess::addSlave( sorter->add(slaveId, total); } - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->add(slaveId, total.nonRevocable()); - foreachpair (const FrameworkID& frameworkId, const Resources& allocation, used) { @@ -666,10 +657,6 @@ void HierarchicalAllocatorProcess::removeSlave( sorter->remove(slaveId, slaves.at(slaveId).getTotal()); } - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->remove( - slaveId, slaves.at(slaveId).getTotal().nonRevocable()); - untrackReservations(slaves.at(slaveId).getTotal().reservations()); slaves.erase(slaveId); @@ -927,17 +914,6 @@ void HierarchicalAllocatorProcess::updateAllocation( offeredResources, updatedOfferedResources); - // Update the allocated resources in the quota sorter. We only update - // the allocated resources if this role has quota set. - if (quotaGuarantees.contains(role)) { - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->update( - role, - slaveId, - offeredResources.nonRevocable(), - updatedOfferedResources.nonRevocable()); - } - // Update the agent total resources so they are consistent with the updated // allocation. We do not directly use `updatedOfferedResources` here because // the agent's total resources shouldn't contain: @@ -1041,7 +1017,7 @@ Future<Nothing> HierarchicalAllocatorProcess::updateAvailable( Try<Resources> updatedTotal = slave.getTotal().apply(operations); CHECK_SOME(updatedTotal); - // Update the total resources in the allocator and role and quota sorters. + // Update the total resources in the sorter. updateSlaveTotal(slaveId, updatedTotal.get()); return Nothing(); @@ -1439,19 +1415,6 @@ void HierarchicalAllocatorProcess::setQuota( // allocation group. quotaGuarantees[role] = ResourceQuantities::fromScalarResources(quota.info.guarantee()); - quotaRoleSorter->add(role); - quotaRoleSorter->activate(role); - - // Copy allocation information for the quota'ed role. - if (roleSorter->contains(role)) { - foreachpair ( - const SlaveID& slaveId, - const Resources& resources, - roleSorter->allocation(role)) { - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->allocated(role, slaveId, resources.nonRevocable()); - } - } metrics.setQuota(role, quota); @@ -1475,7 +1438,6 @@ void HierarchicalAllocatorProcess::removeQuota( // Do not allow removing quota if it is not set. CHECK_CONTAINS(quotaGuarantees, role); - CHECK_CONTAINS(*quotaRoleSorter, role); // TODO(alexr): Print all quota info for the role. LOG(INFO) << "Removed quota " << quotaGuarantees[role] << " for role '" @@ -1483,7 +1445,6 @@ void HierarchicalAllocatorProcess::removeQuota( // Remove the role from the quota'ed allocation group. quotaGuarantees.erase(role); - quotaRoleSorter->remove(role); metrics.removeQuota(role); @@ -1503,8 +1464,6 @@ void HierarchicalAllocatorProcess::updateWeights( foreach (const WeightInfo& weightInfo, weightInfos) { CHECK(weightInfo.has_role()); - - quotaRoleSorter->updateWeight(weightInfo.role(), weightInfo.weight()); roleSorter->updateWeight(weightInfo.role(), weightInfo.weight()); } @@ -1844,8 +1803,12 @@ void HierarchicalAllocatorProcess::__allocate() Slave& slave = slaves.at(slaveId); - foreach (const string& role, quotaRoleSorter->sort()) { - CHECK_CONTAINS(quotaGuarantees, role); + foreach (const string& role, roleSorter->sort()) { + // We only allocate to roles with non-default guarantees + // in the first stage. + if (!quotaGuarantees.contains(role)) { + continue; + } const ResourceQuantities& quotaGuarantee = quotaGuarantees.at(role); @@ -2607,9 +2570,6 @@ void HierarchicalAllocatorProcess::untrackFrameworkUnderRole( // for correctness (roles with no registered frameworks will not be offered // any resources), but since many different role names might be used over // time, we want to avoid leaking resources for no-longer-used role names. - // Note that we don't remove the role from `quotaRoleSorter` if it exists - // there, since roles with a quota set still influence allocation even if - // they don't have any registered frameworks. if (roles.at(role).frameworks.empty()) { CHECK_EQ(frameworkSorters.at(role)->count(), 0u); @@ -2716,10 +2676,6 @@ bool HierarchicalAllocatorProcess::updateSlaveTotal( sorter->add(slaveId, total); } - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->remove(slaveId, oldTotal.nonRevocable()); - quotaRoleSorter->add(slaveId, total.nonRevocable()); - return true; } @@ -2842,11 +2798,6 @@ void HierarchicalAllocatorProcess::trackAllocatedResources( roleSorter->allocated(role, slaveId, allocation); frameworkSorters.at(role)->allocated( frameworkId.value(), slaveId, allocation); - - if (quotaGuarantees.contains(role)) { - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->allocated(role, slaveId, allocation.nonRevocable()); - } } } @@ -2878,11 +2829,6 @@ void HierarchicalAllocatorProcess::untrackAllocatedResources( frameworkId.value(), slaveId, allocation); roleSorter->unallocated(role, slaveId, allocation); - - if (quotaGuarantees.contains(role)) { - // See comment at `quotaRoleSorter` declaration regarding non-revocable. - quotaRoleSorter->unallocated(role, slaveId, allocation.nonRevocable()); - } } } diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp index 71a9656..e03e0ea 100644 --- a/src/master/allocator/mesos/hierarchical.hpp +++ b/src/master/allocator/mesos/hierarchical.hpp @@ -52,17 +52,16 @@ namespace allocator { // can typedef an instantiation of it with DRF sorters. template < typename RoleSorter, - typename FrameworkSorter, - typename QuotaRoleSorter> + typename FrameworkSorter> class HierarchicalAllocatorProcess; -typedef HierarchicalAllocatorProcess<DRFSorter, DRFSorter, DRFSorter> +typedef HierarchicalAllocatorProcess<DRFSorter, DRFSorter> HierarchicalDRFAllocatorProcess; typedef MesosAllocator<HierarchicalDRFAllocatorProcess> HierarchicalDRFAllocator; -typedef HierarchicalAllocatorProcess<RandomSorter, RandomSorter, RandomSorter> +typedef HierarchicalAllocatorProcess<RandomSorter, RandomSorter> HierarchicalRandomAllocatorProcess; typedef MesosAllocator<HierarchicalRandomAllocatorProcess> @@ -290,14 +289,12 @@ class HierarchicalAllocatorProcess : public MesosAllocatorProcess public: HierarchicalAllocatorProcess( const std::function<Sorter*()>& roleSorterFactory, - const std::function<Sorter*()>& _frameworkSorterFactory, - const std::function<Sorter*()>& quotaRoleSorterFactory) + const std::function<Sorter*()>& _frameworkSorterFactory) : initialized(false), paused(true), metrics(*this), completedFrameworkMetrics(0), roleSorter(roleSorterFactory()), - quotaRoleSorter(quotaRoleSorterFactory()), frameworkSorterFactory(_frameworkSorterFactory) {} ~HierarchicalAllocatorProcess() override {} @@ -608,25 +605,6 @@ protected: // The total cluster resources are used as the resource pool. process::Owned<Sorter> roleSorter; - // TODO(bmahler): Remove this in favor of either using the same sorting - // between satisfying guarantees and bursting above guarantees up to - // limits, or have a different sorting technique specifically for - // satisfying guarantees (e.g. MESOS-8026). This is tech debt from - // when a "quota role" was considered different from a "non-quota" - // role. However, they are the same, one just has a default quota. - // - // A dedicated sorter for roles that have a non-default quota. - // This sorter determines the order in which guarantees are allocated - // during Level 1 of the first stage. Since only non-revocable - // resources are available for quota, the total cluster non-revocable - // resources are used as the resource pool. - // - // NOTE: A role appears in `quotaRoleSorter` if it has a non-default - // quota (even if no frameworks are currently registered in that role). - // In contrast, `roleSorter` only contains entries for roles with one - // or more registered frameworks. - process::Owned<Sorter> quotaRoleSorter; - // A collection of sorters, one per active role. Each sorter determines // the order in which frameworks that belong to the same role are allocated // resources inside the role's share. These sorters are used during Level 2 @@ -725,8 +703,7 @@ private: // to keep the implementation of the allocator in the implementation file. template < typename RoleSorter, - typename FrameworkSorter, - typename QuotaRoleSorter> + typename FrameworkSorter> class HierarchicalAllocatorProcess : public internal::HierarchicalAllocatorProcess { @@ -737,8 +714,7 @@ public: [this]() -> Sorter* { return new RoleSorter(this->self(), "allocator/mesos/roles/"); }, - []() -> Sorter* { return new FrameworkSorter(); }, - []() -> Sorter* { return new QuotaRoleSorter(); }) {} + []() -> Sorter* { return new FrameworkSorter(); }) {} }; } // namespace allocator {
