Re: Review Request 52070: Mentioned `reserved_resources_full` info on the agent in relevant docs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52070]

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 Sept. 19, 2016, 10:34 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52070/
> ---
> 
> (Updated Sept. 19, 2016, 10:34 p.m.)
> 
> 
> Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and haosdent 
> huang.
> 
> 
> Bugs: MESOS-4668
> https://issues.apache.org/jira/browse/MESOS-4668
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mentioned `reserved_resources_full` info on the agent in relevant docs.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md 4a5d5322d7d95cdbc4d1580ee1ca7a05b2ed9acb 
>   docs/reservation.md 6408646cd42408514566e323beff548b69b6af41 
> 
> Diff: https://reviews.apache.org/r/52070/diff/
> 
> 
> Testing
> ---
> 
> Looks fine in local markdown viewer.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52059: Added test for running the logrotate module as non-root.

[Benjamin Bannier]

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



I just had a chance to look at the fixture.


src/tests/container_logger_tests.cpp (lines 784 - 785)


Ideally you would prevent such leakage from the test fixture.

Any reason you cannot use an existing unpriviledged account like e.g., 
`nobody`?



src/tests/container_logger_tests.cpp (line 832)


Note: since you use `CHECK_SOME` here no fixture teardown will be run (not 
even its dtr).


- Benjamin Bannier


On Sept. 19, 2016, 9:38 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52059/
> ---
> 
> (Updated Sept. 19, 2016, 9:38 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Sivaram Kannan.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test creates a non-privileged user and but runs the agent
> as root.  Logrotate has several branches of logic that are only
> exercised as root.  In this case, we expect logrotate to work
> if the agent and module are root, but the task is non-root.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
> 
> Diff: https://reviews.apache.org/r/52059/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> The new test currently fails (as expected).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Guangya Liu]

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




src/common/resources.cpp (line 613)


Not yours, but I prefer that we quota token as 

```
"Bad value for resources, missing or extra ':' in '" + token + "'");
```



src/common/resources.cpp (lines 633 - 634)


I prefer that we quota `key` and `token`

```
return Error(
"Bad value for resources, mismatched brackets in '" +
 key + "' for '" + token + "'");
```

Ditto for others



src/common/resources.cpp (line 700)


What about adding `UNREACHABLE()` here?


- Guangya Liu


On 九月 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated 九月 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Qian Zhang]

> On Sept. 6, 2016, 7:33 p.m., Vinod Kone wrote:
> >
> 
> Vinod Kone wrote:
> also, can you add a test for this?
> 
> Qian Zhang wrote:
> Sure, I can add a test. I am just wondering how to use a test to verify 
> the logic we added in this patch, maybe we can just add a test to launch a 
> short task and check `ContainerTermination.status` is 0 when 
> `Slave::executorTerminated()` is call which means the HTTP command executor 
> has received the ACK from agent for the terminal status update. Any comments?
> 
> Vinod Kone wrote:
> That sounds like a good test. Can you add that?

Posted a patch for the test:
https://reviews.apache.org/r/52075/


- Qian


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


On Sept. 15, 2016, 4:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Sept. 15, 2016, 4:57 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 302 - 306)


Hum, i still prefer create Info struct after everything is recovered. You 
can pass hashset of subsystem names to `recover`.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (lines 528 - 548)


I don't think we need this change because 'isolate' is not goint to be 
called for old containers. We only need to do the check for methods that can be 
potentially called for the old container.

Plus, you introduced a bug during the conflict resolution.



src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp (line 569)


Ditto. No need for this.


- Jie Yu


On Sept. 20, 2016, 4:13 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51631/
> ---
> 
> (Updated Sept. 20, 2016, 4:13 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6063
> https://issues.apache.org/jira/browse/MESOS-6063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Recover newly added cgroups subsystems on existing containers would
> fail, and continue to perform the `update` and other operations of
> the newly added subsystems on them don't make sense. This patch add
> the tracking for the recovered or prepared cgroups subsystems of a
> container and skip performing unnecessary subsystem operations on the
> container if the subsystem is never recovered or prepared.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
> cf143eef2411b17b775579b1be07c927b64ea835 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 937356b3ef76408bbb01ab699b7a2bd097cdfe82 
> 
> Diff: https://reviews.apache.org/r/51631/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 51631: Tracked recovered and prepared cgroups subsystems for containers.

[haosdent huang]

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

(Updated Sept. 20, 2016, 4:13 a.m.)


Review request for mesos, Gilbert Song, Jie Yu, and Qian Zhang.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Recover newly added cgroups subsystems on existing containers would
fail, and continue to perform the `update` and other operations of
the newly added subsystems on them don't make sense. This patch add
the tracking for the recovered or prepared cgroups subsystems of a
container and skip performing unnecessary subsystem operations on the
container if the subsystem is never recovered or prepared.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/cgroups.hpp 
cf143eef2411b17b775579b1be07c927b64ea835 
  src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
937356b3ef76408bbb01ab699b7a2bd097cdfe82 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52065: Fixed warnings in Windows `shell.hpp`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52061, 52062, 52063, 52065]

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 Sept. 19, 2016, 8:04 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52065/
> ---
> 
> (Updated Sept. 19, 2016, 8:04 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed warnings in Windows `shell.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> 8fb48c1c4178485cac2934833b5c440d873e4fc4 
> 
> Diff: https://reviews.apache.org/r/52065/diff/
> 
> 
> Testing
> ---
> 
> Windows: build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

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



Ditto for v1


include/mesos/resources.hpp (lines 187 - 188)


Why update here, I prefer

```
parses text in the form "name(role):value;name:value;...".
```



include/mesos/resources.hpp (line 201)


I think that we should find a better name for this, the above API `parse` 
is actually also parsing a text.

What about `parseResourceVector` or else some other meaningful name in your 
mind?



src/common/resources.cpp (line 586)


s/vector to Resources/`vector` to `Resources` object



src/slave/containerizer/containerizer.cpp (line 64)


I suggest that we move this to 
https://reviews.apache.org/r/51879/diff/3#index_header


- Guangya Liu


On 九月 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Review Request 52075: Added the test `HTTPCommandExecutorTest.TerminateWithACK`.

[Qian Zhang]

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

Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Added the test `HTTPCommandExecutorTest.TerminateWithACK`.


Diffs
-

  src/slave/slave.hpp 13c76d1eaea2b49519948c9116e5db5caf9407ea 
  src/tests/command_executor_tests.cpp 07e5eb4d7c2ace2b6714fbe02f29d41663152556 
  src/tests/mock_slave.hpp 53e1c9ee1ae5150fc2e6deebb574c9c86e08df7f 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Qian Zhang]

> On Sept. 19, 2016, 5:24 p.m., Jan Schlicht wrote:
> > src/master/validation.cpp, lines 63-78
> > 
> >
> > This will cause problems with tasks that don't use a container image: 
> > While `ContainerInfo.type` is required, the `DockerInfo` or `MesosInfo` 
> > fields are not. If a user doesn't want to use a container image, he will 
> > leave these fields empty and probably set the type to `MESOS`.
> > To fix this, the validation should check if the right type is set, when 
> > an image is specified. Like
> > ```
> >   if (container.has_mesos() && container.type() == 
> > ContainerInfo::MESOS) {
> > return Error("Expecting 'mesos' to be set for mesos container");
> >   }
> > ```
> 
> Jan Schlicht wrote:
> Oops, mistake in the example, should be
> ```
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS) {
>   return Error("Expecting 'mesos' to be set for mesos container");
> }
> ```
> 
> Alexander Rukletsov wrote:
> The "union trick" we use in `ContainerInfo` protobuf 
> (https://github.com/apache/mesos/blob/7a5ec49877a2e1be5b053a6b59ed6f32760fbe7a/include/mesos/mesos.proto#L2058)
>  does not encourage setting type without setting the corresponding field.
> 
> You say that something like this task definition is valid and possible:
> ```
> {
>   "name": "cni-test",
>   "task_id": "task01",
>   "slave_id": 
>  …
>   "container": {
> "type": "mesos",
> "network_infos" : [
>   { "name": "network-a" }
> ]
>   },
>   "command" : {
> "value": "foo"
>  }
> ```
> 
> I would argue, that setting type to `mesos` is misleading. If we allow 
> such task definitions, we should probably introduce another type, e.g. `NONE`.
> 
> Jie Yu wrote:
> This is rather unfortunate and was introduced long time ago. I think we 
> cannot apply this check as it will break existing users.
> 
> I think 'container.type()' is to tell which containerizer to use. We put 
> all the common fields in ContainerInfo. YOu can think of 'docker' and 'mesos' 
> fields contain additional information if needed, but they are optional.
> 
> I think we should check:
> ```
> if (container.has_docker() && container.type() != ContainerInfo::DOCKER)
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS)
> ```
> 
> We should add more documentation here. AlexR, can you own this?
> 
> Alexander Rukletsov wrote:
> > I think 'container.type()' is to tell which containerizer to use.
> 
> But what if we omit `ContainerInfo` altogether? In this case, we do not 
> explicitly tell what containerizer to use and it is deduced automagically.
> 
> I'm happy to drive the doc update, however I would need some help in 
> understanding what the options are : )
> 
> Jie Yu wrote:
> > But what if we omit ContainerInfo altogether? In this case, we do not 
> explicitly tell what containerizer to use and it is deduced automagically.
> 
> If ContainerInfo is not set, we assume it'll be handled by 
> MesosContainerizer. It's like saying the default container runtime is 
> MesosContainerizer.

It seems this check has broken the existing users: 
https://issues.apache.org/jira/browse/MESOS-6208


- Qian


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


On Sept. 16, 2016, 6:19 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51865/
> ---
> 
> (Updated Sept. 16, 2016, 6:19 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6157
> https://issues.apache.org/jira/browse/MESOS-6157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
>   src/tests/container_logger_tests.cpp 
> 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 0d611c196870b6adabea52a48abcd344c8dad5d1 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 1671d45171307cda62184505ce1dbadc476abca6 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> df4642d2667407b1758ffe2efcfdbf9968cf2c33 
>   src/tests/containerizer/isolator_tests.cpp 
> 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/slave_tests.cpp 

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Guangya Liu]

> On 九月 19, 2016, 6 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 64-71
> > 
> >
> > A question here: Does the `resources` here will include the zero value 
> > resource?
> > 
> > If so, what is the differencen of this code block and 
> > `Resources::parse()`? The `Resources::parse()` seems similar as here.
> 
> Anindya Sinha wrote:
> The original code returns a `Resources` which contains individual 
> `Resource` objects, which removes any empty `Resource` objects 
> (https://github.com/apache/mesos/blob/master/src/common/resources.cpp#L1610).
> Since we need to know of empty `Resource` objects for this patch, we 
> actually accumulate the `Resource` objects in a `vector` (which 
> will contain any empty `Resource` object) which is used when disk resources 
> are present (to check for scalar value of 0 for the auto detection logic). 
> But for the rest of the resources, we do a `Resources resources = 
> parsed.get();` which implicitly converts a `vector` to `Resources` 
> which removes the empty `Resource` objects.

I see, that's why this update makes me confused: You are actually doing the 
same thing as `Resources::parse()` here.

I think that it is not good to put the update here but merge this to 
https://reviews.apache.org/r/51879/diff/3#index_header , comments?


- Guangya


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


On 九月 19, 2016, 10:42 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated 九月 19, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Jie Yu]

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



Haven't looked at the tests yet.


src/Makefile.am (line 833)


This should belong linux files below?



src/Makefile.am (line 958)


Ditto.



src/slave/containerizer/mesos/containerizer.cpp (line 73)


Hum, the headers here is crazy (too many ifdef linux here). The order of 
this is not correct.

I suggest we group all linux related headers together.



src/slave/containerizer/mesos/containerizer.cpp (line 99)


Not yours, but why this is not wrapped with ifdef linux?



src/slave/containerizer/mesos/containerizer.cpp (lines 1242 - 1251)


I understand why you want to do this check, but we usually do not do such 
checks in containerizer. We rely on operator to properly configure the agent.

The correct solution I believe is to advertise agent isolation capabilities 
to the master and let master reject those tasks.

I would simply remove this check here for now.



src/slave/containerizer/mesos/isolators/linux/capabilities.hpp (line 17)


LINUX_CAPABILITIES



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 40)


Given that we don't need to do the check in the agent, please just inline 
this method.



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (line 94)


I don't follow the if/else here. Should we simply calculate "what will be 
the capabilities for the task/executor, depending on what is inside 
containerConfig.container_info() and flags.allowed_capabilities" first, and 
then, depending on if the container is a command task (w or w/ rootfs) or 
custom executor, setting launchInfo accordingly?



src/slave/containerizer/mesos/isolators/linux/capabilities.cpp (lines 103 - 104)


Hum, if i specify `--allowed_capabilities` and the framework does not 
specify capabilities for TaskInfo.container. Will this work?

I think you need to consider three cases here:
1) For command task that has rootfs. In that case, we need to make sure 
when we launch the executor, we don't specify capabilities (keep it None() in 
launchInfo, indicating that we don't want to limit capabilities). This is 
because the executor will perform priviledged operations like pivot_root and 
setuid, we want to make sure it is not restricted.
2) For command task that has no rootfs. In that case, simply set 
launchInfo.capabilities, meaning that when launching the executor, we limit the 
capabilities of it. So the task will inherit the capabilities when the executor 
fork-exec the task.
3) For custom executor case, simply set launchInfo.capabilities.

To check if it is a command task is by checking 
containerConfig.has_task_info().


- Jie Yu


On Sept. 19, 2016, 2:21 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 19, 2016, 2:21 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes
> net capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed from the isolator via a new
> capability field in `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 20db010ea158a813034b41ce9cddac7d8317 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
>   src/tests/containerizer/isolator_tests.cpp 
> 93ce75180520d382f63ce7323be844fe43c6594e 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 

Re: Review Request 52059: Added test for running the logrotate module as non-root.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52059]

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 Sept. 19, 2016, 7:38 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52059/
> ---
> 
> (Updated Sept. 19, 2016, 7:38 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan and Sivaram Kannan.
> 
> 
> Bugs: MESOS-5856
> https://issues.apache.org/jira/browse/MESOS-5856
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This test creates a non-privileged user and but runs the agent
> as root.  Logrotate has several branches of logic that are only
> exercised as root.  In this case, we expect logrotate to work
> if the agent and module are root, but the task is non-root.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
> 
> Diff: https://reviews.apache.org/r/52059/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> The new test currently fails (as expected).
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[haosdent huang]

> On Sept. 19, 2016, 4:55 p.m., haosdent huang wrote:
> > src/common/resources.cpp, lines 616-648
> > 
> >
> > This is conflict with range resources `ports:[21000-24000,3-34000]`.
> 
> Anindya Sinha wrote:
> I do not think so. `ports:[21000-24000,3-34000]` will have 
> `key=ports` and `value=[21000-24000,3-34000]`. So this additional loop 
> would not be entered since the `key` does not include the square brackets. 
> Note that key is set based on number of tokens (2 in this specific case, and 
> 3 when we include MOUNT/PATH disks, ie. `disk[MOUNT:/tmp]:2048`). The new 
> logic is not applied on value, but only on key (which is whole resource 
> excluding the value).

Got it, thanks for your explanation! Another question is if we have more 
attributes in the future, would it use this form?

```
disk[attr_1:value_1,attr_2:value2]
```


- haosdent


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


On Sept. 19, 2016, 10:43 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52033: Escalated some openssl logs from VLOG to INFO.

[Joris Van Remoortere]

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




3rdparty/libprocess/src/openssl.cpp (line 289)


Can you see if there is any other valuable information in the 
initialization routine that might be of value when a user posts a JIRA issue to 
help identify the code paths their system went through?
Might be a good time to do a full review of the logging in this function 
rather than just up a few levels.

You have been debugging many scenarios so I think you are in a unique 
position to do this!
Thanks Till!



3rdparty/libprocess/src/openssl.cpp (line 446)


Why not this one?



3rdparty/libprocess/src/openssl.cpp (line 475)


Why not this one?



3rdparty/libprocess/src/openssl.cpp (line 478)


Why not this one?



3rdparty/libprocess/src/openssl.cpp (line 485)


Why not this one?


- Joris Van Remoortere


On Sept. 19, 2016, 1:50 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52033/
> ---
> 
> (Updated Sept. 19, 2016, 1:50 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The logging messages in question prove to be very useful for debugging the
> cluster setup and hence we decided they should be generally available as their
> noise by far outwages their helpfulnes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52033/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joris Van Remoortere]

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



Agree with @Joseph's comments about keeping the error codes.


3rdparty/libprocess/src/openssl.cpp (lines 481 - 482)


Is there any information we can provide here about where we are looking for 
the defaults to help the user identify the problem?


- Joris Van Remoortere


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 52056: Exposed unknown container case from Containerizer::destroy.

[Benjamin Mahler]

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

Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

Currently the callers of `destroy` cannot determine if the call
succeeds or fails (without a secondary call to `wait()`).

This also allows the caller to distinguish between a failure and
waiting on an unknown container. This is important for the upcoming
agent child container API, as the end-user would benefit from the
distinction.


Diffs
-

  src/slave/containerizer/composing.hpp 
ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
  src/slave/containerizer/composing.cpp 
5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
  src/slave/containerizer/containerizer.hpp 
f13669d0dfc4ce3287cfe630cabd0470dc765b51 
  src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
  src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
  src/slave/containerizer/mesos/containerizer.hpp 
078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
  src/slave/containerizer/mesos/containerizer.cpp 
dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
  src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
  src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
  src/tests/containerizer/composing_containerizer_tests.cpp 
51aab33cd190b53328339e39fd12853714882454 
  src/tests/containerizer/docker_containerizer_tests.cpp 
28cd3fa66886dbdbae3fdeca77707147faafcb7a 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

Added tests.

make check


Thanks,

Benjamin Mahler

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joris Van Remoortere]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.
> 
> Joseph Wu wrote:
> I think the error codes might be useful for debugging.  I'm guessing the 
> error strings may change between OpenSSL versions (I wonder if there's 
> localization too), but the error codes will not.

The error codes are very helpful. Please don't remove them. As Joseph has 
suggested, they're the easiest way to search for references on google, openssl 
mailing list, etc.


- Joris


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Avinash sridharan]

> On Sept. 19, 2016, 11:43 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 666-668
> > 
> >
> > Do we need a new mount namespace if i just want execute some command on 
> > the host mount table without a rootfs?
> 
> Avinash sridharan wrote:
> This code is part of the else block, which means that the container is 
> joining a non-host network, implying that it needs its own mount namespace, 
> since the network files will be different than the host network files?
> 
> This is a child container, so it doesn't need a new NETNS or UTS 
> namespace but it will require a new MNT namespace.
> 
> Jie Yu wrote:
> OK, I guess here is a little tricky. For a nested container, the 
> containerizer will correctly enter the namespace of its parent container's 
> network namespace and UTS namespace. The following cases are possible:
> 1) The parent container joins the host network, does not have a rootfs
> 2) The parent container joins the host network, has a rootfs
> 3) The parent container joins CNI networks, does not have a rootfs
> 4) The parent contianer joins CNI networks, has a rootfs
> 
> For 1), there's no Info associated with the parent container, and I don't 
> think we need a mount namespace for child container.
> For 2), there's an Info associated with the parent container (only has 
> rootfs set). If the child container does not have a rootfs, we don't need a 
> mount namespace. If the child container has a rootfs, we need a mount 
> namespace.
> For 3), there's an Info (both rootfs and containerNetworks are set). No 
> mather if the child has a rootfs or not, we need a mount namespace.
> For 4), same as 4)
> 
> Better to document the above in the code.
> 
> I think the invarient is that: we only create Info for child containers 
> that need a new mount namespace.

Yeah, I see your point. Not very evident from the code. Let me merge the 
`prepare` and `isolate` patches and I will add an explicit comment highlighting 
these cases.


- Avinash


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


On Sept. 16, 2016, 11 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 16, 2016, 11 p.m.)
> 
> 
> Review request for Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `prepare` method to be aware of nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 40411: Libprocess Reinit: Modify test to use PID.

[Joseph Wu]

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

(Updated Sept. 19, 2016, 5:41 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Joris Van Remoortere, 
and Vinod Kone.


Changes
---

Update one new test (added since last rebase).


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


Repository: mesos


Description
---

Updates the test's reference to the global metrics singleton.


Diffs (updated)
-

  src/tests/scheduler_driver_tests.cpp faf2e6c8ad17e07964b4340d0b340654b03f9086 
  src/tests/scheduler_tests.cpp b0ea0bbcce9d847285fda40f778caaf721804457 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Review Request 52055: Exposed unknown container case from Containerizer::wait.

[Benjamin Mahler]

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

Review request for mesos, Jie Yu and Vinod Kone.


Repository: mesos


Description
---

This allows the caller to distinguish between a failure and waiting
on an unknown container. This is important for the upcoming agent
child container API, as the end-user would benefit from the
distinction.


Diffs
-

  src/slave/containerizer/composing.hpp 
ef3c2ee22cf0ced35c64025c7f9f7ca75165d54d 
  src/slave/containerizer/composing.cpp 
5ff3e65facd5831e2637ae1cee3ea01b21d0f6b2 
  src/slave/containerizer/containerizer.hpp 
f13669d0dfc4ce3287cfe630cabd0470dc765b51 
  src/slave/containerizer/docker.hpp a378fa9772742b4934578efcec54aeeaf5791a83 
  src/slave/containerizer/docker.cpp 110a1eb41b1ff7cb94f3630a1843d9f01efbe09c 
  src/slave/containerizer/mesos/containerizer.hpp 
078ef4f4e7bf5e1522804a720c51cfa5518d8efd 
  src/slave/containerizer/mesos/containerizer.cpp 
dc18e4e3b0eca3f116f1e240217bbebf64a75e3a 
  src/slave/slave.hpp e659c44fcc415f10a6980e7b29cc1ce536dfe657 
  src/slave/slave.cpp 2c7b5ada1aa81babb9ceee0c9f928685878a778c 
  src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
  src/tests/containerizer.hpp f1fd57945c09fb80b7790f9124843a5d4ea785ee 
  src/tests/containerizer.cpp bda3e6f84f77a95f0eb9ff3aabdc9513f0f18b3f 
  src/tests/containerizer/composing_containerizer_tests.cpp 
51aab33cd190b53328339e39fd12853714882454 
  src/tests/containerizer/docker_containerizer_tests.cpp 
28cd3fa66886dbdbae3fdeca77707147faafcb7a 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
df4642d2667407b1758ffe2efcfdbf9968cf2c33 
  src/tests/containerizer/isolator_tests.cpp 
9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 
  src/tests/containerizer/port_mapping_tests.cpp 
7fac6ca1550acf54616fb4cef2eab1335f5e9760 
  src/tests/health_check_tests.cpp 50a252b5267cbc3b21ddc75e1da86c4975f6faf1 
  src/tests/hook_tests.cpp d864ef3d6153dedab8cc47403b1b9f76d75aeb5c 
  src/tests/slave_recovery_tests.cpp d58d9bc0dbb0a60e114699485d1306202981ecf5 

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


Testing
---

Added tests.

make check


Thanks,

Benjamin Mahler

Re: Review Request 51931: Added `ping` to test linux rootfs.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 19, 2016, 2:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51931/
> ---
> 
> (Updated Sept. 19, 2016, 2:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `ping` to test linux rootfs.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/rootfs.hpp f8fcfd445687760b9c0f025f137deaa0f1afb786 
> 
> Diff: https://reviews.apache.org/r/51931/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/50271/ on ubuntu-14.04 w/ 
> gcc-4.8.4 w/o optimizations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51930: Introduced Linux capabilities support for Mesos executor.

[Jie Yu]

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




src/launcher/executor.cpp (line 817)


It looks wierd that we have ifdef here but not have that for the field 
member. I'd suggest we don't do any ifdef for flags as they are optional 
anyway. Otherwise, you'll have to change all the constructors as well, which is 
a little messy.


- Jie Yu


On Sept. 19, 2016, 2:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51930/
> ---
> 
> (Updated Sept. 19, 2016, 2:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces Linux capability-based security the Mesos
> exector. A new flag `capabilities` is introduced to optionally specify
> the capabilities tasks launched by the Mesos executor are allowed to
> use.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
>   src/launcher/posix/executor.hpp 9e467263dcc0623fce83b18887896547db9aa16b 
>   src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
> 
> Diff: https://reviews.apache.org/r/51930/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/50271/ on ubuntu-14.04 w/ 
> gcc-4.8.4 w/o optimizations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51930: Introduced Linux capabilities support for Mesos executor.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 19, 2016, 2:19 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51930/
> ---
> 
> (Updated Sept. 19, 2016, 2:19 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces Linux capability-based security the Mesos
> exector. A new flag `capabilities` is introduced to optionally specify
> the capabilities tasks launched by the Mesos executor are allowed to
> use.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 5370634ef9e6f3ac9717fed71f6a77707026a16a 
>   src/launcher/posix/executor.hpp 9e467263dcc0623fce83b18887896547db9aa16b 
>   src/launcher/posix/executor.cpp 7c40ebebb983b7977526fee123bf07c86fbb5e50 
> 
> Diff: https://reviews.apache.org/r/51930/diff/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/50271/ on ubuntu-14.04 w/ 
> gcc-4.8.4 w/o optimizations.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

> On Aug. 13, 2016, 9:13 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, line 413
> > 
> >
> > `all` is a bit misleading since "gc" is not being terminated. can you 
> > just call it "terminate"? i know there is already a process specific 
> > overload. or maybe call it "finalize" ?

I renamed this earlier to remove the semantic between 
`ProcessManager::finalize` and `process::finalize`.
See: https://reviews.apache.org/r/40266/#comment194260

I can call it `terminate_all_except_gc` to be clearer.


> On Aug. 13, 2016, 9:13 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1143
> > 
> >
> > but all the processes are already terminated by this point? do you mean 
> > just the gc process my dereference it?

Even `gc` will be terminated in a codepath that hits `socket_manager`.  And new 
processes *may* be created between terminating everything (but gc) and cleaning 
up the socket manager.


> On Aug. 13, 2016, 9:13 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, line 1170
> > 
> >
> > but process manager is cleaned up above?

`__address__` is mostly used to distinguish local and remote PIDs.


> On Aug. 13, 2016, 9:13 p.m., Vinod Kone wrote:
> > 3rdparty/libprocess/src/process.cpp, lines 2410-2417
> > 
> >
> > why do this here instead of #1155?

The `processes_mutex` is only available inside the ProcessManager.  We need to 
synchronize the delete of `gc` as an asynchronous call to `process::spawn(..., 
true)` during finalization may try to spawn a gc-managed process.


- Joseph


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


On Sept. 19, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40266/
> ---
> 
> (Updated Sept. 19, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Joris Van Remoortere, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-3910
> https://issues.apache.org/jira/browse/MESOS-3910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `SocketManager` and `ProcessManager` are highly inter-dependent, 
> which requires some untangling in `process::finalize`.
> 
> * Logic originally found in `~ProcessManager` has been split into 
>   `ProcessManager::finalize` due to what happens during cleanup.
> * The future from `__s__->accept()` must be explicitly discarded as 
>   libevent does not detect a locally closed socket.
> * Terminating `HttpProxy`s must close the associated socket.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a6fbf92bd6d69d94490b9faf4960f14aa112e8d2 
> 
> Diff: https://reviews.apache.org/r/40266/diff/
> 
> 
> Testing
> ---
> 
> `make check` (libev)
> `make check` (--enable-libevent --enable-ssl)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

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

(Updated Sept. 19, 2016, 5:26 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Joris Van Remoortere, 
and Vinod Kone.


Changes
---

Rename the ProcessManager termination helper function.  Revise a bunch of 
comments.


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


Repository: mesos


Description
---

The `SocketManager` and `ProcessManager` are highly inter-dependent, 
which requires some untangling in `process::finalize`.

* Logic originally found in `~ProcessManager` has been split into 
  `ProcessManager::finalize` due to what happens during cleanup.
* The future from `__s__->accept()` must be explicitly discarded as 
  libevent does not detect a locally closed socket.
* Terminating `HttpProxy`s must close the associated socket.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp a6fbf92bd6d69d94490b9faf4960f14aa112e8d2 

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


Testing
---

`make check` (libev)
`make check` (--enable-libevent --enable-ssl)


Thanks,

Joseph Wu

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Jie Yu]

> On Sept. 19, 2016, 11:43 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 666-668
> > 
> >
> > Do we need a new mount namespace if i just want execute some command on 
> > the host mount table without a rootfs?
> 
> Avinash sridharan wrote:
> This code is part of the else block, which means that the container is 
> joining a non-host network, implying that it needs its own mount namespace, 
> since the network files will be different than the host network files?
> 
> This is a child container, so it doesn't need a new NETNS or UTS 
> namespace but it will require a new MNT namespace.

OK, I guess here is a little tricky. For a nested container, the containerizer 
will correctly enter the namespace of its parent container's network namespace 
and UTS namespace. The following cases are possible:
1) The parent container joins the host network, does not have a rootfs
2) The parent container joins the host network, has a rootfs
3) The parent container joins CNI networks, does not have a rootfs
4) The parent contianer joins CNI networks, has a rootfs

For 1), there's no Info associated with the parent container, and I don't think 
we need a mount namespace for child container.
For 2), there's an Info associated with the parent container (only has rootfs 
set). If the child container does not have a rootfs, we don't need a mount 
namespace. If the child container has a rootfs, we need a mount namespace.
For 3), there's an Info (both rootfs and containerNetworks are set). No mather 
if the child has a rootfs or not, we need a mount namespace.
For 4), same as 4)

Better to document the above in the code.

I think the invarient is that: we only create Info for child containers that 
need a new mount namespace.


- Jie


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


On Sept. 16, 2016, 11 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 16, 2016, 11 p.m.)
> 
> 
> Review request for Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `prepare` method to be aware of nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52069: Moved `CHECK_NE` close to the `if (task.isSome())`.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 19, 2016, 11:14 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52069/
> ---
> 
> (Updated Sept. 19, 2016, 11:14 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We would vote `CHECK_NE` close to the `if (task.isSome())` loop
> because the `CHECK_NE` just above it makes it clear that taskGroup
> is some in the else block.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 
> 
> Diff: https://reviews.apache.org/r/52069/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51871: Modified the `isolate` method to be nesting aware.

[Avinash sridharan]

> On Sept. 19, 2016, 11:55 p.m., Jie Yu wrote:
> > Let's merge the patches into one single patch. These patches are highly 
> > related. It's nice to review them in a single diff.

Will do.


> On Sept. 19, 2016, 11:55 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 736-764
> > 
> >
> > I'd combine this with the host network namespace with rootfs case. See 
> > my comments in 'prepare' patch.

The reason I wanted to treat them separate from the host network namespace with 
rootfs case, is that child containers joining host network with rootfs are the 
same as any other container, they don't get a new network namespace and they 
basically get the hosts network files bind mounted to the container's rootfs. 
However, a child container joining a non-host network namespace is special in 
the sense that it basically gets the networks files from its parent (root 
container). Given the dissimilarity of the cases, didn't make sense to treat 
them the same?


> On Sept. 19, 2016, 11:55 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 728
> > 
> >
> > Instead of a CHECK, i'd prefer just return Failure here.

Doesn't this look like a bug? We should never hit this condition. Hence thought 
a CHECK made more sense than a FAILURE?


- Avinash


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


On Sept. 18, 2016, 5:40 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51871/
> ---
> 
> (Updated Sept. 18, 2016, 5:40 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a symlink to a parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51871/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Avinash sridharan]

> On Sept. 19, 2016, 11:54 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 600-604
> > 
> >
> > I was under the impressesion child container's always need to have the 
> > `container_info` set? If it does not have `contianer_info` set, does it 
> > mean it just shares the paren't network namespace?
> 
> Jie Yu wrote:
> Yes, exactly. In that case, why do you return a failure?

No you are right, I mistakenly assumed that child container's had to have thier 
`container_info` set.


- Avinash


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


On Sept. 16, 2016, 11 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 16, 2016, 11 p.m.)
> 
> 
> Review request for Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `prepare` method to be aware of nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Jie Yu]

> On Sept. 19, 2016, 11:54 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 600-604
> > 
> >
> > I was under the impressesion child container's always need to have the 
> > `container_info` set? If it does not have `contianer_info` set, does it 
> > mean it just shares the paren't network namespace?

Yes, exactly. In that case, why do you return a failure?


- Jie


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


On Sept. 16, 2016, 11 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 16, 2016, 11 p.m.)
> 
> 
> Review request for Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `prepare` method to be aware of nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51857: Modified the `prepare` method to be aware of nested containers.

[Avinash sridharan]

> On Sept. 19, 2016, 11:43 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 607
> > 
> >
> > Why we need to do that? If we copy containerNetworks, that means 
> > 'isolate' will try to create containerDir? I think we should ignore 
> > contaienrNetworks for nested containers, and only record 'rootfs' (similar 
> > to the containers that want to join host network, but have rootfs defined)

If you look at the code below we handle container's joinging the host network 
vs container's joining a new network namespace differently, based on whether 
the contianerNetworks is empty or not. I wanted to keep the logic the same for 
child containers in pods.

As far as isolate is concerned we do an indireciton to __isolate in isolate 
based on whether the container has a rootfs and joins the host network. We do a 
similar indirection in a separate patch for isolate for child containers.


> On Sept. 19, 2016, 11:43 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 600-604
> > 
> >
> > Hum, why this change? What if i just want to launch a command without 
> > extra container info?

I was under the impressesion child container's always need to have the 
container_info set? If it does not have contianer_info set, does it mean it 
just shares the paren't network namespace?


> On Sept. 19, 2016, 11:43 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 666-668
> > 
> >
> > Do we need a new mount namespace if i just want execute some command on 
> > the host mount table without a rootfs?

This code is part of the else block, which means that the container is joining 
a non-host network, implying that it needs its own mount namespace, since the 
network files will be different than the host network files?

This is a child container, so it doesn't need a new NETNS or UTS namespace but 
it will require a new MNT namespace.


- Avinash


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


On Sept. 16, 2016, 11 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51857/
> ---
> 
> (Updated Sept. 16, 2016, 11 p.m.)
> 
> 
> Review request for Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified the `prepare` method to be aware of nested containers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 822f11eab5b00c014563322a8c3b2c14cb440e0b 
> 
> Diff: https://reviews.apache.org/r/51857/diff/
> 
> 
> Testing
> ---
> 
> make 
> make check
> and
> sudo ./bin/mesos-tests.sh
> 
> The only tests that failed were the SUDO make check tests:
> [  FAILED  ] 3 tests, listed below:
> [  FAILED  ] CgroupsAnyHierarchyWithCpuMemoryTest.ROOT_CGROUPS_Listen
> [  FAILED  ] CgroupsAnyHierarchyMemoryPressureTest.ROOT_IncreaseRSS
> [  FAILED  ] LinuxFilesystemIsolatorTest.ROOT_RecoverOrphanedPersistentVolume
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51871: Modified the `isolate` method to be nesting aware.

[Jie Yu]

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



Let's merge the patches into one single patch. These patches are highly 
related. It's nice to review them in a single diff.


src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 728)


Instead of a CHECK, i'd prefer just return Failure here.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 736 - 764)


I'd combine this with the host network namespace with rootfs case. See my 
comments in 'prepare' patch.


- Jie Yu


On Sept. 18, 2016, 5:40 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51871/
> ---
> 
> (Updated Sept. 18, 2016, 5:40 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, Joseph Wu, and Qian Zhang.
> 
> 
> Bugs: MESOS-6156
> https://issues.apache.org/jira/browse/MESOS-6156
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The network file setup in the `network/cni` isolator is now nesting
> aware. Since the children share the network and UTS namespace with the
> parent, the network files need to be created only for the parent
> container. For the child containers, the network files will be simply
> a symlink to a parents network files.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 359479083894e887647a694a1a133dce44817073 
> 
> Diff: https://reviews.apache.org/r/51871/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> sudo ./bin/mesos-tests.sh
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

> On Sept. 19, 2016, 8:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.
> 
> Joseph Wu wrote:
> Right, forgot about the fetcher's own stdout/stderr :)
> 
> I'd argue it makes sense to not do any chown-ing at all.  The 
> stdout/stderr files are technically created by other entities than the 
> executor (i.e. the fetcher, agent, and logger module).  Not sure if this 
> breaks any expectations in the fetcher though...

Since now the fetcher is being run as task user as a result of jira 
https://issues.apache.org/jira/browse/MESOS-5845 so these 2 files need to be 
chowned to the same user for the fetcher to be able to write to them.


- Megha


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


On Sept. 19, 2016, 11:08 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 19, 2016, 11:08 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52054: Updated slave test to expect 500 rather than 503 from libprocess.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52053, 52054]

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 Sept. 19, 2016, 7:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52054/
> ---
> 
> (Updated Sept. 19, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously HTTP handler failures lead to a 'Service Unavailble'
> response in libprocess. Now it leads to an 'Internal Server Error'.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 2f3fa5fae634b6250f3c00bbef3077493f79af95 
> 
> Diff: https://reviews.apache.org/r/52054/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Joseph Wu]

> On Sept. 19, 2016, 1:35 p.m., Joseph Wu wrote:
> > src/slave/containerizer/fetcher.cpp, lines 773-775
> > 
> >
> > (You'll want to update this comment.)
> > 
> > There isn't always a stdout/stderr file in the sandbox, as this depends 
> > on the ContainerLogger module.  Would non-recursively chown-ing the sandbox 
> > directory work as well?  (This is already done when the agent first creates 
> > the sandbox.)
> 
> Megha Sharma wrote:
> The FetcherProcess::run(...) already creates the stdout and stderr and 
> then recursively changes the ownership of the sandbox to make sure these 2 
> files have the right ownership which I modified to change the ownership of 
> just these 2 files exclusively and not the entire sandbox. So, at this point 
> the stdout and stderr are already created. 
> Also, like you mentioned the agent is already doing a chown while 
> creating the sandbox directory so the sandbox is already owned by the desired 
> user. The concern that I am addressing here is avoiding unnecessary chowning 
> of sandbox so files laid out by other entities don't change ownership but we 
> still anyway need to set the right owners for stdout and stderr here.

Right, forgot about the fetcher's own stdout/stderr :)

I'd argue it makes sense to not do any chown-ing at all.  The stdout/stderr 
files are technically created by other entities than the executor (i.e. the 
fetcher, agent, and logger module).  Not sure if this breaks any expectations 
in the fetcher though...


- Joseph


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


On Sept. 19, 2016, 4:08 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Sept. 19, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5218
> https://issues.apache.org/jira/browse/MESOS-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fetcher code changes the ownership of entire sandbox directory
> recursively to the taskuser and as a resut also changes the
> ownership of files laid out by other entities in the sandbox.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 96e24500a12825161553eb050da389088b122695 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 51869: Export "reserved_resources" in the agent HTTP endpoint.

[Jiang Yan Xu]

> On Sept. 18, 2016, 6:51 a.m., haosdent huang wrote:
> > It would be better that we add this in the new operator api as well. Should 
> > I create a ticket at https://issues.apache.org/jira/browse/MESOS-6007 ?
> 
> Jiang Yan Xu wrote:
> We should (my comment above). I'll take care of it.

Added MESOS-6211.


- Jiang Yan


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


On Sept. 16, 2016, 11:24 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51869/
> ---
> 
> (Updated Sept. 16, 2016, 11:24 a.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-6085
> https://issues.apache.org/jira/browse/MESOS-6085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also the 'resource' field now exposes the total resources, which is
> consistent with the same field in the master's /slaves endpoint.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 67463105d7fa38b2158a64f6994e3dd353a9fcc7 
>   src/slave/slave.cpp 7f99e4610d06ebadbef48ce314fec6ad04acb307 
> 
> Diff: https://reviews.apache.org/r/51869/diff/
> 
> 
> Testing
> ---
> 
> make check. A new test is added in https://reviews.apache.org/r/51870/.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52069: Moved `CHECK_NE` close to the `if (task.isSome())`.

[Guangya Liu]

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

(Updated 九月 19, 2016, 11:14 p.m.)


Review request for mesos, Abhishek Dasgupta and Vinod Kone.


Summary (updated)
-

Moved `CHECK_NE` close to the `if (task.isSome())`.


Repository: mesos


Description (updated)
---

We would vote `CHECK_NE` close to the `if (task.isSome())` loop
because the `CHECK_NE` just above it makes it clear that taskGroup
is some in the else block.


Diffs (updated)
-

  src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 19, 2016, 11:08 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

(Updated Sept. 19, 2016, 11:04 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52069: Moved `CHECK_NE` out of the offers loop.

[Vinod Kone]

> On Sept. 19, 2016, 10:40 p.m., Vinod Kone wrote:
> > src/cli/execute.cpp, line 384
> > 
> >
> > While it's true that this can only be done once, I would vote to keep 
> > it close to the `if (task.isSome())` loop because the CHECK just above it 
> > makes it clear that `taskGroup` is some in the `else` block.
> 
> Guangya Liu wrote:
> Thanks Viond, got it. If this is the case, then for 
> https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L557-L558 , 
> do we need to move this right before 
> https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L600 ?

yup, good catch! you can use this review to do that :) also needs a new line 
before #562


- Vinod


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


On Sept. 19, 2016, 10:34 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52069/
> ---
> 
> (Updated Sept. 19, 2016, 10:34 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We only need to `CHECK_NE(task.isSome(), taskGroup.isSome())` when
> get offers and there is no need to `CHECK_NE` for each offer.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 
> 
> Diff: https://reviews.apache.org/r/52069/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 52069: Moved `CHECK_NE` out of the offers loop.

[Guangya Liu]

> On 九月 19, 2016, 10:40 p.m., Vinod Kone wrote:
> > src/cli/execute.cpp, line 384
> > 
> >
> > While it's true that this can only be done once, I would vote to keep 
> > it close to the `if (task.isSome())` loop because the CHECK just above it 
> > makes it clear that `taskGroup` is some in the `else` block.

Thanks Viond, got it. If this is the case, then for 
https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L557-L558 , do 
we need to move this right before 
https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L600 ?


- Guangya


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


On 九月 19, 2016, 10:34 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52069/
> ---
> 
> (Updated 九月 19, 2016, 10:34 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We only need to `CHECK_NE(task.isSome(), taskGroup.isSome())` when
> get offers and there is no need to `CHECK_NE` for each offer.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 
> 
> Diff: https://reviews.apache.org/r/52069/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 52071: Updated docs to reflect handling of disk resource with size of 0.

[Anindya Sinha]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

When a disk resource with size of 0 is specified in `--resources`
startup flag of mesos agent, the disk size is auto detected by the
agent. This is done for both root disks as well as MOUNT disks, but
is not allowed for PATH disks since PATH disks can be carved into
smaller volumes.


Diffs
-

  docs/attributes-resources.md 2caa44927d45c0ab13f8160794b50f4fde3711aa 

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


Testing
---

Documentation change only. All tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


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


Repository: mesos


Description
---

When static resources indicate disks with a positive size, we use that
for the disk resources on the agent. However, --resources can include
disks with size of 0, which indicates that mesos agent determine the
size of those disks from the host and uses that information instead.
Note that this is not allowed for PATH disks since PATH disks can be
carved into smaller chunks and we cannot assume that the whole
physical disk is available to the PATH disk.

With this change, JSON or textual representation for disk resources
that specify scalar value of 0 would not result in an error, but those
resources will not be accounted for until a valid size is determined
for such resources. A scalar value of -1 in JSON or textual formats
still results in an invalid resource.


Diffs (updated)
-

  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 

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


Testing
---

Tests passed.


Thanks,

Anindya Sinha

Re: Review Request 51880: Added unit tests to determine disk size for MOUNT disks.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added unit tests to determine disk size for MOUNT disks.


Diffs (updated)
-

  src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
  src/tests/containerizer/common_containerizer_tests.cpp PRE-CREATION 

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


Testing
---

All tests including the additional tests in this RR passed.


Thanks,

Anindya Sinha

Re: Review Request 51879: Determine disk size when not specified in static resources.

[Anindya Sinha]

> On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote:
> > src/slave/containerizer/containerizer.cpp, lines 198-259
> > 
> >
> > I suggest that we move the logic which parse `disk(0)` to resources.cpp 
> > or resources_utils.cpp.

`Resources` will never contain a resource which has a size of 0, since it does 
not add empty resources. That is the reason we refactored the 
`Resources::parse()` to the new function which returns a 
`Try` instead of `Try`. Since this logic is very 
specific to how `--resources` are handled, I think it is fine staying within 
this module, and I do not think it needs to move to resources.cpp or 
resources_utils.cpp (since these files deals specific to `Resources`).


> On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote:
> > src/slave/containerizer/containerizer.cpp, lines 228-229
> > 
> >
> > `.disk().source().type()` should be enough?

We need to check for `has_disk()` [done via `Resources::isRootDisk()`], 
`disk().has_source()` and then retrieve `disk().source().type()`. I think 
having a single function for this makes sense to me.


> On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote:
> > src/slave/containerizer/containerizer.hpp, lines 57-60
> > 
> >
> > I think we should avoid adding the `diskSize` in the definition header 
> > of `containerizer` because it looks irrelevant. Suppose we would like to 
> > modular the containerizer, it would make us in trouble as the TODO 
> > mentioned below.
> > 
> > ```
> >   // TODO(idownes): Consider making this non-static and moving to
> >   // containerizer implementations to enable a containerizer to best
> >   // determine the resources, particularly if containerizeration is
> >   // delegated.
> >   static Try resources(const Flags& flags);
> > ```
> > 
> > `fs.hpp`, `resources.cpp` or  `resources_utils.cpp` would be a better 
> > place?

Removed from the `Containerizer` class, and made it an internal helper function 
(`static` function).


> On Sept. 18, 2016, 6:22 p.m., haosdent huang wrote:
> > src/slave/containerizer/containerizer.cpp, line 169
> > 
> >
> > Use 0 to represent that determine the disk size automatically a bit 
> > nonintuitive.
> > Please update docs/attributes-resources.md if we have to use this way 
> > eventually.

Docs updated in https://reviews.apache.org/r/52071/.


- Anindya


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


On Sept. 18, 2016, 4:55 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51879/
> ---
> 
> (Updated Sept. 18, 2016, 4:55 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When static resources indicate disks with a positive size, we use that
> for the disk resources on the agent. However, --resources can include
> disks with size of 0, which indicates that mesos agent determine the
> size of those disks from the host and uses that information instead.
> Note that this is not allowed for PATH disks since PATH disks can be
> carved into smaller chunks and we cannot assume that the whole
> physical disk is available to the PATH disk.
> 
> With this change, JSON or textual representation for disk resources
> that specify scalar value of 0 would not result in an error, but those
> resources will not be accounted for until a valid size is determined
> for such resources. A scalar value of -1 in JSON or textual formats
> still results in an invalid resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> f13669d0dfc4ce3287cfe630cabd0470dc765b51 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
> 
> Diff: https://reviews.apache.org/r/51879/diff/
> 
> 
> Testing
> ---
> 
> Tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52002: Added helper methods to determine types of disk resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


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


Repository: mesos


Description
---

Added helper methods to determine types of disk resources.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 19, 2016, 4:55 p.m., haosdent huang wrote:
> > src/common/resources.cpp, lines 616-648
> > 
> >
> > This is conflict with range resources `ports:[21000-24000,3-34000]`.

I do not think so. `ports:[21000-24000,3-34000]` will have `key=ports` and 
`value=[21000-24000,3-34000]`. So this additional loop would not be entered 
since the `key` does not include the square brackets. Note that key is set 
based on number of tokens (2 in this specific case, and 3 when we include 
MOUNT/PATH disks, ie. `disk[MOUNT:/tmp]:2048`). The new logic is not applied on 
value, but only on key (which is whole resource excluding the value).


- Anindya


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


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

> On Sept. 19, 2016, 6:40 a.m., Guangya Liu wrote:
> > src/common/resources.cpp, line 611
> > 
> >
> > A unit test is needed to test the case when token size is 3

Tests for various cases of token sizes (2 and 3) are captured in 
https://reviews.apache.org/r/51880/


- Anindya


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


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52001/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Text based representation of disk resources can indicate source type
> (ie. PATH or MOUNT) and its root. This makes it closer to the JSON
> representation.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/52001/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:42 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


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


Repository: mesos


Description
---

During parsing of resources in the startup flags of mesos agent, all
the valid resources (including empty resources) are accumulated in a
vector of Resource protobufs.


Diffs (updated)
-

  include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
  include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/slave/containerizer/containerizer.cpp 
d46882baa904fd439bffb23c324828b777228f1c 
  src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 51999: Refactor parsing of resources to include all valid resources.

[Anindya Sinha]

> On Sept. 19, 2016, 6 a.m., Guangya Liu wrote:
> > src/slave/containerizer/containerizer.cpp, lines 64-71
> > 
> >
> > A question here: Does the `resources` here will include the zero value 
> > resource?
> > 
> > If so, what is the differencen of this code block and 
> > `Resources::parse()`? The `Resources::parse()` seems similar as here.

The original code returns a `Resources` which contains individual `Resource` 
objects, which removes any empty `Resource` objects 
(https://github.com/apache/mesos/blob/master/src/common/resources.cpp#L1610).
Since we need to know of empty `Resource` objects for this patch, we actually 
accumulate the `Resource` objects in a `vector` (which will contain 
any empty `Resource` object) which is used when disk resources are present (to 
check for scalar value of 0 for the auto detection logic). But for the rest of 
the resources, we do a `Resources resources = parsed.get();` which implicitly 
converts a `vector` to `Resources` which removes the empty `Resource` 
objects.


> On Sept. 19, 2016, 6 a.m., Guangya Liu wrote:
> > include/mesos/resources.hpp, line 200
> > 
> >
> > A unit test is needed for this.

Tests for `Containerizer::resources()` -> `Resources::parseText()` are added in 
https://reviews.apache.org/r/51880/


- Anindya


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


On Sept. 19, 2016, 5:01 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51999/
> ---
> 
> (Updated Sept. 19, 2016, 5:01 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6062
> https://issues.apache.org/jira/browse/MESOS-6062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> During parsing of resources in the startup flags of mesos agent, all
> the valid resources (including empty resources) are accumulated in a
> vector of Resource protobufs.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp 3ef8cacee529addc745b4aeb6398d7606c61b749 
>   include/mesos/v1/resources.hpp ef56b4960b103a3efd916fab64796aa334ba44c6 
>   src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
>   src/slave/containerizer/containerizer.cpp 
> d46882baa904fd439bffb23c324828b777228f1c 
>   src/tests/resources_tests.cpp 3e493007d6d1d8194d07035aaa1cde28dedf2b5a 
>   src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 
> 
> Diff: https://reviews.apache.org/r/51999/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52001: Allow text based disk resources to indicate source type and root.

[Anindya Sinha]

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

(Updated Sept. 19, 2016, 10:43 p.m.)


Review request for mesos and Jiang Yan Xu.


Changes
---

Minor updates based on review comments.


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


Repository: mesos


Description
---

Text based representation of disk resources can indicate source type
(ie. PATH or MOUNT) and its root. This makes it closer to the JSON
representation.


Diffs (updated)
-

  src/common/resources.cpp 4602bff22ec7ff77f069056085ad92bfa04595f3 
  src/v1/resources.cpp 7c4db44d4c31a8295cc16b0ce1ef5adf314e6ac6 

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


Testing
---


Thanks,

Anindya Sinha

Re: Review Request 52069: Moved `CHECK_NE` out of the offers loop.

[Vinod Kone]

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




src/cli/execute.cpp (line 384)


While it's true that this can only be done once, I would vote to keep it 
close to the `if (task.isSome())` loop because the CHECK just above it makes it 
clear that `taskGroup` is some in the `else` block.


- Vinod Kone


On Sept. 19, 2016, 10:34 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52069/
> ---
> 
> (Updated Sept. 19, 2016, 10:34 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We only need to `CHECK_NE(task.isSome(), taskGroup.isSome())` when
> get offers and there is no need to `CHECK_NE` for each offer.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 
> 
> Diff: https://reviews.apache.org/r/52069/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 52069: Moved `CHECK_NE` out of the offers loop.

[Guangya Liu]

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

Review request for mesos, Abhishek Dasgupta and Vinod Kone.


Repository: mesos


Description
---

We only need to `CHECK_NE(task.isSome(), taskGroup.isSome())` when
get offers and there is no need to `CHECK_NE` for each offer.


Diffs
-

  src/cli/execute.cpp f1806723b25c72839475769e85fd7cbe0126d67d 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Review Request 52070: Mentioned `reserved_resources_full` info on the agent in relevant docs.

[Jiang Yan Xu]

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

Review request for mesos, (Disabled_DoNotUse) Anindya Sinha and haosdent huang.


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


Repository: mesos


Description
---

Mentioned `reserved_resources_full` info on the agent in relevant docs.


Diffs
-

  docs/persistent-volume.md 4a5d5322d7d95cdbc4d1580ee1ca7a05b2ed9acb 
  docs/reservation.md 6408646cd42408514566e323beff548b69b6af41 

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


Testing
---

Looks fine in local markdown viewer.


Thanks,

Jiang Yan Xu

Re: Review Request 52042: Changed error message in pailer if user is unauthorized.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [52042]

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

Error:
2016-09-19 22:14:38 URL:https://reviews.apache.org/r/52042/diff/raw/ 
[1070/1070] -> "52042.patch" [1]
Traceback (most recent call last):
  File "support/apply-reviews.py", line 342, in 
reviewboard()
  File "support/apply-reviews.py", line 321, in reviewboard
apply_review()
  File "support/apply-reviews.py", line 139, in apply_review
commit_patch()
  File "support/apply-reviews.py", line 180, in commit_patch
data = patch_data()
  File "support/apply-reviews.py", line 199, in patch_data
return reviewboard_data()
  File "support/apply-reviews.py", line 258, in reviewboard_data
review_url=url)
UnicodeEncodeError: 'ascii' codec can't encode character u'\u2026' in position 
230: ordinal not in range(128)

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

- Mesos ReviewBot


On Sept. 19, 2016, 3:16 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52042/
> ---
> 
> (Updated Sept. 19, 2016, 3:16 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If attempts to use the pailer to access sandboxes and/or mesos logs fail
> because the user is unauthorized, an error telling the user he is
> unauthorized is shown to the client instead of showing the misguiding
> _Failed to initialize… retrying_. It also disables retry in case the
> client is unauthorized.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/jquery.pailer.js 
> 79c3c0dad59c2dde78913116bb08383f7a6c4d5e 
> 
> Diff: https://reviews.apache.org/r/52042/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> and manual tests with the following ACLs
> 
> ```json
> {
>   "permissive": true,
>   "access_mesos_logs" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "logs" : { "type" : "NONE" }
> }
>   ],
>   "access_sandboxes" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "users" : { "type" : "NONE" }
> }
>   ]
> }
> 
> ```
> 
> and credentials:
> 
> ```
> foo bar
> baz bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 52063: Fixed warnings in Windows `os.hpp`.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Sept. 19, 2016, 1:05 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52063/
> ---
> 
> (Updated Sept. 19, 2016, 1:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed warnings in Windows `os.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows/os.hpp 
> da458fb488d67d42b5742e93f0c5437d5ef32267 
> 
> Diff: https://reviews.apache.org/r/52063/diff/
> 
> 
> Testing
> ---
> 
> Windows: build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 52062: Fixed warnings in `numify.hpp`.

[Joseph Wu]

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




3rdparty/stout/include/stout/numify.hpp (lines 29 - 32)


Couldn't this be a static assert (failure) instead?  

It doesn't make sense to negate an unsigned value.


- Joseph Wu


On Sept. 19, 2016, 1:05 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52062/
> ---
> 
> (Updated Sept. 19, 2016, 1:05 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When the template is instatiated for unsigned scalars
> the unary negation generates warnings.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/numify.hpp 
> c174fcb8cb9d809f443e44058f07b58751bed9dd 
> 
> Diff: https://reviews.apache.org/r/52062/diff/
> 
> 
> Testing
> ---
> 
> Windows: build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 52061: Fixed warnings in `duration.hpp`.

[Joseph Wu]

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


Fix it, then Ship it!




I'll tweak the spacing before committing.


3rdparty/stout/include/stout/duration.hpp (lines 312 - 313)


Need 4 spaces for alignment after an open parentheses + newline.  And it 
would be good to keep the newline.


- Joseph Wu


On Sept. 19, 2016, 1:04 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52061/
> ---
> 
> (Updated Sept. 19, 2016, 1:04 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed warnings in `duration.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/duration.hpp 
> cd4f205bbfbc4c2647db52fe72f44bd5c8f1c44c 
> 
> Diff: https://reviews.apache.org/r/52061/diff/
> 
> 
> Testing
> ---
> 
> Windows: build
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51870: Added a test for reservation info in the agent's endpoint.

[Anindya Sinha]

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


Ship it!




Ship It!

- Anindya Sinha


On Sept. 19, 2016, 8:48 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51870/
> ---
> 
> (Updated Sept. 19, 2016, 8:48 p.m.)
> 
> 
> Review request for mesos, Anindya Sinha and Neil Conway.
> 
> 
> Bugs: MESOS-6085
> https://issues.apache.org/jira/browse/MESOS-6085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for reservation info from the agent's endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/reservation_endpoints_tests.cpp 
> 28b497eccd5fadd03dc24e73e11e59ce36d78d32 
> 
> Diff: https://reviews.apache.org/r/51870/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 52048: Fixed OsTest.User test failure due to gids ordering.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52048]

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 Sept. 19, 2016, 6:42 p.m., Mao Geng wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52048/
> ---
> 
> (Updated Sept. 19, 2016, 6:42 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kapil Arya.
> 
> 
> Bugs: MESOS-5909
> https://issues.apache.org/jira/browse/MESOS-5909
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed OsTest.User test failure due to gids ordering.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/tests/os_tests.cpp c2900b8bf25b06a498c32c81bd2f8c852eb682f5 
> 
> Diff: https://reviews.apache.org/r/52048/diff/
> 
> 
> Testing
> ---
> 
> Just changed the OsTest.User test. `make check` passed on Ubuntu 12.04.5 LTS.
> 
> 
> Thanks,
> 
> Mao Geng
> 
>

Re: Review Request 51984: Cleaned up initialization of atomic fields in ProcessManager.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Sept. 17, 2016, 8:25 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51984/
> ---
> 
> (Updated Sept. 17, 2016, 8:25 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `std::atomic` has a trivial default constructor, which means
> `std::atomic` values are left uninitialized by default. `ProcessManager`
> uses two atomic fields that are default-constructed; although we `store`
> to them before reading from them, it would be cleaner and safer to
> initialize them to a well-defined value in the first place.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 1e48fd5269d1a94c2217e8826af54b9b42ec4b23 
> 
> Diff: https://reviews.apache.org/r/51984/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51958: Removed use of "--registry_strict" master flag.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 17, 2016, 2:23 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51958/
> ---
> 
> (Updated Sept. 17, 2016, 2:23 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5951
> https://issues.apache.org/jira/browse/MESOS-5951
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The flag will remain for a few more months per the deprecation policy,
> but we no longer need to set it in the unit tests. Providing an
> extensive usage summary in "--help" output also doesn't seem useful.
> 
> 
> Diffs
> -
> 
>   src/master/flags.cpp 19ae6c1c30a1054b64a9585f325bd0bf943af218 
>   src/tests/cluster.cpp b04653af97d17aaa9d0d3ee872169b66cd67126b 
>   src/tests/mesos.cpp 07a64f0ff49a753ec26260cdf859d0584c3f935a 
> 
> Diff: https://reviews.apache.org/r/51958/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX and Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51870: Added a test for reservation info in the agent's endpoint.

[Jiang Yan Xu]

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

(Updated Sept. 19, 2016, 1:48 p.m.)


Review request for mesos, Anindya Sinha and Neil Conway.


Changes
---

Rebased. Added expectations for "unreserved_resources".


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


Repository: mesos


Description
---

Added a test for reservation info from the agent's endpoint.


Diffs (updated)
-

  src/tests/reservation_endpoints_tests.cpp 
28b497eccd5fadd03dc24e73e11e59ce36d78d32 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 52064: Support for multiple versions of docs.

[Tim Anderegg]

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

(Updated Sept. 19, 2016, 8:43 p.m.)


Review request for mesos and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Support for multiple versions of docs.  The approach I took was to use the 
"git" ruby library to iterate over each tagged version of Mesos, and generate 
the documentation.  This uses the "releases.yml" file to determine which 
versions should build documentation.  The same is done for images, since they 
may change between versions.  A select box was added to the website's 
breadcrumbs bar that allows the user to easily switch between documentation 
versions.  If a page is reached that doesn't exist in an older version of the 
documentation, the user is notified and redirect to the main page for that 
version.


Diffs
-

  site/Gemfile 4dcd0a5ad452085e6e3bb152a6547575636cd807 
  site/Gemfile.lock 1393246f06631155bb052669a86e431778e891bf 
  site/Rakefile 01356891c29f9e69fa0f7813cf87e7662eda400b 
  site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
  site/config.rb 9e6738fccc48365e16221188269c1ed40772eca7 
  site/data/releases.yml 1e9bb8555d266658baaf37c4b608eebeb0d14da8 
  site/source/assets/js/versions.js PRE-CREATION 
  site/source/layouts/basic.erb 7464e40b619e883daad93c72c3fbdbfbdda8f152 
  site/source/layouts/documentation.erb 
a91f916a5fb7348b2702c272e7a2059bdf628c66 
  site/source/layouts/gettingstarted.erb PRE-CREATION 

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


Testing
---

Testing was done manually to verify that the documentation was built for each 
version of Mesos that is supported (some older versions do not have compatible 
documentation).


Thanks,

Tim Anderegg

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Joseph Wu]

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

(Updated Sept. 19, 2016, 1:40 p.m.)


Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
Harutyunyan, and Joris Van Remoortere.


Repository: mesos


Description
---

This updates the CHANGELOG to reflect:
  * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
  * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.


Diffs
-

  CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 

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


Testing (updated)
---

Ran the 0.27.x, 0.28.x, and 1.0.x branches + backports through an internal CI...
... and results came back as expected (passing except for some known flaky 
tests in 0.27.x and 0.28.x).


Thanks,

Joseph Wu

Re: Review Request 50857: Modified a scheduler test to run with SSL enabled.

[Greg Mann]

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

(Updated Sept. 19, 2016, 8:23 p.m.)


Review request for mesos, Joseph Wu and Vinod Kone.


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


Repository: mesos


Description
---

This patch modifies the test `SchedulerTest.Teardown` to
be parametrized by both `ContentType` and SSL configuration,
and renames it to `SchedulerSSLTest.RunTaskAndTeardown`.
This allows the test to verify the scheduler's behavior with
SSL both enabled and disabled.


Diffs
-

  src/tests/scheduler_tests.cpp b0ea0bbcce9d847285fda40f778caaf721804457 

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


Testing (updated)
---

The test was run in repetition as follows:

`GTEST_REPEAT=-1 GTEST_BREAK_ON_FAILURE=1 
GTEST_FILTER="ContentTypeAndSSLConfig/SchedulerSSLTest.RunTaskAndTeardown*" 
bin/mesos-tests.sh`


Thanks,

Greg Mann

Review Request 52064: Support for multiple versions of docs.

[Tim Anderegg]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Support for multiple versions of docs.


Diffs
-

  site/Gemfile 4dcd0a5ad452085e6e3bb152a6547575636cd807 
  site/Gemfile.lock 1393246f06631155bb052669a86e431778e891bf 
  site/Rakefile 01356891c29f9e69fa0f7813cf87e7662eda400b 
  site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
  site/config.rb 9e6738fccc48365e16221188269c1ed40772eca7 
  site/data/releases.yml 1e9bb8555d266658baaf37c4b608eebeb0d14da8 
  site/source/assets/js/versions.js PRE-CREATION 
  site/source/layouts/basic.erb 7464e40b619e883daad93c72c3fbdbfbdda8f152 
  site/source/layouts/documentation.erb 
a91f916a5fb7348b2702c272e7a2059bdf628c66 
  site/source/layouts/gettingstarted.erb PRE-CREATION 

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


Testing
---

Testing was done manually to verify that the documentation was built for each 
version of Mesos that is supported (some older versions do not have compatible 
documentation).


Thanks,

Tim Anderegg

Review Request 52062: Fixed warnings in `numify.hpp`.

[Daniel Pravat]

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

When the template is instatiated for unsigned scalars
the unary negation generates warnings.


Diffs
-

  3rdparty/stout/include/stout/numify.hpp 
c174fcb8cb9d809f443e44058f07b58751bed9dd 

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


Testing
---

Windows: build


Thanks,

Daniel Pravat

Review Request 52063: Fixed warnings in Windows `os.hpp`.

[Daniel Pravat]

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Fixed warnings in Windows `os.hpp`.


Diffs
-

  3rdparty/stout/include/stout/windows/os.hpp 
da458fb488d67d42b5742e93f0c5437d5ef32267 

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


Testing
---

Windows: build


Thanks,

Daniel Pravat

Review Request 52065: Fixed warnings in Windows `shell.hpp`.

[Daniel Pravat]

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Fixed warnings in Windows `shell.hpp`.


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
8fb48c1c4178485cac2934833b5c440d873e4fc4 

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


Testing
---

Windows: build


Thanks,

Daniel Pravat

Review Request 52061: Fixed warnings in `duration.hpp`.

[Daniel Pravat]

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

Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Fixed warnings in `duration.hpp`.


Diffs
-

  3rdparty/stout/include/stout/duration.hpp 
cd4f205bbfbc4c2647db52fe72f44bd5c8f1c44c 

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


Testing
---

Windows: build


Thanks,

Daniel Pravat

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Abhishek Dasgupta]

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

(Updated Sept. 19, 2016, 7:45 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


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


Repository: mesos


Description
---

Through this patch, mesos-execute supports running
task groups. Single task provided through command
line option is supported as well for backward
compatibility.


Diffs (updated)
-

  src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 

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


Testing
---

On Ubuntu 16.04:
sudo make -j4

and manually ran this command:
**Successful**
mesos-execute --master=127.0.0.1:5050 
--taskgroup=file:///home/abhishek/taskgroup.txt

`/home/abhishek/taskgroup.txt:`
{
"tasks": [{
"name": "sigmtdroid",
"task_id": {
"value": "sigmoid"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 1
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}, {
"name": "sigmteroid",
"task_id": {
"value": "sigmoid2"
},
"agent_id": {
"value": ""
},
"resources": [{
"name": "cpus",
"type": "SCALAR",
"scalar": {
"value": 2
},
"role": "*"
}, {
"name": "mem",
"type": "SCALAR",
"scalar": {
"value": 32
},
"role": "*"
}],
"command": {
"value": "sleep 1000"
},
"volumes": [{
"container_path": "/mnt/volume",
"mode": "RW",
"source": {
"docker_volume": {
"driver": "volume_driver",
"docker_options": {
"parameter": [{
"key": "key",
"value": "value"
}]
},
"name": "volume_name"
},
"type": "DOCKER_VOLUME"
}
}]
}]
}


Thanks,

Abhishek Dasgupta

Review Request 52059: Added test for running the logrotate module as non-root.

[Joseph Wu]

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

Review request for mesos, Artem Harutyunyan and Sivaram Kannan.


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


Repository: mesos


Description
---

This test creates a non-privileged user and but runs the agent
as root.  Logrotate has several branches of logic that are only
exercised as root.  In this case, we expect logrotate to work
if the agent and module are root, but the task is non-root.


Diffs
-

  src/tests/container_logger_tests.cpp 1b121a2fcce2d874aeefc4257b9d4e594866e78d 

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


Testing
---

make check

The new test currently fails (as expected).


Thanks,

Joseph Wu

Re: Review Request 52039: Avoided using SIGUSR1 in two test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [50235, 50416, 50417, 50418, 50422, 50699, 50700, 50701, 
50702, 50703, 50704, 50705, 50706, 50707, 50844, 50845, 50846, 51020, 51371, 
51374, 51375, 51376, 51377, 51021, 51706, 51653, 51707, 51805, 51845, 51891, 
51909, 51913, 51953, 51954, 51955, 51956, 51957, 51958, 52039]

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 Sept. 19, 2016, 2:54 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52039/
> ---
> 
> (Updated Sept. 19, 2016, 2:54 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We want to cause the agent to shutdown gracefully (i.e., to send an
> "unregister" message to the master). This can be accomplished by sending
> the whole process a SIGUSR1 but that seems fragile; using the agent's
> `shutdown()` method seems more robust.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 696e0f71190238133f29b6380bc58b994a556e69 
> 
> Diff: https://reviews.apache.org/r/52039/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 15, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joseph Wu]

> On Sept. 19, 2016, 12:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.
> 
> Till Toenshoff wrote:
> I agree, those are confusing. On a second thought, shall we simply remove 
> those error-code-numbers? I was trying to follow the initial example but now 
> feel we should complete get rid of those as we can expect the error string to 
> be helpful.

I think the error codes might be useful for debugging.  I'm guessing the error 
strings may change between OpenSSL versions (I wonder if there's localization 
too), but the error codes will not.


- Joseph


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


On Sept. 19, 2016, 6:13 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 51921: Added backports for a potential double-close in libevent sockets.

[Vinod Kone]

> On Sept. 15, 2016, 5:56 p.m., Joris Van Remoortere wrote:
> > Did you want to fold the minor fix into your original patch to backport?
> 
> Joseph Wu wrote:
> Do you mean MESOS-6152?  (i.e. `s/fd/owned_fd`)
> 
> I kept the cherry-picks un-squashed to retain history.
> 
> Joris Van Remoortere wrote:
> Yeah. Maybe Vinod / BenM can chime in regarding squash vs. retain history.

yea, lets not squash backports.


- Vinod


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


On Sept. 15, 2016, 5:26 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51921/
> ---
> 
> (Updated Sept. 15, 2016, 5:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, Greg Mann, Artem 
> Harutyunyan, and Joris Van Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This updates the CHANGELOG to reflect:
>   * Backporting MESOS-5986 to 0.27.4 and 0.28.3.
>   * Backporting MESOS-6104 and MESOS-6152 to 0.27.4, 0.28.3, and 1.0.2.
> 
> 
> Diffs
> -
> 
>   CHANGELOG ef31563e3f4a2ceda598a0ddcc7ad325f64a8538 
> 
> Diff: https://reviews.apache.org/r/51921/diff/
> 
> 
> Testing
> ---
> 
> TODO: Running the 0.27.x, 0.28.x, and 1.0.x branches + backports through an 
> internal CI...
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 51985: Waited for agent finish registering in test case.

[Joseph Wu]

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




src/tests/slave_recovery_tests.cpp (lines 4073 - 4081)


This change doesn't seem necessary, as waiting for an offer includes 
waiting for the agent to (re)register.

Ditto for the below `AWAIT`s.


- Joseph Wu


On Sept. 17, 2016, 8:41 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51985/
> ---
> 
> (Updated Sept. 17, 2016, 8:41 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Greg Mann, Jie Yu, Joseph Wu, and 
> Qian Zhang.
> 
> 
> Bugs: MESOS-6180
> https://issues.apache.org/jira/browse/MESOS-6180
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Waited for agent finish registering in test case.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_recovery_tests.cpp 23cf9d8cd76d568fa841817d6f4279c4aa2fdc75 
> 
> Diff: https://reviews.apache.org/r/51985/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 52058: Fixed fetcher to not call chown on sandbox again.

[Megha Sharma]

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

Review request for mesos.


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


Repository: mesos


Description
---

Fetcher code changes the ownership of entire sandbox directory
recursively to the taskuser and as a resut also changes the
ownership of files laid out by other entities in the sandbox.


Diffs
-

  src/slave/containerizer/fetcher.cpp 4d3fc521bf8a7c438c48e31c549f108ac3602b3f 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
96e24500a12825161553eb050da389088b122695 

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


Testing
---

make check on linux


Thanks,

Megha Sharma

Re: Review Request 52054: Updated slave test to expect 500 rather than 503 from libprocess.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp (line 1919)


should we call this out in the CHANGELOG? best to create a ticket to link 
the reviews.


- Vinod Kone


On Sept. 19, 2016, 7:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52054/
> ---
> 
> (Updated Sept. 19, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously HTTP handler failures lead to a 'Service Unavailble'
> response in libprocess. Now it leads to an 'Internal Server Error'.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 2f3fa5fae634b6250f3c00bbef3077493f79af95 
> 
> Diff: https://reviews.apache.org/r/52054/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52053: Return 500 rather than 503 when response handlers fail.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 19, 2016, 7:15 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52053/
> ---
> 
> (Updated Sept. 19, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Libprocess was returning 'Service Unavailable' when an HTTP handler
> Future fails. More appropriate would be an 'Internal Server Error'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> a6fbf92bd6d69d94490b9faf4960f14aa112e8d2 
>   3rdparty/libprocess/src/tests/http_tests.cpp 
> 24b266df5f17e28e0c95326f5d1ea451952500e8 
> 
> Diff: https://reviews.apache.org/r/52053/diff/
> 
> 
> Testing
> ---
> 
> Added a test.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 51623: Updated mesos-execute to support task groups.

[Vinod Kone]

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


Ship it!




LGTM module "taskGroup" flag name to "task_group" per guangya's

- Vinod Kone


On Sept. 19, 2016, 6:54 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51623/
> ---
> 
> (Updated Sept. 19, 2016, 6:54 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Through this patch, mesos-execute supports running
> task groups. Single task provided through command
> line option is supported as well for backward
> compatibility.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51623/diff/
> 
> 
> Testing
> ---
> 
> On Ubuntu 16.04:
> sudo make -j4
> 
> and manually ran this command:
> **Successful**
> mesos-execute --master=127.0.0.1:5050 
> --taskgroup=file:///home/abhishek/taskgroup.txt
> 
> `/home/abhishek/taskgroup.txt:`
> {
>   "tasks": [{
>   "name": "sigmtdroid",
>   "task_id": {
>   "value": "sigmoid"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 1
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }, {
>   "name": "sigmteroid",
>   "task_id": {
>   "value": "sigmoid2"
>   },
>   "agent_id": {
>   "value": ""
>   },
>   "resources": [{
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 2
>   },
>   "role": "*"
>   }, {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
>   "value": 32
>   },
>   "role": "*"
>   }],
>   "command": {
>   "value": "sleep 1000"
>   },
>   "volumes": [{
>   "container_path": "/mnt/volume",
>   "mode": "RW",
>   "source": {
>   "docker_volume": {
>   "driver": "volume_driver",
>   "docker_options": {
>   "parameter": [{
>   "key": "key",
>   "value": "value"
>   }]
>   },
>   "name": "volume_name"
>   },
>   "type": "DOCKER_VOLUME"
>   }
>   }]
>   }]
> }
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Review Request 52054: Updated slave test to expect 500 rather than 503 from libprocess.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Previously HTTP handler failures lead to a 'Service Unavailble'
response in libprocess. Now it leads to an 'Internal Server Error'.


Diffs
-

  src/tests/slave_tests.cpp 2f3fa5fae634b6250f3c00bbef3077493f79af95 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Review Request 52053: Return 500 rather than 503 when response handlers fail.

[Benjamin Mahler]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Libprocess was returning 'Service Unavailable' when an HTTP handler
Future fails. More appropriate would be an 'Internal Server Error'.


Diffs
-

  3rdparty/libprocess/src/process.cpp a6fbf92bd6d69d94490b9faf4960f14aa112e8d2 
  3rdparty/libprocess/src/tests/http_tests.cpp 
24b266df5f17e28e0c95326f5d1ea451952500e8 

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


Testing
---

Added a test.


Thanks,

Benjamin Mahler

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Till Toenshoff]

> On Sept. 19, 2016, 7:02 p.m., Joseph Wu wrote:
> > 3rdparty/libprocess/src/openssl.cpp, lines 509-510
> > 
> >
> > I'd prefer the parentheses containing the `stringify(error)` to mention 
> > OpenSSL, since the number of ambiguous/confusing otherwise.  
> > 
> > i.e. `(OpenSSL error #___): ...`
> > 
> > Ditto other error messages.

I agree, those are confusing. On a second thought, shall we simply remove those 
error-code-numbers? I was trying to follow the initial example but now feel we 
should complete get rid of those as we can expect the error string to be 
helpful.


- Till


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


On Sept. 19, 2016, 1:13 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 1:13 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Jie Yu]

> On Sept. 19, 2016, 9:24 a.m., Jan Schlicht wrote:
> > src/master/validation.cpp, lines 63-78
> > 
> >
> > This will cause problems with tasks that don't use a container image: 
> > While `ContainerInfo.type` is required, the `DockerInfo` or `MesosInfo` 
> > fields are not. If a user doesn't want to use a container image, he will 
> > leave these fields empty and probably set the type to `MESOS`.
> > To fix this, the validation should check if the right type is set, when 
> > an image is specified. Like
> > ```
> >   if (container.has_mesos() && container.type() == 
> > ContainerInfo::MESOS) {
> > return Error("Expecting 'mesos' to be set for mesos container");
> >   }
> > ```
> 
> Jan Schlicht wrote:
> Oops, mistake in the example, should be
> ```
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS) {
>   return Error("Expecting 'mesos' to be set for mesos container");
> }
> ```
> 
> Alexander Rukletsov wrote:
> The "union trick" we use in `ContainerInfo` protobuf 
> (https://github.com/apache/mesos/blob/7a5ec49877a2e1be5b053a6b59ed6f32760fbe7a/include/mesos/mesos.proto#L2058)
>  does not encourage setting type without setting the corresponding field.
> 
> You say that something like this task definition is valid and possible:
> ```
> {
>   "name": "cni-test",
>   "task_id": "task01",
>   "slave_id": 
>  …
>   "container": {
> "type": "mesos",
> "network_infos" : [
>   { "name": "network-a" }
> ]
>   },
>   "command" : {
> "value": "foo"
>  }
> ```
> 
> I would argue, that setting type to `mesos` is misleading. If we allow 
> such task definitions, we should probably introduce another type, e.g. `NONE`.
> 
> Jie Yu wrote:
> This is rather unfortunate and was introduced long time ago. I think we 
> cannot apply this check as it will break existing users.
> 
> I think 'container.type()' is to tell which containerizer to use. We put 
> all the common fields in ContainerInfo. YOu can think of 'docker' and 'mesos' 
> fields contain additional information if needed, but they are optional.
> 
> I think we should check:
> ```
> if (container.has_docker() && container.type() != ContainerInfo::DOCKER)
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS)
> ```
> 
> We should add more documentation here. AlexR, can you own this?
> 
> Alexander Rukletsov wrote:
> > I think 'container.type()' is to tell which containerizer to use.
> 
> But what if we omit `ContainerInfo` altogether? In this case, we do not 
> explicitly tell what containerizer to use and it is deduced automagically.
> 
> I'm happy to drive the doc update, however I would need some help in 
> understanding what the options are : )

> But what if we omit ContainerInfo altogether? In this case, we do not 
> explicitly tell what containerizer to use and it is deduced automagically.

If ContainerInfo is not set, we assume it'll be handled by MesosContainerizer. 
It's like saying the default container runtime is MesosContainerizer.


- Jie


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


On Sept. 16, 2016, 10:19 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51865/
> ---
> 
> (Updated Sept. 16, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6157
> https://issues.apache.org/jira/browse/MESOS-6157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
>   src/tests/container_logger_tests.cpp 
> 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 0d611c196870b6adabea52a48abcd344c8dad5d1 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 1671d45171307cda62184505ce1dbadc476abca6 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> df4642d2667407b1758ffe2efcfdbf9968cf2c33 
>   src/tests/containerizer/isolator_tests.cpp 
> 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/slave_tests.cpp b44b6fc5627ad97a33be151cb21133da57f3efd8 
> 
> Diff: https://reviews.apache.org/r/51865/diff/
> 
> 
> Testing
> ---
> 
> make check on 

Re: Review Request 52033: Escalated some openssl logs from VLOG to INFO.

[Joseph Wu]

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


Ship it!




This isn't noisy since it only happens once.  Might be somewhat noisy in tests, 
but that's OK.

- Joseph Wu


On Sept. 19, 2016, 6:50 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52033/
> ---
> 
> (Updated Sept. 19, 2016, 6:50 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The logging messages in question prove to be very useful for debugging the
> cluster setup and hence we decided they should be generally available as their
> noise by far outwages their helpfulnes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52033/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 51990: Added << operator to stringify v1::TaskGroupInfo protobuf.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 18, 2016, 9:08 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51990/
> ---
> 
> (Updated Sept. 18, 2016, 9:08 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is needed as Flags add() function needs to stringify
> v1::TaskGroupInfo before it is parsed to a variable of that
> type.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.hpp 936af77f95ccbdc333452a1946187a81222e480f 
>   src/v1/mesos.cpp c6a8799648af488d11a48e0c8841b98313c7dff1 
> 
> Diff: https://reviews.apache.org/r/51990/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 51978: Refactored commandScheduler in cli/execute.cpp to take TaskInfo.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Sept. 19, 2016, 6:15 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51978/
> ---
> 
> (Updated Sept. 19, 2016, 6:15 a.m.)
> 
> 
> Review request for mesos, Guangya Liu and Vinod Kone.
> 
> 
> Bugs: MESOS-6096
> https://issues.apache.org/jira/browse/MESOS-6096
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This refactoring was needed for better accomodating pod support
> for mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 525c89803ad1b29328420c5925a3e6045e487645 
> 
> Diff: https://reviews.apache.org/r/51978/diff/
> 
> 
> Testing
> ---
> 
> Manually Ran:
> mesos-execute --master=127.0.0.1:5050 --name="Hello1" --command="echo hello" 
> --docker_image="ubuntu" --containerizer="docker"
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 52031: Added openssl error string output to initializing failures.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/libprocess/src/openssl.cpp (lines 467 - 468)


And if you decide to add that error code label (see below comment)... this 
is the only place that has the same pattern.



3rdparty/libprocess/src/openssl.cpp (lines 509 - 510)


I'd prefer the parentheses containing the `stringify(error)` to mention 
OpenSSL, since the number of ambiguous/confusing otherwise.  

i.e. `(OpenSSL error #___): ...`

Ditto other error messages.


- Joseph Wu


On Sept. 19, 2016, 6:13 a.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52031/
> ---
> 
> (Updated Sept. 19, 2016, 6:13 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Joseph Wu.
> 
> 
> Bugs: MESOS-5320
> https://issues.apache.org/jira/browse/MESOS-5320
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the human readable openssl error messages for failure cases. Also
> fixes a spacing nit in one of the existing messages.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> c09cdc89509e4e4ca4c8a0f4fb0a57156a3a6091 
> 
> Diff: https://reviews.apache.org/r/52031/diff/
> 
> 
> Testing
> ---
> 
> make check && functional testing
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Re: Review Request 51804: Fixed typos in subprocess_base.hpp.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 19, 2016, 6:48 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51804/
> ---
> 
> (Updated Sept. 19, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typos in subprocess_base.hpp.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> acd23c99162f164c02a7781abeb95b3c220ead12 
> 
> Diff: https://reviews.apache.org/r/51804/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 51786: Fixed Typo in subprocess.hpp.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Sept. 11, 2016, 3:17 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51786/
> ---
> 
> (Updated Sept. 11, 2016, 3:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Typo in subprocess.hpp.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp 
> 57f71d15e6fbb032c8b6c5d0c73a93751022e7e7 
> 
> Diff: https://reviews.apache.org/r/51786/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 51065: Changed hostname used for SSL cert creation in tests.

[Greg Mann]

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

(Updated Sept. 19, 2016, 6:47 p.m.)


Review request for mesos, Joseph Wu and Vinod Kone.


Changes
---

Rebase


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


Repository: mesos


Description
---

This changes the SSL helpers in libprocess to generate
certs using a hostname determined using the same address
that is advertised by libprocess. This assures that
validation of the certificates will succeed. The test
`SSLTest.BasicSameProcess` is also updated to accommodate
this change.


Diffs (updated)
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 
2ad623a5ea3b3286983e895010af756f14f51b64 
  3rdparty/libprocess/src/tests/ssl_tests.cpp 
a5ac03969694c4345c2c5004051b08b6d1da8555 

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


Testing
---

`make check` on Ubuntu 14.04


Thanks,

Greg Mann

Re: Review Request 51804: Fixed typos in subprocess_base.hpp.

[Joerg Schad]

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

(Updated Sept. 19, 2016, 6:48 p.m.)


Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Fixed typos in subprocess_base.hpp.


Diffs
-

  3rdparty/libprocess/include/process/subprocess_base.hpp 
acd23c99162f164c02a7781abeb95b3c220ead12 

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


Testing
---


Thanks,

Joerg Schad

Re: Review Request 51865: Added validation for `ContainerInfo`.

[Alexander Rukletsov]

> On Sept. 19, 2016, 9:24 a.m., Jan Schlicht wrote:
> > src/master/validation.cpp, lines 63-78
> > 
> >
> > This will cause problems with tasks that don't use a container image: 
> > While `ContainerInfo.type` is required, the `DockerInfo` or `MesosInfo` 
> > fields are not. If a user doesn't want to use a container image, he will 
> > leave these fields empty and probably set the type to `MESOS`.
> > To fix this, the validation should check if the right type is set, when 
> > an image is specified. Like
> > ```
> >   if (container.has_mesos() && container.type() == 
> > ContainerInfo::MESOS) {
> > return Error("Expecting 'mesos' to be set for mesos container");
> >   }
> > ```
> 
> Jan Schlicht wrote:
> Oops, mistake in the example, should be
> ```
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS) {
>   return Error("Expecting 'mesos' to be set for mesos container");
> }
> ```
> 
> Alexander Rukletsov wrote:
> The "union trick" we use in `ContainerInfo` protobuf 
> (https://github.com/apache/mesos/blob/7a5ec49877a2e1be5b053a6b59ed6f32760fbe7a/include/mesos/mesos.proto#L2058)
>  does not encourage setting type without setting the corresponding field.
> 
> You say that something like this task definition is valid and possible:
> ```
> {
>   "name": "cni-test",
>   "task_id": "task01",
>   "slave_id": 
>  …
>   "container": {
> "type": "mesos",
> "network_infos" : [
>   { "name": "network-a" }
> ]
>   },
>   "command" : {
> "value": "foo"
>  }
> ```
> 
> I would argue, that setting type to `mesos` is misleading. If we allow 
> such task definitions, we should probably introduce another type, e.g. `NONE`.
> 
> Jie Yu wrote:
> This is rather unfortunate and was introduced long time ago. I think we 
> cannot apply this check as it will break existing users.
> 
> I think 'container.type()' is to tell which containerizer to use. We put 
> all the common fields in ContainerInfo. YOu can think of 'docker' and 'mesos' 
> fields contain additional information if needed, but they are optional.
> 
> I think we should check:
> ```
> if (container.has_docker() && container.type() != ContainerInfo::DOCKER)
> if (container.has_mesos() && container.type() != ContainerInfo::MESOS)
> ```
> 
> We should add more documentation here. AlexR, can you own this?

> I think 'container.type()' is to tell which containerizer to use.

But what if we omit `ContainerInfo` altogether? In this case, we do not 
explicitly tell what containerizer to use and it is deduced automagically.

I'm happy to drive the doc update, however I would need some help in 
understanding what the options are : )


- Alexander


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


On Sept. 16, 2016, 10:19 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51865/
> ---
> 
> (Updated Sept. 16, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-6157
> https://issues.apache.org/jira/browse/MESOS-6157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp aa743d730bebebc0353fa0a7a31f68bc94e7e25d 
>   src/tests/container_logger_tests.cpp 
> 1b121a2fcce2d874aeefc4257b9d4e594866e78d 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 0d611c196870b6adabea52a48abcd344c8dad5d1 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 1671d45171307cda62184505ce1dbadc476abca6 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> ca7bffd3b1773a11a4679d114885d3edd977b02b 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> df4642d2667407b1758ffe2efcfdbf9968cf2c33 
>   src/tests/containerizer/isolator_tests.cpp 
> 9bb1e69209f34b18b5b64c3daf5ea26780f2ab74 
>   src/tests/master_validation_tests.cpp 
> 16c5773aa44016f923e00cb348ded6b8c46d4b4b 
>   src/tests/slave_tests.cpp b44b6fc5627ad97a33be151cb21133da57f3efd8 
> 
> Diff: https://reviews.apache.org/r/51865/diff/
> 
> 
> Testing
> ---
> 
> make check on Mac OS and Linux.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 52048: Fixed OsTest.User test failure due to gids ordering.

[Mao Geng]

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

(Updated Sept. 19, 2016, 6:42 p.m.)


Review request for mesos, Gilbert Song and Kapil Arya.


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


Repository: mesos


Description
---

Fixed OsTest.User test failure due to gids ordering.


Diffs
-

  3rdparty/stout/tests/os_tests.cpp c2900b8bf25b06a498c32c81bd2f8c852eb682f5 

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


Testing (updated)
---

Just changed the OsTest.User test. `make check` passed on Ubuntu 12.04.5 LTS.


Thanks,

Mao Geng

Re: Review Request 52048: Fixed OsTest.User test failure due to gids ordering.

[Mao Geng]

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

(Updated Sept. 19, 2016, 6:41 p.m.)


Review request for mesos, Gilbert Song and Kapil Arya.


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


Repository: mesos


Description
---

Fixed OsTest.User test failure due to gids ordering.


Diffs
-

  3rdparty/stout/tests/os_tests.cpp c2900b8bf25b06a498c32c81bd2f8c852eb682f5 

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


Testing
---

Just changed the OsTest.User test. `make check` passed


Thanks,

Mao Geng

Re: Review Request 52020: Fixed indent in allocator.

[Joseph Wu]

> On Sept. 19, 2016, 6:20 a.m., Alexander Rukletsov wrote:
> > src/master/allocator/mesos/hierarchical.cpp, line 1653
> > 
> >
> > This looks fine: 4 spaces after '('.
> 
> Guangya Liu wrote:
> Does the indent always start from begining? 
> 
> 1) 
> https://github.com/apache/mesos/blob/master/src/cli/execute.cpp#L645-L646
> 2) 
> https://github.com/apache/mesos/blob/master/docs/c%2B%2B-style-guide.md#function-definitioninvocation

```
if (!remainingClusterResources.contains(
```
In this case, the code should be aligned based on the "`.contains(`", rather 
than on the "`if (`".  So the existing alignment is alright.

---

You may see "`if (`" alignment in cases like:
```
if (really_really_really_long_boolean_expression &&
other_really_long_boolean_expression)
```


- Joseph


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


On Sept. 18, 2016, 6:59 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52020/
> ---
> 
> (Updated Sept. 18, 2016, 6:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indent in allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 2d56bd011f2c87c67a02d0ae467a4a537d36867e 
> 
> Diff: https://reviews.apache.org/r/52020/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51784, 51930, 51931, 50271]

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 Sept. 19, 2016, 2:21 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50271/
> ---
> 
> (Updated Sept. 19, 2016, 2:21 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Jie Yu.
> 
> 
> Bugs: MESOS-5275
> https://issues.apache.org/jira/browse/MESOS-5275
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This isolator evaluates agent allowed capabilities and passes
> net capabilities on to `mesos-containerizer` which enforces the
> capabilities.
> 
> Capability information is passed from the isolator via a new
> capability field in `ContainerLaunchInfo`.
> 
> 
> Diffs
> -
> 
>   include/mesos/slave/containerizer.proto 
> 20db010ea158a813034b41ce9cddac7d8317 
>   src/CMakeLists.txt 42c52b60cc850901f2eff1545cf7900f4a65ca81 
>   src/Makefile.am 6fb095f58cf943c5597175df695046cfa21b68fd 
>   src/slave/containerizer/mesos/containerizer.cpp 
> e54169ba00f6e0cdd7043075b4cdd12d714c99e3 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/linux/capabilities.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/launch.cpp 
> fc51e04ec1572679e6a48ff4f0fa31ef2dfd6ec3 
>   src/tests/containerizer/isolator_tests.cpp 
> 93ce75180520d382f63ce7323be844fe43c6594e 
> 
> Diff: https://reviews.apache.org/r/50271/diff/
> 
> 
> Testing
> ---
> 
> `make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o 
> optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>