This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch 1.8.x in repository https://gitbox.apache.org/repos/asf/mesos.git
commit d39b99256aad43cdc7bc415834a29f280562a97a Author: Andrei Sekretenko <[email protected]> AuthorDate: Wed Jul 3 16:08:33 2019 -0400 Extracted suppression logic in allocator for use in update framework. This patch moves the logic of suppressing/unsuppressing a role set from the inside of 'suppressOffers()'/'reviveOffers()' into separate methods. Specifically, 'reviveOffers()' includes filter clearing logic that we don't want when unsuppressing roles during framework update. For 'supppressOffers()', we need the empty set == all roles semantics, but we don't want that in the suppression logic during framework update. Longer term, the empty set == all roles semantics could be done in the master and we won't need the extra function to provide empty set == all roles logic in the allocator. This is a prerequisite for using thes methods to fix 'updateFramework()' in a subsequent patch. Review: https://reviews.apache.org/r/70994/ --- src/master/allocator/mesos/hierarchical.cpp | 54 +++++++++++++++++++++++------ src/master/allocator/mesos/hierarchical.hpp | 10 ++++++ 2 files changed, 54 insertions(+), 10 deletions(-) diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp index d75ab02..993c62c 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -1357,9 +1357,9 @@ void HierarchicalAllocatorProcess::recoverResources( } -void HierarchicalAllocatorProcess::suppressOffers( +void HierarchicalAllocatorProcess::suppressRoles( const FrameworkID& frameworkId, - const set<string>& roles_) + const set<string>& roles) { CHECK(initialized); CHECK(frameworks.contains(frameworkId)); @@ -1369,7 +1369,6 @@ void HierarchicalAllocatorProcess::suppressOffers( // Deactivating the framework in the sorter is fine as long as // SUPPRESS is not parameterized. When parameterization is added, // we have to differentiate between the cases here. - const set<string>& roles = roles_.empty() ? framework.roles : roles_; foreach (const string& role, roles) { CHECK(frameworkSorters.contains(role)); @@ -1379,12 +1378,14 @@ void HierarchicalAllocatorProcess::suppressOffers( framework.metrics->suppressRole(role); } + // TODO(bmahler): This logs roles that were already suppressed, + // only log roles that transitioned from unsuppressed -> suppressed. LOG(INFO) << "Suppressed offers for roles " << stringify(roles) << " of framework " << frameworkId; } -void HierarchicalAllocatorProcess::reviveOffers( +void HierarchicalAllocatorProcess::suppressOffers( const FrameworkID& frameworkId, const set<string>& roles_) { @@ -1392,24 +1393,57 @@ void HierarchicalAllocatorProcess::reviveOffers( CHECK(frameworks.contains(frameworkId)); Framework& framework = frameworks.at(frameworkId); - framework.inverseOfferFilters.clear(); const set<string>& roles = roles_.empty() ? framework.roles : roles_; + suppressRoles(frameworkId, roles); +} + + +void HierarchicalAllocatorProcess::unsuppressRoles( + const FrameworkID& frameworkId, + const set<string>& roles) +{ + CHECK(initialized); + CHECK(frameworks.contains(frameworkId)); + + Framework& framework = frameworks.at(frameworkId); - // Activating the framework in the sorter on REVIVE is fine as long as + // Activating the framework in the sorter is fine as long as // SUPPRESS is not parameterized. When parameterization is added, // we may need to differentiate between the cases here. foreach (const string& role, roles) { - framework.offerFilters.erase(role); - CHECK(frameworkSorters.contains(role)); - frameworkSorters.at(role)->activate(frameworkId.value()); + frameworkSorters.at(role)->activate(frameworkId.value()); framework.suppressedRoles.erase(role); framework.metrics->reviveRole(role); } - LOG(INFO) << "Revived offers for roles " << stringify(roles) + // TODO(bmahler): This logs roles that were already unsuppressed, + // only log roles that transitioned from suppressed -> unsuppressed. + LOG(INFO) << "Unsuppressed offers for roles " << stringify(roles) + << " of framework " << frameworkId; +} + + +void HierarchicalAllocatorProcess::reviveOffers( + const FrameworkID& frameworkId, + const set<string>& roles_) +{ + CHECK(initialized); + CHECK(frameworks.contains(frameworkId)); + + Framework& framework = frameworks.at(frameworkId); + framework.inverseOfferFilters.clear(); + + const set<string>& roles = roles_.empty() ? framework.roles : roles_; + foreach (const string& role, roles) { + framework.offerFilters.erase(role); + } + + unsuppressRoles(frameworkId, roles); + + LOG(INFO) << "Revived roles " << stringify(roles) << " of framework " << frameworkId; allocate(); diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp index 2ba44d7..4f71682 100644 --- a/src/master/allocator/mesos/hierarchical.hpp +++ b/src/master/allocator/mesos/hierarchical.hpp @@ -648,6 +648,16 @@ private: const FrameworkID& frameworkId, const std::string& role); + // TODO(bmahler): Take a `Framework*` here (and in other functions!), + // it's currently not possible because `Framework` doesn't store the + // framework ID. + void suppressRoles( + const FrameworkID& frameworkId, + const std::set<std::string>& roles); + void unsuppressRoles( + const FrameworkID& frameworkId, + const std::set<std::string>& roles); + // `trackReservations` and `untrackReservations` are helpers // to track role resource reservations. We need to keep // track of reservations to enforce role quota limit
