Re: Review Request 52569: Updated the nested container launch to correctly determine the user.

[Gilbert Song]

> On Oct. 12, 2016, 11:46 a.m., Gilbert Song wrote:
> > src/slave/http.cpp, lines 1964-1965
> > 
> >
> > Would you mind removing this `TODO`? They are no longer valid.

I will follow up with a patch. Sorry not review on time.


- Gilbert


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


On Oct. 5, 2016, 1:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52569/
> ---
> 
> (Updated Oct. 5, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-6289
> https://issues.apache.org/jira/browse/MESOS-6289
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This locates the executor that corresponds to the parent container,
> and uses the user that was used to launch the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52569/diff/
> 
> 
> Testing
> ---
> 
> Now that the executor is looked up, the tests have been updated to
> launch an executor and use it's container id as the parent.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52569: Updated the nested container launch to correctly determine the user.

[Gilbert Song]

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




src/slave/http.cpp (lines 1964 - 1965)


Would you mind removing this `TODO`? They are no longer valid.


- Gilbert Song


On Oct. 5, 2016, 1:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52569/
> ---
> 
> (Updated Oct. 5, 2016, 1:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-6289
> https://issues.apache.org/jira/browse/MESOS-6289
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This locates the executor that corresponds to the parent container,
> and uses the user that was used to launch the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52569/diff/
> 
> 
> Testing
> ---
> 
> Now that the executor is looked up, the tests have been updated to
> launch an executor and use it's container id as the parent.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52786, 52250, 52251, 52560, 52561, 52563]

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. 12, 2016, 2:38 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52563/
> ---
> 
> (Updated Oct. 12, 2016, 2:38 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Gastón Kleiman.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Replace `Testing` to `Tests` in comments.
> * Remove redundant `.Times(1)`.
> * Fix typos.
> 
> 
> Diffs
> -
> 
>   src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
> 
> Diff: https://reviews.apache.org/r/52563/diff/
> 
> 
> Testing
> ---
> 
> ```
> sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest.*" 
> --gtest_also_run_disabled_tests
> [--] Global test environment tear-down
> [==] 19 tests from 1 test case ran. (35264 ms total)
> [  PASSED  ] 19 tests.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52412: Supported logger with nested containers in Mesos Containerizer.

[Gilbert Song]

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

(Updated Oct. 12, 2016, 11:39 a.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, and 
Joseph Wu.


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


Repository: mesos


Description
---

Currently, there is an issue in mesos containerizer using logger
for nested contaienrs:

An empty executorinfo is passed to logger when launching a nested
container, it would potentially break some logger modules if any
module tries to access the required proto field (e.g., executorId).

We should pass the ExecutorInfo of a nested container's top level
parent container to the logger when launching a nested container.


Diffs (updated)
-

  include/mesos/slave/container_logger.hpp 
939974736f9eb744c83036e074718d2a1eba8b0a 
  src/slave/containerizer/mesos/containerizer.cpp 
7ec6f78c739145c874fb5ee63c5034dfb838c17c 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 52647: Fix new sign comparison errors produced by hardened flags

[Neil Conway]

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



As mentioned elsewhere, can you split stout and libprocess changes into 
separate reviews?


3rdparty/libprocess/src/decoder.hpp (line 244)


Whitespace looks wonky.



3rdparty/libprocess/src/decoder.hpp (line 438)


Whitespace looks wonky.


- Neil Conway


On Oct. 11, 2016, 10:43 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52647/
> ---
> 
> (Updated Oct. 11, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6239
> https://issues.apache.org/jira/browse/MESOS-6239
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The hardening flags produced many new sign comparison errors that need to be 
> fixed for Mesos to compile/run.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/decoder.hpp c79296b 
>   3rdparty/libprocess/src/encoder.hpp 005d1cc 
>   3rdparty/libprocess/src/process.cpp f1d746c 
>   3rdparty/libprocess/src/tests/decoder_tests.cpp 4535614 
>   3rdparty/libprocess/src/tests/encoder_tests.cpp 9e57375 
>   3rdparty/libprocess/src/tests/http_tests.cpp 8d6c8c4 
>   3rdparty/libprocess/src/tests/io_tests.cpp b85c79f 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp c8350cf 
>   3rdparty/stout/tests/cache_tests.cpp 0950c85 
>   3rdparty/stout/tests/flags_tests.cpp 94ba915 
>   3rdparty/stout/tests/hashmap_tests.cpp 2626d67 
>   3rdparty/stout/tests/hashset_tests.cpp 66e59db 
>   3rdparty/stout/tests/ip_tests.cpp 59e69a5 
>   3rdparty/stout/tests/json_tests.cpp 2bc4c88 
>   3rdparty/stout/tests/linkedhashmap_tests.cpp 7a80769 
>   3rdparty/stout/tests/multimap_tests.cpp 488991b 
>   3rdparty/stout/tests/os/process_tests.cpp 4977d02 
>   3rdparty/stout/tests/os/sendfile_tests.cpp e221689 
>   3rdparty/stout/tests/os/systems_tests.cpp 110ba5b 
>   3rdparty/stout/tests/os_tests.cpp 6a7b836 
>   3rdparty/stout/tests/strings_tests.cpp 7dd3301 
> 
> Diff: https://reviews.apache.org/r/52647/diff/
> 
> 
> Testing
> ---
> 
> Made sure compilation, tests, and benchmarks worked with both gcc and clang. 
> Ran `make && make check && make bench`.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52740: Refactored some code into a separate function.

[Neil Conway]

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

(Updated Oct. 12, 2016, 7:34 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Refactored some code into a separate function.


Diffs (updated)
-

  src/slave/slave.hpp d8c5873bb7380b8449f8f344e85689519c467251 
  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 

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


Testing
---

`make check`


Thanks,

Neil Conway

Review Request 52801: Changed description of TASK_GONE.

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Changed description of TASK_GONE.


Diffs
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 

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


Testing
---

`make check`


Thanks,

Neil Conway

Re: Review Request 52746: Changed agent to send TASK_DROPPED for task launch failures.

[Neil Conway]

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

(Updated Oct. 12, 2016, 7:35 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Rebase.


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


Repository: mesos


Description
---

If the agent cannot launch a task due to a variety of possible error
conditions, we now send TASK_DROPPED to partition-aware frameworks
rather than TASK_LOST.


Diffs (updated)
-

  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 

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


Testing
---

`make check`.

Note that none of these code paths appear to have unit tests, so I couldn't 
update the unit tests to ensure that `TASK_DROPPED` is sent appropriately. We 
should add unit tests for at least some of these situations, if feasible -- but 
I'll defer that for now.


Thanks,

Neil Conway

Review Request 52798: Windows: Implemented os::execvpe with _spawnvpe.

[Joseph Wu]

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

Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Jie 
Yu, and Joris Van Remoortere.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
0ababb4ff76d55d82b8d528f54c99bd4b231b21b 
  3rdparty/stout/include/stout/windows/os.hpp 
7d6530e37e389a314185e5aaa85d8096a28b9c41 

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


Testing
---

Windows: msbuild

This fixes the Windows build.


Thanks,

Joseph Wu

Re: Review Request 52798: Windows: Implemented os::execvpe with _spawnvpe.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Oct. 12, 2016, 7:20 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52798/
> ---
> 
> (Updated Oct. 12, 2016, 7:20 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Alex Clemmer, Jie 
> Yu, and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-6323
> https://issues.apache.org/jira/browse/MESOS-6323
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> 0ababb4ff76d55d82b8d528f54c99bd4b231b21b 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 7d6530e37e389a314185e5aaa85d8096a28b9c41 
> 
> Diff: https://reviews.apache.org/r/52798/diff/
> 
> 
> Testing
> ---
> 
> Windows: msbuild
> 
> This fixes the Windows build.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 52802: Added a new slave metric, "tasks_gone".

[Neil Conway]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added a new slave metric, "tasks_gone".


Diffs
-

  src/slave/metrics.hpp d4432136590a3021f6743c79e98db64cb044118a 
  src/slave/metrics.cpp 86eb8db644227cb593e52305bfbd05444bb87a6e 
  src/slave/slave.cpp 6bd9b49c3bbdb973a0d03552ae8fe55b33371083 

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


Testing
---

`make check`


Thanks,

Neil Conway

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/
---

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/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 40266: Libprocess Reinit: Cleanup SocketManager alongside ProcessManager.

[Joseph Wu]

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

(Updated Oct. 12, 2016, 1:48 p.m.)


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


Changes
---

* Added a guard in `ProcessManager::finalize` to prevent any further `spawn`s.  
This removes the need for logic in `~ProcessManager`.
* Added a bunch of comments for clarification of why some things are done in 
some order.
* Changed `ProcessManager::finalize` to terminated by UPID.
* Changed the order of `Clock::finalize`


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


Repository: mesos


Description
---

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

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


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 

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


Testing
---

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


Thanks,

Joseph Wu

Re: Review Request 51769: Implemented `delegate` method.

[Avinash sridharan]

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

(Updated Oct. 12, 2016, 8:54 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Implemented `delegate` method.


Diffs (updated)
-

  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 04736451481506f17c462b35fa579f6546c6d6ce 

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


Testing
---

make


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. 12, 2016, 8:55 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
 04736451481506f17c462b35fa579f6546c6d6ce 

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

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. 12, 2016, 8:55 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
 04736451481506f17c462b35fa579f6546c6d6ce 

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 52412: Supported logger with nested containers in Mesos Containerizer.

[Gilbert Song]

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

(Updated Oct. 12, 2016, 2:06 p.m.)


Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, and 
Joseph Wu.


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


Repository: mesos


Description
---

Currently, there is an issue in mesos containerizer using logger
for nested contaienrs:

An empty executorinfo is passed to logger when launching a nested
container, it would potentially break some logger modules if any
module tries to access the required proto field (e.g., executorId).

We should pass the ExecutorInfo of a nested container's top level
parent container to the logger when launching a nested container.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
7ec6f78c739145c874fb5ee63c5034dfb838c17c 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 40410: Libprocess Reinit: Move MetricsProcess instantiation into process.cpp.

[Joseph Wu]

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

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


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


Changes
---

Rename `metrics_singleton` to `metrics`.  Some whitespace tweaks.  Added 
`process::initialize` in a couple places.


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


Repository: mesos


Description
---

The metrics singleton must be unified with `process::initialize` so 
that it also falls under the scope of reinitialization.  The singleton 
must also not be guarded by `Once`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/metrics/metrics.hpp 
54487ab2614c7f8e8df10d1e0c39880a6cf5bde3 
  3rdparty/libprocess/src/metrics/metrics.cpp 
4ab3ac7fb7688df937ff62d496d9104628324018 
  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 
  3rdparty/libprocess/src/tests/system_tests.cpp 
0f4d0424689522337806ba2227ec4330c700e17e 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Re: Review Request 40413: Libprocess Reinit: Move ReaperProcess instantiation into process.cpp.

[Joseph Wu]

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

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


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


Changes
---

Renamed `reaper_singleton` to `reaper`.  Some whitespace tweaks.


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


Repository: mesos


Description
---

The reaper singleton must be unified with `process::initialize` so 
that it also falls under the scope of reinitialization.  The singleton 
must also not be guarded by `Once`.


Diffs (updated)
-

  3rdparty/libprocess/include/process/reap.hpp 
d7e0fa381df63a9ca7d438d5cea0631e1b0ec7ee 
  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 
  3rdparty/libprocess/src/reap.cpp 5fc2a4d67a3a6fe56005fc2c2d16d4983d53b83a 

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

Re: Review Request 51049: Libprocess Reinit: Removed authorizer callback leaks.

[Joseph Wu]

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

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


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


Changes
---

Rebase on the large change in the previous review.


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


Repository: mesos


Description
---

The `authorization_callbacks` were never being freed after being set
or unset.  If you run the tests in repetition, this leaks memory.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 

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


Testing
---

make

(More testing done later in the chain)


Thanks,

Joseph Wu

Re: Review Request 40512: Libprocess Reinit: Add a test-only method to reinitialize libprocess.

[Joseph Wu]

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

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


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


Changes
---

Rebase and whitespace tweak.


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


Repository: mesos


Description
---

This builds upon earlier changes to complete `process::finalize`.  
Tests which need to reconfigure libprocess, such as some SSL-related 
tests, can use `process::reinitialize` to do so.  

This method may also be useful for providing additional isolation 
between tests, such as cleaning up all processes after each test case.


Diffs (updated)
-

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

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


Testing
---

TODO: Define `process::reinitialize` in several tests, such as `HTTPTest`, 
`MetricsTest`, and `ReapTest` and call `process::reinitialize` before and after 
tests.


Thanks,

Joseph Wu

Re: Review Request 50621: Libprocess reinit: Moved HttpProxy finalization and destruction.

[Joseph Wu]

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

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


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


Changes
---

Rebase on the large change in the prior-prior review.


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


Repository: mesos


Description
---

Moves the destructor code in `HttpProxy` into the `Process::finalize`
function.  And changes the `HttpProxy`s termination logic to 
terminate via `UPID` which guards against double-termination.

Removes some unused initialization code, too.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp f1d746c52cfe659f5cd7da4b7a6424ff585619a3 

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


Testing
---

make

(More testing done later in the chain)


Thanks,

Joseph Wu

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[Vinod Kone]

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




src/master/validation.cpp (line 1115)


need to add a validation here to ensure taskInfo.executor() is same as 
executor for all tasks in the task group.



src/tests/master_validation_tests.cpp 


Keep this test and modify it to make sure the operation is rejected 
ifExecutorinfo inside TaskInfo doesn't match the top level Executorinfo.


- Vinod Kone


On Oct. 12, 2016, 5:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[Joseph Wu]

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

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


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


Changes
---

Renamed `metrics_singleton` to `metrics`.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

Tests done in a subsequent review.


Thanks,

Joseph Wu

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. 12, 2016, 9:38 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 52412: Supported logger with nested containers in Mesos Containerizer.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On Oct. 12, 2016, 2:06 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52412/
> ---
> 
> (Updated Oct. 12, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, Joris Van Remoortere, 
> and Joseph Wu.
> 
> 
> Bugs: MESOS-6290
> https://issues.apache.org/jira/browse/MESOS-6290
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, there is an issue in mesos containerizer using logger
> for nested contaienrs:
> 
> An empty executorinfo is passed to logger when launching a nested
> container, it would potentially break some logger modules if any
> module tries to access the required proto field (e.g., executorId).
> 
> We should pass the ExecutorInfo of a nested container's top level
> parent container to the logger when launching a nested container.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 7ec6f78c739145c874fb5ee63c5034dfb838c17c 
> 
> Diff: https://reviews.apache.org/r/52412/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 52771: Captured the `stderr` during execution of CNI plugin.

[Avinash sridharan]

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

Review request for mesos, Jie Yu and Qian Zhang.


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


Repository: mesos


Description
---

Till now we were capturing only the `stdout` of the CNI plugin. However,
during errors it makes sense to add the output from `stderr` for the
CNI plugin as well, since as per the CNI spec the CNI plugin is
supposed to output all unstructured output (debugs for example) to
`stderr`. This greatly helps in debugging the exact reason for CNI
plugin failures.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
70f30831819d7a0e6233fcb13a703dc6981324b6 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
1b22b28825e8160f659c3cbac37cc576f01666d5 

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


Testing
---

make and make check.


Thanks,

Avinash sridharan

Review Request 52772: Added a filter for '_CNIPLUGINS_'.

[Avinash sridharan]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added a filter for '_CNIPLUGINS_'.


Diffs
-

  src/tests/environment.cpp 970136f98c601c4fbc71e8ffbf506bebe89c3658 

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


Testing
---

make and make check


Thanks,

Avinash sridharan

Re: Review Request 52756: Invoke the shutdown executor callback for checkpointed frameworks.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52755, 52756]

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. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52756/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6370
> https://issues.apache.org/jira/browse/MESOS-6370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the executor library used to commit suicide after the
> recovery timeout without invoking the executor's shutdown callback.
> This behavior was not consistent with the executor driver.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp a0e5d8382fa3886b747ae92507dce17e75c2d749 
> 
> Diff: https://reviews.apache.org/r/52756/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Guangya Liu]

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




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.


- Guangya Liu


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 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. 12, 2016, 9:29 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 79061c3cd94d856ec695e5a82bf6792bf089d1f8 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[haosdent huang]

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

(Updated Oct. 12, 2016, 9:29 a.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

Address @vinodkone's comments.


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


Repository: mesos


Description
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 

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


Testing (updated)
---


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. 12, 2016, 9:29 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. 12, 2016, 9:29 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

Review Request 52779: Reimplement test macros without (non-standard) statement expressions.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

Reimplement test macros without (non-standard) compound statements.

In GCC, compound statements that are wrapped in parentheses are treated
as expresssions. For example, `({int x = 3; x;})` would return 3. This
is useful, for example, in macros, when you don't want to explicitly
define a function that returns a value.

Unfortunately, this is non-standard, and not supported at all by MSVC.
Since our tests depend heavily on this construct, we must re-write all
of them from scratch before we can light them up on Windows.

This commit will port the subset of testing constructs that use this
feature, which we need in order to add Windows support for the tests we
care about.


Diffs
-

  src/tests/api_tests.cpp f0bafd1d5207a687ca0489b05ed1acdc2a6d875d 
  src/tests/authentication_tests.cpp fc7afae0abde8e9274685b9a2121f5a91d59f3d6 
  src/tests/command_executor_tests.cpp 6e47243941626bb5b6224430f9a12ced8a3f5062 
  src/tests/container_logger_tests.cpp f76117230e0517ddc3cb8e0bf482085fad6950d2 
  src/tests/containerizer/cgroups_isolator_tests.cpp 
b5797574ca2bfe3a005c80a3ba6bb6965c54cc95 
  src/tests/containerizer/cni_isolator_tests.cpp 
b032c4345683813bca5f9a5eec09f73d860299cc 
  src/tests/containerizer/cpu_isolator_tests.cpp 
b16cde3c3ddb47859aec2d0496df4970def7c89f 
  src/tests/containerizer/docker_containerizer_tests.cpp 
6d26797abf6d2b5e42b9e7743789e1edc62c9c1a 
  src/tests/containerizer/docker_volume_isolator_tests.cpp 
ca7bffd3b1773a11a4679d114885d3edd977b02b 
  src/tests/containerizer/linux_capabilities_isolator_tests.cpp 
f040c209b4b4c87cef00b0569b7da7581f4ccf03 
  src/tests/containerizer/linux_filesystem_isolator_tests.cpp 
eb191a32381f9d1ca84ec29adf352dde375c2f2d 
  src/tests/containerizer/memory_isolator_tests.cpp 
62cffa748b1d18d28ad1118f2b26c1caac3f623a 
  src/tests/containerizer/memory_pressure_tests.cpp 
42ae3232a5e0c508382ba3be97c4ee9787151236 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
fea1f9f0a03373692ef2a6dd2bc5722dc6f46d5b 
  src/tests/containerizer/port_mapping_tests.cpp 
fbdc0db9238c85d2f6eaba7d13ee5ce23342b527 
  src/tests/containerizer/provisioner_appc_tests.cpp 
a999fc7991da805dfbcdf5659fcfb762aee5b2b9 
  src/tests/containerizer/provisioner_docker_tests.cpp 
10fbc4149ac2e7503ffe7f2746fbd0e14a2365b4 
  src/tests/containerizer/runtime_isolator_tests.cpp 
c0c11607024b6a80d5bf5a486b91f7905a9083d7 
  src/tests/containerizer/xfs_quota_tests.cpp 
27d1f9f64ba3000154ccbe3e9734be33fe7f32c0 
  src/tests/credentials_tests.cpp 1ceb31b72446a87771705cab2255c264d8e00c08 
  src/tests/default_executor_tests.cpp 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
  src/tests/disk_quota_tests.cpp 90c42b301d749e7a9c6d6d42a51e929ec66d761c 
  src/tests/dynamic_weights_tests.cpp 6f1e249e51e41aee7fdb22a2ccbfa9be71774e6d 
  src/tests/exception_tests.cpp 7b66bd1b0beb004641fe0a08e02db168f2653354 
  src/tests/executor_http_api_tests.cpp 
a9f1a7b0498acd541c6f58ad1388da49c9951e22 
  src/tests/fault_tolerance_tests.cpp 5a9944cf459ab688907d95bbda09f464b37efd1e 
  src/tests/fetcher_cache_tests.cpp 03e817d46194d451eceb70f4cebb54dfdcb4c2e7 
  src/tests/files_tests.cpp f6a6aeaa4efa05957d1038c564157216fce70246 
  src/tests/gc_tests.cpp d9776b602bee780b9732cd93d06b9c8f3cc8a4d0 
  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 
  src/tests/hook_tests.cpp d334d6c5ff7f966d55b395bfbf4f25ee3fa2 
  src/tests/http_fault_tolerance_tests.cpp 
57ef562058f8abf9256e2ab8a4a85b36b5a7add4 
  src/tests/logging_tests.cpp 886dcd0d6d6ccf509c98578e8b86b1c6ecd5775f 
  src/tests/master_allocator_tests.cpp bb94e38d5bb472801366c172cfc036f2eecdcbcb 
  src/tests/master_authorization_tests.cpp 
a4623d15c246651fd1038fdedf16321b1d5f273f 
  src/tests/master_contender_detector_tests.cpp 
2a7d713f74c907235f82d83eaf46630046645faf 
  src/tests/master_maintenance_tests.cpp 
77eb405ab7314da906bed9ec1d0018c24928d8d8 
  src/tests/master_quota_tests.cpp 48be7406181646c8cc1d169b82a4a4ca71cdf03b 
  src/tests/master_slave_reconciliation_tests.cpp 
2983c1b074c2d4179e95e619083f5dd4e9ac6730 
  src/tests/master_tests.cpp 88cf1e612ad8186ef2cea161b3a52b0df9517305 
  src/tests/master_validation_tests.cpp 
99e350e0587e73e9ee25ef20dd369cd146bd446a 
  src/tests/mesos.hpp 5e15f5164cddebd60bb6d9856ae63ad357643cb2 
  src/tests/mesos.cpp 2aae160fb941ab3672a5665ae27f517ff40600e2 
  src/tests/metrics_tests.cpp 27fc35f3bc47fc2905f4ce400b1051307dcacb2e 
  src/tests/oversubscription_tests.cpp b356fb62a4e068bc171a75a76001c6d0e76af92a 
  src/tests/partition_tests.cpp 12fe8593ff17c35d540f944c428cf7f33b7dcbb3 
  src/tests/persistent_volume_endpoints_tests.cpp 
f35592ae311f49d3e331fa78c8e427f34bf5 
  src/tests/persistent_volume_tests.cpp 

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

[Guangya Liu]

> 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?

Which fix depend on it? Why cannot move it to /r/50125 ?


- Guangya


---
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 52556: Added the MESOS_RUNTIME_DIRECTORY executor env variable.

[Alexander Rukletsov]

> 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?

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.


- Alexander


---
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 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. 12, 2016, 9:32 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

Review Request 52778: Added Windows support to Agent test environment harness.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat and Joseph Wu.


Repository: mesos


Description
---

Added Windows support to Agent test harness.

Several files required to run any of the Agent tests currently can't be
compiled by MSVC. The individual reasons are mostly small, incidental
details -- missing headers, assumption of POSIX paths, and so on. The
one big exception to this is that the `main` function of the test binary
itself isn't setting up the WSA stack and making sure the right SEH
callbacks are present.

This commit addresses all of these issues.


Diffs
-

  src/sched/sched.cpp 9d1b5ce2e1a179b2e6ea212d99d8d7fe72a0793a 
  src/scheduler/scheduler.cpp e282d419119dd1f01e170acf5cc2c6175b59791d 
  src/tests/containerizer.hpp 940c4146f4e854a6b1b9ccaba5687e76d5723cba 
  src/tests/environment.cpp 970136f98c601c4fbc71e8ffbf506bebe89c3658 
  src/tests/main.cpp e1507bae8267a10c85e631d10f243f299ff81dc9 
  src/tests/mock_docker.hpp 1bf09c8dba020b421526b650523c879fb87380f8 
  src/tests/utils.cpp fc004a9c567898ffbc38a42cc2340dd57347a829 

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


Testing
---


Thanks,

Alex Clemmer

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52637, 52638, 52765, 52639]

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. 12, 2016, 12:18 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52639/
> ---
> 
> (Updated Oct. 12, 2016, 12:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Xiaojian Huang.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test for `recovered` AgentID and `AGENT_ADDED` after reregister.
> 
> 
> Diffs
> -
> 
>   src/tests/api_tests.cpp 39f3f6641cfb4b2e05cbaa1e8156e8a2f480f87a 
> 
> Diff: https://reviews.apache.org/r/52639/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[haosdent huang]

> On Oct. 12, 2016, 12:08 a.m., Vinod Kone wrote:
> > src/master/master.cpp, lines 3529-3555
> > 
> >
> > I think we might want to mutate TaskInfo here so that authorization can 
> > use that information.
> > 
> > More importantly, we want pending tasks to have ExecutorInfo for the 
> > operator endpoints right? This is one of the main reasons we want to mutate 
> > TaskInfo right?
> > 
> > 
> > ```
> > const RepeatedPtrField& tasks = [&]() {
> >   if (operation.type() == Offer::Operation::LAUNCH) {
> > return operation.launch().task_infos();
> >   } else if (operation.type() == Offer::Operation::LAUNCH_GROUP) {
> > // We mutate TaskInfo to include ExecutorInfo to make it easy
> > // for operator API and WebUI to get access to the corresponding
> > // executor for task group tasks.
> > for (int i = 0; i < operation.launch_group().tasks().size(); ++i) {
> >   TaskInfo* task = 
> > operation.launch_group().mutable_task_group()->mutable_tasks(i);
> >   if (!task->has_executor()) {
> > 
> > task->mutable_executor()->CopyFrom(operation.launch_group().executor());
> >   }
> > }
> > 
> > return operation.launch_group().task_group().tasks();
> >   }
> > UNREACHABLE();
> >   }();
> > ```

Now I mutate this at the entrypoint of `accept` because of `_accpet` need to 
access the `TaskInfo` with `ExecutorInfo` (Call `Master::addTask`) as well.


- haosdent


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


On Oct. 12, 2016, 9:29 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 9:29 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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/#review152287
---




src/docker/docker.hpp (lines 107 - 123)


Instead of defining it here, what about move this definition to docker.cpp?

You can define the declariation at #365 as

```
std::ostream& operator<<(std::ostream& stream, const Docker::Device& 
device);
```

Then implement the definition at the end of docker.cpp.

```
std::ostream& operator<<(std::ostream& stream, const Docker::Device& device)
{
  stream << device.hostPath.string() << ":";
  stream << device.containerPath.string() << ":";

  if (device.access.read) {
stream << "r";
  }
  if (device.access.write) {
stream << "w";
  }
  if (device.access.mknod) {
stream << "m";
  }

  return stream;
}
```

You can take a look at 
https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/gpu/allocator.hpp#L86
 for detail.



src/docker/docker.cpp (line 395)


You already got the error here, so I would propose the message as:

```
return Error("Failed to parse device from 'HostConfig.Devices' entry: " + 
device.error());
```



src/docker/docker.cpp (lines 754 - 756)


What about keep here not touched?


- Guangya Liu


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 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[haosdent huang]

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

(Updated Oct. 12, 2016, 10:02 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
---

This fixed the navigate error in Web UI because Web UI uses the
executor id of the task to search the corresponding sandbox directory.
Web UI uses the task id as the executor id if the executor id of the
task is empty when searching the sandbox directory. It works fine when
tasks are launched by `CommandExecutor` because the executor id of the
task is equal to the task id in this case. However, when tasks are
launched by `DefaultExecutor`, the executor id of the task is defined
in the framework side and may different with the task id. So we need to
fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
the Web UI uses incorrect executor id to search sandbox directory.


Diffs (updated)
-

  include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
  include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
  src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
  src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
  src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

[Jiang Yan Xu]

> On Oct. 12, 2016, 8:20 a.m., Jiang Yan Xu wrote:
> > src/tests/persistent_volume_tests.cpp, lines 1125-1128
> > 
> >
> > We have reduced the number of places that advance time to one, we can 
> > just pull the Clock::pause() down here so it's grouped together.

I overlooked a place above that also relies on the paused clock (the comment I 
added below) so ignore this.


- Jiang Yan


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


On Oct. 7, 2016, 8:12 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52288/
> ---
> 
> (Updated Oct. 7, 2016, 8:12 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6257
> https://issues.apache.org/jira/browse/MESOS-6257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a DESTROY of a shared volume, and that volume
> is not in use by a running or a pending task, we rescind the offers
> from frameworks to which the shared volume is present in pending offers
> so that the volume is not assigned to any task in a future ACCEPT call.
> At that time, we need to recover the resources as well for proper
> accounting of such resources by the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c7e74df71aa31edb490f4f3bd95f2d5aa94b4324 
>   src/tests/persistent_volume_tests.cpp 
> e10a79e9662530e143ccaa2aa2506a4d25158364 
> 
> Diff: https://reviews.apache.org/r/52288/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52769: Removed ports ranges benchmark test from scalar benchmark test.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52599, 50556, 50551, 51033, 50380, 52769]

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. 12, 2016, 3:25 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52769/
> ---
> 
> (Updated Oct. 12, 2016, 3:25 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5898
> https://issues.apache.org/jira/browse/MESOS-5898
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed ports ranges benchmark test from scalar benchmark test.
> 
> 
> Diffs
> -
> 
>   src/tests/resources_tests.cpp 6a12783c26f359dda835b4866b299a8fcfb3f972 
> 
> Diff: https://reviews.apache.org/r/52769/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --benchmark 
> --gtest_filter="*Resources_Scalar_BENCHMARK_Test.Arithmetic/*"
> [==] Running 3 tests from 1 test case.
> [--] Global test environment set-up.
> [--] 3 tests from 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test
> [ RUN  ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/0
> Took 56798us to perform 5 'total += r' operations on cpus(*):1; 
> gpus(*):1; mem(*):128; disk(*):256
> Took 81900us to perform 5 'total -= r' operations on cpus(*):1; 
> gpus(*):1; mem(*):128; disk(*):256
> Took 262816us to perform 5 'total = total + r' operations on cpus(*):1; 
> gpus(*):1; mem(*):128; disk(*):256
> Took 248757us to perform 5 'total = total - r' operations on cpus(*):1; 
> gpus(*):1; mem(*):128; disk(*):256
> [   OK ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/0 (651 ms)
> [ RUN  ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/1
> Took 4.685568secs to perform 10 'total += r' operations on cpus(0, 
> principal_0, {key_0: value_0}):1; gpus(...
> Took 4.865824secs to perform 10 'total -= r' operations on cpus(0, 
> principal_0, {key_0: value_0}):1; gpus(...
> Took 4.68405secs to perform 10 'total = total + r' operations on cpus(0, 
> principal_0, {key_0: value_0}):1; gpus(...
> Took 4.801695secs to perform 10 'total = total - r' operations on cpus(0, 
> principal_0, {key_0: value_0}):1; gpus(...
> [   OK ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/1 (19095 
> ms)
> [ RUN  ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/2
> Took 49434us to perform 5 'total += r' operations on cpus(*):1; 
> mem(*):128; disk(test)[persistentId:...
> Took 51654us to perform 5 'total -= r' operations on cpus(*):1; 
> mem(*):128; disk(test)[persistentId:...
> Took 322006us to perform 5 'total = total + r' operations on cpus(*):1; 
> mem(*):128; disk(test)[persistentId:...
> Took 336050us to perform 5 'total = total - r' operations on cpus(*):1; 
> mem(*):128; disk(test)[persistentId:...
> [   OK ] 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test.Arithmetic/2 (760 ms)
> [--] 3 tests from 
> ResourcesScalarOperators/Resources_Scalar_BENCHMARK_Test (20506 ms total)
> 
> [--] Global test environment tear-down
> [==] 3 tests from 1 test case ran. (20515 ms total)
> [  PASSED  ] 3 tests.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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/
---

(Updated Oct. 12, 2016, 2:37 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Use test-helper to test.


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


Repository: mesos


Description
---

Added test cases for HTTP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52561: Renamed `flags` to `agentFlags` in health check test cases.

[haosdent huang]

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

(Updated Oct. 12, 2016, 2:38 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Use test-helper to test.


Repository: mesos


Description
---

This patch renames `slave` to `agent` in health check test cases as
well.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52695: Harden libprocess

[Aaron Wood]

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




3rdparty/libprocess/Makefile.am (line 29)


Only use `-fstack-protector-strong` if we have GCC >= 4.9.


- Aaron Wood


On Oct. 11, 2016, 10:47 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52695/
> ---
> 
> (Updated Oct. 11, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> libprocess. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 020b0e1 
> 
> Diff: https://reviews.apache.org/r/52695/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 52696: Harden stout

[Aaron Wood]

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




3rdparty/stout/Makefile.am (line 26)


Only use `-fstack-protector-strong` if we have GCC >= 4.9.


- Aaron Wood


On Oct. 11, 2016, 10:47 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52696/
> ---
> 
> (Updated Oct. 11, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> stout. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/Makefile.am fda069d 
> 
> Diff: https://reviews.apache.org/r/52696/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 51715: Added a parallel gtest runner.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Oct. 11, 2016, 8:18 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51715/
> ---
> 
> (Updated Oct. 11, 2016, 8:18 a.m.)
> 
> 
> Review request for mesos, Kevin Klues and Till Toenshoff.
> 
> 
> Bugs: MESOS-6140
> https://issues.apache.org/jira/browse/MESOS-6140
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This adds `b814987b28cfb65a7c9635c83399545e423e690a` of
> https://github.com/bbannier/gtest-shrdr.
> 
> 
> Diffs
> -
> 
>   support/mesos-gtest-runner.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51715/diff/
> 
> 
> Testing
> ---
> 
> Tested with e.g., `stout-tests`
> 
> $ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests
> 
> [PASS]
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52707: Updated CLI bootstrap to search for local virtualenv installations.

[Kevin Klues]

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


Ship it!




Ship It!

- Kevin Klues


On Oct. 10, 2016, 9:42 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52707/
> ---
> 
> (Updated Oct. 10, 2016, 9:42 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A locally installed virtualenv does not always show up on the `PATH`,
> which fails `which virtualenv`.  This adds an extra search location
> based on the user's site package install directory.
> 
> 
> Diffs
> -
> 
>   src/cli_new/bootstrap 74a8568daef4e0cb93911352cc73c165a2fbea1d 
> 
> Diff: https://reviews.apache.org/r/52707/diff/
> 
> 
> Testing
> ---
> 
> pip uninstall virtualenv -y  # Global
> pip uninstall virtualenv -y  # Local
> 
> src/cli_new/bootstrap# Fails
> 
> pip install --user virtualenv
> 
> src/cli_new/bootstrap# Succeeds
> 
> pip uninstall virtualenv -y  # Local
> pip install virtualenv   # Global
> 
> src/cli_new/bootstrap# Succeeds
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 52786: Add the health check test helper.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


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


Repository: mesos


Description
---

This patch updates the test helper for the health check. Via starts a
simple HTTP server which listens on given IP and port, it makes testing
HTTP health check and TCP health check easier.


Diffs
-

  src/Makefile.am fd01e1dfbdb04d073484ff6b7cc94b8d769f8a8e 
  src/tests/CMakeLists.txt f5d66dc63143455506d8660674fbd9eb227625ff 
  src/tests/containerizer/health_check_test_helper.hpp PRE-CREATION 
  src/tests/containerizer/health_check_test_helper.cpp PRE-CREATION 
  src/tests/test_helper_main.cpp 129a5e427b72fb577e55c3756d071f72ced9ff14 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

[Jiang Yan Xu]

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


Ship it!




Committing with these cleanups.


src/tests/persistent_volume_tests.cpp (line 1117)


s/Framework 1/Framework1/ for consistency.



src/tests/persistent_volume_tests.cpp (line 1119)


s/its/their/



src/tests/persistent_volume_tests.cpp (lines 1125 - 1128)


We have reduced the number of places that advance time to one, we can just 
pull the Clock::pause() down here so it's grouped together.



src/tests/persistent_volume_tests.cpp (line 1140)


Indentation.


- Jiang Yan Xu


On Oct. 7, 2016, 8:12 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52288/
> ---
> 
> (Updated Oct. 7, 2016, 8:12 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6257
> https://issues.apache.org/jira/browse/MESOS-6257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a DESTROY of a shared volume, and that volume
> is not in use by a running or a pending task, we rescind the offers
> from frameworks to which the shared volume is present in pending offers
> so that the volume is not assigned to any task in a future ACCEPT call.
> At that time, we need to recover the resources as well for proper
> accounting of such resources by the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c7e74df71aa31edb490f4f3bd95f2d5aa94b4324 
>   src/tests/persistent_volume_tests.cpp 
> e10a79e9662530e143ccaa2aa2506a4d25158364 
> 
> Diff: https://reviews.apache.org/r/52288/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 51715: Added a parallel gtest runner.

[Benjamin Bannier]

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

(Updated Oct. 12, 2016, 5:28 p.m.)


Review request for mesos, Kevin Klues and Till Toenshoff.


Changes
---

Fixed commit message.


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


Repository: mesos


Description (updated)
---

This commits adds a driver capable of running different GoogleTest test
cases in parallel. We will add the driver to the build setup in later
commits.


Diffs
-

  support/mesos-gtest-runner.py PRE-CREATION 

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


Testing
---

Tested with e.g., `stout-tests`

$ ./support/mesos-gtest-runner.py build/3rdparty/stout/stout-tests

[PASS]


Thanks,

Benjamin Bannier

Re: Review Request 52741: Added capabilities support to mesos-execute.

[Benjamin Bannier]

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

(Updated Oct. 12, 2016, 4:31 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Removed accidential lines.


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


Repository: mesos


Description
---

This change introduces a flags `capabilities` to mesos-execute which
can be used to specify the capabilities a task should be able to
aquire.


Diffs (updated)
-

  src/cli/execute.cpp fcf627b0dcbedd01700cc8c9acadf7ba0dae4faa 

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


Testing
---

* `make check`, including `ROOT` tests,
* tested as root with a `ping` task with an agent where both the task and the 
agent have different or no capabilities configured.


Thanks,

Benjamin Bannier

Re: Review Request 52563: Minor clean ups in `health_check_tests.cpp` for consistency.

[haosdent huang]

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

(Updated Oct. 12, 2016, 2:38 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Use test-helper to test.


Repository: mesos


Description
---

* Replace `Testing` to `Tests` in comments.
* Remove redundant `.Times(1)`.
* Fix typos.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---

```
sudo GLOG_v=1 ./bin/mesos-tests.sh --gtest_filter="*HealthCheckTest.*" 
--gtest_also_run_disabled_tests
[--] Global test environment tear-down
[==] 19 tests from 1 test case ran. (35264 ms total)
[  PASSED  ] 19 tests.
```


Thanks,

haosdent huang

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

[haosdent huang]

---
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.


Changes
---

Use test-helper to test.


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


Repository: mesos


Description
---

Added test cases for TCP health check.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52288: Recover resources when offer is rescinded on DESTROY of shared volume.

[Jiang Yan Xu]

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




src/tests/persistent_volume_tests.cpp (lines 1079 - 1086)


Actually, if we don't guarantee that `acceptOffer` is processed first, we 
can't guarantee that `framework2` receives an offer below even with time 
advancement (if framework2 registers before framework1 launches)

We should settle clock here and then the time advancement below 

```
AWAIT_READY(frameworkId2);

Clock::advance(masterFlags.allocation_interval);

AWAIT_READY(offers2);
```

won't be necessary.


- Jiang Yan Xu


On Oct. 7, 2016, 8:12 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52288/
> ---
> 
> (Updated Oct. 7, 2016, 8:12 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6257
> https://issues.apache.org/jira/browse/MESOS-6257
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a framework issues a DESTROY of a shared volume, and that volume
> is not in use by a running or a pending task, we rescind the offers
> from frameworks to which the shared volume is present in pending offers
> so that the volume is not assigned to any task in a future ACCEPT call.
> At that time, we need to recover the resources as well for proper
> accounting of such resources by the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp c7e74df71aa31edb490f4f3bd95f2d5aa94b4324 
>   src/tests/persistent_volume_tests.cpp 
> e10a79e9662530e143ccaa2aa2506a4d25158364 
> 
> Diff: https://reviews.apache.org/r/52288/diff/
> 
> 
> Testing
> ---
> 
> All tests passed.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52560: Avoided temporary `MockDocker` pointers in health check test cases.

[haosdent huang]

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

(Updated Oct. 12, 2016, 2:38 p.m.)


Review request for mesos, Alexander Rukletsov and Gastón Kleiman.


Changes
---

Use test-helper to test.


Repository: mesos


Description
---

Avoided temporary `MockDocker` pointers in health check test cases.


Diffs (updated)
-

  src/tests/health_check_tests.cpp 1d1676d7259bf52cfb1e499954fa815fe7e37522 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 52741: Added capabilities support to mesos-execute.

[Benjamin Bannier]

> On Oct. 12, 2016, 7:43 a.m., Jie Yu wrote:
> > src/cli/execute.cpp, line 298
> > 
> >
> > Why string? Can this be `Option`?

This requires I/O support for `v1::CapabilityInfo`. I added these in the 
preceeding rr https://reviews.apache.org/r/52780/; could you help me get this 
one is as well?


> On Oct. 12, 2016, 7:43 a.m., Jie Yu wrote:
> > src/cli/execute.cpp, lines 1064-1084
> > 
> >
> > Why this is not part of `getContainerInfo`?

I added this to `getContainerInfo`, but left a modified version of the 
consistency check in `main`.


- Benjamin


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


On Oct. 12, 2016, 2:49 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52741/
> ---
> 
> (Updated Oct. 12, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5303
> https://issues.apache.org/jira/browse/MESOS-5303
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces a flags `capabilities` to mesos-execute which
> can be used to specify the capabilities a task should be able to
> aquire.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp fcf627b0dcbedd01700cc8c9acadf7ba0dae4faa 
> 
> Diff: https://reviews.apache.org/r/52741/diff/
> 
> 
> Testing
> ---
> 
> * `make check`, including `ROOT` tests,
> * tested as root with a `ping` task with an agent where both the task and the 
> agent have different or no capabilities configured.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52741: Added capabilities support to mesos-execute.

[Benjamin Bannier]

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

(Updated Oct. 12, 2016, 2:49 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description (updated)
---

This change introduces a flags `capabilities` to mesos-execute which
can be used to specify the capabilities a task should be able to
aquire.


Diffs (updated)
-

  src/cli/execute.cpp fcf627b0dcbedd01700cc8c9acadf7ba0dae4faa 

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


Testing
---

* `make check`, including `ROOT` tests,
* tested as root with a `ping` task with an agent where both the task and the 
agent have different or no capabilities configured.


Thanks,

Benjamin Bannier

Review Request 52780: Added input and output functions for v1::CapabilityInfo.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added input and output functions for v1::CapabilityInfo.


Diffs
-

  include/mesos/v1/mesos.hpp f17145857355d7b85d70b6452bf1f89bf54edbac 
  src/common/parse.hpp 62a333cc1f287439c761aa6f0c7f0bf7ade4a818 
  src/v1/mesos.cpp 0f88abaf49117e844eeb3654edabeeba3862a0eb 

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


Testing
---

* confirmed identically configured unversioned and `v1` `CapabilityInfo`s 
produce identical output,
* used and tested implicitly as part of https://reviews.apache.org/r/52741/


Thanks,

Benjamin Bannier

Re: Review Request 52774: Added a test for verifying nested container environment.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [52774]

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. 12, 2016, 5:35 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52774/
> ---
> 
> (Updated Oct. 12, 2016, 5:35 a.m.)
> 
> 
> Review request for mesos and Gilbert Song.
> 
> 
> Bugs: MESOS-6323
> https://issues.apache.org/jira/browse/MESOS-6323
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for verifying nested container environment.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nested_mesos_containerizer_tests.cpp 
> 9b278e546dacc14b10dac51ee50e8fb89df86b87 
>   src/tests/mesos.hpp 5e15f5164cddebd60bb6d9856ae63ad357643cb2 
> 
> Diff: https://reviews.apache.org/r/52774/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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/
---

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added documentation for mesos-containerizer Linux capabilities support.


Diffs
-

  docs/mesos-containerizer.md 76544625838845fe2817347fea483a60ab250f99 

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


Testing
---

Checked with local renderer.


Thanks,

Benjamin Bannier

Review Request 52784: Added MESOS-5275 to the CHANGELOG.

[Benjamin Bannier]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added MESOS-5275 to the CHANGELOG.


Diffs
-

  CHANGELOG 11e361b22254a7f892f5fa6b656480caf859d9ec 

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


Testing
---


Thanks,

Benjamin Bannier

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/
---

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added a loopback test with CNI ptp plugin.


Diffs
-

  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 50123: Added GPU scheduler for docker containerizer process.

[Yubo Li]

> On 十月 9, 2016, 10:16 a.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.hpp, line 76
> > 
> >
> > This will cause build failed on Mac OS as `NvidiaGpuAllocator` was only 
> > defined in linux 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp#L21
> > 
> > I think that you can follow the same way as we did for 
> > `NvidiaComponents` 
> > https://github.com/apache/mesos/blob/master/src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp#L27

changed `#include "slave/containerizer/mesos/isolators/gpu/nvidia.hpp"` to 
`#include "slave/containerizer/mesos/isolators/gpu/components.hpp"`.


- Yubo


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


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/50123/
> ---
> 
> (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
> ---
> 
> 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
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
>   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 50123: Added GPU scheduler for docker containerizer process.

[Yubo Li]

> On 十月 12, 2016, 5:52 a.m., Guangya Liu wrote:
> > src/tests/mock_docker.hpp, line 40
> > 
> >
> > include `components.hpp` is good enough.

Thanks! I'll test in my mac soon.


- Yubo


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


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/50123/
> ---
> 
> (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
> ---
> 
> 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
> -
> 
>   src/slave/containerizer/docker.hpp 8da63101f951892e673612134770fc155d86112d 
>   src/slave/containerizer/docker.cpp 1d27761fcb3f310cf954d45ed41f4c89ecbd5982 
>   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 51716: Added configure option for Mesos test runner.

[Till Toenshoff]

> On Oct. 10, 2016, 3:28 p.m., Till Toenshoff wrote:
> > configure.ac, lines 616-617
> > 
> >
> > Not a biggy but maybe it is a good idea to make this better readable / 
> > parseable by creating a local temporary variable.
> 
> Benjamin Bannier wrote:
> I agree, this is not very readible. I tried to make this more readible, 
> but did not succeed with my limited autoconf/m4 knowledge, so I added a 
> comment explaining what exactly we do here, and a `TODO`,
> 
> +# We here set up `TEST_DRIVER` to contain an unexpanded automake
> +# variable name; this allows us to reuse the test runner to run 
> bundled
> +# 3rdparty checks. The special quoting ensures that this containing a
> +# space is not expanded by autoconf.
> +# TODO(bbannier): Make this more readible by using autoconf/m4 magic.
> 
> Do you think this is sufficient?

Perfectly fine, thanks for trying :)


- Till


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


On Oct. 11, 2016, 8:19 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51716/
> ---
> 
> (Updated Oct. 11, 2016, 8:19 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-6140
> https://issues.apache.org/jira/browse/MESOS-6140
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added configure option for Mesos test runner.
> 
> 
> Diffs
> -
> 
>   configure.ac 034bb91bfacd74ce0349685f1760f9784fdc1a18 
> 
> Diff: https://reviews.apache.org/r/51716/diff/
> 
> 
> Testing
> ---
> 
> `make check` with `--enable-parallel-test-execution` (OS X, python 2.7.12)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 52638: Populated `recovered_agents` field in `GetAgents` response.

[Zhitao Li]

> On Oct. 10, 2016, 6:53 p.m., Anand Mazumdar wrote:
> > Would you be following up with the change to `/state` in a separate review?

Done. r/52765


- Zhitao


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


On Oct. 12, 2016, 12:18 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52638/
> ---
> 
> (Updated Oct. 12, 2016, 12:18 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Xiaojian Huang.
> 
> 
> Bugs: MESOS-6177
> https://issues.apache.org/jira/browse/MESOS-6177
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Populated `recovered_agents` field in `GetAgents` response.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp bb9c87327dfe2161a6f1fd4cded72aa9a5ffaf66 
> 
> Diff: https://reviews.apache.org/r/52638/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51740: Added the 'name' and 'args' field to the 'delegate' plugin's CNI config.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 28, 2016, 7:29 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51740/
> ---
> 
> (Updated Sept. 28, 2016, 7:29 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
> ---
> 
> When the port-mapper plugin invokes the 'delegate' plugin, the CNI
> config of the 'delegate' plugin needs to have the 'name' and the
> 'args' fields set. We are therefore updating the config during
> creation of the `PortMapper` object, and will use this config when
> the 'delegate' plugin is invoked.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  0ecf64f2de5fc27f208e9dd0e3608b9a6750e9a6 
> 
> Diff: https://reviews.apache.org/r/51740/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51767: Added constants for CNI commands.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Sept. 28, 2016, 7:29 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51767/
> ---
> 
> (Updated Sept. 28, 2016, 7:29 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 constants for CNI commands.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp 
> 2d0187beaf0d20e87d5a9ff80f5672ce7b8cc53b 
> 
> Diff: https://reviews.apache.org/r/51767/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51009: Collect throttle related cpu.stat for Docker Containerizer.

[Zhitao Li]

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

(Updated Oct. 12, 2016, 5 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, Jie Yu, and Steve 
Niemitz.


Changes
---

Rebase only.


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


Repository: mesos


Description
---

Collect throttle related cpu.stat for Docker Containerizer.


Diffs (updated)
-

  src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 

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


Testing
---

Run agent with cfs quota enabled, and observe that throttle related metrics are 
in `/containers` and `/monitoring/statistics`


Thanks,

Zhitao Li

Re: Review Request 52645: Harden Mesos

[Aaron Wood]

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




src/Makefile.am (line 112)


Move these into `configure.ac`.


- Aaron Wood


On Oct. 11, 2016, 10:47 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52645/
> ---
> 
> (Updated Oct. 11, 2016, 10:47 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-6229
> https://issues.apache.org/jira/browse/MESOS-6229
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Use a default set of flags to provide additional security and hardening to 
> Mesos. Additionally, check and catch more warnings/errors.
> 
> 
> Diffs
> -
> 
>   configure.ac 034bb91 
>   src/Makefile.am fd01e1d 
> 
> Diff: https://reviews.apache.org/r/52645/diff/
> 
> 
> Testing
> ---
> 
> Compared the benchmarks with and without the flags being used. Also did a 
> comparsion with the flags being used with and without optimizations and 
> without the flags being used with and without optimizations. Overall the 
> performance hit was very small with a 3-8% overhead (optimizations brings 
> this down slightly). Most benchmarks were about 5% (or less) slower.
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

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/#review152408
---


Ship it!




Ship It!

- Kevin Klues


On Oct. 12, 2016, 4:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 12, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> 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
> 
>

Review Request 52810: Added tests to test usernamespaces.

[Srinivas Brahmaroutu]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

WIP : Added tests to test usernamespaces.


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
8fefeef8c83ed2ab01f56a1ec572d3acb307143c 

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


Testing
---


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52597: Added more detailed error message when failing in MountInfoTable::read.

[Kevin Klues]

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

(Updated Oct. 12, 2016, 11:05 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated error message with even more information (including a newline between 
the output and the mount table lines that are printed).


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


Repository: mesos


Description
---

Added more detailed error message when failing in MountInfoTable::read.


Diffs (updated)
-

  src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce 

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


Testing
---

make


Thanks,

Kevin Klues

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/#review152236
---



High-level comments:

1. We should tailor this towards the users/operators with a brief intro and 
remove things they don't care. We can link to the Epic and your MesosCon talk 
for more technical discussions.
2. We should write the doc as if we started writing it today, explaining all 
features already checked in. So we are not proposing things, nor should we 
include things that are still in the works (add them later).


docs/home.md (line 54)


s/Shareable/Shared/

also, emphasize the usage in persistent volume? i.e.,

"allow tasks to set persistent volumes as shared"?



docs/shared-resources.md (line 5)


I think we can 

s/Shared Resources/Shared Persistent Volumes/

even though we call this document shared-resources.md (for the reasons 
below).



docs/shared-resources.md (lines 9 - 20)


Shared resources is an abstract concept and we have yet to generalize it in 
a way that it can be applied elsewhere other than the persistent volumes. 

Since the focus of this document is on using shared persistent volumes, I 
think it's more clear if we for now make it the only topic (until we can 
articulate it better to non-messo-core developers)

We should jump right into "usage" after a very brief into. We just need to 
explain some basics first:

- Access to regular persistent volumes are exclusive to one 
container/executor at a time.
- Shared persistent volumes allow multiple executor/containers access to 
the same persistent volume simultaneously.
- Simulatenous accesses to the same shared persistent volume are not 
isolated.



docs/shared-resources.md (line 11)


`RW` not relevant here?



docs/shared-resources.md (line 12)


`task` here is not precise, `executor/container` is. Shared persistent 
volume allows access from different containers, we can stick to this 
terminology so the distinction between this feature and the Pods is clear 
(without us needing to expand on that here).



docs/shared-resources.md (lines 29 - 36)


No longer a proposal. We should describe that now that this feature exsits, 
how should the user leverage it.



docs/shared-resources.md (lines 40 - 48)


This is not done and is potentially not going to be done this way 
(MESOS-6374). I think we can avoid saying too much about it for now. We can 
always add a paragraph when it MESOS-4324 is done.



docs/shared-resources.md (lines 50 - 54)


I don't think we need to emphasize this here. Multiple tasks with the same 
executor can access the same regular persistent volume and overwrite each 
other's content whereas if multiple containers strictly want to access 
different sub-directories of a persistent volume, they gain nothing by sharing 
a persistent volume.



docs/shared-resources.md (lines 58 - 65)


The doc should be written in either the user's or the "framework"'s 
perspective, not "we" as in "Mesos".



docs/shared-resources.md (lines 67 - 71)


We should assume `SharedInfo` is already a thing (not a new thing, as 
compared to e.g., `RecocableInfo`, as least the operator shouldn't care) and we 
are just informing users how to use it.

So I suggest we just skip this section jump into the usage examples.



docs/shared-resources.md (lines 73 - 75)


Here we haven't included the scheduler DESTROY API or HTTP endpoints, 
presumably because they will be the duplicate of what's in the persistent 
volume doc, which I agree we shouldn't do.

So I think we should just make it clear that we are only going to highlight 
a few things different than the regular persistent volumes in terms of usage, 
but with examples. Then we can refer the readers to the persistent volume doc 
for details.

If we are not separately explaning all APIs, we probably don't need the `## 
Framework Scheduler API` heading.



docs/shared-resources.md (lines 77 - 79)


To avoid duplicating the regular persistent volume doc, how about the 
following for using shared persistent volumes? 

---

The framework can create a shared persistent volume using the existing 
persistent 

Re: Review Request 52703: Added test to test corner cases with sorted 'MountInfoTable::read()'.

[Kevin Klues]

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

(Updated Oct. 12, 2016, 11:11 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed Jie's comments.


Summary (updated)
-

Added test to test corner cases with sorted 'MountInfoTable::read()'.


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


Repository: mesos


Description (updated)
---

We allow entries in the MountInfoTable to be out of order, as well as
parent's of themselves. This test makes sure that this functionality
is exercised.


Diffs (updated)
-

  src/tests/containerizer/fs_tests.cpp 0dd212f94d2845494163a86bc2059f999e018cd0 

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


Testing
---

GTEST_FILTER="*Sorted*" src/mesos-tests


Thanks,

Kevin Klues

Review Request 52811: Updated 'MountInfoTableReadSorted' test to use a hashset.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Updated 'MountInfoTableReadSorted' test to use a hashset.


Diffs
-

  src/tests/containerizer/fs_tests.cpp 0dd212f94d2845494163a86bc2059f999e018cd0 

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


Testing
---

GTEST_FILTER="*Sorted*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 51052: Make mesos-docker-execute understand cgroups_enable_cfs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51052]

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. 12, 2016, 5:02 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51052/
> ---
> 
> (Updated Oct. 12, 2016, 5:02 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-6134
> https://issues.apache.org/jira/browse/MESOS-6134
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes cpu quota for command executor (which runs outside
> of the docker container) by ensuing --cpu-quota flag to docker
> run.
> 
> Note: we have to add the boolean variable to `Docker` class
> because `Docker::run()` has reached the maximum argument length
> GMOCK can support.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 9093371afc8ea792ba94f61c6875703e547ea6b0 
>   src/docker/docker.cpp 50fda393a42afefc70790a26b44911e4cf17185e 
>   src/docker/executor.hpp a49ec1b4045116741af6af08578791fb0440ad8f 
>   src/docker/executor.cpp ab3f0473fdc9105d1c425f0dbe7b81c566d541e8 
>   src/slave/containerizer/docker.cpp d71386089bf7677872bcb1bd36e07da9263dcf0d 
>   src/tests/mock_docker.cpp 6a0e613bde6889864a37ffd7ec0b454e5fe4df1c 
> 
> Diff: https://reviews.apache.org/r/51052/diff/
> 
> 
> Testing
> ---
> 
> I am now able to make docker containers launched through mesos-execute have a 
> cpu quota.
> 
> Also making sure `make check` still works on mac os for the linux only flag.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 52755: Made default executor handle shutdown events while disconnected.

[Anand Mazumdar]

> On Oct. 12, 2016, 10:52 p.m., Vinod Kone wrote:
> > src/launcher/default_executor.cpp, line 700
> > 
> >
> > can you add a comment on how this can happen when we are in `CONNECTED` 
> > state? it's not obvious to me.

Sure, will do. It can happen in `CONNECTED` state if the agent explicitly asks 
the executor to shut down if it's not able to subscribe within the re-register 
timeout for some reason.


- Anand


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


On Oct. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52755/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6363
> https://issues.apache.org/jira/browse/MESOS-6363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the default executor used to crash with a failed assertion
> when the executor library injected a shutdown event when it noticed
> a disconnection with the agent for non-checkpointed frameworks and
> upon recovery timeout for checkpointed frameworks. This change
> modifies it to commit suicide minus the failed assertion.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
> 
> Diff: https://reviews.apache.org/r/52755/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 52569: Updated the nested container launch to correctly determine the user.

[Benjamin Mahler]

> On Oct. 12, 2016, 6:46 p.m., Gilbert Song wrote:
> > src/slave/http.cpp, lines 1964-1965
> > 
> >
> > Would you mind removing this `TODO`? They are no longer valid.
> 
> Gilbert Song wrote:
> I will follow up with a patch. Sorry not review on time.

Done, thanks for catching this!


- Benjamin


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


On Oct. 5, 2016, 8:45 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52569/
> ---
> 
> (Updated Oct. 5, 2016, 8:45 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Vinod Kone.
> 
> 
> Bugs: MESOS-6289
> https://issues.apache.org/jira/browse/MESOS-6289
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This locates the executor that corresponds to the parent container,
> and uses the user that was used to launch the executor.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp bdafe1db9f1b22ac0761f5119e73f87b531c6a3c 
>   src/slave/slave.hpp 9beefa12283e936684d82c97ebd3ac6506475edf 
>   src/slave/slave.cpp fba089506850eec87f67a46694a1ef645cdf4ad5 
>   src/tests/api_tests.cpp 7b0ad3c18a38b5ec859a79d09f5707f6958960fb 
> 
> Diff: https://reviews.apache.org/r/52569/diff/
> 
> 
> Testing
> ---
> 
> Now that the executor is looked up, the tests have been updated to
> launch an executor and use it's container id as the parent.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 52756: Invoke the shutdown executor callback for checkpointed frameworks.

[Anand Mazumdar]

> On Oct. 12, 2016, 10:56 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 734
> > 
> >
> > I think this should call `_shutdown()` to ensure ShutdownProcess is 
> > spawned  whenever we want to shutdown.

The actual plumbing happens through the `receive()` handler that first invokes 
the callback on the executor and then invokes `_shutdown()`. All other places 
in the code use `shutdown()` to inject the `SHUTDOWN` event.


- Anand


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


On Oct. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52756/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6370
> https://issues.apache.org/jira/browse/MESOS-6370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the executor library used to commit suicide after the
> recovery timeout without invoking the executor's shutdown callback.
> This behavior was not consistent with the executor driver.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp a0e5d8382fa3886b747ae92507dce17e75c2d749 
> 
> Diff: https://reviews.apache.org/r/52756/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 52756: Invoke the shutdown executor callback for checkpointed frameworks.

[Vinod Kone]

> On Oct. 12, 2016, 10:56 p.m., Vinod Kone wrote:
> > src/executor/executor.cpp, line 734
> > 
> >
> > I think this should call `_shutdown()` to ensure ShutdownProcess is 
> > spawned  whenever we want to shutdown.
> 
> Anand Mazumdar wrote:
> The actual plumbing happens through the `receive()` handler that first 
> invokes the callback on the executor and then invokes `_shutdown()`. All 
> other places in the code use `shutdown()` to inject the `SHUTDOWN` event.

gotcha.


- Vinod


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


On Oct. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52756/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6370
> https://issues.apache.org/jira/browse/MESOS-6370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the executor library used to commit suicide after the
> recovery timeout without invoking the executor's shutdown callback.
> This behavior was not consistent with the executor driver.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp a0e5d8382fa3886b747ae92507dce17e75c2d749 
> 
> Diff: https://reviews.apache.org/r/52756/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

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

[Kevin Klues]

> On Oct. 12, 2016, 10:10 p.m., Kevin Klues wrote:
> > Ship It!

Can you link this to the proper JIRA?


- Kevin


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


On Oct. 12, 2016, 4:13 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52787/
> ---
> 
> (Updated Oct. 12, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> 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 52809: User Namespace implementation.

[Srinivas Brahmaroutu]

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

(Updated Oct. 12, 2016, 11:01 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

WIP: User Namespace implementation.


Diffs
-

  src/Makefile.am fd01e1dfbdb04d073484ff6b7cc94b8d769f8a8e 
  src/slave/containerizer/mesos/containerizer.cpp 
32058c35ea9ca95f0a2665994c1ebccd5c840345 
  src/slave/containerizer/mesos/isolators/user/user.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/user.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/usermaps.hpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
c6b669a04c006edfc78c06560d1eb088278c2f8e 
  src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
  src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 

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


Testing
---

Work in progress implementing User namespaces.
Phase 1: Create isolator and enable isolator to when Agent is run with 
"userns=true". If this flags is not set the original functionality will run the 
task as user who started the task. With User namespace the task will be run 
inside the user namespace with as a root with the user who started the task is 
mapped to outside of the container. Approriate uid and gid maps are created.
Phase 2: Provide mount point support for containers running in user namespace.


Thanks,

Srinivas Brahmaroutu

Review Request 52809: User Namespace implementation.

[Srinivas Brahmaroutu]

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

Review request for mesos, Gilbert Song and Jie Yu.


Repository: mesos


Description
---

WIP: User Namespace implementation.


Diffs
-

  src/Makefile.am fd01e1dfbdb04d073484ff6b7cc94b8d769f8a8e 
  src/slave/containerizer/mesos/containerizer.cpp 
32058c35ea9ca95f0a2665994c1ebccd5c840345 
  src/slave/containerizer/mesos/isolators/user/user.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/user.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/user/usermaps.hpp PRE-CREATION 
  src/slave/containerizer/mesos/launch.cpp 
c6b669a04c006edfc78c06560d1eb088278c2f8e 
  src/slave/flags.hpp 3952d04f6a00ac1dca1adf2bea7cc6e415620ce5 
  src/slave/flags.cpp 491d10f6a8a7ea8adbfe0a09f5fce79943bccfac 

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


Testing
---

Work in progress implementing User namespaces.
Phase 1: Create isolator and enable isolator to when Agent is run with 
"userns=true". If this flags is not set the original functionality will run the 
task as user who started the task. With User namespace the task will be run 
inside the user namespace with as a root with the user who started the task is 
mapped to outside of the container. Approriate uid and gid maps are created.
Phase 2: Provide mount point support for containers running in user namespace.


Thanks,

Srinivas Brahmaroutu

Re: Review Request 52704: Refactored 'MountInfoTable::read()' into two separate functions.

[Kevin Klues]

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

(Updated Oct. 12, 2016, 11:10 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

The original function now calls a helper which takes a string
representation of a 'MountInfoTable'. In a subsequent commit we will
use this helper to write more meaningful tests to stress the core
logic of the 'MountInfoTable::read()' functionality.


Diffs (updated)
-

  src/linux/fs.hpp 090c82616d965b1cedf9bb13ffcde2d37304a1f7 
  src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce 

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


Testing
---

GTEST_FILTER="*Sorted*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52596: Added special case when sorting hierarchically in MountInfoTable::read.

[Kevin Klues]

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

(Updated Oct. 12, 2016, 11:09 p.m.)


Review request for mesos and Jie Yu.


Changes
---

It turns out that I can't preemptively keep a self reference out of the child 
list, otherwise the algorithm for adding entries back into the MountInfoTable 
vector doesn't work as expected.  Instead, I add the self reference into the 
child list, but don't recurse down into it once it is visited.


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


Repository: mesos


Description
---

It is legal to have entries in a `MountInfoTable` whose `entry.id` is
the same as `entry.parent`. This can happen (for example), if a system
boots from the network and then keeps the original `/` in RAM.
However, to avoid cycles when walking the mount hierarchy, we should
not treat these entries as children of their parent so we skip them.

This commit adds functionality to handle this case.


Diffs (updated)
-

  src/linux/fs.cpp 4b10141a49dfb3c6defdb07e295eb14cfcdd36ce 

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


Testing
---

GTEST_FILTER="" make -j40 check
GTEST_FILTER="FsTest.MountInfoTableReadSorted" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52470: Filled missing executor info in tasks when `LAUNCH_GROUP`.

[Vinod Kone]

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




include/mesos/mesos.proto 


2) `TaskInfo.executor` need not be set. If set, it should match 
`LaunchGroup.executor`.


- Vinod Kone


On Oct. 12, 2016, 5:18 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52470/
> ---
> 
> (Updated Oct. 12, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-6283
> https://issues.apache.org/jira/browse/MESOS-6283
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixed the navigate error in Web UI because Web UI uses the
> executor id of the task to search the corresponding sandbox directory.
> Web UI uses the task id as the executor id if the executor id of the
> task is empty when searching the sandbox directory. It works fine when
> tasks are launched by `CommandExecutor` because the executor id of the
> task is equal to the task id in this case. However, when tasks are
> launched by `DefaultExecutor`, the executor id of the task is defined
> in the framework side and may different with the task id. So we need to
> fill the `ExecutorInfo` of the `TaskInfo` when `LAUNCH_GROUP` to avoid
> the Web UI uses incorrect executor id to search sandbox directory.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 7d0201ead69c816b9d51f1a41cd64a8f922b069d 
>   include/mesos/v1/mesos.proto 301d1aedca7ac0729229ba9b05e5bf2b79f286a5 
>   src/master/master.hpp 43518b9bf1bfaa54e26acc7f2e70c4161c667a84 
>   src/master/master.cpp ad8993ae43e2508a3777f4062acaca1177ac77f2 
>   src/master/validation.cpp e5da3c9bf0a1042b42522f1ab74ce798fbb1738d 
>   src/tests/default_executor_tests.cpp 
> 9e0fd678a7f5e9c2288fbb11a60cf6f339efa24f 
>   src/tests/master_validation_tests.cpp 
> 99e350e0587e73e9ee25ef20dd369cd146bd446a 
> 
> Diff: https://reviews.apache.org/r/52470/diff/
> 
> 
> Testing
> ---
> 
> Add new test case `DefaultExecutorTest.ROOT_TaskUsesExecutor`.
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 52756: Invoke the shutdown executor callback for checkpointed frameworks.

[Vinod Kone]

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




src/executor/executor.cpp (line 731)


I think this should call `_shutdown()` to ensure ShutdownProcess is spawned 
 whenever we want to shutdown.


- Vinod Kone


On Oct. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52756/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6370
> https://issues.apache.org/jira/browse/MESOS-6370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the executor library used to commit suicide after the
> recovery timeout without invoking the executor's shutdown callback.
> This behavior was not consistent with the executor driver.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp a0e5d8382fa3886b747ae92507dce17e75c2d749 
> 
> Diff: https://reviews.apache.org/r/52756/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 52755: Made default executor handle shutdown events while disconnected.

[Vinod Kone]

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


Fix it, then Ship it!





src/launcher/default_executor.cpp (line 698)


can you add a comment on how this can happen when we are in `CONNECTED` 
state? it's not obvious to me.


- Vinod Kone


On Oct. 12, 2016, 12:15 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52755/
> ---
> 
> (Updated Oct. 12, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6363
> https://issues.apache.org/jira/browse/MESOS-6363
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the default executor used to crash with a failed assertion
> when the executor library injected a shutdown event when it noticed
> a disconnection with the agent for non-checkpointed frameworks and
> upon recovery timeout for checkpointed frameworks. This change
> modifies it to commit suicide minus the failed assertion.
> 
> 
> Diffs
> -
> 
>   src/launcher/default_executor.cpp 2454bd7df608254af72af01460782f5ab78a19c1 
> 
> Diff: https://reviews.apache.org/r/52755/diff/
> 
> 
> Testing
> ---
> 
> Manual testing (It's hard to test this directly owing to the executor library 
> immediately invoking the `connected()` callback since the libprocess instance 
> initialized in tests is always active!)
> 
> Filed MESOS-6373 to address this.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 45962: Added a persistent volume test framework for shared volumes.

[Anindya Sinha]

> On Oct. 7, 2016, 5:20 p.m., Greg Mann wrote:
> > Given how similar the code in the new example framework is to the existing 
> > persistent volumes framework, I think it could make sense to add shared 
> > volume features to the existing framework, which could be enabled/disabled 
> > via command-line flags. Especially since you add some nice new DESTROY 
> > logic in the new framework; it would be great to have that change in the 
> > existing one, and avoid duplicating a lot of code. What do you think?

Agreed. Makes sense. Will do that in the next update.


- Anindya


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


On Sept. 28, 2016, 12:15 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45962/
> ---
> 
> (Updated Sept. 28, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a persistent volume test framework for shared volumes.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am fba488f9d676851dd046a8b8c7dd175b3c0d9ef0 
>   src/examples/persistent_shared_volume_framework.cpp PRE-CREATION 
>   src/tests/examples_tests.cpp 52fac33733cc86dd718c7836c5031999f1597a5c 
>   src/tests/persistent_shared_volume_framework_test.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45962/diff/
> 
> 
> Testing
> ---
> 
> New test framework for shared resources added.
> Tests successful.
> 
> 
> Thanks,
> 
> Anindya Sinha
> 
>

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 28
> > 
> >
> > I've been out of the game awhile-- are we wrapping comments at 70 
> > characters these days?

I typically use a 72 character wrap for comments because it looks nicer. I only 
go up to (or near to) 80 if the line I'm commenting is around 80 characters 
itself.


> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 25
> > 
> >
> > Couple of questions here:
> > 
> > * This seems useful for POSIX platforms, but I don't really understand 
> > the implications of having this enabled on Windows. Windows will never 
> > support the signal semantics of POSIX, and in general my feeling is that 
> > I'd rather not define things on platforms that don't support those 
> > semantics, unless there is a particularly pertinent reason to. Thoughts?
> > * In the 2 or 3 places we do use `W_EXITCODE`, rather than defining it 
> > for Windows, we simply `#ifdef` the code out. So the precedent is to just 
> > not use the macro. Is there a compelling reason to change this?
> > 
> > 
> > Also, tiny, tiny nit question: is it style to have 2 spaces between 
> > `W_EXITCODE(ret, sig)` and `((ret) << 8 | (sig))`

To answer both your bullets at once...
If you look in `windows.hpp` all of the other `W*` macros from the posix 
standard are defined in there (even though they may be meaningless on windows). 
This is consistent with them all being defined in `sys/wait.h` on a posix 
compliant system. This is why I separated the two by a simple `#ifdef`

Adding the code below to be added to both windows and poix systems:

```
#ifndef W_EXITCODE
#define W_EXITCODE(ret, sig)  ((ret) << 8 | (sig))
#endif
```

is just to cover a corner case where some libc variants don't actually define 
this macro (because it's not technically posix compliant). Almost all libc 
variants do define it, but a bug was filed against this because (apparently) 
`musl` does not.

Given that adding this code makes both windows and any variant of libc now 
define all of the `W*` macros symetrically, I think this is likely the right 
way to do it for now. In the future when we completely strip out all `W*` 
macros from windows, we can revisit this. 

---

Regarding the formatting question. I just copy and pasted this from the glibc 
header file. I don't think we have a preferred style for this, but I tried it 
both ways just now, and it seems to be more readable as 2 spaces, so I'll leave 
it.


> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 17
> > 
> >
> > How much work is it to do this now? If we're going to have a dedicated 
> > header, I would greatly prefer to put all the definitions in one place.

See comment below for why it probably makes sense to just leave things as they 
are for now


- Kevin


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


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51620: Removed two std::move in MountInfoTable::read.

[Kevin Klues]

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




src/linux/fs.cpp (line 145)


If you remove the `std::move()`, then you can remove this temporary 
variable reference.


- Kevin Klues


On Sept. 3, 2016, 12:25 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51620/
> ---
> 
> (Updated Sept. 3, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Kevin Klues.
> 
> 
> Bugs: MESOS-6118
> https://issues.apache.org/jira/browse/MESOS-6118
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since 'entry' is a const reference, the std::move won't reduce the
> extra copying. This patch removed this optimization.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 14ae5a9089916549f691363bc2269e13c5260a14 
> 
> Diff: https://reviews.apache.org/r/51620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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

[Jiang Yan Xu]

> On Oct. 12, 2016, 4:13 p.m., Jiang Yan Xu wrote:
> > docs/shared-resources.md, line 175
> > 
> >
> > We can add a section for things that aren't in the doc for regular 
> > persistent volumes. i.e.,
> > 
> > ---
> > 
> > ## Unique Shared Persistent Volume Features
> > 
> > ### Launching multiple tasks on the shared persistent volume
> > 
> > ### Destroying shared persistent volumes
> >  > to be more cautious about this for shared persistent volumes>
> > 
> > ---
> > 
> > I feel these are what make a separate doc for shared persistent volumes 
> > necessary (and we can continue to add more topics). Otherwise we could just 
> > bundle everything into the persistent volume doc.

The fomatting is messed up, I meant

```
## Unique Shared Persistent Volume Features


### Launching multiple tasks on the same shared persistent volume

### Destroying shared persistent volumes

```


- Jiang Yan


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


On Sept. 27, 2016, 5:16 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45967/
> ---
> 
> (Updated Sept. 27, 2016, 5:16 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 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

> On Oct. 11, 2016, 8:48 p.m., Alex Clemmer wrote:
> > 3rdparty/stout/include/stout/wait.hpp, line 17
> > 
> >
> > How much work is it to do this now? If we're going to have a dedicated 
> > header, I would greatly prefer to put all the definitions in one place.
> 
> Kevin Klues wrote:
> See comment below for why it probably makes sense to just leave things as 
> they are for now

See comment below for rationale.


- Kevin


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


On Oct. 10, 2016, 9:22 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/52705/
> ---
> 
> (Updated Oct. 10, 2016, 9:22 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6310
> https://issues.apache.org/jira/browse/MESOS-6310
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This was motivated by the need for a default definition of
> 'W_EXITCODE' (since it is not technically POSIX compliant).
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/wait.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/52705/diff/
> 
> 
> Testing
> ---
> 
> sudo GTEST_FILTER="*Nested*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 52706: Updated mesos containerizer to use new 'stout/wait.hpp' header.

[Kevin Klues]

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

(Updated Oct. 13, 2016, 12:08 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie and Alex's comments.


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


Repository: mesos


Description
---

Updated mesos containerizer to use new 'stout/wait.hpp' header.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
e6bd9f7a8284d220be157a3db2da094e6b1b6d33 

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


Testing
---

sudo GTEST_FILTER="*Nested*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 52705: Added 'stout/wait.hpp' header.

[Kevin Klues]

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

(Updated Oct. 13, 2016, 12:08 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie and Alex's comments.


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


Repository: mesos


Description
---

This was motivated by the need for a default definition of
'W_EXITCODE' (since it is not technically POSIX compliant).


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am b0b08d8e0d284a88bc8daa4570540659b94dc2d0 
  3rdparty/stout/include/stout/os/wait.hpp PRE-CREATION 

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


Testing
---

sudo GTEST_FILTER="*Nested*" src/mesos-tests


Thanks,

Kevin Klues