Re: Review Request 58280: Added a test to verify persistent volume mount points removal.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58278, 58279, 58280]

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 April 8, 2017, 12:27 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58280/
> ---
> 
> (Updated April 8, 2017, 12:27 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is used to catch regression related to MESOS-7366.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/58280/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> Without the fix, this test failed:
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from LinuxFilesystemIsolatorTest
> [ RUN  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup
> /home/jie/workspace/mesos/src/tests/containerizer/linux_filesystem_isolator_tests.cpp:879:
>  Failure
> (wait).failure(): Failed to clean up an isolator when destroying container: 
> Failed to unmount volume 
> '/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
>  Failed to unmount 
> '/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
>  Device or resource busy
> [  FAILED  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup (113 ms)
> [--] 1 test from LinuxFilesystemIsolatorTest (121 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (165 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 58279: Lazily unmount persistent volumes in DockerContainerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On April 7, 2017, 4:46 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58279/
> ---
> 
> (Updated April 7, 2017, 4:46 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use MNT_DETACH to unmount persistent volumes in DockerContainerizer to
> workaround an issue of incorrect handling of container destroy
> failures. Currently, if unmount fails there, the containerizer will
> still treat the container as terminated, and the agent will schedule
> the cleanup of the container's sandbox. Since the mount hasn't been
> removed in the sandbox (e.g., due to EBUSY), that'll result in data in
> the persistent volume being incorrectly deleted. Use MNT_DETACH so
> that the mount point in the sandbox will be removed immediately. See
> MESOS-7366 for more details.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp be1a298b12374bced44e2467cb7e90a1599abb8f 
> 
> 
> Diff: https://reviews.apache.org/r/58279/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 58278: Lazily unmount persistent volumes in MesosContainerizer.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On April 7, 2017, 4:45 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58278/
> ---
> 
> (Updated April 7, 2017, 4:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use MNT_DETACH when unmounting persistent volumes in Linux filesystem
> isolator to workaround an issue of incorrect handling of container
> destroy failures. Currently, if isolator cleanup returns a failure,
> the slave will treat the container as terminated, and will schedule
> the cleanup of the container's sandbox. Since the mount hasn't been
> removed in the sandbox (e.g., due to EBUSY), that'll result in data in
> the persistent volume being incorrectly deleted. Use MNT_DETACH so
> that the mount point in the sandbox will be removed immediately.  See
> MESOS-7366 for more details.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> ae0031d8d8d6dfe0334b605fbb85e83de88ab436 
> 
> 
> Diff: https://reviews.apache.org/r/58278/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 58280: Added a test to verify persistent volume mount points removal.

[Gilbert Song]

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


Ship it!




Ship It!

- Gilbert Song


On April 7, 2017, 5:27 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58280/
> ---
> 
> (Updated April 7, 2017, 5:27 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is used to catch regression related to MESOS-7366.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/58280/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> Without the fix, this test failed:
> ```
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from LinuxFilesystemIsolatorTest
> [ RUN  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup
> /home/jie/workspace/mesos/src/tests/containerizer/linux_filesystem_isolator_tests.cpp:879:
>  Failure
> (wait).failure(): Failed to clean up an isolator when destroying container: 
> Failed to unmount volume 
> '/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
>  Failed to unmount 
> '/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
>  Device or resource busy
> [  FAILED  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup (113 ms)
> [--] 1 test from LinuxFilesystemIsolatorTest (121 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (165 ms total)
> [  PASSED  ] 0 tests.
> [  FAILED  ] 1 test, listed below:
> [  FAILED  ] 
> LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup
> ```
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 57166: Updated role validation for hierarchical roles.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 7, 2017, 7:38 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57166/
> ---
> 
> (Updated March 7, 2017, 7:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Role names can now contain forward slashes. Each component in a role
> name must now be validated separately.
> 
> 
> Diffs
> -
> 
>   src/common/roles.cpp 31774a9b8f99f5efeed35b1c3e3486e05ca00f6a 
>   src/tests/role_tests.cpp 77f3d46a544a51ba71476e2f0735bb32758dd9e1 
> 
> 
> Diff: https://reviews.apache.org/r/57166/diff/3/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 58280: Added a test to verify persistent volume mount points removal.

[Jie Yu]

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

(Updated April 8, 2017, 12:27 a.m.)


Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.


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


Repository: mesos


Description
---

This test is used to catch regression related to MESOS-7366.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5e489ef6a522000c55b0fb9a27bce2567f82bb73 


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


Testing (updated)
---

sudo make check

Without the fix, this test failed:
```
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from LinuxFilesystemIsolatorTest
[ RUN  ] LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup
/home/jie/workspace/mesos/src/tests/containerizer/linux_filesystem_isolator_tests.cpp:879:
 Failure
(wait).failure(): Failed to clean up an isolator when destroying container: 
Failed to unmount volume 
'/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
 Failed to unmount 
'/tmp/LinuxFilesystemIsolatorTest_ROOT_PersistentVolumeMountPointCleanup_Tl5a9W/sandbox/volume':
 Device or resource busy
[  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_PersistentVolumeMountPointCleanup 
(113 ms)
[--] 1 test from LinuxFilesystemIsolatorTest (121 ms total)

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


Thanks,

Jie Yu

Re: Review Request 58280: Added a test to verify persistent volume mount points removal.

[Jie Yu]

> On April 8, 2017, 12:17 a.m., Zhitao Li wrote:
> > Can you confirm that this test fails without the fix in r/58278?

Updated the "test" section.


- Jie


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


On April 7, 2017, 11:47 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58280/
> ---
> 
> (Updated April 7, 2017, 11:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is used to catch regression related to MESOS-7366.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/58280/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 58280: Added a test to verify persistent volume mount points removal.

[Zhitao Li]

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



Can you confirm that this test fails without the fix in r/58278?


src/tests/containerizer/linux_filesystem_isolator_tests.cpp
Lines 842 (patched)


Move this comment before the code block above?


- Zhitao Li


On April 7, 2017, 11:47 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58280/
> ---
> 
> (Updated April 7, 2017, 11:47 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.
> 
> 
> Bugs: MESOS-7366
> https://issues.apache.org/jira/browse/MESOS-7366
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test is used to catch regression related to MESOS-7366.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> 5e489ef6a522000c55b0fb9a27bce2567f82bb73 
> 
> 
> Diff: https://reviews.apache.org/r/58280/diff/1/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 58280: Added a test to verify persistent volume mount points removal.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.


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


Repository: mesos


Description
---

This test is used to catch regression related to MESOS-7366.


Diffs
-

  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
5e489ef6a522000c55b0fb9a27bce2567f82bb73 


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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 58279: Lazily unmount persistent volumes in DockerContainerizer.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.


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


Repository: mesos


Description
---

Use MNT_DETACH to unmount persistent volumes in DockerContainerizer to
workaround an issue of incorrect handling of container destroy
failures. Currently, if unmount fails there, the containerizer will
still treat the container as terminated, and the agent will schedule
the cleanup of the container's sandbox. Since the mount hasn't been
removed in the sandbox (e.g., due to EBUSY), that'll result in data in
the persistent volume being incorrectly deleted. Use MNT_DETACH so
that the mount point in the sandbox will be removed immediately. See
MESOS-7366 for more details.


Diffs
-

  src/slave/containerizer/docker.cpp be1a298b12374bced44e2467cb7e90a1599abb8f 


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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 58278: Lazily unmount persistent volumes in MesosContainerizer.

[Jie Yu]

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

Review request for mesos, Gilbert Song, Jason Lai, and Zhitao Li.


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


Repository: mesos


Description
---

Use MNT_DETACH when unmounting persistent volumes in Linux filesystem
isolator to workaround an issue of incorrect handling of container
destroy failures. Currently, if isolator cleanup returns a failure,
the slave will treat the container as terminated, and will schedule
the cleanup of the container's sandbox. Since the mount hasn't been
removed in the sandbox (e.g., due to EBUSY), that'll result in data in
the persistent volume being incorrectly deleted. Use MNT_DETACH so
that the mount point in the sandbox will be removed immediately.  See
MESOS-7366 for more details.


Diffs
-

  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
ae0031d8d8d6dfe0334b605fbb85e83de88ab436 


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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 58253: Added a ContainerID to 'ObjectApprover::Object'.

[Greg Mann]

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



For some reason I'm having trouble replying to your previous comment, so I'll 
post a new one :)

I think that it makes sense to have claims in the `authorization::Subject`, 
since this maps directly onto the `Principal` provided by the client. However, 
in the case of the `authorization::Object`, I don't think that the agent should 
dictate the use of particular claims there. For example, a custom authorizer 
might have a different way to determine which `ContainerID`s a principal should 
be able to launch containers within. I don't think that we should leak the 
specific claim keys used by the `SecretGenerator` into the 
`authorization::Object`, since in the future we will make the `SecretGenerator` 
modular and the claims within the executor token could be different for a 
custom generator. Does that make sense?

- Greg Mann


On April 7, 2017, 3:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> ---
> 
> (Updated April 7, 2017, 3:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 75801ccc753a60ce5e5979b6723fd2294ce7ffe5 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58253/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58254: Added implicit executor authorization to the agent operator API.

[Greg Mann]

> On April 7, 2017, 11:40 a.m., Alexander Rojas wrote:
> > src/authorizer/local/authorizer.cpp
> > Lines 654-657 (original), 683-690 (patched)
> > 
> >
> > I'm not so sure returning a `RejectingObjectApprover()` is the right 
> > thing to do. It looks to me that the request is in an invalid state and at 
> > the very least should log that, but this if sounds like a precondition to 
> > me: If it has claims, it needs to be one of the three actions. Probably 
> > chang it for a `CHECK`.
> > 
> > If I'm wrong, at least a comment mentioning when is it a valid case 
> > having a request that fails the check would help.

There's nothing stopping a client from providing a token with claims but no 
value in an operator API request with some other action, so we need to fail 
authorization for those cases somewhere. However, it's probably better to do it 
in the body of `getObjectApprover`, rather than here. Will update and add a 
comment.


> On April 7, 2017, 11:40 a.m., Alexander Rojas wrote:
> > src/authorizer/local/authorizer.cpp
> > Lines 692-699 (patched)
> > 
> >
> > Refer to my review of previous patch.

I'm not sure precisely what you mean to say here, could you elaborate?


- Greg


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


On April 7, 2017, 11:25 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58254/
> ---
> 
> (Updated April 7, 2017, 11:25 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the agent handlers for the LAUNCH_, WAIT_,
> and KILL_NESTED_CONTAINER calls of the operator API to set the
> `container_id` field within the authorization object,
> facilitating implicit executor authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
>   src/slave/http.cpp b07ce7c73a90ef297d980806ebba9530d86f25ae 
> 
> 
> Diff: https://reviews.apache.org/r/58254/diff/2/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58254: Added implicit executor authorization to the agent operator API.

[Greg Mann]

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

(Updated April 7, 2017, 11:25 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
Kone.


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


Repository: mesos


Description
---

This patch updates the agent handlers for the LAUNCH_, WAIT_,
and KILL_NESTED_CONTAINER calls of the operator API to set the
`container_id` field within the authorization object,
facilitating implicit executor authorization.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
736f76d552956f2351ffd40fc51d088dff83f8c8 
  src/authorizer/local/authorizer.cpp e241edf4afa48d35d94d72e8e8690f5bedfc 
  src/slave/http.cpp b07ce7c73a90ef297d980806ebba9530d86f25ae 


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

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


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 58252: Allowed the local authorizer to accept subjects with no value.

[Greg Mann]

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

(Updated April 7, 2017, 11:15 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
Kone.


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


Repository: mesos


Description
---

This patch updates checks in the local authorizer to allow subjects
which specify `claims` instead of a `value`.


Diffs (updated)
-

  src/authorizer/local/authorizer.cpp e241edf4afa48d35d94d72e8e8690f5bedfc 


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

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


Testing
---

Testing details can be found at the end of this chain.


Thanks,

Greg Mann

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Greg Mann]

> On April 7, 2017, 7:47 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/authenticator.hpp
> > Line 69 (original), 69 (patched)
> > 
> >
> > My only concern with using either a `std::map` or a `hashmap` is that 
> > it forces clients to have only one entry of each claim which forces us to 
> > find creative ways around this. How about using `std::multi_map`, 
> > `std::multi_hashmap` or `hashmap`?

I think our decision here should be determined by what we think is the most 
appropriate API for authenticators/authorizers. It could make sense to allow 
multiple occurrences of the same key; for example, to allow a single 
`Principal` to contain claims associating it with multiple usernames. If we 
decide to go that way, we could use our `multihashmap` class.

I would suggest that we merge this change to unblock the current patch chain, 
and consider independently the option of moving to a `multihashmap`. I think it 
may be a good change to make, and best to do it before 1.3 ships if we're going 
to.


- Greg


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


On April 7, 2017, 3:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 3:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 57473: Added support for authorization of Hierachical roles.

[Neil Conway]

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




src/authorizer/local/authorizer.cpp
Lines 401 (patched)


"a specialized"



src/authorizer/local/authorizer.cpp
Lines 573 (patched)


Seems a bit inconsistent: we use `*(object->value)` elsewhere, but we omit 
the extra set of parentheses here.



src/authorizer/local/authorizer.cpp
Line 895 (original)


What do you think about splitting the `break` changes into a separate 
review?


- Neil Conway


On April 4, 2017, 9:21 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57473/
> ---
> 
> (Updated April 4, 2017, 9:21 a.m.)
> 
> 
> Review request for mesos, Adam B and Benjamin Bannier.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds mechanisms to support authorization of hierarchical roles,
> that is, it allows operators to write ACLs of the form `role/%`
> which will enforce the rule for any nested role, e.g. `role/a`,
> `role/b` and such.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/57473/diff/5/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 57972: Added base stout Environment class to mesos-tests Environment class.

[Joseph Wu]

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




src/tests/environment.cpp
Lines 721-722 (original), 86-90 (patched)


Instead of moving all those test filters into the header file, why not do 
this?

```
Environment::Environment(const Flags& _flags) 
  : stout::internal::tests::Environment(
std::vector{
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared(),
std::make_shared()}), 
flags(_flags)
```


- Joseph Wu


On April 5, 2017, 12:08 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57972/
> ---
> 
> (Updated April 5, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added base stout Environment class to mesos-tests Environment class.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.hpp 06b6d54fc76b327e3e26914eb61c16619a36de42 
>   src/tests/environment.cpp e3cff1847c44bb06bbe898211bfca35cf851217a 
>   src/tests/main.cpp 5d062c3451bdfb5d5fc459ac7c071ab18e6d8043 
> 
> 
> Diff: https://reviews.apache.org/r/57972/diff/2/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 57824: Filtered stout tests with symlinks when unable to create symlinks.

[Joseph Wu]

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




3rdparty/stout/include/stout/tests/environment.hpp
Lines 205-210 (patched)


This is an incomplete function (declared but not defined).  Remove it?



3rdparty/stout/tests/main.cpp
Lines 69 (patched)


Consider putting this into the Environment constructor, so that deriving 
Environments do not need to add this to their own filters.


- Joseph Wu


On April 5, 2017, 12:08 p.m., John Kordich wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/57824/
> ---
> 
> (Updated April 5, 2017, 12:08 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer and Joseph Wu.
> 
> 
> Bugs: MESOS-6731
> https://issues.apache.org/jira/browse/MESOS-6731
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Filtered stout tests with symlinks when unable to create symlinks.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/tests/environment.hpp PRE-CREATION 
>   3rdparty/stout/tests/main.cpp f41b29c087402d5dbed1144504980746e0e9b6da 
>   3rdparty/stout/tests/os/filesystem_tests.cpp 
> e445daf9ed4e4c0d44dbb95e1cbebd0342c1acbc 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ed43b44663cbf04d7ddb449fd9f42b8de210bc6e 
>   3rdparty/stout/tests/os_tests.cpp 8b9531443f625ae0e1f00d29e9e45299f181880e 
> 
> 
> Diff: https://reviews.apache.org/r/57824/diff/3/
> 
> 
> Testing
> ---
> 
> Ran make check on Linux and the tests on Windows.
> 
> 
> Thanks,
> 
> John Kordich
> 
>

Re: Review Request 58247: Windows: Fixed test CopyFetcherPluginTest.FetchExistingFile.

[Joseph Wu]

> On April 6, 2017, 4:10 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/copy.cpp
> > Line 98 (original), 99-104 (patched)
> > 
> >
> > I think I would have named it `path` over `baseCommand`, and remove the 
> > trailing whitespace, but otherwise, great!
> 
> Joseph Wu wrote:
> Mostly minor nits from me, which I can fix before committing.
> 
> * `// ifndef __WINDOWS__` can be `// __WINDOWS__`
> * I'd actually go for `copyCommand` instead of `baseCommand` or `path` :P
> * Instead of a character pointer, we prefer `const char copyCommand[]` 
> for const c-strings.
> 
> Jeff Coffler wrote:
> I actually did the `#ifndef` intentionally, I tend to do the "primary" 
> paths first, save the `#else` for the rarer cases. I thought Linux was more 
> common, so I handled that first.
> 
> All of this was style stuff, I'm fine with that. Do you have a coding 
> standards doc I can look at, or do committers just pick this up with time?

Style doc here: http://mesos.apache.org/documentation/latest/c++-style-guide/
We basically follow the Google C++ style guide, plus a few rules of our own.


- Joseph


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


On April 6, 2017, 2:25 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58247/
> ---
> 
> (Updated April 6, 2017, 2:25 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7311
> https://issues.apache.org/jira/browse/MESOS-7311
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Fixed test CopyFetcherPluginTest.FetchExistingFile.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/copy.cpp 86605a0f3ecc22e7964b093979aecf46954af0f5 
> 
> 
> Diff: https://reviews.apache.org/r/58247/diff/1/
> 
> 
> Testing
> ---
> 
> On Windows:
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CopyFetcherPluginTest
> [ RUN  ] CopyFetcherPluginTest.FetchExistingFile
> [   OK ] CopyFetcherPluginTest.FetchExistingFile (97 ms)
> [--] 1 test from CopyFetcherPluginTest (99 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (1937 ms total)
> [  PASSED  ] 1 test.
> 
> 
> On Linux:
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CopyFetcherPluginTest
> [ RUN  ] CopyFetcherPluginTest.FetchExistingFile
> [   OK ] CopyFetcherPluginTest.FetchExistingFile (51 ms)
> [--] 1 test from CopyFetcherPluginTest (51 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (77 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58262, 58263]

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 April 7, 2017, 4:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 7, 2017, 4:19 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/2/
> 
> 
> Testing
> ---
> 
> make check on Mac OS.
> Linux pending.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58247: Windows: Fixed test CopyFetcherPluginTest.FetchExistingFile.

[Jeff Coffler]

> On April 6, 2017, 11:10 p.m., Andrew Schwartzmeyer wrote:
> > src/uri/fetchers/copy.cpp
> > Line 98 (original), 99-104 (patched)
> > 
> >
> > I think I would have named it `path` over `baseCommand`, and remove the 
> > trailing whitespace, but otherwise, great!
> 
> Joseph Wu wrote:
> Mostly minor nits from me, which I can fix before committing.
> 
> * `// ifndef __WINDOWS__` can be `// __WINDOWS__`
> * I'd actually go for `copyCommand` instead of `baseCommand` or `path` :P
> * Instead of a character pointer, we prefer `const char copyCommand[]` 
> for const c-strings.

I actually did the `#ifndef` intentionally, I tend to do the "primary" paths 
first, save the `#else` for the rarer cases. I thought Linux was more common, 
so I handled that first.

All of this was style stuff, I'm fine with that. Do you have a coding standards 
doc I can look at, or do committers just pick this up with time?


- Jeff


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


On April 6, 2017, 9:25 p.m., Jeff Coffler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58247/
> ---
> 
> (Updated April 6, 2017, 9:25 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, John Kordich, Joseph Wu, and 
> Li Li.
> 
> 
> Bugs: MESOS-7311
> https://issues.apache.org/jira/browse/MESOS-7311
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Fixed test CopyFetcherPluginTest.FetchExistingFile.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/copy.cpp 86605a0f3ecc22e7964b093979aecf46954af0f5 
> 
> 
> Diff: https://reviews.apache.org/r/58247/diff/1/
> 
> 
> Testing
> ---
> 
> On Windows:
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CopyFetcherPluginTest
> [ RUN  ] CopyFetcherPluginTest.FetchExistingFile
> [   OK ] CopyFetcherPluginTest.FetchExistingFile (97 ms)
> [--] 1 test from CopyFetcherPluginTest (99 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (1937 ms total)
> [  PASSED  ] 1 test.
> 
> 
> On Linux:
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from CopyFetcherPluginTest
> [ RUN  ] CopyFetcherPluginTest.FetchExistingFile
> [   OK ] CopyFetcherPluginTest.FetchExistingFile (51 ms)
> [--] 1 test from CopyFetcherPluginTest (51 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (77 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Jeff Coffler
> 
>

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[haosdent huang]

> On April 7, 2017, 4:31 p.m., haosdent huang wrote:
> >

This requires `mesos-docker-executor` share the same namespace with 
`mesos-agent`. So need `--pid=host`.

And `nsenter` requires privileged permissions, so need `--privileged=true`.


- haosdent


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


On April 7, 2017, 4:40 p.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58200/
> ---
> 
> (Updated April 7, 2017, 4:40 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-7210
> https://issues.apache.org/jira/browse/MESOS-7210
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Becuase MESOS HTTP checks doesn't work when mesos runs with
> --docker_mesos_image ( pid namespace mismatch ).So let docker
> executor run with container add host pid mapping(--pid=host)
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 
> 
> 
> Diff: https://reviews.apache.org/r/58200/diff/1/
> 
> 
> Testing
> ---
> 
> 1. Build the image with latest code. Let's name the image with `mesos-build` 
> here.
> 
> 2. Launch mesos master.
> 
> ```
> $ docker run \
>   -it \
>   --pid host \
>   --net host \
>   --privileged \
>   -v /var/run/docker.sock:/var/run/docker.sock \
>   -v /sys/fs/cgroup:/sys/fs/cgroup \
>   mesos-build \
>   mesos-master \
>   --hostname=127.0.0.1 \
>   --ip=127.0.0.1 \
>   --port=5050 \
>   --work_dir=/tmp/mesos
> ```
> 
> 3. Launch mesos agent.
> 
> ```
> $ docker run \
>   -it \
>   --pid host \
>   --net host \
>   --privileged \
>   -v /var/run/docker.sock:/var/run/docker.sock \
>   -v /sys/fs/cgroup:/sys/fs/cgroup \
>   mesos-build \
>   mesos-agent \
>   --hostname=127.0.0.1 \
>   --ip=127.0.0.1 \
>   --master=127.0.0.1:5050 \
>   --systemd_enable_support=false \
>   --work_dir=/tmp/mesos \
>   --containerizers=docker,mesos \
>   --docker_mesos_image=mesos-build
> ```
> 
> 4. Launch task with health check.
> 
> Define the task with health check.
> 
> ```
> $ cat /tmp/task.json
> {
>   "name": "test-health-check",
>   "task_id": {"value" : "test-health-check"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   },
>   "role": "*"
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   },
>   "role": "*"
> }
>   ],
>   "command": {
> "value": "sleep 1000"
>   },
>   "container": {
> "type": "DOCKER",
> "volumes": [],
> "docker": {
>   "image": "mesos-build",
>   "network": "HOST"
> }
>   },
>   "health_check": {
> "type": "HTTP",
> "http": {
>   "scheme": "http",
>   "port": 5050
> },
> "gracePeriodSeconds": 300,
> "intervalSeconds": 60,
> "timeoutSeconds": 20,
> "maxConsecutiveFailures": 3
>   }
> }
> ```
> 
> Lauch task
> 
> ```
> $ mesos-execute --master=127.0.0.1:5050 --task=/tmp/task.json
> ```
> 
> And verified the healthy status of task is correct.
> 
> ```
> I0407 16:29:57.258509 88767 health_checker.cpp:123] Entered the net namespace 
> of task (pid: '88727') successfully
> I0407 16:29:57.334801 88643 health_checker.cpp:395] Performed HTTP health 
> check for task 'test-health-check' in 86.311186ms
> I0407 16:29:57.334872 88643 health_checker.cpp:319] HTTP health check for 
> task 'test-health-check' passed
> ```
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[Deshi Xiao]

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

(Updated April 7, 2017, 4:40 p.m.)


Review request for mesos, Alexander Rukletsov and haosdent huang.


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


Repository: mesos


Description
---

Becuase MESOS HTTP checks doesn't work when mesos runs with
--docker_mesos_image ( pid namespace mismatch ).So let docker
executor run with container add host pid mapping(--pid=host)


Diffs
-

  src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 


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


Testing (updated)
---

1. Build the image with latest code. Let's name the image with `mesos-build` 
here.

2. Launch mesos master.

```
$ docker run \
-it \
--pid host \
--net host \
--privileged \
-v /var/run/docker.sock:/var/run/docker.sock \
-v /sys/fs/cgroup:/sys/fs/cgroup \
mesos-build \
mesos-master \
--hostname=127.0.0.1 \
--ip=127.0.0.1 \
--port=5050 \
--work_dir=/tmp/mesos
```

3. Launch mesos agent.

```
$ docker run \
-it \
--pid host \
--net host \
--privileged \
-v /var/run/docker.sock:/var/run/docker.sock \
-v /sys/fs/cgroup:/sys/fs/cgroup \
mesos-build \
mesos-agent \
--hostname=127.0.0.1 \
--ip=127.0.0.1 \
--master=127.0.0.1:5050 \
--systemd_enable_support=false \
--work_dir=/tmp/mesos \
--containerizers=docker,mesos \
--docker_mesos_image=mesos-build
```

4. Launch task with health check.

Define the task with health check.

```
$ cat /tmp/task.json
{
  "name": "test-health-check",
  "task_id": {"value" : "test-health-check"},
  "agent_id": {"value" : ""},
  "resources": [
{
  "name": "cpus",
  "type": "SCALAR",
  "scalar": {
"value": 0.1
  },
  "role": "*"
},
{
  "name": "mem",
  "type": "SCALAR",
  "scalar": {
"value": 32
  },
  "role": "*"
}
  ],
  "command": {
"value": "sleep 1000"
  },
  "container": {
"type": "DOCKER",
"volumes": [],
"docker": {
  "image": "mesos-build",
  "network": "HOST"
}
  },
  "health_check": {
"type": "HTTP",
"http": {
  "scheme": "http",
  "port": 5050
},
"gracePeriodSeconds": 300,
"intervalSeconds": 60,
"timeoutSeconds": 20,
"maxConsecutiveFailures": 3
  }
}
```

Lauch task

```
$ mesos-execute --master=127.0.0.1:5050 --task=/tmp/task.json
```

And verified the healthy status of task is correct.

```
I0407 16:29:57.258509 88767 health_checker.cpp:123] Entered the net namespace 
of task (pid: '88727') successfully
I0407 16:29:57.334801 88643 health_checker.cpp:395] Performed HTTP health check 
for task 'test-health-check' in 86.311186ms
I0407 16:29:57.334872 88643 health_checker.cpp:319] HTTP health check for task 
'test-health-check' passed
```


Thanks,

Deshi Xiao

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[haosdent huang]

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




src/slave/containerizer/docker.cpp
Lines 361-364 (patched)


I use

```
+Parameter* pidParameter = dockerInfo.add_parameters();
+pidParameter->set_key("pid");
+pidParameter->set_value("host");
+
+Parameter* privilegedParameter = dockerInfo.add_parameters();
+privilegedParameter->set_key("privileged");
+privilegedParameter->set_value("true");
+
```

and work in my side. May you help to double check again?


- haosdent huang


On April 5, 2017, 11:35 p.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58200/
> ---
> 
> (Updated April 5, 2017, 11:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-7210
> https://issues.apache.org/jira/browse/MESOS-7210
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Becuase MESOS HTTP checks doesn't work when mesos runs with
> --docker_mesos_image ( pid namespace mismatch ).So let docker
> executor run with container add host pid mapping(--pid=host)
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 
> 
> 
> Diff: https://reviews.apache.org/r/58200/diff/1/
> 
> 
> Testing
> ---
> 
> first testing : 
> https://gist.github.com/xiaods/c5a11e3ab51e89a9609edc2c477f7ea8
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58190, 58191, 58192, 58193, 58194, 58195, 58196]

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 April 7, 2017, 3:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58196/
> ---
> 
> (Updated April 7, 2017, 3:25 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp fb939d85dbec2bf7e81e0c11518ccecddc5a7a11 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp d14fbfb65d1cc44c954c8cdbbf36d7ddf6ede19e 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58196/diff/2/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various linux variants.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

> On April 7, 2017, 2:30 p.m., Jie Yu wrote:
> > What if agent crashes and restart?

Jie, I've added checkpointing. We probably need test this functionality. Do you 
think we should write a separate test or you have an idea which test we maybe 
can augment?


- Alexander


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


On April 7, 2017, 4:21 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 4:21 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
>   src/slave/containerizer/mesos/paths.hpp 
> d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
>   src/slave/containerizer/mesos/paths.cpp 
> ed4bbd2491e71ad1e4a41e0575b514377d02da9b 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/3/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 4:21 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Checkpointed container's launch envirnoment.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 
  src/slave/containerizer/mesos/paths.hpp 
d85fd34660faacd9c73de2ba7b87b3bbd4b6007b 
  src/slave/containerizer/mesos/paths.cpp 
ed4bbd2491e71ad1e4a41e0575b514377d02da9b 


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

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


Testing
---

See https://reviews.apache.org/r/58263/


Thanks,

Alexander Rukletsov

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 4:19 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Changes
---

Rebased.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


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

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


Testing
---

make check on Mac OS.
Linux pending.


Thanks,

Alexander Rukletsov

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 3:25 p.m.)


Review request for mesos, Gastón Kleiman and Vinod Kone.


Changes
---

Addressed some comments.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/checks/checker.hpp fb939d85dbec2bf7e81e0c11518ccecddc5a7a11 
  src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
  src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
  src/launcher/executor.cpp d14fbfb65d1cc44c954c8cdbbf36d7ddf6ede19e 
  src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 


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

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


Testing
---

make check on Mac OS 10.11.6 and various linux variants.


Thanks,

Alexander Rukletsov

Re: Review Request 58200: Fix mesos runs with docker(pid namespace mismatch).

[haosdent huang]

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




src/slave/containerizer/docker.cpp
Lines 361 (patched)


Got this error

```
Failed to enter the net namespace of task (pid: '78851'): Operation not 
permitted
```

Need to add capacity.


- haosdent huang


On April 5, 2017, 11:35 p.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58200/
> ---
> 
> (Updated April 5, 2017, 11:35 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Bugs: MESOS-7210
> https://issues.apache.org/jira/browse/MESOS-7210
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Becuase MESOS HTTP checks doesn't work when mesos runs with
> --docker_mesos_image ( pid namespace mismatch ).So let docker
> executor run with container add host pid mapping(--pid=host)
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp ad9ab847cb3093724ef374d036c896b4e7f18b5e 
> 
> 
> Diff: https://reviews.apache.org/r/58200/diff/1/
> 
> 
> Testing
> ---
> 
> first testing : 
> https://gist.github.com/xiaods/c5a11e3ab51e89a9609edc2c477f7ea8
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Gastón Kleiman]

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




src/checks/checker.cpp
Lines 1163-1164 (patched)


I'd add comments saying that this is the stderr/stdout of the TCP checker 
process.

We should also change the logging level to `VLOG(1)` and also do something 
similar for `HTTP` checks that use `curl`.


- Gastón Kleiman


On April 4, 2017, 10:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58196/
> ---
> 
> (Updated April 4, 2017, 10:25 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp fb939d85dbec2bf7e81e0c11518ccecddc5a7a11 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58196/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various linux variants.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Alexander Rukletsov]

> On April 7, 2017, 1:58 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Lines 1081 (patched)
> > 
> >
> > Does this work even when executor is running with its own file system?

This is a good question. My understanding is that an executor always have 
access to `launcherDir` because it's necessary for `mesos-containerizer` 
command, which we, e.g., put into `pre_exec_commands`. However, this might not 
be the case for the default executor, because launch is delegated to the agent 
in this case. @Jie, @Gilbert, any comments?


- Alexander


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


On April 4, 2017, 10:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58196/
> ---
> 
> (Updated April 4, 2017, 10:25 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp fb939d85dbec2bf7e81e0c11518ccecddc5a7a11 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58196/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various linux variants.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Jie Yu]

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



What if agent crashes and restart?

- Jie Yu


On April 7, 2017, 11:05 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58262/
> ---
> 
> (Updated April 7, 2017, 11:05 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Bugs: MESOS-6782
> https://issues.apache.org/jira/browse/MESOS-6782
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
> 
> 
> Diff: https://reviews.apache.org/r/58262/diff/2/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58263/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58196: Implemented TCP check support in command and default executors.

[Alexander Rukletsov]

> On April 7, 2017, 1:58 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Lines 1166-1169 (patched)
> > 
> >
> > It's a bit unfortunate that the `errno` returned by `connect` is 
> > subsumed by the tcp check command. But I guess there is no easy way to 
> > expose that to the scheduler.

I think that the scheduler is not really interested in `errno`. What 
information can be interesting for a scheduler, is whether
- a check command was configured properly and succeeded to run, but the check 
result was `false`,
- or there was a problem with the check command itself.

We can distinguish these cases in our API by returning `CheckStatusInfo.Tcp` 
with absent `succeeded` field. The question is, which errors shall belong to 
which category. For example, connection timeouts, or socket creation failures. 
I'm not too worried about this, because we can relatively easy adjust later, 
since the API is already in place.


- Alexander


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


On April 4, 2017, 10:25 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58196/
> ---
> 
> (Updated April 4, 2017, 10:25 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.hpp fb939d85dbec2bf7e81e0c11518ccecddc5a7a11 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58196/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS 10.11.6 and various linux variants.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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



```
I0407 11:44:08.444990 22135 linux_launcher.cpp:429] Launching nested container 
af12f877-9ba3-4bcf-a909-24c47d796cb8.b5c0c3b1-d124-4af4-a4a5-5d6868242575 and 
cloning with namespaces 
I0407 11:44:08.474304 22133 fetcher.cpp:353] Starting to fetch URIs for 
container: 
af12f877-9ba3-4bcf-a909-24c47d796cb8.b5c0c3b1-d124-4af4-a4a5-5d6868242575, 
directory: 
/tmp/NestedMesosContainerizerTest_ROOT_CGROUPS_CURL_INTERNET_LaunchNestedDebugCheckMntNamespace_Db0xy2/slaves/2bae98f0-6967-4cb6-a4c0-f48b70bb5ec8-S0/frameworks/2bae98f0-6967-4cb6-a4c0-f48b70bb5ec8-/executors/d01a9376-25b1-48d1-a5a5-5c5a287dfcc7/runs/af12f877-9ba3-4bcf-a909-24c47d796cb8/containers/b5c0c3b1-d124-4af4-a4a5-5d6868242575
Failed to chdir into current working directory 
'/tmp/NestedMesosContainerizerTest_ROOT_CGROUPS_CURL_INTERNET_LaunchNestedDebugCheckMntNamespace_Db0xy2/slaves/2bae98f0-6967-4cb6-a4c0-f48b70bb5ec8-S0/frameworks/2bae98f0-6967-4cb6-a4c0-f48b70bb5ec8-/executors/d01a9376-25b1-48d1-a5a5-5c5a287dfcc7/runs/af12f877-9ba3-4bcf-a909-24c47d796cb8':
 No such file or directory
```
Looks like the nested container is not always able to use parent's workdir. 
@Jie, @Gastón thoughts?

- Alexander Rukletsov


On April 7, 2017, 1:15 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 7, 2017, 1:15 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS.
> Linux pending.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58195: Added TCP checks in Mesos API.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58195/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> From now on executors may implement TCP checks for tasks.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
>   include/mesos/v1/mesos.proto 82d020e05b303a8248a90bc482b76b54b335146c 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/common/type_utils.cpp dc0dd71f52581e2067fed279677bda8c82aa7298 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
>   src/v1/mesos.cpp 5605ff22da77724a7947637bc17e12143ee34802 
> 
> 
> Diff: https://reviews.apache.org/r/58195/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58195: Added TCP checks in Mesos API.

[Alexander Rukletsov]

> On April 7, 2017, 1:09 a.m., Vinod Kone wrote:
> > src/checks/checker.cpp
> > Line 341 (original)
> > 
> >
> > why no space?

We seem a bit inconsistent regarding blank lines between cases in `switch` 
statements, but it looks we tend to avoid them especially for concise cases.


- Alexander


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


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58195/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> From now on executors may implement TCP checks for tasks.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
>   include/mesos/v1/mesos.proto 82d020e05b303a8248a90bc482b76b54b335146c 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/common/type_utils.cpp dc0dd71f52581e2067fed279677bda8c82aa7298 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
>   src/v1/mesos.cpp 5605ff22da77724a7947637bc17e12143ee34802 
> 
> 
> Diff: https://reviews.apache.org/r/58195/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58195: Added TCP checks in Mesos API.

[Alexander Rukletsov]

> On April 7, 2017, 1:09 a.m., Vinod Kone wrote:
> > include/mesos/mesos.proto
> > Lines 1773 (patched)
> > 
> >
> > `succeeded` seems a bit weird, can we call it `status` or 
> > `connection_status` to be consistent?
> > 
> > Also, is `boolean` enough to represent the TCP connection status? Looks 
> > like a TCP connection can be in a few different  
> > stateshttps://doc.nexthink.com/Documentation/Nexthink/V5.3/GlossaryAndReferences/StatusofTCPconnections
> >  ?

I don't think `status==true` is good either, hence I went with 
`succeeded==true`, because it _reads good_. Let's look at other bool field in 
mesos.proto:
`TaskStatus.healthy` `==true` — sounds good
`Image.Docker.cached` `==true` — sounds good
`ContainerInfo.DockerInfo.privileged` `==true` sounds good
`ContainerInfo.DockerInfo.force_pull_image` `==true` sounds good
`FrameworkInfo.checkpoint` `==true` sounds good
`CommandInfo.executable` `==true` sounds good
`CommandInfo.extract` `==true` sounds good
`CommandInfo.shell` `==true` sounds good

Regarding multiple states. I don't think we care what we get for our `SYN` 
request, unless it is `SYN-ACK`, in which case TCP check is considered 
successful. Using the document you provide:
`established`: means TCP check succeeds
`closed`: same, implies TCP handshake was successful => TCP checks succeeds
`no service`: `RST` as response, i.e. no `SYN-ACK` => handshake fails => TCP 
check fails
`no host`: no responce, i.e. no `SYN-ACK` => handshake times out => TCP check 
fails
`rejected`: no `SYN-ACK` => handshake fails => TCP check fails

If in the future we want to distinguish timeout handshakes, we probably should 
do it globally for all types of checks.


- Alexander


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


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58195/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Bugs: MESOS-7275
> https://issues.apache.org/jira/browse/MESOS-7275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> From now on executors may implement TCP checks for tasks.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto dd90465cc3da283c078d4e907cc6a4a0e50309ac 
>   include/mesos/v1/mesos.proto 82d020e05b303a8248a90bc482b76b54b335146c 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
>   src/common/type_utils.cpp dc0dd71f52581e2067fed279677bda8c82aa7298 
>   src/launcher/default_executor.cpp 9cc40c6391a931f9cdf6965e64be231980537299 
>   src/launcher/executor.cpp bc69beb884d95d1616b2a3d928cdbf00f70f7c88 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
>   src/v1/mesos.cpp 5605ff22da77724a7947637bc17e12143ee34802 
> 
> 
> Diff: https://reviews.apache.org/r/58195/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 1:15 p.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


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


Testing (updated)
---

make check on Mac OS.
Linux pending.


Thanks,

Alexander Rukletsov

Re: Review Request 58095: Refactored functions that render /roles and GetRoles endpoints.

[Alexander Rojas]

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




src/master/http.cpp
Line 3383 (original), 3385 (patched)


With the proposed changes, there's no reason to have this function in 
`Master::Http` and can be move directly to `Master` IMHO.

Probably the name can be change to `activeRoles` too, since it makes more 
sense given the description of _interesting roles_.



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


All the indentation of this lambda is off



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


Indentation is off.



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


Indentation is off.


- Alexander Rojas


On April 5, 2017, 4:24 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58095/
> ---
> 
> (Updated April 5, 2017, 4:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Benjamin Mahler, and 
> Michael Park.
> 
> 
> Bugs: MESOS-4732 and MESOS-7260
> https://issues.apache.org/jira/browse/MESOS-4732
> https://issues.apache.org/jira/browse/MESOS-7260
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of generating JSON object, `Master::Http::roles()` now
> leverages `jsonify` to compute output. Also approver is taken
> out from its continuation function. This is for easier and cleaner
> implementation of framework authorization in /roles endpoint,
> see MESOS-7260.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 6cf9d350446d1b2d4a6e67d552217daff32657ff 
>   src/master/master.hpp 1b077424373d6e195e4ab29e150dedbc3f3f95ab 
> 
> 
> Diff: https://reviews.apache.org/r/58095/diff/2/
> 
> 
> Testing
> ---
> 
> no functional changes
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 58194: Hardened HTTP check tests.

[Alexander Rukletsov]

> On April 7, 2017, 12:31 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Line 837 (original), 839 (patched)
> > 
> >
> > Does this mean the test runs for atleast 1 second? If yes, that's 
> > unfortunate. Also, if for whatever reason (VM slowness) it takes longer 
> > than 1s this will fail?
> > 
> > Can you check the server is ready to serve requests some other way? 
> > Maybe a while loop that checks that the server is up or something?
> 
> Gastón Kleiman wrote:
> Yes, this means that the tests runs for at least 1 second, however I 
> don't see a way of making the delay period dynamic.
> 
> The tests could be rewritten in such a way that status updates with an 
> empty check status are ignored until a non-empty check status is received. 
> Then the test should fail if a non-empty check status is not received within 
> 15 seconds of having started the task but that wouldn't be a small 
> change, and this patch already makes the teets much more robust.
> 
> I'd still ship this, probably mentioning in a TODO how to make the tests 
> more robust/quick.

I had similar thoughts when making this change. One of the main reasons we've 
added checks is to tell when a server is ready to serve requests. I'd argue 
that making this test run 1s is a reasonable trade-off.


- Alexander


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


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58194/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduce 1s delay to ensure the task (HTTP server) has enough time
> to start and start serving request.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58194/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58194: Hardened HTTP check tests.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58194/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduce 1s delay to ensure the task (HTTP server) has enough time
> to start and start serving request.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58194/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58194: Hardened HTTP check tests.

[Gastón Kleiman]

> On April 7, 2017, 12:31 a.m., Vinod Kone wrote:
> > src/tests/check_tests.cpp
> > Line 837 (original), 839 (patched)
> > 
> >
> > Does this mean the test runs for atleast 1 second? If yes, that's 
> > unfortunate. Also, if for whatever reason (VM slowness) it takes longer 
> > than 1s this will fail?
> > 
> > Can you check the server is ready to serve requests some other way? 
> > Maybe a while loop that checks that the server is up or something?

Yes, this means that the tests runs for at least 1 second, however I don't see 
a way of making the delay period dynamic.

The tests could be rewritten in such a way that status updates with an empty 
check status are ignored until a non-empty check status is received. Then the 
test should fail if a non-empty check status is not received within 15 seconds 
of having started the task but that wouldn't be a small change, and this 
patch already makes the teets much more robust.

I'd still ship this, probably mentioning in a TODO how to make the tests more 
robust/quick.


- Gastón


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


On April 4, 2017, 10:24 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58194/
> ---
> 
> (Updated April 4, 2017, 10:24 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduce 1s delay to ensure the task (HTTP server) has enough time
> to start and start serving request.
> 
> 
> Diffs
> -
> 
>   src/tests/check_tests.cpp d7fcbf940102efe0841f07e8c4937a78361bc422 
> 
> 
> Diff: https://reviews.apache.org/r/58194/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58224: RFC: Add some consistency checks for libprocess UPIDs.

[Alexander Rojas]

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




3rdparty/libprocess/src/process.cpp
Lines 471-477 (patched)


I think `SocketSession` or `SocketConnection` are better names for this 
abstraction, since it better describes the intention of its usage.



3rdparty/libprocess/src/process.cpp
Lines 841-843 (original), 851-853 (patched)


For what I saw, every time the `context` is around, it appears with the 
`socket` itself and the `decoder`. Why not puting them (the socket and the 
decoder) inside the context and let it manage their lifetimes.


- Alexander Rojas


On April 6, 2017, 12:15 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58224/
> ---
> 
> (Updated April 6, 2017, 12:15 a.m.)
> 
> 
> Review request for mesos and Mesos Reviewbot.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In general, libprocess is unable to validate that a peer is a legitimate
> owner of the UPID it claims in a libprocess message. This change adds
> 2 checks that make impersonation somewhat harder.
> 
> First, we bind the first UPID to the socket context. This prevents a
> peer attempting to switch UPIDs during a session.
> 
> Second, we enforce that the IP address in the UPID matches the peer
> address. This makes spoofing the UPID harder (eg. to send authenticated
> messages), but also breaks some legitimate configurations, particularly
> on multihomed hosts.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> d0cba0c2299bddfedeb8bfde5b93aae733a9cd5b 
> 
> 
> Diff: https://reviews.apache.org/r/58224/diff/1/
> 
> 
> Testing
> ---
> 
> Minimal manual testing.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Mesos Reviewbot]

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



Patch looks great!

Reviews applied: [58262, 58263]

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 April 7, 2017, 11:05 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58263/
> ---
> 
> (Updated April 7, 2017, 11:05 a.m.)
> 
> 
> Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 29a99f33e646593127b9dc126f398f7bca88e21d 
>   src/slave/containerizer/mesos/containerizer.cpp 
> bc611a5e085de10e9953b5f942d98f2b5747fce6 
> 
> 
> Diff: https://reviews.apache.org/r/58263/diff/1/
> 
> 
> Testing
> ---
> 
> make check on Mac OS and various Linux distros.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58193: Renamed variables in checker library for clarity.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On April 4, 2017, 10:23 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58193/
> ---
> 
> (Updated April 4, 2017, 10:23 p.m.)
> 
> 
> Review request for mesos, Gastón Kleiman and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/checks/checker.cpp 7510bf23977e007d101fab635865b7160c3a5af6 
> 
> 
> Diff: https://reviews.apache.org/r/58193/diff/1/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/58196/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 58254: Added implicit executor authorization to the agent operator API.

[Alexander Rojas]

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




src/authorizer/local/authorizer.cpp
Line 576 (original), 601-603 (patched)


I think options are default initialized to `None()`, in which case I think

```c++
Option subject;
if (request.has_subject()) {
  subject = request.subject();
}
```

would work just as well.



src/authorizer/local/authorizer.cpp
Lines 654-657 (original), 683-690 (patched)


I'm not so sure returning a `RejectingObjectApprover()` is the right thing 
to do. It looks to me that the request is in an invalid state and at the very 
least should log that, but this if sounds like a precondition to me: If it has 
claims, it needs to be one of the three actions. Probably chang it for a 
`CHECK`.

If I'm wrong, at least a comment mentioning when is it a valid case having 
a request that fails the check would help.



src/authorizer/local/authorizer.cpp
Lines 692-699 (patched)


Refer to my review of previous patch.



src/authorizer/local/authorizer.cpp
Lines 743 (patched)


Given the ussage, this one could be promoted to a top level class at this 
point.


- Alexander Rojas


On April 7, 2017, 5:38 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58254/
> ---
> 
> (Updated April 7, 2017, 5:38 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the agent handlers for the LAUNCH_, WAIT_,
> and KILL_NESTED_CONTAINER calls of the operator API to set the
> `container_id` field within the authorization object,
> facilitating implicit executor authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
>   src/slave/http.cpp b07ce7c73a90ef297d980806ebba9530d86f25ae 
> 
> 
> Diff: https://reviews.apache.org/r/58254/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

(Updated April 7, 2017, 11:05 a.m.)


Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


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

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


Testing (updated)
---

See https://reviews.apache.org/r/58263/


Thanks,

Alexander Rukletsov

Review Request 58263: Inherited working dir from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


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


Testing
---

make check on Mac OS and various Linux distros.


Thanks,

Alexander Rukletsov

Review Request 58262: Inherited environment from parent when launching a DEBUG container.

[Alexander Rukletsov]

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

Review request for mesos, Gastón Kleiman, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

DEBUG containers inherit their environment and working directory from
the parent container.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
29a99f33e646593127b9dc126f398f7bca88e21d 
  src/slave/containerizer/mesos/containerizer.cpp 
bc611a5e085de10e9953b5f942d98f2b5747fce6 


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


Testing
---

make check on Mac OS and various Linux distros.


Thanks,

Alexander Rukletsov

Re: Review Request 58253: Added a ContainerID to 'ObjectApprover::Object'.

[Alexander Rojas]

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




include/mesos/authorizer/authorizer.proto
Lines 57 (patched)


I was thinking that instead of having one field `container_id`, why not 
having a map of claims, then you can verify that each claim made by the subject 
matches the claims in the object whithout needing to know the supported claims 
in advance.

Limiting the fields is what lead to the whole redising of the object in the 
first place, from a `string value` to suport the info objects.


- Alexander Rojas


On April 7, 2017, 5:33 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58253/
> ---
> 
> (Updated April 7, 2017, 5:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch adds a new member, `container_id` to the
> `ObjectApprover::Object` to facilitate implicit executor
> authorization.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> 75801ccc753a60ce5e5979b6723fd2294ce7ffe5 
>   include/mesos/authorizer/authorizer.proto 
> 736f76d552956f2351ffd40fc51d088dff83f8c8 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58253/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 56152: Implemented the 'fetchManifest()' method of prefix puller.

[Qian Zhang]

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

(Updated April 7, 2017, 4:10 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Changes
---

Replaced `ManifestDescriptor` with `Descriptor`.


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


Repository: mesos


Description
---

Implemented the 'fetchManifest()' method of prefix puller.


Diffs (updated)
-

  include/mesos/oci/spec.hpp d8eef84b5770608c359285d9168f7ea5de4eba12 
  src/slave/containerizer/mesos/provisioner/oci/paths.hpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/paths.cpp PRE-CREATION 
  src/slave/containerizer/mesos/provisioner/oci/prefix_puller.cpp PRE-CREATION 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 58261: Unified the descriptor type in the OCI protobuf messages.

[Qian Zhang]

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

(Updated April 7, 2017, 4:09 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

See https://github.com/opencontainers/image-spec/pull/620 for details.


Diffs
-

  include/mesos/oci/spec.proto f7f197909ebd5517941e063d8c80b0a7f745bb8d 
  src/oci/spec.cpp 06fceb4f0441431d756eb45c8aaf9730ef9e248a 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 55331: Added 'OCI' message into 'Image' message.

[Qian Zhang]

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

(Updated April 7, 2017, 4:09 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added 'OCI' message into 'Image' message.


Diffs (updated)
-

  include/mesos/mesos.proto 9a66967ab459f75f21faf21be644f39b3fad670b 
  include/mesos/v1/mesos.proto 115f1b4d9e129f83a3aed62a95eb11faa12e04d1 


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

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


Testing
---


Thanks,

Qian Zhang

Review Request 58261: Unified the descriptor type in the OCI protobuf messages.

[Qian Zhang]

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

Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

See https://github.com/opencontainers/image-spec/pull/620 for details.


Diffs
-

  include/mesos/oci/spec.proto f7f197909ebd5517941e063d8c80b0a7f745bb8d 
  src/oci/spec.cpp 06fceb4f0441431d756eb45c8aaf9730ef9e248a 


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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 58220: Used move semantics when returning gzip compressed responses.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On April 5, 2017, 1:53 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58220/
> ---
> 
> (Updated April 5, 2017, 1:53 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We used to copy the compressed body into the eventual response.
> For large responses especially from `/state` endpoint made by the
> Web UI, these can be pretty expensive. This change replaces the
> copy by a move operation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/encoder.hpp 
> ea629d72a68e093343562db1ef7e5d00c723f03b 
> 
> 
> Diff: https://reviews.apache.org/r/58220/diff/1/
> 
> 
> Testing
> ---
> 
> make check
> 
> For a ~10mb compressed response, the original took 3ms vs 2ns (newer).
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 58252: Allowed the local authorizer to accept subjects with no value.

[Alexander Rojas]

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




src/authorizer/local/authorizer.cpp
Line 1091 (original), 1091 (patched)


Not yours (it might even be mine), but if we already checked in the 
previous line `!request.has_subject()`, the only posibility is that it does 
have a subject and `request.has_subject()` is unnecesary. I would delete it.


- Alexander Rojas


On April 7, 2017, 5:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58252/
> ---
> 
> (Updated April 7, 2017, 5:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates checks in the local authorizer to allow subjects
> which specify `claims` instead of a `value`.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e241edf4afa48d35d94d72e8e8690f5bedfc 
> 
> 
> Diff: https://reviews.apache.org/r/58252/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 58251: Changed 'Principal.claims' to a hashmap.

[Alexander Rojas]

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




3rdparty/libprocess/include/process/authenticator.hpp
Line 69 (original), 69 (patched)


My only concern with using either a `std::map` or a `hashmap` is that it 
forces clients to have only one entry of each claim which forces us to find 
creative ways around this. How about using `std::multi_map`, 
`std::multi_hashmap` or `hashmap`?


- Alexander Rojas


On April 7, 2017, 5:26 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58251/
> ---
> 
> (Updated April 7, 2017, 5:26 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Till Toenshoff, and Vinod 
> Kone.
> 
> 
> Bugs: MESOS-7014
> https://issues.apache.org/jira/browse/MESOS-7014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch changes the `claims` member of the authentication
> `Principal` struct from a `std::map` to a `hashmap`, so that
> we can make use of the `contains()` helper during authorization.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/authenticator.hpp 
> 272d92117547512328c09dfc04c6ca4bf7b6b937 
> 
> 
> Diff: https://reviews.apache.org/r/58251/diff/1/
> 
> 
> Testing
> ---
> 
> Testing details can be found at the end of this chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>