Re: Review Request 44834: Documented task, executor, and volume IDs reuse is discouraged.

[Neil Conway]

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




include/mesos/mesos.proto (line 626)


BTW, I removed this comment because I thought it was confusing: frameworks 
"create" volumes by sending `Offer::Operation::Create` with an appropriate 
resource value. As written, it suggests (to me) that you can create a 
persistent volume by just launching a task with the persistence info defined.


- Neil Conway


On March 15, 2016, 5:54 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44834/
> ---
> 
> (Updated March 15, 2016, 5:54 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-2198
> https://issues.apache.org/jira/browse/MESOS-2198
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is generally a bad idea for (at least) two reasons:
> 
> (1) Determining when the previous entity with the ID has terminated
> can be non-trivial. Several frameworks have done this incorrectly
> in the past.
> 
> (2) When reusing IDs, logs can be more difficult to read because the
> same ID can refer to different entities at different times.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
> 
> Diff: https://reviews.apache.org/r/44834/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44828: Remove `SlaveState` in ContainerLoggerTest related test cases.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44798, 44822, 44823, 44824, 44825, 44826, 44828]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 15, 2016, 2:08 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44828/
> ---
> 
> (Updated March 15, 2016, 2:08 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove `SlaveState` in ContainerLoggerTest related test cases.
> 
> 
> Diffs
> -
> 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
> 
> Diff: https://reviews.apache.org/r/44828/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

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

[haosdent huang]

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

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


Diffs
-

  src/common/parse.hpp 9535fad0d50b469fbb4dc6ac5cf5c89b40d29b47 

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 44652: Omitted names of unused parameters in command executor.

[Ben Mahler]

> On March 10, 2016, 5:56 p.m., Joerg Schad wrote:
> > src/launcher/executor.cpp, line 124
> > 
> >
> > Not sure whether we have a real style for that, but the example I seen 
> > (and used myself) are to comment the name. See 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/src/openssl.cpp#L155
> >  for an example.
> > In this case I don't believe it makes a large difference as the Type is 
> > descriptive enough, but that might be different with more general types 
> > (imagine a string).

The openssl file is pretty new, I think the general style for this has not been 
to use `/* */` comments but rather to leave the name or omit the name.


- Ben


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


On March 14, 2016, 5:48 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44652/
> ---
> 
> (Updated March 14, 2016, 5:48 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
> 
> Diff: https://reviews.apache.org/r/44652/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44651: Fixed formatting in executor library.

[Ben Mahler]

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




src/exec/exec.cpp (line 364)


Can you rebase this without the `/* */` changes? I can't apply this patch 
as is.


- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44651/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
> 
> Diff: https://reviews.apache.org/r/44651/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44650: Omitted names of unused parameters in executor library.

[Ben Mahler]

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




src/exec/exec.cpp (line 206)


This doesn't seem like the style we use for unused arguments..?


- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44650/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Joerg Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
> 
> Diff: https://reviews.apache.org/r/44650/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 43763: Passed `Duration` as const reference in the executor library.

[Ben Mahler]

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


Ship it!




s/library/driver/

- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43763/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
> 
> Diff: https://reviews.apache.org/r/43763/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44635: Corrected the log message and variable name in executor library.

[Ben Mahler]

> On March 15, 2016, 3:13 a.m., Ben Mahler wrote:
> > Just a minor tweak that I'll apply.

We refer to this as the executor driver rather than the executor library, I'll 
use the following commit message:

Minor environment variable parsing cleanup in executor driver.


- Ben


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


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44635/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
> 
> Diff: https://reviews.apache.org/r/44635/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44635: Corrected the log message and variable name in executor library.

[Ben Mahler]

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


Fix it, then Ship it!




Just a minor tweak that I'll apply.


src/exec/exec.cpp (lines 712 - 713)


We tend to put the space on the next line, and I think having the "of" on 
the next line is clearer here, so:

```
  EXIT(EXIT_FAILURE)
<< "Failed to parse value '" << value.get() << "'"
<< " of 'MESOS_RECOVERY_TIMEOUT': " << parse.error();
```


- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44635/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
> 
> Diff: https://reviews.apache.org/r/44635/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Kevin Klues]

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

(Updated March 15, 2016, 3:11 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Addressed neilc's comments and addressed external comment from bmahler about 
moving this to src/common/parse.hpp.

It is currently not generic enough to live in stout (it is too opinionated 
about the fact that the lists *must* be comma separated, and that (if we 
generalized it) it wouldn't quite work wright for strings containing commas.  
We sould need to come up with some extra rules around formatting for that, 
which we would need to document somehwere.


Summary (updated)
-

Added a flags parser for vector to src/common/parse.hpp.


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


Repository: mesos


Description (updated)
---

Added a flags parser for vector to src/common/parse.hpp.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
295eac77d5aba4084bbc044d94f45d660410d725 
  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 44634: Updated the log message in the HTTP API executor library.

[Ben Mahler]

> On March 15, 2016, 3:08 a.m., Ben Mahler wrote:
> > Ship It!

I'll update the description to mention that this is consistent with the 
executor driver's log format.


- Ben


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


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44634/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 0050e2271d50c247ef3a396ce0848f94686762f3 
> 
> Diff: https://reviews.apache.org/r/44634/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

[Qian Zhang]

> On March 11, 2016, 11:43 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 46
> > 
> >
> > Can we list the required field before the optional fields? We can keep 
> > the position identifiers as is, if required.

Actually I'd like to remove it, please take a look at 
https://github.com/appc/cni/blob/master/pkg/types/types.go#L59:L66, this field 
is even not in the struct of CNI.


> On March 11, 2016, 11:43 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/spec.proto, line 57
> > 
> >
> > Ditto on arrangement of required followed by optional fields.

Ditto.


- Qian


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


On March 10, 2016, 10:44 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 10, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44634: Updated the log message in the HTTP API executor library.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44634/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 0050e2271d50c247ef3a396ce0848f94686762f3 
> 
> Diff: https://reviews.apache.org/r/44634/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44631: Cleaned up the comment around executor shutdown event.

[Ben Mahler]

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


Ship it!




Thanks, also two thoughts for later:

(1) I assume you've also updated this in a subsequent patch for when 
ExecutorInfo includes a grace period.

(2) I would also like to clarify that this is the _most_ amount of time they 
will get. They must not assume they will be alloted this full amount of time. I 
assume that you'll borrow the comments I had written for the 
'shutdown_grace_period' in the ExecutorInfo changes?

- Ben Mahler


On March 14, 2016, 5:45 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44631/
> ---
> 
> (Updated March 14, 2016, 5:45 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/executor/executor.proto 
> a67dbad458e24b22875cf15b73b64c14a084b1ea 
>   include/mesos/v1/executor/executor.proto 
> c4ad0b4ac6a4e8def013b276877a9c0a5264d4e9 
> 
> Diff: https://reviews.apache.org/r/44631/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44777: Added a flags parser for vector to stout.

[Neil Conway]

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




3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp (line 64)


Typo. Also, "i.e.," is usually preferrable.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp (line 69)


I wonder if this variable would be better named `result`.



3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp (line 76)


Remove whitespace before string literal



3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp (line 80)


Newline here?


- Neil Conway


On March 15, 2016, 2:23 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44777/
> ---
> 
> (Updated March 15, 2016, 2:23 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 stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
> 295eac77d5aba4084bbc044d94f45d660410d725 
> 
> 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 44630: Renamed `EXECUTOR_SHUTDOWN_GRACE_PERIOD` constant.

[Ben Mahler]

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


Ship it!




Would be great to include some description in these reviews, for example:

`EXECUTOR_SHUTDOWN_GRACE_PERIOD` is now a flag default and is no longer intended
to be used as a hard-coded constant used in the source. The general pattern in
the code is to name a flag default as `DEFAULT_X` rather than just `X`.

- Ben Mahler


On March 14, 2016, 5:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44630/
> ---
> 
> (Updated March 14, 2016, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-1571
> https://issues.apache.org/jira/browse/MESOS-1571
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
>   src/slave/constants.hpp 4110061d576d5e20e512e84bb049b6ac910ceab0 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44630/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44777: Added a flags parser for vector to stout.

[Kevin Klues]

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

(Updated March 15, 2016, 2:23 a.m.)


Review request for mesos and Ben Mahler.


Changes
---

Per some external feedback, I added a TODO about generalizing the parser to 
return a list of any comma separated input type.


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


Repository: mesos


Description
---

Added a flags parser for vector to stout.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
295eac77d5aba4084bbc044d94f45d660410d725 

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 44628: Fixed a comment and ordering in mesos.proto.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 5:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44628/
> ---
> 
> (Updated March 14, 2016, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
> 
> Diff: https://reviews.apache.org/r/44628/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44629: Fixed ordering and inconsistencies in slave constants.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 5:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44629/
> ---
> 
> (Updated March 14, 2016, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4110061d576d5e20e512e84bb049b6ac910ceab0 
> 
> Diff: https://reviews.apache.org/r/44629/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44627: Removed a stale comment in the 1.0 mesos.proto.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 5:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44627/
> ---
> 
> (Updated March 14, 2016, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
> 
> Diff: https://reviews.apache.org/r/44627/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44626: Fixed whitespaces in mesos.proto.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 14, 2016, 5:44 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44626/
> ---
> 
> (Updated March 14, 2016, 5:44 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
> 
> Diff: https://reviews.apache.org/r/44626/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Review Request 44828: Remove `SlaveState` in ContainerLoggerTest related test cases.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in ContainerLoggerTest related test cases.


Diffs
-

  src/tests/container_logger_tests.cpp 00f4129e46aa9268fbb66da25b34e61004fa87b2 

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


Testing
---


Thanks,

haosdent huang

Review Request 44826: Remove `SlaveState` in `TestContainerizer` during recover.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in `TestContainerizer` during recover.


Diffs
-

  src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc 
  src/tests/containerizer.cpp c6772ce5908edaab6c3189a65e8446217d1c7c27 

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


Testing
---


Thanks,

haosdent huang

Review Request 44825: Remove `SlaveState` in `ExternalContainerizer` during recover.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in `ExternalContainerizer` during recover.


Diffs
-

  src/slave/containerizer/external_containerizer.hpp 
feeb0278a3d8fec8c7177a9a3dc443ee0c198c5c 
  src/slave/containerizer/external_containerizer.cpp 
fe368dd52644b89e47ffd4d947de290b3998fb1a 

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


Testing
---


Thanks,

haosdent huang

Review Request 44824: Remove `SlaveState` in `ComposingContainerizer` during recover.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in `ComposingContainerizer` during recover.


Diffs
-

  src/slave/containerizer/composing.hpp 
f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 
  src/slave/containerizer/composing.cpp 
15d059f0bbda4e8cb93c65c09327dde1e34d3e7b 
  src/tests/containerizer/composing_containerizer_tests.cpp 
e7e3b622b6606a812aef046c761bf92368d34af2 

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


Testing
---


Thanks,

haosdent huang

Review Request 44823: Remove `SlaveState` in `DockerContainerizer` during recover.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Remove `SlaveState` in `DockerContainerizer` during recover.


Diffs
-

  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 

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


Testing
---


Thanks,

haosdent huang

Review Request 44822: Remove `SlaveState` in `MesosContainerizer` during recover.

[haosdent huang]

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

Review request for mesos.


Repository: mesos


Description
---

Remove `SlaveState` in `MesosContainerizer` during recover.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
4638d08328127a8c4ae37555f2be74fe9695da31 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 3:28 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 73-77
> > 
> >
> > I think the CNI plugin should also support the case where the user does 
> > not specify a name for the network. In other words,  the CNI network 
> > isolator should support host network as well. So it's OK to have no plugins.
> 
> Qian Zhang wrote:
> I have two questions:
> 1. How are we going to define the design behavior of supporting host 
> network in CNI network isolator? I mean if user does not specify name in 
> NetworkInfo when launching task, what should CNI network isolator do in 
> `prepare()` and `isolate()`? And what if user even does not specify the whole 
> NetworkInfo in ContainerInfo? My thinking is, CNI network isolator should not 
> do anything in this case.
> 2. In the code below, I check whether the plugin binary specified in each 
> network config file exists under `--network_cni_plugins` or not, if here we 
> allow the directory specified by `--network_cni_plugins` to be empty, then I 
> should remove that check too, right? But that way, we can not bail out agent 
> in `create()` to let operator know the issue which will be left to launching 
> task.
> 
> Avinash sridharan wrote:
> For (1), I think what Jie was mentioning is to have the network/cni 
> isolator do a no-op. That is return `Nothing` . 
> 
> I think (1) and (2) are not related. Consider the case where an operator 
> specifies a bunch of network configs, and there are frameworks that want to 
> join these networks. But, there could be frameworks that want to run on the 
> host network. The frameworks that want to run on host network don't provide a 
> `name` making them default to the host network.
> 
> Qian Zhang wrote:
> OK, so for (1), we all agree that `network/cni` isolator should do a 
> no-op in that case.
> 
> And for 2), I understand the case you described which does make sense to 
> me. However my concern is, if the directory specified by 
> `--network_cni_plugins_dir` is empty (i.e., no any CNI plugins in this 
> directory) and there are a bunch of network config files under 
> `--network_cni_config_dir`, then in `create()` do we still need to check 
> whether the plugin binary specified in each network config file exists under 
> `--network_cni_plugins_dir`? If we do the check, then it will definitely fail 
> since there is no CNI plugins under `--network_cni_plugins_dir`, if we do not 
> do the check, that means a network config file with an inexistent plugin will 
> be loaded, then an error must happen when launching task to join such network.
> 
> Avinash sridharan wrote:
> Semantic check. We should bail out if we detect a bad config. A bad 
> config, is a malformed config or a config for which the plugin is not present 
> or the plugin is not executable. IN the case you mentioned missing plugin is 
> semantic failure and hence isolator should bail out. Not sure what the 
> problem is here?

The problem is, in `create()`, how do we handle the case the directory 
specified by `--network_cni_plugins_dir` is empty and there are a bunch of 
network config files under `--network_cni_config_dir`? In this case, the plugin 
in each network config file is not present because `--network_cni_plugins_dir` 
is an empty directory, so do you mean we should always bail out in this case?


- Qian


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


On March 9, 2016, 2:01 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44555/
> ---
> 
> (Updated March 9, 2016, 2:01 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the framework and create() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44555/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44798: Update `Slave::_recoverContainerizer` to use `ContainerState`.

[haosdent huang]

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

(Updated March 15, 2016, 2:02 a.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Rebase


Summary (updated)
-

Update `Slave::_recoverContainerizer` to use `ContainerState`.


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


Repository: mesos


Description (updated)
---

Update `Slave::_recoverContainerizer` to use `ContainerState`.


Diffs (updated)
-

  src/slave/containerizer/containerizer.hpp 
ff78b4d0fd4a3b862f6019fc757c16b7367cd3cf 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44380: Change IOTest.BufferedRead to write to the temporary directory.

[Yong Tang]

> On March 14, 2016, 8:53 a.m., Benjamin Bannier wrote:
> > Looks mostly good to me. I left some comments, but if you want to make any 
> > progress here you'll still need to find a shepherd (see 
> > http://mesos.apache.org/documentation/latest/submitting-a-patch/).
> 
> Yong Tang wrote:
> Thanks for the help Benjamin. Let me update the review request shortly.

Just updated the review reuest. Please let me know if there are any other 
issues.


- Yong


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


On March 15, 2016, 1:38 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44380/
> ---
> 
> (Updated March 15, 2016, 1:38 a.m.)
> 
> 
> Review request for mesos and Benjamin Bannier.
> 
> 
> Bugs: MESOS-4807
> https://issues.apache.org/jira/browse/MESOS-4807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit changes IOTest.BufferedRead so that this specific
> test (not all IOTest) could be executed from temporary
> directories via TemporaryDirectoryTest fixture (MESOS-4807).
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/io_tests.cpp 
> 2bffc7cd9c3aa204a1d1b8eb45f0bff12f49ca62 
> 
> Diff: https://reviews.apache.org/r/44380/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 4:02 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.
> 
> Avinash sridharan wrote:
> For (2) why not bind mount directly to 
> /var/run/mesos/cni/netns/[continaerId] and create a simlink to 
> /var/run/netns/. Also, Jie mentioned it would be a good idea to prefix the 
> `[containerId]` with a conanical string like mesos-cni. For (3) I don't think 
> we should rely on files created by CNI plugins. There is no guarantee that 
> the IPAM plugin would maintain those files. Instead if we have already bind 
> moutned the network namespace, the namespace will persist in the kernel even 
> after the container has exited (became an orphan). At recover we just need to 
> walk to list of `containerID` stored under /var/run/mesos/cni/netns and for 
> each network namespace we can run the `ip netns comamnd` to retrieve the link 
> and IP address for that network namespace.

For 2), what symlink do you want to create under `/var/run/netns`? 
`/var/run/netns/[pid] -> /var/run/mesos/cni/netns/[containerId]`? Or 
`/var/run/netns/[containerId] -> /var/run/mesos/cni/netns/[contiainerId]`? And 
you can take a look at 
https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/isolators/network/port_mapping.cpp#L2205:L2243,
 What it does is same as what I proposed above.

And good point for 3), I agree in `recover()`, we can run `ip netns exec 
[namespace] ip addr` to retrieve the link and IP, but the problem is how we can 
know which CNI networks that these links/IPs belong to? E.g., after run `ip 

Re: Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Guangya Liu]

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




src/tests/containerizer/runtime_isolator_tests.cpp (lines 93 - 95)


The comments should also be updated? Also should mention that do not want 
use up disk due to work dir may be copied to rootfs?

Ditto for other `flags.docker_store_dir`


- Guangya Liu


On 三月 15, 2016, 12:22 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44753/
> ---
> 
> (Updated 三月 15, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4942
> https://issues.apache.org/jira/browse/MESOS-4942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed runtime isolator tests out of disk issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 9fd5ea9e465f21624d3cdc0125a702528fe7548b 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> d8a000413fb30d7daaa77827287a2395d81b8b04 
> 
> Diff: https://reviews.apache.org/r/44753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Guangya Liu]

> On 三月 15, 2016, 12:58 a.m., Guangya Liu wrote:
> > @Gilbert, can you please show some detail in commit messsage to descrbe how 
> > does the fix resolve the out of disk issue? Thanks.

I saw that you just filed a JIRA and show some explantaion there, thanks!


- Guangya


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


On 三月 15, 2016, 12:22 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44753/
> ---
> 
> (Updated 三月 15, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4942
> https://issues.apache.org/jira/browse/MESOS-4942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed runtime isolator tests out of disk issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 9fd5ea9e465f21624d3cdc0125a702528fe7548b 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> d8a000413fb30d7daaa77827287a2395d81b8b04 
> 
> Diff: https://reviews.apache.org/r/44753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Guangya Liu]

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



@Gilbert, can you please show some detail in commit messsage to descrbe how 
does the fix resolve the out of disk issue? Thanks.

- Guangya Liu


On 三月 15, 2016, 12:22 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44753/
> ---
> 
> (Updated 三月 15, 2016, 12:22 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4942
> https://issues.apache.org/jira/browse/MESOS-4942
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed runtime isolator tests out of disk issue.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> 9fd5ea9e465f21624d3cdc0125a702528fe7548b 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> d8a000413fb30d7daaa77827287a2395d81b8b04 
> 
> Diff: https://reviews.apache.org/r/44753/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 73-77
> > 
> >
> > I think the CNI plugin should also support the case where the user does 
> > not specify a name for the network. In other words,  the CNI network 
> > isolator should support host network as well. So it's OK to have no plugins.
> 
> Qian Zhang wrote:
> I have two questions:
> 1. How are we going to define the design behavior of supporting host 
> network in CNI network isolator? I mean if user does not specify name in 
> NetworkInfo when launching task, what should CNI network isolator do in 
> `prepare()` and `isolate()`? And what if user even does not specify the whole 
> NetworkInfo in ContainerInfo? My thinking is, CNI network isolator should not 
> do anything in this case.
> 2. In the code below, I check whether the plugin binary specified in each 
> network config file exists under `--network_cni_plugins` or not, if here we 
> allow the directory specified by `--network_cni_plugins` to be empty, then I 
> should remove that check too, right? But that way, we can not bail out agent 
> in `create()` to let operator know the issue which will be left to launching 
> task.
> 
> Avinash sridharan wrote:
> For (1), I think what Jie was mentioning is to have the network/cni 
> isolator do a no-op. That is return `Nothing` . 
> 
> I think (1) and (2) are not related. Consider the case where an operator 
> specifies a bunch of network configs, and there are frameworks that want to 
> join these networks. But, there could be frameworks that want to run on the 
> host network. The frameworks that want to run on host network don't provide a 
> `name` making them default to the host network.
> 
> Qian Zhang wrote:
> OK, so for (1), we all agree that `network/cni` isolator should do a 
> no-op in that case.
> 
> And for 2), I understand the case you described which does make sense to 
> me. However my concern is, if the directory specified by 
> `--network_cni_plugins_dir` is empty (i.e., no any CNI plugins in this 
> directory) and there are a bunch of network config files under 
> `--network_cni_config_dir`, then in `create()` do we still need to check 
> whether the plugin binary specified in each network config file exists under 
> `--network_cni_plugins_dir`? If we do the check, then it will definitely fail 
> since there is no CNI plugins under `--network_cni_plugins_dir`, if we do not 
> do the check, that means a network config file with an inexistent plugin will 
> be loaded, then an error must happen when launching task to join such network.

Semantic check. We should bail out if we detect a bad config. A bad config, is 
a malformed config or a config for which the plugin is not present or the 
plugin is not executable. IN the case you mentioned missing plugin is semantic 
failure and hence isolator should bail out. Not sure what the problem is here?


- Avinash


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


On March 9, 2016, 6:01 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44555/
> ---
> 
> (Updated March 9, 2016, 6:01 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the framework and create() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44555/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 3:28 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 73-77
> > 
> >
> > I think the CNI plugin should also support the case where the user does 
> > not specify a name for the network. In other words,  the CNI network 
> > isolator should support host network as well. So it's OK to have no plugins.
> 
> Qian Zhang wrote:
> I have two questions:
> 1. How are we going to define the design behavior of supporting host 
> network in CNI network isolator? I mean if user does not specify name in 
> NetworkInfo when launching task, what should CNI network isolator do in 
> `prepare()` and `isolate()`? And what if user even does not specify the whole 
> NetworkInfo in ContainerInfo? My thinking is, CNI network isolator should not 
> do anything in this case.
> 2. In the code below, I check whether the plugin binary specified in each 
> network config file exists under `--network_cni_plugins` or not, if here we 
> allow the directory specified by `--network_cni_plugins` to be empty, then I 
> should remove that check too, right? But that way, we can not bail out agent 
> in `create()` to let operator know the issue which will be left to launching 
> task.
> 
> Avinash sridharan wrote:
> For (1), I think what Jie was mentioning is to have the network/cni 
> isolator do a no-op. That is return `Nothing` . 
> 
> I think (1) and (2) are not related. Consider the case where an operator 
> specifies a bunch of network configs, and there are frameworks that want to 
> join these networks. But, there could be frameworks that want to run on the 
> host network. The frameworks that want to run on host network don't provide a 
> `name` making them default to the host network.

OK, so for (1), we all agree that `network/cni` isolator should do a no-op in 
that case.

And for 2), I understand the case you described which does make sense to me. 
However my concern is, if the directory specified by 
`--network_cni_plugins_dir` is empty (i.e., no any CNI plugins in this 
directory) and there are a bunch of network config files under 
`--network_cni_config_dir`, then in `create()` do we still need to check 
whether the plugin binary specified in each network config file exists under 
`--network_cni_plugins_dir`? If we do the check, then it will definitely fail 
since there is no CNI plugins under `--network_cni_plugins_dir`, if we do not 
do the check, that means a network config file with an inexistent plugin will 
be loaded, then an error must happen when launching task to join such network.


- Qian


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


On March 9, 2016, 2:01 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44555/
> ---
> 
> (Updated March 9, 2016, 2:01 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the framework and create() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44555/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44753: Fixed runtime isolator tests out of disk issue.

[Gilbert Song]

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

(Updated March 14, 2016, 5:22 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Fixed runtime isolator tests out of disk issue.


Diffs
-

  src/tests/containerizer/provisioner_docker_tests.cpp 
9fd5ea9e465f21624d3cdc0125a702528fe7548b 
  src/tests/containerizer/runtime_isolator_tests.cpp 
d8a000413fb30d7daaa77827287a2395d81b8b04 

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


Testing
---

make check

sudo ./bin/mesos-test.sh


Thanks,

Gilbert Song

Re: Review Request 44545: Separated standalone and zookeeper classes.

[Anurag Singh]

> On March 11, 2016, 9:42 p.m., Joseph Wu wrote:
> > src/master/detectors/standalone.cpp, lines 58-60
> > 
> >
> > I don't see where these helpers are deleted in your later reviews.

This may have happened during one of my rebase -i attempts. Fixing this now.


- Anurag


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


On March 14, 2016, 11:51 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44545/
> ---
> 
> (Updated March 14, 2016, 11:51 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead of keeping standalone and zookeper contender/detector class
> definitions and implementations in the same file, separated them. Also
> made the necessary changes in users of class headers to point to the
> new locations.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
>   src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
>   src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
>   src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
>   src/master/contenders/contender.hpp PRE-CREATION 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/contenders/standalone.cpp PRE-CREATION 
>   src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
>   src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
>   src/master/detectors/detector.hpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
>   src/master/detectors/standalone.cpp PRE-CREATION 
>   src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
>   src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
>   src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/fault_tolerance_tests.cpp 
> 349669d6aa0ead63b2ebcfcc2f769c99a6db0192 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 6a4de4709960d7ca505e99396e14a1bb51d6902d 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
>   src/tests/mesos.hpp 908a53a146208a6d41c0bda5208415e39e2eae05 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_tests.cpp 
> e9215de2e073025f67cdc73e8a8de38cf030671f 
>   src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
>   src/tests/reservation_tests.cpp c88091635949037fcfc6be504043764f9af35f79 
>   src/tests/scheduler_event_call_tests.cpp 
> 8c02ceeb3ec1783cb2f63f100700508e70f586e4 
>   src/tests/scheduler_http_api_tests.cpp 
> dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
>   src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 
>   src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
>   src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
> 
> Diff: https://reviews.apache.org/r/44545/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44081: Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 14, 2016, 9:10 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44081/
> ---
> 
> (Updated March 14, 2016, 9:10 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
> 79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 9ee233b988c08d953e70345c55bcdd5c2f7c101b 
> 
> Diff: https://reviews.apache.org/r/44081/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44288: Changed MasterDetector/Contender namespace.

[Anurag Singh]

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

(Updated March 14, 2016, 11:49 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Also modified users of MasterContender and MasterDetector to use this
namespace.


Diffs (updated)
-

  include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
  include/mesos/v1/scheduler.hpp 765935e97b6c1686ab464a5cf1cf2dfd816f51f1 
  src/cli/resolve.cpp 257e29034abf32491511f9a4e476b6859714829d 
  src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
  src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
  src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
  src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
  src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
  src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
  src/sched/sched.cpp 525255eec808c3fe5c0e38b3d1a2086bbd4eb171 
  src/scheduler/scheduler.cpp 35f479483740baed3f7bdbf45bf1d5bba3b9febc 
  src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 
  src/tests/fault_tolerance_tests.cpp 349669d6aa0ead63b2ebcfcc2f769c99a6db0192 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_authorization_tests.cpp 
6a4de4709960d7ca505e99396e14a1bb51d6902d 
  src/tests/master_contender_detector_tests.cpp 
255ab8119a04b55bb4f1b61dee19c4be64499376 
  src/tests/master_slave_reconciliation_tests.cpp 
d41178eb41df519073fc0890c5716bbc9fed6ad2 
  src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
  src/tests/mesos.hpp 908a53a146208a6d41c0bda5208415e39e2eae05 
  src/tests/mesos.cpp 7cca4ed4753fa365b209d1d22f0df4650b19bc6a 
  src/tests/oversubscription_tests.cpp e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
  src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
  src/tests/persistent_volume_tests.cpp 
e9215de2e073025f67cdc73e8a8de38cf030671f 
  src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
  src/tests/reservation_tests.cpp c88091635949037fcfc6be504043764f9af35f79 
  src/tests/scheduler_event_call_tests.cpp 
8c02ceeb3ec1783cb2f63f100700508e70f586e4 
  src/tests/scheduler_http_api_tests.cpp 
dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
  src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 
  src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 

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


Testing
---

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


Thanks,

Anurag Singh

Re: Review Request 44670: Added master_detector and master_contender flags.

[Anurag Singh]

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

(Updated March 14, 2016, 11:53 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

The master_detector and master_contender flags allow modules to be
used for specifying the MasterContender and MasterDetector
implementations to use.


Diffs (updated)
-

  src/master/flags.hpp f8d2cc4c6c8dab00e34ca737dbcb5b9ca3870d6d 
  src/master/flags.cpp e6fea6421ea1a16b9cd78b0e42b830829b95ad61 
  src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
  src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 

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


Testing
---

In addition to all unit tests passing, we are currently using this 
functionality in our environment with a custom consensus stack. In our world, 
we have a C++ plugin that calls out to an HTTP REST service (implemented in 
Java/Scala, not that it matters).


Thanks,

Anurag Singh

Re: Review Request 44547: Added functions in promises to the collect header.

[Anurag Singh]

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

(Updated March 14, 2016, 11:52 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Added functions in promises to the collect header.


Diffs (updated)
-

  3rdparty/libprocess/include/process/collect.hpp 
5a92b72eb7668494dc832ec446a41b3d673a20cc 

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


Testing
---


Thanks,

Anurag Singh

Re: Review Request 44289: Added support for contender and detector modules.

[Anurag Singh]

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

(Updated March 14, 2016, 11:52 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Added support for contender and detector modules.


Diffs (updated)
-

  include/mesos/module/contender.hpp PRE-CREATION 
  include/mesos/module/detector.hpp PRE-CREATION 
  src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
  src/examples/test_contender_module.cpp PRE-CREATION 
  src/examples/test_detector_module.cpp PRE-CREATION 
  src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
  src/master/contenders/zookeeper.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
  src/master/detectors/zookeeper.cpp 9274435802d6292b183be48f42b43999476e016e 
  src/module/manager.cpp 8c9aaf7cd00c904daba9994a99df9e1329831c01 
  src/tests/module.hpp 4b32f29f2ce76100433621a5cb6b8cc87c9b38f8 
  src/tests/module.cpp 8cc305c0ef606b07eea39d548d3165a2bb2b042a 

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


Testing
---

In addition to all unit tests passing, we are currently using this 
functionality in our environment with a custom consensus stack. In our world, 
we have a C++ plugin that calls out to an HTTP REST service (implemented in 
Java/Scala, not that it matters).


Thanks,

Anurag Singh

Re: Review Request 44669: Added createFromModule methods to MasterContender and MasterDetector.

[Anurag Singh]

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

(Updated March 14, 2016, 11:52 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

The createFromModule will be used to create a MasterContender/Detector
from a module (specified using the --modules flag on the command
line).


Diffs (updated)
-

  src/master/contenders/contender.cpp PRE-CREATION 
  src/master/detectors/detector.cpp PRE-CREATION 

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


Testing
---


Thanks,

Anurag Singh

Re: Review Request 44082: Stout: Un-commented out functions and marked them as deleted instead.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 14, 2016, 9:12 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44082/
> ---
> 
> (Updated March 14, 2016, 9:12 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Un-commented out functions and marked them as deleted instead.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 6a391ff198ab724f689bcef79d4e2e05a786cbc2 
> 
> Diff: https://reviews.apache.org/r/44082/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44545: Separated standalone and zookeeper classes.

[Anurag Singh]

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

(Updated March 14, 2016, 11:51 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Instead of keeping standalone and zookeper contender/detector class
definitions and implementations in the same file, separated them. Also
made the necessary changes in users of class headers to point to the
new locations.


Diffs (updated)
-

  src/Makefile.am 7ee5a653fc96907021f14ab28f74c0b3ed0649d9 
  src/local/local.cpp f8599e7378e9a0065bbd01ad8f23f11debb30c91 
  src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
  src/master/contender.cpp 9ad49ce10439fb41d78d52eaa4c1e6b9c5c7f735 
  src/master/contenders/contender.hpp PRE-CREATION 
  src/master/contenders/contender.cpp PRE-CREATION 
  src/master/contenders/standalone.cpp PRE-CREATION 
  src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 
  src/master/detector.cpp 9274435802d6292b183be48f42b43999476e016e 
  src/master/detectors/detector.hpp PRE-CREATION 
  src/master/detectors/detector.cpp PRE-CREATION 
  src/master/detectors/standalone.cpp PRE-CREATION 
  src/master/main.cpp 7c1656bcc266f6c94cb4befad37fa813a218b2fa 
  src/master/master.hpp 124d43931a5c8a00ee0aaa604feb1761795209f2 
  src/slave/main.cpp e3a4d13ddaeb89ba01c9b2ddfc72c37934f753eb 
  src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
  src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 
  src/tests/containerizer/external_containerizer_test.cpp 
8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
  src/tests/containerizer/isolator_tests.cpp 
342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
  src/tests/fault_tolerance_tests.cpp 349669d6aa0ead63b2ebcfcc2f769c99a6db0192 
  src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_authorization_tests.cpp 
6a4de4709960d7ca505e99396e14a1bb51d6902d 
  src/tests/master_contender_detector_tests.cpp 
255ab8119a04b55bb4f1b61dee19c4be64499376 
  src/tests/master_slave_reconciliation_tests.cpp 
d41178eb41df519073fc0890c5716bbc9fed6ad2 
  src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
  src/tests/mesos.hpp 908a53a146208a6d41c0bda5208415e39e2eae05 
  src/tests/oversubscription_tests.cpp e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
  src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
  src/tests/persistent_volume_tests.cpp 
e9215de2e073025f67cdc73e8a8de38cf030671f 
  src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
  src/tests/reservation_tests.cpp c88091635949037fcfc6be504043764f9af35f79 
  src/tests/scheduler_event_call_tests.cpp 
8c02ceeb3ec1783cb2f63f100700508e70f586e4 
  src/tests/scheduler_http_api_tests.cpp 
dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
  src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 
  src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 

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


Testing
---


Thanks,

Anurag Singh

Re: Review Request 44544: Moved contender and detector definitions into separate directories.

[Anurag Singh]

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

(Updated March 14, 2016, 11:50 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

Moved contender and detector definitions into separate directories.


Diffs (updated)
-

  src/master/contenders/contender.hpp PRE-CREATION 
  src/master/contenders/contender.cpp PRE-CREATION 
  src/master/detectors/detector.hpp PRE-CREATION 
  src/master/detectors/detector.cpp PRE-CREATION 

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


Testing
---


Thanks,

Anurag Singh

Re: Review Request 44543: Removed unnecessary MasterContender and MasterDetector definitions.

[Anurag Singh]

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

(Updated March 14, 2016, 11:50 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

MasterContender and MasterDetector are now defined in
include/mesos/master/contender.hpp and detector.hpp.


Diffs (updated)
-

  src/master/contender.hpp 3fd20f8e94daab349b76d8f5ecc87398a187a847 
  src/master/detector.hpp eb5d2a90b60c629150ddf04acf00f0edca1ca723 

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


Testing
---


Thanks,

Anurag Singh

Re: Review Request 40375: Support distinguishing revocable resources in the Resource protobuf.

[Guangya Liu]

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




src/common/resources.cpp (line 187)


update to throttle_info



src/common/resources.cpp (lines 1551 - 1553)


update to throttle_info


- Guangya Liu


On 三月 14, 2016, 7:12 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/40375/
> ---
> 
> (Updated 三月 14, 2016, 7:12 a.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-3888
> https://issues.apache.org/jira/browse/MESOS-3888
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3888: We need to distinguish revocable resource for usage slack and 
> allocation slack.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
>   include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
>   src/common/resources.cpp 1f23dc83f7330c305a836d698f114b7eaf3d7ba1 
>   src/tests/resources_tests.cpp 6b004d64bb25112b19fc5d98b5bca874c5329e8c 
>   src/v1/resources.cpp c6f125ec317e2da537a6456f5cff2da0a48701d8 
> 
> Diff: https://reviews.apache.org/r/40375/diff/
> 
> 
> Testing
> ---
> 
> make && make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 44287: Added MasterContender and MasterDetector abstract classes.

[Anurag Singh]

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

(Updated March 14, 2016, 11:48 p.m.)


Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

MasterContender and MasterDetector abstract classes can be subclassed by
user-provided classes to allow for arbitrary contender/detector implementations.


Diffs (updated)
-

  include/mesos/master/contender.hpp PRE-CREATION 
  include/mesos/master/detector.hpp PRE-CREATION 

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


Testing
---

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


Thanks,

Anurag Singh

Re: Review Request 44808: Fixup POSIX build by removing headers from load.*.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 14, 2016, 9:06 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44808/
> ---
> 
> (Updated March 14, 2016, 9:06 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixup POSIX build by removing headers from load.*.
> 
> 
> Diffs
> -
> 
>   src/slave/qos_controllers/load.hpp 098a6d0b2dfc54b5b95a261a780eea70a838c12d 
>   src/slave/qos_controllers/load.cpp dd44f9209ad283bfea95f16a8c1017e309757f23 
> 
> Diff: https://reviews.apache.org/r/44808/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44808: Fixup POSIX build by removing headers from load.*.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 14, 2016, 9:06 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44808/
> ---
> 
> (Updated March 14, 2016, 9:06 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixup POSIX build by removing headers from load.*.
> 
> 
> Diffs
> -
> 
>   src/slave/qos_controllers/load.hpp 098a6d0b2dfc54b5b95a261a780eea70a838c12d 
>   src/slave/qos_controllers/load.cpp dd44f9209ad283bfea95f16a8c1017e309757f23 
> 
> Diff: https://reviews.apache.org/r/44808/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44080: Windows:[1/2] Implemented `os::gmtime_r`.

[Yi Sun]

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


Ship it!




Ship It!

- Yi Sun


On March 14, 2016, 8:48 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44080/
> ---
> 
> (Updated March 14, 2016, 8:48 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4462
> https://issues.apache.org/jira/browse/MESOS-4462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows:[1/2] Implemented `os::gmtime_r`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 9ee233b988c08d953e70345c55bcdd5c2f7c101b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 6a391ff198ab724f689bcef79d4e2e05a786cbc2 
> 
> Diff: https://reviews.apache.org/r/44080/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 44708: Fixed signed / unsigned comparison in docker.cpp.

[Timothy Chen]

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


Ship it!




Ship It!

- Timothy Chen


On March 14, 2016, 5:50 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44708/
> ---
> 
> (Updated March 14, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4110061d576d5e20e512e84bb049b6ac910ceab0 
> 
> Diff: https://reviews.apache.org/r/44708/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

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

[Kevin Klues]

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



As mentioned in https://reviews.apache.org/r/44476/ this patch should probably 
be partially combined with it and then split out into tow pieces:

1) The new API code
2) The tests.

Other comments below.


src/linux/cgroups.hpp (line 34)


This should go away, given the feedback in the previous patch.



src/linux/cgroups.hpp (lines 629 - 635)


This function probably shouldn't do the work of checking the list for an 
entry.  Instead, it should return a vector of whitelist entries (i.e. 
`vector`) which the user can parse as he sees fit. The comment 
should cahnge to match.



src/linux/cgroups.cpp (lines 2426 - 2427)


Given the feedback on the previous patch, this should probably take a 
`DeviceEntry`, not a `Selector` and `Access` parameter separately.

I realize this is contary to what we suggested previously, but we've had a 
bit more time to think about it now, and we think this makes the msot sense.



src/linux/cgroups.cpp (lines 2429 - 2437)


As per the comment above, we should probably jsut return a vector here.  
Let the code outside of this decide what to do with the list.



src/linux/cgroups.cpp (lines 2443 - 2444)


Same comment as above.



src/linux/cgroups.cpp (lines 2450 - 2451)


We should probably put a blank line between thses two statements for 
consistency with other code.



src/linux/cgroups.cpp (lines 2461 - 2462)


Same comment as above.



src/linux/cgroups.cpp (lines 2468 - 2469)


Same comment as above.



src/tests/containerizer/cgroups_tests.cpp (line 1284)


Can you put a comment here about why we need to deny all by default for 
this to work (I know why this is necessary, but it may not be clear for others 
trying to walk through all the cases)?

For example, without first denying all, the following fails:

* write allow all
* check /dev/console
  FAIL

This will fail because of the way the devices.list file works. It only 
actually prints out entries **explicitly** written to it previously.

This means that writing "allow all" followed by "check /dev/null" will 
always fail, even though we obviously have access.

Unfortunately, this is just the nature of the interface and due 
"whitelisting" instead of "blacklisting".



src/tests/containerizer/cgroups_tests.cpp (lines 1286 - 1287)


Update throughout with `DeviceEntry`



src/tests/containerizer/cgroups_tests.cpp (line 1287)


I would probably not use any sort of default constructor for either 
`Selector` or `Access`.  It is much more readable to see the actual parameters 
passed.



src/tests/containerizer/cgroups_tests.cpp (lines 1289 - 1293)


This test will probably have to change given the new semantics of 
`devices::list()`


- Kevin Klues


On March 14, 2016, 6:25 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44439/
> ---
> 
> (Updated March 14, 2016, 6:25 p.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 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
> 
> Diff: https://reviews.apache.org/r/44439/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44661: Deprecated the `docker_stop_timeout` flag.

[Timothy Chen]

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




src/slave/flags.cpp (line 531)


Seems like we usually just specify that "This flag is deprecated, ." in 
the other places in the code base. I prefer to big bold fonts like this but I 
don't want us to have many different ways to mark deprecation either.

I say we keep it all consistent.


- Timothy Chen


On March 14, 2016, 5:51 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44661/
> ---
> 
> (Updated March 14, 2016, 5:51 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Timothy Chen.
> 
> 
> Bugs: MESOS-4910
> https://issues.apache.org/jira/browse/MESOS-4910
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Instead, a combination of `executor_shutdown_grace_period`
> agent flag and task kill policies should be used.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 739d4ff9aeeb1ba70ce11033168d63d37b6ef56b 
>   src/docker/executor.hpp abbc419533ab40312e917931a2fc2ce78b38da41 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
> 
> Diff: https://reviews.apache.org/r/44661/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44797: Updated libprocess makefile to include device_tests.cpp.

[Kevin Klues]

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



The order of reviewboard dependencies is wrong in this patch.  This patch 
actually depends on 44797, not the other way around as it is now. Did you use 
support/post-reviews.py to post these patches?  Usually, it sets the 
"Depends-On" and "Blocked-By" fields properly for your commit chain.


That said, I talked to Ben, and he agress that the new types for `Selector` and 
`Access` should not live in stout, but rather in `src/linux/cgroups.hpp`, 
making this patch unnecessary.  It should likely be discarded.


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


Reviewboard is marking this as incorrect spacing, but I downloaded it 
locally, and everything looks good.

For future reference to people watching this review: to get the right 
spacing, you should set your tabspace=8 and then tab out until the '\'s all 
line up.


- Kevin Klues


On March 14, 2016, 6:17 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44797/
> ---
> 
> (Updated March 14, 2016, 6:17 p.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
> ---
> 
> Updated libprocess makefile to include device_tests.cpp.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 
> ddf7e3d9bf76d4a03c33f02d52ec29812aef8509 
> 
> Diff: https://reviews.apache.org/r/44797/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44796: Added helper classes for cgroups device entry.

[Kevin Klues]

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



This commit and https://reviews.apache.org/r/44439/ should probably be combined 
and then broken up into the follow two commits (one for the implementation and 
one for the test):

```
Added test for cgroups device entry parsing.
Added helper classes and functions for managing cgroups devices.
```

The motivation being that these helpers should probably not live inside stout.  
Stout is designed as a **platform independent** API, whereas the stuff you 
added here is very specific to cgroups on Linux.  I would suggest putting it 
into `src/linux/cgroups.{hpp,cpp}` with the rest of the cgroups stuff (as you 
alrdady have some stuff in 44439).  As for the test, I would put it at the 
bottom of `src/tests/containerizer/cgroups_tests.cp` and call it 
`TEST(CgroupsDeviceTest, ParseTest)`.

The comments on the files below are still valid, they just be moved to these 
new locations.


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


Reviewboard is reporting a whitespace error here. However, I downloaded 
things locally and everything is fine.

That said, this test should probably not exist in stout (nor should the 
device helpers).  See the other comments for a suggestion on where to put it 
instead.



3rdparty/libprocess/3rdparty/stout/include/Makefile.am (line 24)


Unnecessary, with move to `src/linux/cgroups.hpp`



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 25)


Once moved to `src/linux/cgroups.hpp`, this should all exist in the 
`cgroups::devices` namespace.

Also, all of the classes in this file should be declared in the `.hpp` file 
and defined in the `.cpp`.

This is a similar pattern followed by `cgroups::memory::pressure::Counter`



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 26 - 31)


I would probably embed this enum inside the `Device` class (as mentioned in 
a comment below).



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 33)


I would suggest adding a class called `DeviceEntry`, which containes 
subfields for `Selector` and `Access`.

With this, a parse function in the `cgroups::devices::parse()` namespace 
can be written to parse full whitelist entries, i.e.:

```
static Try parse(const std::string& entry);
```

I would also then make the `Selector` and `Access` classes structs instead 
of classes.



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (line 35)


I would probably not embed parse functions in the `Selector` and `Access` 
classes as you have done here.  I sugest having a global `parse()` function 
that operates on `DeviceEntry` objects as described in a previous comment.



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 37 - 40)


We typically don't "pre-declare" variables like this in Mesos.  Instead, we 
declare them just before their first use.

Also, using a size_t for index isn't really the correct type here. It 
should probably be `std::string::size_type`.



3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp (lines 42 - 100)


Assuming we move this parse function out to a global `Try 
parse()` function, I feel we can get away sith something much simpler here. 
Something like:

```
Try parse(const std::string& entry)
{
  std::vector tokens = strings::tokenize(entry, " ");

  if (tokens.size() == 0) {
return Error("Unable to parse cgroups device entry: no arguments 
provided");
  }

  if (token[0] == "a") {
Selector selector(Selector::Type::ALL, "*", "*");
Access access(true, true, true);
return DeviceEntry(selector, access);
  }

  Selector selector;
  
  if (token[0] == "b") {
selector.deviceType = Selector::Type::BLOCK;
  else if (token[0] == "c") {
selector.deviceType = Selector::Type::CHARACTER;
  } else {
return Error("Unable to parse cgroups device entry: unknown device 
type: ", + token[0]);
  }
  
  if (tokens.size() != 3) {
return Error("Unable to parse cgroups device entry: not enough 
arguments for device type: ", + token[0]);
  }  
  
  Selector selector;
  selector.deviceType = token[0];

  std::vector deviceNumbers = strings::tokenize(token[1], ":");
  
 

Re: Review Request 44710: Fixed placement of mock call expectation for fetcher cache tests.

[Anand Mazumdar]

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


Ship it!




LGTM.

- Anand Mazumdar


On March 14, 2016, 1:35 p.m., Bernd Mathiske wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44710/
> ---
> 
> (Updated March 14, 2016, 1:35 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Jan Schlicht, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2858
> https://issues.apache.org/jira/browse/MESOS-2858
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The EXPECT_CALL() statement in question should not be repeated in the loop 
> once per task. It should only be called exactly once before the driver is 
> started.
> 
> 
> Diffs
> -
> 
>   src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
> 
> Diff: https://reviews.apache.org/r/44710/diff/
> 
> 
> Testing
> ---
> 
> make check - all fetcher cache tests succeeded.
> 
> 
> Thanks,
> 
> Bernd Mathiske
> 
>

Re: Review Request 44734: Marked a few internal functions `static`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On March 12, 2016, 12:29 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44734/
> ---
> 
> (Updated March 12, 2016, 12:29 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Marked a few internal functions `static`.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 3e92979567c7bcefa9ab5f32aa7b23000aa9deb8 
> 
> Diff: https://reviews.apache.org/r/44734/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 43823: Updated `/tasks` master endpoint to use jsonify.

[Michael Park]

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


Ship it!





src/master/http.cpp (line 1950)


Committed with `this` removed.


- Michael Park


On March 11, 2016, 10:10 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43823/
> ---
> 
> (Updated March 11, 2016, 10:10 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated `/tasks` master endpoint to use jsonify.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 893e0651b89de4df1813f8f4f365eab80a7b3bc2 
> 
> Diff: https://reviews.apache.org/r/43823/diff/
> 
> 
> Testing
> ---
> 
> make check. Also verified that this endpoint is covered by the unit tests.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

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

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44439, 44796, 44797]

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

Error:
2016-03-14 22:32:01 URL:https://reviews.apache.org/r/44796/diff/raw/ 
[8734/8734] -> "44796.patch" [1]
error: patch failed: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am:21
error: 3rdparty/libprocess/3rdparty/stout/include/Makefile.am: patch does not 
apply

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

- Mesos ReviewBot


On March 14, 2016, 6:25 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44439/
> ---
> 
> (Updated March 14, 2016, 6:25 p.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 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
> 
> Diff: https://reviews.apache.org/r/44439/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44424: Updated http_command_executor.cpp to use v1 API.

[Anand Mazumdar]

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



Qian, any updates on this?

- Anand Mazumdar


On March 6, 2016, 9:08 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44424/
> ---
> 
> (Updated March 6, 2016, 9:08 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-3558
> https://issues.apache.org/jira/browse/MESOS-3558
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated http_command_executor.cpp to use v1 API.
> 
> 
> Diffs
> -
> 
>   include/mesos/v1/mesos.proto 31960a52061f70d80528fb8326522ae1d6f75b2c 
>   src/launcher/http_command_executor.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44424/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 8:02 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.hpp, line 85
> > 
> >
> > I would suggest we have a `Info` for each container.
> > 
> > ```
> > struct Info
> > {
> >   ...
> > };
> > ```
> > 
> > Remember that for any field you put into the Info struct, you need to 
> > be able to 'recover' it in recover function. It's also possible that 
> > ExecutorInfo/TaskInfo are not available during recover (orphans due to 
> > wiped meta data). You need to think about how to recover (e.g., the name of 
> > the network) that the container has joined.
> 
> Avinash sridharan wrote:
> We also need to recover the IP address associated with the container. 
> Since this has to be returned as part of `NetworkINfo` . Need to figure out 
> how to recover this?
> 
> Jie Yu wrote:
> I am not sure. Why do we need to store/recover IP address? When destroy a 
> container, the CNI plugin only needs container ID (which can be the same as 
> our container Id), the namespace handle and the networkconfig file. The rest 
> can all be fixed.
> 
> I think the trick part is: when there is an orphan container (no 
> executorinfo/taskinfo), how can we figure out its network namespace handle 
> and networconfig file so that we can destroy it (call CNI plugin DEL). We 
> need to know the name of the network to get the networkcofig file, and the 
> pid to get the network namespace handle.
> 
> Qian Zhang wrote:
> Yes, that is the tricky part. For orphan container (e.g., the container 
> created by the framework without checkpoint enabled), when agent restarts, in 
> the recover() we only have container ID which is not enough to invoke CNI 
> plugin DEL. Totally we need:
> 1) Container ID.
> 2) Network namespace path.
> 3) Network configuration.
> 4) Name of the interface inside the container.
> 
> I think currently we only have 1).
> For 2), my idea is, in `isolate()`, before calling CNI plugin, we do a 
> bind mount (`/var/run/netns/[pid]` -> `proc/[pid]/ns/net`), and then a symbol 
> link (`/var/run/mesos/cni/netns/[containerId]` -> `/var/run/netns/[pid]`). 
> And then in `recover()`, since we have `containerId` for orphan containers, 
> we should be able to figure out the `pid` based on the symbol link, and then 
> get the network namespace path `/proc/[pid]/ns/net`.
> For 3), actually when we call CNI plugin for a container in `isolate()`, 
> the CNI plugin (actually the CNI `host-local` IPAM plugin) will create a file 
> under `/var/lib/cni/networks/[neworkName]/`, the name of the file is the IP 
> assigned to the container, and the content of file is the container ID. So if 
> a container is assigned an IP from a CNI network, it must have a file under 
> `/var/lib/cni/networks/[neworkName]/`, we may leverage this information to 
> figure out the name of CNI networks the orphan container joins. But this may 
> only work for the CNI network which uses `host-local` IPAM plugin, for `dhcp` 
> IPAM plugin, I think it will not write any files for containers.
> For 4), I do not have idea for it yet :-(
> 
> Or another solution would be, when we create the symbol link in 
> `isolate()`, we may encode more info into the name of symbol link, e.g., 
> `/var/run/mesos/cni/netns/[containerId]-[net1]_[ifName in 
> net1]-[net2]_[ifName in net2]...`, however this seems ugly to me.

For (2) why not bind mount directly to /var/run/mesos/cni/netns/[continaerId] 
and create a simlink to /var/run/netns/. Also, Jie mentioned it would be a good 
idea to prefix the `[containerId]` with a conanical string like mesos-cni. For 
(3) I don't think we should rely on files created by CNI plugins. There is no 
guarantee that the IPAM plugin would maintain those files. Instead if we have 
already bind moutned the network namespace, the namespace will persist in the 
kernel even after the container has exited (became an orphan). At recover we 
just need to walk to list of `containerID` stored under 
/var/run/mesos/cni/netns and for each network namespace we can run the `ip 
netns comamnd` to retrieve the link and IP address for that network namespace.


- Avinash


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


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> 

Re: Review Request 44733: Added fault tolerance tests for the V1 API.

[Anand Mazumdar]

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

(Updated March 14, 2016, 10:14 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Review comments from Vinod


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


Repository: mesos


Description
---

Similar to already existing tests in `fault_tolerance_tests.cpp`,
this change adds fault tolerance tests for HTTP schedulers/executors.
Some tests have been omitted since they were no longer valid now due
to the persistent streaming connection between the master and the
scheduler in the new API.


Diffs (updated)
-

  src/Makefile.am 8abef3bd3237a6e456faa7884637207dc4d63282 
  src/tests/http_fault_tolerance_tests.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Anand Mazumdar

Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.

[Greg Mann]

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

(Updated March 14, 2016, 10 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


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


Repository: mesos


Description (updated)
---

Modified basic HTTP authenticator creator to accept realm.

To accommodate different authentication realms for the master and agent, the 
default basic HTTP authenticator needs to accept its authentication realm as a 
parameter. This patch adds this parameter and modifies the HTTP authentication 
tests to validate it appropriately. A new test was also added: 
`HttpAuthenticationTest.BasicWithoutRealm`.


Diffs
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
  src/authentication/http/basic_authenticator_factory.cpp 
62f851685db3b42c52bbcb7cff3e4f4703004ed7 
  src/examples/test_http_authenticator_module.cpp 
459b7046bd76d3043d2484a2dd30c10d7deaedd4 
  src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
  src/tests/http_authentication_tests.cpp 
cf2bb762272fa38e04e5c26aef2858300bbd0459 

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


Testing (updated)
---

HTTP authentication tests were updated to pass the authentication realm to the 
basic HTTP authenticator, and to adhere to the new credentials format in the 
module parameters. A new test was also added: 
`HttpAuthenticationTest.BasicWithoutRealm`

`make check` was used to test on both OSX and CentOS 7.


Thanks,

Greg Mann

Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.

[Greg Mann]

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

(Updated March 14, 2016, 9:58 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Added new test for basic authenticator without a realm. Improved error checking.


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


Repository: mesos


Description
---

Modified basic HTTP authenticator creator to accept realm.

To accommodate different authentication realms for the master and agent, the 
default basic HTTP authenticator needs to accept its authentication realm as a 
parameter. This patch adds this parameter and modifies the HTTP authentication 
tests to validate it appropriately.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
  src/authentication/http/basic_authenticator_factory.cpp 
62f851685db3b42c52bbcb7cff3e4f4703004ed7 
  src/examples/test_http_authenticator_module.cpp 
459b7046bd76d3043d2484a2dd30c10d7deaedd4 
  src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
  src/tests/http_authentication_tests.cpp 
cf2bb762272fa38e04e5c26aef2858300bbd0459 

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


Testing
---

`make check` was used to test on both OSX and CentOS 7.


Thanks,

Greg Mann

Review Request 44810: Documentation change for the update quota feature.

[Zhitao Li]

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

Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
Michael Browning.


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


Repository: mesos


Description
---

Documentation change for the update quota feature.

This serves as the first patch in the review chain for quota update feature and 
documents the current plan. Actual implementations to follow once we reach 
consensus on the this plan.


Diffs
-

  docs/quota.md 12696bf805d43f997d80149e56281c5e7dc0557e 

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


Testing
---


Thanks,

Zhitao Li

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Joseph Wu]

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

(Updated March 14, 2016, 2:33 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Rebase and update tests changed due to Alexander's authorizer changes and 
Joerg's auth changes.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description (updated)
---

Includes the following changes:

* Added the `` header where appropriate.
* Added the namespace `using process::Owned;` where appropriate.
* Generally replaced `Try` with `Try`.  
And `Try` with `Try`.
* Added the (now required) `MasterDetector` argument to all slaves.  Before, 
this was fetched from the first master in `Cluster`.
* Removed `Shutdown();` from all tests.
* Replaced `Stop(...)` with the appropriate master/slave destruction calls.
* Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
launchers, etc).
* Replace `CHECK` in tests with `ASSERT`.


Diffs (updated)
-

  src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
  src/tests/command_executor_tests.cpp 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
  src/tests/container_logger_tests.cpp 00f4129e46aa9268fbb66da25b34e61004fa87b2 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
  src/tests/containerizer/external_containerizer_test.cpp 
8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
  src/tests/containerizer/filesystem_isolator_tests.cpp 
e72239a55724f1aeeec5362cc370c93dbeca7164 
  src/tests/containerizer/isolator_tests.cpp 
342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
  src/tests/containerizer/memory_pressure_tests.cpp 
03879d99c371f296f8d9904666911b34209c114d 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
  src/tests/containerizer/port_mapping_tests.cpp 
a1427fd0157dee343b643f3272dba8ffea61f7b0 
  src/tests/containerizer/provisioner_docker_tests.cpp 
9fd5ea9e465f21624d3cdc0125a702528fe7548b 
  src/tests/containerizer/runtime_isolator_tests.cpp 
d8a000413fb30d7daaa77827287a2395d81b8b04 
  src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
  src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
  src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
  src/tests/executor_http_api_tests.cpp 
2fc0893f5f5e80a783296fb31b30abe86d92df1b 
  src/tests/fault_tolerance_tests.cpp 349669d6aa0ead63b2ebcfcc2f769c99a6db0192 
  src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
  src/tests/health_check_tests.cpp 9605859b665645654bbdb2688455cae2d692a057 
  src/tests/hook_tests.cpp bb287c77493209e663a37e7f88d09a0459855f7d 
  src/tests/master_allocator_tests.cpp cba7c36471f93b678d94e1da0251a28a893696b1 
  src/tests/master_authorization_tests.cpp 
6a4de4709960d7ca505e99396e14a1bb51d6902d 
  src/tests/master_contender_detector_tests.cpp 
255ab8119a04b55bb4f1b61dee19c4be64499376 
  src/tests/master_quota_tests.cpp f1277f03f5b5cf735fea7ba324a27614a829cd50 
  src/tests/master_slave_reconciliation_tests.cpp 
d41178eb41df519073fc0890c5716bbc9fed6ad2 
  src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
  src/tests/master_validation_tests.cpp 
c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
  src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
  src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
  src/tests/oversubscription_tests.cpp e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
  src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
  src/tests/persistent_volume_endpoints_tests.cpp 
b5f425ab7f2b4a540418d761bea145565467bd2b 
  src/tests/persistent_volume_tests.cpp 
e9215de2e073025f67cdc73e8a8de38cf030671f 
  src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
  src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
  src/tests/registrar_zookeeper_tests.cpp 
3df9779ee5d076e16f6a538326693a36f986b6d0 
  src/tests/repair_tests.cpp 0dfb557a62f64baf2334dbc9f75ecdfb10473c2d 
  src/tests/reservation_endpoints_tests.cpp 
f95ae7a32c3809d150adf1e9e515a3b527e61699 
  src/tests/reservation_tests.cpp c88091635949037fcfc6be504043764f9af35f79 
  src/tests/role_tests.cpp f45ee816f6ab2f988fbe4efdb14c58b7538e06c8 
  src/tests/scheduler_driver_tests.cpp f6dc25d82ae5f1e77fc6ede7ff2660ed0d9ea039 
  src/tests/scheduler_event_call_tests.cpp 
8c02ceeb3ec1783cb2f63f100700508e70f586e4 
  src/tests/scheduler_http_api_tests.cpp 
dfb0f51fec67a3951e396eab28eedb0dbf9493ae 
  src/tests/slave_tests.cpp 124e9587180f2a55e659d966d1c9060234c19457 
  src/tests/status_update_manager_tests.cpp 
9440d3b14a7f5fe6277bd1d61a891014f5de9fc3 
  

Re: Review Request 43630: Especially updated scheduler tests to use the updated MesosTest helpers.

[Joseph Wu]

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

(Updated March 14, 2016, 2:32 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Rebase and update MasterMaintenanceTest changes.  Also modified the new 
EndpointsBadAuthentication test.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Continuation of https://reviews.apache.org/r/43615/ with re-ordering of some 
local variables due to the order of destruction.


Diffs (updated)
-

  src/tests/master_maintenance_tests.cpp 
1dce98bebcd920de53124b1dde10c2067bfc4020 
  src/tests/scheduler_tests.cpp fa42fb42f2d18060a867ade547cebbdcaead07d4 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43629: Especially updated tests to use the updated MesosTest helpers.

[Joseph Wu]

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

(Updated March 14, 2016, 2:32 p.m.)


Review request for mesos, Bernd Mathiske and Artem Harutyunyan.


Changes
---

Modified the slightly irregular (and new) DynamicWeightsTests.  The 
DynamicWeightsTests have a helper that uses the `Master`'s PID.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Continuation of https://reviews.apache.org/r/43615/ with a slightly different 
pattern.


Diffs (updated)
-

  src/tests/dynamic_weights_tests.cpp a95663f633f376954d4201b6cfbe693429be9c39 
  src/tests/fetcher_cache_tests.cpp f9c48f5d938c2601cb8f826029d6969d676ab98e 
  src/tests/resource_offers_tests.cpp 0bad45dd1dabecc88fef1ab46e8ea26718070b33 
  src/tests/slave_recovery_tests.cpp bd7b94f3f1fac6705e5bf14c6f6103b540cde56c 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Re: Review Request 43613: Refactor cluster test helpers into self-contained objects.

[Joseph Wu]

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

(Updated March 14, 2016, 2:31 p.m.)


Review request for mesos, Benjamin Hindman, Bernd Mathiske, Artem Harutyunyan, 
and Michael Park.


Changes
---

Rebase and integrate Alexander's authorizer changes.


Bugs: MESOS-4633 and MESOS-4634
https://issues.apache.org/jira/browse/MESOS-4633
https://issues.apache.org/jira/browse/MESOS-4634


Repository: mesos


Description
---

Major rewrite of the `tests/cluster` helpers.  This strongly ties the scope of 
test objects to the test body.

Changes the `Cluster` class into two RAII objects (`Master` and `Slave`).  The 
`Slave` object performs cleanup originally found in `cluster::Slave::stop`.  
`cluster::Master::start` and `cluster::Slave::start` were changed to factory 
methods.


Diffs (updated)
-

  src/tests/cluster.hpp 99a785ab0d4ee1a1e745202d2551de58a7631a85 
  src/tests/cluster.cpp e5796d3cc17f814bec8f02dccf40515b65cfea91 

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


Testing
---

Tests are run at the end of this review chain.


Thanks,

Joseph Wu

Review Request 44082: Stout: Un-commented out functions and marked them as deleted instead.

[Alex Clemmer]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Stout: Un-commented out functions and marked them as deleted instead.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
6a391ff198ab724f689bcef79d4e2e05a786cbc2 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 44081: Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.

[Alex Clemmer]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Stout: Moved `os::libraries::` namespace back to `stout/os.hpp`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os.hpp 
79e30ca04c6d23f92e3a2f80fbe38ae63fde3520 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
9ee233b988c08d953e70345c55bcdd5c2f7c101b 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 44808: Fixup POSIX build by removing headers from load.*.

[Alex Clemmer]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


Repository: mesos


Description
---

Fixup POSIX build by removing headers from load.*.


Diffs
-

  src/slave/qos_controllers/load.hpp 098a6d0b2dfc54b5b95a261a780eea70a838c12d 
  src/slave/qos_controllers/load.cpp dd44f9209ad283bfea95f16a8c1017e309757f23 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44798: [WIP]Remove SlaveState in containerizer interface.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [44798]

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

Error:
2016-03-14 20:56:30 URL:https://reviews.apache.org/r/44798/diff/raw/ 
[51223/51223] -> "44798.patch" [1]
Total errors found: 0
Checking 16 files
Error: Commit message summary (the first line) must start with a capital letter.

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

- Mesos ReviewBot


On March 14, 2016, 6:36 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44798/
> ---
> 
> (Updated March 14, 2016, 6:36 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Bugs: MESOS-3709
> https://issues.apache.org/jira/browse/MESOS-3709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove SlaveState in containerizer interface.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/composing.hpp 
> f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 
>   src/slave/containerizer/composing.cpp 
> 15d059f0bbda4e8cb93c65c09327dde1e34d3e7b 
>   src/slave/containerizer/containerizer.hpp 
> ff78b4d0fd4a3b862f6019fc757c16b7367cd3cf 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
>   src/slave/containerizer/external_containerizer.hpp 
> feeb0278a3d8fec8c7177a9a3dc443ee0c198c5c 
>   src/slave/containerizer/external_containerizer.cpp 
> fe368dd52644b89e47ffd4d947de290b3998fb1a 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
>   src/slave/containerizer/mesos/containerizer.cpp 
> af3ff5750649497d8852b4761c78d4cae5455a02 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc 
>   src/tests/containerizer.cpp c6772ce5908edaab6c3189a65e8446217d1c7c27 
>   src/tests/containerizer/composing_containerizer_tests.cpp 
> e7e3b622b6606a812aef046c761bf92368d34af2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
> 
> Diff: https://reviews.apache.org/r/44798/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 44080: Windows:[1/2] Implemented `os::gmtime_r`.

[Alex Clemmer]

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

Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, Joris 
Van Remoortere, Michael Park, M Lawindi, and Yi Sun.


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


Repository: mesos


Description
---

Windows:[1/2] Implemented `os::gmtime_r`.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
9ee233b988c08d953e70345c55bcdd5c2f7c101b 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
6a391ff198ab724f689bcef79d4e2e05a786cbc2 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 6:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 228
> > 
> >
> > Can there be a use case where you want multiple NICs to be attached to 
> > the same network? Servers use this configuration when they want to utilize 
> > NIC bonding. To aggregate the bandwidth available on the NICs
> 
> Qian Zhang wrote:
> Good point! I think it makes sense that user enables NIC bonding by 
> creating a bond device (e.g., bond0) as the master of the normal ethernet 
> devices (e.g., eth0 and eth1), and both eth0 and eth1 are set up by CNI 
> plugin and get assigned IP from CNI plugin in the same subnet. My only 
> concern is, how to configure the IP for bond0, maybe just use IP of either 
> eth0 or eth1 as its IP?

I think for the time being we can leave the behavior as is. To support 
something like NIC bonding, I think we will need to introduce the concept of 
`repeated` devices in `NetworkInfo`, which is definitely out of the scope of 
this work.


> On March 11, 2016, 6:19 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 236
> > 
> >
> > If the CNI configuration is host-local then we will be calling the 
> > host-local plugin during `isolate`. So why do we need this?
> 
> Qian Zhang wrote:
> The purpose of this comment is, when we support to explicitly request an 
> IP for container in future, here we need to get `NetworkInfo.ip_addresses` to 
> know which IP the container want to request, and keep it in the isolator 
> (e.g., in the Info struct for the container), and later in isolate() method, 
> we need to set the value of the environment variable `CNI_ARGS` to that IP 
> when invoking plugin, see 
> https://github.com/appc/cni/blob/master/Documentation/host-local.md#supported-arguments
>  for more details.

Now that we have a `struct Info` for each container, we should have 
`NetworkInfo` as a field in `struct Info`. We can store any statically defined 
IP addresses in the `NetworkInfo`.


- Avinash


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


On March 10, 2016, 2:20 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 10, 2016, 2:20 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44555: Implemented the framework and create() method of "network/cni" isolator.

[Avinash sridharan]

> On March 11, 2016, 7:28 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, lines 73-77
> > 
> >
> > I think the CNI plugin should also support the case where the user does 
> > not specify a name for the network. In other words,  the CNI network 
> > isolator should support host network as well. So it's OK to have no plugins.
> 
> Qian Zhang wrote:
> I have two questions:
> 1. How are we going to define the design behavior of supporting host 
> network in CNI network isolator? I mean if user does not specify name in 
> NetworkInfo when launching task, what should CNI network isolator do in 
> `prepare()` and `isolate()`? And what if user even does not specify the whole 
> NetworkInfo in ContainerInfo? My thinking is, CNI network isolator should not 
> do anything in this case.
> 2. In the code below, I check whether the plugin binary specified in each 
> network config file exists under `--network_cni_plugins` or not, if here we 
> allow the directory specified by `--network_cni_plugins` to be empty, then I 
> should remove that check too, right? But that way, we can not bail out agent 
> in `create()` to let operator know the issue which will be left to launching 
> task.

For (1), I think what Jie was mentioning is to have the network/cni isolator do 
a no-op. That is return `Nothing` . 

I think (1) and (2) are not related. Consider the case where an operator 
specifies a bunch of network configs, and there are frameworks that want to 
join these networks. But, there could be frameworks that want to run on the 
host network. The frameworks that want to run on host network don't provide a 
`name` making them default to the host network.


- Avinash


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


On March 9, 2016, 6:01 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44555/
> ---
> 
> (Updated March 9, 2016, 6:01 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented the framework and create() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 8f57a5701073bf1eaaa223383e928cf5db8f8ae4 
>   src/Makefile.am a41e95ddeb838fdebf4ced953c4a29181916e261 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44555/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[Abhishek Dasgupta]

> On March 9, 2016, 1:58 a.m., Ben Mahler wrote:
> > src/tests/containerizer/cgroups_tests.cpp, lines 1278-1309
> > 
> >
> > Does this test pass? Did you make sure this test ran? Note that you 
> > have to run the tests under 'sudo' on Linux for this test to run because it 
> > has the ROOT and CGROUPS strings in the test name.

Hi,

I actually ran these tests. And here is the output:

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from CgroupsAnyHierarchyDevicesTest
[ RUN  ] CgroupsAnyHierarchyDevicesTest.ROOT_CGROUPS_Devices
[   OK ] CgroupsAnyHierarchyDevicesTest.ROOT_CGROUPS_Devices (10 ms)
[--] 1 test from CgroupsAnyHierarchyDevicesTest (10 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (31 ms total)
[  PASSED  ] 1 test.
make[3]: Leaving directory '/home/abhishek/test/mesos/build/src'
make[2]: Leaving directory '/home/abhishek/test/mesos/build/src'

devices.list can't be changed by writing on it. It is to be changed by writing 
in devices.deny or device.allow. I followed this logic developing the code. 
Please, contradict me if I am wrong in any assumption/deduction.


- Abhishek


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


On March 14, 2016, 6:25 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44439/
> ---
> 
> (Updated March 14, 2016, 6:25 p.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 
>   src/tests/containerizer/cgroups_tests.cpp 
> acaed9b3f8a04964092cef413133834d0cf5a145 
> 
> Diff: https://reviews.apache.org/r/44439/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44709: Allowed unknown flags in command and docker executors.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44626, 44627, 44628, 44629, 44630, 44631, 44634, 44635, 
43763, 44650, 44651, 44652, 44653, 44654, 44655, 44656, 44707, 44657, 44658, 
44659, 44708, 44660, 44661, 44662, 44709]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 14, 2016, 5:51 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44709/
> ---
> 
> (Updated March 14, 2016, 5:51 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 
>   src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
> 
> Diff: https://reviews.apache.org/r/44709/diff/
> 
> 
> Testing
> ---
> 
> `make check` on Mac OS 10.10.4
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44583: Reran `generate-endpoint-help.py` script for `/weights` endpoint.

[Kevin Klues]

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



The generated documentation is now wrong.  It almost loooks like no delegate 
was set on the master for some reason.

- Kevin Klues


On March 14, 2016, 5:40 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44583/
> ---
> 
> (Updated March 14, 2016, 5:40 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reran `generate-endpoint-help.py` script for `/weights` endpoint.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/index.md 27b178efa1c2d1a00f557018e3343293ebfb3430 
>   docs/endpoints/master/weights.md PRE-CREATION 
>   src/master/http.cpp bda03da5d0fdc69b92fad2fad6662b805e49f046 
> 
> Diff: https://reviews.apache.org/r/44583/diff/
> 
> 
> Testing
> ---
> 
> Previewed via site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44583: Reran `generate-endpoint-help.py` script for `/weights` endpoint.

[Neil Conway]

> On March 14, 2016, 6:54 p.m., Kevin Klues wrote:
> > The generated documentation is now wrong.  It almost loooks like no 
> > delegate was set on the master for some reason.

Ah -- I rebased but didn't actually rerun the script to pickup the recent 
changes; that is done now.


- Neil


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


On March 14, 2016, 6:58 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44583/
> ---
> 
> (Updated March 14, 2016, 6:58 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reran `generate-endpoint-help.py` script for `/weights` endpoint.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/index.md 27b178efa1c2d1a00f557018e3343293ebfb3430 
>   docs/endpoints/master/weights.md PRE-CREATION 
>   src/master/http.cpp bda03da5d0fdc69b92fad2fad6662b805e49f046 
> 
> Diff: https://reviews.apache.org/r/44583/diff/
> 
> 
> Testing
> ---
> 
> Previewed via site-docker.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44583: Reran `generate-endpoint-help.py` script for `/weights` endpoint.

[Neil Conway]

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

(Updated March 14, 2016, 6:58 p.m.)


Review request for mesos and Adam B.


Changes
---

Reran script to pickup recent changes.


Repository: mesos


Description
---

Reran `generate-endpoint-help.py` script for `/weights` endpoint.


Diffs (updated)
-

  docs/endpoints/index.md 27b178efa1c2d1a00f557018e3343293ebfb3430 
  docs/endpoints/master/weights.md PRE-CREATION 
  src/master/http.cpp bda03da5d0fdc69b92fad2fad6662b805e49f046 

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


Testing
---

Previewed via site-docker.


Thanks,

Neil Conway

Re: Review Request 44785: Don't use --as-needed on OSX.

[Steve Niemitz]

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

(Updated March 14, 2016, 6:58 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


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


Repository: mesos


Description
---

Don't use --as-needed on OSX.


Diffs
-

  src/python/native_common/ext_modules.py.in 
0a005dc5746aec428be6dbaf02e58f293d87a05c 

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


Testing
---


Thanks,

Steve Niemitz

Re: Review Request 44785: Don't use --as-needed on OSX.

[Steve Niemitz]

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

(Updated March 14, 2016, 6:48 p.m.)


Review request for mesos, Ben Mahler and Vinod Kone.


Changes
---

Added TODO


Repository: mesos


Description
---

Don't use --as-needed on OSX.


Diffs (updated)
-

  src/python/native_common/ext_modules.py.in 
0a005dc5746aec428be6dbaf02e58f293d87a05c 

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


Testing
---


Thanks,

Steve Niemitz

Review Request 44798: [WIP]Remove SlaveState in containerizer interface.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

Remove SlaveState in containerizer interface.


Diffs
-

  src/slave/containerizer/composing.hpp 
f3eebd19bc9e6b3b8a969a2ad967b3e2909e0ee4 
  src/slave/containerizer/composing.cpp 
15d059f0bbda4e8cb93c65c09327dde1e34d3e7b 
  src/slave/containerizer/containerizer.hpp 
ff78b4d0fd4a3b862f6019fc757c16b7367cd3cf 
  src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
  src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
  src/slave/containerizer/external_containerizer.hpp 
feeb0278a3d8fec8c7177a9a3dc443ee0c198c5c 
  src/slave/containerizer/external_containerizer.cpp 
fe368dd52644b89e47ffd4d947de290b3998fb1a 
  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
af3ff5750649497d8852b4761c78d4cae5455a02 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/container_logger_tests.cpp 00f4129e46aa9268fbb66da25b34e61004fa87b2 
  src/tests/containerizer.hpp 67fbe7fedbe170c3f22a2dcbb5aebf4195a5aabc 
  src/tests/containerizer.cpp c6772ce5908edaab6c3189a65e8446217d1c7c27 
  src/tests/containerizer/composing_containerizer_tests.cpp 
e7e3b622b6606a812aef046c761bf92368d34af2 
  src/tests/containerizer/docker_containerizer_tests.cpp 
8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44087: Moved logic to assign process to freezer hierarchy into parentHook.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44790, 44791, 44087]

Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh

- Mesos ReviewBot


On March 14, 2016, 3:44 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44087/
> ---
> 
> (Updated March 14, 2016, 3:44 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-4839
> https://issues.apache.org/jira/browse/MESOS-4839
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously assigning the process to the freezer hierarchy basically 
> reimplemented the same blocking and error handling logic provided by 
> parentHooks.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 9c80cfb621ef2e28aabfb2649846892964d2d4f3 
> 
> Diff: https://reviews.apache.org/r/44087/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (on Linux)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Abhishek Dasgupta]

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

(Updated March 14, 2016, 6:25 p.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 (updated)
-

  src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
  src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
  src/tests/containerizer/cgroups_tests.cpp 
acaed9b3f8a04964092cef413133834d0cf5a145 

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


Testing
---

make check


Thanks,

Abhishek Dasgupta

Review Request 44796: Added helper classes for cgroups device entry.

[Abhishek Dasgupta]

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

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

Helper classes are added for device entry which is passed on to device cgroups 
to provide devices as resources.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/Makefile.am 
400c6dc451602926f93b22713af8c66d7ca59ca6 
  3rdparty/libprocess/3rdparty/stout/include/Makefile.am 
c10c6d9d7c68a2d5b27d68736a49d212e70dcd05 
  3rdparty/libprocess/3rdparty/stout/include/stout/device.hpp PRE-CREATION 
  3rdparty/libprocess/3rdparty/stout/tests/CMakeLists.txt 
03e09fb33bf038930a2be0370883511e93f94753 
  3rdparty/libprocess/3rdparty/stout/tests/device_tests.cpp PRE-CREATION 

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


Testing
---

sudo make -j2 check GTEST_FILTER="DeviceTest.parseTest"


Thanks,

Abhishek Dasgupta

Review Request 44797: Updated libprocess makefile to include device_tests.cpp.

[Abhishek Dasgupta]

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

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

Updated libprocess makefile to include device_tests.cpp.


Diffs
-

  3rdparty/libprocess/3rdparty/Makefile.am 
ddf7e3d9bf76d4a03c33f02d52ec29812aef8509 

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


Testing
---


Thanks,

Abhishek Dasgupta

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 14, 2016, 6:16 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.

[Greg Mann]

> On March 14, 2016, 9:38 a.m., Adam B wrote:
> > src/authentication/http/basic_authenticator_factory.cpp, lines 63-64
> > 
> >
> > Seems like you're changing the meaning of the parameters in 
> > `BasicAuthenticatorFactory::create(Parameters)`. Before, `Parameters` was 
> > just a convenient protobuf for username/password key-value pairs. Now, 
> > you're interpreting the keys as module parameter names. Are you actually 
> > introducing a new "authentication_realm" module parameter to the 
> > Authenticator module interface?
> > Why not just use `BasicAuthenticatorFactory::create(const string realm, 
> > const Parameters& parameters)`?

One could certainly use the other overloads of `create` to specify a realm, but 
if we don't provide a way to include the realm in the `Parameters`, then we 
still need a default realm for this version of `create`, and I was trying to 
eliminate the default realm from this file. AFAICT, there isn't a well-defined 
interface for the HTTP authenticator modules, with respect to their input 
parameters. I didn't find mention of it in the [design 
doc](https://docs.google.com/document/d/1kM3_f7DSqXcE2MuERrLTGp_XMC6ss2wmpkNYDCY5rOM/edit#heading=h.bbxx6wwg3tpe),
 or in related JIRAs. I was working under the assumption that the structure of 
the input parameters for any given HTTP authN module is defined arbitrarily by 
the module's implementation, but perhaps that's not the case?


- Greg


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


On March 14, 2016, 6:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44678/
> ---
> 
> (Updated March 14, 2016, 6:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
> https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Modified basic HTTP authenticator creator to accept realm.
> 
> To accommodate different authentication realms for the master and agent, the 
> default basic HTTP authenticator needs to accept its authentication realm as 
> a parameter. This patch adds this parameter and modifies the HTTP 
> authentication tests to validate it appropriately.
> 
> 
> Diffs
> -
> 
>   include/mesos/authentication/http/basic_authenticator_factory.hpp 
> c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
>   src/authentication/http/basic_authenticator_factory.cpp 
> 62f851685db3b42c52bbcb7cff3e4f4703004ed7 
>   src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
>   src/tests/http_authentication_tests.cpp 
> cf2bb762272fa38e04e5c26aef2858300bbd0459 
> 
> Diff: https://reviews.apache.org/r/44678/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on both OSX and CentOS 7.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44678: Modified basic HTTP authenticator creator to accept realm.

[Greg Mann]

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

(Updated March 14, 2016, 6:01 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Modified basic HTTP authenticator creator to accept realm.

To accommodate different authentication realms for the master and agent, the 
default basic HTTP authenticator needs to accept its authentication realm as a 
parameter. This patch adds this parameter and modifies the HTTP authentication 
tests to validate it appropriately.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 
  src/authentication/http/basic_authenticator_factory.cpp 
62f851685db3b42c52bbcb7cff3e4f4703004ed7 
  src/master/master.cpp d0380db3b90a9166607445f8dd50cc63d547228e 
  src/tests/http_authentication_tests.cpp 
cf2bb762272fa38e04e5c26aef2858300bbd0459 

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


Testing
---

`make check` was used to test on both OSX and CentOS 7.


Thanks,

Greg Mann

Re: Review Request 44660: Used `KillPolicy` and shutdown grace period in docker executor.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:50 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44708: Fixed signed / unsigned comparison in docker.cpp.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:50 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/constants.hpp 4110061d576d5e20e512e84bb049b6ac910ceab0 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44659: Updated the comment about docker executor.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:50 p.m.)


Review request for mesos, Ben Mahler and Timothy Chen.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44707: Added validation for task's kill policy.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:49 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/master/validation.hpp 29dbdf1da540db4966ff9e86037badde06e69a4b 
  src/master/validation.cpp 820a9faee6fde09a98317854b181fe897167e2ca 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44655: Made `shutdown_grace_period` configurable in `ExecutorInfo`.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:48 p.m.)


Review request for mesos and Gilbert Song.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  include/mesos/mesos.proto 56d456acfd35fa59f394b27d62f62772eec03f6a 
  include/mesos/v1/mesos.proto 4fba77464bb052d27c424f7721397142850b1144 
  src/exec/exec.cpp dec7e8814e7151718d1c89381458753f2e22739e 
  src/executor/executor.cpp 0050e2271d50c247ef3a396ce0848f94686762f3 
  src/slave/flags.cpp 5bacf109bcd9c9eae1b7ec3c32095c72899ebdbd 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44709: Allowed unknown flags in command and docker executors.

[Alexander Rukletsov]

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

(Updated March 14, 2016, 5:51 p.m.)


Review request for mesos and Ben Mahler.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 
  src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 

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


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov