Re: Review Request 47736: Used TaskObjectAllower to filter /tasks endpoint.

[Joerg Schad]

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

(Updated May 25, 2016, 5:21 a.m.)


Review request for mesos, Adam B and Michael Park.


Changes
---

Used collect(...) instead of collect(list).


Repository: mesos


Description
---

Used TaskObjectAllower to filter /tasks endpoint.


Diffs (updated)
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

Re: Review Request 47704: Used Tasked ObjectAllower to filter /state endpoint.

[Joerg Schad]

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

(Updated May 25, 2016, 5:21 a.m.)


Review request for mesos and Michael Park.


Changes
---

Used collect(..) instead of collect(list).


Repository: mesos


Description
---

Used Tasked ObjectAllower to filter /state endpoint.


Diffs (updated)
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

Make check (OSX)


Thanks,

Joerg Schad

Re: Review Request 47558: Added ObjectAllower interface to authorizer.

[Joerg Schad]

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

(Updated May 25, 2016, 5:19 a.m.)


Review request for mesos, Adam B and Michael Park.


Summary (updated)
-

Added ObjectAllower interface to authorizer.


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


Repository: mesos


Description (updated)
---

Added ObjectAllower interface to authorizer.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
ed5f9e73661e25a83722cf1e408ae61023cd4a21 
  src/authorizer/local/authorizer.hpp 61388454025211fd7d53e71a86983fd8479950b6 
  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
  src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
  src/tests/mesos.cpp 629135f0dc59346f0fcddb2cbe65ca5770fad34e 

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


Testing
---

tested entire chain.


Thanks,

Joerg Schad

Re: Review Request 47795: Enabled authorization for sandboxes.

[Adam B]

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



Great job! Just a few suggestions:
1. role -> user (to match /state filtering)
2. Add `optional FrameworkInfo` and `optionalExecutorInfo` to 
`authorization::Object` instead of serializing to string
3. Remove `_WITH_INFO` and add a clear comment about what to expect in the 
Object.
4. Pass the frameworkId to authorizeSandboxAccess

Should I expect a separate ACL/patch for VIEW_LOGS?


include/mesos/authorizer/acls.proto (line 183)


Just opening a discussion: Is "Access" the proper verb, vs. "Get" or "View"?
Perhaps, since one could also "Browse" the sandbox, which is not exactly a 
get/view.
I don't think it's necessary to have separate actions for 
Browse/Read/Download, but it would make the verbs obvious.
I'm ok with "Access", but would like to hear others' thoughts.



include/mesos/authorizer/acls.proto (line 184)


Subjects: HTTP username.



include/mesos/authorizer/acls.proto (line 186)


Please change this to "users" to match the /state filtering (WebUI should 
be consistent)
https://reviews.apache.org/r/46613/diff/9#0



include/mesos/authorizer/acls.proto (lines 221 - 222)


Not yours, but.. Please put these in numerical order. Deprecated fields 
already have the deprecated tag and a comment to distinguish them.
Misordered fields are more likely to confuse somebody into reusing an 
existing value.



include/mesos/authorizer/authorizer.proto (lines 58 - 59)


Not yours, but.. Please move these down to their spot in numerical order.



include/mesos/authorizer/authorizer.proto (line 69)


s/ACCESS_SANDBOX_WITH_INFO/ACCESS_SANDBOX/ since the INFO doesn't actually 
inform what kind of info you're passing in the Object.
Instead, we'll need a clear comment here (and in docs/ explaining what an 
authorizer module writer should expect in the Object (FwkInfo+ExecInfo).



src/slave/slave.cpp (lines 5385 - 5386)


Wouldn't this be a bit more efficient if you knew the desired frameworkId? 
I think `authorizeSandboxAccess()` should also take frameworkId as a parameter.



src/slave/slave.cpp (line 5388)


No need to use Joerg's filter/allower here. There's only 1 
Framework+ExecutorInfo to copy here, and it's probably much smaller than the 
file the user wants to download, so we can spare the copy for clarity.
You can remove this TODO.



src/slave/slave.cpp (lines 5396 - 5400)


Rather than stringifying, let's just add `optional FrameworkInfo` and 
`optional ExecutorInfo` to `authorization::Object` like in:
https://reviews.apache.org/r/46613/diff/9#1


- Adam B


On May 24, 2016, 2:41 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47795/
> ---
> 
> (Updated May 24, 2016, 2:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables authorization of the sandboxes using the callback function
> parameter of `Files::attach()`.
> 
> It also adds relevant ACLs and support on the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47795/diff/
> 
> 
> Testing
> ---
> 
> on OSX the script:
> 
> ```bash
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "access_sandboxes" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "roles" : { "values" : ["test"] }
> }
>   ]
> }
> EOF
> 
> ./mesos-master.sh --work_dir=/tmp/mesos/master &
> ./mesos-slave.sh --work_dir=/tmp/mesos/slave \
>  --master=127.0.0.1:5050 \
>  

Re: Review Request 47736: Used TaskObjectAllower to filter /tasks endpoint.

[Joerg Schad]

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

(Updated May 25, 2016, 4:29 a.m.)


Review request for mesos, Adam B and Michael Park.


Summary (updated)
-

Used TaskObjectAllower to filter /tasks endpoint.


Repository: mesos


Description (updated)
---

Used TaskObjectAllower to filter /tasks endpoint.


Diffs (updated)
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

Re: Review Request 47704: Used Tasked ObjectAllower to filter /state endpoint.

[Joerg Schad]

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

(Updated May 25, 2016, 4:28 a.m.)


Review request for mesos and Michael Park.


Summary (updated)
-

Used Tasked ObjectAllower to filter /state endpoint.


Repository: mesos


Description (updated)
---

Used Tasked ObjectAllower to filter /state endpoint.


Diffs (updated)
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

Make check (OSX)


Thanks,

Joerg Schad

Re: Review Request 47559: Added authorization based filtering to /state-summary.

[Joerg Schad]

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

(Updated May 25, 2016, 4:27 a.m.)


Review request for mesos, Adam B and Michael Park.


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


Repository: mesos


Description
---

Added authorization based filtering to /state-summary.


Diffs (updated)
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

make check + (sudo) make check on various linux systems


Thanks,

Joerg Schad

Re: Review Request 47558: Added Allower interface to authorizer.

[Joerg Schad]

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

(Updated May 25, 2016, 4:26 a.m.)


Review request for mesos, Adam B and Michael Park.


Summary (updated)
-

Added Allower interface to authorizer.


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


Repository: mesos


Description (updated)
---

Added Allower interface to authorizer.


Diffs (updated)
-

  include/mesos/authorizer/authorizer.hpp 
ed5f9e73661e25a83722cf1e408ae61023cd4a21 
  src/authorizer/local/authorizer.hpp 61388454025211fd7d53e71a86983fd8479950b6 
  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
  src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
  src/tests/mesos.cpp 629135f0dc59346f0fcddb2cbe65ca5770fad34e 

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


Testing
---

tested entire chain.


Thanks,

Joerg Schad

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline.

[Dario Rexin]

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

(Updated May 25, 2016, 4:09 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Fixed "Bugs", "People" and "Summary". -- @vinodkone


Summary (updated)
-

Fixed a memory leak in SchedulerProcess.decline.


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


Repository: mesos


Description
---

MesosScheduler.declineOffers has been changed ~6 months ago to send a
Decline message instead of calling acceptOffers with an empty list of
task infos. The changed version of declineOffer however did not remove
the offerId from the savedOffers map, causing a memory leak.


Diffs
-

  src/sched/sched.cpp 9e55885 

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


Testing
---

make check


Thanks,

Dario Rexin

Re: Review Request 47068: Added fine-grained filtering master flag.

[Joerg Schad]

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

(Updated May 25, 2016, 4:02 a.m.)


Review request for mesos, Adam B, Alexander Rojas, and Michael Park.


Repository: mesos


Description
---

Added a master flag enabling the fine-grained
authorization based filtering of HTTP endpoints.
The default for this setting is `false` as the filtering incurs
a performance penalty.


Diffs
-

  docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
  docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
  src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
  src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 

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


Testing
---

tested entire chain.


Thanks,

Joerg Schad

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Dario Rexin]

> On May 25, 2016, 1:35 a.m., Vinod Kone wrote:
> > src/sched/sched.cpp, line 1353
> > 
> >
> > Can you log the warning like we do in acceptOffers()?
> > 
> > ```
> > if (!savedOffers.contains(offerId)) {
> >   LOG(WARNING) << "Attempting to decline an unknown offer << offerId;
> > }
> > 
> > savedOffers.erase(offerId);
> > 
> > ```

Thanks for the feedback. I updated the patch accordingly.


- Dario


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


On May 25, 2016, 3:45 a.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 25, 2016, 3:45 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a
> Decline message instead of calling acceptOffers with an empty list of
> task infos. The changed version of declineOffer however did not remove
> the offerId from the savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 47805: Add authorization to GET /weights.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47805]

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 May 25, 2016, 2:04 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47805/
> ---
> 
> (Updated May 25, 2016, 2:04 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: mesos-5335
> https://issues.apache.org/jira/browse/mesos-5335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add 'GET_WEIGHTS_WITH_ROLE' for the authorization of GET /weights.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/weights.md 1e540795c5fb90c8ea957dd444b9e564e0b1ac23 
>   include/mesos/authorizer/acls.proto 
> b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
>   src/master/master.hpp 1a875c32eddfb6d884e3d0dda7f5716ee53966c3 
>   src/master/weights_handler.cpp 4bc060fdb015df6658194eef92fe11b14aa15c79 
>   src/tests/dynamic_weights_tests.cpp 
> 362c59aae7b305710d5985bfec28f881be3b64b8 
> 
> Diff: https://reviews.apache.org/r/47805/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Dario Rexin]

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

(Updated May 25, 2016, 3:45 a.m.)


Review request for mesos.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449


Repository: mesos


Description (updated)
---

MesosScheduler.declineOffers has been changed ~6 months ago to send a
Decline message instead of calling acceptOffers with an empty list of
task infos. The changed version of declineOffer however did not remove
the offerId from the savedOffers map, causing a memory leak.


Diffs (updated)
-

  src/sched/sched.cpp 9e55885 

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


Testing
---

make check


Thanks,

Dario Rexin

Re: Review Request 47794: Added authorization support for mesos::internal::Files.

[Guangya Liu]

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




src/files/files.cpp (lines 66 - 67)


ajdust order



src/files/files.cpp (line 92)


blank line above


- Guangya Liu


On 五月 24, 2016, 9:39 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47794/
> ---
> 
> (Updated 五月 24, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds an optional parameter to the `mesos::internal::Files::attach()`
> method. The type of this parameter is a callable object which returns
> a future to a boolean and takes as parameter an optional string
> representing a principal name.
> 
> The parameter is called, if set, whenever one of the routed endpoints
> of the `Files` object is accessed through HTTP. If the callable object
> returns a false boolean, then processing of the request is aborted
> and a `403 Forbidden` response is returned.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 90acb3406c46c164108deb559af71fb109a5773b 
>   src/files/files.cpp e4b0ada00aabba6553810391f4015a896f8a69a5 
>   src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 
> 
> Diff: https://reviews.apache.org/r/47794/diff/
> 
> 
> Testing
> ---
> 
> On OSX:
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47794: Added authorization support for mesos::internal::Files.

[Adam B]

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



Looking pretty good. Just a couple of comments about wrapping/indentation, and 
some Options that no longer need to be optional.


src/files/files.cpp (lines 152 - 153)


I'd love a comment explaining what the key/value represent here



src/files/files.cpp (line 304)


s/attached/requestPath/? since it's not necessarily the attached path until 
it is stripped to something that exists in `authorizations`. In fact, they may 
have requested a path that doesn't exist
Ditto elsewhere



src/files/files.cpp (line 307)


Are we guaranteed that this will always be false on the first iteration? 
What if the requested path is "/" or "dirname"?
Perhaps this should be a `do{}while()`?



src/files/files.cpp (lines 309 - 310)


Can you wrap on `.then(` instead, please?



src/files/files.cpp (line 327)


`path` is guaranteed to be Some by here, so you can remove the `Option`



src/files/files.cpp (lines 436 - 439)


This wrapping/indentation feels off.
First of all, we usually put `.then(...` on a new line, indented 2 spaces 
from the previous.
Secondly, if the `bool authorize...{` was on the same line as the previous, 
then `if (authorized) {` would only be indented two spaces in from `.then(`, so 
I'd think that should hold, even if we have to wrap the `bool authorized)...{` 
line.

Perhaps:
```
return authorizations[attached](principal)
  .then([this, offset, length, path, jsonp](bool authorized) 
  -> Future {
if (authorized) {
```



src/tests/files_tests.cpp (line 334)


s/reads/browse/



src/tests/files_tests.cpp (line 390)


s/reads/download/


- Adam B


On May 24, 2016, 2:39 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47794/
> ---
> 
> (Updated May 24, 2016, 2:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds an optional parameter to the `mesos::internal::Files::attach()`
> method. The type of this parameter is a callable object which returns
> a future to a boolean and takes as parameter an optional string
> representing a principal name.
> 
> The parameter is called, if set, whenever one of the routed endpoints
> of the `Files` object is accessed through HTTP. If the callable object
> returns a false boolean, then processing of the request is aborted
> and a `403 Forbidden` response is returned.
> 
> 
> Diffs
> -
> 
>   src/files/files.hpp 90acb3406c46c164108deb559af71fb109a5773b 
>   src/files/files.cpp e4b0ada00aabba6553810391f4015a896f8a69a5 
>   src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 
> 
> Diff: https://reviews.apache.org/r/47794/diff/
> 
> 
> Testing
> ---
> 
> On OSX:
> `make check`
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47795: Enabled authorization for sandboxes.

[Guangya Liu]

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




include/mesos/authorizer/acls.proto (lines 182 - 187)


Some comments as others?



include/mesos/authorizer/authorizer.proto (line 69)


New line above



src/authorizer/local/authorizer.cpp (lines 19 - 20)


adjust the order



src/authorizer/local/authorizer.cpp (lines 278 - 303)


I think that the {} here is not needed.

case authorization::ACCESS_SANDBOX_WITH_INFO:
  authorization::Request realRequest;
  ...
  return authorized(realRequest, acls_);
  break;



src/authorizer/local/authorizer.cpp (lines 279 - 292)


Why not use `request` directly? Can you please show more detail or add some 
comments here?



src/authorizer/local/authorizer.cpp (line 294)


suggest use `foreach` to keep consistent with others


- Guangya Liu


On 五月 24, 2016, 9:41 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47795/
> ---
> 
> (Updated 五月 24, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables authorization of the sandboxes using the callback function
> parameter of `Files::attach()`.
> 
> It also adds relevant ACLs and support on the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47795/diff/
> 
> 
> Testing
> ---
> 
> on OSX the script:
> 
> ```bash
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "access_sandboxes" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "roles" : { "values" : ["test"] }
> }
>   ]
> }
> EOF
> 
> ./mesos-master.sh --work_dir=/tmp/mesos/master &
> ./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 &
> 
> ./mesos-execute --command='while true; do echo "Hello world"; sleep 3; done' \
> --role=test \
> --master=127.0.0.1:5050 \
> --name=echoer &
> 
> SANDBOX_VPATH=`http GET http://127.0.0.1:5051/files/debug -a foo:bar -b  
> --pretty=none \
>  | python -c 'import json,sys;obj=json.load(sys.stdin);print 
> obj.keys()[0]'`
> 
> # This should yield a 200 OK response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
> foo:bar
> 
> # HTTP/1.1 200 OK
> # Content-Disposition: attachment; filename=stdout
> # Content-Length: 3267
> # Content-Type: application/octet-stream
> # Date: Fri, 20 May 2016 13:52:31 GMT
> #
> # Received SUBSCRIBED event
> # Subscribed executor on localhost
> # Received LAUNCH event
> # Starting task echoer
> # sh -c 'while true; do echo "Hello world"; sleep 3; done'
> # Forked command at 26162
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> 
> # This shold yield a 403 Forbidden response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
> baz:bar
> 
> # HTTP/1.1 403 Forbidden
> # Content-Length: 0
> # Date: Fri, 20 May 2016 13:52:37 GMT
> #
> #
> #
> 
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47558: Added allower interface to authorizer.

[Joerg Schad]

> On May 24, 2016, 8:39 p.m., Alexander Rojas wrote:
> > src/authorizer/local/authorizer.cpp, line 116
> > 
> >
> > This method is reentrant and it doesn't depends on anything but its 
> > parameters, so it would be better to make it `static` in the local 
> > authorizer and call that one instead of copying paying the whole code.

already captured that implicitly in the mental comment above for allows.


- Joerg


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


On May 22, 2016, 9:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47558/
> ---
> 
> (Updated May 22, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Bugs: MESOS-5403
> https://issues.apache.org/jira/browse/MESOS-5403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allower interface to authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ed5f9e73661e25a83722cf1e408ae61023cd4a21 
>   src/authorizer/local/authorizer.hpp 
> 61388454025211fd7d53e71a86983fd8479950b6 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
>   src/tests/mesos.cpp 629135f0dc59346f0fcddb2cbe65ca5770fad34e 
> 
> Diff: https://reviews.apache.org/r/47558/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47360: Updated dynamic reservation and persistent volume docs.

[Guangya Liu]

> On 五月 14, 2016, 1:55 p.m., Guangya Liu wrote:
> > docs/persistent-volume.md, line 96
> > 
> >
> > Can you please show more detail for `may take any value, or may be 
> > omitted.`
> > 
> > a) In which condition can take any value if the framework did not 
> > provide a principal.
> > b) In which condition the `principal` will be omitted if the framework 
> > did not provide a principal.
> > 
> > Ditto for others.
> 
> Greg Mann wrote:
> I changed the text slightly to clarify my meaning. This text is saying 
> that if frameworkInfo.principal is not provided, then in all cases 
> disk.persistence.principal can take any value or can be left unset. Let me 
> know if my changes don't make this any clearer!

Thanks Greg, one minor comment for this: can we simplify the sentense as that 
the `principal` will be ignored for such case?


- Guangya


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


On 五月 24, 2016, 10:09 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47360/
> ---
> 
> (Updated 五月 24, 2016, 10:09 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Neil Conway.
> 
> 
> Bugs: MESOS-5215
> https://issues.apache.org/jira/browse/MESOS-5215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the documentation of RESERVE and
> CREATE operations, both via offer operations and
> operator endpoints. Specifically, we clarify the
> Mesos master's expectations for the values of the
> `principal` fields found in `ReservationInfo` and
> `DiskInfo.Persistence`.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md c13d79124f0b5ea2a715b4d2990fda4e06b2fb02 
>   docs/reservation.md a400d19aec7a48d122ba1c9c23d38d792b8dbe6f 
> 
> Diff: https://reviews.apache.org/r/47360/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the Mesos website container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

Error:
No reviewers specified. Please find a reviewer by asking on JIRA or the mailing 
list.

- Mesos ReviewBot


On May 25, 2016, 12:51 a.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 25, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
> message instead of calling acceptOffers with an empty list of task infos. The 
> changed version of declineOffer however did not remove the offerId from the 
> savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 46097: Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".

[haosdent huang]

> On May 17, 2016, 1:27 p.m., haosdent huang wrote:
> > src/Makefile.am, line 2048
> > 
> >
> > I think you forgot to update the CMake build files.
> 
> Qian Zhang wrote:
> I see all the tests are not included in `src/CMakeLists.txt`, so it seem 
> CMake build has not enabled tests yet?

Sry, my bad. Only libprocess/stout enable tests in CMake.


- haosdent


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


On May 12, 2016, 8:11 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46097/
> ---
> 
> (Updated May 12, 2016, 8:11 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5167
> https://issues.apache.org/jira/browse/MESOS-5167
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the test "CniIsolatorTest.ROOT_LaunchCommandTask".
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
>   src/tests/containerizer/cni_isolator_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46097/diff/
> 
> 
> Testing
> ---
> 
> [ RUN  ] CniIsolatorTest.ROOT_LaunchCommandTask
> + /home/stack/workspace/mesos/build/src/mesos-containerizer mount 
> --help=false --operation=make-rslave --path=/
> + grep+  -E /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/.+ 
> /proc/self/mountinfo
> + grepcut -v -d  -f5
>  d06b117d-518b-41e2-b8e0-62a12083773c
> + xargs --no-run-if-empty umount -l
> + mount -n --rbind 
> /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/provisioner/containers/d06b117d-518b-41e2-b8e0-62a12083773c/backends/copy/rootfses/7ea27011-cd3a-43b0-8301-b0b94d9f9b47
>  
> /tmp/CniIsolatorTest_ROOT_LaunchCommandTask_HRK4Dz/slaves/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0/frameworks/18dea042-5bb5-4336-8bc8-358ed1fbf6dd-/executors/60e6d35d-6d33-47ae-9c23-d2e5c913c892/runs/d06b117d-518b-41e2-b8e0-62a12083773c/.rootfs
> I0420 22:26:00.924844  9305 exec.cpp:150] Version: 0.29.0
> I0420 22:26:00.942319  9375 exec.cpp:225] Executor registered on agent 
> 18dea042-5bb5-4336-8bc8-358ed1fbf6dd-S0
> Registered executor on mesos
> Starting task 60e6d35d-6d33-47ae-9c23-d2e5c913c892
> Forked command at 9382
> sh -c 'ls /'
> bin  dev  etc  home lib  linuxrc  mediamnt  proc  
>root run  sbin sys  tmp  usr  var
> Command exited with status 0 (pid: 9382)
> I0420 22:26:01.098331  9380 exec.cpp:399] Executor asked to shutdown
> [   OK ] CniIsolatorTest.ROOT_LaunchCommandTask (42603 ms)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 47805: Add authorization to GET /weights.

[zhou xing]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Add 'GET_WEIGHTS_WITH_ROLE' for the authorization of GET /weights.


Diffs
-

  docs/endpoints/master/weights.md 1e540795c5fb90c8ea957dd444b9e564e0b1ac23 
  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 
  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
  src/master/master.hpp 1a875c32eddfb6d884e3d0dda7f5716ee53966c3 
  src/master/weights_handler.cpp 4bc060fdb015df6658194eef92fe11b14aa15c79 
  src/tests/dynamic_weights_tests.cpp 362c59aae7b305710d5985bfec28f881be3b64b8 

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


Testing
---

make
make check


Thanks,

zhou xing

Re: Review Request 47360: Updated dynamic reservation and persistent volume docs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47359, 47360]

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 May 24, 2016, 10:09 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47360/
> ---
> 
> (Updated May 24, 2016, 10:09 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Neil Conway.
> 
> 
> Bugs: MESOS-5215
> https://issues.apache.org/jira/browse/MESOS-5215
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch updates the documentation of RESERVE and
> CREATE operations, both via offer operations and
> operator endpoints. Specifically, we clarify the
> Mesos master's expectations for the values of the
> `principal` fields found in `ReservationInfo` and
> `DiskInfo.Persistence`.
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md c13d79124f0b5ea2a715b4d2990fda4e06b2fb02 
>   docs/reservation.md a400d19aec7a48d122ba1c9c23d38d792b8dbe6f 
> 
> Diff: https://reviews.apache.org/r/47360/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the Mesos website container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Vinod Kone]

> On May 25, 2016, 1:27 a.m., Joseph Wu wrote:
> > src/sched/sched.cpp, line 325
> > 
> >
> > On inspection, there's another (rarer) leak that can appear if:
> > 
> > 1) Master sends offers to the framework.
> > 2) Network partition disconnects the framework.
> > 3) Framework uses `acceptOffers` or `declinesOffers`.
> > 4) Because the framework is `!connected`, both `acceptOffers` and 
> > `declinesOffers` exit early and do not modify `savedOffers`.
> > 
> > It would be correct to clear `savedOffers` when the framework 
> > disconnects.  (Because all offers become invalid.)
> 
> haosdent huang wrote:
> Nice comment!

Lets do this in a follow up review to keep this review on point.


- Vinod


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


On May 25, 2016, 12:51 a.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 25, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
> message instead of calling acceptOffers with an empty list of task infos. The 
> changed version of declineOffer however did not remove the offerId from the 
> savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Vinod Kone]

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


Fix it, then Ship it!




Thanks for catching this.


src/sched/sched.cpp (line 1353)


Can you log the warning like we do in acceptOffers()?

```
if (!savedOffers.contains(offerId)) {
  LOG(WARNING) << "Attempting to decline an unknown offer << offerId;
}

savedOffers.erase(offerId);

```


- Vinod Kone


On May 25, 2016, 12:51 a.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 25, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
> message instead of calling acceptOffers with an empty list of task infos. The 
> changed version of declineOffer however did not remove the offerId from the 
> savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[haosdent huang]

> On May 25, 2016, 1:27 a.m., Joseph Wu wrote:
> > src/sched/sched.cpp, line 325
> > 
> >
> > On inspection, there's another (rarer) leak that can appear if:
> > 
> > 1) Master sends offers to the framework.
> > 2) Network partition disconnects the framework.
> > 3) Framework uses `acceptOffers` or `declinesOffers`.
> > 4) Because the framework is `!connected`, both `acceptOffers` and 
> > `declinesOffers` exit early and do not modify `savedOffers`.
> > 
> > It would be correct to clear `savedOffers` when the framework 
> > disconnects.  (Because all offers become invalid.)

Nice comment!


- haosdent


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


On May 25, 2016, 12:51 a.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 25, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
> message instead of calling acceptOffers with an empty list of task infos. The 
> changed version of declineOffer however did not remove the offerId from the 
> savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Gilbert Song]

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

(Updated May 24, 2016, 6:33 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented support for passing agent default docker config.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/docker/store.cpp 
eeec94326a4fd67675df10e0b6a32267e555fa96 
  src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub. Both cases are 
tested:
1. --docker_registry=private_registry
2. private_registry/repo


Thanks,

Gilbert Song

Review Request 47807: Implemented parsing a docker config to a hashmap.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Implemented parsing a docker config to a hashmap.


Diffs
-

  include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45951: Implemented http basic auth to get docker auth token.

[Gilbert Song]

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

(Updated May 24, 2016, 6:33 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented http basic auth to get docker auth token.


Diffs (updated)
-

  src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 

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


Testing
---

make check

Tested with mesos-execute, using a private repo from docker hub.


Thanks,

Gilbert Song

Review Request 47806: Add docker config auth protobuf to docker spec.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Add docker config auth protobuf to docker spec.


Diffs
-

  include/mesos/docker/spec.proto 4f2ff2d7d88ceca2f25864b60a04ff5ec05317ff 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 47808: Added test for parsing docker config.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Added test for parsing docker config.


Diffs
-

  src/tests/containerizer/docker_spec_tests.cpp 
796b020f58f8451362bc1357ab6d7ceb4e946b3c 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45950: Added test for docker spec get credential helper.

[Gilbert Song]

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

(Updated May 24, 2016, 6:31 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Added test for docker spec get credential helper.


Diffs (updated)
-

  src/tests/containerizer/docker_spec_tests.cpp 
796b020f58f8451362bc1357ab6d7ceb4e946b3c 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45949: Implemented docker config get credential helper.

[Gilbert Song]

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

(Updated May 24, 2016, 6:31 p.m.)


Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
Chen.


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


Repository: mesos


Description
---

Implemented docker config get credential helper.


Diffs (updated)
-

  include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Joseph Wu]

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


Fix it, then Ship it!




Note: You'll need add newlines to your description (< 72 characters per line).


src/sched/sched.cpp (line 325)


On inspection, there's another (rarer) leak that can appear if:

1) Master sends offers to the framework.
2) Network partition disconnects the framework.
3) Framework uses `acceptOffers` or `declinesOffers`.
4) Because the framework is `!connected`, both `acceptOffers` and 
`declinesOffers` exit early and do not modify `savedOffers`.

It would be correct to clear `savedOffers` when the framework disconnects.  
(Because all offers become invalid.)



src/sched/sched.cpp (line 1353)


Nit: Add a comment like:
```
// Remove the offer.  We do not need to save any PIDs when declining an 
offer.
```


- Joseph Wu


On May 24, 2016, 5:51 p.m., Dario Rexin wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47804/
> ---
> 
> (Updated May 24, 2016, 5:51 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: https://issues.apache.org/jira/browse/MESOS-5449
> 
> https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
> message instead of calling acceptOffers with an empty list of task infos. The 
> changed version of declineOffer however did not remove the offerId from the 
> savedOffers map, causing a memory leak.
> 
> 
> Diffs
> -
> 
>   src/sched/sched.cpp 9e55885 
> 
> Diff: https://reviews.apache.org/r/47804/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Dario Rexin
> 
>

Review Request 47804: Fixed a memory leak in SchedulerProcess.decline

[Dario Rexin]

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

Review request for mesos.


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

https://issues.apache.org/jira/browse/https://issues.apache.org/jira/browse/MESOS-5449


Repository: mesos


Description
---

MesosScheduler.declineOffers has been changed ~6 months ago to send a Decline 
message instead of calling acceptOffers with an empty list of task infos. The 
changed version of declineOffer however did not remove the offerId from the 
savedOffers map, causing a memory leak.


Diffs
-

  src/sched/sched.cpp 9e55885 

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


Testing
---

make check


Thanks,

Dario Rexin

Re: Review Request 47486: Windows: Escaped command line arguments.

[Daniel Pravat]

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

(Updated May 25, 2016, 12:47 a.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joris 
Van Remoortere, and Michael Park.


Repository: mesos


Description
---

The command line for the containerizer has the command encoded
as JSON. Non escaped quotes are removed during the containerizer
startup and the JSON processed is invalid.


Diffs (updated)
-

  3rdparty/libprocess/include/process/windows/subprocess.hpp 
551770b9ec24b880c13441f413c5d1871fbf5c3a 

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


Testing
---

Windows: build/run


Thanks,

Daniel Pravat

Re: Review Request 47486: Windows: Escaped command line arguments.

[Daniel Pravat]

> On May 24, 2016, 11:49 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/include/process/windows/subprocess.hpp, lines 134-138
> > 
> >
> > Can you explain why `strings::replace` doesn't work here? Why do we 
> > need to manually implement it?

It will work.


- Daniel


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


On May 19, 2016, 11:07 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47486/
> ---
> 
> (Updated May 19, 2016, 11:07 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joris 
> Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The command line for the containerizer has the command encoded
> as JSON. Non escaped quotes are removed during the containerizer
> startup and the JSON processed is invalid.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> 551770b9ec24b880c13441f413c5d1871fbf5c3a 
> 
> Diff: https://reviews.apache.org/r/47486/diff/
> 
> 
> Testing
> ---
> 
> Windows: build/run
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 47795: Enabled authorization for sandboxes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47794, 47795]

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 May 24, 2016, 9:41 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47795/
> ---
> 
> (Updated May 24, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables authorization of the sandboxes using the callback function
> parameter of `Files::attach()`.
> 
> It also adds relevant ACLs and support on the authorizer interface.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47795/diff/
> 
> 
> Testing
> ---
> 
> on OSX the script:
> 
> ```bash
> #! /usr/bin/env bash
> 
> rm -rf /tmp/mesos/*
> 
> cat < /tmp/credentials.txt
> foo bar
> baz bar
> EOF
> 
> cat < /tmp/acls.json
> {
>   "permissive": false,
>   "access_sandboxes" : [
> {
>   "principals" : { "values" : ["foo"] },
>   "roles" : { "values" : ["test"] }
> }
>   ]
> }
> EOF
> 
> ./mesos-master.sh --work_dir=/tmp/mesos/master &
> ./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 &
> 
> ./mesos-execute --command='while true; do echo "Hello world"; sleep 3; done' \
> --role=test \
> --master=127.0.0.1:5050 \
> --name=echoer &
> 
> SANDBOX_VPATH=`http GET http://127.0.0.1:5051/files/debug -a foo:bar -b  
> --pretty=none \
>  | python -c 'import json,sys;obj=json.load(sys.stdin);print 
> obj.keys()[0]'`
> 
> # This should yield a 200 OK response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
> foo:bar
> 
> # HTTP/1.1 200 OK
> # Content-Disposition: attachment; filename=stdout
> # Content-Length: 3267
> # Content-Type: application/octet-stream
> # Date: Fri, 20 May 2016 13:52:31 GMT
> #
> # Received SUBSCRIBED event
> # Subscribed executor on localhost
> # Received LAUNCH event
> # Starting task echoer
> # sh -c 'while true; do echo "Hello world"; sleep 3; done'
> # Forked command at 26162
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> # Hello world
> 
> # This shold yield a 403 Forbidden response
> http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
> baz:bar
> 
> # HTTP/1.1 403 Forbidden
> # Content-Length: 0
> # Date: Fri, 20 May 2016 13:52:37 GMT
> #
> #
> #
> 
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47577: Agent: Added minor changes to various .cpp files to support Windows.

[Joris Van Remoortere]

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




src/docker/docker.cpp (lines 451 - 452)


we can rename these as we did the others:
`stdout_fd` -> `_stdout`.



src/files/files.cpp (lines 560 - 561)


Did we rename this to `PATH_SEPARATOR`?
Does this need to be rebased?



src/health-check/main.cpp (lines 278 - 317)


These changes are not relevant any more.
I think this review needs to be rebased.



src/logging/logging.cpp (line 203)


Indentation. Missing period. Don't need the parantheses, wrap with 
backticks instead.



src/uri/fetcher.hpp (lines 42 - 48)


Not sure what's going on here. Add a comment? Is this still relevant?



src/uri/fetcher.cpp (lines 49 - 52)


Can you reference this JIRA you created to add support for hadoop files?

It seems you also need one for docker?



src/zookeeper/group.cpp (line 659)


rebase.



src/zookeeper/group.cpp (line 700)


rebase.


- Joris Van Remoortere


On May 19, 2016, 4:07 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47577/
> ---
> 
> (Updated May 19, 2016, 4:07 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added minor changes to various .cpp files to support Windows.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 
>   src/docker/executor.cpp d60addcbe4a1869945ce42f4bb4b1e80e3f29f19 
>   src/files/files.cpp 4e916101b378b0e9032a08a3f6c73e195b2a08a1 
>   src/health-check/main.cpp 98ea5d3675f088e3a341037dcee92695e4857999 
>   src/logging/logging.cpp 20d2f6341bd39fc5d056f1046d258d006fc602e4 
>   src/uri/fetcher.hpp 8af2c9122e0b15fd54f7d3a84779540e7186f566 
>   src/uri/fetcher.cpp aa9df5d0256a26b8684934c2bd37b82a069088f7 
>   src/uri/fetchers/copy.cpp 2180adfba1f33852d11069eed9d9bca72e2e3b6f 
>   src/uri/fetchers/curl.cpp c4420623d718d87776f2eb8e13faf02ef5edb335 
>   src/zookeeper/group.cpp 01680899778e554af70b176db82498ca92b51b60 
> 
> Diff: https://reviews.apache.org/r/47577/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47602: Stout:[1/2] Added Windows support for folder `launcher/`.

[Joris Van Remoortere]

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




3rdparty/stout/include/stout/windows.hpp (lines 420 - 431)


Can you provide a little more context for these?
I see what you're doing, not sure of why or what it impacts.
Thanks!


- Joris Van Remoortere


On May 19, 2016, 4:08 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47602/
> ---
> 
> (Updated May 19, 2016, 4:08 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout:[1/2] Added Windows support for folder `launcher/`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/windows.hpp 
> a7a59e78575e1456b4e14d18ac97f51dd23d794e 
> 
> Diff: https://reviews.apache.org/r/47602/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47486: Windows: Escaped command line arguments.

[Joris Van Remoortere]

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




3rdparty/libprocess/include/process/windows/subprocess.hpp (lines 134 - 138)


Can you explain why `strings::replace` doesn't work here? Why do we need to 
manually implement it?


- Joris Van Remoortere


On May 19, 2016, 11:07 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47486/
> ---
> 
> (Updated May 19, 2016, 11:07 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joris 
> Van Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The command line for the containerizer has the command encoded
> as JSON. Non escaped quotes are removed during the containerizer
> startup and the JSON processed is invalid.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp 
> 551770b9ec24b880c13441f413c5d1871fbf5c3a 
> 
> Diff: https://reviews.apache.org/r/47486/diff/
> 
> 
> Testing
> ---
> 
> Windows: build/run
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 47576: Agent: Add Windows support to the containerizer.

[Joris Van Remoortere]

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




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


Please us `os::chown` in the comment here to be consistent with your 
previous comments.

Same with the other comments in this review.



src/slave/containerizer/external_containerizer.cpp (lines 641 - 645)


Can we please flip this to have the windows version come second?
Same below.



src/slave/containerizer/mesos/containerizer.cpp (line 59)


Why are we not using quotes like all the other lines?



src/slave/containerizer/mesos/containerizer.cpp (lines 285 - 287)


Quick comment saying that the network isolator is currently not supported 
on windows.

Can you add (and reference in the comment) a JIRA to update the autoconf to 
catch this case during configure?



src/slave/containerizer/mesos/containerizer.cpp (lines 1170 - 1174)


Update once we re-organize to `os::pipe`.



src/slave/containerizer/mesos/launcher.hpp (lines 117 - 118)


need an extra new line



src/slave/containerizer/mesos/launcher.hpp (lines 118 - 119)


Is this the only difference?
It would be great if we could clearly identify the differences, and 
reference a JIRA here to keep track of them.


- Joris Van Remoortere


On May 19, 2016, 2:47 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47576/
> ---
> 
> (Updated May 19, 2016, 2:47 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3617, MESOS-3618, MESOS-3619, MESOS-3622, MESOS-3623, MESOS-3624, 
> MESOS-3681, MESOS-3682, and MESOS-3684
> https://issues.apache.org/jira/browse/MESOS-3617
> https://issues.apache.org/jira/browse/MESOS-3618
> https://issues.apache.org/jira/browse/MESOS-3619
> https://issues.apache.org/jira/browse/MESOS-3622
> https://issues.apache.org/jira/browse/MESOS-3623
> https://issues.apache.org/jira/browse/MESOS-3624
> https://issues.apache.org/jira/browse/MESOS-3681
> https://issues.apache.org/jira/browse/MESOS-3682
> https://issues.apache.org/jira/browse/MESOS-3684
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Add Windows support to the containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 7af14f4472283ceefd73c06dd8df60af4cf6f7e8 
>   src/slave/containerizer/external_containerizer.cpp 
> cf4384cce44172a028c890f52f71ceb8ae109383 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 75e5a32a3e70ec60a6800e21a621673184ea0956 
>   src/slave/containerizer/mesos/launcher.hpp 
> 5977c30c0aacc569019f7b34bb0c6577823ec887 
>   src/slave/containerizer/mesos/launcher.cpp 
> a5c8c31b72773d0bd10b9d02675a01f1d641d41c 
> 
> Diff: https://reviews.apache.org/r/47576/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46418: Refactored the `os::access` function between POSIX and Windows.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46418]

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 May 24, 2016, 9:36 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46418/
> ---
> 
> (Updated May 24, 2016, 9:36 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Alex Clemmer, and Joris Van 
> Remoortere.
> 
> 
> Bugs: MESOS-5237
> https://issues.apache.org/jira/browse/MESOS-5237
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Refactored the `os::access` function between POSIX and Windows.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am ae4116b02474f2228d86ad4a9bbf69f9c1bb4342 
>   3rdparty/stout/include/stout/os.hpp 
> 2a306ae87422300b586e9f627a3f125d8faac4db 
>   3rdparty/stout/include/stout/os/access.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/posix/os.hpp 
> 1a66184d86d6a0a13b8facfc9f969e09dd824725 
>   3rdparty/stout/include/stout/windows/os.hpp 
> 71a8c61335e2bce340447e94e855a6bd79c49dec 
> 
> Diff: https://reviews.apache.org/r/46418/diff/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 47536: Agent: Added Windows isolators.

[Joris Van Remoortere]

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




src/slave/containerizer/mesos/isolators/filesystem/posix.cpp (line 130)


Why?
What about the code below isn't supported on windows?



src/slave/containerizer/mesos/isolators/filesystem/windows.hpp (lines 31 - 38)


Can you explain what we are accomplishing here by making this proxy?


- Joris Van Remoortere


On May 19, 2016, 2:50 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47536/
> ---
> 
> (Updated May 19, 2016, 2:50 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3619, MESOS-3620 and MESOS-3683
> https://issues.apache.org/jira/browse/MESOS-3619
> https://issues.apache.org/jira/browse/MESOS-3620
> https://issues.apache.org/jira/browse/MESOS-3683
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added Windows isolators.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.hpp 
> c6cea98e16f2bdea2da0220c235468080bbcd17b 
>   src/slave/containerizer/mesos/isolators/filesystem/posix.cpp 
> 01c0ad6dbb6d509e62e769365586b3d23dcb240d 
>   src/slave/containerizer/mesos/isolators/filesystem/windows.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/filesystem/windows.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/posix.hpp 
> 227505a70a440f15e68ac001878bcf25610db45f 
>   src/slave/containerizer/mesos/isolators/windows.hpp PRE-CREATION 
>   src/usage/main.cpp 731acb69900b6fc2bb7bd19cccd78aafb0cc 
>   src/usage/usage.hpp 2e1996a78c617c2559ef882ebede5c8aab6f899f 
>   src/usage/usage.cpp 3b19682e67372b81484eacddbab78c2e5eda3c5b 
> 
> Diff: https://reviews.apache.org/r/47536/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47472: Windows: Added support for `fetcher.cpp`.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/slave/containerizer/fetcher.cpp (lines 210 - 211)


Why is this logic compiled out on windows?
Does it not apply? If so, can we add a comment?
If it does, but we can't implement it, can we add a `TODO` referencing a 
JIRA?
If the issue is the slash separator, should we be using the new constant 
you introduced? `os::PATH_SEPARATOR`?



src/slave/containerizer/fetcher.cpp (lines 311 - 312)


A `TODO` referencing the JIRA to add support would be good.



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


Missing period.
Can we wrap `os::chown` with backticks and remove the parentheses?



src/slave/containerizer/fetcher.cpp (lines 750 - 752)


In Mesos we use `camelCase`. Sorry for the current inconsistency in the 
project.
Either `_stderr` or `stderrSandbox` would be fine here.



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


can you use `os::chown` instead of `chown` here to be consistent with your 
comment higher up in the file?


- Joris Van Remoortere


On May 17, 2016, 4:02 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47472/
> ---
> 
> (Updated May 17, 2016, 4:02 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3618
> https://issues.apache.org/jira/browse/MESOS-3618
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Added support for `fetcher.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/fetcher.cpp 
> 176d8863d1becd8864218a0012ab45c614f0ad77 
> 
> Diff: https://reviews.apache.org/r/47472/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47469: Agent: Added `launch.cpp` to Windows build.

[Joris Van Remoortere]

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




src/CMakeLists.txt (line 259)


Why here as opposed to alphabetical?



src/slave/containerizer/mesos/launch.cpp (lines 138 - 146)


Are we gaining something specifically on the windows side here?
Is this just mapping the FDs into an abstraction, or are we actually 
creating a pipe?
If so, why wasn't that done before?


- Joris Van Remoortere


On May 17, 2016, 4:02 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47469/
> ---
> 
> (Updated May 17, 2016, 4:02 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3624
> https://issues.apache.org/jira/browse/MESOS-3624
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added `launch.cpp` to Windows build.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/slave/containerizer/mesos/launch.cpp 
> e22106b014c871e2184a15c2ab154a0674874e47 
> 
> Diff: https://reviews.apache.org/r/47469/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47360: Updated dynamic reservation and persistent volume docs.

[Greg Mann]

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

(Updated May 24, 2016, 10:09 p.m.)


Review request for mesos, Bernd Mathiske and Neil Conway.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

This patch updates the documentation of RESERVE and
CREATE operations, both via offer operations and
operator endpoints. Specifically, we clarify the
Mesos master's expectations for the values of the
`principal` fields found in `ReservationInfo` and
`DiskInfo.Persistence`.


Diffs (updated)
-

  docs/persistent-volume.md c13d79124f0b5ea2a715b4d2990fda4e06b2fb02 
  docs/reservation.md a400d19aec7a48d122ba1c9c23d38d792b8dbe6f 

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


Testing
---

Viewed with the Mesos website container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 46613: Introduced filtering relevant actions and acls.

[Joerg Schad]

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

(Updated May 24, 2016, 9:43 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Michael Park.


Changes
---

Added new actions to Request model.


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


Repository: mesos


Description
---

In order to allow for framework and task level filtering we introduce
the following authorizer actions:
* VIEW_FRAMEWORK_WITH_INFO
* VIEW_TASK_WITH_TASK

Note that we need different actions for authorizing a tasks
based on the object being authorized.

We also introduce the following acls for the local authorizer:
* ViewFrameworks  (giving access to frameworks running under
  a specific OS user)
* ViewTasks view_tasks (giving access to Tasks run under a
specific OS user)


Diffs (updated)
-

  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 
  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

Review Request 47795: Enabled authorization for sandboxes.

[Alexander Rojas]

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

Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
and Vinod Kone.


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


Repository: mesos


Description
---

Enables authorization of the sandboxes using the callback function
parameter of `Files::attach()`.

It also adds relevant ACLs and support on the authorizer interface.


Diffs
-

  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 
  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
  src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
  src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 

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


Testing
---

on OSX the script:

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

rm -rf /tmp/mesos/*

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

cat < /tmp/acls.json
{
  "permissive": false,
  "access_sandboxes" : [
{
  "principals" : { "values" : ["foo"] },
  "roles" : { "values" : ["test"] }
}
  ]
}
EOF

./mesos-master.sh --work_dir=/tmp/mesos/master &
./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 &

./mesos-execute --command='while true; do echo "Hello world"; sleep 3; done' \
--role=test \
--master=127.0.0.1:5050 \
--name=echoer &

SANDBOX_VPATH=`http GET http://127.0.0.1:5051/files/debug -a foo:bar -b  
--pretty=none \
 | python -c 'import json,sys;obj=json.load(sys.stdin);print obj.keys()[0]'`

# This should yield a 200 OK response
http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
foo:bar

# HTTP/1.1 200 OK
# Content-Disposition: attachment; filename=stdout
# Content-Length: 3267
# Content-Type: application/octet-stream
# Date: Fri, 20 May 2016 13:52:31 GMT
#
# Received SUBSCRIBED event
# Subscribed executor on localhost
# Received LAUNCH event
# Starting task echoer
# sh -c 'while true; do echo "Hello world"; sleep 3; done'
# Forked command at 26162
# Hello world
# Hello world
# Hello world
# Hello world
# Hello world

# This shold yield a 403 Forbidden response
http GET http://127.0.0.1:5051/files/download?path=${SANDBOX_VPATH}/stdout -a 
baz:bar

# HTTP/1.1 403 Forbidden
# Content-Length: 0
# Date: Fri, 20 May 2016 13:52:37 GMT
#
#
#

```


Thanks,

Alexander Rojas

Review Request 47794: Added authorization support for mesos::internal::Files.

[Alexander Rojas]

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

Review request for mesos, Adam B, Benjamin Mahler, Joerg Schad, Michael Park, 
and Vinod Kone.


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


Repository: mesos


Description
---

Adds an optional parameter to the `mesos::internal::Files::attach()`
method. The type of this parameter is a callable object which returns
a future to a boolean and takes as parameter an optional string
representing a principal name.

The parameter is called, if set, whenever one of the routed endpoints
of the `Files` object is accessed through HTTP. If the callable object
returns a false boolean, then processing of the request is aborted
and a `403 Forbidden` response is returned.


Diffs
-

  src/files/files.hpp 90acb3406c46c164108deb559af71fb109a5773b 
  src/files/files.cpp e4b0ada00aabba6553810391f4015a896f8a69a5 
  src/tests/files_tests.cpp 5d6620d13babaf0bb7f9c888bb1b4fa2228b6ccd 

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


Testing
---

On OSX:
`make check`


Thanks,

Alexander Rojas

Re: Review Request 46418: Refactored the `os::access` function between POSIX and Windows.

[Michael Park]

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

(Updated May 24, 2016, 9:36 p.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description (updated)
---

Refactored the `os::access` function between POSIX and Windows.


Diffs (updated)
-

  3rdparty/stout/include/Makefile.am ae4116b02474f2228d86ad4a9bbf69f9c1bb4342 
  3rdparty/stout/include/stout/os.hpp 2a306ae87422300b586e9f627a3f125d8faac4db 
  3rdparty/stout/include/stout/os/access.hpp PRE-CREATION 
  3rdparty/stout/include/stout/posix/os.hpp 
1a66184d86d6a0a13b8facfc9f969e09dd824725 
  3rdparty/stout/include/stout/windows/os.hpp 
71a8c61335e2bce340447e94e855a6bd79c49dec 

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


Testing
---

`make check`


Thanks,

Michael Park

Re: Review Request 47216: Wired up the new docker environment hook.

[Joseph Wu]

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

(Updated May 24, 2016, 2:05 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Spacing tweak.


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


Repository: mesos


Description
---

Modifies the code path for docker command executors. (Custom 
executors are not supported because they may break if exposed to an
unfamiliar flag.)

Docker command executors are now launched with an additional flag
that is filled in by a hook.  The --task_environment flag tells the
command executor to pass some specified mapping of environment
variables to the task.


Diffs (updated)
-

  src/slave/containerizer/docker.hpp fac26a239e9981e5b1c2f2f90b52785819492c2a 
  src/slave/containerizer/docker.cpp 7af14f4472283ceefd73c06dd8df60af4cf6f7e8 

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


Testing
---

sudo make check


Thanks,

Joseph Wu

Re: Review Request 47150: Implemented new asynchronous docker pre-launch hook.

[Joseph Wu]

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

(Updated May 24, 2016, 2:04 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Minor tweaks.


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


Repository: mesos


Description (updated)
---

Introduces, but does not fully wire up a new hook.

The new hook, "slavePreLaunchDockerEnvironmentDecoratorAndValidator",
has divergent semantics compared with existing hooks:

* The hook is asynchronous,
* can prevent a task from launching if it errors,
* can overwrite environment variables.

The new hook is a strictly-superior replacement for 
the existing hook "slavePreLaunchDockerHook".  
The arguments to both hooks are identical.


Diffs (updated)
-

  include/mesos/hook.hpp 210ffba09f5acae34ca49b888a781f683777f9ca 
  src/examples/test_hook_module.cpp 4b97f84204934d0e678786fd6cde38b89a6f8f48 
  src/hook/manager.hpp 528674e36639fe78137ba0a4bb004c99730e7a22 
  src/hook/manager.cpp 381807d582998043d73e9b8c9d3c1fddbcf73cf1 
  src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 

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


Testing
---

make check


Thanks,

Joseph Wu

Re: Review Request 47150: Implemented new asynchronous docker pre-launch hook.

[Joseph Wu]

> On May 23, 2016, 8:38 p.m., Kapil Arya wrote:
> > include/mesos/hook.hpp, line 112
> > 
> >
> > Can we replace `name` with something more explicit/precise?

This argument is a combination of the AgentID and ContainerID (the "container 
name").  This argument is ignored for custom command executors and has some 
caveats for the mesos-in-docker case.

I'd rather keep this variable consistent with `slavePreLaunchDockerHook`.


> On May 23, 2016, 8:38 p.m., Kapil Arya wrote:
> > include/mesos/hook.hpp, line 116
> > 
> >
> > I wonder if we should force the module to use `Environment` and 
> > internally use `map` if needed?

This is consistent with `slavePreLaunchDockerHook`.  Also, if we use 
`Environment`, the hook manager will need to do an extra `Environment` -> map 
-> `Environment` conversion.


> On May 23, 2016, 8:38 p.m., Kapil Arya wrote:
> > src/examples/test_hook_module.cpp, line 202
> > 
> >
> > It could be my screen, but could you double-check the alignment here?

The `<<` weren't aligned.  That "descriptive" hook name complicates line 
wrapping :)


- Joseph


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


On May 24, 2016, 2:04 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47150/
> ---
> 
> (Updated May 24, 2016, 2:04 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduces, but does not fully wire up a new hook.
> 
> The new hook, "slavePreLaunchDockerEnvironmentDecoratorAndValidator",
> has divergent semantics compared with existing hooks:
> 
> * The hook is asynchronous,
> * can prevent a task from launching if it errors,
> * can overwrite environment variables.
> 
> The new hook is a strictly-superior replacement for 
> the existing hook "slavePreLaunchDockerHook".  
> The arguments to both hooks are identical.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 210ffba09f5acae34ca49b888a781f683777f9ca 
>   src/examples/test_hook_module.cpp 4b97f84204934d0e678786fd6cde38b89a6f8f48 
>   src/hook/manager.hpp 528674e36639fe78137ba0a4bb004c99730e7a22 
>   src/hook/manager.cpp 381807d582998043d73e9b8c9d3c1fddbcf73cf1 
>   src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 
> 
> Diff: https://reviews.apache.org/r/47150/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47212: Removed duplicate call to containerizer::executorEnvironment.

[Joseph Wu]

> On May 23, 2016, 8:10 p.m., Kapil Arya wrote:
> > src/slave/containerizer/docker.cpp, line 1195
> > 
> >
> > Would there be any difference in the computed environment here vs in 
> > container constructor?

The first call is here:
https://github.com/apache/mesos/blob/d6846f952b55c47d33a926e4d4da38d058e6dcf3/src/slave/containerizer/docker.hpp#L348-L355

The arguments are the same, and these specific arguments are not modified 
between `executorEnvironment` and `launchExecutorProcess`.


- Joseph


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


On May 10, 2016, 7:13 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47212/
> ---
> 
> (Updated May 10, 2016, 7:13 p.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this code path, where the task uses the default command executor, 
> and the agent is not dockerized
> (i.e. `taskInfo.isSome() && flags.docker_mesos_image.isNone()`), 
> the `executorEnvironment` function is called twice.
> 
> The first call is inside the `Container*` constructor called by 
> `Container::create`.  Since `Container::create` gives passes `None` 
> for the `environment` field, the constructor will call 
> `executorEnvironment` to populate the `environment` field.  
> The populated field is then accessible by `launchExecutorProcess`.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp 7af14f4472283ceefd73c06dd8df60af4cf6f7e8 
> 
> Diff: https://reviews.apache.org/r/47212/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47213: Added FlagsBase::toVector method as an alternative to stringify(flags).

[Joseph Wu]

> On May 23, 2016, 8:17 p.m., Kapil Arya wrote:
> > 3rdparty/stout/include/stout/flags/flags.hpp, line 1027
> > 
> >
> > Just curious, will it do the right thing for boolean flags such as 
> > `--quiet`, `--no-quiet`, `--version` and `--no-version`?

Those flags will be transformed like:
```
--quiet  => --quiet=true
--no-quiet   => --quiet=false
--version=> --version=true
--no-version => --version=false
```

Round-trips will work, but we don't preserve the flags *exactly* as they are 
passed in.


- Joseph


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


On May 16, 2016, 11 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47213/
> ---
> 
> (Updated May 16, 2016, 11 a.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Bugs: MESOS-5350
> https://issues.apache.org/jira/browse/MESOS-5350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `stringify(flags)` will return a version of the flags that can be passed
> through `/bin/sh` for execution.  This method returns the same flags,
> but in a more `exec`-friendly form.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/flags/flags.hpp 
> 91542f56431579be8a3da5ec6e3e58f68e7dfcbe 
> 
> Diff: https://reviews.apache.org/r/47213/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47205: Added optional environment variable argument to mesos-docker-executor.

[Joseph Wu]

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

(Updated May 24, 2016, 2:02 p.m.)


Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.


Changes
---

Move map validation logic into main.


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


Repository: mesos


Description
---

This flag opens up a way for hooks to specify environment variables for
docker tasks.  Existing hooks can only affect the environment variables
of docker executors.


Diffs (updated)
-

  src/docker/executor.hpp 798ca3d4e261854a3b911d59929f2ca2afeb9ac8 
  src/docker/executor.cpp 04f44044a80e1b355580ee3d1d8d301ac3baf565 

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


Testing
---

See later reviews in chain.


Thanks,

Joseph Wu

Re: Review Request 47558: Added allower interface to authorizer.

[Alexander Rojas]

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




src/authorizer/local/authorizer.cpp (line 116)


This method is reentrant and it doesn't depends on anything but its 
parameters, so it would be better to make it `static` in the local authorizer 
and call that one instead of copying paying the whole code.


- Alexander Rojas


On May 22, 2016, 11:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47558/
> ---
> 
> (Updated May 22, 2016, 11:25 p.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Bugs: MESOS-5403
> https://issues.apache.org/jira/browse/MESOS-5403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allower interface to authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ed5f9e73661e25a83722cf1e408ae61023cd4a21 
>   src/authorizer/local/authorizer.hpp 
> 61388454025211fd7d53e71a86983fd8479950b6 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
>   src/tests/mesos.cpp 629135f0dc59346f0fcddb2cbe65ca5770fad34e 
> 
> Diff: https://reviews.apache.org/r/47558/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47786: Removed unneeded #includes from type_utils.hpp.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47752, 47753, 47754, 47755, 47756, 47786]

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 May 24, 2016, 6:06 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47786/
> ---
> 
> (Updated May 24, 2016, 6:06 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed unneeded #includes from type_utils.hpp.
> 
> 
> Diffs
> -
> 
>   include/mesos/type_utils.hpp 27fa8c9256c24b5ec6f5d259305ea66b5f556673 
> 
> Diff: https://reviews.apache.org/r/47786/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 47559: Added authorization based filtering to /state-summary.

[Joerg Schad]

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




src/authorizer/local/authorizer.cpp (line 471)


new LocalAuthorizerObjectAllower(alcs, subject, action)


- Joerg Schad


On May 21, 2016, 9:41 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47559/
> ---
> 
> (Updated May 21, 2016, 9:41 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-5170
> https://issues.apache.org/jira/browse/MESOS-5170
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added authorization based filtering to /state-summary.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
> 
> Diff: https://reviews.apache.org/r/47559/diff/
> 
> 
> Testing
> ---
> 
> make check + (sudo) make check on various linux systems
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47199: Implemented parsing docker labels in v1 spec.

[Gilbert Song]

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

(Updated May 24, 2016, 10:18 a.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


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


Repository: mesos


Description
---

Implemented parsing docker labels in v1 spec.


Diffs (updated)
-

  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 47198: Added labels to docker v1 spec config.

[Gilbert Song]

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

(Updated May 24, 2016, 10:18 a.m.)


Review request for mesos, Benjamin Mahler, Artem Harutyunyan, Jie Yu, and Kevin 
Klues.


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


Repository: mesos


Description
---

Added labels to docker v1 spec config.


Diffs (updated)
-

  include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 46370: Introduced linux capabilities API.

[Jie Yu]

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




src/linux/capabilities.hpp (line 37)


As I mentioned earlier, can you specify the enum base as well?
http://en.cppreference.com/w/cpp/language/enum



src/linux/capabilities.hpp (line 84)


Let's move this too the end of this file.



src/linux/capabilities.hpp (line 107)


s/index/type/



src/linux/capabilities.cpp (line 48)


constexpr char
I would kill empty lines between those constants.



src/linux/capabilities.cpp (lines 58 - 60)


Not needed.



src/linux/capabilities.cpp (line 69)


I would suggest that we do not use a union here. Instead, use `struct 
__user_cap_data_struct data` directly.



src/linux/capabilities.cpp (line 70)


`s/__u32/uint32_t/



src/linux/capabilities.cpp (lines 75 - 90)


Let's inline those helpers and make SyscallPayload a pure struct.



src/linux/capabilities.cpp (lines 93 - 111)


Is it being used? Why do we need that?



src/linux/capabilities.cpp (line 116)


I would follow the following style:
```
switch (cap) {
  case CHOWN: return stream << "CHOWN";
  case DAC_OVERRIDE: return stream << "DAC_OVERRIDE";
  ...
  default:
UNREACHABLE();
}
```



src/linux/capabilities.cpp (lines 354 - 370)


Same here.



src/linux/capabilities.cpp (lines 545 - 547)


Making 'sets[x]' here Option is confusing. Can you make it non optional?



src/linux/capabilities.cpp (lines 555 - 570)


The check here will be performed by the kernel. I would say let's just rely 
on kernel to do the check, and get rid of the redundent check here.



src/linux/capabilities.cpp (lines 606 - 639)


Similarily, let's make this method very simple and it's callers 
responsibility to make sure it has proper capability to do prctl here.


- Jie Yu


On May 17, 2016, 2:59 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46370/
> ---
> 
> (Updated May 17, 2016, 2:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change introduces basic API for linux capabilities. This is not a
> comprehensive API but is strictly limited to the need for securing Mesos
> containers using linux capabilities.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
>   src/linux/capabilities.hpp PRE-CREATION 
>   src/linux/capabilities.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46370/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 47069: Added `user` field to `Task` protobuf message.

[Joerg Schad]

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

(Updated May 24, 2016, 4:06 p.m.)


Review request for mesos, Adam B, Alexander Rojas, and Michael Park.


Changes
---

Adressed review


Repository: mesos


Description
---

The LocalAuthorizer might use the OS \`user\` for
authorization of tasks, so we add it to the \`Task\` protobuf
message. Note that the master stores \`Task\` (as opposed
to \`TaskInfo\`) for running and completed tasks.


Diffs (updated)
-

  include/mesos/mesos.proto 887ffe9c5f04e25539a4c6b3d52ce5299c65e8d3 
  src/common/http.cpp ad6a4b44af3ec847a3a6c0839a88fba18cda5011 
  src/common/protobuf_utils.cpp 4f4711d54c471922f1a103310d4d360e41a99870 
  src/common/type_utils.cpp 037c4336276258d671d0b1bf66cdab50b5bf9fb8 
  src/tests/common/http_tests.cpp 300f7cc21239b7d8727f7f0f02963f1af0dc80d7 

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


Testing
---

tested entire chain.


Thanks,

Joerg Schad

Re: Review Request 47491: Added `environment` field to `Task` protobuf message.

[Joerg Schad]

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

(Updated May 24, 2016, 3:59 p.m.)


Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

An Authorizer might use environment variables for
authorization of tasks, so we add it to the \`Task\` protobuf
message. Note that the master stores \`Task\` (as opposed
to \`TaskInfo\`) for running and completed tasks.


Diffs
-

  include/mesos/mesos.proto 887ffe9c5f04e25539a4c6b3d52ce5299c65e8d3 
  src/common/protobuf_utils.cpp 4f4711d54c471922f1a103310d4d360e41a99870 

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


Testing
---

make check OS-X + (sudo) make check on a variety of Linux systems on internal 
CI.


Thanks,

Joerg Schad

Re: Review Request 36610: Add explicit syscall header file to linux fs

[Tomasz Janiszewski]

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



Is this patch still valid? Take a look at https://reviews.apache.org/r/46730

- Tomasz Janiszewski


On Lip 20, 2015, 4:21 rano, Jihun Kang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36610/
> ---
> 
> (Updated Lip 20, 2015, 4:21 rano)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-3085
> https://issues.apache.org/jira/browse/MESOS-3085
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add explicit syscall header file to linux fs
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp ea0891e320154b85a21ed2d138c192821efae9cd 
> 
> Diff: https://reviews.apache.org/r/36610/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jihun Kang
> 
>

Re: Review Request 47509: Fixed authorization::Request initializings.

[Benjamin Bannier]

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



LGTM!

I had to mechanically update the patch for changes in `master`, the patch is:

```
>From ee43fc0b1c8e0b6b5a46c54f8262a1c8c9c864a4 Mon Sep 17 00:00:00 2001
From: Benjamin Bannier 
Date: Tue, 24 May 2016 17:04:13 +0200
Subject: [PATCH] Updated patch of rb47509.

---
 src/authorizer/local/authorizer.cpp |  3 +++
 src/common/http.cpp |  4 +++-
 src/master/http.cpp | 12 ---
 src/master/master.cpp   | 40 ++---
 src/master/quota_handler.cpp| 20 ++-
 src/master/weights_handler.cpp  |  4 +++-
 src/slave/http.cpp  |  4 +++-
 src/tests/authorization_tests.cpp   | 12 ++-
 8 files changed, 75 insertions(+), 24 deletions(-)

diff --git a/src/authorizer/local/authorizer.cpp 
b/src/authorizer/local/authorizer.cpp
index dc53bc4..39eab7f 100644
--- a/src/authorizer/local/authorizer.cpp
+++ b/src/authorizer/local/authorizer.cpp
@@ -493,6 +493,9 @@ LocalAuthorizer::~LocalAuthorizer()
 process::Future LocalAuthorizer::authorized(
   const authorization::Request& request)
 {
+  CHECK(request.has_subject());
+  CHECK(request.has_object());
+
   typedef Future (LocalAuthorizerProcess::*F)(
   const authorization::Request&);
 
diff --git a/src/common/http.cpp b/src/common/http.cpp
index ad6a4b4..dcbbc60 100644
--- a/src/common/http.cpp
+++ b/src/common/http.cpp
@@ -599,8 +599,10 @@ const AuthorizationCallbacks createAuthorizationCallbacks(
 authorization::Request authRequest;
 authRequest.set_action(mesos::authorization::GET_ENDPOINT_WITH_PATH);
 
+authorization::Subject* subject = authRequest.mutable_subject();
+
 if (principal.isSome()) {
-  authRequest.mutable_subject()->set_value(principal.get());
+  subject->set_value(principal.get());
 }
 
 const string path = httpRequest.url.path;
diff --git a/src/master/http.cpp b/src/master/http.cpp
index b36b439..b881612 100644
--- a/src/master/http.cpp
+++ b/src/master/http.cpp
@@ -1956,12 +1956,16 @@ Future Master::Http::teardown(
   authorization::Request teardown;
   teardown.set_action(authorization::TEARDOWN_FRAMEWORK_WITH_PRINCIPAL);
 
+  authorization::Subject* subject = teardown.mutable_subject();
+
   if (principal.isSome()) {
-teardown.mutable_subject()->set_value(principal.get());
+subject->set_value(principal.get());
   }
 
+  authorization::Object* object = teardown.mutable_object();
+
   if (framework->info.has_principal()) {
-teardown.mutable_object()->set_value(framework->info.principal());
+object->set_value(framework->info.principal());
   }
 
   return master->authorizer.get()->authorized(teardown)
@@ -2790,8 +2794,10 @@ Future Master::Http::authorizeEndpoint(
 return Failure("Unexpected request method '" + method + "'");
   }
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-request.mutable_subject()->set_value(principal.get());
+subject->set_value(principal.get());
   }
 
   request.mutable_object()->set_value(endpoint);
diff --git a/src/master/master.cpp b/src/master/master.cpp
index 0005a29..d215b76 100644
--- a/src/master/master.cpp
+++ b/src/master/master.cpp
@@ -1961,8 +1961,10 @@ Future Master::authorizeFramework(
   authorization::Request request;
   request.set_action(authorization::REGISTER_FRAMEWORK_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (frameworkInfo.has_principal()) {
-request.mutable_subject()->set_value(frameworkInfo.principal());
+subject->set_value(frameworkInfo.principal());
   }
 
   request.mutable_object()->set_value(frameworkInfo.role());
@@ -3035,8 +3037,10 @@ Future Master::authorizeTask(
   authorization::Request request;
   request.set_action(authorization::RUN_TASK_WITH_USER);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (framework->info.has_principal()) {
-request.mutable_subject()->set_value(framework->info.principal());
+subject->set_value(framework->info.principal());
   }
 
   request.mutable_object()->set_value(user);
@@ -3056,8 +3060,10 @@ Future Master::authorizeReserveResources(
   authorization::Request request;
   request.set_action(authorization::RESERVE_RESOURCES_WITH_ROLE);
 
+  authorization::Subject* subject = request.mutable_subject();
+
   if (principal.isSome()) {
-request.mutable_subject()->set_value(principal.get());
+subject->set_value(principal.get());
   }
 
   // The operation will be authorized if the principal is allowed to make
@@ -3066,10 +3072,11 @@ Future Master::authorizeReserveResources(
   hashset roles;
   list authorizations;
   foreach (const Resource& 

Re: Review Request 47771: Validate ACLs on creating an instance of local authorizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47771]

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 May 24, 2016, 12:52 p.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47771/
> ---
> 
> (Updated May 24, 2016, 12:52 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5406
> https://issues.apache.org/jira/browse/MESOS-5406
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Catch redundant and conflicting acls specified by users.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/tests/authorization_tests.cpp 54bfb46a807677f4a4a2bb88dcb78a358cf5121a 
> 
> Diff: https://reviews.apache.org/r/47771/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Review Request 47771: Validate ACLs on creating an instance of local authorizer.

[Jay Guo]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Catch redundant and conflicting acls specified by users.


Diffs
-

  src/authorizer/local/authorizer.cpp dc53bc4374aea98b5ed41ade5617374d2447229b 
  src/tests/authorization_tests.cpp 54bfb46a807677f4a4a2bb88dcb78a358cf5121a 

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


Testing
---

make check


Thanks,

Jay Guo

Re: Review Request 45668: Enable CMake build.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45668]

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 May 24, 2016, 9:22 a.m., Juan Larriba wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45668/
> ---
> 
> (Updated May 24, 2016, 9:22 a.m.)
> 
> 
> Review request for mesos, Alex Clemmer, Joerg Schad, and Vinod Kone.
> 
> 
> Bugs: MESOS-5101
> https://issues.apache.org/jira/browse/MESOS-5101
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enable CMake build.
> 
> 
> Diffs
> -
> 
>   support/docker_build.sh 28ef4dce3f473adab9919d4c2170075a0900af41 
> 
> Diff: https://reviews.apache.org/r/45668/diff/
> 
> 
> Testing
> ---
> 
> Built using docker_build.sh on both centos:7 and ubuntu:14.04 using both 
> cmake and autotools. In ubuntu:14.04 was built using gcc and clang, in 
> centos:7 only gcc.
> 
> 
> Thanks,
> 
> Juan Larriba
> 
>

Re: Review Request 45668: Enable CMake build.

[Juan Larriba]

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

(Updated Mayo 24, 2016, 9:22 a.m.)


Review request for mesos, Alex Clemmer, Joerg Schad, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Enable CMake build.


Diffs (updated)
-

  support/docker_build.sh 28ef4dce3f473adab9919d4c2170075a0900af41 

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


Testing
---

Built using docker_build.sh on both centos:7 and ubuntu:14.04 using both cmake 
and autotools. In ubuntu:14.04 was built using gcc and clang, in centos:7 only 
gcc.


Thanks,

Juan Larriba

Re: Review Request 47511: Added documentation for `docker/volume` isolator.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [47511]

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

Error:
2016-05-24 08:24:17 URL:https://reviews.apache.org/r/47511/diff/raw/ 
[12837/12837] -> "47511.patch" [1]
error: missing binary patch data for 'docs/images/docker-volume-isolator.png'
error: binary patch does not apply to 'docs/images/docker-volume-isolator.png'
error: docs/images/docker-volume-isolator.png: patch does not apply

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

- Mesos ReviewBot


On May 24, 2016, 7:20 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47511/
> ---
> 
> (Updated May 24, 2016, 7:20 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5216
> https://issues.apache.org/jira/browse/MESOS-5216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for `docker/volume` isolator.
> 
> 
> Diffs
> -
> 
>   docs/docker-volume-isolator.md PRE-CREATION 
>   docs/images/docker-volume-isolator.png PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47511/diff/
> 
> 
> Testing
> ---
> 
> You can review the document here: 
> https://github.com/jay-lau/mesos/blob/master/docs/docker-volume-isolator.md
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46904: Fixed a typo in libprocess.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On May 19, 2016, 2:44 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46904/
> ---
> 
> (Updated May 19, 2016, 2:44 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Corrected a misspelled word in 'future.hpp'.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/future.hpp 
> 011fea8cedc75ac06599602252974f4ce662c893 
> 
> Diff: https://reviews.apache.org/r/46904/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46438: Added the test "CniIsolatorTest.ROOT_SlaveRecovery".

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46096, 46097, 46435, 46436, 46438]

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 May 24, 2016, 3:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46438/
> ---
> 
> (Updated May 24, 2016, 3:20 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5167
> https://issues.apache.org/jira/browse/MESOS-5167
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added the test "CniIsolatorTest.ROOT_SlaveRecovery".
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> 6ef15cc9b089319c4e62f8cd20565f00686f2089 
> 
> Diff: https://reviews.apache.org/r/46438/diff/
> 
> 
> Testing
> ---
> 
> [ RUN  ] CniIsolatorTest.ROOT_SlaveRecovery
> I0524 11:06:18.697321 20808 exec.cpp:150] Version: 0.29.0
> I0524 11:06:18.706725 20807 exec.cpp:225] Executor registered on agent 
> 7399d7c2-a7b4-47ed-9b95-8a0a87312816-S0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received LAUNCH event
> Starting task d4e6feb2-dce2-4d90-b7c4-35ce27b8757d
> Forked command at 20814
> sh -c 'sleep 1000'
> I0524 11:06:18.895862 20806 exec.cpp:271] Received reconnect request from 
> agent 7399d7c2-a7b4-47ed-9b95-8a0a87312816-S0
> I0524 11:06:18.899616 20807 exec.cpp:248] Executor re-registered on agent 
> 7399d7c2-a7b4-47ed-9b95-8a0a87312816-S0
> Received SUBSCRIBED event
> Subscribed executor on mesos
> Received KILL event
> Received kill for task d4e6feb2-dce2-4d90-b7c4-35ce27b8757d with grace period 
> of 3secs
> Sending SIGTERM to process tree at pid 20814
> Sent SIGTERM to the following process trees:
> [ 
> -+- 20814 sh -c sleep 1000 
>  --- 20815 sleep 1000 
> ]
> Scheduling escalation to SIGKILL in 3secs from now
> Command terminated with signal Terminated (pid: 20814)
> I0524 11:06:21.020820 20808 exec.cpp:399] Executor asked to shutdown
> [   OK ] CniIsolatorTest.ROOT_SlaveRecovery (3575 ms)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45952: Implemented support for passing agent default docker config.

[Guangya Liu]

> On 四月 11, 2016, 7:37 a.m., Guangya Liu wrote:
> > src/uri/fetchers/docker.cpp, line 636
> > 
> >
> > Add a log here to identify that the fetcher is now using auth to fetch 
> > the image?
> 
> Guangya Liu wrote:
> Adding the log in `getCredential` may be better, post some comments in 
> https://reviews.apache.org/r/45949/ , please check. Thanks.
> 
> Gilbert Song wrote:
> Thanks for the comments, Guangya! Seems like we dont have any log in 
> docker fetcher.

It will be very difficult for trouble shooting when I encounter some issue in 
fetcher, is it possible that you can put some log first for your project and we 
can add more log if needed?


- Guangya


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


On 五月 23, 2016, 9:52 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45952/
> ---
> 
> (Updated 五月 23, 2016, 9:52 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented support for passing agent default docker config.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/docker/store.cpp 
> eeec94326a4fd67675df10e0b6a32267e555fa96 
>   src/uri/fetchers/docker.hpp c855a2b55a07bb398f7547b44a85b8ba2d2b2ec3 
>   src/uri/fetchers/docker.cpp ab8f5e05758b7de2573605c81ac80e656bb1db24 
> 
> Diff: https://reviews.apache.org/r/45952/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with mesos-execute, using a private repo from docker hub. Both cases 
> are tested:
> 1. --docker_registry=private_registry
> 2. private_registry/repo
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45949: Implemented docker config get credential helper.

[Guangya Liu]

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




include/mesos/docker/spec.hpp (line 75)


s/encode/encoded ?


- Guangya Liu


On 五月 23, 2016, 9:51 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45949/
> ---
> 
> (Updated 五月 23, 2016, 9:51 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-4938
> https://issues.apache.org/jira/browse/MESOS-4938
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented docker config get credential helper.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
>   src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
> 
> Diff: https://reviews.apache.org/r/45949/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47511: Added documentation for `docker/volume` isolator.

[Guangya Liu]

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

(Updated 五月 24, 2016, 7:20 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added documentation for `docker/volume` isolator.


Diffs (updated)
-

  docs/docker-volume-isolator.md PRE-CREATION 
  docs/images/docker-volume-isolator.png PRE-CREATION 

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


Testing
---

You can review the document here: 
https://github.com/jay-lau/mesos/blob/master/docs/docker-volume-isolator.md


Thanks,

Guangya Liu

Re: Review Request 47511: Added documentation for `docker/volume` isolator.

[Guangya Liu]

> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 17-20
> > 
> >
> > newline for each.

Can you please take a look at 
https://github.com/jay-lau/mesos/blob/master/docs/docker-volume-isolator.md to 
see the output of this? What I want is that I hope that I can list all of the 
driver backends as `including Convoy plugin, Flocker plugin, GlusterFS plugin, 
REX-Ray plugin etc.`


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, line 30
> > 
> >
> > s/docker volume driver isolator/ docker volume isolator/g

I do not found I have this in the document ;-)


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 33-40
> > 
> >
> > Merge together and rephase.

I think that diving those to two parts may be better and seems difficult to 
merge those two part.


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, line 42
> > 
> >
> > Move this below when introducing `container_path`:
> > 
> > The docker volume isolator supports tasks with/without rootfs:
> > with rootfs v.s. without rootfs
> > absolute path v.s. relative path

I was clarifying this part in line 198, hope it is OK.


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 45-52
> > 
> >
> > kill this.

Here I want to list an e2e use case just like what we did for 
https://github.com/apache/mesos/blob/master/docs/networking-for-mesos-managed-containers.md#framework-requests-ip-address-for-containers
 here.


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 54-75
> > 
> >
> > kill this.
> > 
> > Generally, we may not need to explain the implementation in detail. 
> > Let's just explain the diagram and mention the recoverablity in one 
> > sentence.

Can we take this as the explanation for the diagram? I was following this 
document as a reference 
https://github.com/apache/mesos/blob/master/docs/networking-for-mesos-managed-containers.md#network-virtualizer-assigns-ip-address-to-the-container-and-isolates-it
 , comments?


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 120-187
> > 
> >
> > let's just pick out related message fields and filter out all comments.

Except `host_path`, all of other fields are needed, so I suggest that we keep 
current protobuf.


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 216-288
> > 
> >
> > we dont need this if we explain ContainerInfo.Volumes above.

I think that it is better put an example here to show how to use the protobuf, 
this can make the framework developer more clear for how to use it, and I was 
following the same way as 
https://github.com/apache/mesos/blob/master/docs/networking-for-mesos-managed-containers.md#enabling-frameworks-for-ip-per-container-capability
 here


> On 五月 23, 2016, 10:53 p.m., Gilbert Song wrote:
> > docs/docker-volume-isolator.md, lines 79-80
> > 
> >
> > could we rephase?

Updated a bit.


- Guangya


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


On 五月 24, 2016, 4:11 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47511/
> ---
> 
> (Updated 五月 24, 2016, 4:11 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5216
> https://issues.apache.org/jira/browse/MESOS-5216
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added documentation for `docker/volume` isolator.
> 
> 
> Diffs
> -
> 
>   docs/docker-volume-isolator.md PRE-CREATION 
>   docs/images/docker-volume-isolator.png PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47511/diff/
> 
> 
> Testing
> ---
> 
> You can review the document here: 
> https://github.com/jay-lau/mesos/blob/master/docs/docker-volume-isolator.md
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 47558: Added allower interface to authorizer.

[Joerg Schad]

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




include/mesos/authorizer/authorizer.hpp (line 42)


Add one blank



src/authorizer/local/authorizer.cpp (line 87)


Pull out to general case.



src/authorizer/local/authorizer.cpp (line 92)


Generate GenericALCs outside (getAutzHandler), and then use existing 
matching logic.



src/authorizer/local/authorizer.cpp (line 167)


move to static function.



src/authorizer/local/authorizer.cpp (line 470)


Move into LocalAuthorizerObjectAllower.


- Joerg Schad


On May 22, 2016, 9:25 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47558/
> ---
> 
> (Updated May 22, 2016, 9:25 p.m.)
> 
> 
> Review request for mesos, Michael Park and Vinod Kone.
> 
> 
> Bugs: MESOS-5403
> https://issues.apache.org/jira/browse/MESOS-5403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added allower interface to authorizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp 
> ed5f9e73661e25a83722cf1e408ae61023cd4a21 
>   src/authorizer/local/authorizer.hpp 
> 61388454025211fd7d53e71a86983fd8479950b6 
>   src/authorizer/local/authorizer.cpp 
> dc53bc4374aea98b5ed41ade5617374d2447229b 
>   src/tests/mesos.hpp 79bf1ff16412ce2a510a9b75ab1ac91c1c182653 
>   src/tests/mesos.cpp 629135f0dc59346f0fcddb2cbe65ca5770fad34e 
> 
> Diff: https://reviews.apache.org/r/47558/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>