Re: Review Request 65939: Updated role endpoints for hierarchical accounting.

[Till Toenshoff]

> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 139-140 (patched)
> > 
> >
> > Now that we are keeping track of `allocatedResources` and 
> > `totalAllocatedResources`, we can replace this hashmap with a simple 
> > hashset. We don't seem to be using `Framework*` elsewhere.

There is more uses of the framework pointer.
See e.g. `quota_handler.cpp:259`

```
foreachvalue (const Framework* framework, roleState->frameworks) {
  if (framework->active()) {
++frameworksInRole;
  }
}
```

However, it seems we can replace that after following your advice:
```
foreach (const FrameworkID& frameworkID, roleState->frameworks) {
  CHECK(master->frameworks.registered.contains(frameworkID));
  Framework* framework = master->frameworks.registered.at(frameworkID);

  if (framework->active()) {
++frameworksInRole;
  }
}
```

Seems that the price for that simplification is not that high.


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 178 (patched)
> > 
> >
> > I am not sure if I understand what `returned in ["a", "a/b"]` means.

New version:
```
  // For example: Single framework registered with the role "a/b". That
  // makes "a/b" an active role. The parent role "a" of "a/b" gets
  // implicitly tracked. This function would return two roles; "a" and
  // "a/b".
```


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.hpp
> > Lines 181 (patched)
> > 
> >
> > Would this search the framework in child roles as well? Can we expand 
> > to comment to explicitly mention it? Something like:
> > 
> > ` Checks if the given framework is being tracked under the given role 
> > or its child roles`

The implementation is this
```
bool RoleTrackingTree::hasFramework(
const string& role,
const FrameworkID& id) const
{
  return hasRole(role) && roleMap.at(role).frameworks.contains(id);
}
```
That means it checks `frameworks` and not `total_frameworks`. The latter is the 
aggregated value containing frameworks of child-roles.


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 531 (patched)
> > 
> >
> > s/childless/leaf ?

I removed "childless".


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 572 (patched)
> > 
> >
> > Wondering if we should use `getRole(role)->frameworks.contains(id)` for 
> > uniformity.

getRole() is not const - calling would render `hasFramework` non const.


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 587 (patched)
> > 
> >
> > Do we need a CHECK here, or is that too defensive?

We technically don't need it as `erase` wont throw on an `unordered_set`. 
However it is nice to have nevertheless :)


> On April 6, 2018, 12:25 a.m., Kapil Arya wrote:
> > src/master/master.cpp
> > Lines 619 (patched)
> > 
> >
> > This VLOG seems to stand out. Do we need similar log messages in other 
> > functions (`addResources`, etc.)?

Indeed a debugging artefact - ty.


- Till


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


On April 5, 2018, 4:15 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> ---
> 
> (Updated April 5, 2018, 4:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
> Michael Park, and Meng Zhu.
> 
> 
> 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".
> Adds dynamic hierarchical role tracking to the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto aa63904a33290a3beda162bbc9f44b56ab04a1e7 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   include/mesos/v1/mesos.proto 

Re: Review Request 65939: Updated role endpoints for hierarchical accounting.

[Kapil Arya]

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



Hey Till,

I have added some comments. Most are minor style sthings which you should feel 
free to ignore :-).

I have two high-level comments (we already talked about the first one but 
thought of putting it in here anyways):
1. Backwards compatibility wrt to the Role proto for /roles endpoint and the 
likes. 
2. I noticed that we have `master->roles.getRole()`, etc., scattered all over 
the code and wondering if we should put some of these methods in the Master 
class itself for better readability?


include/mesos/mesos.proto
Line 3407 (original), 3407-3409 (patched)


I wonder how this would affect backwards compatibility. Do we need to keep 
`resources` as is and add `allocated` and `total_allocated` in addition? If we 
do this, we'd probably need to figure out a way to handle old vs new frameworks.



include/mesos/v1/master/master.proto
Lines 458-459 (original), 458-459 (patched)


Please update the corresponding V0 master.proto as well.



src/master/master.hpp
Lines 139-140 (patched)


Now that we are keeping track of `allocatedResources` and 
`totalAllocatedResources`, we can replace this hashmap with a simple hashset. 
We don't seem to be using `Framework*` elsewhere.



src/master/master.hpp
Lines 173-174 (patched)


Can we change the third sentence ("An implicit ... active role.") to 
something like:
An implicit role is not active - frameworks are connected to its child 
roles instead.



src/master/master.hpp
Lines 178 (patched)


I am not sure if I understand what `returned in ["a", "a/b"]` means.



src/master/master.hpp
Lines 181 (patched)


Would this search the framework in child roles as well? Can we expand to 
comment to explicitly mention it? Something like:

` Checks if the given framework is being tracked under the given role or 
its child roles`



src/master/master.cpp
Lines 495-501 (patched)


Just a minor comment. Would it improve readability if we were to switch the 
if condition with the CHECK condition? As in:
```
if (!roleMap.contains(path)) {
  CHECK(!parent->children.contains(path));
  VLOG(1) ...
  ...
}
```



src/master/master.cpp
Lines 519 (patched)


s/stop going up the tree/not removing it

Here and below. That way it applies to the top-level roles as well.



src/master/master.cpp
Lines 520 (patched)


s/break/return/ ?

Here and below.



src/master/master.cpp
Lines 529 (patched)


What if we bring this line after VLOG line. Further, we need a 
`CHECK_NOTNULL(parent);` here?



src/master/master.cpp
Lines 531 (patched)


s/childless/leaf ?



src/master/master.cpp
Lines 572 (patched)


Wondering if we should use `getRole(role)->frameworks.contains(id)` for 
uniformity.



src/master/master.cpp
Lines 587 (patched)


Do we need a CHECK here, or is that too defensive?



src/master/master.cpp
Lines 619 (patched)


This VLOG seems to stand out. Do we need similar log messages in other 
functions (`addResources`, etc.)?


- Kapil Arya


On April 5, 2018, 12:15 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> ---
> 
> (Updated April 5, 2018, 12:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
> Michael Park, and Meng Zhu.
> 
> 
> 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".
> Adds dynamic hierarchical role tracking to the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   

Re: Review Request 65939: Updated role endpoints for hierarchical accounting.

[Till Toenshoff]

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

(Updated April 5, 2018, 4:15 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
Michael Park, and Meng Zhu.


Changes
---

Switched from snapshot view towards constant master owned hierarchical roles 
bookkeeping. Added `total_frameworks` as another inherited role attribute.


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


Repository: mesos


Description (updated)
---

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".
Adds dynamic hierarchical role tracking to the master.


Diffs (updated)
-

  include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
  include/mesos/v1/master/master.proto ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
  include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
  src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
  src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
  src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c 
  src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c 
  src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 
  src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
  src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 


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

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


Testing
---

make check


Thanks,

Till Toenshoff

Re: Review Request 65939: Updated role endpoints for hierarchical accounting.

[Till Toenshoff]

> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3491 (patched)
> > 
> >
> > 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?)

We do make sure that we got no double-slashes via 
https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71 so we 
could indeed use split here.


> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3494-3513 (patched)
> > 
> >
> > 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.

We could do this - so far I hesitated fearing that this duplicates the amount 
of invocations on our resource arithmetics. Let's discuss value vs. price.


> On March 8, 2018, 12:20 a.m., Benjamin Mahler wrote:
> > src/master/http.cpp
> > Lines 3702-3710 (patched)
> > 
> >
> > 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 :)

That is what my updated RR now proposes.


- Till


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


On April 5, 2018, 4:15 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/65939/
> ---
> 
> (Updated April 5, 2018, 4:15 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Kapil Arya, 
> Michael Park, and Meng Zhu.
> 
> 
> 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".
> Adds dynamic hierarchical role tracking to the master.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 676f0b090cad7ebf59eb32556f17ff8b5f247907 
>   include/mesos/v1/master/master.proto 
> ddb28f96b2a3a439bb9a829995a9a3015f65ba43 
>   include/mesos/v1/mesos.proto 10d506517c9f098374ab0c8f4dcfda42e1896c95 
>   src/master/http.cpp 34c9023906eca94965acc994f20e888c1f47b962 
>   src/master/master.hpp 0d9620dd0c232dc1df83477e838eeb7313bf8828 
>   src/master/master.cpp 18382fa93fd0c59e641e00f2028ac1ae2e67c01c 
>   src/master/quota_handler.cpp 21bafd0064e9397f88185eaf687a58f85da94e2c 
>   src/master/weights_handler.cpp 1053652804a8fc6af31a3b8a9f632f836c897fa9 
>   src/tests/api_tests.cpp dd8e221d8fd1b2a241505345337897e4ee4a6347 
>   src/tests/role_tests.cpp a609ed27ef828ca82efc0d269669cda92e5f50a1 
> 
> 
> Diff: https://reviews.apache.org/r/65939/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 65939: Updated role endpoints for hierarchical accounting.

[Benjamin Mahler]

---
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_" 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)


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)


s/resources/allocation/ ?



src/master/http.cpp
Lines 3491 (patched)


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)


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)


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)


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
> 

Review Request 65939: Updated role endpoints for hierarchical accounting.

[Till Toenshoff]

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

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