[mesos] 02/04: Refactored framework role tracking logic in the allocator.

[mz...@apache.org]

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 81e10fc023cafda0570658dc7288c7b1f71df618
Author: Meng Zhu <mz...@mesosphere.io>
AuthorDate: Wed Sep 18 15:16:24 2019 -0700

    Refactored framework role tracking logic in the allocator.
    
    A framework is tracked under the role if the framework:
    (1) is subscribed to the role;
    Or
    (2) has resources allocated under the role.
    
    (2) could be true without (1). This is because we allow
    frameworks to update their roles without revoking their
    allocated or offered resources.
    
    This patch recognizes this fact and encapsulates the related
    checks in the helper.
    
    Benchmark result: no major performance impact.
    
    Master
    
    BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/2
    Added 3000 agents in 90.816373ms
    Added 3000 frameworks in 17.202199348secs
    Made 3500 allocations in 15.46714944secs
    Made 0 allocation in 13.767830924secs
    
    Master + this patch:
    
    BENCHMARK_HierarchicalAllocator_WithQuotaParam.LargeAndSmallQuota/2
    Added 3000 agents in 86.782773ms
    Added 3000 frameworks in 18.174924815secs
    Made 3500 allocations in 15.813011454secs
    Made 0 allocation in 12.882917161secs
    
    Review: https://reviews.apache.org/r/71516
---
 src/master/allocator/mesos/hierarchical.cpp | 64 +++++++++++++++--------------
 src/master/allocator/mesos/hierarchical.hpp | 27 ++++++++----
 2 files changed, 54 insertions(+), 37 deletions(-)

diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp
index 256fdce..62750cc 100644
--- a/src/master/allocator/mesos/hierarchical.cpp
+++ b/src/master/allocator/mesos/hierarchical.cpp
@@ -633,7 +633,7 @@ void HierarchicalAllocatorProcess::addFramework(
   const Framework& framework = *CHECK_NOTNONE(getFramework(frameworkId));
 
   foreach (const string& role, framework.roles) {
-    trackFrameworkUnderRole(frameworkId, role);
+    trackFrameworkUnderRole(framework, role);
 
     Sorter* frameworkSorter = CHECK_NOTNONE(getFrameworkSorter(role));
 
@@ -699,7 +699,10 @@ void HierarchicalAllocatorProcess::removeFramework(
       untrackAllocatedResources(slaveId, frameworkId, allocated);
     }
 
-    untrackFrameworkUnderRole(frameworkId, role);
+    CHECK(tryUntrackFrameworkUnderRole(framework, role))
+      << " Framework: " << frameworkId
+      << " role: " << role
+      << " allocation: " << allocation;
   }
 
   // Transfer ownership of this framework's metrics to
@@ -790,7 +793,8 @@ void HierarchicalAllocatorProcess::updateFramework(
     // under the role because a framework can unsubscribe from a role
     // while it still has resources allocated to the role.
     if (!isFrameworkTrackedUnderRole(frameworkId, role)) {
-      trackFrameworkUnderRole(frameworkId, role);
+      // TODO(mzhu): `CHECK` the above case.
+      trackFrameworkUnderRole(framework, role);
     }
 
     frameworkSorters.at(role)->activate(frameworkId.value());
@@ -801,11 +805,7 @@ void HierarchicalAllocatorProcess::updateFramework(
 
     frameworkSorter->deactivate(frameworkId.value());
 
-    // Stop tracking the framework under this role if there are
-    // no longer any resources allocated to it.
-    if (frameworkSorter->allocation(frameworkId.value()).empty()) {
-      untrackFrameworkUnderRole(frameworkId, role);
-    }
+    tryUntrackFrameworkUnderRole(framework, role);
 
     if (framework.offerFilters.contains(role)) {
       framework.offerFilters.erase(role);
@@ -1485,13 +1485,6 @@ void HierarchicalAllocatorProcess::recoverResources(
 
     if (frameworkSorter->contains(frameworkId.value())) {
       untrackAllocatedResources(slaveId, frameworkId, resources);
-
-      // Stop tracking the framework under this role if it's no longer
-      // subscribed and no longer has resources allocated to the role.
-      if ((*framework)->roles.count(role) == 0 &&
-          frameworkSorter->allocation(frameworkId.value()).empty()) {
-        untrackFrameworkUnderRole(frameworkId, role);
-      }
     }
   }
 
@@ -2768,7 +2761,7 @@ const Quota& HierarchicalAllocatorProcess::getQuota(const string& role) const
 
 
 void HierarchicalAllocatorProcess::trackFrameworkUnderRole(
-    const FrameworkID& frameworkId, const string& role)
+    const Framework& framework, const string& role)
 {
   CHECK(initialized);
 
@@ -2792,28 +2785,32 @@ void HierarchicalAllocatorProcess::trackFrameworkUnderRole(
     }
   }
 
-  roleTree.trackFramework(frameworkId, role);
+  roleTree.trackFramework(framework.frameworkId, role);
 
   Sorter* frameworkSorter = CHECK_NOTNONE(getFrameworkSorter(role));
 
-  CHECK_NOT_CONTAINS(*frameworkSorter, frameworkId.value())
+  CHECK_NOT_CONTAINS(*frameworkSorter, framework.frameworkId.value())
     << " for role " << role;
-  frameworkSorter->add(frameworkId.value());
+  frameworkSorter->add(framework.frameworkId.value());
 }
 
 
-void HierarchicalAllocatorProcess::untrackFrameworkUnderRole(
-    const FrameworkID& frameworkId, const string& role)
+bool HierarchicalAllocatorProcess::tryUntrackFrameworkUnderRole(
+    const Framework& framework, const string& role)
 {
   CHECK(initialized);
 
-  roleTree.untrackFramework(frameworkId, role);
-
   Sorter* frameworkSorter = CHECK_NOTNONE(getFrameworkSorter(role));
-
-  CHECK_CONTAINS(*frameworkSorter, frameworkId.value())
+  CHECK_CONTAINS(*frameworkSorter, framework.frameworkId.value())
     << " for role " << role;
-  frameworkSorter->remove(frameworkId.value());
+
+  if (!frameworkSorter->allocation(framework.frameworkId.value()).empty()) {
+    return false;
+  }
+
+  roleTree.untrackFramework(framework.frameworkId, role);
+
+  frameworkSorter->remove(framework.frameworkId.value());
 
   if (roleTree.get(role).isNone() ||
       (*roleTree.get(role))->frameworks().empty()) {
@@ -2821,6 +2818,8 @@ void HierarchicalAllocatorProcess::untrackFrameworkUnderRole(
     roleSorter->remove(role);
     frameworkSorters.erase(role);
   }
+
+  return true;
 }
 
 
@@ -2961,7 +2960,7 @@ void HierarchicalAllocatorProcess::trackAllocatedResources(
     // 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);
+      trackFrameworkUnderRole(*CHECK_NOTNONE(getFramework(frameworkId)), role);
     }
 
     CHECK_CONTAINS(*roleSorter, role);
@@ -2991,7 +2990,7 @@ void HierarchicalAllocatorProcess::untrackAllocatedResources(
   // But currently, a slave is removed first via `removeSlave()`
   // and later a call to `recoverResources()` occurs to recover
   // the framework's resources.
-  CHECK_CONTAINS(frameworks, frameworkId);
+  Framework* framework = CHECK_NOTNONE(getFramework(frameworkId));
 
   // TODO(bmahler): Calling allocations() is expensive since it has
   // to construct a map. Avoid this.
@@ -3007,10 +3006,15 @@ void HierarchicalAllocatorProcess::untrackAllocatedResources(
 
     roleTree.untrackOfferedOrAllocated(allocation);
 
-    frameworkSorter->unallocated(
-        frameworkId.value(), slaveId, allocation);
+    frameworkSorter->unallocated(frameworkId.value(), slaveId, allocation);
 
     roleSorter->unallocated(role, slaveId, allocation);
+
+    // If the framework is no longer subscribed to the role, we can try to
+    // untrack the framework under the role.
+    if (framework->roles.count(role) == 0) {
+      tryUntrackFrameworkUnderRole(*framework, role);
+    }
   }
 }
 
diff --git a/src/master/allocator/mesos/hierarchical.hpp b/src/master/allocator/mesos/hierarchical.hpp
index 5ea3791..36aa2fc 100644
--- a/src/master/allocator/mesos/hierarchical.hpp
+++ b/src/master/allocator/mesos/hierarchical.hpp
@@ -168,7 +168,16 @@ private:
   // By default, weights == DEFAULT_WEIGHT == 1.0.
   double weight_;
 
-  // IDs of the frameworks subscribed to the role, if any.
+  // IDs of the frameworks tracked under the role, if any.
+  // A framework is tracked under the role if the framework:
+  //
+  // (1) is subscribed to the role;
+  // *OR*
+  // (2) has resources allocated under the role.
+  //
+  // NOTE: (2) could be true without (1). This is because the allocator
+  // interface allows for a framework role to be removed without recovering
+  // resources offered or allocated to this role.
   hashset<FrameworkID> frameworks_;
 
   // Total allocated or offered scalar resources to this role, including
@@ -773,13 +782,17 @@ private:
 
   const Quota& getQuota(const std::string& role) const;
 
+  // Helpers to track and untrack a framework under a role.
+  // Frameworks should be tracked under a role either if it subscribes to the
+  // role *OR* it has resources allocated/offered to that role. when neither
+  // conditions are met, it should be untracked.
+  //
+  // `tryUntrackFrameworkUnderRole` returns true if the framework is untracked
+  // under the role.
   void trackFrameworkUnderRole(
-      const FrameworkID& frameworkId,
-      const std::string& role);
-
-  void untrackFrameworkUnderRole(
-      const FrameworkID& frameworkId,
-      const std::string& role);
+      const Framework& framework, const std::string& role);
+  bool tryUntrackFrameworkUnderRole(
+      const Framework& framework, const std::string& role);
 
   void suppressRoles(Framework& framework, const std::set<std::string>& roles);
   void reviveRoles(Framework& framework, const std::set<std::string>& roles);