Re: Review Request 54590: Removed unused peek function.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54590]

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 Dec. 13, 2016, 8:44 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54590/
> ---
> 
> (Updated Dec. 13, 2016, 8:44 p.m.)
> 
> 
> Review request for mesos and Daniel Pravat.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> b9825e8633f64c23e4b1ea904537cdc8da64ed5b 
> 
> Diff: https://reviews.apache.org/r/54590/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53790, 54712, 53791]

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 Dec. 13, 2016, 6:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Dec. 13, 2016, 6:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

[Zhitao Li]

> On Dec. 13, 2016, 12:47 a.m., Anand Mazumdar wrote:
> > src/tests/api_tests.cpp, line 1493
> > 
> >
> > Can you do this outside of this scope as it's common to all subsequent 
> > blocks? It would then be consistent to all the other tests in this file.

I think I will do this in a separate sweeping patch instead of this one.


- Zhitao


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


On Dec. 14, 2016, 4:49 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Dec. 14, 2016, 4:49 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 
>   src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53344: Updated `/slaves.md` doc.

[Zhitao Li]

> On Dec. 13, 2016, 12:52 a.m., Anand Mazumdar wrote:
> > Did you manually edit this file or reran the 
> > `support/generate-endpoint-help.py` script?

I used the `support/generate-endpoint-help.py` to generate this file.


- Zhitao


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


On Dec. 14, 2016, 4:52 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53344/
> ---
> 
> (Updated Dec. 14, 2016, 4:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `/slaves.md` doc.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/slaves.md 90d0fb8496866b70777e5dbc86449b14ae1cc910 
> 
> Diff: https://reviews.apache.org/r/53344/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 53344: Updated `/slaves.md` doc.

[Zhitao Li]

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

(Updated Dec. 14, 2016, 4:52 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


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


Repository: mesos


Description
---

Updated `/slaves.md` doc.


Diffs (updated)
-

  docs/endpoints/master/slaves.md 90d0fb8496866b70777e5dbc86449b14ae1cc910 

Diff: https://reviews.apache.org/r/53344/diff/


Testing
---


Thanks,

Zhitao Li

Review Request 54732: Reused same json helper for `SlaveInfo`.

[Zhitao Li]

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

Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


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


Repository: mesos


Description
---

Reused same json helper for `SlaveInfo`.


Diffs
-

  src/master/http.cpp d52806dcf8e4d64ebb98e191a01408c0fcae17ac 

Diff: https://reviews.apache.org/r/54732/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52639: Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.

[Zhitao Li]

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

(Updated Dec. 14, 2016, 4:49 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


Changes
---

Split test as requested.


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


Repository: mesos


Description
---

Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.


Diffs (updated)
-

  src/tests/api_tests.cpp 82c0fc27e5e707adb73faeb26828a2ce3e3feb16 
  src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 

Diff: https://reviews.apache.org/r/52639/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 52765: Populated `recovered_slaves` in `/state` and `/slaves` endpoints.

[Zhitao Li]

> On Dec. 12, 2016, 10:11 p.m., Anand Mazumdar wrote:
> > src/master/http.cpp, line 158
> > 
> >
> > Any reason for not adding `port` here?

It was never added in the past. I will add it in this patch.


- Zhitao


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


On Dec. 14, 2016, 4:48 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52765/
> ---
> 
> (Updated Dec. 14, 2016, 4:48 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Populated `recovered_slaves` in `/state` and `/slaves` endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d52806dcf8e4d64ebb98e191a01408c0fcae17ac 
>   src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 
> 
> Diff: https://reviews.apache.org/r/52765/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52765: Populated `recovered_slaves` in `/state` and `/slaves` endpoints.

[Zhitao Li]

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

(Updated Dec. 14, 2016, 4:48 a.m.)


Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.


Changes
---

- Skip resources since it may not reflect checkpointed resources;
- Add port;
- comment fix.


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


Repository: mesos


Description
---

Populated `recovered_slaves` in `/state` and `/slaves` endpoints.


Diffs (updated)
-

  src/master/http.cpp d52806dcf8e4d64ebb98e191a01408c0fcae17ac 
  src/tests/master_tests.cpp b3b5630899082a69119d2cccb6460766d1352963 

Diff: https://reviews.apache.org/r/52765/diff/


Testing
---


Thanks,

Zhitao Li

Re: Review Request 54731: Fixed a check bug in LAUNCH_NESTED_CONTAINER_SESSION_CALL.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Dec. 13, 2016, 8:15 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54731/
> ---
> 
> (Updated Dec. 13, 2016, 8:15 p.m.)
> 
> 
> Review request for mesos, Adam B and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `attachContainerOutput` could return a non OK response (e.g., authz
> error). We should return the response in this case instead of failing
> the CHECK.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 56c2879c41a84f959107e28410a46aeb11457975 
> 
> Diff: https://reviews.apache.org/r/54731/diff/
> 
> 
> Testing
> ---
> 
> I will add a test in the next review.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54731: Fixed a check bug in LAUNCH_NESTED_CONTAINER_SESSION_CALL.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 14, 2016, 4:15 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54731/
> ---
> 
> (Updated Dec. 14, 2016, 4:15 a.m.)
> 
> 
> Review request for mesos, Adam B and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `attachContainerOutput` could return a non OK response (e.g., authz
> error). We should return the response in this case instead of failing
> the CHECK.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 56c2879c41a84f959107e28410a46aeb11457975 
> 
> Diff: https://reviews.apache.org/r/54731/diff/
> 
> 
> Testing
> ---
> 
> I will add a test in the next review.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 54731: Fixed a check bug in LAUNCH_NESTED_CONTAINER_SESSION_CALL.

[Vinod Kone]

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

Review request for mesos, Adam B and Kevin Klues.


Repository: mesos


Description
---

`attachContainerOutput` could return a non OK response (e.g., authz
error). We should return the response in this case instead of failing
the CHECK.


Diffs
-

  src/slave/http.cpp 56c2879c41a84f959107e28410a46aeb11457975 

Diff: https://reviews.apache.org/r/54731/diff/


Testing
---

I will add a test in the next review.


Thanks,

Vinod Kone

Re: Review Request 54650: Added validation for roles in ACCEPT call.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54649, 54650]

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 Dec. 13, 2016, 4:41 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> ---
> 
> (Updated Dec. 13, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
> 
> Diff: https://reviews.apache.org/r/54650/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[Jiang Yan Xu]

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


Ship it!




Now that the implementation is moved to cpp, we typically prefer using 
declaration + unqualified types in the code. Ideally it should be done in this 
patch but I see that you are already doing it (partiall) in /r/53791/ so it's 
fine to fix it there too.

- Jiang Yan Xu


On Dec. 13, 2016, 10:36 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53790/
> ---
> 
> (Updated Dec. 13, 2016, 10:36 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move containerizer Rootfs support to a cpp file.
> 
> No functional changes. This is just moving the existing code.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/tests/CMakeLists.txt 0966b7f283ea2ce646a417e81b6dfe1134a7188c 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[Jiang Yan Xu]

> On Dec. 13, 2016, 2:24 a.m., Benjamin Bannier wrote:
> >

Yeah these are often the source of confusion. We should clarify them in the 
style guide. (Just sent an email: 
https://lists.apache.org/thread.html/b99a26d43410eaa3cf45c634e6f45abee012b82b14dd2d2685ecb8e8@%3Cdev.mesos.apache.org%3E)


> On Dec. 13, 2016, 2:24 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 29
> > 
> >
> > Please include this one before any other headers.

For now let's stick to the common pratice of treating it the same way as the 
other project files. I can take care of this.


- Jiang Yan


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


On Dec. 13, 2016, 10:36 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53790/
> ---
> 
> (Updated Dec. 13, 2016, 10:36 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move containerizer Rootfs support to a cpp file.
> 
> No functional changes. This is just moving the existing code.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/tests/CMakeLists.txt 0966b7f283ea2ce646a417e81b6dfe1134a7188c 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Kevin Klues]

> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > 
> >
> > hmm, is this enough verification to ensure that a container can be 
> > attached after an agent restart? IIUC, this just validates that we are able 
> > to establish a connection. What if a subsequent request fails with a non 
> > `OK()` response?
> 
> Vinod Kone wrote:
> Yes, this test verifes that `IOSwitchboardTest.Attach` test works even 
> after agent restart. Just like that test this one doesn't send actual 
> requests, just tests connection.
> 
> Of course, it is still valuable to write tests that send speific 
> `ATTACH_*` calls and verify them but those to me are different tests. I will 
> get to them after :)
> 
> Anand Mazumdar wrote:
> gotcha, Dropping the issue.

Yeah, it's an `Attach` test, not an `AttachInput` test.


- Kevin


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


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Anand Mazumdar]

> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > 
> >
> > hmm, is this enough verification to ensure that a container can be 
> > attached after an agent restart? IIUC, this just validates that we are able 
> > to establish a connection. What if a subsequent request fails with a non 
> > `OK()` response?
> 
> Vinod Kone wrote:
> Yes, this test verifes that `IOSwitchboardTest.Attach` test works even 
> after agent restart. Just like that test this one doesn't send actual 
> requests, just tests connection.
> 
> Of course, it is still valuable to write tests that send speific 
> `ATTACH_*` calls and verify them but those to me are different tests. I will 
> get to them after :)

gotcha, Dropping the issue.


- Anand


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


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Dec. 14, 2016, 12:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54727/
> ---
> 
> (Updated Dec. 14, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out some code into helpers that can be re-used in other tests.
> I plan to use them in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54727/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Anand Mazumdar]

> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 91-115
> > 
> >
> > hmm, I imagine us having a client for the Operator API in the future. 
> > If it has the same interface as the scheduler/executor library, it would be 
> > more suited if we rename this function to ease the transition in the future:
> > 
> > ```cpp
> > Future send(
> > const Call& call,
> > http::Connection connection);
> > ```
> > 
> > Also, we would have helpers e.g., `createAttachOutputCall()` etc. 
> > similar to what we have for the Scheduler API (`createAcceptCall()`)
> 
> Vinod Kone wrote:
> note that there are implicit assumptions about accept/content-type, 
> request type and streaming responses in this helper, so not sure a 
> `send(call, connection)` would be enough.
> 
> i could add more params to this function to make it generic, but that 
> seems like an overkill at this point. i'm basically trying to reduce the 
> boiler plate code i need to write in tests, not necessarily a library 
> function. i can add a TODO to make it generic once we have a few tests using 
> this helper.

I see. Adding a `TODO` for now sgtm.


- Anand


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


On Dec. 14, 2016, 12:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54727/
> ---
> 
> (Updated Dec. 14, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out some code into helpers that can be re-used in other tests.
> I plan to use them in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54727/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Vinod Kone]

> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 896-897
> > 
> >
> > hmm, is this enough verification to ensure that a container can be 
> > attached after an agent restart? IIUC, this just validates that we are able 
> > to establish a connection. What if a subsequent request fails with a non 
> > `OK()` response?

Yes, this test verifes that `IOSwitchboardTest.Attach` test works even after 
agent restart. Just like that test this one doesn't send actual requests, just 
tests connection.

Of course, it is still valuable to write tests that send speific `ATTACH_*` 
calls and verify them but those to me are different tests. I will get to them 
after :)


- Vinod


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


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Vinod Kone]

> On Dec. 14, 2016, 1:34 a.m., Anand Mazumdar wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, lines 91-115
> > 
> >
> > hmm, I imagine us having a client for the Operator API in the future. 
> > If it has the same interface as the scheduler/executor library, it would be 
> > more suited if we rename this function to ease the transition in the future:
> > 
> > ```cpp
> > Future send(
> > const Call& call,
> > http::Connection connection);
> > ```
> > 
> > Also, we would have helpers e.g., `createAttachOutputCall()` etc. 
> > similar to what we have for the Scheduler API (`createAcceptCall()`)

note that there are implicit assumptions about accept/content-type, request 
type and streaming responses in this helper, so not sure a `send(call, 
connection)` would be enough.

i could add more params to this function to make it generic, but that seems 
like an overkill at this point. i'm basically trying to reduce the boiler plate 
code i need to write in tests, not necessarily a library function. i can add a 
TODO to make it generic once we have a few tests using this helper.


- Vinod


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


On Dec. 14, 2016, 12:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54727/
> ---
> 
> (Updated Dec. 14, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out some code into helpers that can be re-used in other tests.
> I plan to use them in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54727/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Anand Mazumdar]

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



LGTM, minus a comment around the helper function naming.


src/tests/containerizer/io_switchboard_tests.cpp (lines 91 - 115)


hmm, I imagine us having a client for the Operator API in the future. If it 
has the same interface as the scheduler/executor library, it would be more 
suited if we rename this function to ease the transition in the future:

```cpp
Future send(
const Call& call,
http::Connection connection);
```

Also, we would have helpers e.g., `createAttachOutputCall()` etc. similar 
to what we have for the Scheduler API (`createAcceptCall()`)


- Anand Mazumdar


On Dec. 14, 2016, 12:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54727/
> ---
> 
> (Updated Dec. 14, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out some code into helpers that can be re-used in other tests.
> I plan to use them in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54727/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Anand Mazumdar]

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



Modulo Kevin's comment around adding the agent subsystem flag.

Similar to the earlier review, looks good minus a query around if establishing 
a connection is enough to ensure that the client can attach?


src/tests/containerizer/io_switchboard_tests.cpp (lines 814 - 817)


Would look less jagged if all arguments are on the same line similar to the 
`MesosSchedulerDriver()` a few lines later. I can understand why you did it in 
this way since the other tests in this file do it in this way.



src/tests/containerizer/io_switchboard_tests.cpp (line 891)


s/EXPECT_EQ/ASSERT_EQ

Might be needed for the other tests in this file too. Otherwise, it might 
crash on the next line.



src/tests/containerizer/io_switchboard_tests.cpp (lines 896 - 897)


hmm, is this enough verification to ensure that a container can be attached 
after an agent restart? IIUC, this just validates that we are able to establish 
a connection. What if a subsequent request fails with a non `OK()` response?


- Anand Mazumdar


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Kevin Klues]

> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 809
> > 
> >
> > I think you need the new:
> > ```
> > #ifdef __linux__
> >   flags.agent_subsystems = None();
> > #endif
> > ```
> > 
> > here.
> 
> Vinod Kone wrote:
> Oh, I just copy pasted from another test in the file. Is this being 
> introduced in a different review?

It was probably introduced between when you first copied the test over and when 
you pushed the review. All of the switchboard realted tests need this if they 
use the posix launcher.


- Kevin


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


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54662: Enabled authorization in SET_LOG_LEVEL API call.

[Adam B]

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


Fix it, then Ship it!





src/slave/http.cpp (line 904)


No need to `defer(slave->self)` here since the lambda doesn't use anything 
from the agent (level, duration, approver are passed in directly), and the 
result is a `dispatch()`? Let me double-check with somebody here so I can 
commit this today.



src/slave/http.cpp (lines 905 - 906)


Looks like weird wrapping to me. I'd prefer to see it wrapped after `](` 
like other lambdas


- Adam B


On Dec. 12, 2016, 6:37 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54662/
> ---
> 
> (Updated Dec. 12, 2016, 6:37 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows only authorized users to change the log
> level of Mesos using the HTTP API v1.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Dec. 13, 2016, 6:47 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54538/
> ---
> 
> (Updated Dec. 13, 2016, 6:47 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
>   src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 
> 
> Diff: https://reviews.apache.org/r/54538/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Vinod Kone]

> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 809
> > 
> >
> > I think you need the new:
> > ```
> > #ifdef __linux__
> >   flags.agent_subsystems = None();
> > #endif
> > ```
> > 
> > here.

Oh, I just copy pasted from another test in the file. Is this being introduced 
in a different review?


> On Dec. 14, 2016, 1:10 a.m., Kevin Klues wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 886
> > 
> >
> > Wait until slvae recovered? Your future is on `slave::Slave::_recover`

`_recover` execution only guarantees that containerizer is recovered. there are 
more steps for slave recovery to complete, which i didn't care for in this test.


- Vinod


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


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

[Adam B]

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


Fix it, then Ship it!





src/slave/http.cpp (line 775)


Looks like excessive `()` wrapping, but I can fix this before committing.


- Adam B


On Dec. 13, 2016, 6:49 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54661/
> ---
> 
> (Updated Dec. 13, 2016, 6:49 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows for restriction of users when attempting
> to access the `GET_FLAGS` API v1 call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54661/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54720: Added a test that verifies container attach after agent restart.

[Kevin Klues]

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


Fix it, then Ship it!





src/tests/containerizer/io_switchboard_tests.cpp (line 809)


I think you need the new:
```
#ifdef __linux__
  flags.agent_subsystems = None();
#endif
```

here.



src/tests/containerizer/io_switchboard_tests.cpp (line 886)


Wait until slvae recovered? Your future is on `slave::Slave::_recover`


- Kevin Klues


On Dec. 13, 2016, 10:20 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54720/
> ---
> 
> (Updated Dec. 13, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test that verifies container attach after agent restart.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54720/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Dec. 14, 2016, 12:46 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54727/
> ---
> 
> (Updated Dec. 14, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pulled out some code into helpers that can be re-used in other tests.
> I plan to use them in subsequent patches.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 
> 
> Diff: https://reviews.apache.org/r/54727/diff/
> 
> 
> Testing
> ---
> 
> make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 54727: Refactored IOSwitchboardServerTest.AttachOutput test.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Kevin Klues.


Repository: mesos


Description
---

Pulled out some code into helpers that can be re-used in other tests.
I plan to use them in subsequent patches.


Diffs
-

  src/tests/containerizer/io_switchboard_tests.cpp 
5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 

Diff: https://reviews.apache.org/r/54727/diff/


Testing
---

make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1


Thanks,

Vinod Kone

Review Request 54719: Added support for deleting CNI network from `network/cni` isolator.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

During `prepare` or `isolate` if the `network/cni` isolator sees an
error while reading the CNI network configuration it goes ahead and
deletes the CNI network from the cache. This allows operators to
dynamically delete CNI network configuration without the need for an
agent restart.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54719/diff/


Testing
---

make check

Manual testing with removing CNI configuration while the agent was up, and 
verifying that the CNI network got deleted without the need for an agent 
restart.


Thanks,

Avinash sridharan

Re: Review Request 54718: Removed dependency of `detach` and `recover` on cached networks.

[Avinash sridharan]

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

(Updated Dec. 14, 2016, 12:29 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

rebased.


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


Repository: mesos


Description (updated)
---

Since CNI networks can be added and removed on the fly, and for a
given container the CNI configuration with which the container was
launched is check-pointed in the containers `network/cni` work
directory, it doesn't make sense to require the network name that the
container is associated with to exist in the in-memory cache. Hence,
removing these dependencies.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54718/diff/


Testing
---

make check


Thanks,

Avinash sridharan

Re: Review Request 54717: Modified `network/cni` isolator for dynamic addition of CNI networks.

[Avinash sridharan]

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

(Updated Dec. 14, 2016, 12:28 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed some stylistic error.s


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


Repository: mesos


Description
---

If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54717/diff/


Testing
---

make check

Manual testing with addition of a non-existent CNI configuration.


Thanks,

Avinash sridharan

Re: Review Request 52765: Populated `recovered_slaves` in `/state` and `/slaves` endpoints.

[Zhitao Li]

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




src/master/http.cpp (line 160)


One thing I just noticed here is that the `resources` may not reflect the 
actual resources state of the slave, due to checkpointed dynamic reservation 
etc.

I'm proposing to drop this field from responses in `recovered_slaves`, to 
avoid confusion.

What do you think?


- Zhitao Li


On Nov. 2, 2016, 4:42 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52765/
> ---
> 
> (Updated Nov. 2, 2016, 4:42 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Xiaojian Huang, and Kunal Thakar.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Populated `recovered_slaves` in `/state` and `/slaves` endpoints.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 2f275f6c78b92e13bd7d38043b581b5a3555ee40 
>   src/tests/master_tests.cpp ce42a106e8c671c9eaddb85d45153f31e0b9a488 
> 
> Diff: https://reviews.apache.org/r/52765/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 54596: Replaced `::dup` with `os::dup` in libprocess.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

Replaced `::dup` with `os::dup` in libprocess.


Diffs
-


Diff: https://reviews.apache.org/r/54596/diff/


Testing
---


Thanks,

Michael Park

Review Request 54718: Removed dependency of `detach` and `recover` on cached networks.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Since CNI networks can be added and removed on the fly, and for a
given container the CNI configuration with which the container was
launched is check-pointed in the containers `network/cni` work
directory it doesn't make sense to require the network name that the
container is associated with to exist in the in-memory cache. Hence,
removing these dependencies.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54718/diff/


Testing
---

make check


Thanks,

Avinash sridharan

Review Request 54717: Modified `network/cni` isolator for dynamic addition of CNI networks.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54717/diff/


Testing
---

make check

Manual testing with addition of a non-existent CNI configuration.


Thanks,

Avinash sridharan

Review Request 54716: Modified the initialization logic for `network/cni` isolator.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI  plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

Diff: https://reviews.apache.org/r/54716/diff/


Testing
---

sudo make check --gtest_filter=*CNI*


Thanks,

Avinash sridharan

Review Request 54720: Added a test that verifies container attach after agent restart.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Kevin Klues.


Repository: mesos


Description
---

Added a test that verifies container attach after agent restart.


Diffs
-

  src/tests/containerizer/io_switchboard_tests.cpp 
5bd9dc4a776fe8a0d04ed31aa167f10c394984e6 

Diff: https://reviews.apache.org/r/54720/diff/


Testing
---

make -j20 check GTEST_FILTER="*Switchboard*" MESOS_VERBOSE=1


Thanks,

Vinod Kone

Review Request 54595: Introduced an `os::dup` abstraction in stout.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

Introduced an `os::dup` abstraction in stout.


Diffs
-

  3rdparty/stout/include/Makefile.am fc020bbf65030eb69a42d5c402e38ecb145f4c0f 
  3rdparty/stout/include/stout/os.hpp cdcbbf488d110e33e796fb0b7414793391761abf 
  3rdparty/stout/include/stout/os/dup.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/posix/dup.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/dup.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/54595/diff/


Testing
---


Thanks,

Michael Park

Review Request 54592: Introduced an `os::lseek` abstraction in stout.

[Michael Park]

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

Review request for mesos.


Repository: mesos


Description
---

Introduced an `os::lseek` abstraction in stout.


Diffs
-

  3rdparty/stout/include/Makefile.am fc020bbf65030eb69a42d5c402e38ecb145f4c0f 
  3rdparty/stout/include/stout/os.hpp cdcbbf488d110e33e796fb0b7414793391761abf 
  3rdparty/stout/include/stout/os/lseek.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/54592/diff/


Testing
---


Thanks,

Michael Park

Review Request 54594: Replaced `::lseek` with `os::lseek` in mesos.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

Replaced `::lseek` with `os::lseek` in mesos.


Diffs
-

  src/files/files.cpp 9c634ac928887b3f1a111f67ebb3fc5229c6fa16 
  src/slave/state.cpp a94bb8d7029295abef70d6595ebc732ac1ab87a8 

Diff: https://reviews.apache.org/r/54594/diff/


Testing
---


Thanks,

Michael Park

Review Request 54593: Replaced `::lseek` with `os::lseek` in stout.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

Replaced `::lseek` with `os::lseek` in stout.


Diffs
-

  3rdparty/stout/include/stout/protobuf.hpp 
80cb20f40a7ddd4309d27973eef9fca9e4052b64 

Diff: https://reviews.apache.org/r/54593/diff/


Testing
---


Thanks,

Michael Park

Review Request 54591: Introduced `WindowsFD` class which is analogous to an `int` in POSIX.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

In POSIX the socket, pipe and a file are represented by the `int` type.
In Windows:
  - A socket is kept in a `SOCKET` type (64 bit wide)
  - A pipe or a WinAPI file descriptor in a `HANDLE` (64 bit wide)
  - A CRT file descriptor in an `int`

The `WindowsFD` class is a type that brings all of these things
together and behaves analogously to an `int` in POSIX.


Diffs
-

  3rdparty/stout/include/stout/os.hpp cdcbbf488d110e33e796fb0b7414793391761abf 
  3rdparty/stout/include/stout/os/int_fd.hpp PRE-CREATION 
  3rdparty/stout/include/stout/os/windows/fd.hpp PRE-CREATION 

Diff: https://reviews.apache.org/r/54591/diff/


Testing
---


Thanks,

Michael Park

Review Request 54590: Removed unused peek function.

[Michael Park]

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

Review request for mesos and Daniel Pravat.


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/src/io.cpp e81f279ed4bf92f75ad2427550ca822a9b03cca5 
  3rdparty/libprocess/src/tests/io_tests.cpp 
b9825e8633f64c23e4b1ea904537cdc8da64ed5b 

Diff: https://reviews.apache.org/r/54590/diff/


Testing
---


Thanks,

Michael Park

Re: Review Request 54422: Removed test assumptions about arrival order of status updates.

[Anand Mazumdar]

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


Ship it!




Thanks for the cleanup!

- Anand Mazumdar


On Dec. 6, 2016, 2:09 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54422/
> ---
> 
> (Updated Dec. 6, 2016, 2:09 p.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Task status updates can arrive in any order. Adjusted the code to not
> assume a particual order.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 3d06c3a4a74bbcba4c9b6bdba4258c5aefdab845 
> 
> Diff: https://reviews.apache.org/r/54422/diff/
> 
> 
> Testing
> ---
> 
> * `make check` on OS X, clang-trunk w/o optimizations
> * `SlaveTest.*` in repetation
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54700: Fixed bug that prevented window size updates in the IOSwitchboard.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54700]

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 Dec. 13, 2016, 12:34 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54700/
> ---
> 
> (Updated Dec. 13, 2016, 12:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed bug that prevented window size updates in the IOSwitchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> f900924dd55c42966deb14c65fca380bebc86e2f 
> 
> Diff: https://reviews.apache.org/r/54700/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual test with a TTY client to make sure the window resized properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 54700: Fixed bug that prevented window size updates in the IOSwitchboard.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On Dec. 13, 2016, 4:34 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54700/
> ---
> 
> (Updated Dec. 13, 2016, 4:34 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed bug that prevented window size updates in the IOSwitchboard.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> f900924dd55c42966deb14c65fca380bebc86e2f 
> 
> Diff: https://reviews.apache.org/r/54700/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual test with a TTY client to make sure the window resized properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Benjamin Bannier]

> On Dec. 13, 2016, 11:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, lines 82-83
> > 
> >
> > Let's move this up right after the check `needed.contains(path)`.
> > 
> > Right now in pathological cases like e.g., a`libA` depending on `libB`, 
> > and `libB` depending on `libA`, we would recurse indefinitely.
> 
> James Peach wrote:
> Adding the path to the `needed` set means that we need the path and have 
> already calculated the dependencies for that path. If we add a path before 
> having all its dependencies, we would return early in the 
> `needed.contains(path)` check.

An entry in `needed` is the only indication on whether we have already visited 
a node in the dependency graph (which is not necessarily a tree).

If you do not add `path` to `needed` before recursing into 
`collectDependencies`, `path` might be visited again as a dependency in 
pathological cases involving cyclic shared library dependencies (which the 
loader might be fine with). In such a case this code would recurse indefinitely.

It looks like a fix might be to add `path` to `needed` after you have found 
that it currently is not in `needed`, but before you recurse.


- Benjamin


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


On Dec. 13, 2016, 7:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Dec. 13, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54685: Windows: Fix build break in libprocess tests.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On Dec. 12, 2016, 4:33 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54685/
> ---
> 
> (Updated Dec. 12, 2016, 4:33 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, Greg Mann, 
> John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit f61f321b calls a macro, `INSTANTIATE_TEST_CASE_P`, with some
> arguments `#ifdef`'d. This breaks MSVC 1900 build; the specification
> does not require the text a macro expands to be processed, which means
> that the un-processed `#ifdef` ends up in the expansion of
> `INSTANTIATE_TEST_CASE_P`, which causes a build-time error.
> 
> This commit will resolve this problem by moving the `#ifdef` to surround
> the invocation of `INSTANTIATE_TEST_CASE_P` instead of being around the
> arguments to the macro.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 822cdb3931d3e30713aa78995427f364e41bfb30 
> 
> Diff: https://reviews.apache.org/r/54685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 54685: Windows: Fix build break in libprocess tests.

[Andrew Schwartzmeyer]

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


Ship it!




Ship It!

- Andrew Schwartzmeyer


On Dec. 13, 2016, 12:33 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54685/
> ---
> 
> (Updated Dec. 13, 2016, 12:33 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, Greg Mann, 
> John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit f61f321b calls a macro, `INSTANTIATE_TEST_CASE_P`, with some
> arguments `#ifdef`'d. This breaks MSVC 1900 build; the specification
> does not require the text a macro expands to be processed, which means
> that the un-processed `#ifdef` ends up in the expansion of
> `INSTANTIATE_TEST_CASE_P`, which causes a build-time error.
> 
> This commit will resolve this problem by moving the `#ifdef` to surround
> the invocation of `INSTANTIATE_TEST_CASE_P` instead of being around the
> arguments to the macro.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 822cdb3931d3e30713aa78995427f364e41bfb30 
> 
> Diff: https://reviews.apache.org/r/54685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[James Peach]

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

(Updated Dec. 13, 2016, 6:36 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description (updated)
---

Move containerizer Rootfs support to a cpp file.

No functional changes. This is just moving the existing code.


Diffs (updated)
-

  src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
  src/tests/CMakeLists.txt 0966b7f283ea2ce646a417e81b6dfe1134a7188c 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53790/diff/


Testing (updated)
---

sudo make check (Fedora 24).


Thanks,

James Peach

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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




src/linux/ldd.cpp (line 37)


IMHO this code is fairly obvious as is. We already do unnecessary parsing 
and copying in the rootfs caller, I don't think we should also make things 
worse here too.



src/tests/containerizer/rootfs.cpp (line 136)


I think that a vector is obvious and a set doesn't make any improvement.



src/tests/containerizer/rootfs.cpp (line 145)


AFAICT 2 is correct.



src/tests/containerizer/rootfs.cpp (line 175)


Not AFAICT :)


- James Peach


On Dec. 13, 2016, 6:34 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Dec. 13, 2016, 6:34 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 54712: Use the stout ELF parser to implement ldd.

[James Peach]

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

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


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


Repository: mesos


Description
---

Use the stout ELF parser to implement an ldd() API that recursively
gathers the link dependencies of a given program.


Diffs
-

  src/CMakeLists.txt 0cc0451ae3ca0183da3d575cc4bd3c5b3ea30ecc 
  src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
  src/linux/ldd.hpp PRE-CREATION 
  src/linux/ldd.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/54712/diff/


Testing
---

sudo make check (Fedora 25)


Thanks,

James Peach

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

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

(Updated Dec. 13, 2016, 6:34 p.m.)


Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.


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


Repository: mesos


Description
---

The dependencies for the programs we need in the Linux root
filesystem vary over time and across distributions. Since Stout
has support for parsing the library dependencies of ELF binaries,
use this to collect the necessary dependencies when constructing
a root filesystem for the tests.


Diffs (updated)
-

  src/Makefile.am a4c03c2b918816e6dd8872d37e5208f055619c47 
  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/containerizer/rootfs.cpp PRE-CREATION 

Diff: https://reviews.apache.org/r/53791/diff/


Testing
---

sudo make check (Fedora 24)


Thanks,

James Peach

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[James Peach]

> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, line 32
> > 
> >
> > Mesos does not like anon namespaces. Since this namespace e.g., 
> > declares no types, you could just replace its usage with making 
> > `collectDependencies` `static` without loss.

There's nothing in the style guide, and the code contains a mix of traditional 
`static` and unnamed namespaces. I changed it anyway.


> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, lines 82-83
> > 
> >
> > Let's move this up right after the check `needed.contains(path)`.
> > 
> > Right now in pathological cases like e.g., a`libA` depending on `libB`, 
> > and `libB` depending on `libA`, we would recurse indefinitely.

Adding the path to the `needed` set means that we need the path and have 
already calculated the dependencies for that path. If we add a path before 
having all its dependencies, we would return early in the 
`needed.contains(path)` check.


> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/linux/ldd.cpp, line 37
> > 
> >
> > Let's not use an out parameter, but instead return a `Try > 
> > Note that this requires making a copy of an in parameter `needed` and 
> > to explicitly calculate the union in below `foreach` loop. In the end the 
> > code  might require more copies, but have clearer side effects.

I don't think we should do this.


> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 137
> > 
> >
> > nit: Not yours, but should be 4 spaces.

Apparently 2 is correct.


> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 157
> > 
> >
> > nit: Not yours, but should be 4 spaces.

Apparantly 2 is correct.


> On Dec. 13, 2016, 10:13 a.m., Benjamin Bannier wrote:
> > src/tests/containerizer/rootfs.cpp, line 191
> > 
> >
> > nit: Not yours, but should be 4 spaces.

Apparantly 2 is correct.


- James


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


On Nov. 29, 2016, 1:21 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 29, 2016, 1:21 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/linux/ldd.hpp PRE-CREATION 
>   src/linux/ldd.cpp PRE-CREATION 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53791/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 54663: Acked correct task id in DefaultExecutorTest.KillTask test.

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On Dec. 13, 2016, 11:50 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54663/
> ---
> 
> (Updated Dec. 13, 2016, 11:50 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-6745
> https://issues.apache.org/jira/browse/MESOS-6745
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When acknowledging task status updates, this test was erroneously
> using a cached task id instead of the actual task id send in the
> status update. This can lead to failed acknowledgements when
> status updates appear in different order.
> 
> This patch just uses the task id sent as part of the status update in
> the acknowledgement.
> 
> 
> Diffs
> -
> 
>   src/tests/default_executor_tests.cpp 
> a88796b83c17fb01e7698907e9b0899a63700782 
> 
> Diff: https://reviews.apache.org/r/54663/diff/
> 
> 
> Testing
> ---
> 
> Test did not fail in my setup in 4000 iterations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54685: Windows: Fix build break in libprocess tests.

[Greg Mann]

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


Ship it!




Ship It!

- Greg Mann


On Dec. 13, 2016, 12:33 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54685/
> ---
> 
> (Updated Dec. 13, 2016, 12:33 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, Greg Mann, 
> John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit f61f321b calls a macro, `INSTANTIATE_TEST_CASE_P`, with some
> arguments `#ifdef`'d. This breaks MSVC 1900 build; the specification
> does not require the text a macro expands to be processed, which means
> that the un-processed `#ifdef` ends up in the expansion of
> `INSTANTIATE_TEST_CASE_P`, which causes a build-time error.
> 
> This commit will resolve this problem by moving the `#ifdef` to surround
> the invocation of `INSTANTIATE_TEST_CASE_P` instead of being around the
> arguments to the macro.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 822cdb3931d3e30713aa78995427f364e41bfb30 
> 
> Diff: https://reviews.apache.org/r/54685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53299: Fixed memory leak in implementation of Future::after().

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53299]

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 Dec. 13, 2016, 12:10 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53299/
> ---
> 
> (Updated Dec. 13, 2016, 12:10 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-6484
> https://issues.apache.org/jira/browse/MESOS-6484
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removes a reference counted pointer that futures kept to themselves
> when using the method `Future::afeter()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 26bf5851f6562cd73aa4938b3308639144657044 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 7c411c7be1849119fe0b070622dbe4488fa11b7a 
> 
> Diff: https://reviews.apache.org/r/53299/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 51028: Fix tests with rapidly triggered allocations.

[Jiang Yan Xu]

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



I think this patch can also follow /r/51027/ in the chain.


src/tests/hierarchical_allocator_tests.cpp (lines 2796 - 2799)


I think the following is sufficient. 

```
Settle to make sure the dispatched allocation is executed.
```



src/tests/hierarchical_allocator_tests.cpp (lines 3003 - 3004)


Similar to Guangya's suggestion, I think we can elaborate on the reason to 
use tow agents and two frameworks.

```
Trigger at least two allocation runs to generate the window statistics. 
Here we are using two pairs of `addSlave()` and `addFramework()` calls because 
each guarantees one allocation run that yields an `Allocation`.
```



src/tests/hierarchical_allocator_tests.cpp (line 3619)


I think we are still intereted in the number of allocations and we expect 
to see them decrease because of batching. We don't use EXPECT on them but we 
print them out as part of the benchmark?


- Jiang Yan Xu


On Sept. 28, 2016, 2:16 p.m., Jacob Janco wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51028/
> ---
> 
> (Updated Sept. 28, 2016, 2:16 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-3157
> https://issues.apache.org/jira/browse/MESOS-3157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Per MESOS-3157, if cluster events with the possibility
>   of triggering an allocation occur rapidly and test
>   assertions depend on gleaning information from assumed
>   order, it is likely they will fail due to lack of parity
>   between event and actual allocation. Ensure that expected
>   allocations occur.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 46553f6d501deb37862dd5ba48be1c114f6e6cb8 
> 
> Diff: https://reviews.apache.org/r/51028/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jacob Janco
> 
>

Re: Review Request 54693: Add ProtoBuf schema for Blkio cgroup subsystem

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [54693, 53546]

Failed command: ./support/apply-review.sh -n -r 53546

Error:
2016-12-13 15:17:50 URL:https://reviews.apache.org/r/53546/diff/raw/ 
[30079/30079] -> "53546.patch" [1]
error: patch failed: src/CMakeLists.txt:160
error: src/CMakeLists.txt: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/16438/console

- Mesos ReviewBot


On Dec. 13, 2016, 5:12 a.m., Jason Lai wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54693/
> ---
> 
> (Updated Dec. 13, 2016, 5:12 a.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, haosdent huang, Jie 
> Yu, Kunal Thakar, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add ProtoBuf schema for Blkio cgroups per the kernel Blkio controller doc.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 0017d3d9d46433b391703025f611ce437bbc7ebe 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/54693/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jason Lai
> 
>

Re: Review Request 54650: Added validation for roles in ACCEPT call.

[Guangya Liu]

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




src/master/validation.cpp (line 1411)


Shall we add some check here to make sure the `allocation_info` exist as 
this field is `optional`?


- Guangya Liu


On 十二月 13, 2016, 4:41 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54650/
> ---
> 
> (Updated 十二月 13, 2016, 4:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and Guangya Liu.
> 
> 
> Bugs: MESOS-6637
> https://issues.apache.org/jira/browse/MESOS-6637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For a multi-role framework, it may receive offers for different roles
> in that framework. However, we disallow multiple offers with different
> roles being accepted in single ACCEPT call.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 96aa36585ded4bd7cf98526f710ccbc4f23b1f0f 
> 
> Diff: https://reviews.apache.org/r/54650/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 54662: Enabled authorization in SET_LOG_LEVEL API call.

[Alexander Rojas]

> On Dec. 13, 2016, 10:44 a.m., Adam B wrote:
> > src/slave/http.cpp, line 898
> > 
> >
> > Is the plan to move everything over to the ObjectApprover API? Why not 
> > use the simpler  `slave->authorizer.get()->authorized(authRequest)` API for 
> > simpler authz checks? Because it's asynchronous?

That has, indeed, been the plan since the introduction of the object approver 
concept. Though my original plan was to do a sweep clean of all the places 
where the `Authorizer::authorized()` calls exist.


- Alexander


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


On Dec. 12, 2016, 3:37 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54662/
> ---
> 
> (Updated Dec. 12, 2016, 3:37 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows only authorized users to change the log
> level of Mesos using the HTTP API v1.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

[Alexander Rojas]

> On Dec. 13, 2016, 10:44 a.m., Adam B wrote:
> > src/slave/http.cpp, line 766
> > 
> >
> > Is the plan to move everything over to the ObjectApprover API?
> > I notice that the v0 /flags endpoint just calls 
> > `slave->authorizer.get()->authorized(authRequest)`
> > Should we update it too?

That has, indeed, been the plan since the introduction of the object approver 
concept. Though my original plan was to do a sweep clean of all the places 
where the `Authorizer::authorized()` calls exist.


- Alexander


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


On Dec. 13, 2016, 3:49 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54661/
> ---
> 
> (Updated Dec. 13, 2016, 3:49 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows for restriction of users when attempting
> to access the `GET_FLAGS` API v1 call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54661/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 3:49 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Adds the stub which allows for restriction of users when attempting
to access the `GET_FLAGS` API v1 call.


Diffs (updated)
-

  src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 

Diff: https://reviews.apache.org/r/54661/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 3:47 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.


Diffs
-

  src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
  src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 

Diff: https://reviews.apache.org/r/54538/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 3:44 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description (updated)
---

Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.


Diffs
-

  src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
  src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 

Diff: https://reviews.apache.org/r/54538/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54589: Improved operator HTTP API docs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54589]

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 Dec. 13, 2016, 3:02 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54589/
> ---
> 
> (Updated Dec. 13, 2016, 3:02 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix syntax errors, avoid use of "smart quotes" in protocol examples, and
> similar tweaks. Also mention Operator HTTP API in the old-style HTTP
> endpoints doc page.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/index.md e9f8b529d5a9e6cc4aef848675a6805a0ec721bc 
>   docs/operator-http-api.md 3ad86c009b54a1656af221102870b329da722bcc 
>   support/generate-endpoint-help.py 90431a729deaa1cb0a1006fa4a85787402c95c34 
> 
> Diff: https://reviews.apache.org/r/54589/diff/
> 
> 
> Testing
> ---
> 
> Reviewed output visually.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 3:04 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description (updated)
---

Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.


Diffs (updated)
-

  src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
  src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 

Diff: https://reviews.apache.org/r/54538/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54702: Fixed the tests by turning off agent subsystems in IOSwitchboard tests.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On Dec. 13, 2016, 12:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54702/
> ---
> 
> (Updated Dec. 13, 2016, 12:53 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Kevin Klues, and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes the changes introduced in 
> `354bca1311c5b3b98a2a50689f0fba6a033e743d ` to enable building on OS X.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> 5b7a668285df2e922bf8a2cb9e759a9066b4a4e1 
> 
> Diff: https://reviews.apache.org/r/54702/diff/
> 
> 
> Testing
> ---
> 
> `make check`, Mac OS 10.12
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 2:44 p.m.)


Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Adds the authorization action `VIEW_CONTAINERS` which takes an object
of type `FrameworkInfo` and `ExecutorInfo` and optionally a
`CommandInfo`.

It also adds the authorization action `SET_LOG_LEVEL` which takes no
object.

Includes testing for the ACLs and interface.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto 3499cac43d77c371448a9c9e1ac95d42b24a54dd 
  include/mesos/authorizer/authorizer.proto 
b7371cd580bbe64eb1566876d0f253eedbd0789d 
  src/authorizer/local/authorizer.cpp 3b983d0c0dea3ad761e7c684a9f943809dc541e9 
  src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8 

Diff: https://reviews.apache.org/r/54535/diff/


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 54702: Fixed the tests by turning off agent subsystems in IOSwitchboard tests.

[Benjamin Bannier]

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

(Updated Dec. 13, 2016, 1:53 p.m.)


Review request for mesos, Jie Yu, Kevin Klues, and Till Toenshoff.


Repository: mesos


Description
---

This fixes the changes introduced in `354bca1311c5b3b98a2a50689f0fba6a033e743d 
` to enable building on OS X.


Diffs
-

  src/tests/containerizer/io_switchboard_tests.cpp 
5b7a668285df2e922bf8a2cb9e759a9066b4a4e1 

Diff: https://reviews.apache.org/r/54702/diff/


Testing
---

`make check`, Mac OS 10.12


Thanks,

Benjamin Bannier

Review Request 54702: Fixed the tests by turning off agent subsystems in IOSwitchboard tests.

[Benjamin Bannier]

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

Review request for mesos, Jie Yu, Kevin Klues, and Till Toenshoff.


Repository: mesos


Description
---

This fixes the changes introduced in `354bca1311c5b3b98a2a50689f0fba6a033e743d 
` to enable building on OS X.


Diffs
-

  src/tests/containerizer/io_switchboard_tests.cpp 
5b7a668285df2e922bf8a2cb9e759a9066b4a4e1 

Diff: https://reviews.apache.org/r/54702/diff/


Testing
---

`make check`, Mac OS 10.12


Thanks,

Benjamin Bannier

Review Request 54700: Fixed bug that prevented window size updates in the IOSwitchboard.

[Kevin Klues]

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

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


Repository: mesos


Description
---

Fixed bug that prevented window size updates in the IOSwitchboard.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
f900924dd55c42966deb14c65fca380bebc86e2f 

Diff: https://reviews.apache.org/r/54700/diff/


Testing
---

make check

Manual test with a TTY client to make sure the window resized properly.


Thanks,

Kevin Klues

Re: Review Request 54677: Windows: Enabled 495 passing tests on Agent builds.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [54677, 54631, 54618, 54611, 54493, 53706, 52624, 52625, 
52972, 52544, 52364]

Failed command: ./support/apply-review.sh -n -r 52544

Error:
2016-12-13 12:11:02 URL:https://reviews.apache.org/r/52544/diff/raw/ 
[15399/15399] -> "52544.patch" [1]
error: patch failed: 3rdparty/stout/include/stout/os.hpp:47
error: 3rdparty/stout/include/stout/os.hpp: patch does not apply

Full log: https://builds.apache.org/job/mesos-reviewbot/16436/console

- Mesos ReviewBot


On Dec. 13, 2016, 12:35 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54677/
> ---
> 
> (Updated Dec. 13, 2016, 12:35 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, and Joseph Wu.
> 
> 
> Bugs: MESOS-6698, MESOS-6699, MESOS-6703, MESOS-6705, MESOS-6706, MESOS-6707, 
> MESOS-6709, and MESOS-6715
> https://issues.apache.org/jira/browse/MESOS-6698
> https://issues.apache.org/jira/browse/MESOS-6699
> https://issues.apache.org/jira/browse/MESOS-6703
> https://issues.apache.org/jira/browse/MESOS-6705
> https://issues.apache.org/jira/browse/MESOS-6706
> https://issues.apache.org/jira/browse/MESOS-6707
> https://issues.apache.org/jira/browse/MESOS-6709
> https://issues.apache.org/jira/browse/MESOS-6715
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will introduce a sizeable chunk of the trivially-passing
> Agent tests to the Windows build. Unlike #54618, which added test files
> to the build only if they needed no modifications to compile and build
> on Windows, here we add test files to the build only if they contain one
> or more tests that either don't compile or don't pass. Hence, this
> commit will partially address MESOS-6698, MESOS-6699, MESOS-6703,
> MESOS-6705, MESOS-6706, MESOS-6707, MESOS-6709, and MESOS-6715.
> 
> In many of the files we added to the build, some of the tests either
> don't compile, or don't pass. For tests that trivially compiled but
> didn't pass, we use the `TEST_X_TEMP_DISABLED_ON_WINDOWS` macros. For
> the tests that didn't compile, we simply `#ifdef` them out for now.
> 
> 
> Diffs
> -
> 
>   src/tests/CMakeLists.txt 0966b7f283ea2ce646a417e81b6dfe1134a7188c 
>   src/tests/anonymous_tests.cpp bc29ff8be94af82dd97f43db4f9594036705e835 
>   src/tests/api_tests.cpp 7a220116aa871650e56221a4e81ccb65f4252a56 
>   src/tests/command_executor_tests.cpp 
> f847f6a9f71270dd4b28b487b5f12bcfeb0d5fa6 
>   src/tests/common/command_utils_tests.cpp 
> afb1532aa114b0ee4752189a09c21ece24d7d27d 
>   src/tests/credentials_tests.cpp 1ceb31b72446a87771705cab2255c264d8e00c08 
>   src/tests/default_executor_tests.cpp 
> a88796b83c17fb01e7698907e9b0899a63700782 
>   src/tests/fault_tolerance_tests.cpp 
> 71fe0df7c3997bea25431f7385e23d53456dd3cf 
>   src/tests/fetcher_tests.cpp 8b8baf87127f430e91165b7cebc93ede697931bb 
>   src/tests/files_tests.cpp f6a6aeaa4efa05957d1038c564157216fce70246 
>   src/tests/gc_tests.cpp d9776b602bee780b9732cd93d06b9c8f3cc8a4d0 
>   src/tests/health_check_tests.cpp 3b0125ee788f7193eef00f54ab6d63da472224b0 
>   src/tests/hierarchical_allocator_tests.cpp 
> cde32c9e7c7ac98d69f51c9f6ff89b4fae5cf595 
>   src/tests/hook_tests.cpp 7956eaea4c8f8b0db33d0472764d00bdb4e6a80e 
>   src/tests/master_allocator_tests.cpp 
> 34e2b4950ad722a1b1470953053213b3db265eb4 
>   src/tests/partition_tests.cpp 00cc815529dc4d303db638680eacb8f55713d1a1 
>   src/tests/paths_tests.cpp 4def898c7eb0a3f5c5ffd858e9d3c9558cc49cce 
>   src/tests/rate_limiting_tests.cpp 61674155bebea493ec60b277b72fdcf8f366e26b 
>   src/tests/resource_offers_tests.cpp 
> b6c15cb298ee49e18419b8ab7d17d6e95a2e1b8c 
>   src/tests/role_tests.cpp 2ee7801bf9cbf68dce6305ac8872e00ed2476247 
>   src/tests/scheduler_tests.cpp 95cad9ae8d9a8bd7f5e9ebd9f49cbc4e20ad596b 
>   src/tests/slave_tests.cpp d2430480ee32d9c39a415631d17b9a26941406e8 
>   src/tests/status_update_manager_tests.cpp 
> 38d8913a5b33aa5325d0bc632c0a1d80480eddf8 
>   src/tests/uri_fetcher_tests.cpp 3c1bd33137612de90d65b7af288bb81ac9876c8b 
> 
> Diff: https://reviews.apache.org/r/54677/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53299: Fixed memory leak in implementation of Future::after().

[Alexander Rojas]

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

(Updated Dec. 13, 2016, 1:10 p.m.)


Review request for mesos, Benjamin Bannier, Jie Yu, and Joris Van Remoortere.


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


Repository: mesos


Description (updated)
---

Removes a reference counted pointer that futures kept to themselves
when using the method `Future::afeter()`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/future.hpp 
26bf5851f6562cd73aa4938b3308639144657044 
  3rdparty/libprocess/src/tests/future_tests.cpp 
7c411c7be1849119fe0b070622dbe4488fa11b7a 

Diff: https://reviews.apache.org/r/53299/diff/


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 53299: Fixed memory leak in implementation of Future::after().

[Alexander Rojas]

> On Dec. 12, 2016, 9:31 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/src/tests/future_tests.cpp, line 266
> > 
> >
> > What does the word 'policy' here denote?
> > Could you pick something more consistent with the other tests?

Sorry, the name came from the original piece of code where I discovered the 
leak. Through the many iterations I reduce the code to the most simple way to 
reproduce the leak but I overlooked naming.


> On Dec. 12, 2016, 9:31 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/future.hpp, line 1321
> > 
> >
> > Can you explain why we're adding this behavior with a comment? It's not 
> > immediately clear to me from the existing code / comment what the 
> > implication is of making this association if the weak future is `none`.

I added a summary comment, but the cause is a little bit more complex than a 
one line code. Consider the following code:

```c++
Future future;
{
  future = someFunction().after(Milliseconds(10), [](const Future&) { 
return Nothing; });
}
```

In that example, the future returned by `someFunction()` goes out of scope 
almost as soon as it appears, but the future returned by the `promised` is 
still kept.


- Alexander


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


On Oct. 31, 2016, 3:48 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53299/
> ---
> 
> (Updated Oct. 31, 2016, 3:48 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-6484
> https://issues.apache.org/jira/browse/MESOS-6484
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removes a reference counted pointer that futures kept to themselves
> when using the method `Future::after()`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 175214a9090f8cc8241a81e535c87370102f3011 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 7c411c7be1849119fe0b070622dbe4488fa11b7a 
> 
> Diff: https://reviews.apache.org/r/53299/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54663: Acked correct task id in DefaultExecutorTest.KillTask test.

[Benjamin Bannier]

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

(Updated Dec. 13, 2016, 12:50 p.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Sprinkle some vertical space goodness.


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


Repository: mesos


Description (updated)
---

When acknowledging task status updates, this test was erroneously
using a cached task id instead of the actual task id send in the
status update. This can lead to failed acknowledgements when
status updates appear in different order.

This patch just uses the task id sent as part of the status update in
the acknowledgement.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp a88796b83c17fb01e7698907e9b0899a63700782 

Diff: https://reviews.apache.org/r/54663/diff/


Testing
---

Test did not fail in my setup in 4000 iterations.


Thanks,

Benjamin Bannier

Re: Review Request 54664: Acked correct task id in DefaultExecutorTest.KillTaskGroupOnTaskFailure.

[Benjamin Bannier]

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

(Updated Dec. 13, 2016, 12:50 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Sprinkle some vertical space goodness.


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


Repository: mesos


Description (updated)
---

When acknowledging task status updates, this test was erroneously
using a cached task id instead of the actual task id send in the
status update. This can lead to failed acknowledgements when
status updates appear in different order.

This patch just uses the task id sent as part of the status update in
the acknowledgement.


Diffs (updated)
-

  src/tests/default_executor_tests.cpp a88796b83c17fb01e7698907e9b0899a63700782 

Diff: https://reviews.apache.org/r/54664/diff/


Testing
---

Test did not fail in my setup in 4000 iterations.


Thanks,

Benjamin Bannier

Re: Review Request 54682: Fixed the tests by turning off agent subsystems in IOSwitchboard tests.

[Benjamin Bannier]

> On Dec. 13, 2016, 12:28 p.m., Benjamin Bannier wrote:
> > src/tests/containerizer/io_switchboard_tests.cpp, line 471
> > 
> >
> > Note that this flag only exists on Linux, so this change suddenly 
> > causes this test to fail on e.g., OS X. `#ifdef`'ing these statements leads 
> > to passing tests.
> > 
> > Here and below.

Sorry, meant to write _faile to build_.


- Benjamin


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


On Dec. 13, 2016, 12:42 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54682/
> ---
> 
> (Updated Dec. 13, 2016, 12:42 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent subsystems should be not be used if the corresponding cgroups
> isolator is not used. Ideally, we should add a check in Mesos to
> disallow such a combination.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> a1ac75549697ba13cbb7c38fcab4a724aefbb402 
> 
> Diff: https://reviews.apache.org/r/54682/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54682: Fixed the tests by turning off agent subsystems in IOSwitchboard tests.

[Benjamin Bannier]

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




src/tests/containerizer/io_switchboard_tests.cpp (line 471)


Note that this flag only exists on Linux, so this change suddenly causes 
this test to fail on e.g., OS X. `#ifdef`'ing these statements leads to passing 
tests.

Here and below.


- Benjamin Bannier


On Dec. 13, 2016, 12:42 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54682/
> ---
> 
> (Updated Dec. 13, 2016, 12:42 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent subsystems should be not be used if the corresponding cgroups
> isolator is not used. Ideally, we should add a check in Mesos to
> disallow such a combination.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/io_switchboard_tests.cpp 
> a1ac75549697ba13cbb7c38fcab4a724aefbb402 
> 
> Diff: https://reviews.apache.org/r/54682/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 54685: Windows: Fix build break in libprocess tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54685]

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 Dec. 13, 2016, 12:33 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54685/
> ---
> 
> (Updated Dec. 13, 2016, 12:33 a.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Daniel Pravat, Greg Mann, 
> John Kordich, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Commit f61f321b calls a macro, `INSTANTIATE_TEST_CASE_P`, with some
> arguments `#ifdef`'d. This breaks MSVC 1900 build; the specification
> does not require the text a macro expands to be processed, which means
> that the un-processed `#ifdef` ends up in the expansion of
> `INSTANTIATE_TEST_CASE_P`, which causes a build-time error.
> 
> This commit will resolve this problem by moving the `#ifdef` to surround
> the invocation of `INSTANTIATE_TEST_CASE_P` instead of being around the
> arguments to the macro.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 822cdb3931d3e30713aa78995427f364e41bfb30 
> 
> Diff: https://reviews.apache.org/r/54685/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 53790: Move containerizer Rootfs support to a cpp file.

[Benjamin Bannier]

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


Fix it, then Ship it!





src/tests/containerizer/rootfs.cpp (lines 17 - 27)


This double-includes `string`, `process/owned.hpp` and `stout/try.hpp` 
which are already included in the header corresponding to this file. Not sure 
about the current dogmatics, but my hunch would be to not do that.



src/tests/containerizer/rootfs.cpp (line 29)


Please include this one before any other headers.


- Benjamin Bannier


On Nov. 29, 2016, 2:21 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53790/
> ---
> 
> (Updated Nov. 29, 2016, 2:21 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Move containerizer Rootfs support to a cpp file.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 85eda538caf39f81f052896e744b7b0c724f81bb 
>   src/tests/CMakeLists.txt 23230123f96f35f2aca3676b0eaa835098ea4f79 
>   src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
>   src/tests/containerizer/rootfs.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/53790/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (Fedora 24)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 53791: Use the stout ELF parser to collect Linux rootfs files.

[Benjamin Bannier]

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



Thanks for looking into this James.

Looks mostly good for me; I was also able to execute tests using the created 
rootfs under e.g., fedora-25 which wasn't possible before.

I left some mostly minor comments on style and edge cases.


src/linux/ldd.hpp (lines 31 - 32)


Please add includes for `string` and `vector`.



src/linux/ldd.cpp (line 32)


Mesos does not like anon namespaces. Since this namespace e.g., declares no 
types, you could just replace its usage with making `collectDependencies` 
`static` without loss.



src/linux/ldd.cpp (line 37)


Let's not use an out parameter, but instead return a `Try

`load.isError()`



src/linux/ldd.cpp (line 53)


nit: 4 spaces.



src/linux/ldd.cpp (line 54)


`dependencies.isError()`



src/linux/ldd.cpp (line 58)


nit: space after `foreach`.



src/linux/ldd.cpp (lines 82 - 83)


Let's move this up right after the check `needed.contains(path)`.

Right now in pathological cases like e.g., a`libA` depending on `libB`, and 
`libB` depending on `libA`, we would recurse indefinitely.



src/linux/ldd.cpp (line 90)


We often don't try hard enough, but since this is new code, could we use 
`Path` for paths everywhere here to make semantics clearer?



src/tests/containerizer/rootfs.cpp (line 56)


We also need to check for `realpath.isNone()`.



src/tests/containerizer/rootfs.cpp (lines 64 - 70)


This will fail to create a working link if `file` is not a direct link to 
`realpath`, e.g., if we have multiple levels of links.

This error already existed in the original implementation, could you add a 
`TODO`, or even better create a ticket?



src/tests/containerizer/rootfs.cpp (line 79)


+1!



src/tests/containerizer/rootfs.cpp (line 92)


nit: Let's break this line after `Error('.



src/tests/containerizer/rootfs.cpp (line 136)


Not originally yours, but we could just as well use a set (e.g., `set` or 
`hashset`) here to express that duplicates are not meaningful.

Here and below.



src/tests/containerizer/rootfs.cpp (line 137)


nit: Not yours, but should be 4 spaces.



src/tests/containerizer/rootfs.cpp (line 144)


Could we move this right above the loop using it?



src/tests/containerizer/rootfs.cpp (line 145)


nit: Not yours, but should be 4 spaces.



src/tests/containerizer/rootfs.cpp (line 153)


Let's break this line after `Error(`.



src/tests/containerizer/rootfs.cpp (line 175)


nit: Not yours, but should be 4 spaces.


- Benjamin Bannier


On Nov. 29, 2016, 2:21 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/53791/
> ---
> 
> (Updated Nov. 29, 2016, 2:21 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6588
> https://issues.apache.org/jira/browse/MESOS-6588
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The dependencies for the programs we need in the Linux root
> filesystem vary over time and across distributions. Since Stout
> has support for parsing the library dependencies of ELF binaries,
> use this to collect the necessary dependencies when constructing
> a root filesystem for the tests.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0884c6bb6f2ebddadc5cfc6b72d704086709748d 
>   src/Makefile.am 

Re: Review Request 54538: Enabled fine grained authorization for the getContainers API Call.

[Adam B]

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



Looks good, but I'm tired. I'll merge all these in tomorrow.


src/slave/http.cpp (line 1869)


Each parameter on a separate line, please



src/slave/http.cpp (line 1929)


Already called `info` a few lines above. Can you reuse that?


- Adam B


On Dec. 12, 2016, 2:46 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54538/
> ---
> 
> (Updated Dec. 12, 2016, 2:46 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables fine grained authorization for the v1 API call `GET_CONTAINERS`.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
>   src/slave/slave.hpp 059eb770051f54d5d1fc116bd491f460ee177b0a 
> 
> Diff: https://reviews.apache.org/r/54538/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54655: Renamed LocalNestedContainerObjectApprover for a more generic name.

[Adam B]

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




src/authorizer/local/authorizer.cpp (lines 558 - 559)


How did we get from required/optional to framework/executor/command_info? 
The connection isn't obvious without reading through the code and the callers' 
code.



src/authorizer/local/authorizer.cpp (lines 582 - 583)


What's optional here? Should there be a `object.has_command_info()` in 
there somewhere?


- Adam B


On Dec. 12, 2016, 1:51 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54655/
> ---
> 
> (Updated Dec. 12, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Renames `LocalNestedContainerObjectApprover` to
> `CombinedACLObjectApprover` in order to allow uses beyond nested
> containers.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> 3b983d0c0dea3ad761e7c684a9f943809dc541e9 
> 
> Diff: https://reviews.apache.org/r/54655/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54661: Enable authorization for the GET_FLAGS API Call.

[Adam B]

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



Looks like we don't ever return Forbidden in this new code path..


src/slave/http.cpp (line 766)


Is the plan to move everything over to the ObjectApprover API?
I notice that the v0 /flags endpoint just calls 
`slave->authorizer.get()->authorized(authRequest)`
Should we update it too?



src/slave/http.cpp (line 775)


Don't you need to check `approver->approved()` before you return OK or 
Forbidden?


- Adam B


On Dec. 12, 2016, 6:35 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54661/
> ---
> 
> (Updated Dec. 12, 2016, 6:35 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows for restriction of users when attempting
> to access the `GET_FLAGS` API v1 call.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54661/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54662: Enabled authorization in SET_LOG_LEVEL API call.

[Adam B]

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



Looks good. Just a question about ObjectApprover vs. authorized()


src/slave/http.cpp (line 898)


Is the plan to move everything over to the ObjectApprover API? Why not use 
the simpler  `slave->authorizer.get()->authorized(authRequest)` API for simpler 
authz checks? Because it's asynchronous?


- Adam B


On Dec. 12, 2016, 6:37 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54662/
> ---
> 
> (Updated Dec. 12, 2016, 6:37 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the stub which allows only authorized users to change the log
> level of Mesos using the HTTP API v1.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 8a71eadd8f26df147ddea800221b6f243280bf3b 
> 
> Diff: https://reviews.apache.org/r/54662/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54535: Added authorization actions VIEW_CONTAINERS and SET_LOG_LEVEL.

[Adam B]

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


Fix it, then Ship it!




Ship it! Just a couple of nits on the comments.


include/mesos/authorizer/acls.proto (line 350)


More accurately, you're authorizing based on the linux user of the executor 
process, not necessarily the linux user of a nested container process. As in 
"... of a container whose executor is running as..."

In theory we could allow nested containers to be run as users different 
from their parent container processes, but I don't know if we actually support 
that use case yet.



include/mesos/authorizer/acls.proto (line 360)


s/authorizedto  change/authorized to change/



src/tests/authorization_tests.cpp (line 1986)


s/a containers/containers/ here and elsewhere


- Adam B


On Dec. 12, 2016, 2:45 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54535/
> ---
> 
> (Updated Dec. 12, 2016, 2:45 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-6670
> https://issues.apache.org/jira/browse/MESOS-6670
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the authorization action `VIEW_CONTAINERS` which takes an object
> of type `FrameworkInfo` and `ExecutorInfo` and optionally a
> `CommandInfo`.
> 
> It also adds the authorization action `SET_LOG_LEVEL` which takes no
> object.
> 
> Includes testing for the ACLs and interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> 3499cac43d77c371448a9c9e1ac95d42b24a54dd 
>   include/mesos/authorizer/authorizer.proto 
> b7371cd580bbe64eb1566876d0f253eedbd0789d 
>   src/authorizer/local/authorizer.cpp 
> 3b983d0c0dea3ad761e7c684a9f943809dc541e9 
>   src/tests/authorization_tests.cpp f70d60d73a64e7eb4be20a17b62a8726b659e1b8 
> 
> Diff: https://reviews.apache.org/r/54535/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 54449: Check quotas are enabled in the XFS disk isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54449]

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 Dec. 13, 2016, 12:15 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54449/
> ---
> 
> (Updated Dec. 13, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6732
> https://issues.apache.org/jira/browse/MESOS-6732
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The XFS disk isolator checks that the filesystem is XFS, but doesn't
> check whether project quotas are actually enabled. This means that
> an invalid configuration will start but will always fail when tasks
> are launched.
> 
> Add a check to test whether project quotas are enabled on the work
> directory and fail hard if they are not.
> 
> 
> Diffs
> -
> 
>   configure.ac 7a18c89854c1f42bec09f067579b72114fb8ec1d 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp 
> dd4df86bf90bfa9cbf4664d89274cf3c64c2e374 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp 
> 7602fe3b6ab069db643397418732e773d0417f8a 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp 
> b9d8e7dc999ba3064bee7105eff0f9553d825df8 
> 
> Diff: https://reviews.apache.org/r/54449/diff/
> 
> 
> Testing
> ---
> 
> Make check on Fedora 25. Manual test on F25 with mesos-execute.
> 
> 
> Thanks,
> 
> James Peach
> 
>