Re: Review Request 59860: Prevent allocating non-capable agents' resources to hierarchical roles.

[Benjamin Mahler]

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


Fix it, then Ship it!




Modulo existing comments.


src/master/allocator/mesos/hierarchical.cpp
Lines 2086 (patched)


This is in a hot path in the allocation loop. You might want to flip this 
conditional to avoid searching every role when the agents are upgraded to 
hierarchical role:

```
if (!slave.capabilities.hierarchicalRole && strings::contains(role, "/")) {
```



src/master/allocator/mesos/hierarchical.cpp
Lines 2089 (patched)


is not HIERARCHICAL_ROLE capable?



src/tests/upgrade_tests.cpp
Lines 537-542 (patched)


Thanks for describing how the tests work! Very helpful to the reader



src/tests/upgrade_tests.cpp
Lines 562-576 (patched)


This needs to come before you start the slave, no?



src/tests/upgrade_tests.cpp
Lines 578-579 (patched)


I wonder if we could have 0 initial delay by default for the tests so that 
you don't have to deal with this here. Want to file a ticket? :D



src/tests/upgrade_tests.cpp
Lines 610-629 (patched)


In order to simplify this test, we could file another ticket for having a 
set of testing schedulers that have common behavior: no-op, task launching, 
etc. So that we don't have to be as verbose setting up expectations around 
registration and offers. We could have this ticket and the previous one I 
suggested be underneath an epic to simplify our tests. I can also file them if 
you like, let me know!



src/tests/upgrade_tests.cpp
Lines 703-717 (patched)


It would be nice to put these above the starting of the slave, see my 
previous comment about being able to avoid the need to advance the clock.


- Benjamin Mahler


On June 7, 2017, 1:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59860/
> ---
> 
> (Updated June 7, 2017, 1:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7633
> https://issues.apache.org/jira/browse/MESOS-7633
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/tests/upgrade_tests.cpp b07426fa1e402c88a8a647eafdb77f6ebadd9959 
> 
> 
> Diff: https://reviews.apache.org/r/59860/diff/1/
> 
> 
> Testing
> ---
> 
> New tests + `make check`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59930: Support RO mode for bind mount volumes with filesystem/linux isolator

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [59930]

Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' 
CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
./support/docker-build.sh

- Mesos Reviewbot


On June 8, 2017, 11:06 p.m., Charles Raimbert wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59930/
> ---
> 
> (Updated June 8, 2017, 11:06 p.m.)
> 
> 
> Review request for mesos, Jason Lai, Jie Yu, and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The filesystem/linux isolator currently creates all bind mount volumes as RW, 
> even if a volume mode is set as RO.
> 
> The TODO in the isolator code helps to spot the missing capability:
> https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/filesystem/linux.cpp#L587
> `
> // TODO(jieyu): Consider the mode in the volume.
> `
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 69804eec61467ae0fd95dfdf53a08875e27a0bca 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 803758c1437b21df9f25ad7e994298e89de44cbe 
> 
> 
> Diff: https://reviews.apache.org/r/59930/diff/1/
> 
> 
> Testing
> ---
> 
> Test cases for RW and RO bind mount volumes have been added, by following the 
> filesystem/linux isolator's existing testing pattern.
> 
> 
> Thanks,
> 
> Charles Raimbert
> 
>

Re: Review Request 59860: Prevent allocating non-capable agents' resources to hierarchical roles.

[Benjamin Mahler]

> On June 8, 2017, 6:24 p.m., Neil Conway wrote:
> > src/master/allocator/mesos/hierarchical.cpp
> > Lines 2087 (patched)
> > 
> >
> > Do we really want to issue a warning here during every allocation 
> > cycle? This could cause a ton of log messages. (I realize we use the same 
> > log level for the multi-role case, but that is also debatable, IMO.)

It's not getting logged every allocation cycle, it's getting logged every time 
we trigger the filter. Since this only gets logged when there is a framework 
with newer capabilities than an agent, my thinking was that this wouldn't get 
logged at all the vast majority of the time. And when it is getting logged, 
it's presumably temporary while a cluster is getting upgraded. We could look 
into logging this only once per allocation cycle, since the filters can fire *a 
lot* within each allocation cycle in some cases (e.g. all old agents, all new 
frameworks).

In the past this kind of filtering occuring silently (for the 'checkpointing' 
bit) led to a lot of confusion, there was nothing in the logs or the ui to help 
users. So it would be nice to log it to some degree.


- Benjamin


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


On June 7, 2017, 1:54 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59860/
> ---
> 
> (Updated June 7, 2017, 1:54 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7633
> https://issues.apache.org/jira/browse/MESOS-7633
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 8ebdbc6a2b98feab7ce4d7f07b15d8fb92992270 
>   src/tests/upgrade_tests.cpp b07426fa1e402c88a8a647eafdb77f6ebadd9959 
> 
> 
> Diff: https://reviews.apache.org/r/59860/diff/1/
> 
> 
> Testing
> ---
> 
> New tests + `make check`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59960: Add data structures and stats/control helpers for the Blkio cgroup subsystem

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59960]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 9, 2017, 11:24 p.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59960/
> ---
> 
> (Updated June 9, 2017, 11:24 p.m.)
> 
> 
> Review request for mesos, Eric Chung, Xiaojian Huang, Gilbert Song, haosdent 
> huang, Jie Yu, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Data structure for Blkio entities 
> * Stats helpers for `blkio.throttle.io*` (generic blkio stats)
> * Stats helpers for `blkio.io*` (CFQ related stats)
> * Comments from the kernel blkio doc for helper functions
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
>   src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 
> 
> 
> Diff: https://reviews.apache.org/r/59960/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 59859: Added `HIERARCHICAL_ROLE` agent capability.

[Benjamin Mahler]

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


Fix it, then Ship it!




Modulo neil's comments, might be nice to have some more clarity in the comments 
about how hierarchical roles affected the agents (volumes paths?).


include/mesos/mesos.proto
Lines 811 (patched)


This isn't a consumable API so marking it as experimental seems odd?



include/mesos/v1/mesos.proto
Lines 805 (patched)


Ditto here


- Benjamin Mahler


On June 6, 2017, 9:53 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59859/
> ---
> 
> (Updated June 6, 2017, 9:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7632
> https://issues.apache.org/jira/browse/MESOS-7632
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/common/protobuf_utils.cpp 3fcaf786b29a00f003c10b0f1614a2c7eddc725d 
>   src/slave/constants.cpp 0fbcab8f1cee4183dfd4c25984cd27f8baabda44 
>   src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 
>   src/tests/slave_tests.cpp b5141d7013acdd6e236606ef3d9b1953b14d373a 
>   src/tests/upgrade_tests.cpp b07426fa1e402c88a8a647eafdb77f6ebadd9959 
> 
> 
> Diff: https://reviews.apache.org/r/59859/diff/1/
> 
> 
> Testing
> ---
> 
> Updated existing tests / `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 59960: Add data structures and stats/control helpers for the Blkio cgroup subsystem

[Jason Lai]

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

Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie Yu, 
and Zhitao Li.


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


Repository: mesos


Description
---

* Data structure for Blkio entities 
* Stats helpers for `blkio.throttle.io*` (generic blkio stats)
* Stats helpers for `blkio.io*` (CFQ related stats)
* Comments from the kernel blkio doc for helper functions


Diffs
-

  src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
  src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 


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


Testing
---


Thanks,

Jason Lai

Re: Review Request 59552: Add support for explicitly setting bounding capabilities.

[James Peach]

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

(Updated June 9, 2017, 11:09 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebase and partially address review feedback.


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


Repository: mesos


Description
---

The linux/capabilities isolator implements the `--allowed_capabilities`
option by granting all the allowed capabilities. This change explicitly
populates the only the bounding capabilities in the case where
`--bounding_capabilities` has been set but the task itself has not been
granted any effective capabilities. This improves the security of tasks
since it is now possible to configure the bounding set without actually
granting privilege to the task.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
60d22aa877c1ab62a08222e5efe8800e337684da 
  src/slave/containerizer/mesos/launch.cpp 
f48d294a0a832dfe248c4a83849ee5a63cb76bce 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1 


Diff: https://reviews.apache.org/r/59552/diff/3/

Changes: https://reviews.apache.org/r/59552/diff/2-3/


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 59552: Add support for explicitly setting bounding capabilities.

[James Peach]

> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
> > Lines 99-103 (patched)
> > 
> >
> > In fact, i think the semantics should be:
> > 
> > 1) treat framework specificied capabilities as effective capabilities.
> > 2) in the future, allow frameworks to set bounding capabilities
> > 3) if effective capabilities is set and bounding capabilities is not 
> > set, set bounding set to be the same as effective set (instead of 
> > "unbounded"). This matches the existing semantics.
> > 4) treat operator flags as the default values if the framework does not 
> > specify them (for both bounding and effective capabilities).
> > 5) if both effective and bounding capabilities are set, make sure the 
> > effective is a subset of bounding
> > 6) if effective capabilities is not set, but bounding capabilities is 
> > set, set effective capabilities to be the same as bounding capabilities
> > 
> > Given that, I'd adjust the logic of this function to the following:
> > 
> > ```
> > Option effective;
> > Option bounding;
> > 
> > if (containerConfig.has_container_info() &&
> > containerConfig.container_info().has_linux_info() &&
> > 
> > containerConfig.container_info().linux_info().has_capability_info()) {
> >   effective = 
> > containerConfig.container_info().linux_info().capability_info();
> > }
> > 
> > // Framework can override the effective capabilities.
> > if (effective.isNone() && flags.effective_capabilities.isSome()) {
> >   effective = flags.effective_capabilities.get();
> > }
> > 
> > if (flags.bounding_capabilities.isSome()) {
> >   bounding = flags.bounding_capabilities.get();
> > }
> > 
> > if (effective.isSome() && bounding.isSome()) {
> >   // Validate that effective is a subset of bounding.
> > }
> > 
> > if (effective.isSome() && bounding.isNone()) {
> >   bounding = effective.get();
> > }
> > 
> > if (effective.isNone() && bounding.isSome()) {
> >   effective = bounding.get();
> > }
> > 
> > ...
> > ```
> > 
> > The above logic is a bit easier to follow, comparing putting some logic 
> > in `mergeCapabilities`. I'll just kill `mergeCapabilities` function.

Thinking some more about this one :)


> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Line 456 (original), 456-457 (patched)
> > 
> >
> > We probably should have a CHECK (or comment) here. both of them are 
> > either set, or either not set. Maybe add a comment in the flags as well.

The effective and bounding capabilities are currently allowed to vary (see 
below).


> On June 9, 2017, 9:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp
> > Lines 627 (patched)
> > 
> >
> > Since two flags are either both set or both not set. I would simply 
> > check both are isSome() here.

They are actually allowed to vary. If the neither the framework nor the 
operator specified effective capabilities, then 
`launchInfo.has_effective_capabilities()` would be false but 
`launchInfo.has_bounding_capabilities()` could still be true.


- James


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


On June 5, 2017, 4:36 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> ---
> 
> (Updated June 5, 2017, 4:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
> 60d22aa877c1ab62a08222e5efe8800e337684da 
>   src/slave/containerizer/mesos/launch.cpp 
> f48d294a0a832dfe248c4a83849ee5a63cb76bce 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 

Re: Review Request 59552: Add support for explicitly setting bounding capabilities.

[Jie Yu]

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




src/slave/containerizer/mesos/launch.cpp
Line 454 (original), 454 (patched)


I suggest we use Option here. `Result` here is super wierd.



src/slave/containerizer/mesos/launch.cpp
Line 457 (original), 458 (patched)


```
Try _capabilitiesManager = Capabilities::create();
if (_capabilitiesManager.isError()) {
  ...
}

capabilitiesManager = _capabilitiesManager.get();

```



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp
Lines 99-103 (patched)


In fact, i think the semantics should be:

1) treat framework specificied capabilities as effective capabilities.
2) in the future, allow frameworks to set bounding capabilities
3) if effective capabilities is set and bounding capabilities is not set, 
set bounding set to be the same as effective set (instead of "unbounded"). This 
matches the existing semantics.
4) treat operator flags as the default values if the framework does not 
specify them (for both bounding and effective capabilities).
5) if both effective and bounding capabilities are set, make sure the 
effective is a subset of bounding
6) if effective capabilities is not set, but bounding capabilities is set, 
set effective capabilities to be the same as bounding capabilities

Given that, I'd adjust the logic of this function to the following:

```
Option effective;
Option bounding;

if (containerConfig.has_container_info() &&
containerConfig.container_info().has_linux_info() &&
containerConfig.container_info().linux_info().has_capability_info()) {
  effective = 
containerConfig.container_info().linux_info().capability_info();
}

// Framework can override the effective capabilities.
if (effective.isNone() && flags.effective_capabilities.isSome()) {
  effective = flags.effective_capabilities.get();
}

if (flags.bounding_capabilities.isSome()) {
  bounding = flags.bounding_capabilities.get();
}

if (effective.isSome() && bounding.isSome()) {
  // Validate that effective is a subset of bounding.
}

if (effective.isSome() && bounding.isNone()) {
  bounding = effective.get();
}

if (effective.isNone() && bounding.isSome()) {
  effective = bounding.get();
}

...
```

The above logic is a bit easier to follow, comparing putting some logic in 
`mergeCapabilities`. I'll just kill `mergeCapabilities` function.



src/slave/containerizer/mesos/launch.cpp
Line 456 (original), 456-457 (patched)


We probably should have a CHECK (or comment) here. both of them are either 
set, or either not set. Maybe add a comment in the flags as well.



src/slave/containerizer/mesos/launch.cpp
Lines 627 (patched)


Since two flags are either both set or both not set. I would simply check 
both are isSome() here.


- Jie Yu


On June 5, 2017, 4:36 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59552/
> ---
> 
> (Updated June 5, 2017, 4:36 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7476
> https://issues.apache.org/jira/browse/MESOS-7476
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The linux/capabilities isolator implements the `--allowed_capabilities`
> option by granting all the allowed capabilities. This change explicitly
> populates the only the bounding capabilities in the case where
> `--bounding_capabilities` has been set but the task itself has not been
> granted any effective capabilities. This improves the security of tasks
> since it is now possible to configure the bounding set without actually
> granting privilege to the task.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp 
> 60d22aa877c1ab62a08222e5efe8800e337684da 
>   src/slave/containerizer/mesos/launch.cpp 
> f48d294a0a832dfe248c4a83849ee5a63cb76bce 
>   src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
> 40376a03fdb8f931f8d3f83b1c3fa6207e02c1d1 
> 
> 
> Diff: https://reviews.apache.org/r/59552/diff/2/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58510: Added two tests for hierarchical reservation.

[Benjamin Mahler]

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



Thanks for adding tests!


src/tests/hierarchical_allocator_tests.cpp
Lines 4598 (patched)


"descendant roles"? Ditto for the test name



src/tests/hierarchical_allocator_tests.cpp
Lines 4606-4607 (patched)


I was a bit confused about why this test was using quota, then I figured 
out that this is how you wanted to ensure that the allocation goes to the 
descendant.

We should document this at the top of the test, or here, to describe that 
we leverage quota to test this.

Simpler to understand would be to not use quota and add two agents, 
expecting one agent to have been allocated to the ancestor and descendant, 
each. Does that not work?



src/tests/reservation_tests.cpp
Lines 2399-2401 (patched)


These two tests are very similar, the only difference seems to be that one 
tests that the reservations are allocated using "fair sharing" between the 
roles (or at least if you removed quota, that would be the case), and the other 
just tests that it is allocatable to the role?

That seems like a reasonable split to ensure that there is a 
ReservationTest covering this, but it's only accurate if you remove the quota 
from the first test (otherwise these tests are essentially the same?)


- Benjamin Mahler


On April 18, 2017, 3:45 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated April 18, 2017, 3:45 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added two tests for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 33e7b455f8664858eb4f03727b076a10c80cd6e0 
>   src/tests/reservation_tests.cpp 4504831d77c1bfcf5f2ddf6d28cd45dea2c421ad 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 59950: Added tests for `DefaultExecutor` setting `MESOS_CONTAINER_IP`.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59949, 59950]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 9, 2017, 6:36 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59950/
> ---
> 
> (Updated June 9, 2017, 6:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Jie Yu.
> 
> 
> Bugs: MESOS-7631
> https://issues.apache.org/jira/browse/MESOS-7631
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests for `DefaultExecutor` setting `MESOS_CONTAINER_IP`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 565e58ae75e918453e4386f5e35a5a844a8b15f8 
> 
> 
> Diff: https://reviews.apache.org/r/59950/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> [==] Running 2 tests from 2 test cases.
> [--] Global test environment set-up.
> [--] 1 test from HostNetwork/DefaultExecutorCniTest
> [ RUN  ] HostNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0
> I0609 18:16:12.417310 22692 executor.cpp:192] Version: 1.4.0
> I0609 18:16:12.435683 22694 default_executor.cpp:182] Received SUBSCRIBED 
> event
> I0609 18:16:12.441695 22694 default_executor.cpp:186] Subscribed executor on 
> centos7
> I0609 18:16:12.442006 22694 default_executor.cpp:182] Received LAUNCH_GROUP 
> event
> W0609 18:16:12.47 22691 default_executor.cpp:443] Setting 
> `MESOS_CONTAINER_IP` to: 127.0.0.1
> I0609 18:16:12.469161 22691 default_executor.cpp:605] Successfully launched 
> tasks [ 34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6 ] in child containers [ 
> 1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e ]
> I0609 18:16:12.472266 22693 default_executor.cpp:678] Waiting for child 
> container 
> 1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e of 
> task '34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6'
> I0609 18:16:12.481412 22689 default_executor.cpp:182] Received ACKNOWLEDGED 
> event
> I0609 18:16:12.575834 22694 default_executor.cpp:823] Child container 
> 1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e of 
> task '34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6' in state TASK_FINISHED exited 
> with status 0
> I0609 18:16:12.575960 22694 default_executor.cpp:945] Terminating after 1secs
> [   OK ] HostNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0 
> (2366 ms)
> [--] 1 test from HostNetwork/DefaultExecutorCniTest (2368 ms total)
> 
> [--] 1 test from CniNetwork/DefaultExecutorCniTest
> [ RUN  ] CniNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0
> I0609 18:16:13.042444 22763 executor.cpp:192] Version: 1.4.0
> I0609 18:16:13.059947 22759 default_executor.cpp:182] Received SUBSCRIBED 
> event
> I0609 18:16:13.065265 22759 default_executor.cpp:186] Subscribed executor on 
> centos7
> I0609 18:16:13.065783 22759 default_executor.cpp:182] Received LAUNCH_GROUP 
> event
> W0609 18:16:13.067163 22763 default_executor.cpp:443] Setting 
> `MESOS_CONTAINER_IP` to: 10.0.2.15
> I0609 18:16:13.181131 22765 default_executor.cpp:605] Successfully launched 
> tasks [ 1d265a47-6422-458e-b350-9a1091130c40 ] in child containers [ 
> 48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 ]
> I0609 18:16:13.183614 22764 default_executor.cpp:678] Waiting for child 
> container 
> 48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 of 
> task '1d265a47-6422-458e-b350-9a1091130c40'
> I0609 18:16:13.191012 22760 default_executor.cpp:182] Received ACKNOWLEDGED 
> event
> I0609 18:16:13.290139 22763 default_executor.cpp:823] Child container 
> 48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 of 
> task '1d265a47-6422-458e-b350-9a1091130c40' in state TASK_FINISHED exited 
> with status 0
> I0609 18:16:13.290249 22763 default_executor.cpp:945] Terminating after 1secs
> [   OK ] CniNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0 (728 
> ms)
> [--] 1 test from CniNetwork/DefaultExecutorCniTest (729 ms total)
> 
> [--] Global test environment tear-down
> [==] 2 tests from 2 test cases ran. (3105 ms total)
> [  PASSED  ] 2 tests.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 58509: Enabled allocator to handle hierarchical reservation.

[Benjamin Mahler]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp
Lines 1611-1612 (original), 1610-1611 (patched)


This would become:

```
Resources resources = available.allocatableTo(role);
```



src/master/allocator/mesos/hierarchical.cpp
Line 1777 (original), 1775 (patched)


This would become:

```
Resources resources = available.allocatableToRole(role);
if (quotas.contains(role)) {
  resources -= resources.unreserved();
}
```


- Benjamin Mahler


On April 18, 2017, 3:44 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58509/
> ---
> 
> (Updated April 18, 2017, 3:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Other than looking at reservation made for a role, now allocator also
> aggregates reservations of all its ancestor in the hierarchy.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 051f749dd5921a322ca930a042c31814616d38f9 
> 
> 
> Diff: https://reviews.apache.org/r/58509/diff/1/
> 
> 
> Testing
> ---
> 
> see next patch in chain
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 59467: Document new Fetcher metrics.

[James Peach]

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

(Updated June 9, 2017, 6:56 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Document new Fetcher metrics.


Diffs (updated)
-

  docs/monitoring.md cb2833642e7e41c03c98ea92f7300d156a216a2e 


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

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


Testing
---

None.


Thanks,

James Peach

Re: Review Request 59466: Add metrics check to Fetcher tests.

[James Peach]

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

(Updated June 9, 2017, 6:56 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

For each fetcher test, verify that the count of fetches and the count
of errors is what we expect.


Diffs (updated)
-

  src/tests/fetcher_tests.cpp b4124158a0e92f289f0edc0c8cb9394350e1cbb5 


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

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 59464: Add Fetcher task total and failed fetch metrics.

[James Peach]

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

(Updated June 9, 2017, 6:55 p.m.)


Review request for mesos and Joseph Wu.


Changes
---

Rebased and addressed review feedback.


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


Repository: mesos


Description
---

Add the Fetcher metrics to track the number of fetch requests
sent to the Fetcher (`containerizer/fetcher/task_fetches_total`)
and the number of errors reported by the Fetcher
(`containerizer/fetcher/task_fetches_failed`).


Diffs (updated)
-

  src/slave/containerizer/fetcher.hpp efeadbf4b7804ea4c1e443d1e5212e303796ace4 
  src/slave/containerizer/fetcher.cpp 770cad3e046e8a6d58b6bc9176eb7ecdbd340db4 


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

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


Testing
---

make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 58509: Enabled allocator to handle hierarchical reservation.

[Benjamin Mahler]

> On April 24, 2017, 7:25 a.m., Qian Zhang wrote:
> > After this change is introduced, I think we may need to update some docs, 
> > e.g., in 
> > https://github.com/apache/mesos/blob/master/docs/persistent-volume.md, it 
> > is mentioned that a persistent volume might be created by one framework and 
> > then offered to a different framework subscribed to the `same role`, but 
> > now I think it is not just `same role`, instead it should be the role 
> > itself and any its descendant roles.
> 
> Jay Guo wrote:
> Good catch! I actually wonder if this is correct at all... need some 
> further discussion. cc @mcypark @bmahler

I've updated the description of 
[MESOS-7033](https://issues.apache.org/jira/browse/MESOS-7033) to be sure we 
capture this. It does seem unfortunate to me that there is no explicit way to 
bind a reservation/volume to a framework, but this is an existing problem so we 
should tackle it separately. Filed 
[MESOS-7651](https://issues.apache.org/jira/browse/MESOS-7651).


- Benjamin


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


On April 18, 2017, 3:44 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58509/
> ---
> 
> (Updated April 18, 2017, 3:44 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Other than looking at reservation made for a role, now allocator also
> aggregates reservations of all its ancestor in the hierarchy.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 051f749dd5921a322ca930a042c31814616d38f9 
> 
> 
> Diff: https://reviews.apache.org/r/58509/diff/1/
> 
> 
> Testing
> ---
> 
> see next patch in chain
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 59949: Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.

[Avinash sridharan]

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

Review request for mesos, Anand Mazumdar and Jie Yu.


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


Repository: mesos


Description
---

Added `MESOS_CONTAINER_IP` to tasks launched by `DefaultExecutor`.


Diffs
-

  src/launcher/default_executor.cpp 1a60e3bb2b0e8312b349cddedb4cd55716c1b574 


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


Testing
---

make check


Thanks,

Avinash sridharan

Review Request 59950: Added tests for `DefaultExecutor` setting `MESOS_CONTAINER_IP`.

[Avinash sridharan]

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

Review request for mesos, Anand Mazumdar and Jie Yu.


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


Repository: mesos


Description
---

Added tests for `DefaultExecutor` setting `MESOS_CONTAINER_IP`.


Diffs
-

  src/tests/containerizer/cni_isolator_tests.cpp 
565e58ae75e918453e4386f5e35a5a844a8b15f8 


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


Testing
---

make check

[==] Running 2 tests from 2 test cases.
[--] Global test environment set-up.
[--] 1 test from HostNetwork/DefaultExecutorCniTest
[ RUN  ] HostNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0
I0609 18:16:12.417310 22692 executor.cpp:192] Version: 1.4.0
I0609 18:16:12.435683 22694 default_executor.cpp:182] Received SUBSCRIBED event
I0609 18:16:12.441695 22694 default_executor.cpp:186] Subscribed executor on 
centos7
I0609 18:16:12.442006 22694 default_executor.cpp:182] Received LAUNCH_GROUP 
event
W0609 18:16:12.47 22691 default_executor.cpp:443] Setting 
`MESOS_CONTAINER_IP` to: 127.0.0.1
I0609 18:16:12.469161 22691 default_executor.cpp:605] Successfully launched 
tasks [ 34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6 ] in child containers [ 
1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e ]
I0609 18:16:12.472266 22693 default_executor.cpp:678] Waiting for child 
container 
1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e of 
task '34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6'
I0609 18:16:12.481412 22689 default_executor.cpp:182] Received ACKNOWLEDGED 
event
I0609 18:16:12.575834 22694 default_executor.cpp:823] Child container 
1a6e3ada-7e68-43c7-95e2-88978c055c7d.1f0a6c53-8189-4989-8dd1-4e38f28f7f5e of 
task '34ddb6bc-bf93-42ff-aab1-cb06d25c5fc6' in state TASK_FINISHED exited with 
status 0
I0609 18:16:12.575960 22694 default_executor.cpp:945] Terminating after 1secs
[   OK ] HostNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0 (2366 
ms)
[--] 1 test from HostNetwork/DefaultExecutorCniTest (2368 ms total)

[--] 1 test from CniNetwork/DefaultExecutorCniTest
[ RUN  ] CniNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0
I0609 18:16:13.042444 22763 executor.cpp:192] Version: 1.4.0
I0609 18:16:13.059947 22759 default_executor.cpp:182] Received SUBSCRIBED event
I0609 18:16:13.065265 22759 default_executor.cpp:186] Subscribed executor on 
centos7
I0609 18:16:13.065783 22759 default_executor.cpp:182] Received LAUNCH_GROUP 
event
W0609 18:16:13.067163 22763 default_executor.cpp:443] Setting 
`MESOS_CONTAINER_IP` to: 10.0.2.15
I0609 18:16:13.181131 22765 default_executor.cpp:605] Successfully launched 
tasks [ 1d265a47-6422-458e-b350-9a1091130c40 ] in child containers [ 
48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 ]
I0609 18:16:13.183614 22764 default_executor.cpp:678] Waiting for child 
container 
48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 of 
task '1d265a47-6422-458e-b350-9a1091130c40'
I0609 18:16:13.191012 22760 default_executor.cpp:182] Received ACKNOWLEDGED 
event
I0609 18:16:13.290139 22763 default_executor.cpp:823] Child container 
48fc4118-8637-4564-9a8f-73de5712e277.69464142-e313-4c8e-859c-cd1d2cce4e26 of 
task '1d265a47-6422-458e-b350-9a1091130c40' in state TASK_FINISHED exited with 
status 0
I0609 18:16:13.290249 22763 default_executor.cpp:945] Terminating after 1secs
[   OK ] CniNetwork/DefaultExecutorCniTest.ROOT_VerifyContainerIP/0 (728 ms)
[--] 1 test from CniNetwork/DefaultExecutorCniTest (729 ms total)

[--] Global test environment tear-down
[==] 2 tests from 2 test cases ran. (3105 ms total)
[  PASSED  ] 2 tests.


Thanks,

Avinash sridharan

Re: Review Request 58508: Introduced method `Resources::inheritable` to aggregate reservations.

[Benjamin Mahler]

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



Per an offline discussion with mpark, I'm not sure that it will be clear what 
'inheritable' means, the following seems more clear:

```
resources.allocatableTo(role);
```

This would include unreserved resources, but does not include resources 
reserved to a non-ancestor role. Then in the allocator code, if we want to 
distinguish between unreserved resources and reserved resources we could do the 
following:

```
Resources reserved = resources.allocatableTo(role) - resources.unreserved();
```

What I find a bit confusing about adding something like 
`inheritablyReservedToRole(role)` is that it suggests that a reservation has 
been inherited to this role, whereas the reality is that there is a reservation 
to an ancestor and we simply are allowed to be allocated the reserved 
resources, but we don't have a reservation on this role.


src/tests/resources_tests.cpp
Lines 1540-1541 (patched)


It seems a little odd that "bx" is the father and "b" is the uncle, I would 
expect that "b" is the father and maybe something like "b2" is the uncle?


- Benjamin Mahler


On April 20, 2017, 4:18 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58508/
> ---
> 
> (Updated April 20, 2017, 4:18 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A reservation is inheritable by 'role' iff:
>  - it is reserved to ancestor of 'role' in hierarchy, OR
>  - it is reserved to 'role' itself.
> 
> This is a helper function for reservation delegate, where reserved
> resources can be 'delegated' downwards in the hierarchy.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 74583e3c784f22b45cce81f07c5a69b339e1684d 
>   src/common/resources.cpp 77bac0c4390da905016a942991b14a79877f8455 
>   src/tests/resources_tests.cpp 4cf320d802a749f1419ac5b9f63b6c73b0c974be 
> 
> 
> Diff: https://reviews.apache.org/r/58508/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 59874: Made `CheckerProcess` use a consistent description for log messages.

[Alexander Rukletsov]

> On June 9, 2017, 3:48 a.m., Mesos Reviewbot Windows wrote:
> > Bad patch!
> > 
> > Reviews applied: [59874, 59902, 59873, 59872, 59871]
> > 
> > Failed command: python support/apply-reviews.py -n -r 59871
> > 
> > Error:
> > Traceback (most recent call last):
> >   File "support/apply-reviews.py", line 381, in 
> > reviewboard()
> >   File "support/apply-reviews.py", line 360, in reviewboard
> > apply_review()
> >   File "support/apply-reviews.py", line 126, in apply_review
> > commit_patch()
> >   File "support/apply-reviews.py", line 225, in commit_patch
> > shell(cmd, options['dry_run'])
> >   File "support/apply-reviews.py", line 111, in shell
> > error_code = subprocess.call(command, stderr=subprocess.STDOUT, 
> > shell=True)
> >   File "C:\Python27\lib\subprocess.py", line 168, in call
> > return Popen(*popenargs, **kwargs).wait()
> >   File "C:\Python27\lib\subprocess.py", line 390, in __init__
> > errread, errwrite)
> >   File "C:\Python27\lib\subprocess.py", line 610, in _execute_child
> > args = '{} /c "{}"'.format (comspec, args)
> > UnicodeEncodeError: 'ascii' codec can't encode character u'\xf3' in 
> > position 25: ordinal not in range(128)
> > 
> > Full log: http://mesos-winbot.westus.cloudapp.azure.com/logs/80/console

Not sure what this means. I'll ask around but ignore it for now.


- Alexander


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


On June 9, 2017, 3:43 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59874/
> ---
> 
> (Updated June 9, 2017, 3:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7092
> https://issues.apache.org/jira/browse/MESOS-7092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `CheckerProcess` use a consistent description for log messages.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp dcc3164f3b623de73bacf024ede95b40c48f7192 
>   src/checks/checker_process.hpp PRE-CREATION 
>   src/checks/checker_process.cpp PRE-CREATION 
>   src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 
> 
> 
> Diff: https://reviews.apache.org/r/59874/diff/3/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 59902: Improved health checks validations.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On June 9, 2017, 3:42 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59902/
> ---
> 
> (Updated June 9, 2017, 3:42 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-6916
> https://issues.apache.org/jira/browse/MESOS-6916
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added validation for the "general" fields, such as `timeout_seconds`.
> 
> 
> Diffs
> -
> 
>   src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 
>   src/tests/health_check_tests.cpp 7917a222daf6a8fbe2a5fe6657852e6ff6e699ba 
> 
> 
> Diff: https://reviews.apache.org/r/59902/diff/2/
> 
> 
> Testing
> ---
> 
> `make check` still passes on GNU/Linux
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 59874: Made `CheckerProcess` use a consistent description for log messages.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On June 9, 2017, 3:43 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59874/
> ---
> 
> (Updated June 9, 2017, 3:43 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7092
> https://issues.apache.org/jira/browse/MESOS-7092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `CheckerProcess` use a consistent description for log messages.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp dcc3164f3b623de73bacf024ede95b40c48f7192 
>   src/checks/checker_process.hpp PRE-CREATION 
>   src/checks/checker_process.cpp PRE-CREATION 
>   src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 
> 
> 
> Diff: https://reviews.apache.org/r/59874/diff/3/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 59873: Refactored `HealthChecker` to use `CheckerProcess`.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On June 9, 2017, 3:36 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59873/
> ---
> 
> (Updated June 9, 2017, 3:36 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Vinod Kone.
> 
> 
> Bugs: MESOS-7092
> https://issues.apache.org/jira/browse/MESOS-7092
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `HealthChecker` now wraps a `CheckerProcess` and gets check status
> updates via a callback.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp dcc3164f3b623de73bacf024ede95b40c48f7192 
>   src/checks/checker_process.hpp PRE-CREATION 
>   src/checks/checker_process.cpp PRE-CREATION 
>   src/checks/health_checker.hpp 25bf7e99bf5982fd7a6ff5012c231ff89fb7cb39 
>   src/checks/health_checker.cpp 9d8c8471caa05af3908d34315dbfed08a343c0f8 
> 
> 
> Diff: https://reviews.apache.org/r/59873/diff/4/
> 
> 
> Testing
> ---
> 
> Tests still pass on GNU/Linux.
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 56895: Allow agents to recover slave state post a reboot.

[Megha Sharma]

> On May 23, 2017, 9:32 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp
> > Line 5954 (original), 5965 (patched)
> > 
> >
> > Previously, looks like any errors during the partial recovery after a 
> > reboot resulted in agent flapping (e.g., checkpointed resources recovery 
> > failure, fetcher recovery failure), but now looks like the agent will 
> > remove the symlink and move along? That seems like a change in behavior

Now we are only cleaning the symlink and continuing if the failure was because 
of slaveInfo mismatch. There's no recovery failure caused by fetcher recovery 
anymore (removed in ToT) so the only class of failure left is checkpointed 
resources failure which will not be affected by agent symlink cleanup so no 
cleanup has been performed and it will cause the agent to flap so no change in 
behavior here.


> On May 23, 2017, 9:32 p.m., Vinod Kone wrote:
> > src/slave/slave.cpp
> > Line 5962 (original)
> > 
> >
> > So, we are continuing if there is a recovery error and the agent has 
> > not rebooted? That doesn't sound right?

Fixed to continue after symlink cleanup if the recovery error is because of 
slaveInfo mismatch, any other errors will cause the agent to behave same as 
before.


- Megha


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


On June 9, 2017, 4:27 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/56895/
> ---
> 
> (Updated June 9, 2017, 4:27 a.m.)
> 
> 
> Review request for mesos, Neil Conway, Vinod Kone, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6223
> https://issues.apache.org/jira/browse/MESOS-6223
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With partition awareness, the agents are now allowed to re-register
> after they have been marked Unreachable. The executors are anyway
> terminated on the agent when it reboots so there is no harm in
> letting the agent keep its SlaveID, re-register with the master
> and reconcile the lost executors. This is a pre-requisite for
> supporting persistent/restartable tasks in mesos.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.cpp 
> a003e1b80dc9b4dec5b3fbbadb2daecf855c90c7 
>   src/slave/containerizer/docker.cpp 9f84109d7de22a39ace6e44e0c7d8d501bcb24de 
>   src/slave/containerizer/mesos/containerizer.cpp 
> f3e6210eccd4a6b445ffd4447e69526d424ea36d 
>   src/slave/slave.hpp 7ffaed14035a05259ec72c70532ee4f0affa1f5d 
>   src/slave/slave.cpp 7d147ac6609933ac884bfc29032dba572a0952c6 
>   src/slave/state.hpp a497ce1f58fb8dc7718ee5bb10bc62dd7479efa5 
>   src/slave/state.cpp 18b790d2cc4f537cc9b0c3cca59b9cbaac0eda10 
>   src/tests/reservation_tests.cpp 6e9c215382ef41700921a673669ac1a7975e9b7f 
>   src/tests/slave_recovery_tests.cpp 38502584186793686f78ff5f4e03f36a3bf7ad1c 
> 
> 
> Diff: https://reviews.apache.org/r/56895/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 59936: Added streaming function for ResourceProviderInfo.

[Benjamin Bannier]

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




include/mesos/type_utils.hpp
Lines 325-328 (patched)


Could you make the equivalent change in v1?


- Benjamin Bannier


On June 9, 2017, 1:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59936/
> ---
> 
> (Updated June 9, 2017, 1:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added streaming function for ResourceProviderInfo.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
> 
> 
> Diff: https://reviews.apache.org/r/59936/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59935: Introduced a streaming function for a vector.

[Benjamin Bannier]

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




include/mesos/type_utils.hpp
Lines 377 (patched)


We should make equivalent cleanups in v1.


- Benjamin Bannier


On June 9, 2017, 1:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59935/
> ---
> 
> (Updated June 9, 2017, 1:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a streaming function for a vector.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
> 
> 
> Diff: https://reviews.apache.org/r/59935/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59935: Introduced a streaming function for a vector.

[Benjamin Bannier]

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


Fix it, then Ship it!





include/mesos/type_utils.hpp
Lines 382-384 (original), 394-396 (patched)


Not yours, but this one has a similar problem.

I believe it should be possible to generalize this to e.g.,

template 
std::ostream& operator<<(
std::ostream& stream,
const hashmap& map)
{
  return stream << stringify(map);
}


- Benjamin Bannier


On June 9, 2017, 1:18 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59935/
> ---
> 
> (Updated June 9, 2017, 1:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a streaming function for a vector.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
> 
> 
> Diff: https://reviews.apache.org/r/59935/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59453, 58955, 58964, 59099, 59100, 59101, 59147, 59525]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 9, 2017, 10:04 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated June 9, 2017, 10:04 a.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by the
> `/slaves` as well as the `GET_AGENTS` API v1 call. This means that
> calls to this endpoint (API call) will hide roles that the user making
> the request is not authorized to see.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp e8f273256b14cde1cac390163f948241757f 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59601: Removed an unused declaration in type_utils.hpp.

[Jan Schlicht]

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


Ship it!




Ship It!

- Jan Schlicht


On June 9, 2017, 1:10 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59601/
> ---
> 
> (Updated June 9, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, Jan 
> Schlicht, and Zhongbo Tian.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused declaration in type_utils.hpp.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
> 
> 
> Diff: https://reviews.apache.org/r/59601/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59601: Removed an unused declaration in type_utils.hpp.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On June 9, 2017, 1:10 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59601/
> ---
> 
> (Updated June 9, 2017, 1:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Chun-Hung Hsiao, Joseph Wu, Jan 
> Schlicht, and Zhongbo Tian.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed an unused declaration in type_utils.hpp.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
> 
> 
> Diff: https://reviews.apache.org/r/59601/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Alexander Rojas]

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

(Updated June 9, 2017, 12:04 p.m.)


Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

Adds support of the `VIEW_ROLE` ACL to the results generated by the
`/slaves` as well as the `GET_AGENTS` API v1 call. This means that
calls to this endpoint (API call) will hide roles that the user making
the request is not authorized to see.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp e8f273256b14cde1cac390163f948241757f 


Diff: https://reviews.apache.org/r/59525/diff/3/

Changes: https://reviews.apache.org/r/59525/diff/2-3/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 59525: Added filtering of `/slaves` endpoint and `GET_AGENTS` API call.

[Alexander Rojas]

> On June 2, 2017, 9 a.m., Greg Mann wrote:
> > src/master/http.cpp
> > Lines 402-403 (patched)
> > 
> >
> > It's not clear to me why we would want to show the unapproved resources 
> > in a special field?

Not sure if it is necesary write it in the TODO, because it may be too verbose, 
but the reason is that if you have X resources, and you have Y unreserved 
resources, and Z_i reserved resources, you would expect that Y+SUM(i, Z_i) = X, 
but if we hide the unapproved ones without checking where they are a user may 
be wondering if perhaps it is a bug.


- Alexander


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


On June 1, 2017, 4:58 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated June 1, 2017, 4:58 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds support of the `VIEW_ROLE` ACL to the results generated by
> the `/slaves` as well as the `GET_AGENTS` API v1 call. This means
> that calls to this endpoint (API call) will hide roles that the
> user making the request is not authorized to see.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp eb80830fa003ad8f58243d3dc4cec9e03285e96f 
>   src/master/master.hpp 89d0790fd5fea59e74276f462581fe0073594732 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>