Re: Review Request 47209: Establish TCP connection after backing off.

2016-05-12 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On May 13, 2016, 3:37 a.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47209/
> ---
> 
> (Updated May 13, 2016, 3:37 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-5330
> https://issues.apache.org/jira/browse/MESOS-5330
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Establish TCP connection after backing off.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
> 
> Diff: https://reviews.apache.org/r/47209/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also started a master and agent and captured a tcpdump to verify the 
> connection is only established _after_ the registration backoff.
> 
> 
> Thanks,
> 
> David Robinson
> 
>



Re: Review Request 47209: Establish TCP connection after backing off.

2016-05-12 Thread Ben Mahler

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




src/slave/slave.cpp (line 996)
<https://reviews.apache.org/r/47209/#comment197322>

Whoops, this will crash when master is None. How about you move it down to 
where we've decided we need to authenticate (below the LOG(INFO)). This makes 
it a bit more symmetric with the registration path).

Also could you include the following?

```
// Ensure there is a link to the master before we start communicating with 
it.
```


- Ben Mahler


On May 12, 2016, 6:18 p.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47209/
> ---
> 
> (Updated May 12, 2016, 6:18 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-5330
> https://issues.apache.org/jira/browse/MESOS-5330
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Establish TCP connection after backing off.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
> 
> Diff: https://reviews.apache.org/r/47209/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also started a master and agent and captured a tcpdump to verify the 
> connection is only established _after_ the registration backoff.
> 
> 
> Thanks,
> 
> David Robinson
> 
>



Re: Review Request 46670: Added deprecated alias for `--authenticate_frameworks` master flag.

2016-05-12 Thread Ben Mahler

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


Fix it, then Ship it!





src/master/flags.cpp (lines 18 - 23)
<https://reviews.apache.org/r/46670/#comment197314>

Mind committing this bit separately? You also need duration.hpp, error.hpp, 
none.hpp, stringify.hpp.


- Ben Mahler


On May 12, 2016, 10:38 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46670/
> ---
> 
> (Updated May 12, 2016, 10:38 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4386
> https://issues.apache.org/jira/browse/MESOS-4386
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `--authenticate` is made a deprecated alias for
> `--authenticate_frameworks` master flag. Also updated the example tests
> to use the new flag.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
>   src/examples/docker_no_executor_framework.cpp 
> 2b82b4f0e2424b98ea29fb08b21993b1713b364e 
>   src/examples/dynamic_reservation_framework.cpp 
> 4ad5f4b846052ec9a2067a5ce21fb017f681debf 
>   src/examples/java/TestExceptionFramework.java 
> 12bf60325567eb7a61c5811cbbac66f1b8e9ae2b 
>   src/examples/java/TestFramework.java 
> 295e54fde11fc9938d00ec03cbeaa7225b76a86c 
>   src/examples/persistent_volume_framework.cpp 
> b4faa0ee25dc3a72c17ef2b0640a3695423ef79a 
>   src/examples/python/test_framework.py 
> 5f8e470721b303a60797c41db3c5587b89b5cf58 
>   src/examples/test_framework.cpp 79113fbe47fda0912f0b01dc10429495a96ba8b8 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
>   src/tests/script.cpp 5f2e2729b1d95b061bf3cbe0a052b397e366277d 
>   src/tests/test_http_framework_test.sh 
> 3a2b24cd5017f3535340cb8ade13b34e341cd7ce 
> 
> Diff: https://reviews.apache.org/r/46670/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Teste manually by running ./bin/mesos-master.sh --authenticate
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>



Re: Review Request 46815: Updated mesos to work with new `flags.load()` signature.

2016-05-12 Thread Ben Mahler

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


Fix it, then Ship it!





src/cli/execute.cpp (line 644)
<https://reviews.apache.org/r/46815/#comment197313>

`->` here and everywhere else


- Ben Mahler


On May 12, 2016, 10:43 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46815/
> ---
> 
> (Updated May 12, 2016, 10:43 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Greg Mann, and Michael Park.
> 
> 
> Bugs: MESOS-5370
> https://issues.apache.org/jira/browse/MESOS-5370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In addition to updating the code for the new signature, deprecation
> warnings are also printed when loading deprecated flag names/aliases.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 4711e80314e2fc4cde077bebd9a8167324c3254e 
>   src/cli/resolve.cpp 23bb0f156fb07aed54ef225b0a5438a1427631bf 
>   src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 
>   src/examples/dynamic_reservation_framework.cpp 
> 4ad5f4b846052ec9a2067a5ce21fb017f681debf 
>   src/examples/load_generator_framework.cpp 
> b22a09e1749d4b523addacad99858d7b6bde3403 
>   src/examples/long_lived_framework.cpp 
> 1740d7cb747d179d06e75153aa334b29e9cdf3c0 
>   src/examples/no_executor_framework.cpp 
> f578edfd99f3b7adf19cf06eab20696532c7b67d 
>   src/examples/persistent_volume_framework.cpp 
> b4faa0ee25dc3a72c17ef2b0640a3695423ef79a 
>   src/examples/test_framework.cpp 79113fbe47fda0912f0b01dc10429495a96ba8b8 
>   src/examples/test_http_framework.cpp 
> db0463d894e9f2fb964781d16f8c622ce8a507a5 
>   src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
>   src/executor/executor.cpp c7187ea2f0685b6553356acbea90a63f643c0713 
>   src/health-check/main.cpp 98ea5d3675f088e3a341037dcee92695e4857999 
>   src/launcher/executor.cpp fa4a89e97b0c427b4595a6adebc397aeb5bfaaa5 
>   src/launcher/fetcher.cpp d323f6341ab8367eeb456c9f399395293960fb66 
>   src/launcher/http_command_executor.cpp 
> c62fe3ee6ae06536cbb89ea208b669790efe4b39 
>   src/local/local.cpp 1c679ecb486cb3d6184ec9a941f2ac5dbd2bcc1f 
>   src/local/main.cpp 51bbdfbd18650fbbe9fede4aca3feb2f43beca72 
>   src/log/tool/benchmark.cpp 8981ea82735f3a1149aa777a62960582fea67a4d 
>   src/log/tool/initialize.cpp bd1e9ef1922ae972a5999b6e7412e08eac92c1ac 
>   src/log/tool/read.cpp b9e90e44c8cd7351767e523af338d8c662e0848c 
>   src/log/tool/replica.cpp e3661df858705132685b0c584c1adc716099bc30 
>   src/master/main.cpp 2d1bd554d8bbbf98ac1c1a7b196c9ab6185e38b9 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
>   src/scheduler/scheduler.cpp 7d83f3c3ffdb0f4bac67af5b156f69302abe7999 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1f228806da32832c9ca1ae4defcd1bdc154adc18 
>   src/slave/container_loggers/logrotate.cpp 
> 7d36c052ff7a180b45ca265fb7ff4c6900d98d64 
>   src/slave/main.cpp 66aea8c79be29678e16359f48452a41e5b473aa2 
>   src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 
>   src/usage/main.cpp 731acb69900b6fc2bb7bd19cccd78aafb0cc 
> 
> Diff: https://reviews.apache.org/r/46815/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>



Re: Review Request 46818: Updated libprocess tests to work with the new `flag.load()` signature.

2016-05-12 Thread Ben Mahler

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


Ship it!





3rdparty/libprocess/src/tests/subprocess_tests.cpp (line 613)
<https://reviews.apache.org/r/46818/#comment197312>

Can you add an expectation that there are no warnings?


- Ben Mahler


On May 12, 2016, 10:42 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46818/
> ---
> 
> (Updated May 12, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Greg Mann, and Michael Park.
> 
> 
> Bugs: MESOS-5370
> https://issues.apache.org/jira/browse/MESOS-5370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> No functional changes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 727e940f12643974de4ff2734fba431b285b5de3 
> 
> Diff: https://reviews.apache.org/r/46818/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>



Re: Review Request 46863: Refactored FlagsBase::load() to move duplicate checking logic.

2016-05-12 Thread Ben Mahler

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


Fix it, then Ship it!





3rdparty/stout/include/stout/flags/flags.hpp (lines 661 - 663)
<https://reviews.apache.org/r/46863/#comment197301>

// TODO(vinod): Reject flags with an unknown name if `unknowns` is false.
// This will break backwards compatibility however!



3rdparty/stout/include/stout/flags/flags.hpp (lines 664 - 665)
<https://reviews.apache.org/r/46863/#comment197300>

How about strings::remove with PREFIX?



3rdparty/stout/include/stout/flags/flags.hpp (line 690)
<https://reviews.apache.org/r/46863/#comment197302>

Since this ends up holding both command and environment values, how about 
just 'values' now?



3rdparty/stout/include/stout/flags/flags.hpp (lines 728 - 729)
<https://reviews.apache.org/r/46863/#comment197303>

How about:

```
// Merge in flags from the environment. Command-line
// flags take precedence over environment flags.
```



3rdparty/stout/include/stout/flags/flags.hpp (line 750)
<https://reviews.apache.org/r/46863/#comment197305>

Don't need this?



3rdparty/stout/include/stout/flags/flags.hpp (line 751)
<https://reviews.apache.org/r/46863/#comment197306>

s/cmdValues/values/



3rdparty/stout/include/stout/flags/flags.hpp (lines 797 - 798)
<https://reviews.apache.org/r/46863/#comment197304>

// Merge in flags from the environment. Command-line
// flags take precedence over environment flags.



3rdparty/stout/include/stout/flags/flags.hpp (line 831)
<https://reviews.apache.org/r/46863/#comment197309>

can you suffix with _ on the second instance?



3rdparty/stout/include/stout/flags/flags.hpp (lines 835 - 836)
<https://reviews.apache.org/r/46863/#comment197307>

Could we wrap at the paren?



3rdparty/stout/include/stout/flags/flags.hpp (line 844)
<https://reviews.apache.org/r/46863/#comment197310>

How about _ as a suffix on the second instance?



3rdparty/stout/include/stout/flags/flags.hpp (line 862)
<https://reviews.apache.org/r/46863/#comment197308>

Could we wrap at the paren?



3rdparty/stout/include/stout/flags/flags.hpp (lines 883 - 884)
<https://reviews.apache.org/r/46863/#comment197311>

Shouldn't this print the `name` being looped over here so that we can see 
the two clashing names?


- Ben Mahler


On May 12, 2016, 10:42 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46863/
> ---
> 
> (Updated May 12, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-5370
> https://issues.apache.org/jira/browse/MESOS-5370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The logic is now moved to Flag::load() keep the duplicate checking logic
> for names and aliases in one place.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 02584e28b938bba738c2e32b3cb7a04a47693853 
> 
> Diff: https://reviews.apache.org/r/46863/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>



Re: Review Request 46669: Added deprecation support to Flag name.

2016-05-12 Thread Ben Mahler

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


Fix it, then Ship it!





3rdparty/stout/include/stout/flags/flag.hpp (line 23)
<https://reviews.apache.org/r/46669/#comment197295>

whoops?



3rdparty/stout/include/stout/flags/flag.hpp (lines 52 - 57)
<https://reviews.apache.org/r/46669/#comment197296>

Maybe `s/name_/name/` and `s/name/n/` or `s/name/name_`?



3rdparty/stout/include/stout/flags/flags.hpp (line 899)
<https://reviews.apache.org/r/46669/#comment197297>

Past tense since the caller will log the warning after load completes?



3rdparty/stout/tests/flags_tests.cpp (lines 271 - 272)
<https://reviews.apache.org/r/46669/#comment197299>

Can you update all of these existing tests to expect no warnings?



3rdparty/stout/tests/flags_tests.cpp (line 502)
<https://reviews.apache.org/r/46669/#comment197298>

Can you use the `->` operator here?

EXPECT?

Also can you add an expectation on the message?


- Ben Mahler


On May 12, 2016, 10:42 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46669/
> ---
> 
> (Updated May 12, 2016, 10:42 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Greg Mann, and Michael Park.
> 
> 
> Bugs: MESOS-5370
> https://issues.apache.org/jira/browse/MESOS-5370
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows callers of Flag to explicitly specify a flag name or its
> alias as deprecated. Loading of deprecated flag name will result in a
> deprecation warning being returned.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/README.md 45dd8f39de9fa34cc11befbe842319079685db02 
>   3rdparty/stout/include/stout/flags/flag.hpp 
> d869904d13301fd4b3fdb037e6279d3a4018ac1e 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 02584e28b938bba738c2e32b3cb7a04a47693853 
>   3rdparty/stout/include/stout/subcommand.hpp 
> 4d37c4afdcf4c3d6d511dd8a27916332b086afa7 
>   3rdparty/stout/tests/flags_tests.cpp 
> a2880a0afdc54faa4331ec8bd91929ce08c8f4a5 
> 
> Diff: https://reviews.apache.org/r/46669/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>



Re: Review Request 47209: Establish TCP connection after backing off.

2016-05-12 Thread Ben Mahler

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




src/slave/slave.cpp (line 966)
<https://reviews.apache.org/r/47209/#comment197233>

Why not call `link` inside the `authenticate()` function in a similar 
manner to how you've called it within `doReliableRegistration()`?



src/slave/slave.cpp (line 1361)
<https://reviews.apache.org/r/47209/#comment197235>

How about the following comment here:

```
  // Ensure there is a link to the master before we start
  // communicating with it. We want to `link` after the
  // initial registration backoff in order to avoid all
  // of the agents establishing connections with the
  // master at once. See MESOS-5330.
    ```


- Ben Mahler


On May 11, 2016, 12:58 a.m., David Robinson wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47209/
> ---
> 
> (Updated May 11, 2016, 12:58 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Ian Downes, and Cong Wang.
> 
> 
> Bugs: MESOS-5330
> https://issues.apache.org/jira/browse/MESOS-5330
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Establish TCP connection after backing off.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
> 
> Diff: https://reviews.apache.org/r/47209/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also started a master and agent and captured a tcpdump to verify the 
> connection is only established _after_ the registration backoff.
> 
> 
> Thanks,
> 
> David Robinson
> 
>



Re: Review Request 47028: Fixed a confusing log line in the allocator.

2016-05-11 Thread Ben Mahler

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


Ship it!





src/master/allocator/mesos/hierarchical.cpp (line 1488)
<https://reviews.apache.org/r/47028/#comment197044>

How about "No allocations performed"? It is a bit odd to refer to offers 
here since we don't know about offers in the allocator currently.


- Ben Mahler


On May 11, 2016, 5:58 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47028/
> ---
> 
> (Updated May 11, 2016, 5:58 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When there is no offer to send out at the end of an allocation run it
> could be entirely possible that it's because all framworks have
> suppressed or filtered offers. "No offers to send out!" simply states
> the case.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 0de03c7347e01fde2b42f5ec38a34a62edf642a1 
> 
> Diff: https://reviews.apache.org/r/47028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>



Review Request 47192: Fixed a head-of-line blocking bug in libevent SSL socket.

2016-05-10 Thread Ben Mahler

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

Review request for mesos, haosdent huang, Joris Van Remoortere, and Till 
Toenshoff.


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


Repository: mesos


Description
---

Currently, the `accept_queue` is used to store connected sockets.
However, we push socket futures into this queue *before* they
complete the SSL handshake or are downgraded. This means that
a future returned from the queue may remain pending. If the user
writes a `Socket::accept` loop consuming accepted sockets they
will experience head-of-line blocking while a slow handshake or
downgrade is in progress.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 
e773fadeb31ca384264a425bdc8d093804e45a82 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
b829e7dd2c0e7730ac1579c042f79f40666b19b9 

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


Testing
---

make check passes

Manually tested SSL blocking issue:

Start the master with SSL enabled:
```
$ SSL_CERT_FILE=~/certificates/localhost.crt 
SSL_KEY_FILE=~/certificates/localhost.key SSL_ENABLED=true 
./bin/mesos-master.sh --work_dir=/tmp
```

Inject a pending socket into the Queue:
```
$ telnet localhost 5050
Trying ::1...
telnet: connect to address ::1: Connection refused
Trying 127.0.0.1...
Connected to localhost.
Escape character is '^]'.
```

Hit the endpoints:
```
$ curl --insecure https://localhost:5050/health
```

These curl requests hangs without this fix.


Thanks,

Ben Mahler



Re: Review Request 46325: Updated HTTP command executor to support kill policy in Kill event.

2016-05-09 Thread Ben Mahler

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


Ship it!




Modulo comments.

- Ben Mahler


On May 7, 2016, 1:03 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46325/
> ---
> 
> (Updated May 7, 2016, 1:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Kill policy can be provided in a kill event. In this case it should
> take precedence over kill policy specified when the task was launched.
> When kill event is issued multiple times during the task termination,
> the signal escalation timeout (the time a task has between SIGTERM
> and SIGKILL) may be reduced.
> 
> Since updating the delay timer (we use it for signal escalation delay)
> is currently not possible, we cancel the existing signal timer and set
> up a new one. `Clock::cancel()` guarantees that, if existed, the timer
> is removed before the function returns; hence we do not set up more
> than 1 timer for signal escalation delay.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46325/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Tested manually using modified `mesos-execute` in a way that **two** extra 
> kill task requests are sent, 2s and 3s after receiving `TASK_RUNNING`. Each 
> kill task request specifies `KillPolicy` with 1s grace period. Together with 
> a kill *without* a kill policy scheduled 1s after the task is being launched, 
> the task receives **three** kill requests in total.
> 
> Master: `./bin/mesos-master.sh --work_dir=/tmp/w/m --ip=127.0.0.1`
> Agent: `./bin/mesos-slave.sh --work_dir=/tmp/w/s --master=127.0.0.1:5050 
> --http_command_executor`
> Mesos-execute: `./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}' 
> --kill_after=1secs`
> 
> HTTP command executor log:
> ```
> Received SUBSCRIBED event
> Subscribed executor on alexr.fritz.box
> Received LAUNCH event
> Starting task test
> sh -c '/Users/alex/bin/unresponsive_process'
> Forked command at 17475
> 14455919081943275466
> Received ACKNOWLEDGED event
> 17172602460659762152
> Received KILL event
> Received kill for task test
> Sending SIGTERM to process tree at pid 17475
> Sent SIGTERM to the following process trees:
> [ 
> --- 17475 /Users/alex/bin/unresponsive_process
> ]
> 4381544758593790168
> Scheduling escalation to SIGKILL in 3secs from now
> Received ACKNOWLEDGED event
> Received KILL event
> Received kill for task test
> Rescheduling escalation to SIGKILL in 1secs from now
> 10370891801885978953
> Process 17475 did not terminate after 1secs, sending SIGKILL to process tree 
> at 17475
> Killed the following process trees:
> [ 
> --- 17475 /Users/alex/bin/unresponsive_process
> ]
> Received KILL event
> Received kill for task test
> Command terminated with signal Killed: 9 (pid: 17475)
> ```
> 
> Excerpt from the agent log that shows all 3 kill task requests and that the 
> segnal escalation timeout was reduced from 3s to 1s:
> ```
> I0418 14:27:17.825070 244285440 slave.cpp:3605] Forwarding the update 
> TASK_RUNNING (UUID: 925e2d89-f6eb-464d-9a50-a74a8e07bc88) for task test of 
> framework ab374773-a018-4531-923b-899cf1e4f573- to master@127.0.0.1:5050
> I0418 14:27:17.831233 242139136 status_update_manager.cpp:392] Received 
> status update acknowledgement (UUID: 925e2d89-f6eb-464d-9a50-a74a8e07bc88) 
> for task test of framework ab374773-a018-4531-923b-899cf1e4f573-
> I0418 14:27:18.834309 244285440 slave.cpp:2046] Asked to kill task test of 
> framework ab374773-a018-4531-923b-899cf1e4f573-
> I0418 14:27:18.842150 244285440 http.cpp:178] HTTP POST for 
> /slave(1)/api/v1/executor from 192.168.178.24:54206
> I0418 14:27:18.842331 244285440 slave.cpp:3207] Handling status update 
> TASK_KILLING (UUID: a2f6eca7-b3e5-4e45-adcb-356f75355563) for task test of 
> framework ab374773-a018-4531-923b-899cf1e4f573-
> I0418 14:27:18.843214 242139136 status_update_manager.cpp:320] Received 
> status update TASK_KILLING (UUID: a2f6eca7-b3e5-4e45-adcb-356f7535556

Re: Review Request 46491: Ensured subsequent kills are ignored after the task is reaped.

2016-05-09 Thread Ben Mahler

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


Ship it!




Modulo comments.

- Ben Mahler


On May 7, 2016, 1:02 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46491/
> ---
> 
> (Updated May 7, 2016, 1:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier and Ben Mahler.
> 
> 
> Bugs: MESOS-5240
> https://issues.apache.org/jira/browse/MESOS-5240
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Capture the state when a task has terminated (i.e., reaped) in a flag,
> and use it to prevent calls to `killTask()` and `escalated()` when
> they are executed after the task has terminated.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46491/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:45 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the accidental dependency.


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


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

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


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:45 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the  include as it is now no longer needed.


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


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs (updated)
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

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


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Re: Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

(Updated May 8, 2016, 12:41 a.m.)


Review request for mesos and Anand Mazumdar.


Changes
---

Removed the accidental dependency.


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


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

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


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Review Request 47092: Removed misleading return values from process::network::.

2016-05-07 Thread Ben Mahler

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

Review request for mesos, Anand Mazumdar and Michael Park.


Repository: mesos


Description
---

Some of the methods in the process::network namespace were returning
the 0 or -1 return code of the underlying socket system call. These
functions will always return 0 when the Try is Some.


Diffs
-

  3rdparty/libprocess/include/process/network.hpp 
9cc4acd11dba561f40c33bc9dabb35a452a80e62 
  3rdparty/libprocess/src/poll_socket.cpp 
e68c7836b6a79253fa646c1919d0f331f21ec131 
  3rdparty/libprocess/src/socket.cpp ec0e913aca30f92d4a0543ad1e685b897617bca3 

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


Testing
---

make check


Thanks,

Ben Mahler



Review Request 47091: Made http::internal::ConnectionProcess a managed Process.

2016-05-07 Thread Ben Mahler

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

Review request for mesos and Anand Mazumdar.


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


Repository: mesos


Description
---

Per the details in MESOS-4658, the `http::Connection` abstraction is
prone to the deadlock of `process::wait`ing on itself. This occurs
when the `http::internal::ConnectionProcess` exposes a Future on
which the caller binds a copy of `http::Connection`. When completing
the Future, the Future clears its callbacks within the execution
context of the `http::internal::ConnectionProcess`. If the last copy
of the `http::Connection` was present within one of the callbacks,
the `http::internal::ConnectionProcess` will wait on itself, leading
to a deadlock.

This surfaces a general pattern of having libprocess manage a
Process when it cannot be guaranteed that the Process will be
waited on by another execution context.


Diffs
-

  3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 

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


Testing
---

Removed the hack put in place within the http::get path to validate the fix.


Thanks,

Ben Mahler



Review Request 47090: Removed side-effects from MetricsTest.SnapshotAuthenticationEnabled.

2016-05-07 Thread Ben Mahler

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

Review request for mesos and Greg Mann.


Repository: mesos


Description
---

The test was previously adding metrics without removing them, which
meant that the test fails when run in repetition. Since this test
only cares about authentication, we can simply remove the metrics
from the test entirely.


Diffs
-

  3rdparty/libprocess/src/tests/metrics_tests.cpp 
1cda7b4ec31fcd06161925ce5788741f299217c7 

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


Testing
---

Ran in repetition.


Thanks,

Ben Mahler



Re: Review Request 46730: Cleanup syscalls logic.

2016-05-06 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 29, 2016, 1:03 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46730/
> ---
> 
> (Updated April 29, 2016, 1:03 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Zhiwei Chen, and haosdent huang.
> 
> 
> Bugs: MESOS-5263
> https://issues.apache.org/jira/browse/MESOS-5263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `` and removed uncessary condition complie.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 81f040e5c7ed7cbca569f5d43cb5afc5da1b5e64 
> 
> Diff: https://reviews.apache.org/r/46730/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>



Re: Review Request 46960: Remove un-necessary copying of `slave->tasks` in master.

2016-05-05 Thread Ben Mahler

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


Fix it, then Ship it!





src/master/master.cpp (lines 5973 - 5976)
<https://reviews.apache.org/r/46960/#comment196011>

Can we just copy the keys here instead of the entire maps?

```
  foreach (const FrameworkID& frameworkId, slave->executors.keys()) {
foreach (const ExecutorID& executorId, 
slave->executors[frameworkId].keys()) {
    ```


- Ben Mahler


On May 4, 2016, 5:14 a.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46960/
> ---
> 
> (Updated May 4, 2016, 5:14 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It seems that we removed an else block in 0.26 that removed the
> `removeTask` call from the `foreachvalue` loop but we still kept
> on copying `slave->tasks` though it is not needed anymore.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 6d3e0f7c634690a35eec1ce827b705e04c3af87e 
> 
> Diff: https://reviews.apache.org/r/46960/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>



Re: Review Request 46325: Updated HTTP command executor to support kill policy in Kill event.

2016-05-05 Thread Ben Mahler

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




src/launcher/http_command_executor.cpp (lines 569 - 575)
<https://reviews.apache.org/r/46325/#comment195958>

It looks like we have to re-assign kill_policy! Otherwise, if kill is 
called internally (e.g. health check failure) after a Kill message with a 
kill_policy arrives, we may accidentally change the grace period!

Something like:

```
if (override.isSome()) {
  killPolicy = override.get();
}
```

If we don't re-assign we will take the old one from TaskInfo when kill is 
called by the executor itself.



src/launcher/http_command_executor.cpp (line 615)
<https://reviews.apache.org/r/46325/#comment195950>

How about?

```
// If the task is being killed but hasn't terminated and
// we receive another kill request, we see if we need
// to adjust the remaining grace period.
```



src/launcher/http_command_executor.cpp (line 617)
<https://reviews.apache.org/r/46325/#comment195957>

grace period



src/launcher/http_command_executor.cpp (lines 622 - 625)
<https://reviews.apache.org/r/46325/#comment195956>

The second bullet isn't really a limitation here right?

Since we track when the grace period started we're now just adjusting the 
end of the grace period to reflect the newly desired duration, so we can easily 
support shrinking and growing.



src/launcher/http_command_executor.cpp (lines 627 - 633)
<https://reviews.apache.org/r/46325/#comment195951>

This is helpful, thanks! Some adjustment suggestions:

```
  // Here are some examples to illustrate:
  //
  // 20, 30 -> Increased grace period is a no-op, grace period remains 
20.
  // 20, 20 -> Retries are a no-op, grace period remains 20.
  // 20, 5  -> if elapsed >= 5:
  // SIGKILL immediately (total grace period is elapsed).
  //   if elapsed < 5:
  // SIGKILL in 5 - elapsed (total grace period is 5).
```



src/launcher/http_command_executor.cpp (lines 635 - 637)
<https://reviews.apache.org/r/46325/#comment195955>

How about re-ordering this?

```
if (killingInitiated + gracePeriod > escalationTimer.timeout().time()) {
  return;
}
```

Since this is an "increase" it seems nice to read that the new grace period 
end is "larger" than the existing one.



src/launcher/http_command_executor.cpp (lines 639 - 642)
<https://reviews.apache.org/r/46325/#comment195949>

"elasped" and "remaining" are more complimentary here, 
"overriddenGracePeriod" suggests the grace period but it's really just the 
remaining time on the timer:

```
  Duration elapsed = Clock::now() - killGracePeriodStart;
  Duration remaining = gracePeriod - elapsed;
```

Why did you have the Duration::zero guard, you shouldn't need it?



src/launcher/http_command_executor.cpp (lines 644 - 645)
<https://reviews.apache.org/r/46325/#comment195948>

How about:

```
cout << "Received a new kill policy grace period of << gracePeriod << "; 
shortening remaining grace period time to " << remaining;
```



src/launcher/http_command_executor.cpp (lines 687 - 690)
<https://reviews.apache.org/r/46325/#comment195947>

It would be nice to have `killingInitiated + gracePeriod == 
escalationTimer.timeout().time()` but they are slightly off right now:

killingInitiated = 0.9
escalationTimer = 1.0 + 5 (gracePeriod)

This means your code above will tend to print out "re-scheduling" on the 
first retry.

One suggestion is to store killingInitiated after you start the timer to 
avoid this. Ideally, we could do this:

```
killGracePeriodTimer = delay();
killGracePeriodStart = timer.start_time();
```

But we can't get the start time out of a timer currently.



src/launcher/http_command_executor.cpp (lines 881 - 884)
<https://reviews.apache.org/r/46325/#comment195929>

Per our offline chat, readers here likely won't be wondering why there are 
two variables for these (that has a lot of context of the road you went down 
trying to consolidate them into just the timer variable). Likely they're more 
interested in what these represent.
    
    If we change the naming, I think we can go without needing a comment here:

```
Option killGracePeriodStart;
Option killGracePeriodTimer;
```


- Ben Mahler


On May 5, 2016, 3:39 p.m., Alexander Rukletsov wrote:
> 
> ---
> This 

Re: Review Request 46321: Renamed a variable in command executors for clarity.

2016-05-05 Thread Ben Mahler

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



Let's punt on this rename in favor of introducing explicit task lifecycle 
states later.

When I see "killing" (since it is present tense) it suggests that it will be 
false once the task is terminated (or "killed"). However, that's not what the 
code is doing (it remains true even after the task terminates). There's also a 
second way to interpret "killed": the "kill" has been issued. If I were to see 
"killed" and "terminated" then I could intuit that these represent the kill 
being issued to the process and the process having terminated.

Also, could push the introduction of "terminated" to the patch that reads the 
value? It's only written to in this patch AFAICT.

- Ben Mahler


On May 5, 2016, 3:37 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46321/
> -------
> 
> (Updated May 5, 2016, 3:37 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When we kill (with SIGTERM) the underlying task, it does not
> necessarily mean it complies immediately, hence the name `killed`
> is misleading.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
>   src/launcher/executor.cpp 4d5e2a9b876cd65be1386bf170dba2f71af86750 
>   src/launcher/http_command_executor.cpp 
> d2f15b0447d91f3a4cd92f07114cb366647cc7d3 
> 
> Diff: https://reviews.apache.org/r/46321/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 46165: Removed dependency on Boost.Foreach.

2016-05-03 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On May 3, 2016, 8:53 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46165/
> ---
> 
> (Updated May 3, 2016, 8:53 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Ben Mahler, and 
> Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3214
> https://issues.apache.org/jira/browse/MESOS-3214
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed dependency on Boost.Foreach.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/foreach.hpp 
> 7fb0044790ee249b69e07b81a26851bd5bfb110f 
> 
> Diff: https://reviews.apache.org/r/46165/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>



Re: Review Request 46945: Renamed a shadowing variable.

2016-05-03 Thread Ben Mahler

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


Fix it, then Ship it!





3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp (lines 749 - 751)
<https://reviews.apache.org/r/46945/#comment195596>

Could we avoid the 'elem' name in favor of 'element' or 'v' (full word or 
single letter)?

Per our conversation, do we want to adjust the foreachpair above?


- Ben Mahler


On May 3, 2016, 10:54 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46945/
> ---
> 
> (Updated May 3, 2016, 10:54 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Hindman, Ben Mahler, and 
> Joris Van Remoortere.
> 
> 
> Bugs: MESOS-3214
> https://issues.apache.org/jira/browse/MESOS-3214
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The `value` variable declared in the `foreach` used to shadow the `value`
> function parameter. This seemed to have been ok with the `FOREACH` macro,
> butthis is no longer ok with `range-based for`.
> This seems to be the only place of violation of this.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/json.hpp 
> d1f4ae6a1d1e6ccfe55f9f8f78390826dc97d894 
> 
> Diff: https://reviews.apache.org/r/46945/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Ubuntu 14.04 with GCC 4.8
> 
> 
> Thanks,
> 
> Michael Park
> 
>



Re: Review Request 46804: Patched glog to eliminate system-level gflag library detection.

2016-04-28 Thread Ben Mahler
:0,
 from src/utilities.h:82,
 from src/logging_unittest.cc:33:
/usr/include/gflags/gflags.h:277:7: note:   ‘gflags::FlagSaver’
 class GFLAGS_DLL_DECL FlagSaver {
   ^
src/logging_unittest.cc:1206:134: error: expected ‘;’ before ‘fs’
 TEST(UserDefinedClass, logging) {

  ^
make[7]: *** [logging_unittest-logging_unittest.o] Error 1
```


Thanks,

Ben Mahler



Review Request 46805: Added backport of MESOS-5018 to 0.27.3.

2016-04-28 Thread Ben Mahler

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This reflects the backport of MESOS-5018 to the 0.27.x branch.


Diffs
-

  CHANGELOG 0c5987779e5f0e2cb416dfe885f7ba836e3958eb 

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


Testing
---

N/A


Thanks,

Ben Mahler



Review Request 46804: Patched glog to eliminate system-level gflag library detection.

2016-04-28 Thread Ben Mahler
:   ‘gflags::FlagSaver’
 class GFLAGS_DLL_DECL FlagSaver {
   ^
src/logging_unittest.cc:1206:134: error: expected ‘;’ before ‘fs’
 TEST(UserDefinedClass, logging) {

  ^
make[7]: *** [logging_unittest-logging_unittest.o] Error 1
```


Thanks,

Ben Mahler



Review Request 46753: Added backport of MESOS-5021 to 0.27.3.

2016-04-27 Thread Ben Mahler

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This reflects the backport of MESOS-5021 to the 0.27.x branch.


Diffs
-

  CHANGELOG 937716068c126fd76b70e08f7654a659ba1029d8 

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


Testing
---

N/A


Thanks,

Ben Mahler



Re: Review Request 44255: Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.

2016-04-27 Thread Ben Mahler

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



The `messages_*` metrics were originally added to track the number of each 
message type received by the master in the old libprocess message-passing API:
https://github.com/apache/mesos/blob/master/src/messages/messages.proto

However, we then introduced the Event/Call based API for schedulers/executors:
https://github.com/apache/mesos/blob/master/include/mesos/scheduler/scheduler.proto

Since these metrics are related to Accept-type Calls rather than the old 
message passing style, it seems we should add Call-based metrics to incorporate 
these:

```
master/scheduler_calls: 1024
master/scheduler_calls/type/accept: 512
master/scheduler_calls/type/accept/operations/launch: 256
master/scheduler_calls/type/accept/operations/reserve: 64
master/scheduler_calls/type/accept/operations/unreserve: 64
master/scheduler_calls/type/accept/operations/create: 64
master/scheduler_calls/type/accept/operations/destroy: 64
master/scheduler_calls/type/decline: 512
master/scheduler_calls/type/kill: 512
...
```

Also, this lets us distinguish between schedulers making calls and operators 
hitting the endpoints (the current patch treats these the same). We should get 
some feedback from Vinod Kone and Anand Mazumdar since they've been working on 
the HTTP API.

- Ben Mahler


On March 24, 2016, 1:54 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44255/
> ---
> 
> (Updated March 24, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Guangya Liu, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4492
> https://issues.apache.org/jira/browse/MESOS-4492
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add metrics for {RESERVE, UNRESERVE, CREATE, DESTROY} offer operation.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 323d01d99456a71bd384faf186264e3fc4bf2207 
>   src/master/http.cpp 950206baf7f3a1cccdc49d810126473966d8d021 
>   src/master/master.cpp 8d6d3c6468c6b85fe09c33cf9747cc3d1f515ab9 
>   src/master/metrics.hpp 9d201fcce1c46a890c86a889ab31029f9a061561 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
> 
> Diff: https://reviews.apache.org/r/44255/diff/
> 
> 
> Testing
> ---
> 
> ChangLog:
> 
> v3:
>   - Move the couting out of common code path to http endpoint and master 
> accept call separately to reflect its logic.
> 
> v2:
>   - Documenting those metrics
>   - Add test code for MetricsTest as suggested by Guangya
>   - post-review.py does not update original 
> RR(https://reviews.apache.org/r/44058/), but only create a new one even if I 
> rebased.
> 
> 
> Tests:
> 1. make check GTEST_FILTER="MetricsTest.Master" on Centos-7 
> (3.10.0-123.el7.x86_640)
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from MetricsTest
> [ RUN  ] MetricsTest.Master
> [   OK ] MetricsTest.Master (211 ms)
> [--] 1 test from MetricsTest (211 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (244 ms total)
> [  PASSED  ] 1 test
> 
> 2. Verify its functionality with 'reserve' http endpoint as an test case
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 0.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> # curl -i -d slaveId=6250553a-2f39-4a92-9073-4618d130f433-S1  -d resources='[ 
> { "name": "cpus", "type": "SCALAR","scalar": { "value": 1 
> },"reservation":{"principal": "XiaoHaHa"}}  ]' -X POST  
> ipdc02-kvm-guest2:5050/master/reserve
> HTTP/1.1 200 OK
> Date: Fri, 26 Feb 2016 19:59:01 GMT
> Content-Length: 0
> 
> # curl http://ipdc02-kvm-guest2:5050/metrics/snapshot  | python -mjson.tool | 
> grep reserve
> "master/messages_reserve_resource": 1.0,
> "master/messages_unreserve_resource": 0.0,
> 
> 
> Thanks,
> 
> fan du
> 
>



Re: Review Request 46730: Cleanup syscalls logic.

2016-04-27 Thread Ben Mahler

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



Could you also update the testing section? How did you ensure this compiles on 
the affected architectures?


src/linux/cgroups.cpp (line 25)
<https://reviews.apache.org/r/46730/#comment194729>

Hm.. where is this header located?

I'd expect to find a unistd.h here:
https://github.com/apache/mesos/tree/master/src/linux



src/linux/ns.hpp (lines 160 - 161)
<https://reviews.apache.org/r/46730/#comment194730>

This looks like a similar change but it would be great to split it into a 
separate patch. It's not immediately obvious to me why `SYS_setns` is being 
changed in favor of `__NR_setns`. Can you add some some background information 
in the review description?


- Ben Mahler


On April 27, 2016, 11:44 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46730/
> ---
> 
> (Updated April 27, 2016, 11:44 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Zhiwei Chen, and haosdent huang.
> 
> 
> Bugs: MESOS-5263
> https://issues.apache.org/jira/browse/MESOS-5263
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used `` and removed uncessary condition complie.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp b57ec05d3e0bf0bc1bf50fca9a9ede767f204253 
>   src/linux/fs.cpp 81f040e5c7ed7cbca569f5d43cb5afc5da1b5e64 
>   src/linux/ns.hpp 244a811b299c29b1dcd6652bd26e861e04df3f54 
> 
> Diff: https://reviews.apache.org/r/46730/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>



Re: Review Request 46610: Fix 'pivot_root is not available' error on ARM.

2016-04-26 Thread Ben Mahler

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


Ship it!




Since this followed the existing approach (used by PowerPC) I've gone ahead and 
committed it.

The cleanup mentioned by haosdent sounds great if it removes unnecessary #ifdef 
logic. Let me know when the cleanup is available!

- Ben Mahler


On April 24, 2016, 11:36 a.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46610/
> ---
> 
> (Updated April 24, 2016, 11:36 a.m.)
> 
> 
> Review request for mesos and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'pivot_root is not available' error on ARM.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 2087b4ac1503e0fd085319b1017389f1f947536f 
> 
> Diff: https://reviews.apache.org/r/46610/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>



Review Request 46663: Added 0.28.2 to the CHANGELOG.

2016-04-25 Thread Ben Mahler

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This reflects the backport of MESOS-4705 to the 0.28.x branch.


Diffs
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 

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


Testing
---

N/A


Thanks,

Ben Mahler



Review Request 46664: Added 0.27.3 to the CHANGELOG.

2016-04-25 Thread Ben Mahler

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This reflects the backport of MESOS-4705 to the 0.27.x branch.


Diffs
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 

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


Testing
---

N/A


Thanks,

Ben Mahler



Review Request 46665: Added 0.26.2 to the CHANGELOG.

2016-04-25 Thread Ben Mahler

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

This reflects the backport of MESOS-4705 to the 0.26.x branch.


Diffs
-

  CHANGELOG 57f5457640c9b7ca2cb7707f2962dd0421f090f7 

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


Testing
---

N/A


Thanks,

Ben Mahler



Re: Review Request 44379: Use tokens size to parse perf stat format.

2016-04-25 Thread Ben Mahler

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


Ship it!




Thanks for the comment!

I've added a commit description and made some minor adjustments that I'll 
commit with your change:

```
commit a5c81d4077400892cd3a5c306143f16903aac62c
Author: fan du <fan...@intel.com>
Date:   Mon Apr 25 13:50:50 2016 -0700

Fixed the 'perf' parsing logic.

Previously the 'perf' parsing logic used the kernel version to
determine the token ordering. However, this approach breaks
when distributions backport perf parsing changes onto older
kernel versions. This updates the parsing logic to understand
all existing formats.

Co-authored with haosdent.

Review: https://reviews.apache.org/r/44379/

diff --git a/src/linux/perf.cpp b/src/linux/perf.cpp
index 749e676..2364ab5 100644
--- a/src/linux/perf.cpp
+++ b/src/linux/perf.cpp
@@ -343,28 +343,24 @@ struct Sample
 // because the unit field can be empty.
 vector tokens = strings::split(line, PERF_DELIMITER);

-if (version >= Version(4, 0, 0)) {
-  // Optional running time and ratio were introduced in Linux v4.0,
-  // which make the format either:
-  //   value,unit,event,cgroup
-  //   value,unit,event,cgroup,running,ratio
-  if ((tokens.size() == 4) || (tokens.size() == 6)) {
-return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
-  }
-} else if (version >= Version(3, 13, 0)) {
-  // Unit was added in Linux v3.13, making the format:
-  //   value,unit,event,cgroup
-  if (tokens.size() == 4) {
-return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
-  }
-} else {
-  // Expected format for Linux kernel <= 3.12 is:
-  //   value,event,cgroup
-  if (tokens.size() == 3) {
-return Sample({tokens[0], internal::normalize(tokens[1]), tokens[2]});
-  }
+// The following formats are possible:
+//   (1) value,event,cgroup (since Linux v2.6.39)
+//   (2) value,unit,event,cgroup (since Linux v3.14)
+//   (3) value,unit,event,cgroup,running,ratio (since Linux v4.1)
+//
+// Note that we do not use the kernel version when parsing
+// because OS vendors often backport perf tool functionality
+// into older kernel versions.
+
+if (tokens.size() == 3) {
+  return Sample({tokens[0], internal::normalize(tokens[1]), tokens[2]});
+}
+
+if (tokens.size() == 4 || tokens.size() == 6) {
+  return Sample({tokens[0], internal::normalize(tokens[2]), tokens[3]});
 }

+// Bail out if the format is not recognized.
 return Error("Unexpected number of fields");
   }
 };
```

- Ben Mahler


On April 18, 2016, 5:41 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44379/
> ---
> 
> (Updated April 18, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4705
> https://issues.apache.org/jira/browse/MESOS-4705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Co-authored with haosdent.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 749e676aaf2ce639dd976f2b23e323300c6114c5 
> 
> Diff: https://reviews.apache.org/r/44379/diff/
> 
> 
> Testing
> ---
> 
> 1. {Found and Test} with Serenity, ema filter could get perf event statistics 
> correctly as expected.
> 2. ./bin/mesos-tests.sh --gtest_filter=PerfEventIsolatorTest* 
> --log_dir=/tmp/mesos/
> 
> 
> Thanks,
> 
> fan du
> 
>



Re: Review Request 46325: Updated HTTP command executor to support kill policy in Kill event.

2016-04-21 Thread Ben Mahler

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



Could you pull out the 'killed' bug fix? Any reason it's bundled in this patch?


src/launcher/http_command_executor.cpp (lines 172 - 176)
<https://reviews.apache.org/r/46325/#comment193489>

Wrap at the paren:

```
  kill(event.kill().task_id(),
   event.kill().has_kill_policy()
 ? Option(event.kill().kill_policy())
 : None());
```

Any way to avoid the explicit Option construction? Does this compile?

```
  kill(event.kill().task_id(),
   event.kill().has_kill_policy()
? Some(event.kill().kill_policy())
: None());
```

If not, consider pulling out the kill policy as an option variable.



src/launcher/http_command_executor.cpp (lines 603 - 605)
<https://reviews.apache.org/r/46325/#comment193490>

How about s/overridingKillPolicy/override/ ?

Alternatively, if we took the `Event::Kill kill` then we could distinguish 
easily:

```
kill.kill_policy(); // vs
killPolicy; // the member variable
```



src/launcher/http_command_executor.cpp (lines 656 - 661)
<https://reviews.apache.org/r/46325/#comment193492>

The addition of 'killed' and this check looks like a bug fix? Any reason it 
is being bundled with this patch?



src/launcher/http_command_executor.cpp (line 665)
<https://reviews.apache.org/r/46325/#comment193496>

Leaving this unimplemented as a TODO seems to have implications on the 
documentation in your previous patch:

https://reviews.apache.org/r/46322/

It seems like all you would need to do to implement the TODO is to remove 
this if condition and make the logging more clear? Otherwise, if you leave this 
out we really need to document it within the Kill Call. My hunch is that in 
writing that documentation we'll realize we should just do an override always.



src/launcher/http_command_executor.cpp (lines 783 - 787)
<https://reviews.apache.org/r/46325/#comment193497>

The addition of 'killed' and this check looks like a bug fix? Any reason it 
is being bundled with this patch?


- Ben Mahler


On April 21, 2016, 2:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46325/
> ---
> 
> (Updated April 21, 2016, 2:28 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Qian Zhang.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Kill policy can be provided in a kill event. In this case it should
> take precedence over kill policy specified when the task was launched.
> When kill event is issued multiple times during the task termination,
> the signal escalation timeout (the time a task has between SIGTERM
> and SIGKILL) may be reduced.
> 
> Since updating the delay timer (we use it for signal escalation delay)
> is currently not possible, we cancel the existing signal timer and set
> up a new one. `Clock::cancel()` guarantees that, if existed, the timer
> is removed before the function returns; hence we do not set up more
> than 1 timer for signal escalation delay.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> 9dfe48108cababeb2d4a6af74434880d79245c21 
> 
> Diff: https://reviews.apache.org/r/46325/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> Tested manually using modified `mesos-execute` in a way that **two** extra 
> kill task requests are sent, 2s and 3s after receiving `TASK_RUNNING`. Each 
> kill task request specifies `KillPolicy` with 1s grace period. Together with 
> a kill *without* a kill policy scheduled 1s after the task is being launched, 
> the task receives **three** kill requests in total.
> 
> Master: `./bin/mesos-master.sh --work_dir=/tmp/w/m --ip=127.0.0.1`
> Agent: `./bin/mesos-slave.sh --work_dir=/tmp/w/s --master=127.0.0.1:5050 
> --http_command_executor`
> Mesos-execute: `./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --command="/Users/alex/bin/unresponsive_process" --env='{"GLOG_v": "2"}' 
> --kill_after=1secs`
> 
> HTTP command executor log:
> ```
> Received SUBSCRIBED event
> Subscribed executor on alexr.fritz.box
> Received LAUNCH event
> Starting task test
> sh -c '/Users/alex/bin/unresponsive_process'
> Fork

Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-21 Thread Ben Mahler

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


Fix it, then Ship it!





src/slave/slave.cpp (lines 2041 - 2042)
<https://reviews.apache.org/r/46323/#comment193488>

const & for both of these since the rhs is not a temporary


- Ben Mahler


On April 21, 2016, 2:28 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> ---
> 
> (Updated April 21, 2016, 2:28 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
>   src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
>   src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46323/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 46324: Corrected indentation in HttpCommandExecutor.

2016-04-18 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46324/
> ---
> 
> (Updated April 18, 2016, 12:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> ad484e0e6f5067b6c166111c91b1ff1e8c05d9ac 
> 
> Diff: https://reviews.apache.org/r/46324/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 46323: Propagated KillPolicy in kill task from scheduler to executor.

2016-04-18 Thread Ben Mahler

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



Looks good, but held off on a ship it because there is a bug in the agent's 
message handler, see below.


src/internal/evolve.hpp (line 46)
<https://reviews.apache.org/r/46323/#comment192878>

Why did you choose to inject it here? Seems better closer to TaskInfo?



src/master/master.cpp (lines 4060 - 4061)
<https://reviews.apache.org/r/46323/#comment192888>

Since it seems straightforward, why not just handle the field instead of 
introducing this NOTE? It seems nice to add the straightforward support here 
and decide on the old API later (if at all).



src/slave/slave.hpp (line 149)
<https://reviews.apache.org/r/46323/#comment192881>

Converting an optional field into an Option is not yet supported, unless 
I'm missing something?

See: https://issues.apache.org/jira/browse/MESOS-2638

Note that I added the current workaround to that ticket description, you 
can take the entire message in the handler to check the optionality.


- Ben Mahler


On April 18, 2016, 12:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46323/
> ---
> 
> (Updated April 18, 2016, 12:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/internal/evolve.hpp cff6b657a6d69ff9e7f7dedc965122a03a200da1 
>   src/internal/evolve.cpp 4ca2abacd75523de8fb4ffe69a9edd8627f512af 
>   src/master/master.cpp 6dacf5fbd73771e5c31ffb0e8723cd2905dcefb3 
>   src/slave/slave.hpp f78c1b4e4d5378ef7223c6eb3ea45491c30fb4c1 
>   src/slave/slave.cpp d82dec2b10d496065013eb4ad6a35dc054b72553 
>   src/tests/mesos.hpp e4b63d41d883807ac39846799468b80e88c84e0b 
>   src/tests/mesos.cpp b5937af7713e1ee2af475518b3e968b2defa8beb 
>   src/tests/slave_tests.cpp ee58488b0b927c7c5833add4718941539663e6d2 
> 
> Diff: https://reviews.apache.org/r/46323/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 46322: Added KillPolicy to scheduler and executor Kill protobuf messages.

2016-04-18 Thread Ben Mahler

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


Fix it, then Ship it!





CHANGELOG (line 16)
<https://reviews.apache.org/r/46322/#comment192872>

The task kill policy defined within TaskInfo can now be overridden when the 
scheduler kills the task. This can be used by schedulers to forcefully kill a 
task which is already being killed, e.g. if something went wrong during a 
graceful kill and a forcible kill is desired. Note that it is the executor's 
responsibility to honor the 'Event.kill.kill_policy' field and override the 
task's kill policy. To use this feature, schedulers and executors must support 
HTTP API; use the `--http_command_executor` agent flag to ensure the agent 
launches the HTTP API based command executor.



CHANGELOG (line 48)
<https://reviews.apache.org/r/46322/#comment192870>

whoops? please commit this separately. But notice that there are a number 
of inconsistent past vs. present tense wording in the CHANGELOG, poor grammar, 
etc. I'd rather see a sweep on a section (e.g. 0.29.0) to minimize history 
churn.



CHANGELOG (lines 61 - 63)
<https://reviews.apache.org/r/46322/#comment192868>

I was initially surprised to see MESOS-4908 repeated here, but I suppose 
the intent was to list all non-deprecation API changes here? If so, we're not 
doing that already (e.g. MESOS-4909 and MESOS-4949 in 0.29.0 for example).

It seems the current approach is that 'new features' may include some API 
changes, but these aren't repeated in 'Additional API' changes. While it would 
be great to have a clearer approach, can you follow the existing approach?



CHANGELOG (line 62)
<https://reviews.apache.org/r/46322/#comment192873>

no need for the "'s" here:

s/scheduler's/scheduler/
s/executor's/executor/



include/mesos/executor/executor.proto (lines 90 - 91)
<https://reviews.apache.org/r/46322/#comment192875>

All of the .proto comments introduced here would benefit from also 
mentioning that the kill policy overrides any previously specified 
kill.kill_policy. Currently it only seems clear that the kill.kill_policy 
overrides task_info.kill_policy. 

Perhaps for now, we just explicitly state that the grace period may be 
"overridden" (or "adjusted"?) in order to give more or less time to a graceful 
kill that is in progress.


- Ben Mahler


On April 18, 2016, 12:43 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46322/
> ---
> 
> (Updated April 18, 2016, 12:43 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4908
> https://issues.apache.org/jira/browse/MESOS-4908
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A framework may want to override the `KillPolicy` set in `TaskInfo`
> when killing a task, for example to forcefully kill a task which is
> already being killed.
> 
> 
> Diffs
> -
> 
>   CHANGELOG d2e902f8295644c527964123e409be460c2a5789 
>   include/mesos/executor/executor.proto 
> 338b3638f986244122c2d39c9aca7905c12008ce 
>   include/mesos/scheduler/scheduler.proto 
> 078c6550f24a3d8ac675251168434130fc3eeef3 
>   include/mesos/v1/executor/executor.proto 
> 4552fb5d3f9d53affd8fad0abf122fce548973b7 
>   include/mesos/v1/scheduler/scheduler.proto 
> 8ed9e19a9e5aa19a518b708b0e0d9cfdc038cd11 
>   src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c 
> 
> Diff: https://reviews.apache.org/r/46322/diff/
> 
> 
> Testing
> ---
> 
> The whole chain is tested in https://reviews.apache.org/r/46325/
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>



Re: Review Request 45304: Change Call and Event Type enums in executor.proto optional.

2016-04-18 Thread Ben Mahler

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




src/examples/test_http_executor.cpp (lines 192 - 194)
<https://reviews.apache.org/r/45304/#comment192866>

Yikes, this case isn't actually unreachable!

Per MESOS-2664 and MESOS-3754, please avoid 'default' in favor of an 
explicit 'case UNKNOWN' so that the compiler helps us catch all switches when 
we introduce a new enum value.

Ditto below.


- Ben Mahler


On March 25, 2016, 9:25 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45304/
> ---
> 
> (Updated March 25, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5015
> https://issues.apache.org/jira/browse/MESOS-5015
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix changes Call and Event Type enums in executor.proto
> optional for the purpose of backwards compatibility. (MESOS-5015)
> 
> 
> Diffs
> -
> 
>   CHANGELOG cd551d896e8ba3ec9df17cc477f0abdd87917ab3 
>   include/mesos/executor/executor.proto 
> 6b4141f0aac27be5a4998f16e986fa87a7047834 
>   include/mesos/v1/executor/executor.proto 
> d7b1dd5eec9bcd424c7859d89471148bce8fe16e 
>   src/examples/test_http_executor.cpp 
> 562b0acfd8555b9b773175f53defe0e7e2744641 
>   src/slave/validation.cpp bc8d6717eac103c41f8cc8720e8482589210ea72 
>   src/tests/mesos.hpp 23694885a69ddcbc7039de1186093ce0ad5eed22 
> 
> Diff: https://reviews.apache.org/r/45304/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>



Re: Review Request 45317: Change Call and Event Type enums in scheduler.proto optional.

2016-04-18 Thread Ben Mahler

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




src/tests/mesos.hpp (lines 932 - 935)
<https://reviews.apache.org/r/45317/#comment192865>

Yikes, this case isn't actually unreachable!

Per MESOS-2664 and MESOS-3754, please avoid 'default' in favor of an 
explicit 'case UNKNOWN' so that the compiler helps us catch all switches when 
we introduce a new enum value.


- Ben Mahler


On March 25, 2016, 8:55 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45317/
> ---
> 
> (Updated March 25, 2016, 8:55 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5014
> https://issues.apache.org/jira/browse/MESOS-5014
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix changes Call and Event Type enums in scheduler.proto
> optional for the purpose of backwards compatibility. (MESOS-5014)
> 
> 
> Diffs
> -
> 
>   CHANGELOG fef0cbfc3ae282707569429d38e52c19d4eb9aba 
>   include/mesos/scheduler/scheduler.proto 
> 0049e1383f50574c3dad6a29b91811001694e82c 
>   include/mesos/v1/scheduler/scheduler.proto 
> 09fafedd06837f2307fedc6fa0e7b4460b21e5b0 
>   src/master/validation.cpp 701a5c4b279f319dde15bd8f2e97b5fd8608e578 
>   src/tests/mesos.hpp aaef158e5784ce077ef60996ebbeb77b356b7c57 
> 
> Diff: https://reviews.apache.org/r/45317/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>



Re: Review Request 45342: Make the Action enum optional to support upgrades (MESOS-5031).

2016-04-18 Thread Ben Mahler

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




src/authorizer/local/authorizer.cpp (line 204)
<https://reviews.apache.org/r/45342/#comment192864>

We generally avoid 'default' cases in enum switches because they allow the 
code to compile when a new enum value is introduced.

If you use a 'case UNKNOWN' instead, this will handle the unknown types and 
will prevent the code from compiling when a new enum value is introduced but 
not handled.

Make sense?

Ditto for the recent executor.proto and scheduler.proto changes.


- Ben Mahler


On March 29, 2016, 2:04 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45342/
> ---
> 
> (Updated March 29, 2016, 2:04 a.m.)
> 
> 
> Review request for mesos, Adam B, Anand Mazumdar, and Vinod Kone.
> 
> 
> Bugs: MESOS-5031
> https://issues.apache.org/jira/browse/MESOS-5031
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fix tries to make the Action enum in Authorization optional
> to support upgrades. See MESOS-4997 for details.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 944a493e0979c7ffbd99f3a67785a10425fd9040 
>   src/authorizer/local/authorizer.cpp 
> 0f0d9276337858984f0b19a82ffca74ee84dc650 
> 
> Diff: https://reviews.apache.org/r/45342/diff/
> 
> 
> Testing
> ---
> 
> make check (Ubuntu 14.04)
> 
> 
> Thanks,
> 
> Yong Tang
> 
>



Review Request 46339: Fixed the flaky MasterSlaveReconciliation.ReconcileRace test.

2016-04-18 Thread Ben Mahler

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

Review request for mesos and haosdent huang.


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


Repository: mesos


Description
---

Since the agent retries registration, we need to ensure that
any duplicate registration messages are flushed prior to
triggering a re-registration in the agent.


Diffs
-

  src/tests/master_slave_reconciliation_tests.cpp 
71fb78afe6ddca061dd05cfda7bbf17d4b3ea834 

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


Testing
---

Ran in repetition.


Thanks,

Ben Mahler



Re: Review Request 46175: Added a test for slavePostFetchHook.

2016-04-13 Thread Ben Mahler

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


Ship it!




Modulo comments from before.

- Ben Mahler


On April 13, 2016, 10:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46175/
> ---
> 
> (Updated April 13, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5209
> https://issues.apache.org/jira/browse/MESOS-5209
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for slavePostFetchHook.
> 
> 
> Diffs
> -
> 
>   src/examples/test_hook_module.cpp 3ff9fd71c275fd1a705ab3aca7a8041f29289bb0 
>   src/tests/hook_tests.cpp 97ff55ac7ec875b5ba5d4f17d80646c2d1dd4142 
> 
> Diff: https://reviews.apache.org/r/46175/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh 
> --gtest_filter=*ROOT_DOCKER_VerifySlavePostFetchHook* `
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 46173: Added a slave post fetch hook.

2016-04-13 Thread Ben Mahler

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


Ship it!





src/hook/manager.cpp (lines 243 - 244)
<https://reviews.apache.org/r/46173/#comment192234>

Would you mind getting the quotes on the same line?

```
LOG(WARNING) << "Failed to execute slave post fetch hook for module"
 << " '" << name << "': " << result.error();
```


- Ben Mahler


On April 13, 2016, 10:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46173/
> ---
> 
> (Updated April 13, 2016, 10:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5209
> https://issues.apache.org/jira/browse/MESOS-5209
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a slave post fetch hook.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 87e01a9dccb04bfdd5395867a45c46574815731b 
>   src/hook/manager.hpp 3af28a76493a95a3427fecb3a23dd80ecb84dfe9 
>   src/hook/manager.cpp 17a42f8362f58f0857acabeb2c3113354589fa1b 
>   src/slave/containerizer/docker.cpp 9c24227171c88ee89b567ebc5200a9563dfff5be 
>   src/slave/containerizer/mesos/containerizer.cpp 
> c25fa92d2a5fa9c828e77c3c0f8b1f795d1b8440 
> 
> Diff: https://reviews.apache.org/r/46173/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 46175: Added a test for slavePostFetchHook.

2016-04-13 Thread Ben Mahler

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




src/examples/test_hook_module.cpp (line 197)
<https://reviews.apache.org/r/46175/#comment192239>

Mind committing this as a separate patch?



src/tests/hook_tests.cpp (lines 751 - 753)
<https://reviews.apache.org/r/46175/#comment192249>

Why did you need the mock here?



src/tests/hook_tests.cpp (line 761)
<https://reviews.apache.org/r/46175/#comment192246>

How about no newline here to make the code block more clear and to be 
consistent with StartMaster above?



src/tests/hook_tests.cpp (lines 764 - 768)
<https://reviews.apache.org/r/46175/#comment192250>

Ditto here, do you need the mock?



src/tests/hook_tests.cpp (line 776)
<https://reviews.apache.org/r/46175/#comment192247>

How about no newline here to make the code block more clear and to be 
consistent with StartMaster above?



src/tests/hook_tests.cpp (line 797)
<https://reviews.apache.org/r/46175/#comment192244>

`offers->size`



src/tests/hook_tests.cpp (line 806)
<https://reviews.apache.org/r/46175/#comment192241>

How about the following to make the sandbox from 
TemporaryDirectoryTest::sandbox usage more explicit?

```
const string file = path::join(sandbox.get(), "post_fetch_hook");
```



src/tests/hook_tests.cpp (line 823)
<https://reviews.apache.org/r/46175/#comment192251>

Seems like you could remove the repeatedly case, we don't expect more 
updates, right?



src/tests/hook_tests.cpp (line 837)
<https://reviews.apache.org/r/46175/#comment192243>

Why `docker.get()->ps` instead of `docker->ps`?



src/tests/hook_tests.cpp (line 843)
<https://reviews.apache.org/r/46175/#comment192242>

Why `docker.get()->rm` instead of `docker->rm`?


- Ben Mahler


On April 13, 2016, 10:33 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46175/
> -----------
> 
> (Updated April 13, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5209
> https://issues.apache.org/jira/browse/MESOS-5209
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test for slavePostFetchHook.
> 
> 
> Diffs
> -
> 
>   src/examples/test_hook_module.cpp 3ff9fd71c275fd1a705ab3aca7a8041f29289bb0 
>   src/tests/hook_tests.cpp 97ff55ac7ec875b5ba5d4f17d80646c2d1dd4142 
> 
> Diff: https://reviews.apache.org/r/46175/diff/
> 
> 
> Testing
> ---
> 
> `sudo bin/mesos-tests.sh 
> --gtest_filter=*ROOT_DOCKER_VerifySlavePostFetchHook* `
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 46027: Documented `libprocess` helper function.

2016-04-13 Thread Ben Mahler


> On April 13, 2016, 9:33 p.m., Ben Mahler wrote:
> > 3rdparty/libprocess/src/process.cpp, line 559
> > <https://reviews.apache.org/r/46027/diff/2/?file=1340142#file1340142line559>
> >
> > s/.  A/. A/
> > 
> > Thanks for clarifying, sorry for the confusion!

Also, would you mind wrapping comments at 70 to stay consistent with most of 
our existing code?

```
// Returns true if `request` contains an inbound libprocess message.
// A libprocess message can either be sent by another instance of
// libprocess (i.e. both of the "User-Agent" and "Libprocess-From"
// headers will be set), or a client that speaks the libprocess
// protocol (i.e. only the "Libprocess-From" header will be set).
// This function returns true for either case.
```


- Ben


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


On April 12, 2016, 12:23 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> -------
> 
> (Updated April 12, 2016, 12:23 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46027/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46027: Documented `libprocess` helper function.

2016-04-13 Thread Ben Mahler

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


Ship it!





3rdparty/libprocess/src/process.cpp (line 559)
<https://reviews.apache.org/r/46027/#comment192211>

s/.  A/. A/

Thanks for clarifying, sorry for the confusion!


- Ben Mahler


On April 12, 2016, 12:23 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> ---
> 
> (Updated April 12, 2016, 12:23 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46027/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46029: Mark a few private functions `static` in libprocess tests.

2016-04-13 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 12, 2016, 12:13 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46029/
> ---
> 
> (Updated April 12, 2016, 12:13 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mark a few private functions `static` in libprocess tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> 8a21c6dc9141fe3372f12908a0007dd1c93c5485 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 274a76fa61a5cd4b1324be124f73979d4b980660 
> 
> Diff: https://reviews.apache.org/r/46029/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46121: Fixed an assumption of direct /tmp access in a slave test.

2016-04-12 Thread Ben Mahler


> On April 13, 2016, midnight, Gilbert Song wrote:
> > src/tests/slave_tests.cpp, line 790
> > <https://reviews.apache.org/r/46121/diff/1/?file=1342073#file1342073line790>
> >
> > use `os::getcwd()` to make consensus with other tests launching 
> > containers.

After thinking about this a bit more, I'm hesitant to use os::getcwd() since it 
makes an implicit assumption about the test being within a sandbox. Ideally the 
cwd comes from the test fixture or environment abstractions to ensure that the 
test is indeed using a temporary directory.


- Ben


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


On April 12, 2016, 11:15 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46121/
> ---
> 
> (Updated April 12, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This leads to test failures when multiple invocations are occuring on the 
> machine, or if root ownes /tmp/stdout.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 9e9dc35c6f03291648ec451581f762b1ab8b5a8d 
> 
> Diff: https://reviews.apache.org/r/46121/diff/
> 
> 
> Testing
> ---
> 
> make check (previously was failing due to /tmp/stdout being owned by root)
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>



Re: Review Request 46121: Fixed an assumption of direct /tmp access in a slave test.

2016-04-12 Thread Ben Mahler


> On April 12, 2016, 11:32 p.m., Jie Yu wrote:
> > Not yours, but this test should belong to mesos containerizer test (rather 
> > than slave test). Can you add a TODO?

Will do.


- Ben


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


On April 12, 2016, 11:15 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46121/
> ---
> 
> (Updated April 12, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This leads to test failures when multiple invocations are occuring on the 
> machine, or if root ownes /tmp/stdout.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp 9e9dc35c6f03291648ec451581f762b1ab8b5a8d 
> 
> Diff: https://reviews.apache.org/r/46121/diff/
> 
> 
> Testing
> ---
> 
> make check (previously was failing due to /tmp/stdout being owned by root)
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>



Review Request 46121: Fixed an assumption of direct /tmp access in a slave test.

2016-04-12 Thread Ben Mahler

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

This leads to test failures when multiple invocations are occuring on the 
machine, or if root ownes /tmp/stdout.


Diffs
-

  src/tests/slave_tests.cpp 9e9dc35c6f03291648ec451581f762b1ab8b5a8d 

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


Testing
---

make check (previously was failing due to /tmp/stdout being owned by root)


Thanks,

Ben Mahler



Re: Review Request 45932: Adds task information to container resource usage information.

2016-04-12 Thread Ben Mahler

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


Fix it, then Ship it!




Just some minor tweaks, I'll take care of these for you and commit shortly. 
Thanks!


include/mesos/v1/mesos.proto (lines 1024 - 1038)
<https://reviews.apache.org/r/45932/#comment191999>

This has drifted from the non-v1 proto. The task_state should be removed?



src/slave/slave.cpp (line 5175)
<https://reviews.apache.org/r/45932/#comment192004>

Since it's optional, we should probably guard this:

```
if (task->has_labels()) {
  t->mutable_labels()->CopyFrom(task->labels());
}
```



src/tests/oversubscription_tests.cpp (lines 230 - 233)
<https://reviews.apache.org/r/45932/#comment192010>

How about key1,value1 and key2,value2? I'm not sure if there's any value in 
having task / executor within the names.


- Ben Mahler


On April 12, 2016, 5:03 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45932/
> ---
> 
> (Updated April 12, 2016, 5:03 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5030
> https://issues.apache.org/jira/browse/MESOS-5030
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds task information to container resource usage information.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1f0527e86e333970f7f7879bb2bcbc33f0f44bc3 
>   include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861 
>   include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/oversubscription_tests.cpp 
> 23671746da2ac505d75bc2bd59114697d9161d52 
> 
> Diff: https://reviews.apache.org/r/45932/diff/
> 
> 
> Testing
> ---
> 
> Added new test to verify ResourceUsage sees task labels.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45572: Add labels to ExecutorInfo and deprecate source.

2016-04-12 Thread Ben Mahler

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


Fix it, then Ship it!




Thanks for the tests, I only commented on the master test since the same 
comments apply to the slave test.

Will get these cleaned up for you and will commit shortly.


CHANGELOG (line 43)
<https://reviews.apache.org/r/45572/#comment191913>

s/not/now/ ?



CHANGELOG (lines 56 - 58)
<https://reviews.apache.org/r/45572/#comment191923>

Since we mention the deprecation above, we probably don't need to mention 
it again here.



CHANGELOG (line 57)
<https://reviews.apache.org/r/45572/#comment191915>

ExecutorInfo typo



src/common/http.cpp (line 359)
<https://reviews.apache.org/r/45572/#comment191934>

Looks like you copied this from elsewhere, but it's odd that the other code 
used an std::move here, the rhs is already an rvalue.



src/tests/master_tests.cpp (lines 3723 - 3725)
<https://reviews.apache.org/r/45572/#comment191967>

Any reason this test was not placed directly above the TaskLabels test?



src/tests/master_tests.cpp (lines 3764 - 3766)
<https://reviews.apache.org/r/45572/#comment191950>

Thanks for the key / value naming!



src/tests/master_tests.cpp (lines 3774 - 3778)
<https://reviews.apache.org/r/45572/#comment191968>

Why is this needed?



src/tests/master_tests.cpp (line 3801)
<https://reviews.apache.org/r/45572/#comment191981>

We have the -> operator on Future/Option/Try/Result which tidies up the 
.get(). syntax used here:

```
  Try parse = JSON::parse(response->body);

```

We haven't done a sweep yet to update existing code, but we want to use it 
going forward.



src/tests/master_tests.cpp (line 3806)
<https://reviews.apache.org/r/45572/#comment191977>

This should be an assert rather than an expect, since the test cannot 
continue if it fails.



src/tests/master_tests.cpp (line 3808)
<https://reviews.apache.org/r/45572/#comment191979>

Since we have the -> operator on result, I would suggest we just do 
`s/find/labels_/` and avoid the labelsObject temporary altogether. labelsObject 
was a poor name since it's a JSON array.



src/tests/master_tests.cpp (lines 3812 - 3814)
<https://reviews.apache.org/r/45572/#comment191980>

Perhaps this was copy paste, usually we wrap at the paren when wrapping on 
the next line looks more jagged, so:

```
EXPECT_EQ(JSON::Value(JSON::protobuf(createLabel("key1", "value1"))),
  labels_->values[0]);
```



src/tests/oversubscription_tests.cpp (lines 230 - 231)
<https://reviews.apache.org/r/45572/#comment191949>

My suggestion for earlier was just to avoid using words like "foo" and 
"bar" in tests in favor of meaningful words like "key" and "value". Ditto below.


- Ben Mahler


On April 12, 2016, 4:59 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> ---
> 
> (Updated April 12, 2016, 4:59 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add labels to ExecutorInfo and deprecate source.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 1f0527e86e333970f7f7879bb2bcbc33f0f44bc3 
>   include/mesos/mesos.proto 63c181ae0a1e350fc27e36b1698e02db100b8861 
>   include/mesos/v1/mesos.proto a60a834e2538d54db7f257a0d4adfbb503ec1b0f 
>   src/common/http.cpp 735796cc9c5dfc8b27d9507e4a9a0659809b6f0d 
>   src/slave/http.cpp a684ff504535e2b9b2064f8048f5e03a0efcb059 
>   src/tests/master_tests.cpp a5b21d3d60f944fd52ceacb4bbbad2613f384db7 
>   src/tests/oversubscription_tests.cpp 
> 23671746da2ac505d75bc2bd59114697d9161d52 
>   src/tests/slave_tests.cpp 4a576b98d1cc58072626ac2c41c599bd3c8385c5 
> 
> Diff: https://reviews.apache.org/r/45572/diff/
> 
> 
> Testing
> ---
> 
> Added a test in oversubciption_tests to make sure executor labels are visible 
> to ResourceEstimator and QoSController.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45941: Add check in agent for incorrect oversubscribed resource.

2016-04-12 Thread Ben Mahler

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


Fix it, then Ship it!




Thanks! Will get this committed shortly.


src/slave/slave.cpp (line 4972)
<https://reviews.apache.org/r/45941/#comment191911>

newline here

missing an 'i' in 'Oversubscrbable'



src/slave/slave.cpp (line 4976)
<https://reviews.apache.org/r/45941/#comment191912>

s/.get()./->/


- Ben Mahler


On April 9, 2016, 5:20 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> ---
> 
> (Updated April 9, 2016, 5:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add check in agent for incorrect oversubscribed resource.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
> 
> Diff: https://reviews.apache.org/r/45941/diff/
> 
> 
> Testing
> ---
> 
> Running existing test, and verify manually that offending resource crashes 
> the agent.
> 
> (Any suggestion to test `CHECK` is welcomed).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.

2016-04-11 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 11, 2016, 2:36 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46024/
> ---
> 
> (Updated April 11, 2016, 2:36 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `SocketManager::swap_implementing_socket`, the previous coding
> accessed the `sockets` instance variable in two CHECKs without
> acquiring `mutex`. This is unsafe in general: `mutex` should be
> acquired before accessing instance variables like `sockets` (unless
> you know all such concurrent accesses will be read-only etc.).
> 
> As it happens, the previous coding was not unsafe because all the
> call-sites of `SocketManager::swap_implementing_socket` acquire
> `mutex` before invoking it. But because `swap_implementing_socket`
> also acquires `mutex`, clearly its API contract allows it to be
> invoked without holding `mutex`, in which case the CHECKs would
> likely be unsafe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46053: Added logic to validate for non-fractional GPU requests in the master.

2016-04-11 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 11, 2016, 10:11 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46053/
> ---
> 
> (Updated April 11, 2016, 10:11 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5178
> https://issues.apache.org/jira/browse/MESOS-5178
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We considered adding this logic directly to the
> 'Resources::validate()' function, but ultimately decided against it.
> The primary reason is that the existing 'Resources::validate()'
> function doesn't consider the semantics of any particular resource
> when performing its validation (it only makes sure that the fields in
> the 'Resource' protobuf message are correctly formed). Since a
> fractional 'gpus' resources is actually well-formed (and only
> semantically incorrect), we decided to push this validation logic up
> into the master.
> 
> Moreover, the existing logic to construct a 'Resources' object from a
> 'RepeatedPtrField' silently drops any resources that don't
> pass 'Resources::validate()'. This means that if we had pushed the
> non-fractional 'gpus' validation into 'Resources::validate()', the
> 'gpus' resources would just have been silently dropped rather than
> causing a TASK_ERROR in the master. This is obviously *not* the
> desired behaviour.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 504cd9b8bd5d40bb591b7aa5a23bd74cc210c2fc 
> 
> Diff: https://reviews.apache.org/r/46053/diff/
> 
> 
> Testing
> ---
> 
> Test coming in subsequent commit.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45969: Fixed indent in Nvidia GPU test.

2016-04-11 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 11, 2016, 10:19 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45969/
> ---
> 
> (Updated April 11, 2016, 10:19 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indent in Nvidia GPU test.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> dfffb2cf4d1d9c1f2de4ad90ce3d8acc3e98631c 
> 
> Diff: https://reviews.apache.org/r/45969/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45970: Added Nvidia GPU test to verify error when requesting fractional GPUs.

2016-04-11 Thread Ben Mahler

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


Ship it!





src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 217 - 223)
<https://reviews.apache.org/r/45970/#comment191734>

How about no newlines between these since they are related?


- Ben Mahler


On April 11, 2016, 10:18 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45970/
> ---
> 
> (Updated April 11, 2016, 10:18 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5159
> https://issues.apache.org/jira/browse/MESOS-5159
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added Nvidia GPU test to verify error when requesting fractional GPUs.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> dfffb2cf4d1d9c1f2de4ad90ce3d8acc3e98631c 
> 
> Diff: https://reviews.apache.org/r/45970/diff/
> 
> 
> Testing
> ---
> 
> ```
> make -j check
> SUCCESS
> ```
> ```
> [klueska@core-dev build]$ sudo 
> GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_FractionalResources" 
> bin/mesos-tests.sh
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from NvidiaGpuTest
> [ RUN  ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_FractionalResources
> [   OK ] NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_FractionalResources (10020 
> ms)
> [--] 1 test from NvidiaGpuTest (10026 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (10059 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 46029: Mark a few private functions `static` in libprocess tests.

2016-04-11 Thread Ben Mahler

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



Mind doing a sweep for this?


3rdparty/libprocess/src/tests/process_tests.cpp 
<https://reviews.apache.org/r/46029/#comment191701>

Is this a complete header audit? Why only this file? I'd suggest we remove 
this part of the diff.


- Ben Mahler


On April 11, 2016, 9:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46029/
> ---
> 
> (Updated April 11, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Also, remove two unused includes.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> e8ee8ee01ee9524b308452c403326e10f75b3ce5 
> 
> Diff: https://reviews.apache.org/r/46029/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46028: Improved comments in SocketManager::next().

2016-04-11 Thread Ben Mahler

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




3rdparty/libprocess/src/process.cpp (lines 1813 - 1816)
<https://reviews.apache.org/r/46028/#comment191686>

Can you move this comment up to the declaration in the SocketManager 
interface?



3rdparty/libprocess/src/process.cpp (lines 1947 - 1954)
<https://reviews.apache.org/r/46028/#comment191700>

The implication of this comment (regardless of your changes) seems to be 
that the socket is an outbound socket? Is that true?


- Ben Mahler


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46028/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved comments in SocketManager::next().
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46028/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46024: Avoided misleading locking in the libprocess SocketManager.

2016-04-11 Thread Ben Mahler

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



Mind adding joris and/or benh as reviewers? I'm happy to review but since they 
wrote the code I'd like them to be aware of this change.

- Ben Mahler


On April 11, 2016, 2:36 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46024/
> ---
> 
> (Updated April 11, 2016, 2:36 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In `SocketManager::swap_implementing_socket`, the previous coding
> accessed the `sockets` instance variable in two CHECKs without
> acquiring `mutex`. This is unsafe in general: `mutex` should be
> acquired before accessing instance variables like `sockets` (unless
> you know all such concurrent accesses will be read-only etc.).
> 
> As it happens, the previous coding was not unsafe because all the
> call-sites of `SocketManager::swap_implementing_socket` acquire
> `mutex` before invoking it. But because `swap_implementing_socket`
> also acquires `mutex`, clearly its API contract allows it to be
> invoked without holding `mutex`, in which case the CHECKs would
> likely be unsafe.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46024/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46027: Documented `libprocess` helper function.

2016-04-11 Thread Ben Mahler

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




3rdparty/libprocess/src/process.cpp (lines 559 - 562)
<https://reviews.apache.org/r/46027/#comment191685>

Hm.. this may be a bit confusing for those without context. A libprocess 
message can be sent by either libprocess itself, or by clients that speak 
libprocess. Here the distinction is that this will tell you if this is a 
libprocess message *and* it is sent by a libprocess instance.


- Ben Mahler


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46027/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is a bit confusing, because different libprocess HTTP
> clients set different headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46027/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46026: Documented `Socket::shutdown()` member function in libprocess.

2016-04-11 Thread Ben Mahler

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




3rdparty/libprocess/include/process/socket.hpp (lines 176 - 187)
<https://reviews.apache.org/r/46026/#comment191681>

Why don't we instead take the "`int how`" as an argument and make the 
SHUT_RD explicit in the callers? This interface had come up in the past, would 
like to see from benh/joris if there's any that the SHUT_RD was made implicit 
and no control was given to the caller.


- Ben Mahler


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46026/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is slightly confusing, because it doesn't match the
> semantics of the shutdown(2) POSIX function.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/socket.hpp 
> 4cb49680d1304899a4ee675ac07379e51d9c55b1 
> 
> Diff: https://reviews.apache.org/r/46026/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46025: Clarified comments on socket data structures in SocketManager.

2016-04-11 Thread Ben Mahler

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



Looks good, just some food for thought and a question related to the 
relationship between `dispose` and `temps`.


3rdparty/libprocess/src/process.cpp (lines 353 - 376)
<https://reviews.apache.org/r/46025/#comment191680>

One question I have looking at this is whether we would benefit from more 
explicit structure here:

```
struct
{
  map<int, Socket*> sockets;
  map<int, Address> addresses;
  set dispose;
  map<Address, int> temps;
  map<Address, int> persists;
  map<int, queue<Encoder*>> outgoing;
} outbound;

struct
{
  map<int, Socket*> sockets;
} inbound;
```

This assumes we don't need to track both inbound and outbound within the 
same map. From a quick glance at the code I can't see any reason to store them 
in the same map but perhaps I'm missing something.

There seems to be potential for significant consolidation of the outbond 
structures as well.



3rdparty/libprocess/src/process.cpp (lines 356 - 358)
<https://reviews.apache.org/r/46025/#comment191679>

Why didn't you update this comment? Is the set of values within `temps` 
equivalent to `dispose`? Would be great to explain the distinction here.


- Ben Mahler


On April 11, 2016, 2:35 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46025/
> ---
> 
> (Updated April 11, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Clarified comments on socket data structures in SocketManager.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 5e9dcfdc52f3a8223bc43af149b8e1f5dbdf5b0a 
> 
> Diff: https://reviews.apache.org/r/46025/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 46030: Used initializer_list for collections in libprocess tests.

2016-04-11 Thread Ben Mahler

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


Fix it, then Ship it!




Hm.. ideally there should not have been a TODO in the firewall tests, since it 
tends to just serve as a distraction.


3rdparty/libprocess/src/tests/future_tests.cpp (lines 469 - 474)
<https://reviews.apache.org/r/46030/#comment191671>

I tend to find an added `=` a little more clear, since it then reads as 
assignment to an initializer list.


- Ben Mahler


On April 11, 2016, 9:01 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46030/
> ---
> 
> (Updated April 11, 2016, 9:01 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used initializer_list for collections in libprocess tests.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/future_tests.cpp 
> c2eeb0e52dab08f390ec524ed727dde1a72916b3 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> e8ee8ee01ee9524b308452c403326e10f75b3ce5 
> 
> Diff: https://reviews.apache.org/r/46030/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 45489: Replaced reinterpret_cast with static_cast in libprocess.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On March 30, 2016, 4:16 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45489/
> ---
> 
> (Updated March 30, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Casting from a virtual base class to a child class should not be
> done with reinterpret_cast. We could use dynamic_cast as well, but
> that has a performance cost; in this case, we know exactly which
> child class the pointer points to, so the performance cost and
> additional safety offered by dynamic_cast is not necessary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> feaffa4334422ec3964f8d079f570061eaf390d2 
> 
> Diff: https://reviews.apache.org/r/45489/diff/
> 
> 
> Testing
> ---
> 
> make check with GCC 5.3 and recent apple-clang.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 45590: Made `Delegate` and `Handlers` libprocess tests less fragile.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 1, 2016, 4:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45590/
> ---
> 
> (Updated April 1, 2016, 4:22 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As previously written, these tests depended on the fact that `post`
> will synchronously deliver a message to a local PID.  That is an
> implementation detail that seems unwise to rely upon. Instead, it is
> quite easy to arrange for both tests to block until the effect of
> the `post` has occurred.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd 
> 
> Diff: https://reviews.apache.org/r/45590/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Without this patch, the libprocess tests fail if the "local message" 
> short-circuit in `transport` is disabled (circa line 556). With the patch, 
> the tests succeed.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 45932: Add stripped TaskInfo's to ResourceUsage.Executor message.

2016-04-08 Thread Ben Mahler

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



Thanks! Looking pretty good, some minor comments below.


CHANGELOG (line 52)
<https://reviews.apache.org/r/45932/#comment191319>

How about:

```
Add task information to container resource usage information.
```



include/mesos/mesos.proto (lines 1026 - 1027)
<https://reviews.apache.org/r/45932/#comment191312>

We should avoid saying "used" here in favor of "allocated". We can probably 
do without this comment.

How about s/resources/allocated here to match the executor above?



include/mesos/mesos.proto (line 1029)
<https://reviews.apache.org/r/45932/#comment191313>

This comment doesn't seem to add anything?



include/mesos/mesos.proto (line 1032)
<https://reviews.apache.org/r/45932/#comment191315>

Can we omit this? Why did you include it?



include/mesos/mesos.proto (lines 1035 - 1036)
<https://reviews.apache.org/r/45932/#comment191314>

Avoid saying "running" here since there are more non-terminal states than 
just RUNNING:

```
// Non-terminal tasks.
```



src/slave/slave.cpp (line 5169)
<https://reviews.apache.org/r/45932/#comment191316>

let's say "non-terminal" instead of running, ideally slave.hpp doesn't say 
"running" either, but let's leave it for now



src/slave/slave.cpp (lines 5171 - 5176)
<https://reviews.apache.org/r/45932/#comment191317>

How about s/taskEntry/t/ ?



src/tests/oversubscription_tests.cpp (lines 230 - 231)
<https://reviews.apache.org/r/45932/#comment191318>

How about s/task_label/key/ s/task_label_value/value/ ? Will it fit on one 
line then?

We generally avoid "foo" and "bar" in favor of things like "key" and 
"value" to make the test clearer, so please ignore the executor labels here.


- Ben Mahler


On April 8, 2016, 5:19 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45932/
> ---
> 
> (Updated April 8, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5030
> https://issues.apache.org/jira/browse/MESOS-5030
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add stripped TaskInfo's to ResourceUsage.Executor message.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 4553465cc3dc17956f168469d405f7a453d6359e 
>   include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
>   src/tests/oversubscription_tests.cpp 
> 23671746da2ac505d75bc2bd59114697d9161d52 
> 
> Diff: https://reviews.apache.org/r/45932/diff/
> 
> 
> Testing
> ---
> 
> Added new test to verify ResourceUsage sees task labels.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45572: Add labels to ExecutorInfo and deprecate source.

2016-04-08 Thread Ben Mahler

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



Looks pretty good, thanks!

Could you also add tests that mirror the task label tests?

```
$ grep -R TEST src/tests | grep TaskLabels
src/tests/master_tests.cpp:TEST_F(MasterTest, TaskLabels)
src/tests/slave_tests.cpp:TEST_F(SlaveTest, TaskLabels)
```


CHANGELOG (lines 49 - 50)
<https://reviews.apache.org/r/45572/#comment191292>

The ExecutorInfo.source deprecation should be mentioned in the 
'Deprecations' section. I would ok duplicating this in both sections, and only 
calling out the labels in the 'Additional API Changes' section.



include/mesos/mesos.proto (line 473)
<https://reviews.apache.org/r/45572/#comment191297>

No need to bother renaming slave to agent in the pre-V1 API.



include/mesos/mesos.proto (line 478)
<https://reviews.apache.org/r/45572/#comment191304>

All comments should end with a period, I guess you may have copied this 
from code that didn't follow that style.



include/mesos/mesos.proto (lines 496 - 498)
<https://reviews.apache.org/r/45572/#comment191311>

Any reason not to re-use the more descriptive comment on TaskInfo.labels? 
Specifically it would be nice to include the warning about the master keeping 
this data in memory.


- Ben Mahler


On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> ---
> 
> (Updated April 1, 2016, 1:42 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add labels to ExecutorInfo and deprecate source.
> 
> 
> Diffs
> -
> 
>   CHANGELOG b90078d41357c29c9102df00a735bde460e797bb 
>   include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
>   src/tests/oversubscription_tests.cpp 
> ba036810758d99a6fb0034c5e2bc7829e2343a44 
> 
> Diff: https://reviews.apache.org/r/45572/diff/
> 
> 
> Testing
> ---
> 
> Added a test in oversubciption_tests to make sure executor labels are visible 
> to ResourceEstimator and QoSController.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45488: Removed an unnecessary `memset` from libprocess.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On March 30, 2016, 2:49 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45488/
> ---
> 
> (Updated March 30, 2016, 2:49 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Zero'ing the input buffer before receiving data is unnecessary.
> Moreover, keeping the `memset` around is confusing, because it makes
> the API contract of Socket.recv() less clear.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> feaffa4334422ec3964f8d079f570061eaf390d2 
> 
> Diff: https://reviews.apache.org/r/45488/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Neil Conway
> 
>



Re: Review Request 45941: Add check in agent for incorrect oversubscribed resource.

2016-04-08 Thread Ben Mahler

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




src/slave/slave.cpp (lines 4989 - 4991)
<https://reviews.apache.org/r/45941/#comment191288>

Any reason why this CHECK isn't below the VLOG(1) above? Having it above 
seems to make it more clear that we're validating the input from the estimator, 
no?

Also, for the comment here, we should avoid talking about the hierarchical 
allocator since that just happens to be where the failure manifests.

Perhaps something like:

```
// Oversubscrbable resources must be considered revocable.
//
// TODO(bmahler): Consider tagging input as revocable
// rather than rejecting and crashing here.
```


- Ben Mahler


On April 8, 2016, 8:02 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45941/
> ---
> 
> (Updated April 8, 2016, 8:02 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joseph Wu.
> 
> 
> Bugs: MESOS-5131
> https://issues.apache.org/jira/browse/MESOS-5131
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add check in agent for incorrect oversubscribed resource.
> 
> I've decided to let agent crash explicitly here instead of fail more silently.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp f090c853b8affc4be5eecb4f616ec881fc2b60c3 
> 
> Diff: https://reviews.apache.org/r/45941/diff/
> 
> 
> Testing
> ---
> 
> Running existing test, and verify manually that offending resource crashes 
> the agent.
> 
> (Any suggestion to test `CHECK` is welcomed).
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45572: Add labels to ExecutorInfo and deprecate source.

2016-04-08 Thread Ben Mahler


> On April 4, 2016, 5:26 p.m., haosdent huang wrote:
> > CHANGELOG, line 49
> > <https://reviews.apache.org/r/45572/diff/1/?file=1321727#file1321727line49>
> >
> > Usually we don't need change file. It would be changed by release 
> > manager.
> 
> Zhitao Li wrote:
> hmm, right now the top section of CHANGELOG is written by each feature 
> author in git blame, and this was also my impression that we should 
> collectively help on changelog documentation so work load of release manager 
> can be reduced.
> 
> Please let me know if my understanding of the process is incorrect and 
> I'll happily drop this change.

We do want contributors adding to the CHANGELOG deprecations / api changes / 
etc. It is difficult for release managers to do a good job at it given how many 
commits are in a release.


- Ben


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


On April 1, 2016, 1:42 a.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45572/
> ---
> 
> (Updated April 1, 2016, 1:42 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5029
> https://issues.apache.org/jira/browse/MESOS-5029
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add labels to ExecutorInfo and deprecate source.
> 
> 
> Diffs
> -
> 
>   CHANGELOG b90078d41357c29c9102df00a735bde460e797bb 
>   include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
>   src/tests/oversubscription_tests.cpp 
> ba036810758d99a6fb0034c5e2bc7829e2343a44 
> 
> Diff: https://reviews.apache.org/r/45572/diff/
> 
> 
> Testing
> ---
> 
> Added a test in oversubciption_tests to make sure executor labels are visible 
> to ResourceEstimator and QoSController.
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>



Re: Review Request 45942: Updated the webui to include GPU metrics.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 8, 2016, 8:36 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45942/
> ---
> 
> (Updated April 8, 2016, 8:36 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5157
> https://issues.apache.org/jira/browse/MESOS-5157
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the webui to include GPU metrics.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/framework.html 
> ee42d1ab841a4c42d95512ee60d577b1bbb66bc8 
>   src/webui/master/static/frameworks.html 
> 15ff1e9cb6c70df8df47a1b939681abde591e010 
>   src/webui/master/static/home.html a691084f4992cda65734f5fee3b2f38349737b83 
>   src/webui/master/static/js/controllers.js 
> f92affab41f8418cd7e5ea25561a182a1761fd79 
>   src/webui/master/static/offers.html 
> 01213e9582f50072a9c729782271269f72972d28 
>   src/webui/master/static/slave.html 4419f7c166e8768040dab7dbc6fb64e1382ad272 
>   src/webui/master/static/slave_executor.html 
> 5acb676390fe4ed17369143c5a13202c0981 
>   src/webui/master/static/slave_framework.html 
> 4b2b1562f38f002b4659b4a883249f0469307323 
>   src/webui/master/static/slaves.html 
> 0cb125a7d95ccc7770916cbffa052f43e8ea3d2c 
> 
> Diff: https://reviews.apache.org/r/45942/diff/
> 
> 
> Testing
> ---
> 
> Manually opened the web UI, clicked around to make sure all the GPU metrics 
> now showed up.
> Also looked at "Inspect Element" to verify that there were no javascript 
> errors when loading.
> 
> Specifically:
> ```
> * The Resources section in the left sidebar of the main page
> * The Resources section of the table in the Frameworks tab
> * The Resources section of the table in the Slaves tab
> * The Resources section of the table in the Offers tab
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45855: Updated docs to include references to GPUs as a first class resource.

2016-04-08 Thread Ben Mahler


- Ben


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


On April 7, 2016, 1:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45855/
> ---
> 
> (Updated April 7, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5135
> https://issues.apache.org/jira/browse/MESOS-5135
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docs to include references to GPUs as a first class resource.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 26468d962440560a6b4b35f51ca248ab059ec31f 
>   docs/monitoring.md 76be32842e8b437fda40c8565a34ec4f8d8dfbcc 
> 
> Diff: https://reviews.apache.org/r/45855/diff/
> 
> 
> Testing
> ---
> 
> Looked at regenerated website and it seems to include the new documentation 
> properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45855: Updated docs to include references to GPUs as a first class resource.

2016-04-08 Thread Ben Mahler


> On April 7, 2016, 2:13 a.m., haosdent huang wrote:
> > docs/attributes-resources.md, line 99
> > <https://reviews.apache.org/r/45855/diff/1/?file=1329266#file1329266line99>
> >
> > How about sort it by alphabetically?
> 
> Kevin Klues wrote:
> The only reason I didn't sort alphabetically was because these resources 
> don't seem to be ordered alphabetically anywhere else (`disk` always seems to 
> come *after* `mem`).  Since `gpus` are pretty similar to `cpus` in their 
> semantics, I decided to place them next to them in the order here.  We can 
> wait to see what Ben thinks.

After "cpus" sounds good to me since it's already not alphabetical.


- Ben


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


On April 7, 2016, 1:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45855/
> -------
> 
> (Updated April 7, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5135
> https://issues.apache.org/jira/browse/MESOS-5135
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docs to include references to GPUs as a first class resource.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 26468d962440560a6b4b35f51ca248ab059ec31f 
>   docs/monitoring.md 76be32842e8b437fda40c8565a34ec4f8d8dfbcc 
> 
> Diff: https://reviews.apache.org/r/45855/diff/
> 
> 
> Testing
> ---
> 
> Looked at regenerated website and it seems to include the new documentation 
> properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45855: Updated docs to include references to GPUs as a first class resource.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 7, 2016, 1:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45855/
> ---
> 
> (Updated April 7, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5135
> https://issues.apache.org/jira/browse/MESOS-5135
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docs to include references to GPUs as a first class resource.
> 
> 
> Diffs
> -
> 
>   docs/attributes-resources.md 26468d962440560a6b4b35f51ca248ab059ec31f 
>   docs/monitoring.md 76be32842e8b437fda40c8565a34ec4f8d8dfbcc 
> 
> Diff: https://reviews.apache.org/r/45855/diff/
> 
> 
> Testing
> ---
> 
> Looked at regenerated website and it seems to include the new documentation 
> properly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45854: Updated the default JSON representation of a Resource to include GPUs.

2016-04-08 Thread Ben Mahler

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


Ship it!





src/tests/common/http_tests.cpp (lines 162 - 163)
<https://reviews.apache.org/r/45854/#comment191248>

I'm a little hesitant about using revocable gpus here given that it's not 
clear how it would work, but I suppose there is no harm in it for this test.


- Ben Mahler


On April 8, 2016, 5:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45854/
> ---
> 
> (Updated April 8, 2016, 5:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5136
> https://issues.apache.org/jira/browse/MESOS-5136
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This includes updating some tests that parse this JSON and look for
> specific values for all of its fields.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 3748c71686c696c47f93a81e3434bbb55448e334 
>   src/tests/common/http_tests.cpp ac6322d7fb58adf7a615c517496d08dee3bf0081 
>   src/tests/dynamic_weights_tests.cpp 
> e36a45fa18d163430df1c9dc72d2372aee9b44e6 
>   src/tests/role_tests.cpp 056359bbe6a02bb129bfa2ce371e82cd3aa4c984 
> 
> Diff: https://reviews.apache.org/r/45854/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45852: Added standard metrics for GPU resources.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 8, 2016, 8:34 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45852/
> ---
> 
> (Updated April 8, 2016, 8:34 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4624
> https://issues.apache.org/jira/browse/MESOS-4624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added standard metrics for GPU resources.
> 
> 
> Diffs
> -
> 
>   src/master/metrics.cpp 30c091198a8fdd6d6a957a351dc37d3dae7788e4 
>   src/slave/metrics.cpp 42c66d7d7176232ccc71f1e040bcae99900f49f8 
>   src/tests/master_tests.cpp 8f93fbaf2bfd66bbc144a85c0097f45c55ff3491 
>   src/tests/slave_tests.cpp 03bb6da687a1bf11d81619839e6730835e5c4d82 
> 
> Diff: https://reviews.apache.org/r/45852/diff/
> 
> 
> Testing
> ---
> 
> Ran:
> ```
> GTEST_FILTER="SlaveTest.MetricsInMetricsEndpoint:SlaveTest.MetricsInMetricsEndpoint"
>  make -j check
> SUCCESS
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45856: Fixed Nvidia GPU test build for namespace change of MasterDetector.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 7, 2016, 1:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45856/
> ---
> 
> (Updated April 7, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5138
> https://issues.apache.org/jira/browse/MESOS-5138
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed Nvidia GPU test build for namespace change of MasterDetector.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 5565227bc0b18f1891265e6eff0c5a22a0c4ab86 
> 
> Diff: https://reviews.apache.org/r/45856/diff/
> 
> 
> Testing
> ---
> 
> make check -j
> 
> ```
> sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
> bin/mesos-tests.sh
> SUCCESS
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45853: Removed 'dashboard.js' from the webui.

2016-04-08 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 7, 2016, 1:54 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45853/
> ---
> 
> (Updated April 7, 2016, 1:54 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-5137
> https://issues.apache.org/jira/browse/MESOS-5137
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This file is no longer referenced anywhere in the webui and none of
> the functions it defines are called anywhere.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/js/dashboard.js 
> d6cbb3cc5aa92249c4b17c6d77260b215203d008 
> 
> Diff: https://reviews.apache.org/r/45853/diff/
> 
> 
> Testing
> ---
> 
> The webui still appears to work in all respects without this file.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44832: Validate string when convert `Flags` to `hashmap<string, string>`.

2016-04-07 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 2, 2016, 8:07 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44832/
> ---
> 
> (Updated April 2, 2016, 8:07 a.m.)
> 
> 
> Review request for mesos, Adam B, Ben Mahler, and Kevin Klues.
> 
> 
> Bugs: MESOS-2023
> https://issues.apache.org/jira/browse/MESOS-2023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Validate string when convert `Flags` to `hashmap<string, string>`.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 9036dae1b55e0c222ddedd15599ea56933c84cd7 
> 
> Diff: https://reviews.apache.org/r/44832/diff/
> 
> 
> Testing
> ---
> 
> 1. Without pass `--env=`.
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo $a'
> ```
> 
> Output:
> ```
> Starting task test
> Forked command at 4542
> sh -c 'echo $a'
> I0315 11:45:39.153188  4402 slave.cpp:3002] Handling status update 
> TASK_RUNNING (UUID: 06b8f974-b6ec-4d10-b1d8-f84b5d4f728d) for task test of 
> framework cf1af3e0-dd66-41e8-8c56-7ad80d7dce98-0001 from execu
> tor(1)@127.0.0.1:54698
> I0315 11:45:39.153964  4401 status_update_manager.cpp:320] Received status 
> update TASK_RUNNING (UUID: 06b8f974-b6ec-4d10-b1d8-f84b5d4f728d) for task 
> test of framework cf1af3e0-dd66-41e8-8c56-7ad80d7dce98-
> 0001
> ```
> 
> 2. Pass `--env=`.
> ```
> $ ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo 
> $a' --env="{\"a\": \"stdin\"}"
> ```
> 
> Output:
> ```
> Registered executor on localhost
> Starting task test
> Forked command at 4675
> sh -c 'echo $a'
> stdin
> I0315 11:46:34.797502  4408 slave.cpp:3002] Handling status update 
> TASK_RUNNING (UUID: 16040c40-f5e4-4bf0-8690-447f2901310b) for task test of 
> framework cf1af3e0-dd66-41e8-8c56-7ad80d7dce98-0003 from execu
> tor(1)@127.0.0.1:57831
> ```
> 
> 3. Pass incorrect json format in `--env=`.
> ```
> ./src/mesos-execute  --master=127.0.0.1:5050 --name=test --command='echo $a' 
> --env="{\"a\": {}}"
> Failed to load flag 'env': Failed to load value '{"a": {}}': The value of key 
> 'a' in '{"a":{}}' is not a valid string.
> ```
> 
> 
> Thanks,
> 
> haosdent huang
> 
>



Re: Review Request 45534: Added per-role and quota share metrics to the DRFSorter.

2016-04-07 Thread Ben Mahler

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



High level thought is that we should start with just the shares within the role 
sorter, since these are the easiest to understand. After Vinod and I talked, we 
realized the "quota" shares were difficult to reason about, so let's not 
include those for now (one thing at a time).

I also would suggest the following naming scheme:

```
allocator/mesos/roles//shares/dominant
```

Since we may want additional shares to be exposed:

```
allocator/mesos/roles//shares/dominant: 0.5
allocator/mesos/roles//shares/cpus: 0.5
allocator/mesos/roles//shares/mem: 0.2
allocator/mesos/roles//shares/disk: 0.1
```


docs/monitoring.md (lines 967 - 973)
<https://reviews.apache.org/r/45534/#comment191107>

Let's not include this one for now, I chatted with vinod about this one to 
get some more thoughts and we agree this is pretty difficult to explain to the 
users and likely the most valuable thing to expose for now is the dominant 
shares for the normal sorter.



src/master/allocator/sorter/drf/metrics.hpp (line 59)
<https://reviews.apache.org/r/45534/#comment191109>

```
// Dominant share of each client.
hashmap<std::string, process::metrics::Gauge> dominantShares;
```



src/master/allocator/sorter/sorter.hpp (lines 46 - 52)
<https://reviews.apache.org/r/45534/#comment191108>

Why a factory rather than just a string prefix? How about:

```
  // Provides the allocator's execution context (via a UPID)
  // and a name prefix in order to support metrics within the
  // sorter implementation.
  explicit Sorter(
  const process::UPID& allocator,
  const std::string& metricsPrefix) {}
```



src/tests/hierarchical_allocator_tests.cpp (lines 2748 - 2752)
<https://reviews.apache.org/r/45534/#comment191105>

How about:

```
// Verifies that per-role dominant share metrics are correctly reported.
```



src/tests/hierarchical_allocator_tests.cpp (lines 2762 - 2764)
<https://reviews.apache.org/r/45534/#comment191104>

Would you mind wrapping to reduce jaggedness in general?

```
  // Register one agent and one framework. The framework will
  // immediately receive receive an offer and make it have the
  // maximum possible dominant share.
```



src/tests/hierarchical_allocator_tests.cpp (lines 2783 - 2787)
<https://reviews.apache.org/r/45534/#comment191110>

We need to settle after these allocator calls, otherwise we may not see the 
changes reflected in the metrics. Did you run this test in repetition?



src/tests/hierarchical_allocator_tests.cpp (lines 2791 - 2792)
<https://reviews.apache.org/r/45534/#comment191106>

How about moving this up to before the decline happens, to be consistent 
with your other comments that describe what's happening in the test, so:

```
  // Decline the offered resources and expect a zero share.
  Future allocation = allocations.get();
  AWAIT_READY(allocation);
  allocator->recoverResources(
  allocation->frameworkId,
  agent1.id(),
  allocation->resources.get(agent1.id()).get(),
  None());
```



src/tests/hierarchical_allocator_tests.cpp (line 2835)
<https://reviews.apache.org/r/45534/#comment191103>

Can you move this down to be adjacent with the expectation on the metrics? 
It's also how we did it in the metrics test above:

```
  expected.values = {
  {"allocator/mesos/dominant_share/roles/roleA", 0.5},
  };
    
  metrics = Metrics();
  EXPECT_TRUE(metrics.contains(expected));
```


- Ben Mahler


On April 4, 2016, 12:32 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45534/
> -------
> 
> (Updated April 4, 2016, 12:32 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4760
> https://issues.apache.org/jira/browse/MESOS-4760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added per-role and quota share metrics to the DRFSorter.
> 
> 
> Diffs
> -
> 
>   docs/monitoring.md 76be32842e8b437fda40c8565a34ec4f8d8dfbcc 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/master/allocator/mesos/hierarchical.hpp 
> e979fdf60da1409d1c2d08f0e9f03cef067506dd 
>   src/master/allocator/sorter/drf/metrics.hpp PRE-CREATION 
>   src/master/allocator/sorter/drf/metrics.cpp PRE-CREATION 
>   src/mast

Re: Review Request 44853: Added benchmark test for the allocator metrics endpoint.

2016-04-07 Thread Ben Mahler

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


Fix it, then Ship it!





src/tests/hierarchical_allocator_tests.cpp (line 2988)
<https://reviews.apache.org/r/44853/#comment191088>

2 spaces here



src/tests/hierarchical_allocator_tests.cpp (lines 3000 - 3017)
<https://reviews.apache.org/r/44853/#comment191090>

size_t above but unsigned here?

Also, why the vectors here?

```
  for (size_t i = 0; i < frameworkCount; i++) {
string role = stringify(i);
allocator->setQuota(role, createQuota(role, "cpus:1;mem:512;disk:256"));
  }

  for (size_t i = 0; i < frameworkCount; i++) {
FrameworkInfo framework = createFrameworkInfo(stringify(i)));
allocator->addFramework(framework.id(), framework, {});
  }

  for (size_t i = 0; i < slaveCount; i++) {
SlaveInfo slave = createSlaveInfo("cpus:16;mem:2048;disk:1024");
allocator->addSlave(slaveid(), slave, None(), slave.resources(), {});
  }
```



src/tests/hierarchical_allocator_tests.cpp (lines 3022 - 3032)
<https://reviews.apache.org/r/44853/#comment191092>

Why the scope here? Looks like it was added because some of the new 
benchmarks in this file are using a scope for the stopwatch? I don't think it's 
bad, but it's a little tricky to follow the reasoning for where to use a scope 
here. I suppose it calls out the timing code, but I'd opt to just avoid the 
extra scoping for now.



src/tests/hierarchical_allocator_tests.cpp (lines 3023 - 3027)
<https://reviews.apache.org/r/44853/#comment191093>

Perhaps this would be clearer?

```
  Stopwatch watch;

  // TODO(bmahler): Avoid timing the JSON parsing here.
  // Ideally we also avoid timing the HTTP layer.
  watch.start();
  JSON::Object metrics = Metrics();
  watch.stop();
```



src/tests/hierarchical_allocator_tests.cpp (lines 3029 - 3031)
<https://reviews.apache.org/r/44853/#comment191091>

How about:

```
  cout << "/metrics/snapshot took " << watch.elapsed()
   << " for " << slaveCount << " slaves"
   << " and " << frameworkCount << " frameworks" << endl;
```


- Ben Mahler


On March 30, 2016, 1:22 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44853/
> ---
> 
> (Updated March 30, 2016, 1:22 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added benchmark test for the allocator metrics endpoint.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> 459e02576f6d05abbbcc83ae5cabac5c66e93f05 
> 
> Diff: https://reviews.apache.org/r/44853/diff/
> 
> 
> Testing
> ---
> 
> The benchmark uses the same parametrized setup as other 
> `HierarchicalAllocator_BENCHMARK_Tests` which already elsewhere take 
> considerable time. The reason for covering the same parameter space here was 
> the assumption that that parameter space does capture the relevant scenarios.
> 
> The benchmark shows that the time needed to obtain the metrics has a linear 
> relationship with the number of registered frameworks, roughly independent of 
> the number of slaves. With my setup, the time per framework was well below 1 
> ms after already a few frameworks.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 45533: Explicitly typed quota role sorter in Mesos allocator.

2016-04-07 Thread Ben Mahler

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.hpp (line 479)
<https://reviews.apache.org/r/45533/#comment191095>

whoops?


- Ben Mahler


On April 1, 2016, 8:07 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45533/
> ---
> 
> (Updated April 1, 2016, 8:07 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Bugs: MESOS-4760
> https://issues.apache.org/jira/browse/MESOS-4760
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Explicitly typed quota role sorter in Mesos allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> e979fdf60da1409d1c2d08f0e9f03cef067506dd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
> 
> Diff: https://reviews.apache.org/r/45533/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk, not optimized)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>



Re: Review Request 45715: Added support to grant access to /dev/nvidiactl in Nvidia GPU isolator.

2016-04-05 Thread Ben Mahler


> On April 5, 2016, 11:53 p.m., Ben Mahler wrote:
> >

Do we also need the uvm device?


- Ben


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


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45715/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5115
> https://issues.apache.org/jira/browse/MESOS-5115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, calls to 'nvidia-smi' would fail inside a container even
> if access to a GPU had been granted. Moreover, access to
> /dev/nvidiactl is actually required for a container to do anything
> useful with a GPU even if it has access to it.
> 
> This patch explicitly grants/revokes access to /dev/nvidiactl as GPUs
> are added and removed from a container in the Nvidia GPU isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> b0f58035c7c819b42e5f249fadd97312f9e3ac7b 
> 
> Diff: https://reviews.apache.org/r/45715/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45715: Added support to grant access to /dev/nvidiactl in Nvidia GPU isolator.

2016-04-05 Thread Ben Mahler

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp (lines 
65 - 78)
<https://reviews.apache.org/r/45715/#comment190468>

Could we avoid the static non-POD?


- Ben Mahler


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45715/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5115
> https://issues.apache.org/jira/browse/MESOS-5115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, calls to 'nvidia-smi' would fail inside a container even
> if access to a GPU had been granted. Moreover, access to
> /dev/nvidiactl is actually required for a container to do anything
> useful with a GPU even if it has access to it.
> 
> This patch explicitly grants/revokes access to /dev/nvidiactl as GPUs
> are added and removed from a container in the Nvidia GPU isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> b0f58035c7c819b42e5f249fadd97312f9e3ac7b 
> 
> Diff: https://reviews.apache.org/r/45715/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44364: Added tests for the Nvidia GPU isolator.

2016-04-05 Thread Ben Mahler

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


Fix it, then Ship it!




Nice test!


src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 62)
<https://reviews.apache.org/r/44364/#comment190434>

Could we have a summary comment?



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 70)
<https://reviews.apache.org/r/44364/#comment190441>

s/std:://



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 73 - 79)
<https://reviews.apache.org/r/44364/#comment190459>

We shouldn't need the fetcher and containerizer here.



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 137 - 140)
<https://reviews.apache.org/r/44364/#comment190465>

Could we put newlines in the string so that it prints pretty?



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 150)
<https://reviews.apache.org/r/44364/#comment190450>

Can you sweep for s/.get()./->/ ?



src/tests/environment.cpp (lines 254 - 276)
<https://reviews.apache.org/r/44364/#comment190433>

It's unfortunate that the curl filter used "error" to express existence, 
could we do a s/error/exists/ here? I'd also be ok shipping a patch for the 
curl filter if you want to take that on.


- Ben Mahler


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44364/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4863
> https://issues.apache.org/jira/browse/MESOS-4863
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit also includes a test filter called 'NvidiaGpuFilter' that
> checks for the existence of 'nvidia-smi' before running any tests that
> contain the 'NVIDIA_GPU_' prefix.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44364/diff/
> 
> 
> Testing
> ---
> 
> On an Nvidia GPU equipped machine:
> 
> ```
> sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
> bin/mesos-tests.sh
> 
> SUCCESS
> ```
> 
> On a **non** Nvidia GPU equipped machine:
> 
> ```
> sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
> bin/mesos-tests.sh
> 
> which: no nvidia-smi in (/sbin:/bin:/usr/sbin:/usr/bin)
> --
> No 'nvidia-smi' command found so no 'NvidiaGpu' tests will run
> --
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45622: Fixed Nvidia GPU isolator build for gcc.

2016-04-05 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 2, 2016, 9:35 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45622/
> ---
> 
> (Updated April 2, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-5082
> https://issues.apache.org/jira/browse/MESOS-5082
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There appears to be a discrepancy between clang and gcc, which allows
> clang to accept `using` declarations of the form `using ns_name::name;`
> that contain nested classes, structs, and enums after the `name` field
> in the declaration (e.g. `using ns_name::name::enum;`).
> 
> The language for describing this functionality is ambiguous in the
> C++11 specification as referenced here:
> http://en.cppreference.com/w/cpp/language/namespace#Using-declarations
> 
> This patch fixes a bug where we relied on this functionality in clang
> and had build errors in gcc.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 53397fc1da11a71259df1c38526a613676a60301 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> c2cdc8fde7a85741be6494ea664d3719d1f13a43 
> 
> Diff: https://reviews.apache.org/r/45622/diff/
> 
> 
> Testing
> ---
> 
> Rebuilt with nvidia gpu support for both clang and gcc.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 45773: Updated the CHANGELOG for 0.28.1.

2016-04-05 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On April 5, 2016, 8:13 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45773/
> ---
> 
> (Updated April 5, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Michael Park, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the CHANGELOG for 0.28.1.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 6309b0f46d6fc53d483758e6121dabd1b4cfbb92 
> 
> Diff: https://reviews.apache.org/r/45773/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jie Yu
> 
>



Re: Review Request 45439: Completed MVP implementation of the Nvidia GPU isolator.

2016-03-31 Thread Ben Mahler

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


Ship it!




Looks good!

We'll add some comments to the top of the class and the commit message to 
mention the current caveats.

- Ben Mahler


On March 31, 2016, 4:48 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45439/
> ---
> 
> (Updated March 31, 2016, 4:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4625
> https://issues.apache.org/jira/browse/MESOS-4625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current implementation includes some code for generic device
> isolation. Once we start isolating more devices, we should abstract
> this out and figure out an architecture to support "sub-device"
> isolation.
> 
> This was joint work with Rob Todd from Nvidia <rt...@nvidia.com>.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.hpp 
> 1e17df1f7d1daf913e0c03e0290dd85b51c2ade0 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> c2cdc8fde7a85741be6494ea664d3719d1f13a43 
> 
> Diff: https://reviews.apache.org/r/45439/diff/
> 
> 
> Testing
> ---
> 
> Test to come in subsequent patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44366: Added GPUs as an explicit resource.

2016-03-29 Thread Ben Mahler

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


Ship it!




Went over this with kevin, we'll be making some minor adjustments to the 
comments.


src/slave/containerizer/containerizer.cpp (line 105)
<https://reviews.apache.org/r/44366/#comment188912>

how about "millis" to try to make the intent of the modulo logic a bit more 
clear?



src/slave/containerizer/containerizer.cpp (lines 117 - 119)
<https://reviews.apache.org/r/44366/#comment188913>

We would probably need this TODO in all of our nvidia gpu blocks :)


- Ben Mahler


On March 14, 2016, 7:39 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44366/
> ---
> 
> (Updated March 14, 2016, 7:39 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4865
> https://issues.apache.org/jira/browse/MESOS-4865
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Currently, we enforce that the number of GPUs specified in the 'gpus'
> resource parameter equal the number of GPUs passed in via the
> --nvidia_gpu_devices flag. In the future, we will generalize this via
> autodiscovery of GPUs and support for GPU types other than Nvidia.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp bb343ad852576a75615a93ef850b413bf77698e0 
>   include/mesos/v1/resources.hpp 719110fbbf39f1755460ac0b32e3893656054a4e 
>   src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
>   src/slave/containerizer/containerizer.cpp 
> f6fc7863d0c215611f170dc0c89aa229407b5137 
>   src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 
> 
> Diff: https://reviews.apache.org/r/44366/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:string"
> Failed to determine slave resources: Bad type for resource gpus value string 
> type TEXT
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.9"
> Failed to determine slave resources: The `gpus` resource must specified as an 
> unsigned integer
>
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0"
> Failed to determine slave resources: When specifying the `gpus` resource, you 
> must also specify a list of GPUs via the `--nvidia_gpu_devices` flag
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3
> Failed to determine slave resources: The number of GPUs passed in the 
> `--nvidia_gpu_devices` flag must match the number of GPUs specified in the 
> `gpus` resource
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --resources="gpus:4.0" 
> --nvidia_gpu_devices=1,2,3,4
> SUCCESS
> 
> **NOTE**: I didn't set the `--isolation` flag here, so the agent actually 
> started up correctly (i.e. it didn't error out saying that the Nvidia GPU 
> isolator is currently unsupported).  This is the correct behaviour, and it 
> properly exercises the code added in this patch.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44365: Added flag to specify available Nvidia GPUs on an agent's command line.

2016-03-29 Thread Ben Mahler

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


Fix it, then Ship it!





docs/configuration.md (lines 1308 - 1309)
<https://reviews.apache.org/r/44365/#comment188908>

Could we add something to clarify that this is relevant when "gpus" are 
specified within --resources?


- Ben Mahler


On March 14, 2016, 7:38 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44365/
> ---
> 
> (Updated March 14, 2016, 7:38 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4864
> https://issues.apache.org/jira/browse/MESOS-4864
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added flag to specify available Nvidia GPUs on an agent's command line.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44365/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,string
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,string': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2.0,3   
> Failed to load flag 'nvidia_gpu_devices': Failed to load value '1,2,3.0': 
> Expecting all elements to be unsigned integers
> 
> ./bin/mesos-slave.sh --master=127.0.0.1:5050 --nvidia_gpu_devices=1,2,3
> SUCCESS
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44361: Added configure flags to build with Nvidia GPU support.

2016-03-29 Thread Ben Mahler

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


Fix it, then Ship it!





configure.ac (line 215)
<https://reviews.apache.org/r/44361/#comment18>

NVML



configure.ac (line 221)
<https://reviews.apache.org/r/44361/#comment19>

s/libs/libraries/ ?



configure.ac (lines 948 - 955)
<https://reviews.apache.org/r/44361/#comment188891>

Could we tell the user explicitly when the path is absolute?


- Ben Mahler


On March 14, 2016, 7:37 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44361/
> ---
> 
> (Updated March 14, 2016, 7:37 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4861
> https://issues.apache.org/jira/browse/MESOS-4861
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the initial commit to begin adding native support for GPUs in
> Mesos. This initial version will only include support for Nvidia GPUs
> that can be managed by the Nvidia Management Library (nvml).
> 
> The configure flags added in this commit can be used to enable Nvidia
> GPU support, as well as specify the installation directories of the
> nvml header and library files if not already installed in standard
> include/library paths on the system.
> 
> In a subsequent commit, we will use these configure flags to
> conditionally build support for Nvidia GPUs into Mesos.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44361/diff/
> 
> 
> Testing
> ---
> 
> I ran `bootstrap` to generate configure.
> 
> I then ran:
> 
> ```
> mkdir build; cd build
> ../configure --enable-nvidia-gpu-support
> ../configure --enable-nvidia-gpu-support --with-nvml-include=
> ../configure --enable-nvidia-gpu-support --with-nvml-include=
> ../configure --enable-nvidia-gpu-support --with-nvml-lib=
> ../configure --enable-nvidia-gpu-support --with-nvml-lib=
> ../configure --enable-nvidia-gpu-support --with-nvml-include= 
> --with-nvml-lib=
> ../configure --enable-nvidia-gpu-support 
> --with-nvml-include= --with-nvml-lib=
> ../configure --enable-nvidia-gpu-support --with-nvml-include= 
> --with-nvml-lib=
> ../configure --enable-nvidia-gpu-support 
> --with-nvml-include= --with-nvml-lib=
> ```
> 
> And verified the proper errors / successes in each case (only the last one is 
> a success).
> 
> The exact command I ran in the success case for my configuration was:
> ```
> ../configure --enable-nvidia-gpu-support 
> --with-nvml-include=/opt/nvidia-gdk/usr/include 
> --with-nvml-lib=/opt/nvidia-gdk/usr/src/gdk/nvml/lib
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44360: Added a script to install the Nvidia GDK on a host.

2016-03-29 Thread Ben Mahler

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


Ship it!




Looks great, thanks! I'll just touch up some minor typos.

- Ben Mahler


On March 14, 2016, 7:35 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44360/
> ---
> 
> (Updated March 14, 2016, 7:35 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4860
> https://issues.apache.org/jira/browse/MESOS-4860
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This script can be used to install the Nvidia GPU Deployment Kit (GDK)
> on a mesos development machine. The purpose of the GDK is to provide
> stubs for compiling code against Nvidia's latest drivers.  It includes
> all the necessary header files (nvml.h) and library files
> (libnvidia-ml.so) necessary to build mesos with Nvidia GPU support.
> However, it does not actually contain any driver code, and it can't be
> used to interact with any GPUs installed on a system. For that, you
> will have to find the latest released drivers themselves.
> 
> We provide this script only for convenience, so that developers
> working on non-GPU equipped systems can build code with Nvidia GPU
> support. It is a simple wrapper around the official GDK installer
> script provided by Nvidia, which can be downloaded here:
> 
>   https://developer.nvidia.com/gpu-deployment-kit
> 
> The script itself takes a few parameters. Run it as
> 'support/install-nvidia-gdk.sh --help' to see its usage semantics.
> 
> 
> Diffs
> -
> 
>   support/install-nvidia-gdk.sh PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44360/diff/
> 
> 
> Testing
> ---
> 
> I ran the script on my Mac to verify that it fails out cleanly.
> 
> I ran it on a linux box to exercise each of the different flags that can be 
> passed to it.
> ```
> install-nvidia-gdk.sh --install-dir=
> install-nvidia-gdk.sh --install-dir=
> install-nvidia-gdk.sh --install-dir= --update-ldcache
> install-nvidia-gdk.sh --install-dir= --update-ldcache
> ```
> 
> With each combination, I verified that the proper error messages were 
> reported, or that the installation was successful.
> 
> As per one of the comments below, I also temporarily changed the download URL 
> for the installer to make sure that the `wget` call errored out cleanly.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44777: Added a flags parser for vector to src/common/parse.hpp.

2016-03-29 Thread Ben Mahler

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


Ship it!





src/common/parse.hpp (line 123)
<https://reviews.apache.org/r/44777/#comment188857>

Do we need this temporary?

```
foreach (const std::string& token, strings::tokenize(value, ",")) {
```



src/common/parse.hpp (line 128)
<https://reviews.apache.org/r/44777/#comment188856>

Unfortunately numify doesn't follow our error composition approach of the 
callee not repeating information available to the caller, so if we followed our 
composition pattern here we would get:

```
return Error("Failed to numify token '" + token + "'"
 ": " + numify.error());

// because numify is repeating caller-available
// information, this yields:
//
// "Failed to numify 'a': Failed to convert 'a' to number"
```

But let's still do this for now, so that this continue to be meaningful 
once we update numify logging to not repeat the arguments available in the 
caller.


- Ben Mahler


On March 18, 2016, 12:14 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44777/
> ---
> 
> (Updated March 18, 2016, 12:14 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4926
> https://issues.apache.org/jira/browse/MESOS-4926
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flags parser for vector to src/common/parse.hpp.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 9535fad0d50b469fbb4dc6ac5cf5c89b40d29b47 
> 
> Diff: https://reviews.apache.org/r/44777/diff/
> 
> 
> Testing
> ---
> 
> Ran `make` and `make check`
> 
> This is also tested out in a subsequent commit when adding support for the 
> `--nvidia_gpu_devices` flag, which uses this functionality.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44776: Did a general cleanup of s/.get()./->/ in the resources abstraction.

2016-03-29 Thread Ben Mahler

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 7:36 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44776/
> ---
> 
> (Updated March 14, 2016, 7:36 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: 4928
> https://issues.apache.org/jira/browse/4928
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Did a general cleanup of s/.get()./->/ in the resources abstraction.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
>   src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 
> 
> Diff: https://reviews.apache.org/r/44776/diff/
> 
> 
> Testing
> ---
> 
> Ran `make` and `make check`
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>



Re: Review Request 44974: Added device support in cgroups abstraction.

2016-03-28 Thread Ben Mahler

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


Fix it, then Ship it!




Thanks! Kevin and I went over this and made some adjustments to the comments 
and error formatting. Will commit shortly.


src/linux/cgroups.hpp (line 625)
<https://reviews.apache.org/r/44974/#comment188652>

s/subsytem/subsystem/ and end with a period here



src/linux/cgroups.hpp (lines 628 - 629)
<https://reviews.apache.org/r/44974/#comment188655>

How about s/ListEntry/Entry/ ? I was a bit confused about whether 
"whitelist" in this context meant that this isn't used for the "deny" file, for 
example.



src/linux/cgroups.hpp (lines 634 - 664)
<https://reviews.apache.org/r/44974/#comment188662>

I'm not sure if we need some of these comments, for example I can tell 
pretty easily from 'Access' that it represents the access permissions and it is 
part of a whitelist entry because it is contained within the ListEntry class. 
Ditto for selector.


- Ben Mahler


On March 21, 2016, 6:06 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 21, 2016, 6:06 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> 
> 
> Bugs: MESOS-3368
> https://issues.apache.org/jira/browse/MESOS-3368
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There are some helper methods added to support device cgroups in
> cgroups abstraction to aid isolators controlling access to devices.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>



Re: Review Request 44975: Updated cgroups test cases for cgroups device support.

2016-03-28 Thread Ben Mahler

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


Ship it!




Thanks! Kevin and I made some minor style adjustments and will get this 
committed shortly.

- Ben Mahler


On March 21, 2016, 6:14 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44975/
> ---
> 
> (Updated March 21, 2016, 6:14 a.m.)
> 
> 
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> 
> 
> Bugs: MESOS-3368
> https://issues.apache.org/jira/browse/MESOS-3368
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cgroups test cases for cgroups device support: DevicesTest.Parse
> and CgroupsAnyHierarchyDevicesTest.ROOT_CGROUPS_Devices
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
> 
> Diff: https://reviews.apache.org/r/44975/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>



  1   2   3   4   5   6   7   8   9   >