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 65939: Updated role endpoints for hierarchical accounting.
Date Thu, 08 Mar 2018 00:20:11 GMT

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



Thanks Till! Can you add Meng to any hierachical role related reviews?

I looked through master and agent metrics and agent endpoints and didn't see any other places
where this should be used.

>From an API perspective, I took at look at what cgroups did for flat vs hierarchical information,
look for "total_<counter>" here:
https://www.kernel.org/doc/Documentation/cgroup-v1/memory.txt

We could probably do something similar here, like:

```
{
  "name": "role",
  "weight": 1,
  
  "quota": 1 cpus, 10GB mem, 100GB disk,
  
  "allocation": 0
  "total_allocation": 1 cpus, 10GB mem, 100GB disk,
  
  "frameworks": ..., // frameworks directly subscribed to "role",
  "total_frameworks": ..., // all frameworks subscribed to roles within "role" tree.
}
```

Make sense?

(1) I was also expecting to see the master's `Role::allocatedResources` return the correct
information for the protobuf rather than logic on top having to do the aggregation. Can we
have the `Role` struct in the master be able to return all the information shown above? (you
can tackle the different pieces in separate patches)

(2) This also lets us update the 'Frameworks' column of the Roles table to be hierarchical
via 'total_frameworks', at which point we probably want to call that column something like
'Total Frameworks'. I think that's probably what a user expects to see in the UI? (E.g. "engineering"
contains 10 frameworks, 2 of which are in "engineering/frontend" and 8 of which are in "engineering/backend").

Also, this would be a great opportunity (once we think we're happy with the API change) to
publish it to the dev@ list to give folks a chance to give feedback. We're trying to start
this practice for API changes as part of the API working group.


src/master/http.cpp
Lines 3476-3477 (patched)
<https://reviews.apache.org/r/65939/#comment278980>

    Any reason you avoided being specific about this being an "allocation" tree? (e.g. you
wanted to re-use it for other cases?)
    
    Otherwise, I would suggest calling this:
    
    ```
    // Provides hierarchical accounting of resource allocation.
    // The allocation for a role is the aggregated allocation of
    // its children (and itself since allocations can be made to
    // non-leaf roles).
    //
    // For example:         a (allocation = 1 + 3 = 5)
    //                     / \
    //   (allocation = 1) b   c (allocation = 3)
    //
    // TODO(tillt): This only supports building up the tree and
    // querying allocation. More interface is needed to keep a
    // tree up-to-date as things change.
    class ResourceAllocationTree
    ```



src/master/http.cpp
Lines 3487 (patched)
<https://reviews.apache.org/r/65939/#comment278986>

    s/resources/allocation/ ?



src/master/http.cpp
Lines 3491 (patched)
<https://reviews.apache.org/r/65939/#comment278984>

    Just to be sure, do we disallow double slashes? E.g. "a//b"? If so, then tokenize and
split provide the same functionality here, and I would be inclined to just use split to avoid
having to reason about what the implications of stripping empty tokens are (e.g. it breaks
the lookup logic?)



src/master/http.cpp
Lines 3494-3513 (patched)
<https://reviews.apache.org/r/65939/#comment278988>

    The way this is written the root 'Node' does not contain the sum of its children's allocations,
unlike all other nodes. This doesn't seem like an issue since the API does not allow you to
query for the root allocation. However, might as well make the root 'Node.resources' contain
the right information.



src/master/http.cpp
Lines 3539-3542 (patched)
<https://reviews.apache.org/r/65939/#comment278982>

    Do we need pointers here? It seems to me the only pointer we need is in `nodes`. Then
you won't need to do any `delete`s?



src/master/http.cpp
Lines 3702-3710 (patched)
<https://reviews.apache.org/r/65939/#comment279080>

    Rather than having to do this in the http code, can we have the Role struct directly expose
the aggregated information as well? I'd also like the Role struct to have quota information,
but that's for another patch :)


- Benjamin Mahler


On March 7, 2018, 12:22 a.m., Till Toenshoff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> -----------------------------------------------------------
> 
> (Updated March 7, 2018, 12:22 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, and Michael
Park.
> 
> 
> Bugs: MESOS-8069
>     https://issues.apache.org/jira/browse/MESOS-8069
> 
> 
> Repository: mesos
> 
> 
> Description
> -------
> 
> When listing role "a", resources allocated to role "a/b" are added to
> those allocated to role "a". These changes affect both, the "/roles"
> endpoint as well as the V1 HTTP Call "GET_ROLES".
> 
> 
> Diffs
> -----
> 
>   src/master/http.cpp 6f692e20b0f82dbe5c676739757b9eeaf4d6d49a 
> 
> 
> Diff: https://reviews.apache.org/r/65939/diff/1/
> 
> 
> Testing
> -------
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>


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