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

[Deshi Xiao]

> On 六月 13, 2017, 12:12 a.m., Vinod Kone wrote:
> > Can you split this review into multiple logical reviews? It is really hard 
> > to review otherwise.
> > 
> > Below are a few splits I could think of
> > 
> > 1) Renames of variables (e.g., in tests).
> > 2) Changes of resources value in tests (not sure of the reason for this 
> > change).
> > 3) Addition of a new helper `recoverSlaveState`
> > 4) Changes to containerizers to short circuit recovery if state is none 
> > (not sure of the reason for this change)
> > 5) Adding `rebooted` field to recovery info and state (you can combine this 
> > with 6th if you want but might be easier to review separately) 
> > 6) Keeping the agent id on reboot
> > 
> > In addition to making it easy to review it will help us to ship parts of 
> > this chain sooner than later.

+1


- Deshi


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


On 六月 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 六月 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 58250: Test that bind-mounted host network configuration is mounted readonly.

[James Peach]

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




src/tests/containerizer/cni_isolator_tests.cpp
Lines 1451 (patched)


This should be `ROOT_INTERNET_ReadOnlyBindMounts` because it doesn't need 
`curl`.


- James Peach


On June 14, 2017, 12:38 a.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58250/
> ---
> 
> (Updated June 14, 2017, 12:38 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test that bind-mounted host network configuration is mounted readonly.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 
> 
> 
> Diff: https://reviews.apache.org/r/58250/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 60035: Added support for printing JSON values via `jsonify`.

[Michael Park]

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

(Updated June 13, 2017, 9:01 p.m.)


Review request for mesos, Benjamin Hindman and Benjamin Mahler.


Changes
---

Addressed BenH's comments.


Repository: mesos


Description
---

Added support for printing JSON values via `jsonify`.


Diffs (updated)
-

  3rdparty/stout/include/stout/json.hpp 
aa581c0345679b6f9617fb986d744110a728f042 
  3rdparty/stout/include/stout/jsonify.hpp 
7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
  3rdparty/stout/tests/jsonify_tests.cpp 
d06eba31e6b7f252b120a10066e4d7c9a25ce4b5 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

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

[Michael Park]

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

(Updated June 13, 2017, 7:16 p.m.)


Review request for mesos, Benjamin Mahler and Neil Conway.


Changes
---

Addressed bmahler's comments.


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


Repository: mesos


Description
---

Prevent allocating non-capable agents' resources to hierarchical roles.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
9a9910f6dac2f8cd1e27c2040b50a459fba563d6 
  src/tests/upgrade_tests.cpp 147475444da3ab149ebdeff5c68f543fe1fcd7e5 


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

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


Testing
---

New tests + `make check`.


Thanks,

Michael Park

Re: Review Request 60001: Removed superfluous log message in docker containerizer.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [60001]

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 12, 2017, 1:36 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60001/
> ---
> 
> (Updated June 12, 2017, 1:36 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 9f84109d7de22a39ace6e44e0c7d8d501bcb24de 
> 
> 
> Diff: https://reviews.apache.org/r/60001/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 60035: Added support for printing JSON values via `jsonify`.

[Benjamin Hindman]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/json.hpp
Line 668 (original), 668 (patched)


Can we comment here why we need to implement these `json` overloads?



3rdparty/stout/include/stout/json.hpp
Lines 681 (patched)


`{` on newline please.



3rdparty/stout/include/stout/json.hpp
Lines 708 (patched)


Small comment here as to why we do nothinig?



3rdparty/stout/include/stout/json.hpp
Lines 711 (patched)


Move into `json` below and make explicit?



3rdparty/stout/include/stout/json.hpp
Lines 722 (patched)


I think we need more comments here as to why we need to have this special 
versin of `json` in the first place.



3rdparty/stout/include/stout/json.hpp
Lines 758 (patched)


Doesn't this patch fix these TODOs?


- Benjamin Hindman


On June 13, 2017, 8:24 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60035/
> ---
> 
> (Updated June 13, 2017, 8:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added support for printing JSON values via `jsonify`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> aa581c0345679b6f9617fb986d744110a728f042 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
>   3rdparty/stout/tests/jsonify_tests.cpp 
> d06eba31e6b7f252b120a10066e4d7c9a25ce4b5 
> 
> 
> Diff: https://reviews.apache.org/r/60035/diff/3/
> 
> 
> Testing
> ---
> 
> Added new tests + `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

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

(Updated June 14, 2017, 1:06 a.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 36217d2e5 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

> On June 13, 2017, 11:57 p.m., Jie Yu wrote:
> >
> 
> Jie Yu wrote:
> Also, for nested container, we don't need to do another read only bind 
> mount.

fixed


- Silas


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


On June 14, 2017, 1:06 a.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated June 14, 2017, 1:06 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 36217d2e5 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 3aecd8c5a 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 60020: Added pushReservation and popReservation.

[Benjamin Mahler]

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


Fix it, then Ship it!





include/mesos/resources.hpp
Lines 433-434 (patched)


Should these return `Try`s? Or is the caller responsible for calling these 
with a valid reservation and only when there is a reservation to pop, 
respectively? (If not we crash)

Would be great to add documentation so it's clear to the caller.



src/common/resources.cpp
Lines 1368 (patched)


Is this hard to do? Any reason it was left out?


- Benjamin Mahler


On June 13, 2017, 8:36 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60020/
> ---
> 
> (Updated June 13, 2017, 8:36 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added pushReservation and popReservation.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
>   include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60020/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

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

(Updated June 14, 2017, 12:38 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Test that bind-mounted host network configuration is mounted readonly.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 


Diff: https://reviews.apache.org/r/58250/diff/5/

Changes: https://reviews.apache.org/r/58250/diff/4-5/


Testing
---


Thanks,

Silas Snider

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

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

(Updated June 14, 2017, 12:33 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Test that bind-mounted host network configuration is mounted readonly.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

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

(Updated June 14, 2017, 12:32 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Test that bind-mounted host network configuration is mounted readonly.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

> On June 14, 2017, 12:02 a.m., Jie Yu wrote:
> > FYI, this test does not pass on my box:
> > ```
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from CniIsolatorTest
> > [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > Executing pre-exec command 
> > '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> > Executing pre-exec command 
> > '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/slaves\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0\/frameworks\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-\/executors\/24b4b6bf-db12-416c-8113-cc4c34af6dcf\/runs\/8d2ef480-b21e-48b1-a140-e585a4762969","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/provisioner\/containers\/8d2ef480-b21e-48b1-a140-e585a4762969\/backends\/overlay\/rootfses\/4517faf0-ff27-49d2-8142-96636b8f8475\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> > I0613 17:01:42.720935 37571 exec.cpp:162] Version: 1.4.0
> > I0613 17:01:42.733803 37597 exec.cpp:237] Executor registered on agent 
> > 3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0
> > I0613 17:01:42.738118 37577 executor.cpp:169] Received SUBSCRIBED event
> > I0613 17:01:42.739022 37577 executor.cpp:173] Subscribed executor on 
> > core-dev
> > I0613 17:01:42.739261 37577 executor.cpp:169] Received LAUNCH event
> > I0613 17:01:42.739573 37577 executor.cpp:624] Starting task 
> > 24b4b6bf-db12-416c-8113-cc4c34af6dcf
> > I0613 17:01:42.741453 37577 executor.cpp:468] Running 
> > '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> > '
> > I0613 17:01:42.743849 37577 executor.cpp:636] Forked command at 37611
> > Changing root to 
> > /tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS/provisioner/containers/8d2ef480-b21e-48b1-a140-e585a4762969/backends/overlay/rootfses/4517faf0-ff27-49d2-8142-96636b8f8475
> > c: applet not found
> > I0613 17:01:42.906725 37605 executor.cpp:915] Command exited with status 
> > 127 (pid: 37611)
> > /home/jie/workspace/mesos/src/tests/containerizer/cni_isolator_tests.cpp:1526:
> >  Failure
> >   Expected: TASK_FINISHED
> > To be equal to: statusFinished->state()
> >   Which is: TASK_FAILED
> > I0613 17:01:42.918128 37585 exec.cpp:435] Executor asked to shutdown
> > I0613 17:01:42.918589 37592 executor.cpp:169] Received SHUTDOWN event
> > I0613 17:01:42.918645 37592 executor.cpp:733] Shutting down
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts (5056 ms)
> > [--] 1 test from CniIsolatorTest (5057 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (5098 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > ```
> 
> Jie Yu wrote:
> Added a missing argv[0], still failed for me:
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CniIsolatorTest
> [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> Executing pre-exec command 
> '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/slaves\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0\/frameworks\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-\/executors\/55d299b4-2663-4bd5-980a-2b5df95181a4\/runs\/2798da96-2f37-4e27-b737-aa01fc6b4a5d","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/provisioner\/containers\/2798da96-2f37-4e27-b737-aa01fc6b4a5d\/backends\/overlay\/rootfses\/c00bda57-a3eb-435b-9499-2e1c2bfb7a56\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> I0613 17:09:37.193990 36312 exec.cpp:162] Version: 1.4.0
> I0613 17:09:37.209614 36310 exec.cpp:237] Executor registered on agent 
> 31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0
> I0613 17:09:37.213358 36323 executor.cpp:169] Received SUBSCRIBED event
> I0613 17:09:37.214424 36323 executor.cpp:173] Subscribed executor on 
> core-dev
> I0613 17:09:37.214689 36323 executor.cpp:169] Received LAUNCH event
> I0613 17:09:37.214915 36323 executor.cpp:624] Starting task 
> 55d299b4-2663-4bd5-980a-2b5df95181a4
> I0613 17:09:37.216902 36323 executor.cpp:468] Running 
> '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> '
> I0613 17:09:37.219539 36323 executor.cpp:636] Forked command at 36346
> Changing root to 
> 

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Jie Yu]

> On June 13, 2017, 11:57 p.m., Jie Yu wrote:
> >

Also, for nested container, we don't need to do another read only bind mount.


- Jie


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


On June 13, 2017, 10:02 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated June 13, 2017, 10:02 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Jie Yu]

> On June 14, 2017, 12:02 a.m., Jie Yu wrote:
> > FYI, this test does not pass on my box:
> > ```
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from CniIsolatorTest
> > [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > Executing pre-exec command 
> > '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> > Executing pre-exec command 
> > '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/slaves\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0\/frameworks\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-\/executors\/24b4b6bf-db12-416c-8113-cc4c34af6dcf\/runs\/8d2ef480-b21e-48b1-a140-e585a4762969","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/provisioner\/containers\/8d2ef480-b21e-48b1-a140-e585a4762969\/backends\/overlay\/rootfses\/4517faf0-ff27-49d2-8142-96636b8f8475\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> > I0613 17:01:42.720935 37571 exec.cpp:162] Version: 1.4.0
> > I0613 17:01:42.733803 37597 exec.cpp:237] Executor registered on agent 
> > 3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0
> > I0613 17:01:42.738118 37577 executor.cpp:169] Received SUBSCRIBED event
> > I0613 17:01:42.739022 37577 executor.cpp:173] Subscribed executor on 
> > core-dev
> > I0613 17:01:42.739261 37577 executor.cpp:169] Received LAUNCH event
> > I0613 17:01:42.739573 37577 executor.cpp:624] Starting task 
> > 24b4b6bf-db12-416c-8113-cc4c34af6dcf
> > I0613 17:01:42.741453 37577 executor.cpp:468] Running 
> > '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> > '
> > I0613 17:01:42.743849 37577 executor.cpp:636] Forked command at 37611
> > Changing root to 
> > /tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS/provisioner/containers/8d2ef480-b21e-48b1-a140-e585a4762969/backends/overlay/rootfses/4517faf0-ff27-49d2-8142-96636b8f8475
> > c: applet not found
> > I0613 17:01:42.906725 37605 executor.cpp:915] Command exited with status 
> > 127 (pid: 37611)
> > /home/jie/workspace/mesos/src/tests/containerizer/cni_isolator_tests.cpp:1526:
> >  Failure
> >   Expected: TASK_FINISHED
> > To be equal to: statusFinished->state()
> >   Which is: TASK_FAILED
> > I0613 17:01:42.918128 37585 exec.cpp:435] Executor asked to shutdown
> > I0613 17:01:42.918589 37592 executor.cpp:169] Received SHUTDOWN event
> > I0613 17:01:42.918645 37592 executor.cpp:733] Shutting down
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts (5056 ms)
> > [--] 1 test from CniIsolatorTest (5057 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (5098 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > ```
> 
> Jie Yu wrote:
> Added a missing argv[0], still failed for me:
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CniIsolatorTest
> [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> Executing pre-exec command 
> '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/slaves\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0\/frameworks\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-\/executors\/55d299b4-2663-4bd5-980a-2b5df95181a4\/runs\/2798da96-2f37-4e27-b737-aa01fc6b4a5d","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/provisioner\/containers\/2798da96-2f37-4e27-b737-aa01fc6b4a5d\/backends\/overlay\/rootfses\/c00bda57-a3eb-435b-9499-2e1c2bfb7a56\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> I0613 17:09:37.193990 36312 exec.cpp:162] Version: 1.4.0
> I0613 17:09:37.209614 36310 exec.cpp:237] Executor registered on agent 
> 31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0
> I0613 17:09:37.213358 36323 executor.cpp:169] Received SUBSCRIBED event
> I0613 17:09:37.214424 36323 executor.cpp:173] Subscribed executor on 
> core-dev
> I0613 17:09:37.214689 36323 executor.cpp:169] Received LAUNCH event
> I0613 17:09:37.214915 36323 executor.cpp:624] Starting task 
> 55d299b4-2663-4bd5-980a-2b5df95181a4
> I0613 17:09:37.216902 36323 executor.cpp:468] Running 
> '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> '
> I0613 17:09:37.219539 36323 executor.cpp:636] Forked command at 36346
> Changing root to 
> 

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

> On June 14, 2017, 12:02 a.m., Jie Yu wrote:
> > FYI, this test does not pass on my box:
> > ```
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from CniIsolatorTest
> > [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > Executing pre-exec command 
> > '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> > Executing pre-exec command 
> > '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/slaves\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0\/frameworks\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-\/executors\/24b4b6bf-db12-416c-8113-cc4c34af6dcf\/runs\/8d2ef480-b21e-48b1-a140-e585a4762969","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/provisioner\/containers\/8d2ef480-b21e-48b1-a140-e585a4762969\/backends\/overlay\/rootfses\/4517faf0-ff27-49d2-8142-96636b8f8475\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> > I0613 17:01:42.720935 37571 exec.cpp:162] Version: 1.4.0
> > I0613 17:01:42.733803 37597 exec.cpp:237] Executor registered on agent 
> > 3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0
> > I0613 17:01:42.738118 37577 executor.cpp:169] Received SUBSCRIBED event
> > I0613 17:01:42.739022 37577 executor.cpp:173] Subscribed executor on 
> > core-dev
> > I0613 17:01:42.739261 37577 executor.cpp:169] Received LAUNCH event
> > I0613 17:01:42.739573 37577 executor.cpp:624] Starting task 
> > 24b4b6bf-db12-416c-8113-cc4c34af6dcf
> > I0613 17:01:42.741453 37577 executor.cpp:468] Running 
> > '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> > '
> > I0613 17:01:42.743849 37577 executor.cpp:636] Forked command at 37611
> > Changing root to 
> > /tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS/provisioner/containers/8d2ef480-b21e-48b1-a140-e585a4762969/backends/overlay/rootfses/4517faf0-ff27-49d2-8142-96636b8f8475
> > c: applet not found
> > I0613 17:01:42.906725 37605 executor.cpp:915] Command exited with status 
> > 127 (pid: 37611)
> > /home/jie/workspace/mesos/src/tests/containerizer/cni_isolator_tests.cpp:1526:
> >  Failure
> >   Expected: TASK_FINISHED
> > To be equal to: statusFinished->state()
> >   Which is: TASK_FAILED
> > I0613 17:01:42.918128 37585 exec.cpp:435] Executor asked to shutdown
> > I0613 17:01:42.918589 37592 executor.cpp:169] Received SHUTDOWN event
> > I0613 17:01:42.918645 37592 executor.cpp:733] Shutting down
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts (5056 ms)
> > [--] 1 test from CniIsolatorTest (5057 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (5098 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > ```
> 
> Jie Yu wrote:
> Added a missing argv[0], still failed for me:
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CniIsolatorTest
> [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> Executing pre-exec command 
> '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> Executing pre-exec command 
> '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/slaves\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0\/frameworks\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-\/executors\/55d299b4-2663-4bd5-980a-2b5df95181a4\/runs\/2798da96-2f37-4e27-b737-aa01fc6b4a5d","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/provisioner\/containers\/2798da96-2f37-4e27-b737-aa01fc6b4a5d\/backends\/overlay\/rootfses\/c00bda57-a3eb-435b-9499-2e1c2bfb7a56\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> I0613 17:09:37.193990 36312 exec.cpp:162] Version: 1.4.0
> I0613 17:09:37.209614 36310 exec.cpp:237] Executor registered on agent 
> 31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0
> I0613 17:09:37.213358 36323 executor.cpp:169] Received SUBSCRIBED event
> I0613 17:09:37.214424 36323 executor.cpp:173] Subscribed executor on 
> core-dev
> I0613 17:09:37.214689 36323 executor.cpp:169] Received LAUNCH event
> I0613 17:09:37.214915 36323 executor.cpp:624] Starting task 
> 55d299b4-2663-4bd5-980a-2b5df95181a4
> I0613 17:09:37.216902 36323 executor.cpp:468] Running 
> '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> '
> I0613 17:09:37.219539 36323 executor.cpp:636] Forked command at 36346
> Changing root to 
> 

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Jie Yu]

> On June 14, 2017, 12:02 a.m., Jie Yu wrote:
> > FYI, this test does not pass on my box:
> > ```
> > [==] Running 1 test from 1 test case.
> > [--] Global test environment set-up.
> > [--] 1 test from CniIsolatorTest
> > [ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > Executing pre-exec command 
> > '{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
> > Executing pre-exec command 
> > '{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/slaves\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0\/frameworks\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-\/executors\/24b4b6bf-db12-416c-8113-cc4c34af6dcf\/runs\/8d2ef480-b21e-48b1-a140-e585a4762969","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/provisioner\/containers\/8d2ef480-b21e-48b1-a140-e585a4762969\/backends\/overlay\/rootfses\/4517faf0-ff27-49d2-8142-96636b8f8475\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
> > I0613 17:01:42.720935 37571 exec.cpp:162] Version: 1.4.0
> > I0613 17:01:42.733803 37597 exec.cpp:237] Executor registered on agent 
> > 3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0
> > I0613 17:01:42.738118 37577 executor.cpp:169] Received SUBSCRIBED event
> > I0613 17:01:42.739022 37577 executor.cpp:173] Subscribed executor on 
> > core-dev
> > I0613 17:01:42.739261 37577 executor.cpp:169] Received LAUNCH event
> > I0613 17:01:42.739573 37577 executor.cpp:624] Starting task 
> > 24b4b6bf-db12-416c-8113-cc4c34af6dcf
> > I0613 17:01:42.741453 37577 executor.cpp:468] Running 
> > '/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
> > '
> > I0613 17:01:42.743849 37577 executor.cpp:636] Forked command at 37611
> > Changing root to 
> > /tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS/provisioner/containers/8d2ef480-b21e-48b1-a140-e585a4762969/backends/overlay/rootfses/4517faf0-ff27-49d2-8142-96636b8f8475
> > c: applet not found
> > I0613 17:01:42.906725 37605 executor.cpp:915] Command exited with status 
> > 127 (pid: 37611)
> > /home/jie/workspace/mesos/src/tests/containerizer/cni_isolator_tests.cpp:1526:
> >  Failure
> >   Expected: TASK_FINISHED
> > To be equal to: statusFinished->state()
> >   Which is: TASK_FAILED
> > I0613 17:01:42.918128 37585 exec.cpp:435] Executor asked to shutdown
> > I0613 17:01:42.918589 37592 executor.cpp:169] Received SHUTDOWN event
> > I0613 17:01:42.918645 37592 executor.cpp:733] Shutting down
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts (5056 ms)
> > [--] 1 test from CniIsolatorTest (5057 ms total)
> > 
> > [--] Global test environment tear-down
> > [==] 1 test from 1 test case ran. (5098 ms total)
> > [  PASSED  ] 0 tests.
> > [  FAILED  ] 1 test, listed below:
> > [  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
> > ```

Added a missing argv[0], still failed for me:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from CniIsolatorTest
[ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
Executing pre-exec command 
'{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
Executing pre-exec command 
'{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/slaves\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0\/frameworks\/31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-\/executors\/55d299b4-2663-4bd5-980a-2b5df95181a4\/runs\/2798da96-2f37-4e27-b737-aa01fc6b4a5d","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M\/provisioner\/containers\/2798da96-2f37-4e27-b737-aa01fc6b4a5d\/backends\/overlay\/rootfses\/c00bda57-a3eb-435b-9499-2e1c2bfb7a56\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
I0613 17:09:37.193990 36312 exec.cpp:162] Version: 1.4.0
I0613 17:09:37.209614 36310 exec.cpp:237] Executor registered on agent 
31e24c6c-cb37-4125-9b4c-d1eb95fea3d9-S0
I0613 17:09:37.213358 36323 executor.cpp:169] Received SUBSCRIBED event
I0613 17:09:37.214424 36323 executor.cpp:173] Subscribed executor on core-dev
I0613 17:09:37.214689 36323 executor.cpp:169] Received LAUNCH event
I0613 17:09:37.214915 36323 executor.cpp:624] Starting task 
55d299b4-2663-4bd5-980a-2b5df95181a4
I0613 17:09:37.216902 36323 executor.cpp:468] Running 
'/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
'
I0613 17:09:37.219539 36323 executor.cpp:636] Forked command at 36346
Changing root to 
/tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_OTI05M/provisioner/containers/2798da96-2f37-4e27-b737-aa01fc6b4a5d/backends/overlay/rootfses/c00bda57-a3eb-435b-9499-2e1c2bfb7a56

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Jie Yu]

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



FYI, this test does not pass on my box:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from CniIsolatorTest
[ RUN  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
Executing pre-exec command 
'{"arguments":["mesos-containerizer","mount","--help=false","--operation=make-rslave","--path=\/"],"shell":false,"value":"\/home\/jie\/workspace\/dist\/mesos\/build\/src\/mesos-containerizer"}'
Executing pre-exec command 
'{"arguments":["mount","-n","--rbind","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/slaves\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0\/frameworks\/3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-\/executors\/24b4b6bf-db12-416c-8113-cc4c34af6dcf\/runs\/8d2ef480-b21e-48b1-a140-e585a4762969","\/tmp\/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS\/provisioner\/containers\/8d2ef480-b21e-48b1-a140-e585a4762969\/backends\/overlay\/rootfses\/4517faf0-ff27-49d2-8142-96636b8f8475\/mnt\/mesos\/sandbox"],"shell":false,"value":"mount"}'
I0613 17:01:42.720935 37571 exec.cpp:162] Version: 1.4.0
I0613 17:01:42.733803 37597 exec.cpp:237] Executor registered on agent 
3fd3e062-7fd7-4a6a-b5c3-f59b4e44fa0c-S0
I0613 17:01:42.738118 37577 executor.cpp:169] Received SUBSCRIBED event
I0613 17:01:42.739022 37577 executor.cpp:173] Subscribed executor on core-dev
I0613 17:01:42.739261 37577 executor.cpp:169] Received LAUNCH event
I0613 17:01:42.739573 37577 executor.cpp:624] Starting task 
24b4b6bf-db12-416c-8113-cc4c34af6dcf
I0613 17:01:42.741453 37577 executor.cpp:468] Running 
'/home/jie/workspace/dist/mesos/build/src/mesos-containerizer launch 
'
I0613 17:01:42.743849 37577 executor.cpp:636] Forked command at 37611
Changing root to 
/tmp/CniIsolatorTest_ROOT_INTERNET_CURL_ReadOnlyBindMounts_44c0WS/provisioner/containers/8d2ef480-b21e-48b1-a140-e585a4762969/backends/overlay/rootfses/4517faf0-ff27-49d2-8142-96636b8f8475
c: applet not found
I0613 17:01:42.906725 37605 executor.cpp:915] Command exited with status 127 
(pid: 37611)
/home/jie/workspace/mesos/src/tests/containerizer/cni_isolator_tests.cpp:1526: 
Failure
  Expected: TASK_FINISHED
To be equal to: statusFinished->state()
  Which is: TASK_FAILED
I0613 17:01:42.918128 37585 exec.cpp:435] Executor asked to shutdown
I0613 17:01:42.918589 37592 executor.cpp:169] Received SHUTDOWN event
I0613 17:01:42.918645 37592 executor.cpp:733] Shutting down
[  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts (5056 ms)
[--] 1 test from CniIsolatorTest (5057 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (5098 ms total)
[  PASSED  ] 0 tests.
[  FAILED  ] 1 test, listed below:
[  FAILED  ] CniIsolatorTest.ROOT_INTERNET_CURL_ReadOnlyBindMounts
```

- Jie Yu


On June 13, 2017, 10:14 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58250/
> ---
> 
> (Updated June 13, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test that bind-mounted host network configuration is mounted readonly.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 
> 
> 
> Diff: https://reviews.apache.org/r/58250/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1918-1928 (patched)


Thought about it more. I don't think we should do read-only bind mount if 
the 'source' is not from host `/etc/*`. We should allow container to modify it 
because the file is private to each container.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1980-1990 (patched)


Ditto here.


- Jie Yu


On June 13, 2017, 10:02 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated June 13, 2017, 10:02 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 59763: Caused master to abort when joining a mixed-region cluster.

[Neil Conway]

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

(Updated June 13, 2017, 11:28 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add comment about forward compatibility.


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


Repository: mesos


Description
---

That is, if a standby master is configured to use region X but it learns
that the current master has region Y, the standby master will abort with
an error message. This enforces the requirement that all masters in a
single Mesos cluster are configured to use the same region (they can be
configured to use different zones in that region, however).

To allow graceful upgrades, we only abort the standby master if both the
standby master and the leading master have a configured domain; if
either master has the unset (default) domain, the standby master does
not abort.

NOTE: It would be nice to have unit tests to validate this behavior, but
the current unit test infrastructure does not support starting multiple
masters (MESOS-2976).


Diffs (updated)
-

  include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
  include/mesos/v1/mesos.hpp c2e6ec5c4fa06d0179d6aaf3371ace4b6e6dbdaf 
  src/master/master.cpp 3c6925dc6055fb7cec12ec03bc557ad462863c58 


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

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


Testing
---

`make check`

Manual testing.


Thanks,

Neil Conway

Re: Review Request 59761: Added master and agent flags to specify domain.

[Neil Conway]

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

(Updated June 13, 2017, 11:13 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add newline.


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


Repository: mesos


Description
---

Added master and agent flags to specify domain.


Diffs (updated)
-

  docs/configuration.md ed510fa638878b71e7fcff4850152a8a8622127e 
  src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
  src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 
  src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d 
  src/slave/flags.hpp 2f9d52e94c2c31e95208cd8b0640a5de2d2a61fd 
  src/slave/flags.cpp 93c8ffb5c822cf6c99071be7aca52a6b3d187619 


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

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [57884, 58250]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 10:14 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58250/
> ---
> 
> (Updated June 13, 2017, 10:14 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test that bind-mounted host network configuration is mounted readonly.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 
> 
> 
> Diff: https://reviews.apache.org/r/58250/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 58510: Added a test for hierarchical reservation.

[Michael Park]

> On June 9, 2017, 12:22 p.m., Benjamin Mahler wrote:
> > 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?
> 
> Jay Guo wrote:
> Since we made changes for both quota and fair share stage of allocation, 
> I'm using quota to make sure we cover both logic paths. I updated the test 
> comments, please take a look.
> 
> Michael Park wrote:
> I would suggest:
> ```
> // This test ensures that resources reserved to ancestor roles can be 
> offered
> // to their descendants. Since both quota and fair-share stages perform
> // reservation allocations, we use a quota'd role to test both cases.
> ```

Committed. Please let me know if you want to further update this.


- Michael


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


On June 13, 2017, 8:40 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated June 13, 2017, 8:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 631e800e6566847e015ea2638cc1c2fe673855be 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> comment out 
> https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
> `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild"
>  --gtest_also_run_disabled_tests`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58510: Added a test for hierarchical reservation.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 13, 2017, 8:40 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated June 13, 2017, 8:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 631e800e6566847e015ea2638cc1c2fe673855be 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> comment out 
> https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
> `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild"
>  --gtest_also_run_disabled_tests`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 13, 2017, 8:27 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58509/
> ---
> 
> (Updated June 13, 2017, 8:27 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When collecting resources allocatable to a role, allocator should
> aggregate not only reservations of the role itself, but also that
> of all its ancestors.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 73d0b42433a1ff3e853e851b8191864b4e233666 
> 
> 
> Diff: https://reviews.apache.org/r/58509/diff/2/
> 
> 
> Testing
> ---
> 
> see next patch in chain
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

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

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On June 13, 2017, 10:35 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58508/
> ---
> 
> (Updated June 13, 2017, 10:35 a.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 allocatable to 'role' iff:
>  - it is reserved to ancestor of 'role' in hierarchy, OR
>  - it is reserved to 'role' itself, OR
>  - it is unreserved.
> 
> This is a helper function for reservation delegate, where reserved
> resources can be 'delegated' downwards in the hierarchy.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
>   include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/58508/diff/3/
> 
> 
> Testing
> ---
> 
> comment out 
> https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
> `./bin/mesos-tests.sh 
> --gtest_filter="ResourcesTest.DISABLED_HierarchicalReservations" 
> --gtest_also_run_disabled_tests`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 59761: Added master and agent flags to specify domain.

[Vinod Kone]

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




src/common/parse.hpp
Lines 226 (patched)


2 blank lines.



src/master/flags.cpp
Lines 645 (patched)


consider using `domain` instead of `domains` here for future proofing.


- Vinod Kone


On June 2, 2017, 8:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59761/
> ---
> 
> (Updated June 2, 2017, 8:10 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7610
> https://issues.apache.org/jira/browse/MESOS-7610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added master and agent flags to specify domain.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md ed510fa638878b71e7fcff4850152a8a8622127e 
>   src/common/parse.hpp 64eabf89f81a6708cfac89081e0d9f9586721693 
>   src/master/flags.hpp 8a9bd2dfa513f1de85dab2cc3cc1d78e80293837 
>   src/master/flags.cpp ca5f02c728a9f39abbbee59fa169f95d20c69d3d 
>   src/slave/flags.hpp 2f9d52e94c2c31e95208cd8b0640a5de2d2a61fd 
>   src/slave/flags.cpp 93c8ffb5c822cf6c99071be7aca52a6b3d187619 
> 
> 
> Diff: https://reviews.apache.org/r/59761/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59760: Added REGION_AWARE framework capability.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On June 2, 2017, 8:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59760/
> ---
> 
> (Updated June 2, 2017, 8:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7609
> https://issues.apache.org/jira/browse/MESOS-7609
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added REGION_AWARE framework capability.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/common/protobuf_utils.hpp be2325f05b81b847fa592eff65175cbc99764fd6 
>   src/tests/protobuf_utils_tests.cpp 6ec3a8e606e86406a0f1c9d5de4ed09d0811e49a 
> 
> 
> Diff: https://reviews.apache.org/r/59760/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59759: Added protobuf definitions for fault domains.

[Vinod Kone]

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




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


Consider `FaultDomainInfo` or `Region` and `Zone`.


- Vinod Kone


On June 2, 2017, 8:08 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59759/
> ---
> 
> (Updated June 2, 2017, 8:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-7608
> https://issues.apache.org/jira/browse/MESOS-7608
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This introduces a first-class notion of a "domain", which is a set of
> hosts that have similar characteristics. Mesos will initially only
> support "fault domains", which identify groups of hosts with similar
> failure characteristics.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/type_utils.hpp c7872c0cd7f19d923e8be1f33a775d5eb17a0cf7 
>   include/mesos/v1/mesos.hpp c2e6ec5c4fa06d0179d6aaf3371ace4b6e6dbdaf 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/common/type_utils.cpp 5f8e72b97d6766f9729079f6c6c013bc117cedb9 
>   src/v1/mesos.cpp 3b9f9b43e748159ae753880bbe4a5975814073ab 
> 
> 
> Diff: https://reviews.apache.org/r/59759/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

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

(Updated June 13, 2017, 10:14 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Test that bind-mounted host network configuration is mounted readonly.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 60058: Fixed the executor API endpoint help.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60056, 60058]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 11:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60058/
> ---
> 
> (Updated June 13, 2017, 11:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authentication was recently added to the V1 executor HTTP
> API. This patch updates the endpoint help to reflect this.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 
> 
> 
> Diff: https://reviews.apache.org/r/60058/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

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

(Updated June 13, 2017, 10:02 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
6e95315b70a5d9d3b4b21c4cf235b0a483760190 


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

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


Testing
---


Thanks,

Silas Snider

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/cni_isolator_tests.cpp
Lines 1451 (patched)


Let's add INTERNET_CURL filter here because this requires docker registry



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1490 (patched)


I would split this into multiple lines. Also, please use multiple lines to 
stick to our style guide:

```
// Verify that the files are not readonly.
command.add_arguments(
  R"~(
  mount | grep /etc/hosts &&
  test -w /etc/hosts &&
  test -w /etc/resolv.conf &&
  test -w /etc/hostname
  )~");
```


- Jie Yu


On June 13, 2017, 4:49 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58250/
> ---
> 
> (Updated June 13, 2017, 4:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test that bind-mounted host network configuration is mounted readonly.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 
> 
> 
> Diff: https://reviews.apache.org/r/58250/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

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

[Avinash sridharan]

> On June 13, 2017, 9:08 p.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1241 (patched)
> > 
> >
> > do you need `mesos::` prefix here?

Yeah without `mesos` the compiler tries to resolve it in 
`mesos::internal::test::v1` and fails.


> On June 13, 2017, 9:08 p.m., Jie Yu wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1260-1271 (patched)
> > 
> >
> > since we parameterize 'network', we just need one of those?
> > 
> > ```
> > INSTANTIATE_TEST_CASE_P(
> > Network,
> > DefaultExecutorCniTest,
> > Values(
> > Option::none(),
> > Option(
> > v1::createNetworkInfo("__MESOS_TEST__"));
> > ```

I seggregated the instantiation to add the Prefix "HostNetwork" and 
"CniNetwork" to give more clarity on the tests if and when they fail.


- Avinash


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


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 
> 

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 1546-1547 (patched)


The file won't even exist in the rootfs? What's the purpose of this test? 
I'll just just remove this test.


- Jie Yu


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 60057: Made explicit that we dispatch to a process manager.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60057]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 8:30 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60057/
> ---
> 
> (Updated June 13, 2017, 8:30 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This member function of 'ProcessManager' was capturing a 'this'
> pointer when dispatching to itself, but did not properly use 'defer'
> or 'dispatch'. While this pattern is usually suspect, it was safe here
> as we can be sure that the process manager lives long enough to safely
> invoke the created callback.
> 
> This patch removes the capture of 'this' and instead explicitly
> references the 'static process_manager' in the created callback to
> signal that we rely on external invariants.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 
> 
> 
> Diff: https://reviews.apache.org/r/60057/diff/1/
> 
> 
> Testing
> ---
> 
> * `make check` (Fedora 25 clang-trunk w/ optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60019: Removed the only use of `flatten` in Mesos core.

[Benjamin Mahler]

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


Ship it!




Looks good, but of course it assumes the new format, which doesn't seem to be 
enforced?


src/common/resources.cpp
Line 1804 (original), 1804 (patched)


How about 'r'?


- Benjamin Mahler


On June 13, 2017, 8:36 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60019/
> ---
> 
> (Updated June 13, 2017, 8:36 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed the only use of `flatten` in Mesos core.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60019/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60058: Fixed the executor API endpoint help.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 13, 2017, 9:08 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60058/
> ---
> 
> (Updated June 13, 2017, 9:08 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Authentication was recently added to the V1 executor HTTP
> API. This patch updates the endpoint help to reflect this.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 
> 
> 
> Diff: https://reviews.apache.org/r/60058/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

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

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/cni_isolator_tests.cpp
Lines 1229 (patched)


I would do the following here instead
```
CniIsolatorTest::CreateSlaveFlags()
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1241 (patched)


do you need `mesos::` prefix here?



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1241-1251 (patched)


I would introduce a `createNetworkInfo` helper in test/mesos.hpp



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1260-1271 (patched)


since we parameterize 'network', we just need one of those?

```
INSTANTIATE_TEST_CASE_P(
Network,
DefaultExecutorCniTest,
Values(
Option::none(),
Option(
v1::createNetworkInfo("__MESOS_TEST__"));
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1265 (patched)


2 lines apart



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1317-1324 (patched)


Please follow this example:

https://github.com/apache/mesos/blob/master/src/tests/containerizer/runtime_isolator_tests.cpp#L334

the test examples in default executor tests need cleanup (much more verbose)



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1390-1392 (patched)


Please use
```
Future updateRunning;
EXPECT_CALL(*scheduler, update(_, _))
  .WillOnce(DoAll(FutureArg<1>(),
  v1::scheduler::SendAcknowledge(
  frameworkId,
  offer.agent_id(;
```



src/tests/containerizer/cni_isolator_tests.cpp
Lines 1394-1412 (patched)


Please use `v1::createCallAccept`


- Jie Yu


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 

Review Request 60058: Fixed the executor API endpoint help.

[Greg Mann]

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

Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.


Repository: mesos


Description
---

Authentication was recently added to the V1 executor HTTP
API. This patch updates the endpoint help to reflect this.


Diffs
-

  src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 


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


Testing
---


Thanks,

Greg Mann

Re: Review Request 60018: Renamed `flatten()` to `toUnreserved()`.

[Benjamin Mahler]

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




src/common/resources.cpp
Line 1366 (original), 1366-1369 (patched)


I'm starting to get used to the code assuming the new format, but I suspect 
this may suprise people so if you could leave a pointer here too that would be 
great.



src/common/resources.cpp
Line 1807 (original), 1812 (patched)


Hm.. I thought based on code you're introducing that we wouldn't be using 
the old format? I see access of `.role()` and `.reservation()` here?



src/master/allocator/mesos/hierarchical.cpp
Lines 1547-1548 (original), 1547-1548 (patched)


Hm.. the `.toUnreserved()` doesn't look necessary anymore if we have the 
new format? Or depends what the meaning of `allocationScalarQuantities` is.. 
FWIW I found this function rather confusing when I encountered it, I have some 
ideas on what to do about it if you want to chat further



src/master/http.cpp
Lines 2247-2248 (original), 2247-2248 (patched)


looks like with the new name you can omit the bit after the semi-colon here?


- Benjamin Mahler


On June 13, 2017, 8:35 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60018/
> ---
> 
> (Updated June 13, 2017, 8:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renamed `flatten()` to `toUnreserved()`.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
>   include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
>   src/cli/execute.cpp 11a2569f788a2ee1ceaf13ab0d40d1d1b275f27a 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/examples/balloon_framework.cpp bfaae3796afb426459ef5cff3a37634a8242032e 
>   src/examples/disk_full_framework.cpp 
> a73e6cf285b9a74492d476a624915235e079051f 
>   src/examples/long_lived_framework.cpp 
> bbe8606c8d2572ad695e44fa23ce7b6129eb 
>   src/examples/test_framework.cpp 05ddc89d953d9b968b78f98fec819aeda7f79e26 
>   src/examples/test_http_framework.cpp 
> 471835c349e0da031a540ed48881227a25887ba7 
>   src/master/allocator/mesos/hierarchical.cpp 
> 73d0b42433a1ff3e853e851b8191864b4e233666 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/tests/mesos.hpp 48072a976cdbe6e655dc6f5f258abc6d737ed068 
>   src/tests/reservation_endpoints_tests.cpp 
> ba283ee74fe27998a01c43e1ab6a8ce0089181fa 
>   src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60018/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60056: Fixed a comment in the v1 master proto definitions.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60056]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 8:05 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60056/
> ---
> 
> (Updated June 13, 2017, 8:05 p.m.)
> 
> 
> Review request for mesos and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a comment in the v1 master proto definitions.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/master/master.proto 
> f8711e4bea5189c6d6aaae4613174713d6391ef5 
> 
> 
> Diff: https://reviews.apache.org/r/60056/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 60057: Made explicit that we dispatch to a process manager.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


Repository: mesos


Description
---

This member function of 'ProcessManager' was capturing a 'this'
pointer when dispatching to itself, but did not properly use 'defer'
or 'dispatch'. While this pattern is usually suspect, it was safe here
as we can be sure that the process manager lives long enough to safely
invoke the created callback.

This patch removes the capture of 'this' and instead explicitly
references the 'static process_manager' in the created callback to
signal that we rely on external invariants.


Diffs
-

  3rdparty/libprocess/src/process.cpp 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 


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


Testing
---

* `make check` (Fedora 25 clang-trunk w/ optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 58510: Added a test for hierarchical reservation.

[Michael Park]

> On June 9, 2017, 12:22 p.m., Benjamin Mahler wrote:
> > 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?
> 
> Jay Guo wrote:
> Since we made changes for both quota and fair share stage of allocation, 
> I'm using quota to make sure we cover both logic paths. I updated the test 
> comments, please take a look.

I would suggest:
```
// This test ensures that resources reserved to ancestor roles can be offered
// to their descendants. Since both quota and fair-share stages perform
// reservation allocations, we use a quota'd role to test both cases.
```


- Michael


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


On June 13, 2017, 8:40 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated June 13, 2017, 8:40 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 631e800e6566847e015ea2638cc1c2fe673855be 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> comment out 
> https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
> `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild"
>  --gtest_also_run_disabled_tests`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 60056: Fixed a comment in the v1 master proto definitions.

[Greg Mann]

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

Review request for mesos and Neil Conway.


Repository: mesos


Description
---

Fixed a comment in the v1 master proto definitions.


Diffs
-

  include/mesos/v1/master/master.proto f8711e4bea5189c6d6aaae4613174713d6391ef5 


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


Testing
---


Thanks,

Greg Mann

Re: Review Request 60017: Resources: Updated utilities.

[Benjamin Mahler]

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




src/common/resources.cpp
Line 1378 (original), 1379-1384 (patched)


Hm.. it's not obvious to me if this is the right thing to do, how much 
thought did you put into this? (can't tell given there's no comment)

i.e. Should we be reducing the reservations down to a single one (and do we 
care about the distinction between static and dynamic for this stripped scalar 
quantity?) vs stripping metadata but preserving the stack.

I think before we just cared about which role it was reserved to, not 
whether it was static or dynamic, so this is a change to the behavior.



src/common/resources.cpp
Lines 2055-2074 (original)


This seems unrelated, can you pull it out of this change so that the diff 
is as clean as possible?



src/common/resources.cpp
Lines 2100-2115 (original), 2085-2101 (patched)


Can you pull the printing out into a separate patch and show in the 
description what the format looks like?



src/common/resources_utils.cpp
Lines 42-44 (original), 42-44 (patched)


Wow I didn't realize this function existed, looks really brittle. Hopefully 
whenever we write integration tests for new operations it doesn't work unless 
this is updated. :)



src/common/resources_utils.cpp
Lines 56-57 (original), 56-60 (patched)


A little comment here would be great, took me a bit of time to understand 
what this was doing.



src/common/resources_utils.cpp
Lines 63-68 (original), 66-71 (patched)


Not yours, but I wish there was a little comment here about why source is 
left in (took me a bit of time to figure that out)


- Benjamin Mahler


On June 13, 2017, 8:35 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60017/
> ---
> 
> (Updated June 13, 2017, 8:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resources: Updated utilities.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/common/resources_utils.cpp c3088433d8c147b06dbef6f310fbe4059c3dc0ba 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60017/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60053: Revert "Disabled support for hierarchical roles".

[Benjamin Mahler]

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



A description would have been nice for posterity. E.g. we're re-enabling the 
support for hierarchical roles because we're planning to ship partial support 
(w/o quota?) in 1.4..? Seems a lot of context is needed to understand this?

- Benjamin Mahler


On June 13, 2017, 6:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60053/
> ---
> 
> (Updated June 13, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7505
> https://issues.apache.org/jira/browse/MESOS-7505
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit e0d8cc7c4ef7a0905044159ca47f64b0923e9788.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 139ae87970d6830a934cc84b0786ece29328d452 
>   src/tests/master_allocator_tests.cpp 
> a301c16fa52cd024dfc8f5f2fdc226810333ffb8 
>   src/tests/role_tests.cpp 03b6f7bd631850839b29b069ac63e7ef8841ad7c 
> 
> 
> Diff: https://reviews.apache.org/r/60053/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60016: Resources: Update the comparison functions.

[Benjamin Mahler]

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




src/common/resources.cpp
Line 223 (original), 231 (patched)


At first I wrote "Whoa, don't we still need to check role here?" and then I 
remembered you said all of the resources are normalized into the new format, 
but I needed a lot of non-local knowledge here, is that documented somewhere? 
It seems a little odd to have come into this patch reading validation code that 
shows two possible formats, but this code is assuming one over the other. We 
probably need a NOTE here and in other places that assume the new format to 
refer to a comment that talks about this invariant?

Shouldn't I have seen code before this patch that actually enforces that 
`Resources` contains only the new format?



src/common/resources.cpp
Line 239 (original), 245 (patched)


Ditto here, would be nice to refer to a comment about the new format being 
what all Resources are being normalized to.



src/common/resources.cpp
Line 312 (original), 320 (patched)


Ditto here.


- Benjamin Mahler


On June 13, 2017, 8:35 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60016/
> ---
> 
> (Updated June 13, 2017, 8:35 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resources: Update the comparison functions.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60016/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60008: Fixed bug causing FUTURE_DISPATCH to react on irrelevant dispatch.

[Michael Park]

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



Thanks for the patch! I'll try to get to this next week.

- Michael Park


On June 13, 2017, 5:46 a.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60008/
> ---
> 
> (Updated June 13, 2017, 5:46 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-5886
> https://issues.apache.org/jira/browse/MESOS-5886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FUTURE_DISPATCH uses DispatchMatcher to figure out whether a processed
> DispatchEvent is the same the user is waiting for. Currently, we
> compare std::type_info of function pointers, which is not enough:
> different class methods with same signatures will be matched (see
> MESOS-5886 for an example).
> This patch adds value of pointer-to-member function in addition to
> std::type_info in DispatchEvent to uniquely identify class methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/event.hpp 
> 8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
>   3rdparty/libprocess/include/process/gmock.hpp 
> e9af943b39436f365fe687301febb5c7fbefffc4 
>   3rdparty/libprocess/src/process.cpp 
> 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 38d787a083a5eb31e922d283f4b4bed2bd62eb0a 
> 
> 
> Diff: https://reviews.apache.org/r/60008/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> NOTE: Test GroupTest.ConnectTimer is broken, bacause it uses FUTURE_DISPATCH 
> on a wrong method (GroupProcess::expired),
> which has the same signature as a private method (GroupProcess::timedout). 
> The later method is actually called,
> thus causing the error after applying this patch.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

> On June 13, 2017, 10:58 a.m., Benjamin Mahler wrote:
> > src/common/resources.cpp
> > Lines 846-849 (original), 870-914 (patched)
> > 
> >
> > This was a little tough to read through, I wonder if the following 
> > would be easier to read:
> > 
> > ```
> > // Validate all of the roles.
> > foreach (const ReservationInfo& reservation, resource.reservations()) {
> >   Option error = roles::validate(reservation.role());
> >   if (error.isSome()) {
> > return error;
> >   }
> > 
> >   if (reservation.role() == "*") {
> > return Error("Invalid reservation: role \"*\" cannot be reserved");
> >   }
> > }
> > 
> > // Validate that each reservation is a refinement of the
> > // preceding reservation.
> > string parentRole = resource.reservations(0).role();
> > 
> > for (int i = 1; i < resource.reservations_size(); ++i) {
> >   if (resource.reservations(i).type() ==
> >   Resource::ReservationInfo::STATIC) {
> > return Error(
> > "Invalid refined reservation: A refined reservation"
> > " cannot be static.");
> >   }
> > 
> >   const string& childRole = resource.reservations(i).role();
> >   
> >   if (!isSubRole(parentRole, childRole)) {
> > return Error(
> > "Invalid refined reservation: role '" + childRole +
> > "' is not a refinement of '" + parentRole + "'");
> >   }
> >   
> >   parentRole = role;
> > }
> > ```
> > 
> > Granted, this doesn't have your tokenize optimization, but perhaps for 
> > now we just add a TODO to optimize this? It seems to me there are bigger 
> > optimizations than just halving the number of tokenizations, like avoiding 
> > the copying that tokenization incurs altogether.

Okay, the optimization is there because I'm already worried about this code 
because `Resources::validate` as far as remember is one of the hottest paths.


- Michael


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


On June 13, 2017, 1:34 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60013/
> ---
> 
> (Updated June 13, 2017, 1:34 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resources: Updated the validation logic.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
>   src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
>   src/tests/master_validation_tests.cpp 
> 4e7ce74edf0069b9317f869b299694a45e2a62f2 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60013/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

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

[Anand Mazumdar]

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



Looks good modulo comments from Jie. Mostly minor style nits.


src/launcher/default_executor.cpp
Lines 83 (patched)


One more new line after this.



src/launcher/default_executor.cpp
Lines 367 (patched)


s/env/environment



src/launcher/default_executor.cpp
Lines 438 (patched)


```cpp
// Set the `MESOS_CONTAINER_IP` environment for the task.
```



src/launcher/default_executor.cpp
Lines 441-445 (patched)


Can we do this after L372 to make them aligned with the code comment 
earlier? It looks more readable that way.

Here, we can just copy the environment into the `CommandInfo` once you make 
the earlier change.



src/launcher/default_executor.cpp
Lines 447 (patched)


Also, in addition to the verbosity, can we do it only once since it's the 
same for all the tasks in the task group?

See my earlier comments around moving this block outside.

Also, use `'MESOS_CONTAINER_IP'` in quotes inside the logline.


- Anand Mazumdar


On June 9, 2017, 6:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 9, 2017, 6:39 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 `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
> 
>

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

[Jie Yu]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp
Lines 371 (patched)


I'd add a TODO here. Because for bridge mode, `self().address.ip` might be 
the advertise IP (i.e., agent IP), which is not what we've wanted here.



src/launcher/default_executor.cpp
Lines 438 (patched)


Let's add a TODO to document this Downward API.



src/launcher/default_executor.cpp
Lines 447 (patched)


Why WARNING here? use INFO instead?


- Jie Yu


On June 9, 2017, 6:39 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59949/
> ---
> 
> (Updated June 9, 2017, 6:39 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 `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
> 
>

Re: Review Request 59320: Added test case for agent re-registration.

[Neil Conway]

> On May 31, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 6018-6025 (original), 6018-6025 (patched)
> > 
> >
> > Hm.. what does slaveWasRemoved mean? Is it equivalen to the agent being 
> > marked as unreachable? Or is being removed something beyond being marked as 
> > unreachable (what it sounds like)? I had a hard time grasping the code 
> > below when it was referring to "unreachable" but the variable is called 
> > "removed".

`slaveWasRemoved` means that _this_ instance of the master was the one that 
removed the agent (because `removed` is only a cache, there might be false 
negatives). Similarly, `removed` contains agents that _this_ instance of the 
master has marked unreachable. No objection to renaming both variables, as long 
as we keep them in sync, although I can't think of a ton of better names.


> On May 31, 2017, 12:09 a.m., Benjamin Mahler wrote:
> > src/master/master.cpp
> > Lines 6027-6043 (original), 6027-6043 (patched)
> > 
> >
> > Hm.. it seems like we could maybe make this code easier to read. 
> > Currently, the loop for dealing with frameworks is down below and the loop 
> > for dealing with tasks is up here. However, it seems to me that these are 
> > highly related, and it would be easier to reason about if it were 
> > consolidated into a loop over the framework and then a loop over the tasks 
> > belonging to that framework. That would also eliminate the need for 
> > `partitionAwareFrameworks`.
> > 
> > E.g.
> > 
> > ```
> >   foreach (const FrameworkInfo& framework, frameworks):
> > if completed:
> >   shut down the framework, drop the tasks
> >   continue
> >   
> > if agent was unreachable/removed and framework is not partition 
> > aware:
> >   shut down the framework, drop the tasks
> >   continue
> > 
> > for each task in the framework:
> >   recoveredTasks.push_back(task)
> >   remove from unreachable tasks
> > ```

Hmmm. I can see how this would makes things clearer, but it might result in 
some subtle behavioral differences. Right now, the master notifies the agent 
that it has re-registered, then sends it `ShutdownFrameworkMessage`s in some 
cases. Adopting the rewrite you suggested would mean shutting down frameworks 
while the agent thinks it hasn't re-registered yet. Not wrong but confusing.

Since this code is likely to change (see the "RFC: Partition Awareness" thread 
on `dev`), I'm inclined to leave this as-is in the short term.


- Neil


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


On May 31, 2017, 8:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59320/
> ---
> 
> (Updated May 31, 2017, 8:35 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add an explanatory comment and a test case for a particular case in the
> master's logic for handling agent re-registration.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp a5376f9fc3c3c6c72b22cfd36ec56a1de0d03a3a 
>   src/tests/partition_tests.cpp 62a84f797201ccd18b71490949e3130d2b9c3668 
> 
> 
> Diff: https://reviews.apache.org/r/59320/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Checked that if you add `CHECK_NOTNULL(framework)` to the master code (circa 
> the newly added comment), the incorrect `CHECK` is triggered by this test 
> case (but not by any existing test cases).
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 59746: Separated discarded and failed cases for container launch.

[Jie Yu]

> On June 2, 2017, 4:37 p.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Line 5147 (original), 5147 (patched)
> > 
> >
> > Can you explain to me in what scenario, the `future` will be in 
> > DISCARDED state? who discard the promise associated with this future?
> 
> Alexander Rukletsov wrote:
> Sure. Consider docker containerizer.
> 
> 1) During container launch, docker containerizer calls `pull()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L1238
> 2) The container enters `PULLING` state: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L435
> 3) While the image is being pulled by docker, future 
> `containers_[containerId]->pull` is returned from `pull()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L446
> 4) This future is part of the `.then` chain returned from `_launch()`: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L1269
> 5) Now while docker is pulling, `destroy()` is called, which discards the 
> "pulling future": 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L2126-L2128
> 6) But discarding that future is propagated up the chain: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/3rdparty/libprocess/include/process/future.hpp#L1410-L1411
> 7) Which triggers the `onAny` callback attached to launch: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/slave.cpp#L2800-L2810
> 8) Which in turn gives us discarded future treated as launch error: 
> https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/slave.cpp#L5147-L5152

https://github.com/apache/mesos/blob/9e61b3c7af35a29664361067c0bfa8b460bfefb9/src/slave/containerizer/docker.cpp#L2126-L2128

This discards the future, but not necessarily transition the future to 
DISCARDED state. That's the reason we have `hasDiscard` and `isDiscarded` 
methods for Future becaue they means different things. Can you point to me 
where the promise associated with this future is actually being transitioned 
into DISCARDED state?


- Jie


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


On June 2, 2017, 1:10 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59746/
> ---
> 
> (Updated June 2, 2017, 1:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joseph Wu, and Jan Schlicht.
> 
> 
> Bugs: MESOS-7601
> https://issues.apache.org/jira/browse/MESOS-7601
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Discarded future returned from the containerizer->launch() does not
> necessarily mean that the container launch has failed. For example,
> a framework may stop while its task are being started.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 5f80170fcd3c05add8b6e9e3107cff062818c1dc 
>   include/mesos/v1/mesos.proto 4b528751006f709f841e44f48c9f5c2dc035b402 
>   src/slave/slave.cpp 0c7e5f4ef905b3897d341c3147a208fc7a8a12e0 
> 
> 
> Diff: https://reviews.apache.org/r/59746/diff/1/
> 
> 
> Testing
> ---
> 
> make check on several Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 60015: Resources: Introduced `currentRole` and `currentPrincipal`.

[Benjamin Mahler]

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




include/mesos/resources.hpp
Lines 307-309 (patched)


How about:

```
reservationRole
reservationPrincipal
```

And noting that this is how people should get the role going forward, 
instead of accessing Resource.role directly.



src/common/resources.cpp
Lines 974-976 (original), 972-974 (patched)


Can you avoid the perfomance loss here? The code within Resources is 
performance sensitive. If I don't provide a role, no need to copy out the 
reservation role (i.e. the old logic was more efficient).



src/common/resources.cpp
Lines 986-988 (original), 982-985 (patched)


currentRole in this context seems less readable, how about 
`reservationRole` and the following:

I guess this function will be changing based on earlier feedback, so I 
won't leave more detailed feedback but note the roles::isSubRole I mentioned 
from the previous patch could be used here.


- Benjamin Mahler


On June 13, 2017, 8:34 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60015/
> ---
> 
> (Updated June 13, 2017, 8:34 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7655
> https://issues.apache.org/jira/browse/MESOS-7655
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Resources: Introduced `currentRole` and `currentPrincipal`.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
>   include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
>   src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
>   src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 
> 
> 
> Diff: https://reviews.apache.org/r/60015/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60053: Revert "Disabled support for hierarchical roles".

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On June 13, 2017, 6:08 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60053/
> ---
> 
> (Updated June 13, 2017, 6:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Neil Conway.
> 
> 
> Bugs: MESOS-7505
> https://issues.apache.org/jira/browse/MESOS-7505
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reverts commit e0d8cc7c4ef7a0905044159ca47f64b0923e9788.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 139ae87970d6830a934cc84b0786ece29328d452 
>   src/tests/master_allocator_tests.cpp 
> a301c16fa52cd024dfc8f5f2fdc226810333ffb8 
>   src/tests/role_tests.cpp 03b6f7bd631850839b29b069ac63e7ef8841ad7c 
> 
> 
> Diff: https://reviews.apache.org/r/60053/diff/1/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 60053: Revert "Disabled support for hierarchical roles".

[Michael Park]

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

(Updated June 13, 2017, 11:08 a.m.)


Review request for mesos, Benjamin Mahler and Neil Conway.


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


Repository: mesos


Description
---

This reverts commit e0d8cc7c4ef7a0905044159ca47f64b0923e9788.


Diffs
-

  src/common/roles.cpp 139ae87970d6830a934cc84b0786ece29328d452 
  src/tests/master_allocator_tests.cpp a301c16fa52cd024dfc8f5f2fdc226810333ffb8 
  src/tests/role_tests.cpp 03b6f7bd631850839b29b069ac63e7ef8841ad7c 


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


Testing (updated)
---

`make check`


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Benjamin Mahler]

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




src/common/resources.cpp
Lines 847-848 (patched)


We try not to use backticks in error or logging messages, they were 
introduced to provide markup for comments. Can you replace them with single 
quotes here and elsewhere?



src/common/resources.cpp
Lines 854 (patched)


Whoops, no periods in error messages



src/common/resources.cpp
Lines 844-845 (original), 864-868 (patched)


Do you want to add a function to roles.hpp for checking this?

```
bool isSubRole(const string& parentRole, const string& subRole);
```



src/common/resources.cpp
Lines 846-849 (original), 870-914 (patched)


This was a little tough to read through, I wonder if the following would be 
easier to read:

```
// Validate all of the roles.
foreach (const ReservationInfo& reservation, resource.reservations()) {
  Option error = roles::validate(reservation.role());
  if (error.isSome()) {
return error;
  }

  if (reservation.role() == "*") {
return Error("Invalid reservation: role \"*\" cannot be reserved");
  }
}

// Validate that each reservation is a refinement of the
// preceding reservation.
string parentRole = resource.reservations(0).role();

for (int i = 1; i < resource.reservations_size(); ++i) {
  if (resource.reservations(i).type() ==
  Resource::ReservationInfo::STATIC) {
return Error(
"Invalid refined reservation: A refined reservation"
" cannot be static.");
  }

  const string& childRole = resource.reservations(i).role();
  
  if (!isSubRole(parentRole, childRole)) {
return Error(
"Invalid refined reservation: role '" + childRole +
"' is not a refinement of '" + parentRole + "'");
  }
  
  parentRole = role;
}
```

Granted, this doesn't have your tokenize optimization, but perhaps for now 
we just add a TODO to optimize this? It seems to me there are bigger 
optimizations than just halving the number of tokenizations, like avoiding the 
copying that tokenization incurs altogether.



src/common/resources.cpp
Lines 907-908 (patched)


We try to open and close the quote on the same line (otherwise we tend to 
forget to close because it's not as obvious), can you do that here?



src/master/validation.hpp
Lines 139 (patched)


Option bool seems a little confusing to me, since I assume false is 
equivalent to None()?

Why not just take optional framework info or to be more specific, optional 
framework capabilities here?



src/master/validation.hpp
Line 154 (original), 155 (patched)


Why not take optional framework capabilities or framework info here? Rather 
than the framework state?



src/master/validation.hpp
Line 182 (original), 183 (patched)


Ditto here



src/master/validation.hpp
Lines 188 (patched)


Ditto here.



src/master/validation.cpp
Lines 601-603 (patched)


Can you add a comment about what this is doing? i.e. that we have an old 
and new format for resource role / reservations? Perhaps name this 
`validateReservationFormat` or something that is a little more specific towards 
that?



src/master/validation.cpp
Lines 603 (patched)


Why not just take the capabilities here, instead of taking the bool?



src/master/validation.cpp
Lines 609-610 (patched)


I don't think we're using periods in these messages (we generally don't)

Also, no backticks in error or logging messages (just comments) (some 
leaked in)

Also, we have been generally trying to put the space on the next line, e.g. 
from below:

```
return Error(
"Dynamically reserved resource " + stringify(resource) +
" cannot be created from revocable resources");
```



src/master/validation.cpp
Lines 615-616 (patched)


Ditto here, no period / backticks.

Also could you put the space on the next line to match our other logging? 
(We started doing this because at the end of the line it's less obvious when we 
missed it).

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

[Jay Guo]

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

(Updated June 13, 2017, 10:35 a.m.)


Review request for mesos and Michael Park.


Changes
---

Updated dependency.


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


Repository: mesos


Description
---

A reservation is allocatable to 'role' iff:
 - it is reserved to ancestor of 'role' in hierarchy, OR
 - it is reserved to 'role' itself, OR
 - it is unreserved.

This is a helper function for reservation delegate, where reserved
resources can be 'delegated' downwards in the hierarchy.


Diffs
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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


Testing
---

comment out 
https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
`./bin/mesos-tests.sh 
--gtest_filter="ResourcesTest.DISABLED_HierarchicalReservations" 
--gtest_also_run_disabled_tests`


Thanks,

Jay Guo

Review Request 60053: Revert "Disabled support for hierarchical roles".

[Michael Park]

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

Review request for mesos, Benjamin Mahler and Neil Conway.


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


Repository: mesos


Description
---

This reverts commit e0d8cc7c4ef7a0905044159ca47f64b0923e9788.


Diffs
-

  src/common/roles.cpp 139ae87970d6830a934cc84b0786ece29328d452 
  src/tests/master_allocator_tests.cpp a301c16fa52cd024dfc8f5f2fdc226810333ffb8 
  src/tests/role_tests.cpp 03b6f7bd631850839b29b069ac63e7ef8841ad7c 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 60035: Added support for printing JSON values via `jsonify`.

[Michael Park]

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

(Updated June 13, 2017, 10:28 a.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Added support for printing JSON values via `jsonify`.


Diffs (updated)
-

  3rdparty/stout/include/stout/json.hpp 
aa581c0345679b6f9617fb986d744110a728f042 
  3rdparty/stout/include/stout/jsonify.hpp 
7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
  3rdparty/stout/tests/jsonify_tests.cpp 
d06eba31e6b7f252b120a10066e4d7c9a25ce4b5 


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

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


Testing
---

Added new tests + `make check`


Thanks,

Michael Park

Re: Review Request 60035: Added support for printing JSON values via `jsonify`.

[Michael Park]

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

(Updated June 13, 2017, 10:10 a.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Added support for printing JSON values via `jsonify`.


Repository: mesos


Description (updated)
---

Added support for printing JSON values via `jsonify`.


Diffs (updated)
-

  3rdparty/stout/include/stout/json.hpp 
aa581c0345679b6f9617fb986d744110a728f042 
  3rdparty/stout/include/stout/jsonify.hpp 
7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
  3rdparty/stout/tests/jsonify_tests.cpp 
d06eba31e6b7f252b120a10066e4d7c9a25ce4b5 


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

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


Testing (updated)
---

Added new tests + `make check`


Thanks,

Michael Park

Re: Review Request 58510: Added a test for hierarchical reservation.

[Jay Guo]

> On June 10, 2017, 3:22 a.m., Benjamin Mahler wrote:
> > 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?

Since we made changes for both quota and fair share stage of allocation, I'm 
using quota to make sure we cover both logic paths. I updated the test 
comments, please take a look.


> On June 10, 2017, 3:22 a.m., Benjamin Mahler wrote:
> > 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?)

Dropped the test.


- Jay


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


On June 13, 2017, 11:40 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58510/
> ---
> 
> (Updated June 13, 2017, 11:40 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7149
> https://issues.apache.org/jira/browse/MESOS-7149
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for hierarchical reservation.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 631e800e6566847e015ea2638cc1c2fe673855be 
> 
> 
> Diff: https://reviews.apache.org/r/58510/diff/2/
> 
> 
> Testing
> ---
> 
> make check
> 
> comment out 
> https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
> `./bin/mesos-tests.sh 
> --gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild"
>  --gtest_also_run_disabled_tests`
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On June 13, 2017, 3:42 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58250/
> ---
> 
> (Updated June 13, 2017, 3:42 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Test that bind-mounted host network configuration is mounted readonly.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 
> 
> 
> Diff: https://reviews.apache.org/r/58250/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Re: Review Request 57884: Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.

[Silas Snider]

> On April 6, 2017, 12:22 a.m., Jie Yu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp
> > Lines 1523 (patched)
> > 
> >
> > Instead of putting this test under LinuxFilesystemIsolatorTest, I would 
> > put it under CniIsolatorTest. The code change is in CNI isolator, so 
> > putting it in CniIsolatorTest is more appropriate?
> > 
> > I would write a similar test like this test:
> > TEST_F(CniIsolatorTest, ROOT_OverrideHostname)
> 
> Silas Snider wrote:
> I disagree (though not super strongly) -- I think that the functionality 
> being guarded/tested is that the linux fs isolator correctly mounts the /etc/ 
> networking files readonly. That it does so *today* by stealth-including the 
> CNI isolator is an implementation detail, and a surprising one at that. To 
> protect against the general case, I put it here.
> 
> If you still think it should move, I'm totally willing to move it though.
> 
> Jie Yu wrote:
> the mount is done by the CNI isolator, not linux filesystem isolator. 
> Even in the future, we allow isolator dependency, and let linux filesystem 
> isolator handles all the bind mount, this is still a logic (read-only bind 
> mount for /etc) introduced by CNI isolator. Put that in other words, if we 
> disable cni isolator and enable linux filesystem isolator, your test will 
> fail.

I finally got the CNI tests working locally per our slack discussion yesterday, 
so I now have a working test that fails before this change and succeeds 
afterwards in the CNI isolator tests in review request 
https://reviews.apache.org/r/58250/


- Silas


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


On April 5, 2017, 7:53 p.m., Silas Snider wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57884/
> ---
> 
> (Updated April 5, 2017, 7:53 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-7268
> https://issues.apache.org/jira/browse/MESOS-7268
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure that host /etc/* files are mounted RDONLY by the CNI Isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 6e95315b70a5d9d3b4b21c4cf235b0a483760190 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/57884/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Silas Snider
> 
>

Review Request 58250: Test that bind-mounted host network configuration is mounted readonly.

[Silas Snider]

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

Review request for mesos.


Repository: mesos


Description
---

Test that bind-mounted host network configuration is mounted readonly.


Diffs
-

  src/tests/containerizer/cni_isolator_tests.cpp 3a5f4ebd4 


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


Testing
---


Thanks,

Silas Snider

Re: Review Request 58510: Added a test for hierarchical reservation.

[Jay Guo]

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

(Updated June 13, 2017, 11:40 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

Added a test for hierarchical reservation.


Diffs
-

  src/tests/hierarchical_allocator_tests.cpp 
631e800e6566847e015ea2638cc1c2fe673855be 


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


Testing (updated)
---

make check

comment out 
https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
`./bin/mesos-tests.sh 
--gtest_filter="HierarchicalAllocatorTest.DISABLED_OfferAncestorReservationsToDescendantChild"
 --gtest_also_run_disabled_tests`


Thanks,

Jay Guo

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

[Jay Guo]

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

(Updated June 13, 2017, 11:38 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 allocatable to 'role' iff:
 - it is reserved to ancestor of 'role' in hierarchy, OR
 - it is reserved to 'role' itself, OR
 - it is unreserved.

This is a helper function for reservation delegate, where reserved
resources can be 'delegated' downwards in the hierarchy.


Diffs
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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


Testing (updated)
---

comment out 
https://github.com/apache/mesos/blob/master/src/common/roles.cpp#L71-L74 and 
`./bin/mesos-tests.sh 
--gtest_filter="ResourcesTest.DISABLED_HierarchicalReservations" 
--gtest_also_run_disabled_tests`


Thanks,

Jay Guo

Re: Review Request 58510: Added a test for hierarchical reservation.

[Jay Guo]

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

(Updated June 13, 2017, 11:31 p.m.)


Review request for mesos and Michael Park.


Changes
---

rebase and address comments.


Summary (updated)
-

Added a test for hierarchical reservation.


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


Repository: mesos


Description (updated)
---

Added a test for hierarchical reservation.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
631e800e6566847e015ea2638cc1c2fe673855be 


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

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


Testing
---

make check


Thanks,

Jay Guo

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

[Jay Guo]

> On June 9, 2017, 2:23 a.m., Neil Conway wrote:
> > src/tests/reservation_tests.cpp
> > Lines 2401 (patched)
> > 
> >
> > I have various minor gripes about how this test is written :)
> > 
> > When the clock is paused, I think it is better to first advance the 
> > clock to ensure the agent has registered. Then register the framework; we 
> > should then get an offer at precisely that point (w/o waiting for batch 
> > alloc). As written, advancing the clock for the batch alloc is actually 
> > what triggers the agent to register, which is fragile (what if agent 
> > registration backoff is > batch alloc interval?).
> > 
> > i.e., I'd adjust the test as follows: 
> > https://gist.github.com/neilconway/2f11988222cd8fb9583905624cb4ddd5

Thank you for great advice! Although I removed this test per @bmahler's 
comment, I'm sure this will help me in the future :)


- Jay


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


On April 18, 2017, 11: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, 11: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 58509: Enabled allocator to handle hierarchical reservation.

[Jay Guo]

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

(Updated June 13, 2017, 11:27 p.m.)


Review request for mesos and Michael Park.


Changes
---

rebase and address comments


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


Repository: mesos


Description (updated)
---

When collecting resources allocatable to a role, allocator should
aggregate not only reservations of the role itself, but also that
of all its ancestors.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
73d0b42433a1ff3e853e851b8191864b4e233666 


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

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


Testing
---

see next patch in chain


Thanks,

Jay Guo

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

[Jay Guo]

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

(Updated June 13, 2017, 11:23 p.m.)


Review request for mesos and Michael Park.


Changes
---

rebase and address comments


Summary (updated)
-

Introduced method `Resources::allocatableTo` to aggregate reservations.


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


Repository: mesos


Description (updated)
---

A reservation is allocatable to 'role' iff:
 - it is reserved to ancestor of 'role' in hierarchy, OR
 - it is reserved to 'role' itself, OR
 - it is unreserved.

This is a helper function for reservation delegate, where reserved
resources can be 'delegated' downwards in the hierarchy.


Diffs (updated)
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 59525: Enabled filterirng of reservations.

[Mesos Reviewbot Windows]

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



Patch looks great!

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

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 2:12 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59525/
> ---
> 
> (Updated June 13, 2017, 2:12 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, '/states' and
> '/states-summary'.
> 
> This means that calls to these endpoints and API call which uses
> reservations will only show the reserved resources the user making the
> requests is allowed to see based on the reservations roles.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp e8f273256b14cde1cac390163f948241757f 
>   src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 
> 
> 
> Diff: https://reviews.apache.org/r/59525/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59525: Enabled filterirng of reservations.

[Alexander Rojas]

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

(Updated June 13, 2017, 4:12 p.m.)


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


Summary (updated)
-

Enabled filterirng of reservations.


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, '/states' and
'/states-summary'.

This means that calls to these endpoints and API call which uses
reservations will only show the reserved resources the user making the
requests is allowed to see based on the reservations roles.


Diffs (updated)
-

  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/master/master.hpp e8f273256b14cde1cac390163f948241757f 
  src/tests/master_tests.cpp 490d7ed4b275ebf5ff6956f7d40dbea3ce3b63e2 


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

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 60008: Fixed bug causing FUTURE_DISPATCH to react on irrelevant dispatch.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [60008]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 12:46 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60008/
> ---
> 
> (Updated June 13, 2017, 12:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Michael Park.
> 
> 
> Bugs: MESOS-5886
> https://issues.apache.org/jira/browse/MESOS-5886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FUTURE_DISPATCH uses DispatchMatcher to figure out whether a processed
> DispatchEvent is the same the user is waiting for. Currently, we
> compare std::type_info of function pointers, which is not enough:
> different class methods with same signatures will be matched (see
> MESOS-5886 for an example).
> This patch adds value of pointer-to-member function in addition to
> std::type_info in DispatchEvent to uniquely identify class methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/event.hpp 
> 8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
>   3rdparty/libprocess/include/process/gmock.hpp 
> e9af943b39436f365fe687301febb5c7fbefffc4 
>   3rdparty/libprocess/src/process.cpp 
> 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 38d787a083a5eb31e922d283f4b4bed2bd62eb0a 
> 
> 
> Diff: https://reviews.apache.org/r/60008/diff/1/
> 
> 
> Testing
> ---
> 
> 1. make check (mac os x 10.12, fedora 25)
> 2. internal CI
> 
> NOTE: Test GroupTest.ConnectTimer is broken, bacause it uses FUTURE_DISPATCH 
> on a wrong method (GroupProcess::expired),
> which has the same signature as a private method (GroupProcess::timedout). 
> The later method is actually called,
> thus causing the error after applying this patch.
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 60008: Fixed bug causing FUTURE_DISPATCH to react on irrelevant dispatch.

[Andrei Budnik]

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

Review request for mesos, Alexander Rukletsov and Michael Park.


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


Repository: mesos


Description
---

FUTURE_DISPATCH uses DispatchMatcher to figure out whether a processed
DispatchEvent is the same the user is waiting for. Currently, we
compare std::type_info of function pointers, which is not enough:
different class methods with same signatures will be matched (see
MESOS-5886 for an example).
This patch adds value of pointer-to-member function in addition to
std::type_info in DispatchEvent to uniquely identify class methods.


Diffs
-

  3rdparty/libprocess/include/process/dispatch.hpp 
3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
  3rdparty/libprocess/include/process/event.hpp 
8afe6266eb0dc5a17af35d79efb6bfdf9e6a0ee9 
  3rdparty/libprocess/include/process/gmock.hpp 
e9af943b39436f365fe687301febb5c7fbefffc4 
  3rdparty/libprocess/src/process.cpp 4ff7448d171f39dbb8cbb81dd9bed136ad43d62d 
  3rdparty/libprocess/src/tests/process_tests.cpp 
38d787a083a5eb31e922d283f4b4bed2bd62eb0a 


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


Testing
---

1. make check (mac os x 10.12, fedora 25)
2. internal CI

NOTE: Test GroupTest.ConnectTimer is broken, bacause it uses FUTURE_DISPATCH on 
a wrong method (GroupProcess::expired),
which has the same signature as a private method (GroupProcess::timedout). The 
later method is actually called,
thus causing the error after applying this patch.


Thanks,

Andrei Budnik

Re: Review Request 60001: Removed superfluous log message in docker containerizer.

[Gilbert Song]

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


Ship it!




LGTM!

- Gilbert Song


On June 12, 2017, 6:36 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60001/
> ---
> 
> (Updated June 12, 2017, 6:36 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 9f84109d7de22a39ace6e44e0c7d8d501bcb24de 
> 
> 
> Diff: https://reviews.apache.org/r/60001/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 59147: Enabled authorization for v1 calls starting and stopping maintenance.

[Alexander Rojas]

> On June 12, 2017, 11:57 p.m., Till Toenshoff wrote:
> > src/master/master.hpp
> > Line 30 (original), 30 (patched)
> > 
> >
> > Aren't we missing 
> > ```
> > #include  
> > ```

Nope, the authorizer is forward delcared 
[here](https://github.com/apache/mesos/blob/03342092988bf62a90d429309ab95f0c36a3c851/src/master/master.hpp#L94)
 and the included in 
[cpp](https://github.com/apache/mesos/blob/03342092988bf62a90d429309ab95f0c36a3c851/src/master/master.cpp#L33)


- Alexander


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


On June 12, 2017, 7:30 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59147/
> ---
> 
> (Updated June 12, 2017, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7415
> https://issues.apache.org/jira/browse/MESOS-7415
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables the use of authorization for the 'START_MAINTENANCE' and
> 'STOP_MAINTENANCE' v1 API calls, using the ACLs 'StartMaintenance' and
> 'StopMaintenance' respectively as well the actions of the same name as
> the API calls.
> 
> It also updates the ApiTests to take into account the authorization
> case.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/master/master.hpp e8f273256b14cde1cac390163f948241757f 
>   src/tests/api_tests.cpp 91b3473452b8e65cab9f2e873837d64a0edf4b54 
>   src/tests/master_maintenance_tests.cpp 
> 9edd74a876cd2933d5a5be590a7dd3d10bc54253 
> 
> 
> Diff: https://reviews.apache.org/r/59147/diff/7/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 59989: Added a test `ProtobufTest.JsonifyMap`.

[Mesos Reviewbot Windows]

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



Patch looks great!

Reviews applied: [59987, 59988, 59989]

Passed command: support\windows-build.bat

- Mesos Reviewbot Windows


On June 13, 2017, 8:47 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59989/
> ---
> 
> (Updated June 13, 2017, 8:47 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ProtobufTest.JsonifyMap`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> 8877e8934e0f7875bfedcfa88b491ce4b13ca44f 
> 
> 
> Diff: https://reviews.apache.org/r/59989/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60038: Prevent allocating reservation refinements to non-capable frameworks.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: [60038, 60041, 60040, 60022, 60021, 60020, 60019, 60018, 
60017, 60016, 60015, 60013, 60036, 60035, 59861, 59860, 59859, 58510, 58509, 
58508, 57516, 58584, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 
57166, 56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On June 13, 2017, 8:44 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60038/
> ---
> 
> (Updated June 13, 2017, 8:44 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Prevent allocating reservation refinements to non-capable frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 73d0b42433a1ff3e853e851b8191864b4e233666 
> 
> 
> Diff: https://reviews.apache.org/r/60038/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 59988: Added a new protobuf message `MapMessage` for protobuf tests.

[Benjamin Bannier]

> On June 12, 2017, 5:41 p.m., Zhitao Li wrote:
> > It seems that  majority of this patch is generated code. Is the `.proto` 
> > change the only real code change? If so, should we write some c++ test code 
> > to use the map based fields and json parsing of map?
> > 
> > Also, explaining which files are generated in summary will help reviewers 
> > to know what changes we can safekly skip reading?
> 
> Qian Zhang wrote:
> Yes, the `.proto` change is the only real code changed, I have updated 
> the description to mention that. And for the test, please refer to 
> https://reviews.apache.org/r/59989/.

Could you have a look and try to find out how hard it would be to generate the 
`*.pb.*` files during the build instead of checking them in, 
https://issues.apache.org/jira/browse/MESOS-6115?


- Benjamin


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


On June 13, 2017, 10:41 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> ---
> 
> (Updated June 13, 2017, 10:41 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.pb.h 
> 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62 
>   3rdparty/stout/tests/protobuf_tests.pb.cc 
> ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6 
>   3rdparty/stout/tests/protobuf_tests.proto 
> d16726aa8060aea2b830040b20dbdd467c801483 
> 
> 
> Diff: https://reviews.apache.org/r/59988/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59989: Added a test `ProtobufTest.JsonifyMap`.

[Qian Zhang]

> On June 12, 2017, 11:44 p.m., Zhitao Li wrote:
> > 3rdparty/stout/tests/protobuf_tests.cpp
> > Lines 614 (patched)
> > 
> >
> > Mind explaining how the magical numerical numbers are picked?

Sure, I have added a comment for that. Actually I use the same set of large 
number with another test `ProtobufTest.JsonifyLargeIntegers` :-)


- Qian


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


On June 13, 2017, 4:47 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59989/
> ---
> 
> (Updated June 13, 2017, 4:47 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `ProtobufTest.JsonifyMap`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.cpp 
> 8877e8934e0f7875bfedcfa88b491ce4b13ca44f 
> 
> 
> Diff: https://reviews.apache.org/r/59989/diff/2/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 59989: Added a test `ProtobufTest.JsonifyMap`.

[Qian Zhang]

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

(Updated June 13, 2017, 4:47 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

Addressed Zhitao's comments.


Repository: mesos


Description
---

Added a test `ProtobufTest.JsonifyMap`.


Diffs (updated)
-

  3rdparty/stout/tests/protobuf_tests.cpp 
8877e8934e0f7875bfedcfa88b491ce4b13ca44f 


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

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


Testing
---

sudo make check


Thanks,

Qian Zhang

Re: Review Request 60041: Devolved reserved resources for the HTTP endpoints.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: [60041, 60040, 60022, 60021, 60020, 60019, 60018, 60017, 
60016, 60015, 60013, 60036, 60035, 59861, 59860, 59859, 58510, 58509, 58508, 
57516, 58584, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 
56805, 57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On June 13, 2017, 4:43 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60041/
> ---
> 
> (Updated June 13, 2017, 4:43 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Devolved reserved resources for the HTTP endpoints.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
>   src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
>   src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 
> 
> 
> Diff: https://reviews.apache.org/r/60041/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 60038: Prevent allocating reservation refinements to non-capable frameworks.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Prevent allocating reservation refinements to non-capable frameworks.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
73d0b42433a1ff3e853e851b8191864b4e233666 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 59988: Added a new protobuf message `MapMessage` for protobuf tests.

[Qian Zhang]

> On June 12, 2017, 11:41 p.m., Zhitao Li wrote:
> > It seems that  majority of this patch is generated code. Is the `.proto` 
> > change the only real code change? If so, should we write some c++ test code 
> > to use the map based fields and json parsing of map?
> > 
> > Also, explaining which files are generated in summary will help reviewers 
> > to know what changes we can safekly skip reading?

Yes, the `.proto` change is the only real code changed, I have updated the 
description to mention that. And for the test, please refer to 
https://reviews.apache.org/r/59989/.


- Qian


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


On June 13, 2017, 4:41 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/59988/
> ---
> 
> (Updated June 13, 2017, 4:41 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Zhitao Li.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The real code changes is adding `MapMessage` to protobuf_tests.proto,
> for the other two files in this patch, they are automatically generated
> by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/protobuf_tests.pb.h 
> 2e4ffe17a07ce2360ec618e936ae4557e9dc8e62 
>   3rdparty/stout/tests/protobuf_tests.pb.cc 
> ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6 
>   3rdparty/stout/tests/protobuf_tests.proto 
> d16726aa8060aea2b830040b20dbdd467c801483 
> 
> 
> Diff: https://reviews.apache.org/r/59988/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 60041: Devolved reserved resources for the HTTP endpoints.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Devolved reserved resources for the HTTP endpoints.


Diffs
-

  src/common/http.cpp 167dce2b9a2d3b68a1df5b4079f701482d34db28 
  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 


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


Testing
---


Thanks,

Michael Park

Review Request 60040: Resources: Updated to evolve/devolve reserved resources.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Resources: Updated to evolve/devolve reserved resources.


Diffs
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 59988: Added a new protobuf message `MapMessage` for protobuf tests.

[Qian Zhang]

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

(Updated June 13, 2017, 4:41 p.m.)


Review request for mesos, Anand Mazumdar and Zhitao Li.


Changes
---

Addressed Zhitao's comments.


Repository: mesos


Description (updated)
---

The real code changes is adding `MapMessage` to protobuf_tests.proto,
for the other two files in this patch, they are automatically generated
by running protobuf-3.3.0 compiler `protoc` on protobuf_tests.proto.


Diffs (updated)
-

  3rdparty/stout/tests/protobuf_tests.pb.h 
2e4ffe17a07ce2360ec618e936ae4557e9dc8e62 
  3rdparty/stout/tests/protobuf_tests.pb.cc 
ad6eff779d1cc0e7d037ea77565533c3ebb0b2d6 
  3rdparty/stout/tests/protobuf_tests.proto 
d16726aa8060aea2b830040b20dbdd467c801483 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 60021: Corrected the use of `toUnreserved()` to be more accurate.

[Michael Park]

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

(Updated June 13, 2017, 1:37 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Corrected the use of `toUnreserved()` to be more accurate.


Diffs (updated)
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60019: Removed the only use of `flatten` in Mesos core.

[Michael Park]

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

(Updated June 13, 2017, 1:36 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

Removed the only use of `flatten` in Mesos core.


Diffs (updated)
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60020: Added pushReservation and popReservation.

[Michael Park]

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

(Updated June 13, 2017, 1:36 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


Summary (updated)
-

Added pushReservation and popReservation.


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


Repository: mesos


Description (updated)
---

Added pushReservation and popReservation.


Diffs (updated)
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60018: Renamed `flatten()` to `toUnreserved()`.

[Michael Park]

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

(Updated June 13, 2017, 1:35 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Renamed `flatten()` to `toUnreserved()`.


Diffs (updated)
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/cli/execute.cpp 11a2569f788a2ee1ceaf13ab0d40d1d1b275f27a 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/examples/balloon_framework.cpp bfaae3796afb426459ef5cff3a37634a8242032e 
  src/examples/disk_full_framework.cpp a73e6cf285b9a74492d476a624915235e079051f 
  src/examples/long_lived_framework.cpp 
bbe8606c8d2572ad695e44fa23ce7b6129eb 
  src/examples/test_framework.cpp 05ddc89d953d9b968b78f98fec819aeda7f79e26 
  src/examples/test_http_framework.cpp 471835c349e0da031a540ed48881227a25887ba7 
  src/master/allocator/mesos/hierarchical.cpp 
73d0b42433a1ff3e853e851b8191864b4e233666 
  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/tests/mesos.hpp 48072a976cdbe6e655dc6f5f258abc6d737ed068 
  src/tests/reservation_endpoints_tests.cpp 
ba283ee74fe27998a01c43e1ab6a8ce0089181fa 
  src/tests/resources_tests.cpp 71c7544470f44f84ef90f9b31afcd81d46493981 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60017: Resources: Updated utilities.

[Michael Park]

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

(Updated June 13, 2017, 1:35 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Resources: Updated utilities.


Diffs (updated)
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/common/resources_utils.cpp c3088433d8c147b06dbef6f310fbe4059c3dc0ba 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60016: Resources: Update the comparison functions.

[Michael Park]

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

(Updated June 13, 2017, 1:35 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Resources: Update the comparison functions.


Diffs (updated)
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60013: Resources: Updated the validation logic.

[Michael Park]

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

(Updated June 13, 2017, 1:34 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Resources: Updated the validation logic.


Diffs (updated)
-

  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/master/validation.hpp 6b53e34f67f6a8b338b92db6cb7398f1dccce5a9 
  src/master/validation.cpp f45f9e816bdaf6304794604570edbd68d5faf715 
  src/tests/master_validation_tests.cpp 
4e7ce74edf0069b9317f869b299694a45e2a62f2 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60015: Resources: Introduced `currentRole` and `currentPrincipal`.

[Michael Park]

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

(Updated June 13, 2017, 1:34 a.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Resources: Introduced `currentRole` and `currentPrincipal`.


Diffs (updated)
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  include/mesos/v1/resources.hpp c790927353122a4eb2bbbee31c6df57d97f3994b 
  src/common/resources.cpp f89f1aae7845f1e93dd5947de2e7a8e2bfce8bc4 
  src/v1/resources.cpp 24240eedc4b2747ca02998534437318c3396db4d 


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

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


Testing
---


Thanks,

Michael Park

Re: Review Request 60035: WIP: Added support for printing JSON values via `jsonify`.

[Mesos Reviewbot Windows]

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



Bad review!

Reviews applied: [60035, 59861, 59860, 59859, 58510, 58509, 58508, 57516, 
58584, 57254, 58112, 58110, 57564, 57528, 57527, 57788, 57167, 57166, 56805, 
57165, 57164]

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos Reviewbot Windows


On June 13, 2017, 1:31 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60035/
> ---
> 
> (Updated June 13, 2017, 1:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> aa581c0345679b6f9617fb986d744110a728f042 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
> 
> 
> Diff: https://reviews.apache.org/r/60035/diff/1/
> 
> 
> Testing
> ---
> 
> TODO
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 60036: Added a hack to avoid printing out the `Resource.role` field.

[Michael Park]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  include/mesos/resources.hpp 451b0cef6f84743e3d630104ba6f55665793f2bc 
  src/master/http.cpp 1dcfe6ef00b0e3984deb79a511e665f638661323 
  src/slave/http.cpp 78b35865e465ff1e8e7e4950fdb60e3a48b916b6 


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


Testing
---


Thanks,

Michael Park

Re: Review Request 60035: WIP: Added support for printing JSON values via `jsonify`.

[Michael Park]

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




3rdparty/stout/include/stout/json.hpp
Lines 711-714 (patched)


Leave a comment about this.


- Michael Park


On June 13, 2017, 1:31 a.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60035/
> ---
> 
> (Updated June 13, 2017, 1:31 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/json.hpp 
> aa581c0345679b6f9617fb986d744110a728f042 
>   3rdparty/stout/include/stout/jsonify.hpp 
> 7d84bac18abbe9f5bcf67a377d4e742e31b4c94d 
> 
> 
> Diff: https://reviews.apache.org/r/60035/diff/1/
> 
> 
> Testing
> ---
> 
> TODO
> 
> 
> Thanks,
> 
> Michael Park
> 
>