Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Yubo Li]

> On 十月 14, 2016, 3:03 a.m., Guangya Liu wrote:
> > src/docker/docker.cpp, lines 397-398
> > 
> >
> > ```
> > if (device.isError()) {
> >   return Error("Failed to parse device from HostConfig.Devices entry"
> >" due to: " + device.error());
> > }
> > ```
> > 
> > I think that we should also quota `HostConfig.Devices` as 
> > `'HostConfig.Devices'`, but seems other log messages also do not include 
> > quota, I think that it is OK to keep consistent here.
> > 
> > But the ideal style is as 
> > https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L410

I see. I think we would not make change here to keep consistent.


- Yubo


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


On 十月 13, 2016, 10:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated 十月 13, 2016, 10:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped helper functions to 'Docker::Device' to handle data
> parsing and serializing between 'Docker::Device' structure and
> string.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Avinash sridharan]

> On Oct. 14, 2016, 4:34 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 198-209
> > 
> >
> > Ditto.

Yeah we actually don't need this anymore. Didn't realize `os::which` also 
checks the permissions on the binary, and not just its existence in the path.


- Avinash


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


On Oct. 12, 2016, 5:49 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52671/
> ---
> 
> (Updated Oct. 12, 2016, 5:49 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6344
> https://issues.apache.org/jira/browse/MESOS-6344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `--network_cni_plugins_dir` was initially designed to take in a
> single directory where all the CNI plugins were expected to be
> present. This however is limiting since the operator will have to
> ensure that all 3rd party plugins are installed in the same location
> which a very hard constraint.
> 
> To make things simpler we are therefore converting the
> `--network_cni_plugins_dir` from a single directory into a search
> path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
>   src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 
> 
> Diff: https://reviews.apache.org/r/52671/diff/
> 
> 
> Testing
> ---
> 
> make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*
> 
> Also ran a single node cluster and tested the flags by moving the bridge 
> plugin from directory to another.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52671: Modified the `--network_cni_plugins_dir` flag.

[Jie Yu]

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


Fix it, then Ship it!





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


insert a line above.



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


`if (plugin.isNone())`



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 155 - 166)


Do you still need this? os::which already did the check for you.



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


insert a line above



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


Ditto.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 180 - 191)


Ditto.



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


Ditto.



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


Ditto.


- Jie Yu


On Oct. 12, 2016, 5:49 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52671/
> ---
> 
> (Updated Oct. 12, 2016, 5:49 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6344
> https://issues.apache.org/jira/browse/MESOS-6344
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `--network_cni_plugins_dir` was initially designed to take in a
> single directory where all the CNI plugins were expected to be
> present. This however is limiting since the operator will have to
> ensure that all 3rd party plugins are installed in the same location
> which a very hard constraint.
> 
> To make things simpler we are therefore converting the
> `--network_cni_plugins_dir` from a single directory into a search
> path.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1b22b28825e8160f659c3cbac37cc576f01666d5 
>   src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 
> 
> Diff: https://reviews.apache.org/r/52671/diff/
> 
> 
> Testing
> ---
> 
> make, make check and sudo ./bin/mesos-tests.sh --gtest_filter=Cni*
> 
> Also ran a single node cluster and tested the flags by moving the bridge 
> plugin from directory to another.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

> On Oct. 13, 2016, 5:52 p.m., Kevin Klues wrote:
> > This does not work for me. I still see:
> > 
> > ```
> > [klueska@core-dev build]$ ./src/mesos-execute --master=127.0.0.1:5050 
> > --name="test-single-1" --command="sleep 2000"
> > I1013 10:47:08.270723 38083 scheduler.cpp:176] Version: 1.1.0
> > I1013 10:47:08.274955 38096 scheduler.cpp:465] New master detected at 
> > master@127.0.0.1:5050
> > Subscribed with ID fe29dfdc-07f0-49c4-8c28-9ade62291388-0001
> > Submitted task 'test-single-1' to agent 
> > 'c81be7b6-da4e-4bac-a884-df27a32f245b-S0'
> > Received status update TASK_FAILED for task 'test-single-1'
> >   message: 'Failed to launch container: Failed to make the containerizer 
> > runtime directory 
> > '/var/run/mesos/containers/8e3a559a-5b13-4460-bb7f-d1e107ab49f0': 
> > Permission denied; Abnormal executor termination: unknown container'
> >   source: SOURCE_AGENT
> >   reason: REASON_CONTAINER_LAUNCH_FAILED
> > ```
> > 
> > Digging into the code in `local.cpp`, it looks like we need to add some 
> > code in there to actually set the agent flag appropriately (just like we do 
> > for `slaveFlags.work_dir`).
> 
> haosdent huang wrote:
> Oh, I see. Because 
> 
> ```
> +export MESOS_RUNTIME_DIR=/tmp/mesos
> ```
> 
> in `mesos-local-flags.sh.in` and
> 
> ```
> Try load = slaveFlags.load("MESOS_");
> 
> ```
> 
> in `local.cpp`, it works although the code in `local.cpp` have not been 
> updated. But I think it should work in your side as well? How you launch the 
> `mesos-local`? Do you use `./bin/mesos-local.sh`?
> 
> Kevin Klues wrote:
> Yes, I ran the commands exactly as you specified. However, I didn't rerun 
> `configure`, which is why I wasn't seeing the change (since 
> `mesos-local-flags.sh` is generated at configure time). Either way, we need 
> to update `local.cpp` to handle the case when this environment variable isn't 
> set and we specify it via the flag to mesos-local itself.

Oh, thanks a lot to help verify! And I found another issue at 
https://reviews.apache.org/r/52856/ 
After apply these two patches, no matter `./bin/mesos-local.sh` or 
`mesos-local` would work as expected.


- haosdent


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


On Oct. 14, 2016, 4:26 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 14, 2016, 4:26 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
>   src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> Test produce 
> 
> ```
> # Run with normal user.
> $ ./bin/mesos-local.sh
> 
> # Open a new session
> $ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
> --command="sleep 2000"
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W1014 00:05:22.457461 36489 scheduler.cpp:161]
> **
> Scheduler driver bound to loopback interface! Cannot communicate with remote 
> master(s). You might want to set 'LIBPROCESS_IP' environment variable to use 
> a routable IP address.
> **
> I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
> I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
> master@127.0.0.1:5050
> Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
> Submitted task 'test-single-1' to agent 
> '8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
> Received status update TASK_RUNNING for task 'test-single-1'
>   source: SOURCE_EXECUTOR
> ```
> 
> ```
> $ sudo make install
> $ mesos-local
> ...
> I1014 12:23:47.563256 4284416 slave.cpp:208] Mesos agent started on 
> (1)@127.0.0.1:5050
> I1014 12:23:47.563302 4284416 slave.cpp:209] Flags at startup: 
> --appc_simple_discovery_uri_prefix="http://; 
> --appc_store_dir="/tmp/mesos/store/appc" 

Review Request 52856: Reverted incorrect changes in 1c2ee5c.

[haosdent huang]

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

Review request for mesos, Jie Yu, Kevin Klues, and Vinod Kone.


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


Repository: mesos


Description
---

Reverted incorrect changes in 1c2ee5c.


Diffs
-

  src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

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

(Updated Oct. 14, 2016, 4:26 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Address @klueska's comments.


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


Repository: mesos


Description
---

Set `runtime_dir` to a temporary folder in `mesos-local`.


Diffs (updated)
-

  bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
  src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
  src/local/local.cpp 257179443827ffdfc946ed655a6840fcea70d454 

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


Testing (updated)
---

To void this error when launch `mesos-local` without `sudo`

```
  message: 'Failed to launch container: Failed to make the containerizer 
runtime directory 
'/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
denied; Abnormal executor termination: unknown container'
```

Test produce 

```
# Run with normal user.
$ ./bin/mesos-local.sh

# Open a new session
$ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
--command="sleep 2000"
WARNING: Logging before InitGoogleLogging() is written to STDERR
W1014 00:05:22.457461 36489 scheduler.cpp:161]
**
Scheduler driver bound to loopback interface! Cannot communicate with remote 
master(s). You might want to set 'LIBPROCESS_IP' environment variable to use a 
routable IP address.
**
I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
master@127.0.0.1:5050
Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
Submitted task 'test-single-1' to agent 
'8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
Received status update TASK_RUNNING for task 'test-single-1'
  source: SOURCE_EXECUTOR
```

```
$ sudo make install
$ mesos-local
...
I1014 12:23:47.563256 4284416 slave.cpp:208] Mesos agent started on 
(1)@127.0.0.1:5050
I1014 12:23:47.563302 4284416 slave.cpp:209] Flags at startup: 
--appc_simple_discovery_uri_prefix="http://; 
--appc_store_dir="/tmp/mesos/store/appc" --authenticate_http_readonly="false" 
--authenticate_http_readwrite="false" --authenticatee="crammd5" 
--authentication_backoff_factor="1secs" --authorizer="local" 
--container_disk_watch_interval="15secs" --containerizers="mesos" 
--default_role="*" --disk_watch_interval="1mins" --docker="docker" 
--docker_kill_orphans="true" --docker_registry="https://registry-1.docker.io; 
--docker_remove_delay="6hrs" --docker_socket="/var/run/docker.sock" 
--docker_stop_timeout="0ns" --docker_store_dir="/tmp/mesos/store/docker" 
--docker_volume_checkpoint_dir="/var/run/mesos/isolators/docker/volume" 
--enforce_container_disk_quota="false" --executor_registration_timeout="1mins" 
--executor_shutdown_grace_period="5secs" --fetcher_cache_dir="/tmp/mesos/fetch" 
--fetcher_cache_size="2GB" --frameworks_home="" --gc_delay="1weeks" 
--gc_disk_headroom="0.1" --hadoop_ho
 me="" --help="false" --hostname_lookup="true" --http_authenticators="basic" 
--http_command_executor="false" --image_provisioner_backend="copy" 
--initialize_driver_logging="true" --isolation="posix/cpu,posix/mem" 
--launcher="posix" --launcher_dir="/usr/local/libexec/mesos" --logbufsecs="0" 
--logging_level="INFO" --max_completed_executors_per_framework="150" 
--oversubscribed_resources_interval="15secs" 
--qos_correction_interval_min="0ns" --quiet="false" --recover="reconnect" 
--recovery_timeout="15mins" --registration_backoff_factor="1secs" 
--runtime_dir="/tmp/mesos/local/0" --sandbox_directory="/mnt/mesos/sandbox" 
--strict="true" --switch_user="true" --version="false" 
--work_dir="/tmp/mesos/local/0"
...

$ mesos-execute --master=127.0.0.1:5050 --name="test-single-1" --command="sleep 
2000"
WARNING: Logging before InitGoogleLogging() is written to STDERR
W1014 12:25:39.922174 3211264 scheduler.cpp:161]
**
Scheduler driver bound to loopback interface! Cannot communicate with remote 
master(s). You might want to set 'LIBPROCESS_IP' environment variable to use a 
routable IP address.
**
I1014 12:25:39.922353 3211264 scheduler.cpp:176] Version: 1.1.0
I1014 12:25:39.927863 3211264 scheduler.cpp:465] New master detected at 
master@127.0.0.1:5050
Subscribed with ID b3af099b-8ee5-4fbe-b27b-fe4832b141c2-0001
Submitted task 'test-single-1' to agent 
'ed8e545b-d1e5-4d22-bc88-517be5632779-S0'
Received status update TASK_RUNNING for task 'test-single-1'
  source: SOURCE_EXECUTOR
```


Thanks,

haosdent huang

Re: Review Request 50003: Propagate work_dir flag from local runs to agents/masters.

[haosdent huang]

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




src/local/local.cpp (line 183)


The `flags` is shadown with the outside variable `flags`. Need to rename to 
`masterFlags`.


- haosdent huang


On Aug. 10, 2016, 11:28 p.m., Ammar Askar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50003/
> ---
> 
> (Updated Aug. 10, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos, Greg Mann and Vinod Kone.
> 
> 
> Bugs: MESOS-5613
> https://issues.apache.org/jira/browse/MESOS-5613
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Propagate work_dir flag from local runs to agents/masters.
> 
> 
> Diffs
> -
> 
>   src/local/flags.hpp f0af0d2 
>   src/local/local.cpp d6fe4d0 
> 
> Diff: https://reviews.apache.org/r/50003/diff/
> 
> 
> Testing
> ---
> 
> Manually tested that `mesos local` and `mesos-local` run now. Not sure if a 
> regression test for this would be good, please advise.
> 
> 
> Thanks,
> 
> Ammar Askar
> 
>

Re: Review Request 52847: Fixed docker v2 image manifest protobuf definition.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 13, 2016, 11:25 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52847/
> ---
> 
> (Updated Oct. 13, 2016, 11:25 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Kevin Klues, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-6173
> https://issues.apache.org/jira/browse/MESOS-6173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the protobuf definition for docker v2 image manifest
> is not correct. Some proto fields should not be 'required'. E.g.,
> when using the private docker registry from JFrog, the 'kid' (the
> key ID) may not exist in the JWK from the manifest signature.
> 
> Depending on this reference:
> https://tools.ietf.org/html/rfc7517#section-4.5
> 
> 'kid' should be OPTIONAL.
> 
> This patch re-inspects the specification of JWS and JWK, and
> fixed the wrong defined protobuf fields.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/v2.proto e2c0a131b8429ca086bc5cd09dccfe3e8d2e50ac 
> 
> Diff: https://reviews.apache.org/r/52847/diff/
> 
> 
> Testing
> ---
> 
> N/A.
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 52854: Fixed the sandbox owner for command tasks.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

If the task has a rootfs, the command executor will be run under root
because it needs to perform pivot_root. Prior to this patch, if the
task wants to run under an unprivileged user, the sandbox of that task
will not be writable because it's owned by root.

This patch fixed the issue (MESOS-6391). The command executor now
changes the owner (non-recursively) of the sandbox to match that of
the task when rootfs is specified for the task.


Diffs
-

  src/launcher/posix/executor.cpp fdee17c5e19b94c350ee192522087051d9c9fe74 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Review Request 52855: Re-enabled the change user test in slave tests.

[Jie Yu]

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

Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

This test verifies that the sandbox of the command task is writable if
the task specifies a non privileged user.


Diffs
-

  src/tests/containerizer/rootfs.hpp 6bc3835cbb62536ec933ef38c9e15138b8611e5f 
  src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 

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


Testing
---

sudo make check


Thanks,

Jie Yu

Re: Review Request 52814: Added notes to the getting started and upgrade docs about --runtime_dir.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52814]

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 Oct. 13, 2016, 5:04 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52814/
> ---
> 
> (Updated Oct. 13, 2016, 5:04 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6312
> https://issues.apache.org/jira/browse/MESOS-6312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added notes to the getting started and upgrade docs about --runtime_dir.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
>   docs/upgrades.md 8e45fdbb485829930f9b918d1d6f24893228e873 
> 
> Diff: https://reviews.apache.org/r/52814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 50125: Added mesos-docker-executor support for devices control.

[Guangya Liu]

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




src/docker/docker.hpp (lines 98 - 101)


We generally do not put `+` at the begining of the code, you can update as 
following:

```
if (deviceInfo.size() != 3) {
  return Error("Parse device information error for '" + devices +
   "', 'PathInHost:PathInContainer:Permission' expected");
}
```

Also you should quota `PathInHost:PathInContainer:Permission` as above to 
hightlight it.



src/docker/executor.hpp (line 81)


s/docker run/`docker run`



src/slave/containerizer/docker.cpp (line 250)


kill this


- Guangya Liu


On 十月 13, 2016, 10:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50125/
> ---
> 
> (Updated 十月 13, 2016, 10:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new flag '--devices' to mesos-docker-executor, and gave its
> feature to control devices exposition, isolation, and access permission.
> Also, passed GPUs assignment to mesos-docker-executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/executor.hpp a49ec1b4045116741af6af08578791fb0440ad8f 
>   src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
> 
> Diff: https://reviews.apache.org/r/50125/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Anindya Sinha]

> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 503-504
> > 
> >
> > MESOS-6374 is where we want eventually but for now the same check is in 
> > `validateDiskInfo()` as well so we don't have to duplicate it here?

Check for `volume.disk().has_volume()` is done since the next line calls 
`volume.disk().volume().mode()`.


> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/master/validation.cpp, lines 505-506
> > 
> >
> > This is called when you destroy a volume as well. So if the framework 
> > is has changed the volume to RO after creating it then it cannot destroy it 
> > (at least in the form it was created)...
> > 
> > I think we should just validate this in the following method?
> > 
> > ```
> > Option validate(
> > const Offer::Operation::Create& create,
> > const Resources& checkpointedResources,
> > const Option& principal) {}
> > ```

Since the offer to frameworks would only contain `Mode::RW`, this check is 
valid for both `CREATE` and `DESTROY` operations. So, we should not allow this 
volume as valid if the `DESTROY` operation marks this as `Mode::RO`.


> On Oct. 13, 2016, 9:58 p.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/linux_filesystem_isolator_tests.cpp, line 1455
> > 
> >
> > Nit: use `frameworkInfo.set_role(DEFAULT_TEST_ROLE);`?

I will leave it as `role1` instead of `DEFAULT_TEST_ROLE` since the remaining 
tests in this file use `role1`. We can change to `DEFAULT_TEST_ROLE` in a 
follow up commit when all tests move to using `DEFAULT_TEST_ROLE`.


- Anindya


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


On Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45963/
> ---
> 
> (Updated Oct. 13, 2016, 5:24 a.m.)
> 
> 
> Review request for mesos, Greg Mann and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4324
> https://issues.apache.org/jira/browse/MESOS-4324
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow the task to specify the persistent volume access to be read-only
> or read-write. Note that the persistent volume is always created as
> read-write.
> If the task is the first consumer of the shared persistent volume, then
> set the ownership of the persistent volume to match that of the task.
> Otherwise, launch the task but if the task is unable to read/write the
> persistent volume, it would fail at that point of time.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 8f62162519f12a157c28ca5f2a76502e466c1481 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> af427c6e5691f1770ab3ebef79502eb2c2176c4a 
>   src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
> eb191a32381f9d1ca84ec29adf352dde375c2f2d 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/45963/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Anindya Sinha]

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

(Updated Oct. 14, 2016, 3:23 a.m.)


Review request for mesos, Greg Mann and Jiang Yan Xu.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Allow the task to specify the persistent volume access to be read-only
or read-write. Note that the persistent volume is always created as
read-write.
If the task is the first consumer of the shared persistent volume, then
set the ownership of the persistent volume to match that of the task.
Otherwise, launch the task but if the task is unable to read/write the
persistent volume, it would fail at that point of time.


Diffs (updated)
-

  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
  src/slave/containerizer/docker.cpp 9a1a8712bf4aba27927136b3a61beaca1b1af997 
  src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
8f62162519f12a157c28ca5f2a76502e466c1481 
  src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
af427c6e5691f1770ab3ebef79502eb2c2176c4a 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
eb191a32381f9d1ca84ec29adf352dde375c2f2d 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 

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


Testing
---

Tests successful.


Thanks,

Anindya Sinha

Re: Review Request 50599: Assigned Nvidia GPU devices to docker container.

[Guangya Liu]

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



You need a rebase for this patch.


src/slave/containerizer/docker.cpp (line 691)


s/"container->devices"/`container->devices`



src/slave/containerizer/docker.cpp (line 707)


two spaces


- Guangya Liu


On 十月 11, 2016, 8:18 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated 十月 11, 2016, 8:18 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Assigned Nvidia GPU devices to docker container based on
> GPUs allocated by Nvidia GPU allocator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Guangya Liu]

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




src/docker/docker.hpp (line 336)


two lines here



src/docker/docker.cpp (lines 395 - 396)


```
if (device.isError()) {
  return Error("Failed to parse device from HostConfig.Devices entry"
   " due to: " + device.error());
}
```

I think that we should also quota `HostConfig.Devices` as 
`'HostConfig.Devices'`, but seems other log messages also do not include quota, 
I think that it is OK to keep consistent here.

But the ideal style is as 
https://github.com/apache/mesos/blob/master/src/docker/docker.cpp#L410



src/docker/docker.cpp (lines 1507 - 1508)


new line here


- Guangya Liu


On 十月 13, 2016, 10:15 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated 十月 13, 2016, 10:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped helper functions to 'Docker::Device' to handle data
> parsing and serializing between 'Docker::Device' structure and
> string.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 52852: Fixed handling of deprecated SSL_ prefix.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 13, 2016, 6:56 p.m., Till Toenshoff wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52852/
> ---
> 
> (Updated Oct. 13, 2016, 6:56 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Bannier, and Joseph Wu.
> 
> 
> Bugs: MESOS-6393
> https://issues.apache.org/jira/browse/MESOS-6393
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> see summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/openssl.cpp 
> 5ddb10dd48908bdae898ccc45d1741d79efe2dce 
> 
> Diff: https://reviews.apache.org/r/52852/diff/
> 
> 
> Testing
> ---
> 
> make check and functional test.
> 
> 
> Thanks,
> 
> Till Toenshoff
> 
>

Review Request 52853: Fixed typo in a comment in hooks.hpp.

[Till Toenshoff]

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

Review request for mesos and Kapil Arya.


Repository: mesos


Description
---

see summary.


Diffs
-

  include/mesos/hook.hpp c8a58641e0768628a9028a70d8c28f7fb412 

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


Testing
---

no functional change - make check


Thanks,

Till Toenshoff

Re: Review Request 52852: Fixed handling of deprecated SSL_ prefix.

[Till Toenshoff]

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

(Updated Oct. 14, 2016, 1:56 a.m.)


Review request for mesos, Adam B, Benjamin Bannier, and Joseph Wu.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 5ddb10dd48908bdae898ccc45d1741d79efe2dce 

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


Testing
---

make check and functional test.


Thanks,

Till Toenshoff

Review Request 52852: Fixed handling of deprecated SSL_ prefix.

[Till Toenshoff]

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

Review request for mesos, Adam B, Benjamin Bannier, and Joseph Wu.


Repository: mesos


Description
---

see summary.


Diffs
-

  3rdparty/libprocess/src/openssl.cpp 5ddb10dd48908bdae898ccc45d1741d79efe2dce 

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


Testing
---

make check and functional test.


Thanks,

Till Toenshoff

Re: Review Request 52803: Changed agent to send TASK_GONE.

[Mesos ReviewBot]

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



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, 52083, 52656, 
52657, 52658, 52659, 52693, 52719, 52720, 52721, 52722, 52723, 52740, 52746, 
52801, 52802, 52803]

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 Oct. 13, 2016, 3:25 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52803/
> ---
> 
> (Updated Oct. 13, 2016, 3:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6332
> https://issues.apache.org/jira/browse/MESOS-6332
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent previously sent TASK_LOST updates for tasks that are killed
> for various reasons, such as containerizer errors or QoS preemption. The
> agent now sends TASK_GONE to partition-aware frameworks instead.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
>   src/tests/oversubscription_tests.cpp 
> b356fb62a4e068bc171a75a76001c6d0e76af92a 
>   src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 
> 
> Diff: https://reviews.apache.org/r/52803/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 50675: Libprocess: Enabled tests that pass on Windows.

[Alex Clemmer]

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

(Updated Oct. 14, 2016, 1:10 a.m.)


Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

A large number of libprocess test files are currently not being built on
Windows. Many of these files contain tests for parts of libprocess that
have already been ported, or require only trivial fixes to work (such as
removing `#include`s on Windows). A small minority of the tests contain
bugs that we should fix.

This commit will add these files to the build, fix some of the
trivially-fixable tests, and disable tests that are known to fail
because of bugs, including comments explaining why and links to JIRA
issues where appropriate.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/CMakeLists.txt 
c267117ab422310854d492493d711039b287d7e3 
  3rdparty/libprocess/src/tests/future_tests.cpp 
383d260a4c0b17e9a0b5af002bb35070e3ec0b01 
  3rdparty/libprocess/src/tests/http_tests.cpp 
8d6c8c4507b93dd11f927bad2a8f0faeb957e388 
  3rdparty/libprocess/src/tests/limiter_tests.cpp 
71a3d25d4040de9b0d3eb23612be80599f4b6a9a 
  3rdparty/libprocess/src/tests/metrics_tests.cpp 
88526e67a20b7b58a6e14034215faa3ae9879fd0 
  3rdparty/libprocess/src/tests/process_tests.cpp 
3936f47dfcacb0f85a6fa0f68f1707159b1c320c 
  3rdparty/libprocess/src/tests/reap_tests.cpp 
586d52891af191d43229707c89a91cf9cbb9007d 
  3rdparty/libprocess/src/tests/subprocess_tests.cpp 
c8350cf8e512dca23933725e6edb3e3d94380211 
  3rdparty/libprocess/src/tests/time_tests.cpp 
9d40eaee046eaa43199bb92a54892207e9c31ef2 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 50674: Stout: Enabled tests that pass on Windows.

[Alex Clemmer]

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

(Updated Oct. 14, 2016, 1:06 a.m.)


Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

A large number of Stout test files are currently not being built on
Windows.  Many of these files contain tests for parts of Stout that have
already been ported, or require only trivial fixes to work (such as
removing `#include`s on Windows). A small minority of the tests contain
bugs that we should fix.

This commit will add these files to the build, fix some of the
trivially-fixable tests, and disable tests that are known to fail
because of bugs, including comments explaining why and links to JIRA
issues where appropriate.


Diffs (updated)
-

  3rdparty/stout/include/stout/gtest.hpp 
fca304a0be6ccfdabb351d43ee670435978c1f0f 
  3rdparty/stout/include/stout/mac.hpp 91c4fdad350459b3e0bdf1744089e14ac883829a 
  3rdparty/stout/tests/CMakeLists.txt 49971c7ccff319c96ed1f48cb2c9665695090688 
  3rdparty/stout/tests/flags_tests.cpp 94ba915c40836e476cf6097274a85c55acd4d73b 
  3rdparty/stout/tests/ip_tests.cpp 59e69a51e41b4773cb7c5a5de9f70d4810fd0294 
  3rdparty/stout/tests/mac_tests.cpp ebd50a00585ef37ea2fa244d48bdd90036040258 
  3rdparty/stout/tests/os/process_tests.cpp 
4977d0210f7520b4f891b2cd6a926c93aafb5706 
  3rdparty/stout/tests/os/rmdir_tests.cpp 
ffe234baac305e26b5a29cffcdd310350d10167e 
  3rdparty/stout/tests/os_tests.cpp 6a7b836f7102d9e014eaf9dbd47e33b987becb33 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 50674: Stout: Enabled tests that pass on Windows.

[Alex Clemmer]

> On Oct. 14, 2016, 12:30 a.m., Joseph Wu wrote:
> > 3rdparty/stout/tests/os_tests.cpp, lines 129-131
> > 
> >
> > What about defining a macro:
> > ```
> > #ifndef __WINDOWS__
> >   #define SLEEP_COMMAND(x) "sleep x"
> > #else
> >   #define SLEEP_COMMAND(x) "timeout x"
> > #endif // __WINDOWS__  
> > ```
> > 
> > We shell out to `sleep` in lots of tests, so this might come in handy 
> > later.

Good idea. However, I think that you might have meant this instead:

```
#define SLEEP_COMMAND(x) "sleep " #x
```

:)


- Alex


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


On Oct. 11, 2016, 11:49 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50674/
> ---
> 
> (Updated Oct. 11, 2016, 11:49 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A large number of Stout test files are currently not being built on
> Windows.  Many of these files contain tests for parts of Stout that have
> already been ported, or require only trivial fixes to work (such as
> removing `#include`s on Windows). A small minority of the tests contain
> bugs that we should fix.
> 
> This commit will add these files to the build, fix some of the
> trivially-fixable tests, and disable tests that are known to fail
> because of bugs, including comments explaining why and links to JIRA
> issues where appropriate.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gtest.hpp 
> fca304a0be6ccfdabb351d43ee670435978c1f0f 
>   3rdparty/stout/include/stout/mac.hpp 
> 91c4fdad350459b3e0bdf1744089e14ac883829a 
>   3rdparty/stout/tests/CMakeLists.txt 
> 49971c7ccff319c96ed1f48cb2c9665695090688 
>   3rdparty/stout/tests/flags_tests.cpp 
> 94ba915c40836e476cf6097274a85c55acd4d73b 
>   3rdparty/stout/tests/ip_tests.cpp 59e69a51e41b4773cb7c5a5de9f70d4810fd0294 
>   3rdparty/stout/tests/mac_tests.cpp ebd50a00585ef37ea2fa244d48bdd90036040258 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
>   3rdparty/stout/tests/os_tests.cpp 6a7b836f7102d9e014eaf9dbd47e33b987becb33 
> 
> Diff: https://reviews.apache.org/r/50674/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Joseph Wu]

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


Ship it!




With the cleanups in ( https://reviews.apache.org/r/52848/ ), this is good to 
go!

- Joseph Wu


On Aug. 1, 2016, 2:24 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50673/
> ---
> 
> (Updated Aug. 1, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Bugs: MESOS-5942
> https://issues.apache.org/jira/browse/MESOS-5942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will fix 2 known bugs in the Windows implementation of
> `os::rmdir`, as chronicled in MESOS-5942, namely:
> 
> 1. Calling `os::rmdir` with a file argument (rather than a directory)
>and the `recursive` parameter set to `true` will fail on Windows,
>but succeed on POSIX.
> 
> The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
> the behavior of which depends on the arguments the caller passes in. If
> the formal parameter `recursive` is set to `true`, then the semantics
> are meant to be`rm -r`; if `false`, the semantics are meant to be
> `::rmdir`.
> 
> The implications of this are somewhat subtle: `::rmdir` will error out
> if you try to delete (e.g.) regular files, while `rm -r` will happily
> delete them.
> 
> On Windows, we currently always have `::rmdir`-style semantics, in that
> we if you pass a path that points at a file to `os::rmdir`, it will not
> delete that file.
> 
> This commit will reverse this, and move make the Windows implementation
> semantically identical to the POSIX implementation (at least in this
> regard).
> 
> 2. Recursively deleting nested directories fails on windows when
>`removeRoot` is set to `false`.
> 
> Currently if you set the `removeRoot` parameter to `false`, the Windows
> implementation of `os::rmdir` will fail to delete a directory inside a
> directory. The reason is that we are propagating the `removeRoot` flag
> to the recursive calls to `os::rmdir`. The implication of this is that
> the recursive call will *not* delete the nested directory (since
> `removeRoot` is `false`).
> 
> This commit will fix this by setting `removeRoot` to `true` in recursive
> calls to `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> b74bf7153a15435ce424880df84901c349dee216 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
> 
> Diff: https://reviews.apache.org/r/50673/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 50674: Stout: Enabled tests that pass on Windows.

[Joseph Wu]

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


Fix it, then Ship it!





3rdparty/stout/include/stout/gtest.hpp (line 179)


Add a TODO to remove these once we've implemented/fixed the tests.



3rdparty/stout/tests/os_tests.cpp (lines 129 - 131)


What about defining a macro:
```
#ifndef __WINDOWS__
  #define SLEEP_COMMAND(x) "sleep x"
#else
  #define SLEEP_COMMAND(x) "timeout x"
#endif // __WINDOWS__  
```

We shell out to `sleep` in lots of tests, so this might come in handy later.


- Joseph Wu


On Oct. 11, 2016, 4:49 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50674/
> ---
> 
> (Updated Oct. 11, 2016, 4:49 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A large number of Stout test files are currently not being built on
> Windows.  Many of these files contain tests for parts of Stout that have
> already been ported, or require only trivial fixes to work (such as
> removing `#include`s on Windows). A small minority of the tests contain
> bugs that we should fix.
> 
> This commit will add these files to the build, fix some of the
> trivially-fixable tests, and disable tests that are known to fail
> because of bugs, including comments explaining why and links to JIRA
> issues where appropriate.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/gtest.hpp 
> fca304a0be6ccfdabb351d43ee670435978c1f0f 
>   3rdparty/stout/include/stout/mac.hpp 
> 91c4fdad350459b3e0bdf1744089e14ac883829a 
>   3rdparty/stout/tests/CMakeLists.txt 
> 49971c7ccff319c96ed1f48cb2c9665695090688 
>   3rdparty/stout/tests/flags_tests.cpp 
> 94ba915c40836e476cf6097274a85c55acd4d73b 
>   3rdparty/stout/tests/ip_tests.cpp 59e69a51e41b4773cb7c5a5de9f70d4810fd0294 
>   3rdparty/stout/tests/mac_tests.cpp ebd50a00585ef37ea2fa244d48bdd90036040258 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
>   3rdparty/stout/tests/os_tests.cpp 6a7b836f7102d9e014eaf9dbd47e33b987becb33 
> 
> Diff: https://reviews.apache.org/r/50674/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/slave/containerizer/fetcher.cpp (line 732)


We are constructing the path at least 3 times. 

How about 

```
const string stdoutPath = path::join(info.sandbox_directory(), "stdout");
```

and use the variable below?

Same for stderr.



src/slave/containerizer/fetcher.cpp (line 740)


Same deal. Use `stderrPath`.



src/slave/containerizer/fetcher.cpp (line 754)


Below the line we should add a TODO

```
TODO(megha.sharma): Fetcher should not create seperate stdout/stderr files 
but rather use FDs prepared by the container logger. See MESOS-6271 for more 
details.
```



src/slave/containerizer/fetcher.cpp (line 764)


Missing space before `+`.



src/slave/containerizer/fetcher.cpp (line 766)


s/ with error//

The ":" is enough to indicate the error message that follows.



src/slave/containerizer/fetcher.cpp (line 781)


s/ with error//

The ":" is enough to indicate the error message that follows.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 591)


"This test verified that" or "Tests that".



src/tests/containerizer/mesos_containerizer_tests.cpp (line 610)


"exit 0" may be a better (more clear w.r.t intention) dummy executor 
command.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 626)


`os::getuid(user)` didn't work?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 631)


s/std::string/string/

s/stdOutPath/stdoutPath/

stdout is common enough that we should just treat it as one word.



src/tests/containerizer/mesos_containerizer_tests.cpp (line 636)


s/std::string/string/

s/stdErrPath/stderrPath/



src/tests/containerizer/mesos_containerizer_tests.cpp (line 642)


2 space indentation.


- Jiang Yan Xu


On Oct. 13, 2016, 7:33 a.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 13, 2016, 7:33 a.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 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 50673: Made semantics of `os::rmdir` consistent between POSIX and Windows.

[Joseph Wu]

> On Aug. 2, 2016, 4:03 p.m., Joseph Wu wrote:
> > 3rdparty/stout/tests/os/rmdir_tests.cpp, lines 116-117
> > 
> >
> > Since these two sets are only modified once within the test, why not do:
> > ```
> >   const hashset expectedRootListing = { newDirectoryName };
> >   const hashset expectedSubListing = { newFileName };
> > ```
> > 
> > I realize this is the pattern within the other tests in this file.  So 
> > perhaps you can change the pattern in a subsequent review :)

Added: https://reviews.apache.org/r/52848/


- Joseph


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


On Aug. 1, 2016, 2:24 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50673/
> ---
> 
> (Updated Aug. 1, 2016, 2:24 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat and Joseph Wu.
> 
> 
> Bugs: MESOS-5942
> https://issues.apache.org/jira/browse/MESOS-5942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit will fix 2 known bugs in the Windows implementation of
> `os::rmdir`, as chronicled in MESOS-5942, namely:
> 
> 1. Calling `os::rmdir` with a file argument (rather than a directory)
>and the `recursive` parameter set to `true` will fail on Windows,
>but succeed on POSIX.
> 
> The POSIX semantics of `os::rmdir` are a union of `rm -r` and `::rmdir`,
> the behavior of which depends on the arguments the caller passes in. If
> the formal parameter `recursive` is set to `true`, then the semantics
> are meant to be`rm -r`; if `false`, the semantics are meant to be
> `::rmdir`.
> 
> The implications of this are somewhat subtle: `::rmdir` will error out
> if you try to delete (e.g.) regular files, while `rm -r` will happily
> delete them.
> 
> On Windows, we currently always have `::rmdir`-style semantics, in that
> we if you pass a path that points at a file to `os::rmdir`, it will not
> delete that file.
> 
> This commit will reverse this, and move make the Windows implementation
> semantically identical to the POSIX implementation (at least in this
> regard).
> 
> 2. Recursively deleting nested directories fails on windows when
>`removeRoot` is set to `false`.
> 
> Currently if you set the `removeRoot` parameter to `false`, the Windows
> implementation of `os::rmdir` will fail to delete a directory inside a
> directory. The reason is that we are propagating the `removeRoot` flag
> to the recursive calls to `os::rmdir`. The implication of this is that
> the recursive call will *not* delete the nested directory (since
> `removeRoot` is `false`).
> 
> This commit will fix this by setting `removeRoot` to `true` in recursive
> calls to `os::rmdir`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/rmdir.hpp 
> b74bf7153a15435ce424880df84901c349dee216 
>   3rdparty/stout/tests/os/rmdir_tests.cpp 
> ffe234baac305e26b5a29cffcdd310350d10167e 
> 
> Diff: https://reviews.apache.org/r/50673/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 52848: Cleaned up style in stout rmdir_tests.cpp.

[Joseph Wu]

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

Review request for mesos, Daniel Pravat, Alex Clemmer, Joris Van Remoortere, 
and Jiang Yan Xu.


Repository: mesos


Description
---

Change a few test variables to `const` and made use of initializer lists
as these variables are only modified once at the beginning of the tests.


Diffs
-

  3rdparty/stout/tests/os/rmdir_tests.cpp 
ffe234baac305e26b5a29cffcdd310350d10167e 

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


Testing
---

make check (OSX)

Windows:
msbuild Mesos.sln /p:PreferredToolArchitecture=x64 /m /t:stout_tests

On an Administrator CmdPrompt:
"3rdparty/stout/tests/Debug/stout_tests.exe"
```
[ RUN  ] RmdirTest.RemoveDirectoryWithNoTargetSymbolicLink
C:\mesos\3rdparty\stout\tests\os\rmdir_tests.cpp(285): error: 
fs::symlink("tmp", link): '_stat' failed on path 'C:\tmp\vehk5d\tmp': No such 
file or directory
[  FAILED  ] RmdirTest.RemoveDirectoryWithNoTargetSymbolicLink (4 ms)

[--] Global test environment tear-down
[==] 187 tests from 32 test cases ran. (2445 ms total)
[  PASSED  ] 185 tests.
[  FAILED  ] 2 tests, listed below:
[  FAILED  ] NetTest.LinkDevice
[  FAILED  ] RmdirTest.RemoveDirectoryWithNoTargetSymbolicLink
```
^ That was failing before; no change in this review.


Thanks,

Joseph Wu

Re: Review Request 52828: Allow the chown code in fetcher to be executed.

[Jiang Yan Xu]

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


Fix it, then Ship it!





src/tests/slave_tests.cpp (line 932)


"test" worked? Could you verify?


- Jiang Yan Xu


On Oct. 13, 2016, 2:36 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52828/
> ---
> 
> (Updated Oct. 13, 2016, 2:36 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: mesos-5218
> https://issues.apache.org/jira/browse/mesos-5218
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved the uri.size() check in fetcher so that the chown to
> task user of stdout/stderr in sandbox directory happens even
> when there is no uri to be fetched.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 
> 
> Diff: https://reviews.apache.org/r/52828/diff/
> 
> 
> Testing
> ---
> 
> make check on macOs and linux
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52827: Added backend suffix to image layer rootfs path.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/docker/paths.cpp (lines 61 - 64)


In fact, i think we should use aufs style whiteout by default because 
that's also the OCI standard. So the logic here should be:
```
if (backend == "overlay") {
  return path::join(layerPath, "rootfs." + backend);
}

return path::join(layerPath, "rootfs");
```

Also, I think we should define constants for "overlay" here as it has been 
referenced multiple times.


- Jie Yu


On Oct. 13, 2016, 2:38 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52827/
> ---
> 
> (Updated Oct. 13, 2016, 2:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously image layer rootfs path is in the format below regardless
> of which backend is used.
>   /layers//rootfs
> This introduced an issue: when agent is restarted with a different
> backend, we will wrongly handle the whiteout files since different
> backends(e.g., aufs and overlay) have different whiteout standard.
> 
> In this commit, we added backend suffix to image layer rootfs path
> for non-aufs backend like below, so each backend will only handle
> its own image layers.
>   /layers//rootfs.
> For aufs backend, it is still in the previous format, this is to
> handle backward compatibility.
> 
> So the expected result of this commit is:
> 1. If user switches backend when restarting agent, all the image
> layers of the previous backend in the store will just be ignored.
> 2. In the upgrade case, if user starts a new version of Mesos agent
> with a non-aufs backend (i.e., copy/bind/overlay), then all the
> image layers in the store will just be ignored, but if user starts
> the agent with aufs backend, all the image layers in the store can
> still be used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 9b09dca5cd8f9e60a90cf574300b250eb18ede35 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 6456d90220a4026696a04f9969763cf682464353 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> 949e0c16a361f22b00e267fcf81093690327041f 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> a5cc952072274c61e8c515e8b1b7ea563344e950 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 1e2cc2dda3518d99ef1ad06e2b8ea7f78a4dcf72 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 52b9ea934a1dbe3312b8f08f5da8085e9e306de5 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 10fbc4149ac2e7503ffe7f2746fbd0e14a2365b4 
> 
> Diff: https://reviews.apache.org/r/52827/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 52827: Added backend suffix to image layer rootfs path.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52827]

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 Oct. 13, 2016, 2:38 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52827/
> ---
> 
> (Updated Oct. 13, 2016, 2:38 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6360
> https://issues.apache.org/jira/browse/MESOS-6360
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously image layer rootfs path is in the format below regardless
> of which backend is used.
>   /layers//rootfs
> This introduced an issue: when agent is restarted with a different
> backend, we will wrongly handle the whiteout files since different
> backends(e.g., aufs and overlay) have different whiteout standard.
> 
> In this commit, we added backend suffix to image layer rootfs path
> for non-aufs backend like below, so each backend will only handle
> its own image layers.
>   /layers//rootfs.
> For aufs backend, it is still in the previous format, this is to
> handle backward compatibility.
> 
> So the expected result of this commit is:
> 1. If user switches backend when restarting agent, all the image
> layers of the previous backend in the store will just be ignored.
> 2. In the upgrade case, if user starts a new version of Mesos agent
> with a non-aufs backend (i.e., copy/bind/overlay), then all the
> image layers in the store will just be ignored, but if user starts
> the agent with aufs backend, all the image layers in the store can
> still be used.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
> 9b09dca5cd8f9e60a90cf574300b250eb18ede35 
>   src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
> 6456d90220a4026696a04f9969763cf682464353 
>   src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
> 949e0c16a361f22b00e267fcf81093690327041f 
>   src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
> a5cc952072274c61e8c515e8b1b7ea563344e950 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> 1e2cc2dda3518d99ef1ad06e2b8ea7f78a4dcf72 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> 52b9ea934a1dbe3312b8f08f5da8085e9e306de5 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 10fbc4149ac2e7503ffe7f2746fbd0e14a2365b4 
> 
> Diff: https://reviews.apache.org/r/52827/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 52847: Fixed docker v2 image manifest protobuf definition.

[Gilbert Song]

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

Review request for mesos, Artem Harutyunyan, Jie Yu, Kevin Klues, and Timothy 
Chen.


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


Repository: mesos


Description
---

Previously, the protobuf definition for docker v2 image manifest
is not correct. Some proto fields should not be 'required'. E.g.,
when using the private docker registry from JFrog, the 'kid' (the
key ID) may not exist in the JWK from the manifest signature.

Depending on this reference:
https://tools.ietf.org/html/rfc7517#section-4.5

'kid' should be OPTIONAL.

This patch re-inspects the specification of JWS and JWK, and
fixed the wrong defined protobuf fields.


Diffs
-

  include/mesos/docker/v2.proto e2c0a131b8429ca086bc5cd09dccfe3e8d2e50ac 

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


Testing
---

N/A.


Thanks,

Gilbert Song

Re: Review Request 52832: Corrected usage of "it's" in Mesos.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 13, 2016, 10:49 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52832/
> ---
> 
> (Updated Oct. 13, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected usage of "it's" in Mesos.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> b419cb4a2cc85cb903af91378d7291e4966bb879 
>   docs/oversubscription.md b4e487310f83234b6182b5c4d1e2962936b054fe 
>   include/mesos/executor.hpp 10be62a86627e3157e8f439b03e139c486e8b161 
>   include/mesos/log/log.hpp 3edb9f11dcdc41aa7ad953763df9ab1367708d76 
>   src/java/src/org/apache/mesos/Executor.java 
> d80ccac77e784f0bfa9376b69f2ff2c4a169fe8a 
>   src/linux/cgroups.hpp 3c9d9e21c8332668286e4170be711aa260c12c1d 
>   src/local/local.cpp 2be5bcf4f6fc102b8cd85eee4dc4cb689f1850f0 
>   src/log/replica.cpp 5ab7b3d55988a9a95e70f900f95746bc12dd3da0 
>   src/master/main.cpp 4a1a8e70ab0535aa131681b2b09a99e51c20158e 
>   src/slave/containerizer/mesos/containerizer.cpp 
> cc9e2bc020be0b0c127ce57e7aa04ca4391126c1 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> e8edfd2ccb123e5853b51f121a881d864498802e 
>   src/slave/main.cpp 949a738ab3e44d8efc878e59c482a750ab41d1e4 
>   src/tests/hierarchical_allocator_tests.cpp 
> cae582e8693bcb5695a53e1f6484e421d31875a6 
>   src/tests/scheduler_http_api_tests.cpp 
> 80a2ef0af9a4c67deaef40e1f36343868ee4428f 
>   src/zookeeper/group.cpp 65a6d05bd379dd7542507ada8e602f855f037b37 
> 
> Diff: https://reviews.apache.org/r/52832/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52830: Corrected usage of "it's" in libprocess.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 13, 2016, 10:49 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52830/
> ---
> 
> (Updated Oct. 13, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected usage of "it's" in libprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/deferred.hpp 
> da87e65c7cdc431d52be771f12ff420967c419c4 
>   3rdparty/libprocess/src/process.cpp 
> f1d746c52cfe659f5cd7da4b7a6424ff585619a3 
> 
> Diff: https://reviews.apache.org/r/52830/diff/
> 
> 
> Testing
> ---
> 
> Visual inspection.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52831: Corrected usage of "it's" in stout.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 13, 2016, 10:49 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52831/
> ---
> 
> (Updated Oct. 13, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected usage of "it's" in stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/README.md 172150c586cd4020edc4dbdabffd1306c9177694 
>   3rdparty/stout/include/stout/os/linux.hpp 
> fd9b0edad5670944ff760b3029fcf62e2dbedd0b 
>   3rdparty/stout/include/stout/os/posix/killtree.hpp 
> 8ba21dedd793d3819ad2bc5674eda3ad41f9295a 
> 
> Diff: https://reviews.apache.org/r/52831/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 51617: Added the `remove` and `insert` methods.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 (line 130)


space after `a`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 262 - 284)


Is this necessary? I think this should be the job of the container runtime? 
In Mesos, if attach fails, 'detach' will be called in cleanup.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 329)


s/portMap/portMapping/



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 331 - 334)


Let's split this into `addPortMapping` and `delPortMapping`



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (line 336)


s/proto/protocol/



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 340 - 350)


I suggest introduce a helper for this:
```
string getIptablesDNATRule(
const IP& ip,
const PortMapping& portMapping);
```



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 373 - 378)


The logic here is a bit complicated. I'd suggest we use C++ string literal 
and a script here. I would probably introduce a helper `ensureChain` to install 
the chain first.

```
strings::format(
R"~(
#!/bin/sh

# COMMENTS HERE.
iptables -t nat --list %s
if [ $? -ne 0 ]; then
  iptables -t nat -N %s
fi

# COMMENTS HERE. (Q: what if this rule already exists?)
iptables -t nat -A PREROUTING --dst-type LOCAL -j %s
)~");
```

And then, you can install the DNAT rule in the chain. Also, I suggest we 
calculate `exclude` devices list here in this function, rather than in the top 
level caller.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 425 - 457)


Let's don't do this for simplicity for now. I think it's ok to leave the 
chain there.



src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 (lines 482 - 494)


Yeah, I understand that we want to do best effort cleanup. I think the 
logic of this method becomes a bit confusing now. I'd suggest we split 
`execute` into two: `handleCommandAdd` and `handleCommandDel`


- Jie Yu


On Oct. 13, 2016, 8:33 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51617/
> ---
> 
> (Updated Oct. 13, 2016, 8:33 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the `remove` and `insert` methods.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  7fad707a240234e35828917aea1bc79f42fe130e 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 
> 
> Diff: https://reviews.apache.org/r/51617/diff/
> 
> 
> Testing
> ---
> 
> Ran the CNI plugin against a network namespace with the following JSON input:
> ```
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS-TEST",
> "excludeDevices": ["mesos-cni0"],
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "192.168.37.0/24",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 8080,
> "container_port" : 9000
>   }
> }
>   }
> }
> }
> ```
> 
> Used the ADD command to test that the CNI plugin correctly invokes the 
> delegate plugin (a CNI bridge plugin in this case) and also inserts the 
> 

Re: Review Request 45963: Allow tasks to set persistent volume as readonly or readwrite resource.

[Jiang Yan Xu]

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




src/master/validation.cpp (lines 500 - 501)


MESOS-6374 is where we want eventually but for now the same check is in 
`validateDiskInfo()` as well so we don't have to duplicate it here?



src/master/validation.cpp (lines 502 - 503)


This is called when you destroy a volume as well. So if the framework is 
has changed the volume to RO after creating it then it cannot destroy it (at 
least in the form it was created)...

I think we should just validate this in the following method?

```
Option validate(
const Offer::Operation::Create& create,
const Resources& checkpointedResources,
const Option& principal) {}
```



src/slave/containerizer/docker.cpp (line 469)


s/container directory/sandbox directory/ because the latter term is used 
widely and explained in documentation. "container directory" is not as clear. 
People may wonder "which contianer directory?".



src/slave/containerizer/docker.cpp (line 470)


We shouldn't use snake casing in this case. Admittedly a single letter `s` 
is usually discourged. So how about

```
struct stat s;

...

const uid_t uid = s.st_uid;
const gid_t gid = s.st_gid;
```

And we use `uid`, `gid` in the code that follows?



src/slave/containerizer/docker.cpp (lines 498 - 499)


If in use, we do nothing right? 

You already have a comment above `if (!isResourceInUse) {` that explains 
what's going to happen if this variable is true so here perhaps kill the 
comment? The variable name is pretty self-describing to me.



src/slave/containerizer/docker.cpp (line 500)


Here we are exclusively talking about persistent volumes so I think 
`isVolumeInUse` or even simpler `isInUse` is a better name.



src/slave/containerizer/docker.cpp (lines 509 - 514)


Consolidate the comments into one paragraph because they are tightly 
related? 

```
Set the ownership of the persistent volume to match that of the sandbox 
directory if the volume is not already in use. If the volume is currently in 
use by other containers, tasks in this container may fail to read from or write 
to the persistent volume due to incompatible ownership and file system 
permissions.
```



src/slave/containerizer/docker.cpp (lines 516 - 521)


This is no longer true, and the comment above already serves as the caution 
note so we can kill this.



src/slave/containerizer/docker.cpp (line 552)


What info in flags is relevant here?



src/slave/containerizer/docker.cpp (line 555)


From the persistent volume to the container?



src/slave/containerizer/docker.cpp (line 570)


s/(as read-only)/as read-only/ since the read-only part is the key info 
here and not "an aside"?



src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 648)


Comments for linux.cpp are the same as those for docker.cpp.



src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (lines 153 - 158)


Comments for posix.cpp are the same as those for docker.cpp.



src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (lines 266 - 268)


Log a warning if the volume is RO?



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1427)


Add a summary that mentions "task attempts to write to the volume fails" 
and also "we use a shared persistent volume here but its sharedness is not 
important here. Regular persistent volumes works no differently".



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1428)



s/ROOT_SharedVolumeUsageReadOnlyMode/ROOT_WriteAccessSharedPersistentVolumeReadOnlyMode/

The name is slightly longer but the full spelling `SharedPersistentVolume` 
helps with code grepping.



src/tests/containerizer/linux_filesystem_isolator_tests.cpp (line 1438)


We don't need "disk/du" for the purpose of this 

Re: Review Request 52828: Allow the chown code in fetcher to be executed.

[Megha Sharma]

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

(Updated Oct. 13, 2016, 9:36 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Moved the uri.size() check in fetcher so that the chown to
task user of stdout/stderr in sandbox directory happens even
when there is no uri to be fetched.


Diffs
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 

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


Testing (updated)
---

make check on macOs and linux


Thanks,

Megha Sharma

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52828, 52058]

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 Oct. 13, 2016, 2:33 p.m., Megha Sharma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52058/
> ---
> 
> (Updated Oct. 13, 2016, 2:33 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 
> 11104d66e6dd05d8eb1d37a2e3250aca19278110 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> ad2070246b7fdb90aa6ed02326b5a7eb95365997 
> 
> Diff: https://reviews.apache.org/r/52058/diff/
> 
> 
> Testing
> ---
> 
> make check on linux and macos
> 
> 
> Thanks,
> 
> Megha Sharma
> 
>

Re: Review Request 52841: Provided more information on failed parallel tests.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On Oct. 13, 2016, 7:52 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52841/
> ---
> 
> (Updated Oct. 13, 2016, 7:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Bugs: MESOS-6140
> https://issues.apache.org/jira/browse/MESOS-6140
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In case of failed tests the parallel test runner always prints the
> full log of all failed shards. Since our tests produce usually more
> than a screenful of output usually only failed tests from the shard
> reported last will be visible which can give the impression that only
> this shard failed.
> 
> This commit adds the number of failed shards to the output so users
> get some hint that more failed tests might be reported off their
> visible screen area.
> 
> 
> Diffs
> -
> 
>   support/mesos-gtest-runner.py 6c1cf54b36461c029241dd9c36540f1019a3707f 
> 
> Diff: https://reviews.apache.org/r/52841/diff/
> 
> 
> Testing
> ---
> 
> * added a `FAIL()` to a single test and confirmed that with that the 
> parallelized test execution reported
> 
> [FAIL]: 1 shards have failed tests
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51617: Added the `remove` and `insert` methods.

[Avinash sridharan]

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

(Updated Oct. 13, 2016, 8:33 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added the `remove` and `insert` methods.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

Ran the CNI plugin against a network namespace with the following JSON input:
```
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS-TEST",
"excludeDevices": ["mesos-cni0"],
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8080,
"container_port" : 9000
  }
}
  }
}
}
```

Used the ADD command to test that the CNI plugin correctly invokes the delegate 
plugin (a CNI bridge plugin in this case) and also inserts the correct iptable 
entries for the given port mapping. After running this plugin, this was the 
output of the `iptables -t nat -S MESOS-TEST` command:
```
sudo iptables -t nat -S MESOS-TEST
-N MESOS-TEST
-A MESOS-TEST ! -i mesos-cni0 -p tcp -m tcp --dport 8080 -j DNAT 
--to-destination 192.168.37.21:9000
```

Ran a python HTTP server in this network namespace and verified that DNAT works 
from outside the box. Was able to connect to port 9000 of this server, by 
connecting to port 8080 on the host.

Used the DEL command to test the CNI plugin correctly deletes the DNAT rule and 
chain, if there are no DNAT rules exist in the chain. After running the DEL 
command (by injecting `NetworkInfo` into the above JSON schema) verified the 
chain and the DNAT rule is deleted from iptables.


Thanks,

Avinash sridharan

Re: Review Request 51486: Added `execute` method.

[Avinash sridharan]

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

(Updated Oct. 13, 2016, 8:32 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Added `execute` method.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 7fad707a240234e35828917aea1bc79f42fe130e 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 2ff8b0e76a11b6f6c98b839d3ac91a81e41285f5 

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


Testing
---

make. 

Created a network namespace using `ip route2` and used the following script to 
test the execution of the 'delegate plugin':
export CNI_COMMAND="DEL"
export CNI_CONTAINERID="00001110"
export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
export CNI_IFNAME="eth0"
export CNI_NETNS="/var/run/netns/$1"
$MESOS_SRC/build/src/.libs/mesos-cni-port-mapper < $2
unset CNI_COMMAND
unset CNI_CONTAINERID
unset CNI_PATH
unset CNI_IFNAME
unset CNI_NETNS

Here $1 is the name of the netns created by `ip route2` and $2 is the CNI 
configuration of the port-mapper plugin. We used the following config:
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS",
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "192.168.37.0/24",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 8090,
"container_port" : 80
  }
}
  }
}
}


Thanks,

Avinash sridharan

Review Request 52841: Provided more information on failed parallel tests.

[Benjamin Bannier]

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

Review request for mesos, Alexander Rukletsov and Till Toenshoff.


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


Repository: mesos


Description
---

In case of failed tests the parallel test runner always prints the
full log of all failed shards. Since our tests produce usually more
than a screenful of output usually only failed tests from the shard
reported last will be visible which can give the impression that only
this shard failed.

This commit adds the number of failed shards to the output so users
get some hint that more failed tests might be reported off their
visible screen area.


Diffs
-

  support/mesos-gtest-runner.py 6c1cf54b36461c029241dd9c36540f1019a3707f 

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


Testing
---

* added a `FAIL()` to a single test and confirmed that with that the 
parallelized test execution reported

[FAIL]: 1 shards have failed tests


Thanks,

Benjamin Bannier

Re: Review Request 52677: Added all test dependency to libprocess' tests target.

[Benjamin Bannier]

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

(Updated Oct. 13, 2016, 9:33 p.m.)


Review request for mesos, Kapil Arya and Till Toenshoff.


Changes
---

Rebased.


Repository: mesos


Description (updated)
---

We rely on some additional tools beyond the currently listed being
present. Since they are already listed in `check_PROGRAMS` to make
`make check` work, just reuse that variable here instead of expanding
the full list by hand.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am f91155b8d42b0c5c58596dd8fddbeb29a3d2b96e 

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


Testing (updated)
---

* `make tests`
* `make check`


Thanks,

Benjamin Bannier

Re: Review Request 52718: Added v1 api helpers to create calls in 'tests/mesos.hpp'.

[Gilbert Song]

> On Oct. 10, 2016, 7:56 p.m., Anand Mazumdar wrote:
> > Thanks for introducing these test helpers Gilbert. These would be very 
> > useful for everyone.
> > 
> > Most of my comments are around whether we can add these as gtest custom 
> > actions instead. Though, we can argue for having both, I would prefer 
> > having them as actions instead since most use-cases would be around just 
> > invoking the default actions (looking at how they are being used today in 
> > the present code).

Thanks for thorough comments, Anand!

Personally I dont really like the way to have custom actions for send out 
`calls`. I would still insist on having both for the following reasons:
1. Explicitly doing `mesos->send()` is more code-readable for other people to 
understand exactly what is happening in our unit tests, vs, implicitly send out 
a call under the hood and rely on a mock class to expect a call.
2. Both ways would only contain two lines of codes. We should allow whether to 
use a gtest custom action or explicitly send a call.
3. Currently the unit tests using v0 api in our code base, use both ways, and 
we have a bunch of tests doing `driver.launchTasks()` explicitly.


> On Oct. 10, 2016, 7:56 p.m., Anand Mazumdar wrote:
> > src/tests/mesos.hpp, lines 934-943
> > 
> >
> > I would prefer this to be an action to be consistent with the 
> > `LaunchTasks(...)` action we already have for the v0 API.
> > 
> > The action should be able to take an 
> > `Option`/`Option > 
> > A caller would then invoke it as:
> > 
> > ```cpp
> > EXPECT_CALL(*scheduler, offers(_, _))
> >   .WillOnce(LaunchTasks(executorInfo, taskGroup));
> > ```

Binding `LaunchTasks` with a custom action is fine, but we should also allow 
doning it explicitly. That is what we are currently doing.
We can grab a lot of them by `driver.launchTasks(offer.id(), {task});`.


- Gilbert


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


On Oct. 10, 2016, 6:15 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52718/
> ---
> 
> (Updated Oct. 10, 2016, 6:15 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added v1 api helpers to create calls in 'tests/mesos.hpp'.
> 
> 
> Diffs
> -
> 
>   src/tests/mesos.hpp 5e15f5164cddebd60bb6d9856ae63ad357643cb2 
> 
> Diff: https://reviews.apache.org/r/52718/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 52311: Pass the user value from executor of switch_user flag is set.

[Sivaram Kannan]

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

(Updated Oct. 13, 2016, 6:52 p.m.)


Review request for mesos and Joseph Wu.


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


Repository: mesos


Description
---

Pass the user value from executor of switch_user flag is set.


Diffs (updated)
-

  src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 

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


Testing
---

1. Run the mesos-logrotate-logger with un-priviledged user and verify whether 
the logs are getting rotated. 
2. Run the mesos-logrotate-logger as root user and verify whether the logs are 
getting rotated.


Thanks,

Sivaram Kannan

Re: Review Request 52251: Added test cases for TCP health check.

[haosdent huang]

> On Oct. 13, 2016, 3:02 p.m., Gastón Kleiman wrote:
> > src/tests/health_check_tests.cpp, line 1679
> > 
> >
> > This test is flaky, because the master will sometimes not listen on 
> > 127.0.0.1.
> > 
> > I think that this test should use `nc` like the Docker tests.

Let's discuss at https://reviews.apache.org/r/52250/


- haosdent


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


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52251/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6279
> https://issues.apache.org/jira/browse/MESOS-6279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for TCP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52251/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52250: Added test cases for HTTP health check.

[haosdent huang]

> On Oct. 13, 2016, 3:04 p.m., Gastón Kleiman wrote:
> > src/tests/health_check_tests.cpp, line 1516
> > 
> >
> > This test is flaky, because the master will sometimes not listen on 
> > 127.0.0.1.
> > 
> > I think that this test should use `nc` like the Docker tests.

Hi, @gaston Good point! It is flaky as you said.

We could test it via nc but nc is flaky as well when @alexr test 
HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP.

So I add a test helper to our health check test cases don't depends on nc.

But another simple solution is we make agent bind to 0.0.0.0 as this email said 
http://search-hadoop.com/m/0Vlr6SPE4d1hepk5


- haosdent


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


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52250/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52250/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52250: Added test cases for HTTP health check.

[haosdent huang]

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




src/tests/health_check_tests.cpp (line 1516)


Hi, @gaston Good point! It is flaky as you said.

We could test it via `nc` but `nc` is flaky as well when @alexr test 
`HealthCheckTest.ROOT_DOCKER_DockerHealthyTaskViaTCP`.

So I add [a test helper](https://reviews.apache.org/r/52786/) to our health 
check test cases don't depends on `nc`.

But another simple solution is we make `agent` bind to `0.0.0.0` as this 
email said http://search-hadoop.com/m/0Vlr6SPE4d1hepk5


- haosdent huang


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52250/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52250/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[Kevin Klues]

> On Oct. 13, 2016, 5:52 p.m., Kevin Klues wrote:
> > This does not work for me. I still see:
> > 
> > ```
> > [klueska@core-dev build]$ ./src/mesos-execute --master=127.0.0.1:5050 
> > --name="test-single-1" --command="sleep 2000"
> > I1013 10:47:08.270723 38083 scheduler.cpp:176] Version: 1.1.0
> > I1013 10:47:08.274955 38096 scheduler.cpp:465] New master detected at 
> > master@127.0.0.1:5050
> > Subscribed with ID fe29dfdc-07f0-49c4-8c28-9ade62291388-0001
> > Submitted task 'test-single-1' to agent 
> > 'c81be7b6-da4e-4bac-a884-df27a32f245b-S0'
> > Received status update TASK_FAILED for task 'test-single-1'
> >   message: 'Failed to launch container: Failed to make the containerizer 
> > runtime directory 
> > '/var/run/mesos/containers/8e3a559a-5b13-4460-bb7f-d1e107ab49f0': 
> > Permission denied; Abnormal executor termination: unknown container'
> >   source: SOURCE_AGENT
> >   reason: REASON_CONTAINER_LAUNCH_FAILED
> > ```
> > 
> > Digging into the code in `local.cpp`, it looks like we need to add some 
> > code in there to actually set the agent flag appropriately (just like we do 
> > for `slaveFlags.work_dir`).
> 
> haosdent huang wrote:
> Oh, I see. Because 
> 
> ```
> +export MESOS_RUNTIME_DIR=/tmp/mesos
> ```
> 
> in `mesos-local-flags.sh.in` and
> 
> ```
> Try load = slaveFlags.load("MESOS_");
> 
> ```
> 
> in `local.cpp`, it works although the code in `local.cpp` have not been 
> updated. But I think it should work in your side as well? How you launch the 
> `mesos-local`? Do you use `./bin/mesos-local.sh`?

Yes, I ran the commands exactly as you specified. However, I didn't rerun 
`configure`, which is why I wasn't seeing the change (since 
`mesos-local-flags.sh` is generated at configure time). Either way, we need to 
update `local.cpp` to handle the case when this environment variable isn't set 
and we specify it via the flag to mesos-local itself.


- Kevin


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


On Oct. 13, 2016, 5:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> Test produce 
> 
> ```
> # Run with normal user.
> $ ./bin/mesos-local.sh
> 
> # Open a new session
> $ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
> --command="sleep 2000"
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W1014 00:05:22.457461 36489 scheduler.cpp:161]
> **
> Scheduler driver bound to loopback interface! Cannot communicate with remote 
> master(s). You might want to set 'LIBPROCESS_IP' environment variable to use 
> a routable IP address.
> **
> I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
> I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
> master@127.0.0.1:5050
> Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
> Submitted task 'test-single-1' to agent 
> '8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
> Received status update TASK_RUNNING for task 'test-single-1'
>   source: SOURCE_EXECUTOR
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

> On Oct. 13, 2016, 5:52 p.m., Kevin Klues wrote:
> > This does not work for me. I still see:
> > 
> > ```
> > [klueska@core-dev build]$ ./src/mesos-execute --master=127.0.0.1:5050 
> > --name="test-single-1" --command="sleep 2000"
> > I1013 10:47:08.270723 38083 scheduler.cpp:176] Version: 1.1.0
> > I1013 10:47:08.274955 38096 scheduler.cpp:465] New master detected at 
> > master@127.0.0.1:5050
> > Subscribed with ID fe29dfdc-07f0-49c4-8c28-9ade62291388-0001
> > Submitted task 'test-single-1' to agent 
> > 'c81be7b6-da4e-4bac-a884-df27a32f245b-S0'
> > Received status update TASK_FAILED for task 'test-single-1'
> >   message: 'Failed to launch container: Failed to make the containerizer 
> > runtime directory 
> > '/var/run/mesos/containers/8e3a559a-5b13-4460-bb7f-d1e107ab49f0': 
> > Permission denied; Abnormal executor termination: unknown container'
> >   source: SOURCE_AGENT
> >   reason: REASON_CONTAINER_LAUNCH_FAILED
> > ```
> > 
> > Digging into the code in `local.cpp`, it looks like we need to add some 
> > code in there to actually set the agent flag appropriately (just like we do 
> > for `slaveFlags.work_dir`).

Oh, I see. Because 

```
+export MESOS_RUNTIME_DIR=/tmp/mesos
```

in `mesos-local-flags.sh.in` and

```
Try load = slaveFlags.load("MESOS_");

```

in `local.cpp`, it works although the code in `local.cpp` have not been 
updated. But I think it should work in your side as well? How you launch the 
`mesos-local`? Do you use `./bin/mesos-local.sh`?


- haosdent


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


On Oct. 13, 2016, 5:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> Test produce 
> 
> ```
> # Run with normal user.
> $ ./bin/mesos-local.sh
> 
> # Open a new session
> $ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
> --command="sleep 2000"
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W1014 00:05:22.457461 36489 scheduler.cpp:161]
> **
> Scheduler driver bound to loopback interface! Cannot communicate with remote 
> master(s). You might want to set 'LIBPROCESS_IP' environment variable to use 
> a routable IP address.
> **
> I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
> I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
> master@127.0.0.1:5050
> Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
> Submitted task 'test-single-1' to agent 
> '8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
> Received status update TASK_RUNNING for task 'test-single-1'
>   source: SOURCE_EXECUTOR
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52676: Made stout's tests a phony target.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On Oct. 10, 2016, 7:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52676/
> ---
> 
> (Updated Oct. 10, 2016, 7:54 a.m.)
> 
> 
> Review request for mesos, Kapil Arya and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the stout root folder contains a folder `tests/` so the automake
> build target `tests` conflicts that one. This e.g., leads to the
> `tests` target no being built unless a user executes e.g.,
> 
> % make check
> 
> Make `tests` a phony target to avoid the name conflict.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am fda069d0f7d75ca80b56eadac415cba57afb03f7 
> 
> Diff: https://reviews.apache.org/r/52676/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52677: Added all test dependency to libprocess' tests target.

[Kapil Arya]

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


Ship it!




Ship It!

- Kapil Arya


On Oct. 10, 2016, 7:54 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52677/
> ---
> 
> (Updated Oct. 10, 2016, 7:54 a.m.)
> 
> 
> Review request for mesos, Kapil Arya and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added all test dependency to libprocess' tests target.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 020b0e1e4c49805a3c7a61b308d897fc82f5e0ce 
> 
> Diff: https://reviews.apache.org/r/52677/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[Kevin Klues]

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



This does not work for me. I still see:

```
[klueska@core-dev build]$ ./src/mesos-execute --master=127.0.0.1:5050 
--name="test-single-1" --command="sleep 2000"
I1013 10:47:08.270723 38083 scheduler.cpp:176] Version: 1.1.0
I1013 10:47:08.274955 38096 scheduler.cpp:465] New master detected at 
master@127.0.0.1:5050
Subscribed with ID fe29dfdc-07f0-49c4-8c28-9ade62291388-0001
Submitted task 'test-single-1' to agent 
'c81be7b6-da4e-4bac-a884-df27a32f245b-S0'
Received status update TASK_FAILED for task 'test-single-1'
  message: 'Failed to launch container: Failed to make the containerizer 
runtime directory 
'/var/run/mesos/containers/8e3a559a-5b13-4460-bb7f-d1e107ab49f0': Permission 
denied; Abnormal executor termination: unknown container'
  source: SOURCE_AGENT
  reason: REASON_CONTAINER_LAUNCH_FAILED
```

Digging into the code in `local.cpp`, it looks like we need to add some code in 
there to actually set the agent flag appropriately (just like we do for 
`slaveFlags.work_dir`).

- Kevin Klues


On Oct. 13, 2016, 5:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> Test produce 
> 
> ```
> # Run with normal user.
> $ ./bin/mesos-local.sh
> 
> # Open a new session
> $ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
> --command="sleep 2000"
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W1014 00:05:22.457461 36489 scheduler.cpp:161]
> **
> Scheduler driver bound to loopback interface! Cannot communicate with remote 
> master(s). You might want to set 'LIBPROCESS_IP' environment variable to use 
> a routable IP address.
> **
> I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
> I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
> master@127.0.0.1:5050
> Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
> Submitted task 'test-single-1' to agent 
> '8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
> Received status update TASK_RUNNING for task 'test-single-1'
>   source: SOURCE_EXECUTOR
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 52832: Corrected usage of "it's" in Mesos.

[Neil Conway]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Corrected usage of "it's" in Mesos.


Diffs
-

  docs/app-framework-development-guide.md 
b419cb4a2cc85cb903af91378d7291e4966bb879 
  docs/oversubscription.md b4e487310f83234b6182b5c4d1e2962936b054fe 
  include/mesos/executor.hpp 10be62a86627e3157e8f439b03e139c486e8b161 
  include/mesos/log/log.hpp 3edb9f11dcdc41aa7ad953763df9ab1367708d76 
  src/java/src/org/apache/mesos/Executor.java 
d80ccac77e784f0bfa9376b69f2ff2c4a169fe8a 
  src/linux/cgroups.hpp 3c9d9e21c8332668286e4170be711aa260c12c1d 
  src/local/local.cpp 2be5bcf4f6fc102b8cd85eee4dc4cb689f1850f0 
  src/log/replica.cpp 5ab7b3d55988a9a95e70f900f95746bc12dd3da0 
  src/master/main.cpp 4a1a8e70ab0535aa131681b2b09a99e51c20158e 
  src/slave/containerizer/mesos/containerizer.cpp 
cc9e2bc020be0b0c127ce57e7aa04ca4391126c1 
  src/slave/containerizer/mesos/linux_launcher.cpp 
e8edfd2ccb123e5853b51f121a881d864498802e 
  src/slave/main.cpp 949a738ab3e44d8efc878e59c482a750ab41d1e4 
  src/tests/hierarchical_allocator_tests.cpp 
cae582e8693bcb5695a53e1f6484e421d31875a6 
  src/tests/scheduler_http_api_tests.cpp 
80a2ef0af9a4c67deaef40e1f36343868ee4428f 
  src/zookeeper/group.cpp 65a6d05bd379dd7542507ada8e602f855f037b37 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 52831: Corrected usage of "it's" in stout.

[Neil Conway]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Corrected usage of "it's" in stout.


Diffs
-

  3rdparty/stout/README.md 172150c586cd4020edc4dbdabffd1306c9177694 
  3rdparty/stout/include/stout/os/linux.hpp 
fd9b0edad5670944ff760b3029fcf62e2dbedd0b 
  3rdparty/stout/include/stout/os/posix/killtree.hpp 
8ba21dedd793d3819ad2bc5674eda3ad41f9295a 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 52830: Corrected usage of "it's" in libprocess.

[Neil Conway]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Corrected usage of "it's" in libprocess.


Diffs
-

  3rdparty/libprocess/include/process/deferred.hpp 
da87e65c7cdc431d52be771f12ff420967c419c4 
  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 

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


Testing
---

Visual inspection.


Thanks,

Neil Conway

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

> On Oct. 13, 2016, 4:59 p.m., Kevin Klues wrote:
> > Can you write the exact test procedure you used to run the command and test 
> > it?

Got it! Added.


- haosdent


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


On Oct. 13, 2016, 5:06 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 5:06 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> Test produce 
> 
> ```
> # Run with normal user.
> $ ./bin/mesos-local.sh
> 
> # Open a new session
> $ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
> --command="sleep 2000"
> WARNING: Logging before InitGoogleLogging() is written to STDERR
> W1014 00:05:22.457461 36489 scheduler.cpp:161]
> **
> Scheduler driver bound to loopback interface! Cannot communicate with remote 
> master(s). You might want to set 'LIBPROCESS_IP' environment variable to use 
> a routable IP address.
> **
> I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
> I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
> master@127.0.0.1:5050
> Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
> Submitted task 'test-single-1' to agent 
> '8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
> Received status update TASK_RUNNING for task 'test-single-1'
>   source: SOURCE_EXECUTOR
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

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

(Updated Oct. 13, 2016, 5:06 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Updated `Testing done`


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


Repository: mesos


Description
---

Set `runtime_dir` to a temporary folder in `mesos-local`.


Diffs
-

  bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
  src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 

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


Testing (updated)
---

To void this error when launch `mesos-local` without `sudo`

```
  message: 'Failed to launch container: Failed to make the containerizer 
runtime directory 
'/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
denied; Abnormal executor termination: unknown container'
```

Test produce 

```
# Run with normal user.
$ ./bin/mesos-local.sh

# Open a new session
$ ./src/mesos-execute --master=127.0.0.1:5050 --name="test-single-1" 
--command="sleep 2000"
WARNING: Logging before InitGoogleLogging() is written to STDERR
W1014 00:05:22.457461 36489 scheduler.cpp:161]
**
Scheduler driver bound to loopback interface! Cannot communicate with remote 
master(s). You might want to set 'LIBPROCESS_IP' environment variable to use a 
routable IP address.
**
I1014 00:05:22.457690 36489 scheduler.cpp:176] Version: 1.1.0
I1014 00:05:22.596534 36497 scheduler.cpp:465] New master detected at 
master@127.0.0.1:5050
Subscribed with ID 51e0d33f-d7b2-4c01-9bb3-10731cc7d1ab-
Submitted task 'test-single-1' to agent 
'8fba7ecb-b70e-48ac-9541-2070c4d9b1d5-S0'
Received status update TASK_RUNNING for task 'test-single-1'
  source: SOURCE_EXECUTOR
```


Thanks,

haosdent huang

Re: Review Request 52814: Added notes to the getting started and upgrade docs about --runtime_dir.

[Kevin Klues]

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

(Updated Oct. 13, 2016, 5:04 p.m.)


Review request for mesos, Alexander Rukletsov, Jie Yu, Till Toenshoff, and 
Vinod Kone.


Changes
---

Rebased on master.


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


Repository: mesos


Description
---

Added notes to the getting started and upgrade docs about --runtime_dir.


Diffs (updated)
-

  docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
  docs/upgrades.md 8e45fdbb485829930f9b918d1d6f24893228e873 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[Kevin Klues]

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



Can you write the exact test procedure you used to run the command and test it?

- Kevin Klues


On Oct. 13, 2016, 6:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jie Yu]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.
> 
> Jiang Yan Xu wrote:
> As in, a custom executor but called via the agent <-> CommandExecutor 
> "protocol"? Is this an argument that the removal of `--launch_dir` flag 
> should go through a deprecation cycle? Could you elaborate?
> 
> Alexander Rukletsov wrote:
> Yes, this is what I meant.
> 
> Another reason is that we probably should maintain upgradability among 
> all 1.x version, i.e. N -> N + m upgrades, at least this is how I read the 
> versioning doc. With this in mind, we can't simply remove the flag.
> 
> Gastón Kleiman wrote:
> I'm dropping this issue, since it seems like we agree that keeping 
> backwards-compatibility is useful for N -> N+1 upgrades.
> 
> If someone wants to shepherd me, I volunteer to make `executor.cpp` fully 
> move to use env variables instead of a mix of flags and env variables.

> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?

I don't understand why env variable can address this issue? If the evn is 
optional, then, it won't fail (same as the flag is optional). If the env in not 
optional, will the task still fail?


- Jie


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


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52773: Added a loopback test with CNI ptp plugin.

[Avinash sridharan]

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

(Updated Oct. 13, 2016, 4:45 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Refactored the code.


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


Repository: mesos


Description
---

Added a loopback test with CNI ptp plugin.


Diffs (updated)
-

  src/tests/containerizer/cni_isolator_tests.cpp 
b032c4345683813bca5f9a5eec09f73d860299cc 

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


Testing
---

make and sudo make check for the loopback test.


Thanks,

Avinash sridharan

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Jie Yu]

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




docs/executor-http-api.md (line 352)


I agree with YanX that we probably should use flags in command executor and 
default executor. The benefit is that environment variables can be picked up by 
flags load as well (so that we can avoid os::getenv calls in the executor).

Also, I think we should call it launcher_dir because runtime_dir has 
different meanings in Mesos (especially mesos containerizer, see the new agent 
flag runtime_dir).


- Jie Yu


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52787]

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 Oct. 13, 2016, 6:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45967: Added documentation for shareable resources.

[Jiang Yan Xu]

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


Ship it!




I have some additional comments but let's check this in first and we can 
iterate on the doc later.

- Jiang Yan Xu


On Oct. 12, 2016, 10:25 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45967/
> ---
> 
> (Updated Oct. 12, 2016, 10:25 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for shareable resources.
> 
> 
> Diffs
> -
> 
>   docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
>   docs/shared-resources.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45967/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52803: Changed agent to send TASK_GONE.

[Neil Conway]

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

(Updated Oct. 13, 2016, 3:25 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak test.


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


Repository: mesos


Description
---

The agent previously sent TASK_LOST updates for tasks that are killed
for various reasons, such as containerizer errors or QoS preemption. The
agent now sends TASK_GONE to partition-aware frameworks instead.


Diffs (updated)
-

  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
  src/tests/oversubscription_tests.cpp b356fb62a4e068bc171a75a76001c6d0e76af92a 
  src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52723: Changed agent to send TASK_DROPPED during reconciliation.

[Neil Conway]

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

(Updated Oct. 13, 2016, 3:10 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment.


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


Repository: mesos


Description
---

If a framework attempts to launch a task but the launch message is
dropped after it reaches the master but before it reaches the slave, the
failed launch will be detected during master <-> agent reconciliation
when the agent re-registers. Previously, the agent would generate a
TASK_LOST status update for such dropped tasks; now it will generate
TASK_DROPPED if the framework is partition-aware.

Note that we'll only send TASK_DROPPED if the agent is running a
sufficiently recent version of Mesos (>= 1.1.0). That means that in a
mixed cluster where the master has been upgraded to Mesos 1.1 but some
of the agents have not been, a partition-aware framework might still see
TASK_LOST in this situation.


Diffs (updated)
-

  src/messages/messages.proto 7d65be1418864333d0c4213e2e0df0374f9ec115 
  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 
  src/tests/master_slave_reconciliation_tests.cpp 
2983c1b074c2d4179e95e619083f5dd4e9ac6730 
  src/tests/slave_recovery_tests.cpp 703948f7a6861a4401ee45ce9cae2644106083f3 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52250: Added test cases for HTTP health check.

[Gastón Kleiman]

> On Oct. 13, 2016, 1:18 p.m., Gastón Kleiman wrote:
> > src/tests/health_check_tests.cpp, line 1506
> > 
> >
> > Shouldn't this test also use netcat?

I moved this comment to the right RR.


- Gastón


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


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52250/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52250/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52250: Added test cases for HTTP health check.

[Gastón Kleiman]

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




src/tests/health_check_tests.cpp (line 1516)


This test is flaky, because the master will sometimes not listen on 
127.0.0.1.

I think that this test should use `nc` like the Docker tests.


- Gastón Kleiman


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52250/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52250/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52251: Added test cases for TCP health check.

[Gastón Kleiman]

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




src/tests/health_check_tests.cpp (line 1679)


This test is flaky, because the master will sometimes not listen on 
127.0.0.1.

I think that this test should use `nc` like the Docker tests.


- Gastón Kleiman


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52251/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6279
> https://issues.apache.org/jira/browse/MESOS-6279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for TCP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52251/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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 Oct. 13, 2016, 2:33 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 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Review Request 52827: Added backend suffix to image layer rootfs path.

[Qian Zhang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Previously image layer rootfs path is in the format below regardless
of which backend is used.
  /layers//rootfs
This introduced an issue: when agent is restarted with a different
backend, we will wrongly handle the whiteout files since different
backends(e.g., aufs and overlay) have different whiteout standard.

In this commit, we added backend suffix to image layer rootfs path
for non-aufs backend like below, so each backend will only handle
its own image layers.
  /layers//rootfs.
For aufs backend, it is still in the previous format, this is to
handle backward compatibility.

So the expected result of this commit is:
1. If user switches backend when restarting agent, all the image
layers of the previous backend in the store will just be ignored.
2. In the upgrade case, if user starts a new version of Mesos agent
with a non-aufs backend (i.e., copy/bind/overlay), then all the
image layers in the store will just be ignored, but if user starts
the agent with aufs backend, all the image layers in the store can
still be used.


Diffs
-

  src/slave/containerizer/mesos/provisioner/docker/local_puller.cpp 
9b09dca5cd8f9e60a90cf574300b250eb18ede35 
  src/slave/containerizer/mesos/provisioner/docker/metadata_manager.cpp 
6456d90220a4026696a04f9969763cf682464353 
  src/slave/containerizer/mesos/provisioner/docker/paths.hpp 
949e0c16a361f22b00e267fcf81093690327041f 
  src/slave/containerizer/mesos/provisioner/docker/paths.cpp 
a5cc952072274c61e8c515e8b1b7ea563344e950 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
1e2cc2dda3518d99ef1ad06e2b8ea7f78a4dcf72 
  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
52b9ea934a1dbe3312b8f08f5da8085e9e306de5 
  src/tests/containerizer/provisioner_docker_tests.cpp 
10fbc4149ac2e7503ffe7f2746fbd0e14a2365b4 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 52828: Allow the chown code in fetcher to be executed.

[Megha Sharma]

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

(Updated Oct. 13, 2016, 2:33 p.m.)


Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Moved the uri.size() check in fetcher so that the chown to
task user of stdout/stderr in sandbox directory happens even
when there is no uri to be fetched.


Diffs (updated)
-

  src/slave/containerizer/fetcher.cpp 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/slave_tests.cpp 93b81d3e5b90d8036409e943f598c865fe335bcc 

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


Testing
---


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 Oct. 13, 2016, 2:30 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 11104d66e6dd05d8eb1d37a2e3250aca19278110 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
ad2070246b7fdb90aa6ed02326b5a7eb95365997 

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


Testing
---

make check on linux and macos


Thanks,

Megha Sharma

Re: Review Request 52693: Changed master to send TASK_UNKNOWN during reconciliation.

[Neil Conway]

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

(Updated Oct. 13, 2016, 2:25 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Tweak comment.


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


Repository: mesos


Description
---

Previously, the master would send TASK_LOST in response to explicit
reconciliation requests for (a) unknown tasks at registered slaves and
(b) tasks at unknown slaves (neither registered nor unreachable). The
master will now send TASK_UNKNOWN for these situations if the framework
has the PARTITION_AWARE capability.


Diffs (updated)
-

  src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
  src/tests/partition_tests.cpp 12fe8593ff17c35d540f944c428cf7f33b7dcbb3 
  src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52664: Moved the `decimalFloat` filter to app.js for consistency.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52817, 52470, 52520, 52471, 52664]

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 Oct. 13, 2016, 6:37 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52664/
> ---
> 
> (Updated Oct. 13, 2016, 6:37 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved the `decimalFloat` filter to app.js for consistency.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
>   src/webui/master/static/js/controllers.js 
> 29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 
> 
> Diff: https://reviews.apache.org/r/52664/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52083: Changed reconciliation for unregistering, reregistering agents.

[Neil Conway]

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

(Updated Oct. 13, 2016, 2:10 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Clarify comments, commit description.


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


Repository: mesos


Description (updated)
---

Previously, explicit reconciliation for an agent that was in the process
of reregistering or unregistering returned no results. This degree of
cleverness seems unwarranted: if the agent hasn't completed the
reregistration or unregistration process, it seems simpler for the
master to return the previous state of the agent. This is what the
framework would observe if their reconcile request lost the race with
the reregister/unregister operation, anyway.

Note that since reregistering agents are no longer considered to be "in
transition", we need to slightly adjust the rules for how we update the
`slaves.recovered` collection in the master: an agent remains in the
"recovered" collection until it has been marked reachable in the
registry (rather than removing it from "recovered" as soon as the
reregistration process beings). This is more consistent with how we
manage the other collections in the master anyway: an agent appears in
the `recovered` list until the registry operation that reregisters it
has been successfully applied.


Diffs (updated)
-

  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp 7ef898781ce5c2349ffeaa3ce43e68dede19c852 
  src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
  src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 

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


Testing
---

`make check` on OSX, Linux.


Thanks,

Neil Conway

Re: Review Request 52083: Changed reconciliation for unregistering, reregistering agents.

[Neil Conway]

> On Oct. 12, 2016, 12:54 a.m., Vinod Kone wrote:
> > don't quite follow the second para in the description. you say agent is no 
> > longer in transitioning while re-registering but then you want keep it in 
> > recovered map which results in the `transitioning()` to return true? am i 
> > reading it right?

The proposal rule is: reconciliation should ignore the effect of in-progress 
operations (since they might not succeed). So if an agent was in `recovered` 
but has not finished reregistration, reconciliation should treat it as 
`recovered` but not yet `reregistered` -- i.e., it should be `transitioning()` 
and reconciliation should return no results. So if the agent was recovered from 
the registry but has not yet finished reregistration, explicit reconciliation 
will be a no-op; if the agent was marked unreachable but has not yet finished 
reregistration, explicit reconciliation will return `TASK_UNREACHABLE`.


> On Oct. 12, 2016, 12:54 a.m., Vinod Kone wrote:
> > src/master/master.cpp, line 1894
> > 
> >
> > hmm. this means an agent could be in `recovered` map and 
> > `reregistering` or `markingUnreachable` maps at the same time. didn't we 
> > decide to not do that? are you still planning to do that cleanup?

This is intentional. An agent appears in `recovered` if it has been recovered 
from the registrar and it has EITHER: (a) completed re-registration, or (b) 
completed being marked unreachable. The `reregistering` and 
`markingUnreachable` collections track whether there is an in-progress 
operation attempting to do (a) or (b), respectively. We only updated 
`recovered` when the in-progress operation has completed successfully.


- Neil


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


On Sept. 20, 2016, 3:26 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52083/
> ---
> 
> (Updated Sept. 20, 2016, 3:26 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6206
> https://issues.apache.org/jira/browse/MESOS-6206
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, explicit reconciliation for an agent that was in the process
> of reregistering or unregistering returned no results. This degree of
> cleverness seems unwarranted: if the agent hasn't completed the
> reregistration or unregistration process, it seems quite reasonable for
> the master to return the previous state of the agent (this is what the
> framework would observe if their reconcile request lost the race with
> the reregister/unregister, anyway).
> 
> Note that since reregistering agents are no longer considered to be "in
> transition", we need to slightly adjust the rules for how we update the
> `slaves.recovered` collection in the master: an agent remains in the
> "recovered" collection until it has been marked reachable in the
> registry (rather than removing it from "recovered" as soon as the
> reregistration process beings). This is more consistent with how we
> manage the other collections in the master anyway.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 35db198748b8652eb53e17f592f6b40d1e6a3ed9 
>   src/master/master.cpp 66a672f6d16233e96b29e330a9e6c474546fa851 
>   src/tests/master_tests.cpp e6c8362da6a5669e2a2d18f6eb4e454365a84f60 
>   src/tests/reconciliation_tests.cpp 1412090299df388456f04ed58a1d384ce3ff550a 
> 
> Diff: https://reviews.apache.org/r/52083/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX, Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 52250: Added test cases for HTTP health check.

[Gastón Kleiman]

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




src/tests/health_check_tests.cpp (line 1506)


Shouldn't this test also use netcat?


- Gastón Kleiman


On Oct. 12, 2016, 2:37 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52250/
> ---
> 
> (Updated Oct. 12, 2016, 2:37 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Bugs: MESOS-6278
> https://issues.apache.org/jira/browse/MESOS-6278
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test cases for HTTP health check.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52250/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45967: Added documentation for shareable resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45963, 45967]

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 Oct. 13, 2016, 5:25 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45967/
> ---
> 
> (Updated Oct. 13, 2016, 5:25 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4325
> https://issues.apache.org/jira/browse/MESOS-4325
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for shareable resources.
> 
> 
> Diffs
> -
> 
>   docs/home.md ad59eb1722b49cd72454795c095ddbd1ec1eb4dd 
>   docs/shared-resources.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45967/diff/
> 
> 
> Testing
> ---
> 
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52783: Added documentation for mesos-containerizer Linux capabilities support.

[Benjamin Bannier]

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

(Updated Oct. 13, 2016, 2:22 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated.


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


Repository: mesos


Description
---

Added documentation for mesos-containerizer Linux capabilities support.


Diffs (updated)
-

  docs/mesos-containerizer.md 76544625838845fe2817347fea483a60ab250f99 

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


Testing
---

Checked with local renderer.


Thanks,

Benjamin Bannier

Re: Review Request 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Gastón Kleiman]

> On Oct. 10, 2016, 3:32 p.m., Jiang Yan Xu wrote:
> > src/launcher/executor.cpp, lines 903-910
> > 
> >
> > You didn't start this but I wonder why we can't consistently use the 
> > flags, which can be provided through environment variables, i.e., use 
> > `flags.load("MESOS_", , );` above.
> > 
> > That question aside, I don't think we need to be concerned about 
> > backwards-compatibility here: this is the command executor bundled with the 
> > agent so they are upgraded together. i.e., we are sure that the agent is 
> > setting the new variable.
> 
> Gastón Kleiman wrote:
> Wouldn't then the tasks started between the moment in which the binary is 
> overwritten and the moment in which the Mesos Agent is restarted fail?
> 
> Jiang Yan Xu wrote:
> OK I guess it'll be helpful in this regard. My main point is that this is 
> not a deprecation cycle concern.
> 
> Generally it's advisable to restart the agent right after upgrading it 
> but even though it's done this way with most Mesos toolings I have seen, I've 
> also seen tasks failing when launched mid-upgrade with libmesos and 
> executable mismatch. 
> 
> Therefore, fine with me to do this to aid N -> N+1 upgrade.
> 
> If we do this, the cleaniest approach may be flag aliases.
> 
> Alexander Rukletsov wrote:
> Technically, people may overwrite the mesos-executor binary.
> 
> Jiang Yan Xu wrote:
> As in, a custom executor but called via the agent <-> CommandExecutor 
> "protocol"? Is this an argument that the removal of `--launch_dir` flag 
> should go through a deprecation cycle? Could you elaborate?
> 
> Alexander Rukletsov wrote:
> Yes, this is what I meant.
> 
> Another reason is that we probably should maintain upgradability among 
> all 1.x version, i.e. N -> N + m upgrades, at least this is how I read the 
> versioning doc. With this in mind, we can't simply remove the flag.

I'm dropping this issue, since it seems like we agree that keeping 
backwards-compatibility is useful for N -> N+1 upgrades.

If someone wants to shepherd me, I volunteer to make `executor.cpp` fully move 
to use env variables instead of a mix of flags and env variables.


- Gastón


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


On Oct. 11, 2016, 6:52 p.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52556/
> ---
> 
> (Updated Oct. 11, 2016, 6:52 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Anand Mazumdar, Jie Yu, and 
> Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6288
> https://issues.apache.org/jira/browse/MESOS-6288
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This environment variable points to the directory containing the
> binaries used by the Mesos Agent. It will eventually replace the
> `--launcher-dir` executor flag.
> 
> 
> Diffs
> -
> 
>   docs/executor-http-api.md 50b4cb4efff48bcc56330b81bd7c4c217b8a22b5 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
>   src/launcher/executor.cpp 3e95d6029bea0ce6e0dfb39c24b795fe98d90d13 
>   src/slave/slave.cpp 119fb36c27b25739f2a86a55d48e964ca4a84ff7 
> 
> Diff: https://reviews.apache.org/r/52556/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 49571: Added a benchmark test for allocations.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45962, 49571]

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 Oct. 13, 2016, 5:24 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49571/
> ---
> 
> (Updated Oct. 13, 2016, 5:24 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5771
> https://issues.apache.org/jira/browse/MESOS-5771
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocations test has the following resource configurations:
> (1) REGULAR: Offers from every slave have regular resources.
> (2) SHARED: Offers from every slave include a shared resource.
> (3) REGULAR: Offers from every alternate slave contain only regular
> resources; and offers from every other alternate slave contains
> a shared resource.
> 
> This test is parameterized based on number of agents, number of
> frameworks and resource configuration.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 46553f6d501deb37862dd5ba48be1c114f6e6cb8 
> 
> Diff: https://reviews.apache.org/r/49571/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> Allocations benchmark test results
> ==
> Support of shared resources has a small impact (roughly 10%) on runtime 
> performance in allocations as compared to HEAD (without shared resources). 
> Also, there is no visible impact in performance when shared resources are 
> added in the tests.
> 
> Following is a snapshot with 1000 agents and 200 frameworks.
> 
> With the patch (and no shared resources)
> 
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6907us
> Added 1000 agents in 2.057098secs
> round 0 allocate() took 1.689164secs to make 1000 offers
> round 50 allocate() took 1.672373secs to make 1000 offers
> round 100 allocate() took 1.680571secs to make 1000 offers
> round 150 allocate() took 1.674683secs to make 1000 offers
> round 199 allocate() took 1.671525secs to make 1000 offers
> 
> With the patch (and shared resources on all agents)
> ---
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/10
> Using 1000 agents and 200 frameworks with resource type 1
> Added 200 frameworks in 6888us
> Added 1000 agents in 2.096218secs
> round 0 allocate() took 1.704491secs to make 1000 offers
> round 50 allocate() took 1.718623secs to make 1000 offers
> round 100 allocate() took 1.716224secs to make 1000 offers
> round 150 allocate() took 1.707343secs to make 1000 offers
> round 199 allocate() took 1.727467secs to make 1000 offers
> 
> With the patch (and shared resources on alternate agents)
> -
> [ RUN  ] 
> AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/11
> Using 1000 agents and 200 frameworks with resource type 2
> Added 200 frameworks in 7304us
> Added 1000 agents in 2.071009secs
> round 0 allocate() took 1.689045secs to make 1000 offers
> round 50 allocate() took 1.691524secs to make 1000 offers
> round 100 allocate() took 1.688873secs to make 1000 offers
> round 150 allocate() took 1.688713secs to make 1000 offers
> round 199 allocate() took 1.691223secs to make 1000 offers
> 
> Based on HEAD, with all regular resources (no shared resources in HEAD 
> supported)
> -
> [ RUN  ] AllResources/HierarchicalAllocations_BENCHMARK_Test.Allocations/9
> Using 1000 agents and 200 frameworks with resource type 0
> Added 200 frameworks in 6801us
> Added 1000 agents in 1.721447secs
> round 0 allocate() took 1.502953secs to make 1000 offers
> round 50 allocate() took 1.520157secs to make 1000 offers
> round 100 allocate() took 1.517221secs to make 1000 offers
> round 150 allocate() took 1.526446secs to make 1000 offers
> round 199 allocate() took 1.538005secs to make 1000 offers
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Yubo Li]

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

(Updated 十月 13, 2016, 10:15 a.m.)


Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

Wrapped helper functions to 'Docker::Device' to handle data
parsing and serializing between 'Docker::Device' structure and
string.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50125: Added mesos-docker-executor support for devices control.

[Yubo Li]

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

(Updated 十月 13, 2016, 10:15 a.m.)


Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

Added a new flag '--devices' to mesos-docker-executor, and gave its
feature to control devices exposition, isolation, and access permission.
Also, passed GPUs assignment to mesos-docker-executor.


Diffs (updated)
-

  src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
  src/docker/executor.hpp a49ec1b4045116741af6af08578791fb0440ad8f 
  src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
  src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Yubo Li]

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

(Updated 十月 13, 2016, 10:14 a.m.)


Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and Rajat 
Phull.


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


Repository: mesos


Description
---

Added control logic to allocate/deallocate GPUs to GPU-related task
when the task is started/terminated.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
  src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50123: Added GPU scheduler for docker containerizer process.

[Yubo Li]

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

(Updated 十月 13, 2016, 10:13 a.m.)


Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.


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


Repository: mesos


Description
---

This added 'NvidiaGpuAllocator' to docker containerizer process so that
docker containerizer process is ready to use it to allocate GPUs to task
with 'gpus' resource.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
  src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
  src/slave/containerizer/mesos/isolators/gpu/allocator.hpp 
b2eabfebef99ccebef427d144bb816adc0175ecf 
  src/slave/containerizer/mesos/isolators/gpu/components.hpp 
82fca5de07c28720f102a02a77563e325e98e066 
  src/tests/mock_docker.hpp 1bf09c8dba020b421526b650523c879fb87380f8 
  src/tests/mock_docker.cpp 6a0e613bde6889864a37ffd7ec0b454e5fe4df1c 

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


Testing
---

make check


Thanks,

Yubo Li

Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Yubo Li]

> On 十月 9, 2016, 10:37 a.m., Guangya Liu wrote:
> > src/docker/docker.hpp, lines 95-120
> > 
> >
> > What about moving this to https://reviews.apache.org/r/50125/ where 
> > this will be used.
> 
> Yubo Li wrote:
> This function is also used by another fix in `src/docker/docker.cpp`. I 
> prefer to leave it here because if we move it, this patch only adds `<<` 
> function and seems a little strange. Your opinion?
> 
> Guangya Liu wrote:
> Which fix depend on it? Why cannot move it to /r/50125 ?
> 
> Yubo Li wrote:
> Yes, we can move it to `/r/50125`. If we do this, I suggest to merge this 
> patch to `/r/50125` directly, because here is only `<<` function remained in 
> this patch, how do you think?

There is no problem now.


- Yubo


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


On 十月 11, 2016, 8:16 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated 十月 11, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped helper functions to 'Docker::Device' to handle data
> parsing and serializing between 'Docker::Device' structure and
> string.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50128: Added helper functions to 'Docker::Device'.

[Yubo Li]

> On 十月 9, 2016, 10:37 a.m., Guangya Liu wrote:
> > src/docker/docker.hpp, lines 95-120
> > 
> >
> > What about moving this to https://reviews.apache.org/r/50125/ where 
> > this will be used.
> 
> Yubo Li wrote:
> This function is also used by another fix in `src/docker/docker.cpp`. I 
> prefer to leave it here because if we move it, this patch only adds `<<` 
> function and seems a little strange. Your opinion?
> 
> Guangya Liu wrote:
> Which fix depend on it? Why cannot move it to /r/50125 ?

Yes, we can move it to `/r/50125`. If we do this, I suggest to merge this patch 
to `/r/50125` directly, because here is only `<<` function remained in this 
patch, how do you think?


- Yubo


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


On 十月 11, 2016, 8:16 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50128/
> ---
> 
> (Updated 十月 11, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Wrapped helper functions to 'Docker::Device' to handle data
> parsing and serializing between 'Docker::Device' structure and
> string.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
> 
> Diff: https://reviews.apache.org/r/50128/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Review Request 52818: Ensured agent is registered in SlaveEndpointTests.

[Benjamin Bannier]

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

Review request for mesos and Anand Mazumdar.


Repository: mesos


Description
---

Ensured agent is registered in SlaveEndpointTests.


Diffs
-

  src/tests/slave_authorization_tests.cpp 
6bd2aa96cb05aa087c0807cc3dd1830451efe106 

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


Testing
---

`make check` (OS X, clang-3.9 w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 52814: Added notes to the getting started and upgrade docs about --runtime_dir.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [52814]

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

Error:
2016-10-13 08:32:52 URL:https://reviews.apache.org/r/52814/diff/raw/ 
[2616/2616] -> "52814.patch" [1]
error: patch failed: docs/upgrades.md:208
error: docs/upgrades.md: patch does not apply

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

- Mesos ReviewBot


On Oct. 13, 2016, 1:51 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52814/
> ---
> 
> (Updated Oct. 13, 2016, 1:51 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, Till Toenshoff, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-6312
> https://issues.apache.org/jira/browse/MESOS-6312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added notes to the getting started and upgrade docs about --runtime_dir.
> 
> 
> Diffs
> -
> 
>   docs/getting-started.md de3fba4a821ca7fa9b9e2fd692b33b40edd35b68 
>   docs/upgrades.md 7250259590db322121a0f30d1cdb0f3732a7a6ee 
> 
> Diff: https://reviews.apache.org/r/52814/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Yubo Li]

> On 十月 11, 2016, 3:40 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 2150-2151
> > 
> >
> > Here will cause failure if gpu is not enabled, the ideal logic could be:
> > 
> > ```
> > if (container has gpu) {
> >   deallocateNvidiaGpus(containerId)
> > .onAny(defer(self(), ::destroy, containerId, killed, 
> > status));
> > } else {
> >   destroy(...);
> > }
> > ```
> > 
> > Or else you can follow the `allocateGpus` logic.
> > 
> > ```
> > Future deallocateGpus = Nothing();
> > 
> > if (container has gpu) {
> >   deallocateGpus = deallocateNvidiaGpus(...);
> > }
> > 
> > deallocateGpus(containerId)
> >   .onAny(defer(self(), ::destroy, containerId, killed, 
> > status));
> > ```

Fixed by following `allocateGpus` logic.


- Yubo


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


On 十月 11, 2016, 8:16 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50841/
> ---
> 
> (Updated 十月 11, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added control logic to allocate/deallocate GPUs to GPU-related task
> when the task is started/terminated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
> 
> Diff: https://reviews.apache.org/r/50841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Yubo Li]

> On 十月 12, 2016, 7:31 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, lines 643-696
> > 
> >
> > I was thinking for how to make less code change but need to make sure 
> > build passed on mac.
> > 
> > How about the following?
> > 
> > ```
> > Future DockerContainerizerProcess::allocateNvidiaGpus(
> > const size_t count,
> > const ContainerID& containerId)
> > {
> > #ifdef __linux__
> >   if (!nvidiaGpuAllocator.isSome()) {
> > return Failure("The 'allocateNvidiaGpus' function was called"
> >" without an 'NvidiaGpuAllocator' set");
> >   }
> > 
> >   if (!containers_.contains(containerId)) {
> > return Failure("Container is already destroyed");
> >   }
> > 
> >   return nvidiaGpuAllocator->allocate(count)
> > .then(defer(
> > self(),
> > ::_allocateNvidiaGpus,
> > lambda::_1,
> > containerId));
> > #else
> >   return Nothing();
> > #endif
> > }
> > 
> > 
> > Future DockerContainerizerProcess::_allocateNvidiaGpus(
> > const set& allocated,
> > const ContainerID& containerId)
> > {
> > #ifdef __linux__
> >   if (!containers_.contains(containerId)) {
> > return nvidiaGpuAllocator->deallocate(allocated)
> >   .onFailed([](const string& message) {
> > return Failure("Failed to deallocate GPUs allocated"
> >" to destroyed container: " + message);
> >   });
> >   }
> > 
> >   containers_.at(containerId)->gpus = allocated;
> > #endif
> > 
> >   return Nothing();
> > }
> > 
> > 
> > Future DockerContainerizerProcess::deallocateNvidiaGpus(
> > const ContainerID& containerId)
> > {
> > #ifdef __linux__
> >   if (!nvidiaGpuAllocator.isSome()) {
> > return Failure("The 'deallocateNvidiaGpus' function was called"
> >" without an 'NvidiaGpuAllocator' set");
> >   }
> > 
> >   return 
> > nvidiaGpuAllocator->deallocate(containers_.at(containerId)->gpus)
> > .then(defer(
> > self(),
> > ::_deallocateNvidiaGpus,
> > containerId));
> > #else
> >   return Nothing();
> > #endif
> > }
> > ```
> > 
> > Then we need to update the comments of `allocateNvidiaGpus` and 
> > `deallocateNvidiaGpus` by identifying that those two APIs return Nothing if 
> > GPU is not enabled.

Fixed. Also added comments `// GPU is only supported on Linux, and it will 
return 'Nothing()' if the OS is not Linux.`


- Yubo


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


On 十月 11, 2016, 8:16 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50841/
> ---
> 
> (Updated 十月 11, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added control logic to allocate/deallocate GPUs to GPU-related task
> when the task is started/terminated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
> 
> Diff: https://reviews.apache.org/r/50841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

> On Oct. 13, 2016, 5:48 a.m., Kevin Klues wrote:
> > I should have looked at this more carefully before giving it a Ship it. It 
> > seemed reasonable at first glance though.
> > 
> > Did you attempt to compile it before posting the patch? Looks like there 
> > were errors with the flags definition.
> > 
> > ```
> > In file included from ../../src/local/main.cpp:26:0:
> > ../../src/local/flags.hpp: In constructor 
> > 'mesos::internal::local::Flags::Flags()':
> > ../../src/local/flags.hpp:54:10: error: 'runtime_dir' is not a member of 
> > 'mesos::internal::local::Flags'
> >  add(::runtime_dir,
> > ```

My bad that compile wrong branch when test. Now fixed, may you help review this 
again? Thank you in advance.


- haosdent


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


On Oct. 13, 2016, 6:44 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 13, 2016, 6:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-6380
> https://issues.apache.org/jira/browse/MESOS-6380
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set `runtime_dir` to a temporary folder in `mesos-local`.
> 
> 
> Diffs
> -
> 
>   bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
>   src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 
> 
> Diff: https://reviews.apache.org/r/52787/diff/
> 
> 
> Testing
> ---
> 
> To void this error when launch `mesos-local` without `sudo`
> 
> ```
>   message: 'Failed to launch container: Failed to make the containerizer 
> runtime directory 
> '/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
> denied; Abnormal executor termination: unknown container'
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52787: Set `runtime_dir` to a temporary folder in `mesos-local`.

[haosdent huang]

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

(Updated Oct. 13, 2016, 6:44 a.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

Updated the patch.


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


Repository: mesos


Description
---

Set `runtime_dir` to a temporary folder in `mesos-local`.


Diffs (updated)
-

  bin/mesos-local-flags.sh.in 5b4553a808dc9f34a15390e69b2f85e95761ec53 
  src/local/flags.hpp c77eff13e7a63630ff6c9428e57f3f6707e1f30f 

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


Testing
---

To void this error when launch `mesos-local` without `sudo`

```
  message: 'Failed to launch container: Failed to make the containerizer 
runtime directory 
'/var/run/mesos/containers/f2d6947f-2916-4f1a-90dc-3d137b360b9c': Permission 
denied; Abnormal executor termination: unknown container'
```


Thanks,

haosdent huang

Re: Review Request 52471: Fixed the wrong sandbox directory of the tasks in Web UI.

[haosdent huang]

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

(Updated Oct. 13, 2016, 6:37 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

For the task launched by default-executor, its sandbox directory is
'executor_directory/tasks/task_id/'. This patch generates the
corresponding sandbox directory in Web UI for the task according to its
executor type.


Diffs (updated)
-

  src/webui/master/static/agent_executor.html 
8b83ed5acc2ecd56f20b1571878ec9f6794efbd2 
  src/webui/master/static/framework.html 
bc3c56adf22030e6cd1dcd8a2c945d44ff79aa4b 
  src/webui/master/static/home.html 179cb15140f85811df0ea0a87620dd3c90dd30c7 
  src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
  src/webui/master/static/js/controllers.js 
29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52664: Moved the `decimalFloat` filter to app.js for consistency.

[haosdent huang]

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

(Updated Oct. 13, 2016, 6:37 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


Repository: mesos


Description
---

Moved the `decimalFloat` filter to app.js for consistency.


Diffs (updated)
-

  src/webui/master/static/js/app.js 400a4286d9358c699453326d87a0bd49dbb105a7 
  src/webui/master/static/js/controllers.js 
29a5a1c8754cc2fb934854750d6dfb04f1eaeae4 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52520: Exposed the executor's type in the endpoints.

[haosdent huang]

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

(Updated Oct. 13, 2016, 6:36 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Exposed the executor's type in the endpoints.


Diffs (updated)
-

  src/common/http.cpp 538330a4c780fbc2dfcdfb31537b0e75f368e3e0 
  src/slave/http.cpp a1fbb7aa8209c92aa59833f1df98475b46830537 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 

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


Testing
---


Thanks,

haosdent huang