This is an automated email from the ASF dual-hosted git repository. bmahler pushed a commit to branch 1.7.x in repository https://gitbox.apache.org/repos/asf/mesos.git
commit eb813a457b2f36dcd973e4bf26a49a70d593a7e0 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 a8488e1..82256fb 100644 --- a/src/master/allocator/mesos/hierarchical.cpp +++ b/src/master/allocator/mesos/hierarchical.cpp @@ -1324,9 +1324,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)); @@ -1336,7 +1336,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)); @@ -1346,12 +1345,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_) { @@ -1359,24 +1360,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 170098d..1fce68f 100644 --- a/src/master/allocator/mesos/hierarchical.hpp +++ b/src/master/allocator/mesos/hierarchical.hpp @@ -649,6 +649,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
