mesos-reviews mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Benjamin Mahler <bmah...@apache.org>
Subject Re: Review Request 55625: Prevented certain kinds of gaming the quota system.
Date Mon, 06 Feb 2017 01:41:12 GMT

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


Fix it, then Ship it!




Thanks Benjamin! High level thought here: there seems to be some inconsistent language around
the case of one or more (but not all) of the quota guarantees being met. We should probably
avoid saying that the quota is "satisfied" in the case that not all of the guarantees is reached.
"Partially satisfied" seems more correct but it is ambiguous as to whether this refers to
one of the guarantees having been met vs some allocation but none of the guarantees being
met. "Minimally satisfied" has an implication of there being a "maximally" or "fully satisfied",
this works but then we don't have a way to refer to the case between minimum and maximum.

Looks like the description needs an update to reflect that we no longer special case common
vs scarce resources. E.g.

```
    Prevent gaming of the quota system due to demand:quota ratio mismatch.

    It is currently possible to game the quota system since the allocator
    will perform allocations until the quotas for all resource types are
    satisfied. This can lead to allocations for certain resource type
    far exceeding the set quota.

    In order to prevent this issue, this patch changes the notion of what
    constitutes a "satisfied quota". Where before a quota could only be
    satisfied when allocations had been met up to the set quota for every
    resource type, we now consider a quota satisfied as soon as at least
    one of the resource types is allocated up to the set quota. What this
    means is that if the allocation and quota vectors are of the same
    proportion, this new technique behaves the same as the previous
    technique. However, if the allocation and quota vectors have different
    resource type ratios, then the quota becomes satisfied as soon as
    the any quota guarantee is hit.

    Longer term, we can consider allocating the resource types that have
    not reached the quota guarantee, but this is non-trivial (consider
    the case of having to allocate only memory, this means we likely
    should only offer it in such a manner that it allows an existing
    container to increase its memory; it cannot be used to create a
    new container unless there is a mechanism to obtain best-effort
    cpu/disk).

    Also note that the quota guarantee is currently applied as a limit,
    whereas longer term we would like to (1) make these distinct, and
    (2) distinguish quota and fair sharing via non-revocable and
    revocable resources, respectively.
```


src/master/allocator/mesos/hierarchical.cpp (lines 1368 - 1372)
<https://reviews.apache.org/r/55625/#comment235971>

    Looks like we don't need this after my suggestion below.



src/master/allocator/mesos/hierarchical.cpp (lines 1394 - 1397)
<https://reviews.apache.org/r/55625/#comment235970>

    Hm.. not related to this change but I couldn't follow this TODO? Do you understand it?



src/master/allocator/mesos/hierarchical.cpp (line 1398)
<https://reviews.apache.org/r/55625/#comment235981>

    Per my suggestion above, maybe "someGuaranteesReached" to be a bit clearer that quota
isn't satisfied?



src/master/allocator/mesos/hierarchical.cpp (lines 1400 - 1401)
<https://reviews.apache.org/r/55625/#comment235969>

    Seems we could incorporate this into the existing comment? We should perhaps include a
reference to MESOS-6432 for posterity. Also, maybe a bit of discussion about the longer term:
    
    ```
          // Longer term, we could ideally allocate what remains
          // unsatisfied to allow an existing container to scale
          // vertically, or to allow the launching of a container
          // with best-effort cpus/mem/disk/etc.
    ```



src/master/allocator/mesos/hierarchical.cpp (lines 1530 - 1533)
<https://reviews.apache.org/r/55625/#comment235968>

    Hm.. what was the reasoning for adding this? While it tries to hold back fewer resources
than necessary given the current quota allocation to the role, it seems a bit suprising that
it no longer holds back sufficient resources to satisfy the quota should the allocation change.
    
    E.g.
    
    Quota: 200cpus, 200mem
    Allocation: 100cpus, 200mem
    
    Previously, we would hold back 100 cpus, but with this change we don't hold any back.
So if this role were to re-size its tasks to increase the cpu we didn't hold sufficient resources
back.
    
    I'm inclined to leave this out, let me know your thoughts!



src/tests/hierarchical_allocator_tests.cpp (lines 3538 - 3542)
<https://reviews.apache.org/r/55625/#comment235973>

    How about the following?
    
    ```
    // This tests the behavior of quota when the allocation and
    // quota are disproportionate. The current approach (see MESOS-6432)
    // is to consider the quota satisfied once one of the resource
    // guarantees is reached. We test the following example:
    //
    //        Quota: cpus:4;mem:1024
    //   Allocation: cpus:2;mem:1024
    //
    // Here the quota is considered satisfied since the memory
    // guarantee is reached. Longer term, we could offer the
    // unsatisfied cpus to allow an existing container to scale
    // vertically, or to allow the launching of a container with
    // best-effort memory/disk, etc.
    ```



src/tests/hierarchical_allocator_tests.cpp (line 3543)
<https://reviews.apache.org/r/55625/#comment235972>

    Hm.. minimally might be a bit unclear, how about something like "DisproportionateQuotaVsAllocation"?



src/tests/hierarchical_allocator_tests.cpp (lines 3558 - 3559)
<https://reviews.apache.org/r/55625/#comment235966>

    I was going to suggest storing Resources directly rather than a string, but it seems createQuota
only takes a string, that's unfortunate.



src/tests/hierarchical_allocator_tests.cpp (lines 3573 - 3574)
<https://reviews.apache.org/r/55625/#comment235974>

    Maybe something like this?
    
    "With that QUOTA_ROLE will not receive more resources since we currently do not have an
ability to offer out only the cpus."



src/tests/hierarchical_allocator_tests.cpp (lines 3579 - 3582)
<https://reviews.apache.org/r/55625/#comment235977>

    We can use the allocation equality / construction that was recently introduced:
    
    ```
        Allocation expected = Allocation(
            framework1.id(),
            {{"role1", {{agent1.id(), agent1.resources()}}}});
        
        AWAIT_EXPECT_EQ(expected, allocations.get());
    ```


- Benjamin Mahler


On Jan. 30, 2017, 10:27 p.m., Benjamin Bannier wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55625/
> -----------------------------------------------------------
> 
> (Updated Jan. 30, 2017, 10:27 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Mahler.
> 
> 
> Bugs: MESOS-6432
>     https://issues.apache.org/jira/browse/MESOS-6432
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> In the certain coarse-grained allocation scheme it is possible to game
> quota since the allocator will perform allocations until the quotas
> for all resource kinds are satisfied. This can lead to allocations for
> certain resource kinds far exceeding the set quota.
> 
> This patch changes the notion of what constitutes a "satisfied quota".
> Where before a quota could only be satisfied if for all resource kinds
> allocations had been me up to the set quota, we now consider a quota
> satisfied as soon as at least one resource kinds is allocated up to
> the set quota. We here take only "common" resource kinds into account
> where a common resource is a resource present on every agent node. The
> notion of common resource kinds is needed to avoid strong bias from
> rare resources in the introduced quota allocation scheme.
> 
> Ultimately this change can only be temporary and should be replaced
> with a more complete fix, e.g., chunked allocations, see MESOS-3765.
> 
> 
> Diffs
> -----
> 
>   src/master/allocator/mesos/hierarchical.cpp f471b6848bebae601a7a0509e9c6ad5eab4fa4a2

>   src/tests/hierarchical_allocator_tests.cpp e04d1998679fcf022bb3741676a62da8b01ce97c

> 
> Diff: https://reviews.apache.org/r/55625/diff/
> 
> 
> Testing
> -------
> 
> Tested of various Linux configurations in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>


Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message