Re: Review Request 49308: Added agent and scheduler authentication backoff.

[Benjamin Bannier]

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

(Updated June 29, 2016, 9:08 a.m.)


Review request for mesos, Adam B and Vinod Kone.


Changes
---

Removed stale comments.


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


Repository: mesos


Description
---

The backoff follows to existing pattern for backoff used during agent
or scheduler registration where we backoff for some random time in an
interval of increasing length, capped by
`REGISTER_RETRY_INTERVAL_MAX`.


Diffs (updated)
-

  src/sched/sched.cpp 9f561d73a2e591afdc3ba4adb35a11763dced402 
  src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 
  src/slave/slave.cpp da643e6e50b2f313705d2f862c961291aa5d2f22 

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


Testing
---

make check (OS X w/o optimizations).


Thanks,

Benjamin Bannier

Re: Review Request 49337: Moved code to group old/v1 operator API implementations together.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49335, 49336, 49337]

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

- Mesos ReviewBot


On June 28, 2016, 8:58 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49337/
> ---
> 
> (Updated June 28, 2016, 8:58 p.m.)
> 
> 
> Review request for mesos, Abhishek Dasgupta, Jay Guo, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change moves the create/destroy volumes, reserve/unreserve
> for the v1 API to be closer to the corresponding implementations
> for the old API.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e827e6948a7eec51c914975e8b77a094844cb63f 
> 
> Diff: https://reviews.apache.org/r/49337/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Adam B]

> On June 28, 2016, 10:17 p.m., Jie Yu wrote:
> > src/master/http.cpp, lines 2080-2082
> > 
> >
> > This looks like a flag too? If you do not want to treat that as a flag, 
> > add a note at least?

Yeah, we can document it. I think the cluster name should be generic enough 
metadata that we can leave it open to any (authenticated) user. Other endpoints 
like /version didn't seem worth protecting either. We're not even trying to 
protect access to cluster resource information, let alone cluster name.


- Adam


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


On June 28, 2016, 3:51 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 28, 2016, 3:51 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_FLAGS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 81804e0522fd6b26155732af08e33c4d0bb0a8fe 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET http://127.0.0.1:5050/state -a baz:bar
> http GET http://127.0.0.1:5051/state -a baz:bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

> On June 29, 2016, 12:51 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1241-1262
> > 
> >
> > flags specific authorization should be done in `_flags()` so that the 
> > v1 API can get the benefit automatically. 
> > 
> > please move this logic inside `_flags()`. you might need to adjust its 
> > return type and add a `__flags()`.

The main problem with this suggestion is, `_flags` return type is 
`JSON::Object`, which means it is supposed to be called synchronously by both 
`flags()` and `getFlags()`.

If I were to make `_flags()` async, the return type would be 
`Future` but then, how do I encode the fact that I must return 
`Forbidden`? I could make `_flags()` return a `Future` but then it 
would not be different from `flags()`, moreover `getFlags()` expects a 
`JSON::Object` since it evolves it to a `v1::master::Response`, so it shouldn't 
take a `Future`.

My solution would be to use the same code in `getFlags()` to perform 
authorization.


- Alexander


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


On June 29, 2016, 12:51 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_FLAGS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 81804e0522fd6b26155732af08e33c4d0bb0a8fe 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET http://127.0.0.1:5050/state -a baz:bar
> http GET http://127.0.0.1:5051/state -a baz:bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

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

(Updated June 29, 2016, 10:33 a.m.)


Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Adds again authorization for flags. Instead of being part of
`get_endpoints` it uses its own action `VIEW_TASKS` which is
used to restrict access to the `/flags` endpoint, as well as
to filter the results of the `/state` endpoint on both master
and agents.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto a6d93cd2cb9161a98565b22e50b06aac4931a671 
  include/mesos/authorizer/authorizer.proto 
fc76796022a6fa3d36a1447c476980868d42c2d0 
  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
  src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
  src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
  src/tests/master_authorization_tests.cpp 
9088d7df901ad9e0b3c43a3ea61882054f55ee93 
  src/tests/slave_authorization_tests.cpp 
78221e200d9b7880cc474f1acef92c5dec1c8e25 

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


Testing
---

- `make check`
- manual tests with browsers.
- Used the script:
 
```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "view_flags" : [
   {
 "principals" : { "values" : ["foo"] },
 "flags" : { "type" : "ANY" }
   },
   {
 "principals" : { "values" : ["foo"] },
 "flags" : { "type" : "NONE" }
   }
  ]
}
EOF

./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
 --authenticate_http \
 --log_dir=/tmp/mesos/logs/master \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

# Returns a 200 OK Response with the contents of the flags
# in JSON object
http GET http://127.0.0.1:5050/flags -a foo:bar
http GET http://127.0.0.1:5051/flags -a foo:bar

# Returned JSON contains a `flags` entry with all the flags.
http GET http://127.0.0.1:5050/state -a foo:bar
http GET http://127.0.0.1:5051/state -a foo:bar

# 403 Forbidden response
http GET http://127.0.0.1:5050/flags -a baz:bar
http GET http://127.0.0.1:5051/flags -a baz:bar

# Returned JSON doesn't include flags information.
http GET http://127.0.0.1:5050/state -a baz:bar
http GET http://127.0.0.1:5051/state -a baz:bar
```


Thanks,

Alexander Rojas

Review Request 49359: Fixed minor style issues.

[Alexander Rojas]

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

Review request for mesos, Adam B and Jie Yu.


Repository: mesos


Description
---

Adds a proper breakline before braces in method definition and a comment
which was not properly indented.


Diffs
-

  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 

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


Testing
---

`make check` (It is not a functional change)


Thanks,

Alexander Rojas

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

> On June 29, 2016, 7:17 a.m., Jie Yu wrote:
> > src/authorizer/local/authorizer.cpp, line 203
> > 
> >
> > Not yours. `{` should be in the next line.

Added the patch [r/49359/](https://reviews.apache.org/r/49359/) which fixes 
this issue (along with a wronly indented comment).


> On June 29, 2016, 7:17 a.m., Jie Yu wrote:
> > src/master/http.cpp, lines 2080-2082
> > 
> >
> > This looks like a flag too? If you do not want to treat that as a flag, 
> > add a note at least?
> 
> Adam B wrote:
> Yeah, we can document it. I think the cluster name should be generic 
> enough metadata that we can leave it open to any (authenticated) user. Other 
> endpoints like /version didn't seem worth protecting either. We're not even 
> trying to protect access to cluster resource information, let alone cluster 
> name.

I added a note.


- Alexander


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


On June 29, 2016, 10:33 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 10:33 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET http://127.0.0.1:5050/state -a baz:bar
> http GET http://127.0.0.1:5051/state -a baz:bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

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

(Updated June 29, 2016, 10:44 a.m.)


Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Adds again authorization for flags. Instead of being part of
`get_endpoints` it uses its own action `VIEW_TASKS` which is
used to restrict access to the `/flags` endpoint, as well as
to filter the results of the `/state` endpoint on both master
and agents.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto a6d93cd2cb9161a98565b22e50b06aac4931a671 
  include/mesos/authorizer/authorizer.proto 
fc76796022a6fa3d36a1447c476980868d42c2d0 
  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
  src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
  src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
  src/tests/master_authorization_tests.cpp 
9088d7df901ad9e0b3c43a3ea61882054f55ee93 
  src/tests/slave_authorization_tests.cpp 
78221e200d9b7880cc474f1acef92c5dec1c8e25 

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


Testing
---

- `make check`
- manual tests with browsers.
- Used the script:
 
```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "view_flags" : [
   {
 "principals" : { "values" : ["foo"] },
 "flags" : { "type" : "ANY" }
   },
   {
 "principals" : { "values" : ["foo"] },
 "flags" : { "type" : "NONE" }
   }
  ]
}
EOF

./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
 --authenticate_http \
 --log_dir=/tmp/mesos/logs/master \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

# Returns a 200 OK Response with the contents of the flags
# in JSON object
http GET http://127.0.0.1:5050/flags -a foo:bar
http GET http://127.0.0.1:5051/flags -a foo:bar

# Returned JSON contains a `flags` entry with all the flags.
http GET http://127.0.0.1:5050/state -a foo:bar
http GET http://127.0.0.1:5051/state -a foo:bar

# 403 Forbidden response
http GET http://127.0.0.1:5050/flags -a baz:bar
http GET http://127.0.0.1:5051/flags -a baz:bar

# Returned JSON doesn't include flags information.
http GET http://127.0.0.1:5050/state -a baz:bar
http GET http://127.0.0.1:5051/state -a baz:bar
```


Thanks,

Alexander Rojas

Re: Review Request 49245: Implement READ_FILE for agent operator API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49301, 49242, 49243, 49244, 49245]

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

- Mesos ReviewBot


On June 29, 2016, 2:06 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49245/
> ---
> 
> (Updated June 29, 2016, 2:06 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, haosdent huang, and Vinod Kone.
> 
> 
> Bugs: mesos-5515
> https://issues.apache.org/jira/browse/mesos-5515
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented readFile method for agent operator API.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 
>   src/tests/api_tests.cpp b3127b716480223a6f23b7908bf6bc1808120f80 
> 
> Diff: https://reviews.apache.org/r/49245/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 49351: Added wrapper function for health check in docker executor.

[haosdent huang]

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

(Updated June 29, 2016, 9:03 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Mahler, and 
Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Added wrapper function for health check in docker executor.


Diffs (updated)
-

  src/docker/executor.cpp 88b7fc4c36ed3974ac6b103a29e1d975619f0c69 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 36816: Supported HTTP/HTTPS in health check.

[haosdent huang]

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

(Updated June 29, 2016, 9:04 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Mahler, and 
Timothy Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported HTTP/HTTPS in health check.


Diffs (updated)
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 

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


Testing
---

* Add unit test cases: HealthCheckTest.HealthyTaskViaHttp and 
HealthCheckTest.HealthyTaskNotMatchHttpStatuses
make check


Thanks,

haosdent huang

Review Request 49360: Supported TCP check in health check.

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Timothy 
Chen.


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


Repository: mesos


Description
---

Supported TCP check in health check.


Diffs
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 

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


Testing
---


Thanks,

haosdent huang

Review Request 49363: Added support for VIEW_FLAGS authorization action in HTTP API.

[Alexander Rojas]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Adds a call to `authorizer::authorize()` where the HTTP API builds
the response for a query of the command-line flags given to start
the master.


Diffs
-

  src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 

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


Testing
---

`make check`


Thanks,

Alexander Rojas

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

> On June 29, 2016, 12:51 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1241-1262
> > 
> >
> > flags specific authorization should be done in `_flags()` so that the 
> > v1 API can get the benefit automatically. 
> > 
> > please move this logic inside `_flags()`. you might need to adjust its 
> > return type and add a `__flags()`.
> 
> Alexander Rojas wrote:
> The main problem with this suggestion is, `_flags` return type is 
> `JSON::Object`, which means it is supposed to be called synchronously by both 
> `flags()` and `getFlags()`.
> 
> If I were to make `_flags()` async, the return type would be 
> `Future` but then, how do I encode the fact that I must return 
> `Forbidden`? I could make `_flags()` return a `Future` but then it 
> would not be different from `flags()`, moreover `getFlags()` expects a 
> `JSON::Object` since it evolves it to a `v1::master::Response`, so it 
> shouldn't take a `Future`.
> 
> My solution would be to use the same code in `getFlags()` to perform 
> authorization.

I added a patch implementing the solution I described 
[r/49363/](https://reviews.apache.org/r/49363/)


- Alexander


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


On June 29, 2016, 10:44 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 10:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET http://127.0.0.1:5050/state -a baz:bar
> http GET http://127.0.0.1:5051/state -a baz:bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49321: GetState response protobuf.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [48835, 48841, 49136, 49321]

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

- Mesos ReviewBot


On June 28, 2016, 3:07 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49321/
> ---
> 
> (Updated June 28, 2016, 3:07 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, zhou xing, haosdent huang, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5489
> https://issues.apache.org/jira/browse/MESOS-5489
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> GetState response protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/master.proto 332261918a069f4e581bfc4d0fc14ce8c86c2617 
>   include/mesos/v1/master/master.proto 
> 681ea1e638777059a6bf792435cbe526bc252f7a 
> 
> Diff: https://reviews.apache.org/r/49321/diff/
> 
> 
> Testing
> ---
> 
> `make` on Mac
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Review Request 49368: Fixed indentation error for comment.

[Joerg Schad]

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

Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Repository: mesos


Description
---

Fixed indentation error for comment.


Diffs
-

  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 

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


Testing
---


Thanks,

Joerg Schad

Review Request 49369: Introduced authorization based filtering for /roles.

[Joerg Schad]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Previously only /weights was filtered but /roles actually
contains the same information and hence both endpoints
should be filtered in the same way.
As part of this work we renamed the GetWeight action to
ViewRole (and same for the acls).


Diffs
-

  include/mesos/authorizer/acls.proto a6d93cd2cb9161a98565b22e50b06aac4931a671 
  include/mesos/authorizer/authorizer.proto 
fc76796022a6fa3d36a1447c476980868d42c2d0 
  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
  src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
  src/master/weights_handler.cpp 0332d86570724e841f97348087808081b2b890af 
  src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
  src/tests/dynamic_weights_tests.cpp c67ed75a050b9db5575ac2bb6100bcf01cfc04ff 
  src/tests/master_authorization_tests.cpp 
81804e0522fd6b26155732af08e33c4d0bb0a8fe 

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


Testing
---

sudo make check


Thanks,

Joerg Schad

Review Request 49370: Updateted documentation for roles endpoint filtering.

[Joerg Schad]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

As part of this also added AUTHORIZATION help for
all endpoints supporting authorization based filtering.


Diffs
-

  CHANGELOG 7a44422e47a0453d2079070857c5537b63f4ae0a 
  docs/authorization.md 9bd6031d2a3aef921fa4e3f6683cc5c234832d47 
  src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 

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


Testing
---

Viewed docs via docker website container.


Thanks,

Joerg Schad

Review Request 49352: Added a flag parser for hashset.

[Guangya Liu]

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

Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added a flag parser for hashset.


Diffs
-

  src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 49181: Fixed a typo in hierarchical.hpp.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:13 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Fixed a typo in hierarchical.hpp.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
98a1f69f14b967c8d01f8a680771c9d28fac14e4 

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


Testing (updated)
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 48895: Added allocator_fairness_excluded_resource_names flag to master.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:14 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added allocator_fairness_excluded_resource_names flag to master.


Diffs (updated)
-

  src/master/flags.hpp 735367ff5412d611f4eae6cb63dd4829c4338002 
  src/master/flags.cpp b6b1603f02d3c6f861edad3de770ecb3fcad0057 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 49190: Enabled calculateShare() to ignore the fairnessExcludeResourceNames.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:16 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Enabled calculateShare() to ignore the fairnessExcludeResourceNames.


Diffs (updated)
-

  src/master/allocator/sorter/drf/sorter.hpp 
35273b5dcf39938125a112c5e56b5a8a542157db 
  src/master/allocator/sorter/drf/sorter.cpp 
27d56f274c41bbabe6f2abbbcebd2c4eff52693e 
  src/master/allocator/sorter/sorter.hpp 
68a2f56cef976203d83ed823486d1a9675f9ab68 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 48908: Added test case for exclude resources from sorter.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:16 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Added test case for exclude resources from sorter.


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


Repository: mesos


Description (updated)
---

Added test case for exclude resources from sorter.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
eb11ff6b1b7bce291f336a199e8700be0bdc61c1 

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


Testing
---

make
make check

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HierarchicalAllocatorTest
[ RUN  ] HierarchicalAllocatorTest.DRFWithExcludeResources
[   OK ] HierarchicalAllocatorTest.DRFWithExcludeResources (59 ms)
[--] 1 test from HierarchicalAllocatorTest (59 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (76 ms total)
[  PASSED  ] 1 test.

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HierarchicalAllocatorTest
[ RUN  ] HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources
[   OK ] HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources 
(129 ms)
[--] 1 test from HierarchicalAllocatorTest (129 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (147 ms total)
[  PASSED  ] 1 test.


Thanks,

Guangya Liu

Re: Review Request 48904: Updated allocator initialize() to include fairnessExcludeResourceNames.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:16 p.m.)


Review request for mesos and Benjamin Mahler.


Summary (updated)
-

Updated allocator initialize() to include fairnessExcludeResourceNames.


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


Repository: mesos


Description (updated)
---

Updated allocator initialize() to include fairnessExcludeResourceNames.


Diffs (updated)
-

  include/mesos/allocator/allocator.hpp 
98025bc182d92098bd4018177a6ae28135d8de95 
  src/master/allocator/mesos/allocator.hpp 
f096d2b7813580fb28e77a2803e290edf1ebda31 
  src/master/allocator/mesos/hierarchical.hpp 
98a1f69f14b967c8d01f8a680771c9d28fac14e4 
  src/master/allocator/mesos/hierarchical.cpp 
c3639342335499a04a23147a4205f1b475c123fa 
  src/master/master.cpp 907233b015919f437fb2ebd25875217930b301b4 
  src/tests/allocator.hpp 41a31ae5d2c8fb8eb902a82d893be570db0da3bd 
  src/tests/api_tests.cpp b3127b716480223a6f23b7908bf6bc1808120f80 
  src/tests/master_allocator_tests.cpp 7910f5532bf36ed92100839eac6c6f6a18838ffa 
  src/tests/master_quota_tests.cpp b9bc49b1ef55d6ea57d94971905d70244f982e9f 
  src/tests/persistent_volume_endpoints_tests.cpp 
6c85e19eeaa69bf3a4e3077261331191db6eec06 
  src/tests/reservation_endpoints_tests.cpp 
3ee59d5db0089dd59acfe48a77910d069ffc377b 
  src/tests/reservation_tests.cpp d7e90bc67a55a909be70691a1108493c33743b02 
  src/tests/resource_offers_tests.cpp 046adaedf9121655f377f503bb30437803bf0005 
  src/tests/slave_recovery_tests.cpp e6e7b8e3d71886eb8749122bd7b441857983d574 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 48907: Enabled allocator transfer the fairnessExcludeResourceNames to sorter.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:16 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Enabled allocator transfer the fairnessExcludeResourceNames to sorter.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.cpp 
c3639342335499a04a23147a4205f1b475c123fa 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 48908: Added test case for exclude resources from sorter.

[Guangya Liu]

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

(Updated 六月 29, 2016, 12:29 p.m.)


Review request for mesos and Benjamin Mahler.


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


Repository: mesos


Description
---

Added test case for exclude resources from sorter.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
eb11ff6b1b7bce291f336a199e8700be0bdc61c1 

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


Testing
---

make
make check

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HierarchicalAllocatorTest
[ RUN  ] HierarchicalAllocatorTest.DRFWithExcludeResources
[   OK ] HierarchicalAllocatorTest.DRFWithExcludeResources (59 ms)
[--] 1 test from HierarchicalAllocatorTest (59 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (76 ms total)
[  PASSED  ] 1 test.

[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from HierarchicalAllocatorTest
[ RUN  ] HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources
[   OK ] HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources 
(129 ms)
[--] 1 test from HierarchicalAllocatorTest (129 ms total)

[--] Global test environment tear-down
[==] 1 test from 1 test case ran. (147 ms total)
[  PASSED  ] 1 test.


Thanks,

Guangya Liu

Re: Review Request 49208: Added tests to check if appc spec is properly parsed.

[Guangya Liu]

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




src/tests/containerizer/appc_spec_tests.cpp (line 30)


kill this line


- Guangya Liu


On 六月 28, 2016, 10:13 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49208/
> ---
> 
> (Updated 六月 28, 2016, 10:13 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added tests to check if appc spec is properly parsed.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/tests/containerizer/appc_spec_tests.cpp PRE-CREATION 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 061f80c62319817b22a5c1880a4858fdafbfb72a 
> 
> Diff: https://reviews.apache.org/r/49208/diff/
> 
> 
> Testing
> ---
> 
> Make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49319: Fixed incorrect comment on ACCESS_SANDBOX in authorizer.proto.

[Joerg Schad]

> On June 28, 2016, 2:50 p.m., Alexander Rojas wrote:
> > include/mesos/authorizer/authorizer.proto, lines 90-91
> > 
> >
> > This comment is rather incomplete:
> > 
> > 1. Either they are both set or none.
> > 2. When none are set it is because the task has not yet created the 
> > directory (because is scheduled) or it is finished and there was a lag 
> > between access and destruction of the enpoint.
> > 
> > All in all, we should mention that having none set is an exceptional 
> > case, and could be treated by returning an error.

1. Are you sure from the setting code that 1. is true?
2. I am not sure this needs to mentioned here as this is just describes the 
semantic, and a module writer has to deal with that no matter how frequent it 
is.

We can surely mention this is an exceptional case, but I would answer similarly 
as for 2.


- Joerg


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


On June 28, 2016, 2:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49319/
> ---
> 
> (Updated June 28, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5730
> https://issues.apache.org/jira/browse/MESOS-5730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current semantic is that these fields might not be set.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
> 
> Diff: https://reviews.apache.org/r/49319/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49219: Added runtime isolator interface to run appc containers.

[Guangya Liu]

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




src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 51)


Please keep two lines for each function.

Seems you uploaded this patch by mistake? I saw that you marked all 
comments as fixed but the patch was not updated based on comments?



src/slave/containerizer/mesos/isolators/appc/runtime.cpp (line 77)


1) s/appc/AppC
2) Add a period to the end


- Guangya Liu


On 六月 29, 2016, 12:46 a.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49219/
> ---
> 
> (Updated 六月 29, 2016, 12:46 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-4778
> https://issues.apache.org/jira/browse/MESOS-4778
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added runtime isolator interface to run appc containers.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 9c80a613818302ad3b417d582e9a91a59a6f666d 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/slave/containerizer/mesos/isolators/appc/runtime.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/appc/runtime.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49219/diff/
> 
> 
> Testing
> ---
> 
> Make Check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 49251: Added missing AUTHORIZATION endpoint documentation.

[Neil Conway]

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




src/master/http.cpp (line 1827)


"based on" -- here and below.



src/master/http.cpp (line 2615)


"to teardown frameworks"


- Neil Conway


On June 27, 2016, 10:37 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49251/
> ---
> 
> (Updated June 27, 2016, 10:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Neil Conway.
> 
> 
> Bugs: MESOS-5711
> https://issues.apache.org/jira/browse/MESOS-5711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added endpoint documentation for the following endpoints:
> * Master::Http::CREATE_VOLUMES_HELP
> * Master::Http::DESTROY_VOLUMES_HELP
> * Master::Http::RESERVE_HELP
> * Master::Http::STATE_HELP
> * Master::Http::STATESUMMARY_HELP
> * Master::Http::TEARDOWN_HELP
> * Master::Http::TASKS_HELP
> * Master::Http::UNRESERVE_HELP
> * Slave::Http::STATE_HELP
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp d55aa05c76bb2b1fb17b795510fd50c021cdb995 
> 
> Diff: https://reviews.apache.org/r/49251/diff/
> 
> 
> Testing
> ---
> 
> make check + viewed generated documentation (see next review)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49323: Added tests that combine the two ways of creating volumes.

[Neil Conway]

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

(Updated June 29, 2016, 1:40 p.m.)


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


Changes
---

Add a second test.


Summary (updated)
-

Added tests that combine the two ways of creating volumes.


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


Repository: mesos


Description (updated)
---

These tests check that dynamic reservations and persistent volumes can
be created via the offer API and then removed via the HTTP endpoints,
and vice versa.


Diffs (updated)
-

  src/tests/persistent_volume_endpoints_tests.cpp 
6c85e19eeaa69bf3a4e3077261331191db6eec06 

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


Testing
---

make check

This test passes without any Mesos changes. i.e., it doesn't reproduce the bug 
by itself, but having more test coverage for this scenario seems wise.


Thanks,

Neil Conway

Review Request 49375: Simplified DRFSorter to not track per-slave total resources.

[Neil Conway]

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

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


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


Repository: mesos


Description
---

The DRFSorter previously kept the total resources at each slave, along
with the total quantity of resources in the cluster. The latter figure
is what most of the clients of the sorter care about. In the few places
where the previous coding was using the per-slave total resources, it is
relatively easy to adjust the code to remove this dependency.

As part of this change, remove `total()` and `update(const SlaveID&
slaveId, const Resources& resources)` from the Sorter interface. The
former was unused; for the latter, code that used it can instead be
rewritten to adjust the total resources in the cluster by first removing
the previous resources at a slave and then adding the new resources.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c3639342335499a04a23147a4205f1b475c123fa 
  src/master/allocator/sorter/drf/sorter.hpp 
35273b5dcf39938125a112c5e56b5a8a542157db 
  src/master/allocator/sorter/drf/sorter.cpp 
27d56f274c41bbabe6f2abbbcebd2c4eff52693e 
  src/master/allocator/sorter/sorter.hpp 
68a2f56cef976203d83ed823486d1a9675f9ab68 
  src/tests/sorter_tests.cpp 6fc58829892dc0223140f1b47593a3e5853cace5 

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


Testing
---

make check on OSX and recent Arch Linux.


Thanks,

Neil Conway

Review Request 49376: Added assertions to DRFSorter.

[Neil Conway]

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

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


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


Repository: mesos


Description
---

Added assertions to DRFSorter.


Diffs
-

  src/master/allocator/sorter/drf/sorter.cpp 
27d56f274c41bbabe6f2abbbcebd2c4eff52693e 

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


Testing
---

These assertions DO NOT PASS. They are conceptually correct, however -- after 
r/49377 they pass on `make check`.


Thanks,

Neil Conway

Re: Review Request 49326: Fixed typo in authorization test description.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On June 29, 2016, 1:41 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49326/
> ---
> 
> (Updated June 29, 2016, 1:41 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo in authorization test description.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> 81804e0522fd6b26155732af08e33c4d0bb0a8fe 
> 
> Diff: https://reviews.apache.org/r/49326/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49368: Fixed indentation error for comment.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On June 29, 2016, 11:37 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49368/
> ---
> 
> (Updated June 29, 2016, 11:37 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed indentation error for comment.
> 
> 
> Diffs
> -
> 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
> 
> Diff: https://reviews.apache.org/r/49368/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 49377: Fixed allocator to update total resources in quota sorter.

[Neil Conway]

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

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


Repository: mesos


Description
---

Each DRFSorter tracks the total resources in the cluster. This means
that each sorter must be updated when the resources in the cluster have
changed, e.g., due to the creation of a dynamic reservation or a
persistent volume. In the previous implementation, the quota role sorter
was not updated for non-quota roles when a reservation or persistent
volume was created by a framework. This resulted in inconsistency
between the total resources in the allocator and the quota role sorter.

This could cause several problems. First, removing a slave from the
cluster would leak resources in the quota role sorter. Second, certain
interleavings of slave removals and reserve/unreserve operations by
frameworks and via HTTP endpoints could lead to CHECK failures.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
c3639342335499a04a23147a4205f1b475c123fa 
  src/master/allocator/sorter/drf/sorter.cpp 
27d56f274c41bbabe6f2abbbcebd2c4eff52693e 
  src/tests/persistent_volume_endpoints_tests.cpp 
6c85e19eeaa69bf3a4e3077261331191db6eec06 

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


Testing
---

make check on OSX and recent Arch Linux.


Thanks,

Neil Conway

Re: Review Request 49251: Added missing AUTHORIZATION endpoint documentation.

[Joerg Schad]

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

(Updated June 29, 2016, 1:59 p.m.)


Review request for mesos, Alexander Rukletsov and Neil Conway.


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


Repository: mesos


Description
---

Added endpoint documentation for the following endpoints:
* Master::Http::CREATE_VOLUMES_HELP
* Master::Http::DESTROY_VOLUMES_HELP
* Master::Http::RESERVE_HELP
* Master::Http::STATE_HELP
* Master::Http::STATESUMMARY_HELP
* Master::Http::TEARDOWN_HELP
* Master::Http::TASKS_HELP
* Master::Http::UNRESERVE_HELP
* Slave::Http::STATE_HELP


Diffs (updated)
-

  src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 

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


Testing
---

make check + viewed generated documentation (see next review)


Thanks,

Joerg Schad

Re: Review Request 49250: Regenerated endpoint documention.

[Joerg Schad]

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

(Updated June 29, 2016, 2 p.m.)


Review request for mesos, Alexander Rukletsov and Neil Conway.


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


Repository: mesos


Description
---

Note that regenerating the endpoint documentation
also picked up a stale change in slave/api/vi.md.


Diffs (updated)
-

  docs/endpoints/master/create-volumes.md 
f6ec9384f7f2bfb8056e54532ac3417a1f15b1d1 
  docs/endpoints/master/destroy-volumes.md 
054e816371f26c91ccc0d9687f377e80641e42a8 
  docs/endpoints/master/frameworks.md 1aed723152528be0e19d79520daac5f470cb49da 
  docs/endpoints/master/reserve.md 9bb04ed0de5b9304301fffd51b39ec07102825cd 
  docs/endpoints/master/state-summary.md 
4eb517ea0f6b5dbc2e7d7c406d54e741cf50e0e5 
  docs/endpoints/master/state.json.md 8989a8e97941178575edbebc75c6c174a1b0e5d3 
  docs/endpoints/master/state.md 9bb753ade7ef73109c7705b2bae270dd9b1bf99f 
  docs/endpoints/master/tasks.json.md 5d2c0e6eb53fa5e04a973eb786cd16bfad38736f 
  docs/endpoints/master/tasks.md c7df686443e20d255b03fc31a999b9b24b5f3a0a 
  docs/endpoints/master/teardown.md 4c14f594c3713304d5adf59e56e9e8af2b05ebee 
  docs/endpoints/master/unreserve.md 5cce4288231a64d7ee7713f2b44487618e7cc0a7 
  docs/endpoints/slave/api/v1.md c867d795104e053e5aae819df3deea68269228bf 
  docs/endpoints/slave/state.json.md ac8536986ceb6532f71b4fe1c62b1b83390b03e7 
  docs/endpoints/slave/state.md c7b61d736582f50b850fed252e558bbae6dad801 

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


Testing
---

viewed generated documentation via website docker container.


Thanks,

Joerg Schad

Re: Review Request 49251: Added missing AUTHORIZATION endpoint documentation.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On June 29, 2016, 1:59 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49251/
> ---
> 
> (Updated June 29, 2016, 1:59 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Neil Conway.
> 
> 
> Bugs: MESOS-5711
> https://issues.apache.org/jira/browse/MESOS-5711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added endpoint documentation for the following endpoints:
> * Master::Http::CREATE_VOLUMES_HELP
> * Master::Http::DESTROY_VOLUMES_HELP
> * Master::Http::RESERVE_HELP
> * Master::Http::STATE_HELP
> * Master::Http::STATESUMMARY_HELP
> * Master::Http::TEARDOWN_HELP
> * Master::Http::TASKS_HELP
> * Master::Http::UNRESERVE_HELP
> * Slave::Http::STATE_HELP
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
> 
> Diff: https://reviews.apache.org/r/49251/diff/
> 
> 
> Testing
> ---
> 
> make check + viewed generated documentation (see next review)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49319: Fixed incorrect comment on ACCESS_SANDBOX in authorizer.proto.

[Alexander Rojas]

> On June 28, 2016, 4:50 p.m., Alexander Rojas wrote:
> > include/mesos/authorizer/authorizer.proto, lines 90-91
> > 
> >
> > This comment is rather incomplete:
> > 
> > 1. Either they are both set or none.
> > 2. When none are set it is because the task has not yet created the 
> > directory (because is scheduled) or it is finished and there was a lag 
> > between access and destruction of the enpoint.
> > 
> > All in all, we should mention that having none set is an exceptional 
> > case, and could be treated by returning an error.
> 
> Joerg Schad wrote:
> 1. Are you sure from the setting code that 1. is true?
> 2. I am not sure this needs to mentioned here as this is just describes 
> the semantic, and a module writer has to deal with that no matter how 
> frequent it is.
> 
> We can surely mention this is an exceptional case, but I would answer 
> similarly as for 2.

1. Yes, It is true!
2. if there is no object to authorize too, how do a module writer has to react 
to it? This is an exceptional case and needs to be mention that it does occur


- Alexander


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


On June 28, 2016, 4:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49319/
> ---
> 
> (Updated June 28, 2016, 4:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5730
> https://issues.apache.org/jira/browse/MESOS-5730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current semantic is that these fields might not be set.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
> 
> Diff: https://reviews.apache.org/r/49319/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49319: Fixed incorrect comment on ACCESS_SANDBOX in authorizer.proto.

[Joerg Schad]

> On June 28, 2016, 2:50 p.m., Alexander Rojas wrote:
> > include/mesos/authorizer/authorizer.proto, lines 90-91
> > 
> >
> > This comment is rather incomplete:
> > 
> > 1. Either they are both set or none.
> > 2. When none are set it is because the task has not yet created the 
> > directory (because is scheduled) or it is finished and there was a lag 
> > between access and destruction of the enpoint.
> > 
> > All in all, we should mention that having none set is an exceptional 
> > case, and could be treated by returning an error.
> 
> Joerg Schad wrote:
> 1. Are you sure from the setting code that 1. is true?
> 2. I am not sure this needs to mentioned here as this is just describes 
> the semantic, and a module writer has to deal with that no matter how 
> frequent it is.
> 
> We can surely mention this is an exceptional case, but I would answer 
> similarly as for 2.
> 
> Alexander Rojas wrote:
> 1. Yes, It is true!
> 2. if there is no object to authorize too, how do a module writer has to 
> react to it? This is an exceptional case and needs to be mention that it does 
> occur

How about:
// This action will have an object which might either have both a 
`FrameworkInfo`,
// and `ExecutorInfo`, or in exceptional cases nothing set.


- Joerg


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


On June 28, 2016, 2:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49319/
> ---
> 
> (Updated June 28, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5730
> https://issues.apache.org/jira/browse/MESOS-5730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current semantic is that these fields might not be set.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
> 
> Diff: https://reviews.apache.org/r/49319/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49319: Fixed incorrect comment on ACCESS_SANDBOX in authorizer.proto.

[Joerg Schad]

> On June 28, 2016, 2:50 p.m., Alexander Rojas wrote:
> > include/mesos/authorizer/authorizer.proto, lines 90-91
> > 
> >
> > This comment is rather incomplete:
> > 
> > 1. Either they are both set or none.
> > 2. When none are set it is because the task has not yet created the 
> > directory (because is scheduled) or it is finished and there was a lag 
> > between access and destruction of the enpoint.
> > 
> > All in all, we should mention that having none set is an exceptional 
> > case, and could be treated by returning an error.
> 
> Joerg Schad wrote:
> 1. Are you sure from the setting code that 1. is true?
> 2. I am not sure this needs to mentioned here as this is just describes 
> the semantic, and a module writer has to deal with that no matter how 
> frequent it is.
> 
> We can surely mention this is an exceptional case, but I would answer 
> similarly as for 2.
> 
> Alexander Rojas wrote:
> 1. Yes, It is true!
> 2. if there is no object to authorize too, how do a module writer has to 
> react to it? This is an exceptional case and needs to be mention that it does 
> occur
> 
> Joerg Schad wrote:
> How about:
> // This action will have an object which might either have both a 
> `FrameworkInfo`,
> // and `ExecutorInfo`, or in exceptional cases nothing set.

Btw Maybe we should later add a check which verfies this invariant...


- Joerg


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


On June 28, 2016, 2:35 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49319/
> ---
> 
> (Updated June 28, 2016, 2:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Alexander Rojas.
> 
> 
> Bugs: MESOS-5730
> https://issues.apache.org/jira/browse/MESOS-5730
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current semantic is that these fields might not be set.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
> 
> Diff: https://reviews.apache.org/r/49319/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49201: Added validation for the `get_endpoints` ACL.

[Alexander Rojas]

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

(Updated June 29, 2016, 5:31 p.m.)


Review request for mesos, Adam B, Jan Schlicht, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

The fact that not all endpoints can be secure through ACLs, yet
the ACL is called `get_endpoints`, can be confusing for operators.
Therefore, if an operator tries to launch an agent/master with an
invalid configuration an error is generated.


Diffs (updated)
-

  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
  src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 
  src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
  src/tests/scheduler_driver_tests.cpp 217185780e3576faf633dd9629ae93a275fac284 

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


Testing
---

`make check` and following scripts:

```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "get_endpoints" : [
   {
 "principals" : { "values" : ["foo"] },
 "paths" : { "values" : ["/frameworks"] }
   }
  ]
}
EOF

# Fails to start up with a message saying that `/frameworks`
# ins't supported.
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

```

and


```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "get_endpoints" : [
   {
 "principals" : { "values" : ["foo"] },
 "paths" : { "values" : ["/monitor/statistics", "/monitor/statistics.json", 
"/containers"] }
   }
  ]
}
EOF

./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
 --authenticate_http \
 --log_dir=/tmp/mesos/logs/master \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

# Following requests succeed (200 OK response)
http http://localhost:5051/monitor/statistics -a foo:bar
http http://localhost:5051/monitor/statistics.json -a foo:bar
http http://localhost:5051/monitor/containers -a foo:bar

# Following requests fail (403 forbidden response)
http http://localhost:5051/monitor/statistics -a baz:bar
http http://localhost:5051/monitor/statistics.json -a baz:bar
http http://localhost:5051/monitor/containers -a baz:bar

```


Thanks,

Alexander Rojas

Re: Review Request 49201: Added validation for the `get_endpoints` ACL.

[Alexander Rojas]

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




src/common/http.cpp (lines 612 - 615)


This line cause a test which set the path as `/metrics//snapshot`.


- Alexander Rojas


On June 29, 2016, 5:31 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49201/
> ---
> 
> (Updated June 29, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Jan Schlicht, and Till Toenshoff.
> 
> 
> Bugs: MESOS-5707
> https://issues.apache.org/jira/browse/MESOS-5707
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The fact that not all endpoints can be secure through ACLs, yet
> the ACL is called `get_endpoints`, can be confusing for operators.
> Therefore, if an operator tries to launch an agent/master with an
> invalid configuration an error is generated.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/scheduler_driver_tests.cpp 
> 217185780e3576faf633dd9629ae93a275fac284 
> 
> Diff: https://reviews.apache.org/r/49201/diff/
> 
> 
> Testing
> ---
> 
> `make check` and following scripts:
> 
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "get_endpoints" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "paths" : { "values" : ["/frameworks"] }
>}
>   ]
> }
> EOF
> 
> # Fails to start up with a message saying that `/frameworks`
> # ins't supported.
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> ```
> 
> and
> 
> 
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "get_endpoints" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "paths" : { "values" : ["/monitor/statistics", 
> "/monitor/statistics.json", "/containers"] }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Following requests succeed (200 OK response)
> http http://localhost:5051/monitor/statistics -a foo:bar
> http http://localhost:5051/monitor/statistics.json -a foo:bar
> http http://localhost:5051/monitor/containers -a foo:bar
> 
> # Following requests fail (403 forbidden response)
> http http://localhost:5051/monitor/statistics -a baz:bar
> http http://localhost:5051/monitor/statistics.json -a baz:bar
> http http://localhost:5051/monitor/containers -a baz:bar
> 
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49201: Added validation for the `get_endpoints` ACL.

[Alexander Rojas]

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

(Updated June 29, 2016, 5:33 p.m.)


Review request for mesos, Adam B, Jan Schlicht, and Till Toenshoff.


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


Repository: mesos


Description (updated)
---

The fact that not all endpoints can be secure through ACLs, yet
the ACL is called `get_endpoints`, can be confusing for operators.
Therefore, if an operator tries to launch an agent/master with an
invalid configuration an error is generated.


Diffs
-

  src/authorizer/local/authorizer.cpp 3fade4168face1cb80b30c9b69b31d9eb4126222 
  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
  src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
  src/slave/slave.hpp 2afd7d152dcd2f5390014cd7bd4e926b62c292d1 
  src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
  src/tests/scheduler_driver_tests.cpp 217185780e3576faf633dd9629ae93a275fac284 

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


Testing
---

`make check` and following scripts:

```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "get_endpoints" : [
   {
 "principals" : { "values" : ["foo"] },
 "paths" : { "values" : ["/frameworks"] }
   }
  ]
}
EOF

# Fails to start up with a message saying that `/frameworks`
# ins't supported.
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

```

and


```sh
#! /usr/bin/env bash

rm -rf /tmp/mesos/*

cat < /tmp/credentials.txt
foo bar
baz bar
EOF

cat < /tmp/acls.json
{
  "permissive": false,
  "get_endpoints" : [
   {
 "principals" : { "values" : ["foo"] },
 "paths" : { "values" : ["/monitor/statistics", "/monitor/statistics.json", 
"/containers"] }
   }
  ]
}
EOF

./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
 --authenticate_http \
 --log_dir=/tmp/mesos/logs/master \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &
./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
 --master=127.0.0.1:5050 \
 --authenticate_http \
 --http_credentials=file:///tmp/credentials.txt \
 --acls=file:///tmp/acls.json &

# Following requests succeed (200 OK response)
http http://localhost:5051/monitor/statistics -a foo:bar
http http://localhost:5051/monitor/statistics.json -a foo:bar
http http://localhost:5051/monitor/containers -a foo:bar

# Following requests fail (403 forbidden response)
http http://localhost:5051/monitor/statistics -a baz:bar
http http://localhost:5051/monitor/statistics.json -a baz:bar
http http://localhost:5051/monitor/containers -a baz:bar

```


Thanks,

Alexander Rojas

Review Request 49381: WIP: Benchmark for Resources class.

[Klaus Ma]

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

Review request for mesos, Benjamin Mahler and Michael Park.


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


Repository: mesos


Description
---

WIP: Benchmark for Resources class.


Diffs
-

  src/tests/resources_tests.cpp dc12bd8f1e2da6972bc8aed598811c55d664036e 

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


Testing
---


Thanks,

Klaus Ma

Re: Review Request 48581: Removed duplicated code in 'strings::tokenize()'.

[Klaus Ma]

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

(Updated June 30, 2016, 12:57 a.m.)


Review request for mesos, Benjamin Mahler and Joris Van Remoortere.


Changes
---

Add Joris as reviewers


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


Repository: mesos


Description
---

Removed duplicated code in 'strings::tokenize()'.


Diffs
-

  3rdparty/stout/include/stout/strings.hpp 
162bdfb6c4f5a6b108761ebccd9b77e672f6dd87 

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


Testing
---

make && make check


Thanks,

Klaus Ma

Re: Review Request 49352: Added a flag parser for hashset.

[Klaus Ma]

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




src/common/parse.hpp (lines 150 - 152)


Call `insert` directly should be fine. `set` will help to avoid duplicated 
item.


- Klaus Ma


On June 29, 2016, 8:12 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49352/
> ---
> 
> (Updated June 29, 2016, 8:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5743
> https://issues.apache.org/jira/browse/MESOS-5743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag parser for hashset.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
> 
> Diff: https://reviews.apache.org/r/49352/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 48895: Added allocator_fairness_excluded_resource_names flag to master.

[Klaus Ma]

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




src/master/flags.cpp (line 437)


s/client/framework

Mesos user does not know client.


- Klaus Ma


On June 29, 2016, 8:14 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48895/
> ---
> 
> (Updated June 29, 2016, 8:14 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5620
> https://issues.apache.org/jira/browse/MESOS-5620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allocator_fairness_excluded_resource_names flag to master.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp 735367ff5412d611f4eae6cb63dd4829c4338002 
>   src/master/flags.cpp b6b1603f02d3c6f861edad3de770ecb3fcad0057 
> 
> Diff: https://reviews.apache.org/r/48895/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49363: Added support for VIEW_FLAGS authorization action in HTTP API.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49313, 49363]

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

- Mesos ReviewBot


On June 29, 2016, 10:22 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49363/
> ---
> 
> (Updated June 29, 2016, 10:22 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a call to `authorizer::authorize()` where the HTTP API builds
> the response for a query of the command-line flags given to start
> the master.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
> 
> Diff: https://reviews.apache.org/r/49363/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 48904: Updated allocator initialize() to include fairnessExcludeResourceNames.

[Klaus Ma]

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




src/master/master.cpp (line 830)


Please check whether the resource name is valid. We only support cpus, mem, 
disk, ports, gpus for now.


- Klaus Ma


On June 29, 2016, 8:16 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48904/
> ---
> 
> (Updated June 29, 2016, 8:16 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5620
> https://issues.apache.org/jira/browse/MESOS-5620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator initialize() to include fairnessExcludeResourceNames.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 98025bc182d92098bd4018177a6ae28135d8de95 
>   src/master/allocator/mesos/allocator.hpp 
> f096d2b7813580fb28e77a2803e290edf1ebda31 
>   src/master/allocator/mesos/hierarchical.hpp 
> 98a1f69f14b967c8d01f8a680771c9d28fac14e4 
>   src/master/allocator/mesos/hierarchical.cpp 
> c3639342335499a04a23147a4205f1b475c123fa 
>   src/master/master.cpp 907233b015919f437fb2ebd25875217930b301b4 
>   src/tests/allocator.hpp 41a31ae5d2c8fb8eb902a82d893be570db0da3bd 
>   src/tests/api_tests.cpp b3127b716480223a6f23b7908bf6bc1808120f80 
>   src/tests/master_allocator_tests.cpp 
> 7910f5532bf36ed92100839eac6c6f6a18838ffa 
>   src/tests/master_quota_tests.cpp b9bc49b1ef55d6ea57d94971905d70244f982e9f 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6c85e19eeaa69bf3a4e3077261331191db6eec06 
>   src/tests/reservation_endpoints_tests.cpp 
> 3ee59d5db0089dd59acfe48a77910d069ffc377b 
>   src/tests/reservation_tests.cpp d7e90bc67a55a909be70691a1108493c33743b02 
>   src/tests/resource_offers_tests.cpp 
> 046adaedf9121655f377f503bb30437803bf0005 
>   src/tests/slave_recovery_tests.cpp e6e7b8e3d71886eb8749122bd7b441857983d574 
> 
> Diff: https://reviews.apache.org/r/48904/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49360: Supported TCP check in health check.

[haosdent huang]

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

(Updated June 29, 2016, 5:24 p.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Mahler, and Timothy 
Chen.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Supported TCP check in health check.


Diffs (updated)
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/health-check/main.cpp 4cc9dde8927acd4594477476f1b1bdc43963d789 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Vinod Kone]

> On June 28, 2016, 10:51 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1241-1262
> > 
> >
> > flags specific authorization should be done in `_flags()` so that the 
> > v1 API can get the benefit automatically. 
> > 
> > please move this logic inside `_flags()`. you might need to adjust its 
> > return type and add a `__flags()`.
> 
> Alexander Rojas wrote:
> The main problem with this suggestion is, `_flags` return type is 
> `JSON::Object`, which means it is supposed to be called synchronously by both 
> `flags()` and `getFlags()`.
> 
> If I were to make `_flags()` async, the return type would be 
> `Future` but then, how do I encode the fact that I must return 
> `Forbidden`? I could make `_flags()` return a `Future` but then it 
> would not be different from `flags()`, moreover `getFlags()` expects a 
> `JSON::Object` since it evolves it to a `v1::master::Response`, so it 
> shouldn't take a `Future`.
> 
> My solution would be to use the same code in `getFlags()` to perform 
> authorization.
> 
> Alexander Rojas wrote:
> I added a patch implementing the solution I described 
> [r/49363/](https://reviews.apache.org/r/49363/)

`_flags()` returning `Future` is fine. `flags()` still needs to deal 
with the REST specific things like METHOD check and jsonp. I prefer that than 
duplicating authz code in `flags()` and `getFlags()`.


- Vinod


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


On June 29, 2016, 8:44 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 8:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET 

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Vinod Kone]

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




src/common/http.hpp (line 138)


another blank line.


- Vinod Kone


On June 29, 2016, 8:44 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 8:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.0.0.1:5050/flags -a foo:bar
> http GET http://127.0.0.1:5051/flags -a foo:bar
> 
> # Returned JSON contains a `flags` entry with all the flags.
> http GET http://127.0.0.1:5050/state -a foo:bar
> http GET http://127.0.0.1:5051/state -a foo:bar
> 
> # 403 Forbidden response
> http GET http://127.0.0.1:5050/flags -a baz:bar
> http GET http://127.0.0.1:5051/flags -a baz:bar
> 
> # Returned JSON doesn't include flags information.
> http GET http://127.0.0.1:5050/state -a baz:bar
> http GET http://127.0.0.1:5051/state -a baz:bar
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49370: Updateted documentation for roles endpoint filtering.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49369, 49370]

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

- Mesos ReviewBot


On June 29, 2016, 11:39 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49370/
> ---
> 
> (Updated June 29, 2016, 11:39 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5709
> https://issues.apache.org/jira/browse/MESOS-5709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As part of this also added AUTHORIZATION help for
> all endpoints supporting authorization based filtering.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 7a44422e47a0453d2079070857c5537b63f4ae0a 
>   docs/authorization.md 9bd6031d2a3aef921fa4e3f6683cc5c234832d47 
>   src/master/http.cpp e089fe960779f39b3321f2ec81ab2acc17d53641 
> 
> Diff: https://reviews.apache.org/r/49370/diff/
> 
> 
> Testing
> ---
> 
> Viewed docs via docker website container.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Alexander Rojas]

> On June 29, 2016, 12:51 a.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1241-1262
> > 
> >
> > flags specific authorization should be done in `_flags()` so that the 
> > v1 API can get the benefit automatically. 
> > 
> > please move this logic inside `_flags()`. you might need to adjust its 
> > return type and add a `__flags()`.
> 
> Alexander Rojas wrote:
> The main problem with this suggestion is, `_flags` return type is 
> `JSON::Object`, which means it is supposed to be called synchronously by both 
> `flags()` and `getFlags()`.
> 
> If I were to make `_flags()` async, the return type would be 
> `Future` but then, how do I encode the fact that I must return 
> `Forbidden`? I could make `_flags()` return a `Future` but then it 
> would not be different from `flags()`, moreover `getFlags()` expects a 
> `JSON::Object` since it evolves it to a `v1::master::Response`, so it 
> shouldn't take a `Future`.
> 
> My solution would be to use the same code in `getFlags()` to perform 
> authorization.
> 
> Alexander Rojas wrote:
> I added a patch implementing the solution I described 
> [r/49363/](https://reviews.apache.org/r/49363/)
> 
> Vinod Kone wrote:
> `_flags()` returning `Future` is fine. `flags()` still needs to 
> deal with the REST specific things like METHOD check and jsonp. I prefer that 
> than duplicating authz code in `flags()` and `getFlags()`.

my point was, the returned response in both cases, `flags()` and `getFlags()` 
is different, in the first case you return `OK(_flags(), 
request.url.query.get("jsonp"));`, in the second case you return 
`OK(serialize(contentType, evolve(_flags())), 
stringify(contentType));`. 

If we go with `_flags()` returning a future response, which one should that one 
be? the first response or the second one?


- Alexander


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


On June 29, 2016, 10:44 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 10:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  --authenticate_http \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> 
> # Returns a 200 OK Response with the contents of the flags
> # in JSON object
> http GET http://127.

Re: Review Request 49313: Added the VIEW_FLAGS authorization action.

[Vinod Kone]

> On June 28, 2016, 10:51 p.m., Vinod Kone wrote:
> > src/master/http.cpp, lines 1241-1262
> > 
> >
> > flags specific authorization should be done in `_flags()` so that the 
> > v1 API can get the benefit automatically. 
> > 
> > please move this logic inside `_flags()`. you might need to adjust its 
> > return type and add a `__flags()`.
> 
> Alexander Rojas wrote:
> The main problem with this suggestion is, `_flags` return type is 
> `JSON::Object`, which means it is supposed to be called synchronously by both 
> `flags()` and `getFlags()`.
> 
> If I were to make `_flags()` async, the return type would be 
> `Future` but then, how do I encode the fact that I must return 
> `Forbidden`? I could make `_flags()` return a `Future` but then it 
> would not be different from `flags()`, moreover `getFlags()` expects a 
> `JSON::Object` since it evolves it to a `v1::master::Response`, so it 
> shouldn't take a `Future`.
> 
> My solution would be to use the same code in `getFlags()` to perform 
> authorization.
> 
> Alexander Rojas wrote:
> I added a patch implementing the solution I described 
> [r/49363/](https://reviews.apache.org/r/49363/)
> 
> Vinod Kone wrote:
> `_flags()` returning `Future` is fine. `flags()` still needs to 
> deal with the REST specific things like METHOD check and jsonp. I prefer that 
> than duplicating authz code in `flags()` and `getFlags()`.
> 
> Alexander Rojas wrote:
> my point was, the returned response in both cases, `flags()` and 
> `getFlags()` is different, in the first case you return `OK(_flags(), 
> request.url.query.get("jsonp"));`, in the second case you return 
> `OK(serialize(contentType, 
> evolve(_flags())), 
> stringify(contentType));`. 
> 
> If we go with `_flags()` returning a future response, which one should 
> that one be? the first response or the second one?

`_flags()` should return http::Response whose body has JSON::Object. 
`getFlags()` can then convert that http::Response to a new http::Response whose 
body is v1::master::Response. does that make sense?


- Vinod


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


On June 29, 2016, 8:44 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49313/
> ---
> 
> (Updated June 29, 2016, 8:44 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5705
> https://issues.apache.org/jira/browse/MESOS-5705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds again authorization for flags. Instead of being part of
> `get_endpoints` it uses its own action `VIEW_TASKS` which is
> used to restrict access to the `/flags` endpoint, as well as
> to filter the results of the `/state` endpoint on both master
> and agents.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> a6d93cd2cb9161a98565b22e50b06aac4931a671 
>   include/mesos/authorizer/authorizer.proto 
> fc76796022a6fa3d36a1447c476980868d42c2d0 
>   src/authorizer/local/authorizer.cpp 
> 3fade4168face1cb80b30c9b69b31d9eb4126222 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/common/http.cpp 95e8fb672b49a00860c64f818cc734fa22cf3516 
>   src/master/http.cpp 311db1a9400ab533f4536e7a7412122275a7044d 
>   src/slave/http.cpp 44d8cc98c0c1ada9d5313a3fe5c66029c9c373c6 
>   src/tests/authorization_tests.cpp 9b99da138fa27a725738d70bd99e889b108b44ae 
>   src/tests/master_authorization_tests.cpp 
> 9088d7df901ad9e0b3c43a3ea61882054f55ee93 
>   src/tests/slave_authorization_tests.cpp 
> 78221e200d9b7880cc474f1acef92c5dec1c8e25 
> 
> Diff: https://reviews.apache.org/r/49313/diff/
> 
> 
> Testing
> ---
> 
> - `make check`
> - manual tests with browsers.
> - Used the script:
>  
> ```sh
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "view_flags" : [
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "ANY" }
>},
>{
>  "principals" : { "values" : ["foo"] },
>  "flags" : { "type" : "NONE" }
>}
>   ]
> }
> EOF
> 
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master \
>  --authenticate_http \
>  --log_dir=/tmp/mesos/logs/master \
>  --http_credentials=file:///tmp/credentials.txt \
>  --acls=file:///tmp/acls.json &
> ./bin/mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>   

Re: Review Request 48908: Added test case for exclude resources from sorter.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49352, 49181, 48895, 48904, 49190, 48907, 48908]

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

- Mesos ReviewBot


On June 29, 2016, 12:29 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48908/
> ---
> 
> (Updated June 29, 2016, 12:29 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5623
> https://issues.apache.org/jira/browse/MESOS-5623
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for exclude resources from sorter.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> eb11ff6b1b7bce291f336a199e8700be0bdc61c1 
> 
> Diff: https://reviews.apache.org/r/48908/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HierarchicalAllocatorTest
> [ RUN  ] HierarchicalAllocatorTest.DRFWithExcludeResources
> [   OK ] HierarchicalAllocatorTest.DRFWithExcludeResources (59 ms)
> [--] 1 test from HierarchicalAllocatorTest (59 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (76 ms total)
> [  PASSED  ] 1 test.
> 
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from HierarchicalAllocatorTest
> [ RUN  ] 
> HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources
> [   OK ] 
> HierarchicalAllocatorTest.DominantShareMetricsWithExcludeResources (129 ms)
> [--] 1 test from HierarchicalAllocatorTest (129 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (147 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 49394: Added support for VIEW_FLAGS authorization action in HTTP API (V2).

[Alexander Rojas]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Adds a call to `authorizer::authorize()` where the HTTP API builds
the response for a query of the command-line flags given to start
the master.


Diffs
-

  src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
  src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 49394: Added support for VIEW_FLAGS authorization action in HTTP API (V2).

[Vinod Kone]

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



looking good. minor comments.


src/master/http.cpp (line 1463)


seems weird for `_flags()` to take Option jsonp when it is not 
going to be used by RPC handler. 

why  not do jsonp logic here?

```
return _flags(principal)
  .then([request](const Response& response) -> Response) {
if (response.status != OK().status) {
  return response;
}

Try json = JSON::parse(response.body);

if (json.isError()) {
  return InternalServerError(json.error());
}

return OK(json.get(), request.url.querty.get("jsonp"));
  }
```



src/master/http.cpp (line 1468)


each parameter should be on a different line. but if you make the change 
above, this will only take `principal`.



src/master/http.cpp (lines 1471 - 1490)


no need for jsonp logic here.



src/master/http.cpp (lines 1521 - 1523)


no need to defer because this is just a static lambda that doesn't use any 
master's internal data.


- Vinod Kone


On June 29, 2016, 8:38 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49394/
> ---
> 
> (Updated June 29, 2016, 8:38 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a call to `authorizer::authorize()` where the HTTP API builds
> the response for a query of the command-line flags given to start
> the master.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 55bd0ac81af80c656a4a80766a3e4b21db9cf0cf 
>   src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
>   src/master/master.hpp e2ab2110fe5a287ab16ac9ef4222fed633e02ebe 
> 
> Diff: https://reviews.apache.org/r/49394/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 49351: Added wrapper function for health check in docker executor.

[Timothy Chen]

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




src/docker/executor.cpp (line 465)


wrapHealthCheckCommand sounds very generic and to me it makes it less clear 
what the extent is, so i'm not sure this change makes the code much more 
understandable.

I don't see you need to refactor this in your other patch as well. 

My opinion I think is to just keep it as is, or perhaps rename wrap 
into something more obvious?


- Timothy Chen


On June 29, 2016, 9:03 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49351/
> ---
> 
> (Updated June 29, 2016, 9:03 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Benjamin Mahler, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2533
> https://issues.apache.org/jira/browse/MESOS-2533
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added wrapper function for health check in docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 88b7fc4c36ed3974ac6b103a29e1d975619f0c69 
> 
> Diff: https://reviews.apache.org/r/49351/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 44431: Do not check overlayfs when create overlay backend.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 28, 2016, 6:45 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44431/
> ---
> 
> (Updated June 28, 2016, 6:45 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Shuai Lin.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not check overlayfs when create overlay backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> 6c0888c60ec0ceb9a33c6848737a0560f593f449 
> 
> Diff: https://reviews.apache.org/r/44431/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 38146: Added missing colon in modules.md.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 14, 2016, 3:16 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/38146/
> ---
> 
> (Updated May 14, 2016, 3:16 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing colon in modules.md.
> 
> 
> Diffs
> -
> 
>   docs/modules.md 28eb233a30b844b302fd95c03e4ff6647355cdfa 
> 
> Diff: https://reviews.apache.org/r/38146/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 48374: Added class to share Nvidia-specific components between containerizers.

[Benjamin Mahler]

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


Fix it, then Ship it!




We should mention in the commit description that we're introducing this to 
contain the `#ifdef`s around the Nvidia classes that we need to pass around.


src/slave/containerizer/containerizer.cpp (lines 234 - 236)


Error context here?



src/slave/containerizer/mesos/isolators/gpu/components.hpp (line 44)


Hm.. why the const here? It seems that the containerizers are going to need 
to do non-const operations on the allocator, and having const here requires 
that they make a copy.



src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp (lines 110 - 111)


Hm.. after chatting with Kevin it seems that this case occurs when they are 
on Linux and ask for the gpu isolator, but NVML is not available. Therefore we 
realized that we shouldn't pass the option here and instead we should surface a 
better error message to the user within the isolator creation logic up in the 
mesos containerizer.


- Benjamin Mahler


On June 29, 2016, 12:25 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48374/
> ---
> 
> (Updated June 29, 2016, 12:25 a.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5562
> https://issues.apache.org/jira/browse/MESOS-5562
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, the `NvidiaGpuAllocator` component was created directly
> inside the `NvidiaGpuIsolatorProcess` even though it was designed to
> be used by multiple containerizers at the same time. To allow for
> simultaneous access, we created a new `NvidiaComponents` class to hold
> an `NvidiaGpuAllocator` instance and pass it to each containerizer as
> it is created. This component can easily be extended to include more
> cross-containerizer components later on.
> 
> We create the `NvidiaComponents` instance inside
> `Containerizer::create()` and pass it to both the docker containerizer
> and the mesos containerizer when they are created.  The docker
> containerizer currently doesn't do anything with this information, but
> its interface has been updated to accomodate it. The mesos
> containerizer has been updated to pass this all the way down to the
> `NvidiaGpuIsolatorProcess` and exploit it.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/slave/containerizer/containerizer.cpp 
> 2449526061f7d6ac46ff42260ccdd0278694d9d4 
>   src/slave/containerizer/docker.hpp 9b0a5a76e457006119e657ee8f627dcd1326c0ce 
>   src/slave/containerizer/docker.cpp 514268fd84507eafa11fc57d38a23b0502f80ef8 
>   src/slave/containerizer/mesos/containerizer.hpp 
> a1a00020668f6da8d611f26e5637afffc87d09ba 
>   src/slave/containerizer/mesos/containerizer.cpp 
> d984efd4742ec084d66538c48a36ea768832324d 
>   src/slave/containerizer/mesos/isolators/gpu/components.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp 
> e35a55fdf44ec01d49d6a9e396a88f4163ef79c3 
>   src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp 
> b8753ec8fc0afc8f2b389250de57d5095287acf9 
> 
> Diff: https://reviews.apache.org/r/48374/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 48900: Some cleanup in docker_volume_isolator_tests.cpp.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On June 18, 2016, 4:34 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48900/
> ---
> 
> (Updated June 18, 2016, 4:34 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some cleanup in docker_volume_isolator_tests.cpp.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_volume_isolator_tests.cpp 
> effa687f0b05124e4e5229dfd6490e796aea4048 
> 
> Diff: https://reviews.apache.org/r/48900/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49122: Added a MAC parse function to stout.

[Jie Yu]

> On June 23, 2016, 2:45 a.m., Jay Guo wrote:
> > 3rdparty/stout/include/stout/mac.hpp, lines 95-97
> > 
> >
> > I wonder if this is necessary given two digit hex ( `if 
> > (tokens[i].size() != 2)` ...)

yep, you're right! used a CHECK instead.


- Jie


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


On June 22, 2016, 11:56 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49122/
> ---
> 
> (Updated June 22, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MAC parse function to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/mac.hpp 
> 1aaafe1380c5300225c2ac9276e87962bd5a0d88 
>   3rdparty/stout/tests/mac_tests.cpp f905d4528040ce97d80010b2ba3f13870dca0c7c 
> 
> Diff: https://reviews.apache.org/r/49122/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49122: Added a MAC parse function to stout.

[Jie Yu]

> On June 23, 2016, 9:48 a.m., Guangya Liu wrote:
> > 3rdparty/stout/include/stout/mac.hpp, line 79
> > 
> >
> > What about 
> > 
> > Invalid format of MAC address '', should be six groups of two 
> > hexadecimal digits as xx:xx:xx:xx:xx:xx
> > 
> > Including the MAC address in the error msg can show more hints to end 
> > user.

The caller should print the mac address.


- Jie


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


On June 22, 2016, 11:56 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49122/
> ---
> 
> (Updated June 22, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MAC parse function to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/mac.hpp 
> 1aaafe1380c5300225c2ac9276e87962bd5a0d88 
>   3rdparty/stout/tests/mac_tests.cpp f905d4528040ce97d80010b2ba3f13870dca0c7c 
> 
> Diff: https://reviews.apache.org/r/49122/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49377: Fixed allocator to update total resources in quota sorter.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49323, 49375, 49376, 49377]

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

- Mesos ReviewBot


On June 29, 2016, 1:49 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49377/
> ---
> 
> (Updated June 29, 2016, 1:49 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Each DRFSorter tracks the total resources in the cluster. This means
> that each sorter must be updated when the resources in the cluster have
> changed, e.g., due to the creation of a dynamic reservation or a
> persistent volume. In the previous implementation, the quota role sorter
> was not updated for non-quota roles when a reservation or persistent
> volume was created by a framework. This resulted in inconsistency
> between the total resources in the allocator and the quota role sorter.
> 
> This could cause several problems. First, removing a slave from the
> cluster would leak resources in the quota role sorter. Second, certain
> interleavings of slave removals and reserve/unreserve operations by
> frameworks and via HTTP endpoints could lead to CHECK failures.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> c3639342335499a04a23147a4205f1b475c123fa 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 27d56f274c41bbabe6f2abbbcebd2c4eff52693e 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6c85e19eeaa69bf3a4e3077261331191db6eec06 
> 
> Diff: https://reviews.apache.org/r/49377/diff/
> 
> 
> Testing
> ---
> 
> make check on OSX and recent Arch Linux.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 49352: Added a flag parser for hashset.

[Benjamin Mahler]

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




src/common/parse.hpp (line 144)


Can we use std::set for now?



src/common/parse.hpp (lines 148 - 155)


We should let the user know that duplicate entries are an error. Otherwise, 
if intending to accomplish something via the duplicates and we silently accept 
them, they are unaware that the duplicates have no effect.


- Benjamin Mahler


On June 29, 2016, 12:12 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49352/
> ---
> 
> (Updated June 29, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5743
> https://issues.apache.org/jira/browse/MESOS-5743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag parser for hashset.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
> 
> Diff: https://reviews.apache.org/r/49352/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 48375: Rearranged Nvidia GPU files to cleanup semantics for header inclusion.

[Kevin Klues]

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

(Updated June 29, 2016, 11:09 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Edited commit message and fixed some "double" includes from rebase.


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


Repository: mesos


Description (updated)
---

Previously, any components outside of
`src/slave/containerizers/mesos/isolators/gpu` that needed access to
header files in this folder had to protect (most) of their #includes
with an #ifdef __linux__ directive. However, at least one header file
should not be protected by this directive (i.e. components.hpp),
making it confusing as to which headers should be protected by
__linux__ and which ones shouldn't. Before this commit, components
could end up with #include blocks such as:

  #ifdef __linux__
  #include "src/slave/containerizers/mesos/isolators/gpu/allocator.hpp"
  #include "src/slave/containerizers/mesos/isolators/gpu/nvidia.hpp"
  #include "src/slave/containerizers/mesos/isolators/gpu/nvml.hpp"
  #endif
  #include "src/slave/containerizers/mesos/isolators/gpu/components.hpp"

This commit cleans up this header madness, by creating a common
"nvidia.hpp" header that takes care of all the dependencies on
__linux__ for you. All componenents outside of
`src/slave/containerizers/mesos/isolators/gpu` now only need to #include
this one header instead of managing everything themselves.


Diffs (updated)
-

  src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
  src/slave/containerizer/containerizer.cpp 
2449526061f7d6ac46ff42260ccdd0278694d9d4 
  src/slave/containerizer/docker.hpp 9b0a5a76e457006119e657ee8f627dcd1326c0ce 
  src/slave/containerizer/mesos/containerizer.hpp 
a1a00020668f6da8d611f26e5637afffc87d09ba 
  src/slave/containerizer/mesos/containerizer.cpp 
d984efd4742ec084d66538c48a36ea768832324d 
  src/slave/containerizer/mesos/isolators/gpu/isolator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp 
e35a55fdf44ec01d49d6a9e396a88f4163ef79c3 
  src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp 
b8753ec8fc0afc8f2b389250de57d5095287acf9 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
28ec3f9954576d78153e9d0f57e22a240e950639 

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


Testing
---

GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 49122: Added a MAC parse function to stout.

[Jie Yu]

> On June 23, 2016, 2:45 a.m., Jay Guo wrote:
> > 3rdparty/stout/include/stout/mac.hpp, lines 95-97
> > 
> >
> > I wonder if this is necessary given two digit hex ( `if 
> > (tokens[i].size() != 2)` ...)
> 
> Jie Yu wrote:
> yep, you're right! used a CHECK instead.

In fact, i realized that `-1` is a valid hex as well. that'll break the CHECK. 
so, i changed my impl. to check each digit explicitly.


- Jie


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


On June 22, 2016, 11:56 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49122/
> ---
> 
> (Updated June 22, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Gilbert Song.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a MAC parse function to stout.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/mac.hpp 
> 1aaafe1380c5300225c2ac9276e87962bd5a0d88 
>   3rdparty/stout/tests/mac_tests.cpp f905d4528040ce97d80010b2ba3f13870dca0c7c 
> 
> Diff: https://reviews.apache.org/r/49122/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 49122: Added a MAC parse function to stout.

[Jie Yu]

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

(Updated June 29, 2016, 11:38 p.m.)


Review request for mesos, Avinash sridharan and Gilbert Song.


Changes
---

Thanks guys! I changed the impl. to do explicit digit checking and added more 
test cases.


Repository: mesos


Description
---

Added a MAC parse function to stout.


Diffs (updated)
-

  3rdparty/stout/include/stout/mac.hpp 1aaafe1380c5300225c2ac9276e87962bd5a0d88 
  3rdparty/stout/tests/mac_tests.cpp f905d4528040ce97d80010b2ba3f13870dca0c7c 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 48375: Rearranged Nvidia GPU files to cleanup semantics for header inclusion.

[Kevin Klues]

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

(Updated June 29, 2016, 11:50 p.m.)


Review request for mesos and Benjamin Mahler.


Changes
---

Rebased


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


Repository: mesos


Description
---

Previously, any components outside of
`src/slave/containerizers/mesos/isolators/gpu` that needed access to
header files in this folder had to protect (most) of their #includes
with an #ifdef __linux__ directive. However, at least one header file
should not be protected by this directive (i.e. components.hpp),
making it confusing as to which headers should be protected by
__linux__ and which ones shouldn't. Before this commit, components
could end up with #include blocks such as:

  #ifdef __linux__
  #include "src/slave/containerizers/mesos/isolators/gpu/allocator.hpp"
  #include "src/slave/containerizers/mesos/isolators/gpu/nvidia.hpp"
  #include "src/slave/containerizers/mesos/isolators/gpu/nvml.hpp"
  #endif
  #include "src/slave/containerizers/mesos/isolators/gpu/components.hpp"

This commit cleans up this header madness, by creating a common
"nvidia.hpp" header that takes care of all the dependencies on
__linux__ for you. All componenents outside of
`src/slave/containerizers/mesos/isolators/gpu` now only need to #include
this one header instead of managing everything themselves.


Diffs (updated)
-

  src/Makefile.am bdad9c2ae07585b53aac97341550f3ea0b852ae7 
  src/slave/containerizer/containerizer.cpp 
f2ff116f938c22c8698ee66046e549229d66d277 
  src/slave/containerizer/docker.hpp 51880a50a45fc89e84c597d831c74010663c440e 
  src/slave/containerizer/mesos/containerizer.hpp 
8e347735fad2301a2bcbc7d141efbf0f2b708435 
  src/slave/containerizer/mesos/containerizer.cpp 
63cf92217054fab43c843379c86e25ce7f07c7d9 
  src/slave/containerizer/mesos/isolators/gpu/isolator.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp 
a6734105dcb3efadfceb7cdd357b749813a5bf40 
  src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp 
88dd9b20ab75355f4f0ac9628f654db05a783a8e 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
28ec3f9954576d78153e9d0f57e22a240e950639 

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


Testing
---

GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests


Thanks,

Kevin Klues

Re: Review Request 49175: Added tests for libprocess linking and unlinking behavior.

[Joseph Wu]

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

(Updated June 29, 2016, 4:54 p.m.)


Review request for mesos, Benjamin Hindman, Benjamin Mahler, Gilbert Song, 
Artem Harutyunyan, and Jie Yu.


Changes
---

Depending on the system's implementation of `sh`, the `kill(...)` may only kill 
the shell, without killing the underlying process.  Using `os::killtree` will 
do the correct thing on all systems (i.e. tests fails on Debian and Ubuntu 
because of this).

Also added the appropriate cmake build changes.


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


Repository: mesos


Description
---

Adds tests which exercise "link" semantics against remote processes.
This includes detection of `ExitedEvents` when the process exits
as well as mixing "link" semantics.

Includes a test case that emulates the failure observed in MESOS-5576.


Diffs (updated)
-

  3rdparty/libprocess/Makefile.am 3c3249fe9799ba919ac7bd13e2ddb07a306737f0 
  3rdparty/libprocess/src/tests/CMakeLists.txt 
0c659dc00fe4fde78b2b57ebcc49cb47daf162d9 
  3rdparty/libprocess/src/tests/process_tests.cpp 
3fce6fc41faaa7b6e8a2a957e85de3de973a51ba 
  3rdparty/libprocess/src/tests/test_linkee.cpp PRE-CREATION 

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


Testing (updated)
---

See end of chain.


Thanks,

Joseph Wu

Review Request 49404: Changed the SocketManager to pass outgoing "Sockets" by value.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Artem Harutyunyan.


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


Repository: mesos


Description
---

`Sockets` is already a reference-counted `shared_ptr` under the covers.
By passing around `Sockets` by value, we avoid potentially deleting
a reference while the same reference is in use by another function.

This fixes a rare race (segfault) between `link`/`send` and 
`ignore_recv_data`.  If the peer of the socket exits between 
establishing a connection and libprocess queuing a `MessageEncoder`, 
`ignore_recv_data` may delete the `Socket` underneath the `link`/`send`.


Diffs
-

  3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 

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


Testing
---

make check (OSX)

3rdparty/libprocess/libprocess-tests 
--gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
--gtest_repeat=1


Thanks,

Joseph Wu

Re: Review Request 49273: Helper binary for executors to chroot tasks.

[Jie Yu]

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



I'll try to refactor `mesos-containerizer launch` so that command executor (and 
thermos) can use that to launch tasks.

- Jie Yu


On June 27, 2016, 5:09 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49273/
> ---
> 
> (Updated June 27, 2016, 5:09 p.m.)
> 
> 
> Review request for mesos, Joshua Cohen and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Uses the same code as the agent uses for chroot'ing on Linux, i.e., 
> pivot_root and setting up /dev etc. Intention is that executors (like 
> Aurora's Thermos) can use it to chroot tasks.
> 
> Currently, the root path is specified as a flag and the remaining arguments 
> are exec'ed. Joshua has also requested that the root path could be specified 
> as the first arg. @Jie, thoughts?
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 86c39fdf379ada470c9b1f86be263ef71dc47c41 
>   src/slave/containerizer/mesos-chroot.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/49273/diff/
> 
> 
> Testing
> ---
> 
> Manual.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 48375: Rearranged Nvidia GPU files to cleanup semantics for header inclusion.

[Benjamin Mahler]

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


Ship it!





src/Makefile.am (line 1079)


Whoops, spaces and tabs are mixed here (that's what the red is indicating).


- Benjamin Mahler


On June 29, 2016, 11:50 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48375/
> ---
> 
> (Updated June 29, 2016, 11:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5563
> https://issues.apache.org/jira/browse/MESOS-5563
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, any components outside of
> `src/slave/containerizers/mesos/isolators/gpu` that needed access to
> header files in this folder had to protect (most) of their #includes
> with an #ifdef __linux__ directive. However, at least one header file
> should not be protected by this directive (i.e. components.hpp),
> making it confusing as to which headers should be protected by
> __linux__ and which ones shouldn't. Before this commit, components
> could end up with #include blocks such as:
> 
>   #ifdef __linux__
>   #include "src/slave/containerizers/mesos/isolators/gpu/allocator.hpp"
>   #include "src/slave/containerizers/mesos/isolators/gpu/nvidia.hpp"
>   #include "src/slave/containerizers/mesos/isolators/gpu/nvml.hpp"
>   #endif
>   #include "src/slave/containerizers/mesos/isolators/gpu/components.hpp"
> 
> This commit cleans up this header madness, by creating a common
> "nvidia.hpp" header that takes care of all the dependencies on
> __linux__ for you. All componenents outside of
> `src/slave/containerizers/mesos/isolators/gpu` now only need to #include
> this one header instead of managing everything themselves.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am bdad9c2ae07585b53aac97341550f3ea0b852ae7 
>   src/slave/containerizer/containerizer.cpp 
> f2ff116f938c22c8698ee66046e549229d66d277 
>   src/slave/containerizer/docker.hpp 51880a50a45fc89e84c597d831c74010663c440e 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 8e347735fad2301a2bcbc7d141efbf0f2b708435 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 63cf92217054fab43c843379c86e25ce7f07c7d9 
>   src/slave/containerizer/mesos/isolators/gpu/isolator.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/gpu/nvidia.hpp 
> a6734105dcb3efadfceb7cdd357b749813a5bf40 
>   src/slave/containerizer/mesos/isolators/gpu/nvidia.cpp 
> 88dd9b20ab75355f4f0ac9628f654db05a783a8e 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp 
> 28ec3f9954576d78153e9d0f57e22a240e950639 
> 
> Diff: https://reviews.apache.org/r/48375/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="" make -j check && sudo GTEST_FILTER="*NVIDIA*" src/mesos-tests
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 49346: Changed the replicated log's network abstraction to use "relink".

[Joseph Wu]

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

(Updated June 29, 2016, 5:02 p.m.)


Review request for mesos, Anand Mazumdar, Benjamin Hindman, Benjamin Mahler, 
Gilbert Song, Artem Harutyunyan, and Jie Yu.


Changes
---

Non-abbreviated summary fits under 68 characters!  :)

Integrated BenM's comment suggestion, mostly verbatim.


Summary (updated)
-

Changed the replicated log's network abstraction to use "relink".


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/log/network.hpp 56c5dbb38eaeaa70735c47a2266b0dbebc42aa35 

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


Testing (updated)
---

make check on Debian 8; Ubuntu 12, 14, 15, 16; CentOS 6, 7; Fedora 23
^ with and without SSL

cmake .. && make && make check on Fedora 23


Thanks,

Joseph Wu

Review Request 49400: Extended utilities to render certificate extension for IP.

[Till Toenshoff]

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

Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


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


Repository: mesos


Description
---

Adds the ability to render a subject alternative name based on a given
IP address within a X509 certificate extension. Additionally the
libprocess test suite makes use of this feature.


Diffs
-

  3rdparty/libprocess/include/process/ssl/gtest.hpp 5435ddd 
  3rdparty/libprocess/include/process/ssl/utilities.hpp ad9ec5d 
  3rdparty/libprocess/src/ssl/utilities.cpp d23f462 

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


Testing
---

make check on OSX and various linux distros.

Functional testing by validating a rendered certificate;

```
openssl x509 -text -noout -in "temp_cert_file_name"
```


Thanks,

Till Toenshoff

Review Request 49411: Updated CHANGELOG for MESOS-5724.

[Till Toenshoff]

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

Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  CHANGELOG 7a44422e47a0453d2079070857c5537b63f4ae0a 

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


Testing
---


Thanks,

Till Toenshoff

Review Request 49412: Updated SSL.md with 'SSL_VERIFY_IPADD'.

[Till Toenshoff]

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

Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  docs/ssl.md 5eef9fd5afbe2b4984f8ba352438f8ea95676355 

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


Testing
---


Thanks,

Till Toenshoff

Review Request 49402: Added tests for IP based certificate validation.

[Till Toenshoff]

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

Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


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


Repository: mesos


Description
---

see summary.


Diffs
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
cf0119d1266cb81aa5fc7a86e034ba302892d44f 

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


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Review Request 49401: Updated certificate validation to check 'IP Address' SAN.

[Till Toenshoff]

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

Review request for mesos, Adam B, Albert Strasheim, Artem Harutyunyan, Joris 
Van Remoortere, and Lukas Loesche.


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


Repository: mesos


Description
---

Allows the verification of X509 certificates based on an IP address
instead of a hostname. Introduces a new environment variable;
`SSL_VERIFY_IPADD` which, when set to `true` will disable any
attempts to reverse-/lookup the hostname for certificate validation.
Instead the peer certificate verification then relies on the IP
address of a connection.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.hpp 1dbdaa8 
  3rdparty/libprocess/src/libevent_ssl_socket.cpp 19d9ae5 
  3rdparty/libprocess/src/openssl.hpp 7d55025 
  3rdparty/libprocess/src/openssl.cpp 0f62aa6 

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


Testing
---

make check on OSX and various linux distros.


Thanks,

Till Toenshoff

Re: Review Request 49404: Changed the SocketManager to pass outgoing "Sockets" by value.

[Benjamin Mahler]

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


Ship it!




Looks good, thanks! Can you also sweep up the remaining Socket pointers that 
were not as related to the race you found?


3rdparty/libprocess/src/process.cpp (line 1838)


For example, no need for a pointer here.



3rdparty/libprocess/src/process.cpp (line 1847)


Or here, etc.


- Benjamin Mahler


On June 29, 2016, 11:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49404/
> ---
> 
> (Updated June 29, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5748
> https://issues.apache.org/jira/browse/MESOS-5748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Sockets` is already a reference-counted `shared_ptr` under the covers.
> By passing around `Sockets` by value, we avoid potentially deleting
> a reference while the same reference is in use by another function.
> 
> This fixes a rare race (segfault) between `link`/`send` and 
> `ignore_recv_data`.  If the peer of the socket exits between 
> establishing a connection and libprocess queuing a `MessageEncoder`, 
> `ignore_recv_data` may delete the `Socket` underneath the `link`/`send`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49404/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> 
> 3rdparty/libprocess/libprocess-tests 
> --gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
> --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49250: Regenerated endpoint documention.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [49251, 49250]

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

- Mesos ReviewBot


On June 29, 2016, 2 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49250/
> ---
> 
> (Updated June 29, 2016, 2 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Neil Conway.
> 
> 
> Bugs: MESOS-5711
> https://issues.apache.org/jira/browse/MESOS-5711
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Note that regenerating the endpoint documentation
> also picked up a stale change in slave/api/vi.md.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/create-volumes.md 
> f6ec9384f7f2bfb8056e54532ac3417a1f15b1d1 
>   docs/endpoints/master/destroy-volumes.md 
> 054e816371f26c91ccc0d9687f377e80641e42a8 
>   docs/endpoints/master/frameworks.md 
> 1aed723152528be0e19d79520daac5f470cb49da 
>   docs/endpoints/master/reserve.md 9bb04ed0de5b9304301fffd51b39ec07102825cd 
>   docs/endpoints/master/state-summary.md 
> 4eb517ea0f6b5dbc2e7d7c406d54e741cf50e0e5 
>   docs/endpoints/master/state.json.md 
> 8989a8e97941178575edbebc75c6c174a1b0e5d3 
>   docs/endpoints/master/state.md 9bb753ade7ef73109c7705b2bae270dd9b1bf99f 
>   docs/endpoints/master/tasks.json.md 
> 5d2c0e6eb53fa5e04a973eb786cd16bfad38736f 
>   docs/endpoints/master/tasks.md c7df686443e20d255b03fc31a999b9b24b5f3a0a 
>   docs/endpoints/master/teardown.md 4c14f594c3713304d5adf59e56e9e8af2b05ebee 
>   docs/endpoints/master/unreserve.md 5cce4288231a64d7ee7713f2b44487618e7cc0a7 
>   docs/endpoints/slave/api/v1.md c867d795104e053e5aae819df3deea68269228bf 
>   docs/endpoints/slave/state.json.md ac8536986ceb6532f71b4fe1c62b1b83390b03e7 
>   docs/endpoints/slave/state.md c7b61d736582f50b850fed252e558bbae6dad801 
> 
> Diff: https://reviews.apache.org/r/49250/diff/
> 
> 
> Testing
> ---
> 
> viewed generated documentation via website docker container.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 49414: Added Executor PID in /containers endpoint. Also Added Test Cases.

[Haris Choudhary]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added Executor PID in /containers endpoint. Also Added Test Cases.

In order to greatly simplify the implementation for the Mesos CLI's container 
plugin, we need the executor PID (Process ID) to be exposed in the /containers 
endpoint. [Mesos CLI Epic](https://issues.apache.org/jira/browse/MESOS-5676)
This change will introduce the pid for an executor if it was launched by the 
mesos containerizer in the /containers endpoint of an agent.


Diffs
-

  include/mesos/mesos.proto 5b15a89bbac0b33c77e305c5b188823f2704a653 
  include/mesos/v1/mesos.proto a1435278e81c8f3179d94cd9d2c3dd8c0ba82d35 
  src/slave/containerizer/mesos/containerizer.cpp 
a96b382f22886362a1159e1166dfe041072985ba 
  src/slave/containerizer/mesos/launcher.hpp 
05320f462653c31fc2f093d6c67e2182e9c794fa 
  src/slave/containerizer/mesos/launcher.cpp 
ff675262af8947b89f8099828665e5e5d86491d8 
  src/slave/containerizer/mesos/linux_launcher.hpp 
89bb2958a41dffe4ade9c2492b9a7412f90a432d 
  src/slave/containerizer/mesos/linux_launcher.cpp 
5028854fa003615f158120e030866b7ec4402b66 
  src/tests/containerizer/launcher.hpp c352634c4766d289706c7cc738677619d7d02ccd 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
6c14f6e20b4d14d5ed095670673571739101b0e4 

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


Testing
---

make and make check.


Thanks,

Haris Choudhary

Review Request 49415: Removed the argv parameter in command executor helper.

[Jie Yu]

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

Review request for mesos and Joris Van Remoortere.


Repository: mesos


Description
---

It's confusing to pass in 'argv' because it's already contained in
'command'. This patch removed this parameter.


Diffs
-

  src/launcher/executor.cpp bb88a4570d183d22dff65f6e220d566c0924001a 
  src/launcher/posix/executor.hpp e5f3c0a8e2d3687a5330adbf2f667db9106970c7 
  src/launcher/posix/executor.cpp ab1dd938a7c59f52b44aafe1340c614087460f84 
  src/launcher/windows/executor.cpp f6da398eeddaf68b8eaf510648c55964d9f5b7c7 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 49404: Changed the SocketManager to pass outgoing "Sockets" by value.

[Benjamin Mahler]

> On June 30, 2016, 12:24 a.m., Benjamin Mahler wrote:
> > Looks good, thanks! Can you also sweep up the remaining Socket pointers 
> > that were not as related to the race you found?

Actually, on second thought per our converstation. Could we fix this more 
minimally by adding `new`s?

```
  socket->recv(data, size)
.onAny(lambda::bind(
&internal::ignore_recv_data,
lambda::_1,
new Socket(*socket), // XXX: new here
data,
size));
```

That would make backporting easier, the fix more obvious for posterity, and we 
can follow up with a complete pointer sweep in a separate patch.


- Benjamin


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


On June 29, 2016, 11:56 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49404/
> ---
> 
> (Updated June 29, 2016, 11:56 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5748
> https://issues.apache.org/jira/browse/MESOS-5748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Sockets` is already a reference-counted `shared_ptr` under the covers.
> By passing around `Sockets` by value, we avoid potentially deleting
> a reference while the same reference is in use by another function.
> 
> This fixes a rare race (segfault) between `link`/`send` and 
> `ignore_recv_data`.  If the peer of the socket exits between 
> establishing a connection and libprocess queuing a `MessageEncoder`, 
> `ignore_recv_data` may delete the `Socket` underneath the `link`/`send`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49404/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> 
> 3rdparty/libprocess/libprocess-tests 
> --gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
> --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 49416: Fixed race between link and peer disconnection.

[Joseph Wu]

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

Review request for mesos, Benjamin Mahler and Artem Harutyunyan.


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


Repository: mesos


Description
---

This fixes a rare race (segfault) between `link` and 
`ignore_recv_data`.  If the peer of the socket exits between 
establishing a connection and libprocess queuing a `MessageEncoder`, 
`ignore_recv_data` may delete the `Socket` underneath the `link`.

This patch is meant to be easily backported.


Diffs
-

  3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 

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


Testing
---

make check (OSX)

3rdparty/libprocess/libprocess-tests 
--gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
--gtest_repeat=1


Thanks,

Joseph Wu

Re: Review Request 49404: Changed the SocketManager to pass "Sockets" by value.

[Joseph Wu]

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

(Updated June 29, 2016, 6:11 p.m.)


Review request for mesos, Benjamin Mahler and Artem Harutyunyan.


Changes
---

Separated out the race fix into a smaller patch, which goes in the opposite 
direction as this patch (adds vs removes pointers).

Changed this patch to remove all `Socket*`s.


Summary (updated)
-

Changed the SocketManager to pass "Sockets" by value.


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


Repository: mesos


Description (updated)
---

`Sockets` is already a reference-counted `shared_ptr` under the covers.
By passing around `Sockets` by value, we avoid potentially deleting
a reference while the same reference is in use by another function.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 9bae71246e751e491be5a989eea8aca29c9aa751 

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


Testing
---

make check (OSX)

3rdparty/libprocess/libprocess-tests 
--gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
--gtest_repeat=1


Thanks,

Joseph Wu

Re: Review Request 49416: Fixed race between link and peer disconnection.

[Benjamin Mahler]

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


Ship it!




Ship It!

- Benjamin Mahler


On June 30, 2016, 1:10 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49416/
> ---
> 
> (Updated June 30, 2016, 1:10 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5748
> https://issues.apache.org/jira/browse/MESOS-5748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This fixes a rare race (segfault) between `link` and 
> `ignore_recv_data`.  If the peer of the socket exits between 
> establishing a connection and libprocess queuing a `MessageEncoder`, 
> `ignore_recv_data` may delete the `Socket` underneath the `link`.
> 
> This patch is meant to be easily backported.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49416/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> 
> 3rdparty/libprocess/libprocess-tests 
> --gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
> --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 49354: Sent SHUTDOWN event for non-checkpointed executor during disconnection.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On June 29, 2016, 5:52 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49354/
> ---
> 
> (Updated June 29, 2016, 5:52 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5295
> https://issues.apache.org/jira/browse/MESOS-5295
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Sent SHUTDOWN event for non-checkpointed executor during disconnection.
> 
> 
> Diffs
> -
> 
>   src/executor/executor.cpp 6ab4c3fc15659e5918e22fe9b1e8571a97c77364 
> 
> Diff: https://reviews.apache.org/r/49354/diff/
> 
> 
> Testing
> ---
> 
> When the agent exits, the task will be killed as well along with the HTTP 
> command executor.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 49418: Decoupled implementation of `tasks()` and `getTasks()` in master.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar, Jay Guo, and Zhitao Li.


Repository: mesos


Description
---

The response of `getTasks()` is going to look different from that of
`tasks()`, so it doesn't make sense to couple their implementations.
Note that this is pure code movement, there is no functional change.


Diffs
-

  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 

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


Testing
---

make check


Thanks,

Vinod Kone

Review Request 49419: Updated GetTasks v1 call in master.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar, Jay Guo, and Zhitao Li.


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


Repository: mesos


Description
---

The response now distinguishes between active tasks, completed tasks,
pending tasks and orphan tasks to make it easy for clients.
Consequently got rid of offset, limit and offset in the Call because
they don't make sense when we have multiple fields in the response.


Diffs
-

  include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
  include/mesos/v1/master/master.proto 93157d57dcc53b54fed2ebbc4772c689ddba2119 
  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 
  src/master/validation.cpp 6939d0e6ac3dbddc10f0315bd0a696a1f71634e2 
  src/tests/api_tests.cpp b3127b716480223a6f23b7908bf6bc1808120f80 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 49352: Added a flag parser for hashset.

[Guangya Liu]

> On 六月 29, 2016, 11:07 p.m., Benjamin Mahler wrote:
> > src/common/parse.hpp, line 144
> > 
> >
> > Can we use std::set for now?

As I want to check if there are duplicate items in the flag of 
`allocator_fairness_excluded_resource_names`, so here seems `hashset` is a good 
option as I can check if there are duplicate items via `contains` method. So 
here I would prefer to use `hashset` but return some error if there are some 
duplicate items, comments?


- Guangya


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


On 六月 29, 2016, 12:12 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49352/
> ---
> 
> (Updated 六月 29, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5743
> https://issues.apache.org/jira/browse/MESOS-5743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag parser for hashset.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
> 
> Diff: https://reviews.apache.org/r/49352/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49352: Added a flag parser for hashset.

[Guangya Liu]

> On 六月 29, 2016, 4:59 p.m., Klaus Ma wrote:
> > src/common/parse.hpp, lines 150-152
> > 
> >
> > Call `insert` directly should be fine. `set` will help to avoid 
> > duplicated item.

Here I need to return error if there are diplicate items, seems I cannot use 
`set` for this as `set` do not have a method like `contain` which can enable me 
to check the duplicate items, comments?


- Guangya


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


On 六月 29, 2016, 12:12 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49352/
> ---
> 
> (Updated 六月 29, 2016, 12:12 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5743
> https://issues.apache.org/jira/browse/MESOS-5743
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a flag parser for hashset.
> 
> 
> Diffs
> -
> 
>   src/common/parse.hpp 19e56dbeb765f8bec92e0a3615f6f7c12466fa9e 
> 
> Diff: https://reviews.apache.org/r/49352/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 48904: Updated allocator initialize() to include fairnessExcludeResourceNames.

[Guangya Liu]

> On 六月 29, 2016, 5:13 p.m., Klaus Ma wrote:
> > src/master/master.cpp, line 830
> > 
> >
> > Please check whether the resource name is valid. We only support cpus, 
> > mem, disk, ports, gpus for now.

One question is that end user can specify customized resources when agent start 
up by adding some other resources, such as ` 
--resources="cpus:8;mem:8000;foo:10;bar:20"`, so here I'm not validating the 
resources, but just make the sorter ignore the resource names in the 
`allocator_fairness_excluded_resource_names`, what do you say?


- Guangya


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


On 六月 29, 2016, 12:16 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/48904/
> ---
> 
> (Updated 六月 29, 2016, 12:16 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-5620
> https://issues.apache.org/jira/browse/MESOS-5620
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated allocator initialize() to include fairnessExcludeResourceNames.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 98025bc182d92098bd4018177a6ae28135d8de95 
>   src/master/allocator/mesos/allocator.hpp 
> f096d2b7813580fb28e77a2803e290edf1ebda31 
>   src/master/allocator/mesos/hierarchical.hpp 
> 98a1f69f14b967c8d01f8a680771c9d28fac14e4 
>   src/master/allocator/mesos/hierarchical.cpp 
> c3639342335499a04a23147a4205f1b475c123fa 
>   src/master/master.cpp 907233b015919f437fb2ebd25875217930b301b4 
>   src/tests/allocator.hpp 41a31ae5d2c8fb8eb902a82d893be570db0da3bd 
>   src/tests/api_tests.cpp b3127b716480223a6f23b7908bf6bc1808120f80 
>   src/tests/master_allocator_tests.cpp 
> 7910f5532bf36ed92100839eac6c6f6a18838ffa 
>   src/tests/master_quota_tests.cpp b9bc49b1ef55d6ea57d94971905d70244f982e9f 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> 6c85e19eeaa69bf3a4e3077261331191db6eec06 
>   src/tests/reservation_endpoints_tests.cpp 
> 3ee59d5db0089dd59acfe48a77910d069ffc377b 
>   src/tests/reservation_tests.cpp d7e90bc67a55a909be70691a1108493c33743b02 
>   src/tests/resource_offers_tests.cpp 
> 046adaedf9121655f377f503bb30437803bf0005 
>   src/tests/slave_recovery_tests.cpp e6e7b8e3d71886eb8749122bd7b441857983d574 
> 
> Diff: https://reviews.apache.org/r/48904/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 49404: Changed the SocketManager to pass "Sockets" by value.

[Benjamin Mahler]

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


Fix it, then Ship it!




Great cleanup, thanks!


3rdparty/libprocess/src/process.cpp (line 1392)


Technically this changes the semantics in that if an old fd is already 
present we will no longer overwrite but that would have been a bug anyway. 
Ideally this code checks that the insertion happens:

```
  synchronized (mutex) {
auto result = sockets.emplace(socket, socket);
CHECK(result->second) << "Socket fd already present in map";
  }
```



3rdparty/libprocess/src/process.cpp (line 1556)


Seems we should check that this inserted:

```
// sockets[s] = socket.get()
CHECK(sockets.emplace(s, socket.get())->second);
```

Or less messy:

```
CHECK_EQ(0u, sockets.count(s));
sockets.emplace(s, socket.get());
```

It's a bit unfortunate that we lost readability.

Ideally we could check our invariants here for the other maps as well, feel 
free to punt since we're only interested in removing the socket pointer mess.



3rdparty/libprocess/src/process.cpp (lines 1580 - 1589)


I was initially mislead by the comment for `existing` that we made a copy 
for some kind of lifetime correctness, rather than just readability to passing 
an argument. Feel free to do this:

```
// Update all the data structures that are mapped to the old
// socket. They will now point to the new socket we are about
// to try to connect. The old socket should no longer have any
// references after the swap and should be closed.
Socket existing(sockets.at(persists.at(to.address)));
swap_implementing_socket(existing, socket.get());
```



3rdparty/libprocess/src/process.cpp (line 2020)


This should now be `socket.get()` because the intention is to print the fd. 
This works for you because of the int cast operator, but in the future if we 
added a different operator for printing sockets, the output here would silently 
change.



3rdparty/libprocess/src/process.cpp (line 2103)


Ditto here for `socket.get()`.


- Benjamin Mahler


On June 30, 2016, 1:11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49404/
> ---
> 
> (Updated June 30, 2016, 1:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-5748
> https://issues.apache.org/jira/browse/MESOS-5748
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `Sockets` is already a reference-counted `shared_ptr` under the covers.
> By passing around `Sockets` by value, we avoid potentially deleting
> a reference while the same reference is in use by another function.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 9bae71246e751e491be5a989eea8aca29c9aa751 
> 
> Diff: https://reviews.apache.org/r/49404/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX)
> 
> 3rdparty/libprocess/libprocess-tests 
> --gtest_filter="ProcessRemoteLinkTest.RemoteLink"  --gtest_break_on_failure 
> --gtest_repeat=1
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Review Request 49420: Updated GetFrameworks v1 call in master.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar, zhou xing, Jay Guo, and Zhitao Li.


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


Repository: mesos


Description
---

This change removes tasks and executors information from
GetFrameworks call because we can get that from GetTasks
and GetExecutors (not yet implemented) calls.


Diffs
-

  include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
  include/mesos/v1/master/master.proto 93157d57dcc53b54fed2ebbc4772c689ddba2119 
  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 

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


Testing
---

make check


Thanks,

Vinod Kone

Re: Review Request 49420: Updated GetFrameworks v1 call in master.

[Vinod Kone]

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

(Updated June 30, 2016, 2:09 a.m.)


Review request for mesos, Anand Mazumdar, zhou xing, Jay Guo, and Zhitao Li.


Changes
---

removed Executor from Framework.


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


Repository: mesos


Description
---

This change removes tasks and executors information from
GetFrameworks call because we can get that from GetTasks
and GetExecutors (not yet implemented) calls.


Diffs (updated)
-

  include/mesos/master/master.proto 2e5d6eeb3a960e4f41c382d65321f18bb05ed6be 
  include/mesos/v1/master/master.proto 93157d57dcc53b54fed2ebbc4772c689ddba2119 
  src/master/http.cpp e5acdb8e0bbcd7a2b7e8a8bc7f4bbeaae2c4fea1 

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


Testing
---

make check


Thanks,

Vinod Kone