You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Meng Zhu <mz...@mesosphere.io> on 2017/12/08 22:13:52 UTC

Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/
-----------------------------------------------------------

Review request for mesos, Benjamin Mahler and Michael Park.


Bugs: MESOS-8293
    https://issues.apache.org/jira/browse/MESOS-8293


Repository: mesos


Description
-------

Now before offering unreserved resources to frameworks, the
resources are holdout for the quota headroom until the headroom
is met (reserved resources are offered unaffected).


Diffs
-----

  src/master/allocator/mesos/hierarchical.cpp 9d8b6714d060f67d570c5653798e705248781869 


Diff: https://reviews.apache.org/r/64467/diff/1/


Testing
-------

make check and a dediated test in #64465


Thanks,

Meng Zhu


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Meng Zhu <mz...@mesosphere.io>.

> On Dec. 11, 2017, 9:03 p.m., Benjamin Mahler wrote:
> > Can you also describe how it worked before in the description? I think that would be helpful for the reader and for posterity.
> 
> Meng Zhu wrote:
>     Done.
> 
> Benjamin Mahler wrote:
>     Hm.. did you forget to add it?

Line 1867

Should have included the line number in the reply.


> On Dec. 11, 2017, 9:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1861 (patched)
> > <https://reviews.apache.org/r/64467/diff/3/?file=1912390#file1912390line1901>
> >
> >     Why are you removing shared?
> 
> Meng Zhu wrote:
>     OK, that is redundant. But just so we are in the same page, shared resources should not be hold back for headroom.
> 
> Benjamin Mahler wrote:
>     Can you elaborate? Not that clear to me.

We only hold back unreserved resource for the quota headroom. Reserved resources are not hold back for the quota headroom.  Shared resources currently are also reserved (so they are also not hold back or counted towards the quota headroom). Even shared resources somehow can become unreserved in the future, they should still not be hold back and counted towards the quota headroom.


> On Dec. 11, 2017, 9:03 p.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1931-1939 (original), 1926-1932 (patched)
> > <https://reviews.apache.org/r/64467/diff/3/?file=1912390#file1912390line1972>
> >
> >     Hm.. doesn't the ancestor comment still apply? Your change is altering the sematics?
> >     
> >     It should be the case that "eng/frontend" would get offered a reservation for "eng", but that won't happen if the headroom is not satisfied? It would only go to "eng/frontend" directly in that case?
> 
> Meng Zhu wrote:
>     OK, fixed and kept the original semantics.
>     
>     hm.. looks like resources reserved by my ancestor, while allocatable to me, is not "reserved" by me. I wonder what's the problem of making the notion of reservation heritable, seems to be easier to understand? -- my parents' reservations are also my reservations.
> 
> Benjamin Mahler wrote:
>     > looks like resources reserved by my ancestor, while allocatable to me, is not "reserved" by me
>     
>     Yes, a reservation to "eng" could be allocated to "eng", "eng/frontend", "eng/backend", "eng/c/d/e/f/g", etc.
>     
>     > I wonder what's the problem of making the notion of reservation heritable, seems to be easier to understand? -- my parents' reservations are also my reservations.
>     
>     Not sure I follow, but here's what I'm wondering about:
>     
>     Let's say I have a reservation for "eng". When we have hierarchical quota, let's say we "eng/frontend" that has reached its quota limit and "eng/backend" which has not. I would expect that the "eng" reservation would only be given to "eng/backend". Or if both "eng/frontend" and "eng/backend" have quota satisfied, then I would expect neither of them to get the "eng" reservation. Make sense?

While I agree in the scenarios you described, those are the desired outcome. It makes the definition and reasoning of reservation allocation convoluted especially now it tangles with quota. And I do not our code today does that?

Let's discuss it offline.


- Meng


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/#review193497
-----------------------------------------------------------


On Dec. 12, 2017, 10:54 p.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64467/
> -----------------------------------------------------------
> 
> (Updated Dec. 12, 2017, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-8293
>     https://issues.apache.org/jira/browse/MESOS-8293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now before offering unreserved resources to frameworks, the
> resources are holdout for the quota headroom until the headroom
> is met (reserved resources are offered unaffected).
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 2b2d1fd2802203eba482be2992a5f2756d100cbf 
>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec 
> 
> 
> Diff: https://reviews.apache.org/r/64467/diff/4/
> 
> 
> Testing
> -------
> 
> make check and a dediated test in #64465
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Benjamin Mahler <bm...@apache.org>.

> On Dec. 12, 2017, 5:03 a.m., Benjamin Mahler wrote:
> > Can you also describe how it worked before in the description? I think that would be helpful for the reader and for posterity.
> 
> Meng Zhu wrote:
>     Done.

Hm.. did you forget to add it?


> On Dec. 12, 2017, 5:03 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1861 (patched)
> > <https://reviews.apache.org/r/64467/diff/3/?file=1912390#file1912390line1901>
> >
> >     Why are you removing shared?
> 
> Meng Zhu wrote:
>     OK, that is redundant. But just so we are in the same page, shared resources should not be hold back for headroom.

Can you elaborate? Not that clear to me.


> On Dec. 12, 2017, 5:03 a.m., Benjamin Mahler wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 1931-1939 (original), 1926-1932 (patched)
> > <https://reviews.apache.org/r/64467/diff/3/?file=1912390#file1912390line1972>
> >
> >     Hm.. doesn't the ancestor comment still apply? Your change is altering the sematics?
> >     
> >     It should be the case that "eng/frontend" would get offered a reservation for "eng", but that won't happen if the headroom is not satisfied? It would only go to "eng/frontend" directly in that case?
> 
> Meng Zhu wrote:
>     OK, fixed and kept the original semantics.
>     
>     hm.. looks like resources reserved by my ancestor, while allocatable to me, is not "reserved" by me. I wonder what's the problem of making the notion of reservation heritable, seems to be easier to understand? -- my parents' reservations are also my reservations.

> looks like resources reserved by my ancestor, while allocatable to me, is not "reserved" by me

Yes, a reservation to "eng" could be allocated to "eng", "eng/frontend", "eng/backend", "eng/c/d/e/f/g", etc.

> I wonder what's the problem of making the notion of reservation heritable, seems to be easier to understand? -- my parents' reservations are also my reservations.

Not sure I follow, but here's what I'm wondering about:

Let's say I have a reservation for "eng". When we have hierarchical quota, let's say we "eng/frontend" that has reached its quota limit and "eng/backend" which has not. I would expect that the "eng" reservation would only be given to "eng/backend". Or if both "eng/frontend" and "eng/backend" have quota satisfied, then I would expect neither of them to get the "eng" reservation. Make sense?


- Benjamin


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/#review193497
-----------------------------------------------------------


On Dec. 13, 2017, 6:54 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64467/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2017, 6:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-8293
>     https://issues.apache.org/jira/browse/MESOS-8293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now before offering unreserved resources to frameworks, the
> resources are holdout for the quota headroom until the headroom
> is met (reserved resources are offered unaffected).
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 2b2d1fd2802203eba482be2992a5f2756d100cbf 
>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec 
> 
> 
> Diff: https://reviews.apache.org/r/64467/diff/4/
> 
> 
> Testing
> -------
> 
> make check and a dediated test in #64465
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/#review193497
-----------------------------------------------------------



Can you also describe how it worked before in the description? I think that would be helpful for the reader and for posterity.


src/master/allocator/mesos/hierarchical.cpp
Lines 1834-1840 (patched)
<https://reviews.apache.org/r/64467/#comment272071>

    I found this code and the the stuff above it a little hard to follow, maybe you could have a comment about the formula at the top and then have the code for computing it?



src/master/allocator/mesos/hierarchical.cpp
Line 1873 (original), 1853 (patched)
<https://reviews.apache.org/r/64467/#comment272062>

    Hm.. rather than naming this based on what we use it for, we should probably name it based on the condition it represents?
    
    E.g.
    
    ```
    bool headroomSatisfied = ...;
    
    if (!headroomSatisfied) {
      // allocate only reservations
    } else {
      // allocate all
    }
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1861 (patched)
<https://reviews.apache.org/r/64467/#comment272069>

    Why are you removing shared?



src/master/allocator/mesos/hierarchical.cpp
Lines 1866 (patched)
<https://reviews.apache.org/r/64467/#comment272070>

    Hm.. shouldn't this be a quantity? Otherwise you're comparing a full set of resources against a quantity?
    
    Probably you want some tests to have agents with non-scalar resources (e.g. "ports") to expose this issue?



src/master/allocator/mesos/hierarchical.cpp
Lines 1868-1872 (patched)
<https://reviews.apache.org/r/64467/#comment272066>

    Does this need to be in the if condition above? Seems like it would be an optimization below it. Also, it seems a little brittle that available is declared twice, maybe we should update the same variable?
    
    ```
    bool headroomSatisfied;
    ...
    
    foreach (const SlaveID& slaveId, slaveIds) {
       ...
    
      if (!headroomSatisfied) {
        headroomSatisfied = currentHeadRoom.contains(requiredHeadRoom);
      }
      
      // Until the headroom is satisfied, we hold back the unreserved
      // resources and only offer the reservations.
      Resources available = headroomSatisfied ?
        slave.available() :
        slave.available().reserved();
      
      currentHeadRoom += slave.available().unreserved().toScalarQuantity();
      
      // As an optimization, we skip the agent early if the headroom
      // is not satisfied and there are no reservations to allocate.
      if (!headroomSatisfied && available.reserved().empty()) {
        continue;
      }
      
      foreach (const string& role, roleSorter->sort()) {
         ...
         // allocate
         available -= resources
         ...
      }
    ```



src/master/allocator/mesos/hierarchical.cpp
Lines 1914-1923 (original), 1910-1918 (patched)
<https://reviews.apache.org/r/64467/#comment272067>

    This seems a little distracting from the diff, it looks like an independent cleanup? Could you pull it out so I can get it committed more quickly and we can have a cleaner diff for this change?



src/master/allocator/mesos/hierarchical.cpp
Lines 1931-1939 (original), 1926-1932 (patched)
<https://reviews.apache.org/r/64467/#comment272068>

    Hm.. doesn't the ancestor comment still apply? Your change is altering the sematics?
    
    It should be the case that "eng/frontend" would get offered a reservation for "eng", but that won't happen if the headroom is not satisfied? It would only go to "eng/frontend" directly in that case?



src/tests/hierarchical_allocator_tests.cpp
Lines 3398-3405 (original), 3391-3403 (patched)
<https://reviews.apache.org/r/64467/#comment272061>

    Do you need the non-determinism here?


- Benjamin Mahler


On Dec. 9, 2017, 12:26 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64467/
> -----------------------------------------------------------
> 
> (Updated Dec. 9, 2017, 12:26 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-8293
>     https://issues.apache.org/jira/browse/MESOS-8293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now before offering unreserved resources to frameworks, the
> resources are holdout for the quota headroom until the headroom
> is met (reserved resources are offered unaffected).
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp 9d8b6714d060f67d570c5653798e705248781869 
>   src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec 
> 
> 
> Diff: https://reviews.apache.org/r/64467/diff/3/
> 
> 
> Testing
> -------
> 
> make check and a dediated test in #64465
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Benjamin Mahler <bm...@apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/#review193758
-----------------------------------------------------------



I would suggest re-phrasing the summary to reflect that this is fixing the issue outlined in the description, e.g.

```
Fixed a bug with quota headroom that can leave reservations unallocated.

Now before offering unreserved resources to frameworks, these
resources are held back for the quota headroom until the headroom
is met (reserved resources are offered regardless).

The previous implementation of quota headroom first calculated
the amount of resources that can be allocated without violating the
quota headroom. Then it kept track of newly allocated resources during
the allocation loop. Once the limit is reached, the allocation loop
terminates. This previous implementation had the issue that agents
with unallocated reservations may not get processed due to the early
loop termination, resulting in missing reservation allocations.

See MESOS-8293.
```

Here's a diff based on the comments below, you can apply this locally with `git apply`:

```
diff --git a/src/master/allocator/mesos/hierarchical.cpp b/src/master/allocator/mesos/hierarchical.cpp
index e5b298554..36fca7e7c 100644
--- a/src/master/allocator/mesos/hierarchical.cpp
+++ b/src/master/allocator/mesos/hierarchical.cpp
@@ -1820,58 +1820,50 @@ void HierarchicalAllocatorProcess::__allocate()
   }
 
   // Frameworks in a quota'ed role may temporarily reject resources by
-  // filtering or suppressing offers. Hence quotas may not be fully allocated.
-  // We need to hold back resources for the quota headroom.
+  // filtering or suppressing offers. Hence, quotas may not be fully
+  // allocated and if so we need to hold back some headroom to ensure
+  // the quota can later be satisfied when needed.
   //
-  // Note: Reservations are counted towards one's quota
-  // even when they are currently unallocated. Thus in order to
-  // figure out the quota headroom for the cluster, we need to know:
-  //    (A) total unallocated quota resources.
-  //    (B) total unallocated reserved resources of roles with quota.
+  // Note: Reservations are counted towards one's quota even when
+  // they are currently unallocated since they will defintely be
+  // allocated to the role. The headroom is computed as:
   //
-  // The we have required headroom = (A) - (B).
+  //   Headroom = unallocated quota - unallocated reservations
+  //                                  for roles with quota
 
-  // Calculate total unallocated quota resources.
+  // Calculate how much quota remains unallocated.
   Resources totalUnallocatedQuotaResources;
   foreachpair (const string& name, const Quota& quota, quotas) {
-    // Compute the amount of quota that the role does not have allocated.
-    //
-    // NOTE: Revocable resources are excluded in `quotaRoleSorter`.
-    // NOTE: Only scalars are considered for quota.
+    // Only non-revocable scalar quantities should be involved here.
     Resources allocated = getQuotaRoleAllocatedResources(name);
-    const Resources required = quota.info.guarantee();
-    totalUnallocatedQuotaResources += (required - allocated);
+    const Resources guarantee = quota.info.guarantee();
+
+    totalUnallocatedQuotaResources += (guarantee - allocated);
   }
 
-  // Calculate total unallocated reserved resources of roles with quota.
+  // Calculate how many reserved resources are unallocated for
+  // the roles with quota.
   Resources totalUnallocatedQuotaRoleReservedResources;
   foreachkey (const string& name, quotas) {
-    // A quota role's unallocated-reserved resources equal to
-    // its reservations minus allocated-reservations.
     totalUnallocatedQuotaRoleReservedResources =
       reservationScalarQuantities.get(name).getOrElse(Resources()) -
       allocatedReservationScalarQuantities.get(name).getOrElse(Resources());
   }
 
-  Resources requiredHeadRoom = totalUnallocatedQuotaResources -
+  Resources requiredHeadroom = totalUnallocatedQuotaResources -
     totalUnallocatedQuotaRoleReservedResources;
 
-  bool headroomSatisfied = requiredHeadRoom.empty();
+  bool headroomSatisfied = requiredHeadroom.empty();
 
   // We leave headroom for unallocated quota by "skipping" agents
-  // until the aggregated unreserved-unallocated resources of "skipped"
-  // agents reaches the quota headroom requirement.
-  //
-  // Note: we still allocate reserved resources on "skipped" agents.
-  Resources currentHeadRoom;
+  // until the we've left enough unreserved resources for the headroom.
+  // We still allocate reserved resources on "skipped" agents.
+  Resources currentHeadroom;
 
   foreach (const SlaveID& slaveId, slaveIds) {
     CHECK(slaves.contains(slaveId));
     Slave& slave = slaves.at(slaveId);
 
-    // Calculate the currently available resources on the slave, which
-    // is the difference in non-shared resources between total and
-    // allocated.
     Resources available = slave.available();
 
     // Until the headroom is satisfied, we hold back the unreserved
@@ -1880,12 +1872,10 @@ void HierarchicalAllocatorProcess::__allocate()
       // TODO(mzhu): Make this fine-grained. Only intersection of
       // `requiredHeadRoom - currentHeadRoom` and `available.unreserved()`
       // should be kept for headroom.
-      Resources addToHeadroom =
-        available.unreserved().createStrippedScalarQuantity();
-      currentHeadRoom += addToHeadroom;
-      available -= addToHeadroom;
+      currentHeadroom += available.unreserved().createStrippedScalarQuantity();
+      available -= available.unreserved();
 
-      headroomSatisfied = currentHeadRoom.contains(requiredHeadRoom);
+      headroomSatisfied = currentHeadroom.contains(requiredHeadroom);
     }
 
     // As an optimization, if no resources are left to be allocated
diff --git a/src/tests/hierarchical_allocator_tests.cpp b/src/tests/hierarchical_allocator_tests.cpp
index 6cb3994b8..1a216709e 100644
--- a/src/tests/hierarchical_allocator_tests.cpp
+++ b/src/tests/hierarchical_allocator_tests.cpp
@@ -3335,8 +3335,10 @@ TEST_F(HierarchicalAllocatorTest, QuotaAbsentFramework)
   // NO_QUOTA_ROLE share = 0
   //   framework share = 0
 
+  Resources agentResources = Resources::parse("cpus:2;mem:1024;disk:0").get();
+
   // Each `addSlave()` triggers an event-based allocation.
-  SlaveInfo agent1 = createSlaveInfo("cpus:2;mem:1024;disk:0");
+  SlaveInfo agent1 = createSlaveInfo(agentResources);
   allocator->addSlave(
       agent1.id(),
       agent1,
@@ -3345,6 +3347,9 @@ TEST_F(HierarchicalAllocatorTest, QuotaAbsentFramework)
       agent1.resources(),
       {});
 
+  // Ensure the allocation completes.
+  Clock::settle();
+
   Future<Allocation> allocation = allocations.get();
 
   // All of agent1's resources should be set aside for `QUOTA_ROLE`
@@ -3352,7 +3357,7 @@ TEST_F(HierarchicalAllocatorTest, QuotaAbsentFramework)
   EXPECT_TRUE(allocation.isPending());
 
   // Add agent2 with identical resources.
-  SlaveInfo agent2 = createSlaveInfo("cpus:2;mem:1024;disk:0");
+  SlaveInfo agent2 = createSlaveInfo(agentResources);
   allocator->addSlave(
       agent2.id(),
       agent2,
@@ -3363,25 +3368,20 @@ TEST_F(HierarchicalAllocatorTest, QuotaAbsentFramework)
 
   AWAIT_READY(allocation);
 
-  // Process all triggered allocation events.
-  Clock::settle();
-
   // Agents are randomly shuffled prior to each allocation cycle,
   // either agent1 or agent2 may be set aside for the quota headroom.
   // The other one will be allocated to `framework`.
-  // We don't care which agent is allocated, only that `framework` received one.
+  //
+  // We don't care which agent is allocated, only that the `framework`
+  // received one.
   ASSERT_EQ(1u, allocation->resources.size());
   ASSERT_TRUE(allocation->resources.contains(NO_QUOTA_ROLE));
 
-  Resources allocated = allocatedResources(agent1.resources(), NO_QUOTA_ROLE);
-
-  if (allocated.empty()) {
-    Resources allocated = allocatedResources(agent2.resources(), NO_QUOTA_ROLE);
-  }
-
+  Resources allocated = allocatedResources(agentResources, NO_QUOTA_ROLE);
   EXPECT_EQ(allocated, Resources::sum(allocation->resources.at(NO_QUOTA_ROLE)));
 
-  // The above allocation is the only allocation happened.
+  // The above allocation is the only allocation that happened.
+  Clock::settle();
   EXPECT_TRUE(allocations.get().isPending());
 }
 

```

The test is broken when I ran it. And I realized that what happens is that the test is written to assume that both agents get allocated to in the same cycle. However, with the `Clock::settle` that I added in the diff above, it breaks (which means the test without the `Clock::settle` is flaky) since both agents are allocated to in the same cycle and the headroom computation leaves them both unallocated.


src/master/allocator/mesos/hierarchical.cpp
Lines 1884-1886 (original), 1872-1874 (patched)
<https://reviews.apache.org/r/64467/#comment272399>

    Is this stale now that you don't strip shared here?



src/master/allocator/mesos/hierarchical.cpp
Lines 1883-1886 (patched)
<https://reviews.apache.org/r/64467/#comment272400>

    Hm.. this doesn't look quite right, `currentHeadroom` is a scalar quantity but `available` is not, i.e.
    
    ```
    Resources unreserved = available.unreserved();
    
    currentHeadroom += unreserved.createStrippedScalarQuantity();
    available -= unreserved;
    ```



src/tests/hierarchical_allocator_tests.cpp
Lines 3350-3352 (patched)
<https://reviews.apache.org/r/64467/#comment272401>

    We need to settle before we check this



src/tests/hierarchical_allocator_tests.cpp
Lines 3378-3382 (original), 3376-3380 (patched)
<https://reviews.apache.org/r/64467/#comment272402>

    This seems a little messy and I don't think this empty check can be true?
    
    We could do the following:
    
    ```
    Resources agentResources = Resources::parse("cpus:2;mem:1024;disk:0");
    
    SlaveInfo agent1 = createSlaveInfo(agentResources);
    ...
    SlaveInfo agent2 = createSlaveInfo(agentResources);
    ...
    
    Resources allocated = allocatedResources(agentResources, NO_QUOTA_ROLE);
    ```



src/tests/hierarchical_allocator_tests.cpp
Lines 3384 (patched)
<https://reviews.apache.org/r/64467/#comment272403>

    allocation that happened


- Benjamin Mahler


On Dec. 13, 2017, 6:54 a.m., Meng Zhu wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/64467/
> -----------------------------------------------------------
> 
> (Updated Dec. 13, 2017, 6:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Michael Park.
> 
> 
> Bugs: MESOS-8293
>     https://issues.apache.org/jira/browse/MESOS-8293
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> Now before offering unreserved resources to frameworks, the
> resources are holdout for the quota headroom until the headroom
> is met (reserved resources are offered unaffected).
> 
> A note of previous implementation: we used to first calculate
> the amount of resources that can be allocated wihtout violating the
> quota headroom. Then we keep track of newly allocated resources during
> the allocation loop. Once we hit the limit, the allocation loop is
> terminated. This has the issue that agents with unallocated reservations
> may not get processed due to the early loop termination, resulting in
> missing reservation allocations. See MESOS-8293.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp fccd71c96fe8e4d914e19b5ea8d8f69e9ebf2406 
>   src/tests/hierarchical_allocator_tests.cpp f5fb47ed09682ebdd047aec7e79a86597ee09f53 
> 
> 
> Diff: https://reviews.apache.org/r/64467/diff/5/
> 
> 
> Testing
> -------
> 
> make check and a dediated test in #64465
> 
> 
> Thanks,
> 
> Meng Zhu
> 
>


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/
-----------------------------------------------------------

(Updated Dec. 12, 2017, 10:54 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Changes
-------

Thank you! Patch updated, comments addressed.


Bugs: MESOS-8293
    https://issues.apache.org/jira/browse/MESOS-8293


Repository: mesos


Description
-------

Now before offering unreserved resources to frameworks, the
resources are holdout for the quota headroom until the headroom
is met (reserved resources are offered unaffected).


Diffs (updated)
-----

  src/master/allocator/mesos/hierarchical.cpp 2b2d1fd2802203eba482be2992a5f2756d100cbf 
  src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec 


Diff: https://reviews.apache.org/r/64467/diff/4/

Changes: https://reviews.apache.org/r/64467/diff/3-4/


Testing
-------

make check and a dediated test in #64465


Thanks,

Meng Zhu


Re: Review Request 64467: Rewrote the quota headroom enforcement logic in the allocator.

Posted by Meng Zhu <mz...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/64467/
-----------------------------------------------------------

(Updated Dec. 8, 2017, 4:26 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Bugs: MESOS-8293
    https://issues.apache.org/jira/browse/MESOS-8293


Repository: mesos


Description
-------

Now before offering unreserved resources to frameworks, the
resources are holdout for the quota headroom until the headroom
is met (reserved resources are offered unaffected).


Diffs (updated)
-----

  src/master/allocator/mesos/hierarchical.cpp 9d8b6714d060f67d570c5653798e705248781869 
  src/tests/hierarchical_allocator_tests.cpp 862f4683da04d37d9fe9f471d6ec9cd7751f39ec 


Diff: https://reviews.apache.org/r/64467/diff/2/

Changes: https://reviews.apache.org/r/64467/diff/1-2/


Testing
-------

make check and a dediated test in #64465


Thanks,

Meng Zhu