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



Bad patch!

Reviews applied: [47511]

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

Error:
2016-05-26 05:45:06 URL:https://reviews.apache.org/r/47511/diff/raw/ 
[11768/11768] -> "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/13335/console

- Mesos ReviewBot


On May 26, 2016, 3:19 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47511/
> ---
> 
> (Updated May 26, 2016, 3:19 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 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!
> 
> Guangya Liu wrote:
> Thanks Greg, one minor comment for this: can we simplify the sentense as 
> that the `principal` will be ignored for such case?
> 
> Greg Mann wrote:
> It's not quite correct to say that the principal is ignored in such 
> cases; the principal in `DiskInfo` might be used for authorization even if 
> the framework doesn't provided a principal when registering.

Got it, thanks Greg. I think that it would make sense if you can add above info 
to the document to make sure end user know the role of principal in DiskInfo 
even if framework does not register with a principal.


- 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 47844: Agent: Added support for `slave/main.cpp`.

[Daniel Pravat]

> On May 25, 2016, 9:04 p.m., Joris Van Remoortere wrote:
> > src/slave/main.cpp, lines 153-162
> > 
> >
> > Can we add a JIRA (and reference it in a comment) to add support for 
> > this?
> > I know a lot of people like this feature.

This is fixed with https://reviews.apache.org/r/47874/.


- Daniel


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


On May 25, 2016, 6:48 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47844/
> ---
> 
> (Updated May 25, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3614
> https://issues.apache.org/jira/browse/MESOS-3614
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added support for `slave/main.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
> 
> Diff: https://reviews.apache.org/r/47844/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47874: Windows: Added support for `--ip_discovery_command` parameter.

[Daniel Pravat]

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

(Updated May 26, 2016, 5:24 a.m.)


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


Repository: mesos


Description
---

Windows: Added support for `--ip_discovery_command` parameter.


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
ea6ae5ab981f422993085e63543d184a8e41524d 

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


Testing (updated)
---

Windows: build/run


Thanks,

Daniel Pravat

Review Request 47874: Windows: Added support for `--ip_discovery_command` parameter.

[Daniel Pravat]

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

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


Repository: mesos


Description
---

Windows: Added support for `--ip_discovery_command` parameter.


Diffs
-

  3rdparty/stout/include/stout/os/windows/shell.hpp 
ea6ae5ab981f422993085e63543d184a8e41524d 

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


Testing
---


Thanks,

Daniel Pravat

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47149, 47205, 47212, 47215, 47150, 47216]

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 26, 2016, 2:21 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47216/
> ---
> 
> (Updated May 26, 2016, 2:21 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
> ---
> 
> 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
> -
> 
>   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
> 
>

Review Request 47873: Windows: Used specific buffer size for `setvbuf`.

[Daniel Pravat]

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

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


Repository: mesos


Description
---

Microsoft MSVCCRT requires a value for the buffer size.


Diffs
-

  src/exec/exec.cpp 69a1fb256fe3005e814833ecac5a5a642b585510 

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


Testing
---

Windows: build/test


Thanks,

Daniel Pravat

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 26, 2016, 4:56 a.m.)


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


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
* VIEW_TASK
* VIEW_EXECUTOR

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:
* ViewFramework  (giving access to frameworks running under
a specific OS user)
* ViewTask  (giving access to Tasks run under a
   specific OS user)
* ViewExecutors (giving access to Executors run under a
   specific OS user)


Diffs (updated)
-

  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

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 26, 2016, 4:44 a.m.)


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


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


Repository: mesos


Description (updated)
---

In order to allow for framework and task level filtering we introduce
the following authorizer actions:
* VIEW_FRAMEWORK
* VIEW_TASK
* VIEW_EXECUTOR

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:
* ViewFramework  (giving access to frameworks running under
a specific OS user)
* ViewTask  (giving access to Tasks run under a
   specific OS user)
* ViewExecutors (giving access to Executors run under a
   specific OS user)


Diffs (updated)
-

  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

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




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


running



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


Above



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


above



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


2 indents



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


use



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


atemp



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


are set, attempts



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


FrameworkInfo



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


remove else



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


FrameworkInfo



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


remove else



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


Similar default.


- Joerg Schad


On May 26, 2016, 1:07 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46613/
> ---
> 
> (Updated May 26, 2016, 1:07 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> 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
> * VIEW_TASK
> * VIEW_EXECUTOR
> 
> 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:
> * ViewFramework  (giving access to frameworks running under
>   a specific OS user)
> * ViewTask  (giving access to Tasks run under a
> specific OS user)
> * ViewExecutors (giving access to Executors run under a
> specific OS user)
> 
> 
> Diffs
> -
> 
>   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
> 
>

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 26, 2016, 4:43 a.m.)


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


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 f666535bcaf8ec263eada7e0cf925d8eb3836ba8 
  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 47490: Moved `Task` to public protobufs.

[Joerg Schad]

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

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


Review request for mesos and Michael Park.


Changes
---

Rebased.


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


Repository: mesos


Description
---

As we need to authorize Tasks we need to include `Task` in
the public protobufs. This move also requires a the json logic to
adapt.


Diffs (updated)
-

  include/mesos/mesos.proto f666535bcaf8ec263eada7e0cf925d8eb3836ba8 
  include/mesos/type_utils.hpp 27fa8c9256c24b5ec6f5d259305ea66b5f556673 
  src/common/http.hpp f47e358a82058408212d9744068641211b37469d 
  src/common/http.cpp ad6a4b44af3ec847a3a6c0839a88fba18cda5011 
  src/common/type_utils.cpp 037c4336276258d671d0b1bf66cdab50b5bf9fb8 
  src/messages/messages.cpp 41dcdb3996ce173fb0a56704053e4b4e03f6dd63 
  src/messages/messages.proto 7fd3a2a3540e57bd1ce02a15de54123bf22b074c 

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


Testing
---

tested entire chain.


Thanks,

Joerg Schad

Re: Review Request 47866: Updated committers.md.

[Joris Van Remoortere]

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


Ship it!




- Joris Van Remoortere


On May 26, 2016, 12:06 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47866/
> ---
> 
> (Updated May 26, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added committers that were not updated on the committers doc.
> 
> 
> Diffs
> -
> 
>   docs/committers.md bb77ed1c1cf761380a4c7cd281267b342b604f13 
> 
> Diff: https://reviews.apache.org/r/47866/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

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 May 26, 2016, 3:19 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]

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

(Updated May 26, 2016, 3:16 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 47865: Sorted values of the enums authorization::Action and ACLs.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47864, 47794, 47795, 47865]

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 26, 2016, 1:09 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47865/
> ---
> 
> (Updated May 26, 2016, 1:09 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unsorted entries in an enum could cause confusion for developers
> writing code agains the enum and/or when introducing new values.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/acls.proto 
> b178f53a299a2941afc073af963f6aff26af1ca8 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
> 
> Diff: https://reviews.apache.org/r/47865/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

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 25, 2016, 7:21 p.m.)


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


Changes
---

Rebase and kick off tests!


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 47215: Changed the dockerized docker command executor CommandInfo usage.

[Joseph Wu]

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

(Updated May 25, 2016, 7:21 p.m.)


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


Changes
---

Explicitly perform the flag stringification inside the docker containerizer 
helper.


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


Repository: mesos


Description
---

This changes how we override the `CommandInfo` when launching a 
dockerized executor; from `shell == true` to `shell = false`.  
This means that flags are now passed directly rather than as 
a long string.

i.e. 
From: 'mesos-docker-executor --foo="bar" --some="thing"'
To: [ 'mesos-docker-executor', '--foo=bar', '--some=thing' ]


Diffs (updated)
-

  src/slave/containerizer/docker.cpp 7af14f4472283ceefd73c06dd8df60af4cf6f7e8 

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


Testing
---

make check


Thanks,

Joseph Wu

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

[Joseph Wu]

> On May 25, 2016, 6:50 p.m., Jie Yu wrote:
> > include/mesos/hook.hpp, lines 108-116
> > 
> >
> > Any reason why all the fields here are needed? For instance, 
> > 'containerInfo' is part of either taskInfo or executorInfo, why bother 
> > passing this duplicated information? Similarily, 'resources' can be derived 
> > as well.

I haven't dug too deeply into what each value could be in the various 
DockerContainerizer paths, but the interface is meant to be almost identical to 
`slavePreLaunchDockerHook`, except for the return type.


- Joseph


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


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 47205: Added optional environment variable argument to mesos-docker-executor.

[Joseph Wu]

> On May 25, 2016, 6:18 p.m., Jie Yu wrote:
> > src/docker/executor.hpp, line 90
> > 
> >
> > Can this be `Option`?

If I make this a `JSON::Object`, the invocation site 
(https://reviews.apache.org/r/47216/diff/2#1) will do one extra copy (see the 
next section):
It would go:
-> `map`
-> `map` 
-> `string`

Instead of:
-> `map`
-> `string`

The parsing path remains unchanged, except there's one less `Try` 
in the executor's main.
I prefer the current flag type.

---

The conversion would look like:
```
map task_environment = ;

JSON::Object json;
foreachpair (const string& key, const string& value, task_environment) {
  json.values[key] = value;
}
```


- Joseph


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


On May 25, 2016, 7:02 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 25, 2016, 7:02 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
> ---
> 
> 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
> -
> 
>   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 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 25, 2016, 7:02 p.m.)


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


Changes
---

Reorder some flags.


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 47149: Split DockerContainerizerProcess::launch into two functions.

[Joseph Wu]

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

(Updated May 25, 2016, 7:02 p.m.)


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


Changes
---

Change lambda::bind to defer.  Add containers map check to _launch.


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


Repository: mesos


Description
---

This prepares the `::launch` method for an asynchronous hook.


Diffs (updated)
-

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

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


Testing
---

sudo make check (CentOS 7)


Thanks,

Joseph Wu

Re: Review Request 47214: Replaced subprocess flag stringification with flags.toVector().

[Joseph Wu]

> On May 25, 2016, 6:25 p.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 150-152
> > 
> >
> > Is this change absolutely necessary for the ticket?
> 
> Jie Yu wrote:
> Oh, i see that being useful in the following patch.
> 
> Jie Yu wrote:
> In fact, i still prefer iterating all the flags in the callers, just to 
> be more explicit. It's not clear to me here what toVector means by just 
> looking at the name.

Ok, I can drop this patch and the previous one 
(https://reviews.apache.org/r/47213/).  

And then modify the following patch (https://reviews.apache.org/r/47215/) to 
explicitly enumerate the flags.


- Joseph


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


On May 16, 2016, 11:01 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47214/
> ---
> 
> (Updated May 16, 2016, 11:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
> 
> Diff: https://reviews.apache.org/r/47214/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47214: Replaced subprocess flag stringification with flags.toVector().

[Jie Yu]

> On May 26, 2016, 1:25 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 150-152
> > 
> >
> > Is this change absolutely necessary for the ticket?
> 
> Jie Yu wrote:
> Oh, i see that being useful in the following patch.

In fact, i still prefer iterating all the flags in the callers, just to be more 
explicit. It's not clear to me here what toVector means by just looking at the 
name.


- Jie


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


On May 16, 2016, 6:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47214/
> ---
> 
> (Updated May 16, 2016, 6:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
> 
> Diff: https://reviews.apache.org/r/47214/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47866: Updated committers.md.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47866]

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 26, 2016, 12:06 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47866/
> ---
> 
> (Updated May 26, 2016, 12:06 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
> Remoortere.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added committers that were not updated on the committers doc.
> 
> 
> Diffs
> -
> 
>   docs/committers.md bb77ed1c1cf761380a4c7cd281267b342b604f13 
> 
> Diff: https://reviews.apache.org/r/47866/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

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 26, 2016, 1:36 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
-

  src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 

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


Testing
---

make check (OSX)


Thanks,

Joerg Schad

Re: Review Request 45377: Made "driver" as optional for DockerVolume.

[Guangya Liu]

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

(Updated 五月 26, 2016, 1:34 a.m.)


Review request for mesos, David vonThenen, Gilbert Song, haosdent huang, and 
Jie Yu.


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


Repository: mesos


Description
---

Made "driver" as optional for DockerVolume.


Diffs (updated)
-

  include/mesos/mesos.proto f666535bcaf8ec263eada7e0cf925d8eb3836ba8 
  include/mesos/v1/mesos.proto ce187e71003f82852a8addadfdbc7c6f82dfaa2b 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
8ab2cf39a6bc3dd841b51639942b3e5258b7292e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

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

[Joerg Schad]

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

(Updated May 26, 2016, 1:29 a.m.)


Review request for mesos, Adam B and Michael Park.


Summary (updated)
-

Used Tasked ObjectAllower to filter /tasks endpoint.


Repository: mesos


Description (updated)
---

Used Tasked ObjectAllower 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 26, 2016, 1:28 a.m.)


Review request for mesos and Michael Park.


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 47214: Replaced subprocess flag stringification with flags.toVector().

[Jie Yu]

> On May 26, 2016, 1:25 a.m., Jie Yu wrote:
> > 3rdparty/libprocess/src/subprocess.cpp, lines 150-152
> > 
> >
> > Is this change absolutely necessary for the ticket?

Oh, i see that being useful in the following patch.


- Jie


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


On May 16, 2016, 6:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47214/
> ---
> 
> (Updated May 16, 2016, 6:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
> 
> Diff: https://reviews.apache.org/r/47214/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47214: Replaced subprocess flag stringification with flags.toVector().

[Jie Yu]

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




3rdparty/libprocess/src/subprocess.cpp (lines 150 - 152)


Is this change absolutely necessary for the ticket?


- Jie Yu


On May 16, 2016, 6:01 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47214/
> ---
> 
> (Updated May 16, 2016, 6:01 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
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
> 
> Diff: https://reviews.apache.org/r/47214/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Jie Yu]

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




src/docker/executor.hpp (line 89)


kill this line. In fact, I would reorder and move the one that's being 
deprecated to the end.



src/docker/executor.hpp (line 90)


Can this be `Option`?



src/docker/executor.cpp (line 680)


json->values



src/docker/executor.cpp (line 685)


I would align `<<` with the `<<` above.


- Jie Yu


On May 24, 2016, 9:02 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47205/
> ---
> 
> (Updated May 24, 2016, 9:02 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
> ---
> 
> 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
> -
> 
>   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 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 26, 2016, 1:16 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 47865: Sorted values of the enums authorization::Action and ACLs.

[Alexander Rojas]

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

(Updated May 26, 2016, 3:09 a.m.)


Review request for mesos and Adam B.


Summary (updated)
-

Sorted values of the enums authorization::Action and ACLs.


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


Repository: mesos


Description
---

Unsorted entries in an enum could cause confusion for developers
writing code agains the enum and/or when introducing new values.


Diffs (updated)
-

  include/mesos/authorizer/acls.proto b178f53a299a2941afc073af963f6aff26af1ca8 
  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check


Thanks,

Alexander Rojas

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 26, 2016, 1:09 a.m.)


Review request for mesos, Adam B and Michael Park.


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


Repository: mesos


Description
---

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/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
  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.

[Alexander Rojas]

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

(Updated May 26, 2016, 3:09 a.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 (updated)
-

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

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"] },
  "users" : { "values" : ["$USER"] }
}
  ]
}
EOF

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

./src/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 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 26, 2016, 1:07 a.m.)


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


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
* VIEW_TASK
* VIEW_EXECUTOR

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:
* ViewFramework  (giving access to frameworks running under
  a specific OS user)
* ViewTask  (giving access to Tasks run under a
specific OS user)
* ViewExecutors (giving access to Executors 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

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

(Updated May 26, 2016, 3:07 a.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 (updated)
-

  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 47864: Extended authorization::Object message with optional fields.

[Alexander Rojas]

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

(Updated May 26, 2016, 3:06 a.m.)


Review request for mesos, Adam B, Joerg Schad, and Michael Park.


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


Repository: mesos


Description (updated)
---

Adds optional fields to `authorization::Object` in order to allow
for complex authorization mechanisms.

The fields are:

- `ExecutorInfo`
- `FrameworkInfo`
- `TaskInfo`


Diffs (updated)
-

  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

[Jiang Yan Xu]

> On May 25, 2016, 1:24 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, lines 90-91
> > 
> >
> > Is this a guard as opposed to a `CHECK` because someone is using this 
> > API correctly and we are mitigating the performance impact of that?
> > 
> > We could argue that it shouldn't be a `CHECK` because it's not an 
> > internal variant; however, I would like to know if there's a higher level 
> > bug we should be fixing :-)

I don't think it should be a CHECK here but it should be a CHECK in /r/47259/ 
as I commented on /r/47259/ for CHECK vs. if guard. 

The motivation for this patch is from debugging /r/43666/. 

> We could argue that it shouldn't be a CHECK because it's not an internal 
> variant;

I think this is exactly right. The "high-level bug" is probably that due to the 
tight coupling between the allocator and the sorter we are forced to CHECK 
public method arguments as well. I think we should miminize this.


- Jiang Yan


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


On May 19, 2016, 11:27 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 11:27 a.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47149: Split DockerContainerizerProcess::launch into two functions.

[Jie Yu]

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




src/slave/containerizer/docker.cpp (lines 1025 - 1032)


Instead of using lambda::bind, you should use `defer(self(), 
::_launch, ...)`. The reason is because `f` might have a longer life cycle 
than `this`.



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


2 lines apart here.



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


What if containerizer->destroy is called between `_launch` and `launch`? Do 
you need to check if `containers_.contains(containerId)` and return a failure 
if it's no longer there?


- Jie Yu


On May 11, 2016, 2:13 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47149/
> ---
> 
> (Updated May 11, 2016, 2:13 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
> ---
> 
> This prepares the `::launch` method for an asynchronous hook.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp fac26a239e9981e5b1c2f2f90b52785819492c2a 
>   src/slave/containerizer/docker.cpp 7af14f4472283ceefd73c06dd8df60af4cf6f7e8 
> 
> Diff: https://reviews.apache.org/r/47149/diff/
> 
> 
> Testing
> ---
> 
> sudo make check (CentOS 7)
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47865: Sorted values of the enum authorization::Action.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47794, 47864, 47795, 47865]

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, 11:20 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47865/
> ---
> 
> (Updated May 25, 2016, 11:20 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unsorted entries in an enum could cause confusion for developers
> writing code agains the enum and/or when introducing new values.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
> 
> Diff: https://reviews.apache.org/r/47865/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47259: CHECK if DRFSorter::add() would introduce a duplicate.

[Jiang Yan Xu]

> On May 25, 2016, 1:28 p.m., Joris Van Remoortere wrote:
> > src/master/allocator/sorter/drf/sorter.cpp, line 50
> > 
> >
> > Why can this be a `CHECK` if the previous review can't?
> > What is the standard we're trying to set for invariants?

We have been using CHECKs and if guards inconsistently in the sorter. 

Normally we shouldn't use CHECKs on public method arguments (i.e., they are not 
invariants) but it's widely used in sorter due to the tight coupling between 
the allocator and the sorter and it's either simpler for the allocator to not 
have to handle an error/none/false result or sometimes it's impossible to 
handle it. I filed MESOS-5280 for the inconsistency but now I feel it's perhaps 
not worth making all the methods return error/none/false instead of doing 
CHECKs, it could be discussed there though.

So if we don't replace CHECKs with error/none/false returns, should we always 
CHECK and crash the problem? I feel that will be too much.

The difference between this patch and the previous is (as I commented on 
MESOS-5279):
- It's acceptable to do nothing when the **exact** thing it is asked to do is 
already done (this doesn't get us into a bad interanl state). (The case in 
/r/47258/)
- It's not acceptable to silently refuse to do something not because it's 
already done but it's impossible to do it because it gets us into a bad 
internal state (duplicate entries). (The case in here /r/47259/, as the caller 
could first call `sorter->add("fw1", 1)` and then `sorter->add("fw1", 2)` which 
would be a **different** thing)

P.S. We should always CHECK internal invariants in the sorter for sure.


- Jiang Yan


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


On May 19, 2016, 11:28 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47259/
> ---
> 
> (Updated May 19, 2016, 11:28 a.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHECK if DRFSorter::add() would introduce a duplicate.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
> 
> Diff: https://reviews.apache.org/r/47259/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Review Request 47866: Updated committers.md.

[Vinod Kone]

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

Review request for mesos, Alexander Rukletsov, Bernd Mathiske, and Joris Van 
Remoortere.


Repository: mesos


Description
---

Added committers that were not updated on the committers doc.


Diffs
-

  docs/committers.md bb77ed1c1cf761380a4c7cd281267b342b604f13 

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


Testing
---

N/A


Thanks,

Vinod Kone

Re: Review Request 47865: Sorted values of the enum authorization::Action.

[Adam B]

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



Please also reorder the `ACLs` in `authorizer/acls.proto`

- Adam B


On May 25, 2016, 4:20 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47865/
> ---
> 
> (Updated May 25, 2016, 4:20 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Unsorted entries in an enum could cause confusion for developers
> writing code agains the enum and/or when introducing new values.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
> 
> Diff: https://reviews.apache.org/r/47865/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47864: Extended authorization::Object message with optional fields.

[Adam B]

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


Fix it, then Ship it!




Update the ordering and the comment, and I'm ready to ship it!


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


"The `Object` for a particular request action is described by either its 
string value or various metadata Info."



include/mesos/authorizer/authorizer.proto (lines 40 - 43)


I'd suggest we reorder these: FrameworkInfo, ExecutorInfo, TaskInfo, then 
Task.


- Adam B


On May 25, 2016, 4:15 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47864/
> ---
> 
> (Updated May 25, 2016, 4:15 p.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Michael Park.
> 
> 
> Bugs: MESOS-5153
> https://issues.apache.org/jira/browse/MESOS-5153
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds optional fields to `authorization::Object` in order to allow
> for complex authorization mechanisms.
> 
> The fields are:
> 
> - `FrameworkInfo`
> - `TaskInfo`
> - `ExecutorInfo`
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.proto 
> 911a2271211249a41c4467f6754e9996f640bf38 
> 
> Diff: https://reviews.apache.org/r/47864/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 47795: Enabled authorization for sandboxes.

[Alexander Rojas]

> On May 25, 2016, 6:34 a.m., Adam B wrote:
> > 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.
> 
> Alexander Rojas wrote:
> I will do it in a follow up patch, although I think AlexR did want them 
> this way.

Done in [r/47865/](https://reviews.apache.org/r/47865/)


> On May 25, 2016, 6:34 a.m., Adam B wrote:
> > include/mesos/authorizer/authorizer.proto, lines 58-59
> > 
> >
> > Not yours, but.. Please move these down to their spot in numerical 
> > order.

Done in [r/47865/](https://reviews.apache.org/r/47865/)


- Alexander


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


On May 24, 2016, 11: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, 11: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
> 
>

Review Request 47865: Sorted values of the enum authorization::Action.

[Alexander Rojas]

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

Review request for mesos and Adam B.


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


Repository: mesos


Description
---

Unsorted entries in an enum could cause confusion for developers
writing code agains the enum and/or when introducing new values.


Diffs
-

  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check


Thanks,

Alexander Rojas

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

(Updated May 26, 2016, 1:18 a.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 (updated)
-

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

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


Testing (updated)
---

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"] },
  "users" : { "values" : ["$USER"] }
}
  ]
}
EOF

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

./src/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 47864: Extended authorization::Object message with optional fields.

[Alexander Rojas]

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

Review request for mesos, Adam B, Joerg Schad, and Michael Park.


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


Repository: mesos


Description
---

Adds optional fields to `authorization::Object` in order to allow
for complex authorization mechanisms.

The fields are:

- `FrameworkInfo`
- `TaskInfo`
- `ExecutorInfo`


Diffs
-

  include/mesos/authorizer/authorizer.proto 
911a2271211249a41c4467f6754e9996f640bf38 

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


Testing
---

make check


Thanks,

Alexander Rojas

Re: Review Request 47795: Enabled authorization for sandboxes.

[Alexander Rojas]

> On May 25, 2016, 6:34 a.m., Adam B wrote:
> > 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.

I will do it in a follow up patch, although I think AlexR did want them this 
way.


> On May 25, 2016, 6:34 a.m., Adam B wrote:
> > 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.

The reason here is that the framework id is not necesarily available when doing 
executor recovering. However the code can be made somewhat more efficient.


- Alexander


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


On May 24, 2016, 11: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, 11: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 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/#review134839
---




src/master/http.cpp (line 1573)


use std::tie



src/master/http.cpp (line 1648)


move to function


- Joerg Schad


On May 25, 2016, 4:58 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47704/
> ---
> 
> (Updated May 25, 2016, 4:58 p.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used Tasked ObjectAllower to filter /state endpoint.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp b36b439a1fa07c52146deff2b90728f92676ade3 
> 
> Diff: https://reviews.apache.org/r/47704/diff/
> 
> 
> Testing
> ---
> 
> Make check (OSX)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47821: Remove SASL dependency for Windows builds.

[Till Toenshoff]

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



Thanks for proposing this workaround for the SASL on Windows challenge. I must 
confess that I am not really *happy* with this but given our constraints, I 
believe the way you reduced my original patch is indeed least painful ;). So it 
looks good for me, barring the needed comment rewording.


cmake/CompilationConfigure.cmake (lines 132 - 139)


Could you please reword this (and the other) comment to reflect the fact 
that we do have a pluggable authentication layer?

I did add some clues for such reword into the linked JIRA issue. Given that 
I am not a native speaker, I guess you can do a much better job than I ever 
could for such updated comment :)
Please note that we commonly have the "link" to the JIRA issue as trailing 
sentence; "... See MESOS-."


- Till Toenshoff


On May 25, 2016, 6:45 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47821/
> ---
> 
> (Updated May 25, 2016, 6:45 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5450
> https://issues.apache.org/jira/browse/MESOS-5450
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SASL is currently a hard dependency for Mesos. Per MESOS-5450, we expect
> that some platforms will not support SASL (namely Windows), so this
> commit will begin the first step in a several-step process of removing
> it as a hard dependency.
> 
> For this first step, this commit will shed SASL dependency in libmesos
> only for Windows builds. We do this by:
> 
>   (1) Adding a preprocessor symbol, `HAS_AUTHENTICATION` that wraps the
>   SASL-dependent code, so that we can conditionally choose not to
>   compile it.
>   (2) Defining `HAS_AUTHENTICATION` on all Unix builds, and leaving it
>   undefined on all Windows builds.
>   (3) Logging an error and exiting the master if the user passes in
>   flags that depend on SASL, such as `--authenticate`.
> 
> Notably, what we do *not* do is:
> 
>   (1) Shed SASL dependency in the tests.
> 
> The impact of this is that, on Windows, relevant libmesos tests will
> either not compile, or not pass. Naturally, as tracked by MESOS-5450,
> the second phase of this series will be to shed the test dependency as
> well.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5c7833ceaed556cc4ffb650996e918c1a542c5f0 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
> 
> Diff: https://reviews.apache.org/r/47821/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47265: Fixed provisioner parameter naming.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 11, 2016, 10:46 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47265/
> ---
> 
> (Updated May 11, 2016, 10:46 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed provisioner parameter naming.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.hpp 
> 5c96e9f6603d39889e6bc807874d35d0cb3556be 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> dcbbbaf797d4467bfd0bb1ee91ee9ce843e7d546 
> 
> Diff: https://reviews.apache.org/r/47265/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47843: Agent: Added Windows support for launcher/fetcher.cpp.

[Joris Van Remoortere]

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




src/launcher/fetcher.cpp (lines 76 - 80)


can we use the updated `paths::join` here to simplify this?

Can you add a JIRA to add an OS agnostic helper to quote command line 
arguments? This would have simplified this review a LOT :-)



src/launcher/fetcher.cpp (lines 112 - 113)


Please add a comment referencing the JIRA for adding support to Windows for 
hadoop



src/launcher/fetcher.cpp (lines 179 - 185)


So is the expecation here that we may receive linux style paths, and we 
want them to work flawlessly on windows?
Can we prefix the code with that goal?



src/launcher/fetcher.cpp (line 190)


Not sure why you're changing this check.
The posix version isn't restricted to returning -1 for error?



src/launcher/fetcher.cpp (lines 249 - 250)


Comment.



src/launcher/fetcher.cpp (line 309)


Is necessary the right word?
It's only 'not necessary' because we don't have a permissions model on 
Windows, right?



src/launcher/fetcher.cpp (line 374)


same comment as above.


- Joris Van Remoortere


On May 25, 2016, 6:48 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47843/
> ---
> 
> (Updated May 25, 2016, 6:48 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
> ---
> 
> Agent: Added Windows support for launcher/fetcher.cpp.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp d323f6341ab8367eeb456c9f399395293960fb66 
> 
> Diff: https://reviews.apache.org/r/47843/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47844: Agent: Added support for `slave/main.cpp`.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/slave/main.cpp (lines 153 - 162)


Can we add a JIRA (and reference it in a comment) to add support for this?
I know a lot of people like this feature.


- Joris Van Remoortere


On May 25, 2016, 6:48 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47844/
> ---
> 
> (Updated May 25, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3614
> https://issues.apache.org/jira/browse/MESOS-3614
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added support for `slave/main.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
> 
> Diff: https://reviews.apache.org/r/47844/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

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




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


task only



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


executor only



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


`VIEW_FRAMEWORK` will FwkInfo set.  

add blank lines in between.



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


`VIEW_TASK` will have (Task or TINFO) and FWKInf set



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


.. and Fwkinfo for the case



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


static



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


static



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


_acls/acls



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


members receive trailing underscore



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


{ next line



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


check indentation



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


make this CHECK



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


identation



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


move &&



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


indentation



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


else if


- Joerg Schad


On May 25, 2016, 4:55 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46613/
> ---
> 
> (Updated May 25, 2016, 4:55 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> 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
> * VIEW_TASK
> * VIEW_EXECUTOR
> 
> 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:
> * ViewFramework  (giving access to frameworks running under
>   a specific OS user)
> * ViewTask  (giving access to Tasks run under a
> specific OS user)
> * ViewExecutors (giving access to Executors run under a
> specific OS user)
> 
> 
> Diffs
> -
> 
>   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
> 
>

Re: Review Request 47842: Point slave flags at programmatic temp path.

[Joris Van Remoortere]

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




src/slave/flags.cpp (line 34)


maybe we can call this `tmpMesos` to follow `camelCase`, and so the name 
matches the structure.

You will need to rebase this on the relocation of the pach that introduces 
`os::temp`


- Joris Van Remoortere


On May 25, 2016, 6:47 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47842/
> ---
> 
> (Updated May 25, 2016, 6:47 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Point slave flags at programmatic temp path.
> 
> 
> Diffs
> -
> 
>   src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
> 
> Diff: https://reviews.apache.org/r/47842/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47470: Stout: Added `path::temp_path`.

[Joris Van Remoortere]

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



As discussed offline:
Let's move this into the `os` namespace.
Let's rename this to `temp`.
Let's consider returning the `Path` type. If it doesn't make sense we can fall 
back to `string`.

- 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/47470/
> ---
> 
> (Updated May 17, 2016, 4:02 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Stout: Added `path::temp_path`.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/Makefile.am 9b39ce32c0269479066cf7991afaeed65d8ab547 
>   3rdparty/stout/include/stout/path.hpp 
> ef538045a8b7a1e3d8962c869317d86a85e0259f 
>   3rdparty/stout/include/stout/posix/path.hpp PRE-CREATION 
>   3rdparty/stout/include/stout/windows/path.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47470/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47259: CHECK if DRFSorter::add() would introduce a duplicate.

[Joris Van Remoortere]

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




src/master/allocator/sorter/drf/sorter.cpp (line 50)


Why can this be a `CHECK` if the previous review can't?
What is the standard we're trying to set for invariants?


- Joris Van Remoortere


On May 19, 2016, 6:28 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47259/
> ---
> 
> (Updated May 19, 2016, 6:28 p.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> CHECK if DRFSorter::add() would introduce a duplicate.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
> 
> Diff: https://reviews.apache.org/r/47259/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47258: Make sure 'activate' is a no-op if the client is already active.

[Joris Van Remoortere]

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




src/master/allocator/sorter/drf/sorter.cpp (lines 90 - 91)


Is this a guard as opposed to a `CHECK` because someone is using this API 
correctly and we are mitigating the performance impact of that?

We could argue that it shouldn't be a `CHECK` because it's not an internal 
variant; however, I would like to know if there's a higher level bug we should 
be fixing :-)


- Joris Van Remoortere


On May 19, 2016, 6:27 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47258/
> ---
> 
> (Updated May 19, 2016, 6:27 p.m.)
> 
> 
> Review request for mesos, Dario Rexin and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Make sure 'activate' is a no-op if the client is already active.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.cpp 
> 4306973277b9d32356eed31ceabac09fb2a03e6c 
>   src/master/allocator/sorter/sorter.hpp 
> 9e04adf54f2d80541a95f0a9a49b329dc9e8f5e3 
> 
> Diff: https://reviews.apache.org/r/47258/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47257: Fixed an unassigned member variable in DRF sorter.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On May 19, 2016, 6:26 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47257/
> ---
> 
> (Updated May 19, 2016, 6:26 p.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5279
> https://issues.apache.org/jira/browse/MESOS-5279
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed an unassigned member variable in DRF sorter.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/sorter/drf/sorter.hpp 
> 05d5205d29ad74e01e07c508d88b6f8371541513 
> 
> Diff: https://reviews.apache.org/r/47257/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 47844: Agent: Added support for `slave/main.cpp`.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [47844, 47843, 47842, 47442, 47489, 47474, 47486, 47492, 47473]

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

Error:
2016-05-25 19:40:44 URL:https://reviews.apache.org/r/47844/diff/raw/ 
[1314/1314] -> "47844.patch" [1]
error: patch failed: src/slave/main.cpp:158
error: src/slave/main.cpp: patch does not apply

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

- Mesos ReviewBot


On May 25, 2016, 6:48 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47844/
> ---
> 
> (Updated May 25, 2016, 6:48 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-3614
> https://issues.apache.org/jira/browse/MESOS-3614
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Agent: Added support for `slave/main.cpp`.
> 
> 
> Diffs
> -
> 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
> 
> Diff: https://reviews.apache.org/r/47844/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47821: Remove SASL dependency for Windows builds.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [47821, 47671, 47442, 47489, 47474, 47486, 47492, 47473]

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

Error:
2016-05-25 19:07:36 URL:https://reviews.apache.org/r/47821/diff/raw/ 
[5978/5978] -> "47821.patch" [1]
error: patch failed: src/master/master.cpp:429
error: src/master/master.cpp: patch does not apply

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

- Mesos ReviewBot


On May 25, 2016, 6:45 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47821/
> ---
> 
> (Updated May 25, 2016, 6:45 p.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5450
> https://issues.apache.org/jira/browse/MESOS-5450
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> SASL is currently a hard dependency for Mesos. Per MESOS-5450, we expect
> that some platforms will not support SASL (namely Windows), so this
> commit will begin the first step in a several-step process of removing
> it as a hard dependency.
> 
> For this first step, this commit will shed SASL dependency in libmesos
> only for Windows builds. We do this by:
> 
>   (1) Adding a preprocessor symbol, `HAS_AUTHENTICATION` that wraps the
>   SASL-dependent code, so that we can conditionally choose not to
>   compile it.
>   (2) Defining `HAS_AUTHENTICATION` on all Unix builds, and leaving it
>   undefined on all Windows builds.
>   (3) Logging an error and exiting the master if the user passes in
>   flags that depend on SASL, such as `--authenticate`.
> 
> Notably, what we do *not* do is:
> 
>   (1) Shed SASL dependency in the tests.
> 
> The impact of this is that, on Windows, relevant libmesos tests will
> either not compile, or not pass. Naturally, as tracked by MESOS-5450,
> the second phase of this series will be to shed the test dependency as
> well.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 5c7833ceaed556cc4ffb650996e918c1a542c5f0 
>   src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
>   src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
> 
> Diff: https://reviews.apache.org/r/47821/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Review Request 47844: Agent: Added support for `slave/main.cpp`.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


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


Repository: mesos


Description
---

Agent: Added support for `slave/main.cpp`.


Diffs
-

  src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47702: Added framework role & principal to the Web UI.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On May 23, 2016, 9:08 p.m., Deshna Jain wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47702/
> ---
> 
> (Updated May 23, 2016, 9:08 p.m.)
> 
> 
> Review request for mesos, haosdent huang and Vinod Kone.
> 
> 
> Bugs: MESOS-4788
> https://issues.apache.org/jira/browse/MESOS-4788
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/framework.html 
> f108e92f5b8d9ed4de718e55aa7302728a84003e 
>   src/webui/master/static/frameworks.html 
> f172e022e18df5b6aa3d232e610c3c732e20aa09 
> 
> Diff: https://reviews.apache.org/r/47702/diff/
> 
> 
> Testing
> ---
> 
> make check (see attached screenshot for ui after the change)
> 
> 
> File Attachments
> 
> 
> image after making the change
>   
> https://reviews.apache.org/media/uploaded/files/2016/05/23/a2b9738e-b33a-4fce-98b0-9457d42a31f7__Screen_Shot_2016-05-23_at_12.38.01_PM.png
> 
> 
> Thanks,
> 
> Deshna Jain
> 
>

Review Request 47843: Agent: Added Windows support for launcher/fetcher.cpp.

[Alex Clemmer]

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

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

Agent: Added Windows support for launcher/fetcher.cpp.


Diffs
-

  src/launcher/fetcher.cpp d323f6341ab8367eeb456c9f399395293960fb66 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 47842: Point slave flags at programmatic temp path.

[Alex Clemmer]

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

(Updated May 25, 2016, 6:47 p.m.)


Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


Repository: mesos


Description
---

Point slave flags at programmatic temp path.


Diffs
-

  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 

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


Testing
---


Thanks,

Alex Clemmer

Review Request 47821: Remove SASL dependency for Windows builds.

[Alex Clemmer]

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

Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
Remoortere, and Michael Park.


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


Repository: mesos


Description
---

SASL is currently a hard dependency for Mesos. Per MESOS-5450, we expect
that some platforms will not support SASL (namely Windows), so this
commit will begin the first step in a several-step process of removing
it as a hard dependency.

For this first step, this commit will shed SASL dependency in libmesos
only for Windows builds. We do this by:

  (1) Adding a preprocessor symbol, `HAS_AUTHENTICATION` that wraps the
  SASL-dependent code, so that we can conditionally choose not to
  compile it.
  (2) Defining `HAS_AUTHENTICATION` on all Unix builds, and leaving it
  undefined on all Windows builds.
  (3) Logging an error and exiting the master if the user passes in
  flags that depend on SASL, such as `--authenticate`.

Notably, what we do *not* do is:

  (1) Shed SASL dependency in the tests.

The impact of this is that, on Windows, relevant libmesos tests will
either not compile, or not pass. Naturally, as tracked by MESOS-5450,
the second phase of this series will be to shed the test dependency as
well.


Diffs
-

  cmake/CompilationConfigure.cmake 5c7833ceaed556cc4ffb650996e918c1a542c5f0 
  src/Makefile.am 447bc2ab511ad173d3d911be10992be0974f4584 
  src/master/master.cpp 35b428b0f7dee5954514d8860cfc498271ccf267 
  src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 

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


Testing
---


Thanks,

Alex Clemmer

Re: Review Request 46371: Added basic tests for capabilities API.

[Jojy Varghese]

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

(Updated May 25, 2016, 6:39 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Added basic tests for capabilities API.


Diffs (updated)
-

  src/Makefile.am ce5245883f3d2661812272702c0d2060513b6d88 
  src/tests/capabilities_test_helper.hpp PRE-CREATION 
  src/tests/capabilities_test_helper.cpp PRE-CREATION 
  src/tests/capabilities_test_helper_main.cpp PRE-CREATION 
  src/tests/capabilities_tests.cpp PRE-CREATION 

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


Testing
---

make check.


Thanks,

Jojy Varghese

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

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

(Updated May 25, 2016, 6:38 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Changes
---

review addressed.


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 (updated)
-

  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 47082: LostSlaveMessage should be sent to affected frameworks only.

[Jiang Yan Xu]

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



I think for this patch we should at least have one test case where a slave lost 
message is not sent.
Perhaps two frameworks connected the master and the slave lost message is only 
sent to one because the other doesn't have any business with that slave?


src/master/master.hpp (line 24)


Use a hashset (already included)?



src/master/master.hpp (line 666)


Use a hashset since no ordering is required?



src/master/master.cpp (line 96)


Remove this if we use a hashset.



src/master/master.cpp (lines 6545 - 6546)


For 1: Also there are inverse offers.

And there is also 
3. They have running tasks on this slave.
4. They have pending tasks for this slave.

You did add framworks for running tasks below when you go through 
`slave->tasks` but I think here we should list all cases for which we send the 
message in one place.

I commented on the pending tasks in one of the tests.



src/master/master.cpp (line 6550)


This is about a single slave so instead of looping through all registered 
frameworks, we can look for things in this particular slave's data members.



src/master/master.cpp (line 6551)


I don't feel that this boolean is necessary, we can easily check if a 
framework is added by checking if it's in the set (O(1) if hashset), right?



src/master/master.cpp (lines 6555 - 6561)


We can loop through `slave->offers` for this and we should look at 
`slave->inverseOffers` as well.

Actually, we already go through offers and inverseOffers below so we can 
just add `affectedFrameworks.insert(framework->id());` there (each with a 
comment).



src/master/master.cpp (lines 6566 - 6570)


Looks like this is no longer applicable in the suggested approach.



src/master/master.cpp (lines 6576 - 6579)


From `slave->checkpointedResource.reservations()` we can get the list of 
roles and from `activeRoles` we can get the list of frameworks for each role.



src/master/master.cpp (line 6722)


Also put the slaveID in this log?

e.g., something like this

```
LOG(WARNING) << "Dropping LostSlaveMessage about agent " << slaveInfo.id() 
 << " for unknown framework " << frameworkId;
```



src/tests/master_authorization_tests.cpp (lines 338 - 339)


If we don't expect it to be call, we should do

```
EXPECT_CALL(sched, slaveLost(, _))
  .Times(0);
```

Here and elsewhere.

But here I think we should fix this to have the framework receive the slave 
lost messsage. See the comment below.



src/tests/master_authorization_tests.cpp (lines 346 - 348)


So if the task is stuck in the `pendingTasks` a slave lost message is not 
sent but later a TASK_LOST is sent with reason `REASON_SLAVE_REMOVED`... We 
should handle this case the same way as the other cases where we do send slave 
lost IMO.

We probably need to add a `pendingTasks` hashmap in the slave as well and 
check against that map during `removeSlave()`, thoughts?


- Jiang Yan Xu


On May 6, 2016, 7:27 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47082/
> ---
> 
> (Updated May 6, 2016, 7:27 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-5143
> https://issues.apache.org/jira/browse/MESOS-5143
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> When a slave is removed, master sends a LostSlaveMessage to affected
> frameworks only (instead of all registered frameworks). An affected
> framework is a framework which satisfied one or more conditions of
> the following:
> 
> 1. There are running tasks on this slave belonging to the framework.
> 2. Reserved resources on the slave have a matching role with the
>role of the framework.
> 3. There are pending offers from this slave for the framework.
> 
> 
> Diffs
> -
> 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/master/master.cpp 

Re: Review Request 47528: Updated validation tests with creator principal.

[Greg Mann]

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

(Updated May 25, 2016, 6:08 p.m.)


Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

The master validation tests are updated to include the
new `validate()` signature, and a new test is added,
`CreateOperationValidationTest.NonMatchingPrincipal`,
to ensure that a non-matching creator principal will
be invalidated.


Diffs (updated)
-

  src/tests/master_validation_tests.cpp 
ca4442aa1ef0087a7d058d1b3aa430a1dbc16960 

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


Testing
---

`make check` was used to test on OSX at the end of this review chain.


Thanks,

Greg Mann

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

[Greg Mann]

> On May 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!
> 
> Guangya Liu wrote:
> Thanks Greg, one minor comment for this: can we simplify the sentense as 
> that the `principal` will be ignored for such case?

It's not quite correct to say that the principal is ignored in such cases; the 
principal in `DiskInfo` might be used for authorization even if the framework 
doesn't provided a principal when registering.


- Greg


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


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 47695: Updated the website generation and development workflows with docker.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On May 24, 2016, 2:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47695/
> ---
> 
> (Updated May 24, 2016, 2:38 a.m.)
> 
> 
> Review request for mesos, Tomasz Janiszewski, Kevin Klues, Neil Conway, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5431
> https://issues.apache.org/jira/browse/MESOS-5431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In this changes, we include below items to make it more convenience to
> develop and generate the website.
> 
> * added `doxygen` and `javadoc` to `rake` default target.
> * added `build.sh` as the wrapper for the website generation to make
>   sure we clean up generated documents when exit.
> * switched the `Dockerfile` to based on `centos:7` because the default
>   `doxygen` version in `ubuntu:14.04` doesn't include `jquery.js`
>   correctly when generate `doxygen` documents.
> * adjusted the `Dockerfile` and `README.md` locations to avoid we
>   distract at finding the website generation document.
> * updated `release-guide.md` with the new website generation and
>   development workflows.
> 
> 
> Diffs
> -
> 
>   docs/release-guide.md 36e79796657a37b0b4649602281556f4ae471c34 
>   site/.gitignore 987a5d3cc161a67de1fb21c2002ed9698bbb0689 
>   site/Gemfile 14c50340d66fd7a703949a7932ab93a6b60f4e07 
>   site/Gemfile.lock 065107ec25f0eff28e2a09051b61152d49f77f9a 
>   site/README.md 468021ef4f8ed942c4a16784bc82349af9ae0d5e 
>   site/Rakefile 845ec3fd8b9e47ebe49991db30718912d4f9c41e 
>   site/build.sh PRE-CREATION 
>   support/site-docker/Dockerfile 8ea3d5c5fe531ec76950ed0bf110d7d477339158 
>   support/site-docker/README.md 957117e1211f06b40c5fb621039dd90aff562a87 
> 
> Diff: https://reviews.apache.org/r/47695/diff/
> 
> 
> Testing
> ---
> 
> Tested in OS X and Ubuntu 14.04
> 
> For document changes, could verified from 
> https://github.com/haosdent/mesos/tree/MESOS-5431/site
> 
> To verify website works fine after this changes, could check it via this gif:
> 
> ![website.gif](https://issues.apache.org/jira/secure/attachment/12805684/website.gif)
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 47795: Enabled authorization for sandboxes.

[Alexander Rojas]

> On May 25, 2016, 5:14 a.m., Guangya Liu wrote:
> > include/mesos/authorizer/authorizer.proto, line 69
> > 
> >
> > New line above

Any reason why we need a new line here? excep for the deprecate entries we 
don't seem to use line breaks in this enum.


> On May 25, 2016, 5:14 a.m., Guangya Liu wrote:
> > 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;

It is needed because of the line

```c++
authorization::Request realRequest;
```

Since that is a declaration of a variable in an optional path, it ends being an 
error if there is no scope here.


> On May 25, 2016, 5:14 a.m., Guangya Liu wrote:
> > src/authorizer/local/authorizer.cpp, lines 279-292
> > 
> >
> > Why not use `request` directly? Can you please show more detail or add 
> > some comments here?

Request is `const` and the `frameworkInfo.role()` is the only field that needs 
to be extracted, we need to create an alternate one.


- Alexander


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


On May 24, 2016, 11: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, 11: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 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/
---

(Updated May 25, 2016, 7:27 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 (updated)
-

  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.

[Alexander Rojas]

> On May 25, 2016, 5:27 a.m., Adam B wrote:
> > src/files/files.cpp, lines 309-310
> > 
> >
> > Can you wrap on `.then(` instead, please?

Sorry, you are right. This is the output generated by clang format.


- Alexander


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


On May 24, 2016, 11: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, 11: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 44839: Enabled mesos containerizer do not cache image for appc.

[Jie Yu]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/provisioner/appc/store.cpp (lines 230 - 231)


I would remove this log line because it is also likely that cached = true 
but imageId is not found in the cache.


- Jie Yu


On May 24, 2016, 2:01 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44839/
> ---
> 
> (Updated May 24, 2016, 2:01 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4886
> https://issues.apache.org/jira/browse/MESOS-4886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled mesos containerizer force_pull_image for appc.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/appc/store.cpp 
> 33e97fc645a9b7fbc9ae47f67c1b5dacf999fce0 
> 
> Diff: https://reviews.apache.org/r/44839/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 44837: Added cached field to Image protobuf.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 23, 2016, 11:30 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44837/
> ---
> 
> (Updated May 23, 2016, 11:30 p.m.)
> 
> 
> Review request for mesos, Gilbert Song, haosdent huang, and Jie Yu.
> 
> 
> Bugs: MESOS-4886
> https://issues.apache.org/jira/browse/MESOS-4886
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added cached field to Image protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 887ffe9c5f04e25539a4c6b3d52ce5299c65e8d3 
>   include/mesos/v1/mesos.proto 9e59aed20965d50ee10989ff6b75db742cf2b83b 
> 
> Diff: https://reviews.apache.org/r/44837/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

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:59 p.m.)


Review request for mesos, Adam B and Michael Park.


Changes
---

Rebased.


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, 4:58 p.m.)


Review request for mesos and Michael Park.


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, 4:57 p.m.)


Review request for mesos, Adam B and Michael Park.


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


Repository: mesos


Description
---

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 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 25, 2016, 4:55 p.m.)


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


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


Repository: mesos


Description (updated)
---

In order to allow for framework and task level filtering we introduce
the following authorizer actions:
* VIEW_FRAMEWORK
* VIEW_TASK
* VIEW_EXECUTOR

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:
* ViewFramework  (giving access to frameworks running under
  a specific OS user)
* ViewTask  (giving access to Tasks run under a
specific OS user)
* ViewExecutors (giving access to Executors 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

Re: Review Request 47608: Add Labels from TaskInfo into TaskStatus message.

[Shuai Lin]

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




src/slave/slave.cpp (line 3454)


Should we also search in in `termiantedTasks`?

```cpp
  // Terminated but pending updates.
  LinkedHashMap terminatedTasks;
```


- Shuai Lin


On May 20, 2016, 6:38 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47608/
> ---
> 
> (Updated May 20, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Add Labels from TaskInfo into TaskStatus message.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 
>   src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 
>   src/tests/slave_tests.cpp e1f5bfe074c357f46403887365b3f9ae554000b4 
> 
> Diff: https://reviews.apache.org/r/47608/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Abhishek Dasgupta]

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

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


Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, Benjamin 
Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

The master and agent code for routing endpoints are made
consistent. Now they both capture 'this' variable in the
lambda calls in routing function arguments. Continuations
are no more need to be static.


Diffs
-

  src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
  src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
  src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 

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


Testing
---

Unit tests.

On ubuntu 16.04:
sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check

Manual testing.

1. Ran master with:


   sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos


2. ACL File:


  {
"get_endpoints": [
  {
"principals": { "type": "NONE" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  } 


3. Ran slave with:


   sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
--acls=file:///home/abhishek/testAcl


4. Ran toy-framework with:


   sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
hello"


5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
error 403: Forbidden

6. Changed ACL to:


  {
"get_endpoints": [
  {
"principals": { "type": "ANY" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  }


7. Ran slave and framework again.

8. Output:



[{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
 Executor (Task: 699) (Command: sh -c 'echo 
hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...


Thanks,

Abhishek Dasgupta

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Abhishek Dasgupta]

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

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


Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, Benjamin 
Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.


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


Repository: mesos


Description (updated)
---

Removed inconsistancy from routing endpoints in agent code.


Diffs (updated)
-

  src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
  src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
  src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 

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


Testing
---

Unit tests.

On ubuntu 16.04:
sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check

Manual testing.

1. Ran master with:


   sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos


2. ACL File:


  {
"get_endpoints": [
  {
"principals": { "type": "NONE" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  } 


3. Ran slave with:


   sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
--acls=file:///home/abhishek/testAcl


4. Ran toy-framework with:


   sudo ./no-executor-framework --master=master@127.0.0.1:5050 --command="echo 
hello"


5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
error 403: Forbidden

6. Changed ACL to:


  {
"get_endpoints": [
  {
"principals": { "type": "ANY" },
"paths": { "values": ["/flags", "/monitor/statistics", "/containers"] }
  }
]
  }


7. Ran slave and framework again.

8. Output:



[{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
 Executor (Task: 699) (Command: sh -c 'echo 
hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...


Thanks,

Abhishek Dasgupta

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

[Michael Park]

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




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


`s/task_info=/task_info =/`



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


`s/VIEW_EXECUTORS/VIEW_EXECUTOR/`
`s/ExcecutorInfo/ExecutorInfo/`

Both instances of `a` should be `an`.



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


`s/informationis/information is/`



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


Shift this 2 spaces to the left


- Michael Park


On May 24, 2016, 9:44 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46613/
> ---
> 
> (Updated May 24, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> 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
> * VIEW_TASK
> * VIEW_EXECUTOR
> 
> 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)
> * ViewExecutors view_executors (giving access to Executors run 
> under a specific OS user)
> 
> 
> Diffs
> -
> 
>   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
> 
>

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

[Michael Park]

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




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


```c++
struct Object
{
```



include/mesos/authorizer/authorizer.hpp (lines 46 - 47)


We should keep `Task` and `TaskInfo` declared in the same order between 
this and https://reviews.apache.org/r/46613.



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


`virtual ~ObjectAllower() = default;`



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


`s/&  subject/& subject/`


- Michael Park


On May 25, 2016, 5:19 a.m., Joerg Schad wrote:
> 
> ---
> 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.
> 
> 
> Bugs: MESOS-5403
> https://issues.apache.org/jira/browse/MESOS-5403
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added ObjectAllower 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 47608: Add Labels from TaskInfo into TaskStatus message.

[Shuai Lin]

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



Sometimes mesos master would also send status update to the framework. For 
example, when a slave is lost, the master would send `TASK_LOST` to frameworks 
for all tasks running on that slave. 

https://github.com/apache/mesos/blob/0.28.1/src/master/master.cpp#L6194-L6229

Should we also add the labels to the stauts update message there?

- Shuai Lin


On May 20, 2016, 6:38 p.m., Srinivas Brahmaroutu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47608/
> ---
> 
> (Updated May 20, 2016, 6:38 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jojy Varghese.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> WIP: Add Labels from TaskInfo into TaskStatus message.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp ce0e7b1f1d17c3b82d835b0a6296ed7b1e9eeac1 
>   src/tests/hook_tests.cpp c6b4e8a50534e455cf44f427b09f74eef71177b2 
>   src/tests/slave_tests.cpp e1f5bfe074c357f46403887365b3f9ae554000b4 
> 
> Diff: https://reviews.apache.org/r/47608/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Srinivas Brahmaroutu
> 
>

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

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On May 24, 2016, 5:18 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47198/
> ---
> 
> (Updated May 24, 2016, 5:18 p.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
> -
> 
>   include/mesos/docker/v1.proto ff18f8cadfce3315467f194d50d3469fa839f686 
> 
> Diff: https://reviews.apache.org/r/47198/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 47528: Updated validation tests with creator principal.

[Bernd Mathiske]

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


Fix it, then Ship it!





src/tests/master_validation_tests.cpp (line 466)


More Hamming distance than just the "2" would make the difference between 
the principals even easier to spot.


- Bernd Mathiske


On May 23, 2016, 9:44 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47528/
> ---
> 
> (Updated May 23, 2016, 9:44 p.m.)
> 
> 
> Review request for mesos, Bernd Mathiske, Joerg Schad, and Neil Conway.
> 
> 
> Bugs: MESOS-5005
> https://issues.apache.org/jira/browse/MESOS-5005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master validation tests are updated to include the
> new `validate()` signature, and a new test is added,
> `CreateOperationValidationTest.NonMatchingPrincipal`,
> to ensure that a non-matching creator principal will
> be invalidated.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> ca4442aa1ef0087a7d058d1b3aa430a1dbc16960 
> 
> Diff: https://reviews.apache.org/r/47528/diff/
> 
> 
> Testing
> ---
> 
> `make check` was used to test on OSX at the end of this review chain.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Benjamin Bannier]

> On May 25, 2016, 11:45 a.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 379
> > 
> >
> > Why was this needed? IMHO the existing explicitness helps to document 
> > what state we drag into the lambdas and should be kept.
> > 
> > Here and below.
> 
> Abhishek Dasgupta wrote:
> Hmmm...in master code, there is [=] in some places. That's why 
> implemented same logic here...else I have to send  [this, requst, slaveFlags] 
> here. Maybe, we need to use specific captures in master also.

We do not seem to have a strong, explicit rule regarding capture list, but 
generally prefer being explicit. Let's just leave it here like it was.


- Benjamin


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


On May 25, 2016, 11:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> ---
> 
> (Updated May 25, 2016, 11:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Benjamin Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
> https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>sudo ./no-executor-framework --master=master@127.0.0.1:5050 
> --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
> 
> [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
>  Executor (Task: 699) (Command: sh -c 'echo 
> hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Jan Schlicht]

> On May 25, 2016, 11:45 a.m., Jan Schlicht wrote:
> > src/slave/http.cpp, line 651
> > 
> >
> > Remove this, same rationale as the comments in `Slave::Http::flags`.
> 
> Abhishek Dasgupta wrote:
> One thing I want to be sure here: "pid" is getting called multiple times 
> in subsequent "defers" in same function. Should not we better save it once 
> and use that in the rest of the calls in same functions? What do you say?

Buy you won't gain anything from using `pid` instead of `slave->self()`; the 
PID won't change during the lifetime of `slave`. Other "defer chains" also use 
`self()`, let's keep it consistent.


- Jan


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


On May 25, 2016, 11:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> ---
> 
> (Updated May 25, 2016, 11:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Benjamin Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
> https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>sudo ./no-executor-framework --master=master@127.0.0.1:5050 
> --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
> 
> [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
>  Executor (Task: 699) (Command: sh -c 'echo 
> hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Abhishek Dasgupta]

> On May 25, 2016, 9:45 a.m., Benjamin Bannier wrote:
> > src/slave/http.cpp, line 379
> > 
> >
> > Why was this needed? IMHO the existing explicitness helps to document 
> > what state we drag into the lambdas and should be kept.
> > 
> > Here and below.

Hmmm...in master code, there is [=] in some places. That's why implemented same 
logic here...else I have to send  [this, requst, slaveFlags] here. Maybe, we 
need to use specific captures in master also.


- Abhishek


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


On May 25, 2016, 9:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> ---
> 
> (Updated May 25, 2016, 9:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Benjamin Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
> https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>sudo ./no-executor-framework --master=master@127.0.0.1:5050 
> --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
> 
> [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
>  Executor (Task: 699) (Command: sh -c 'echo 
> hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47753: Enabled building libprocess without building 3rdparty first.

[Kapil Arya]

> On May 25, 2016, 5 a.m., Till Toenshoff wrote:
> > Seems you still need to fix configure.ac within libprocess. When running a 
> > `bootstrap` I am getting
> > ```
> > configure.ac:62: error: required file '3rdparty/Makefile.in' not found
> > configure.ac:64: error: required file '3rdparty/gmock_sources.cc.in' not 
> > found
> > ```

That is a somewhat non-trivial fix and so will be a separate patchset (post 
0.29.0) :-/.


- Kapil


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


On May 23, 2016, 10:49 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47753/
> ---
> 
> (Updated May 23, 2016, 10:49 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere and Till Toenshoff.
> 
> 
> Bugs: MESOS-5445
> https://issues.apache.org/jira/browse/MESOS-5445
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doing a `make` inside libprocess would now automatically build bundled
> dependencies.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am 3c3249fe9799ba919ac7bd13e2ddb07a306737f0 
> 
> Diff: https://reviews.apache.org/r/47753/diff/
> 
> 
> Testing
> ---
> 
> `make clean` in 3rdparty/ followed by `make` in libprocess succeeds with this 
> patch.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Abhishek Dasgupta]

> On May 25, 2016, 9:45 a.m., Jan Schlicht wrote:
> > src/slave/http.cpp, line 651
> > 
> >
> > Remove this, same rationale as the comments in `Slave::Http::flags`.

One thing I want to be sure here: "pid" is getting called multiple times in 
subsequent "defers" in same function. Should not we better save it once and use 
that in the rest of the calls in same functions? What do you say?


- Abhishek


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


On May 25, 2016, 9:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> ---
> 
> (Updated May 25, 2016, 9:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Benjamin Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
> https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>sudo ./no-executor-framework --master=master@127.0.0.1:5050 
> --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
> 
> [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
>  Executor (Task: 699) (Command: sh -c 'echo 
> hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 47609: Reverted adding libelf as a default dynamic library in libprocess.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On May 19, 2016, 6:08 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47609/
> ---
> 
> (Updated May 19, 2016, 6:08 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Till Toenshoff.
> 
> 
> Bugs: MESOS-5414
> https://issues.apache.org/jira/browse/MESOS-5414
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The libelf library is not a standard library installed on all Linux
> systems, and it is not a library required to build libprocess in
> general. It is only required for subsystems that happen to use the
>  header. As such, we should only require this
> dependency for those specific subsystems. If we ever decide to build
> something into libprocess that depends on libelf by default, we can
> revisit this issue then.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/configure.ac 2daded51122ab97d71ec57109b85c2473460eeda 
> 
> Diff: https://reviews.apache.org/r/47609/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 47610: Reverted adding libelf as a default dynamic library in stout.

[Till Toenshoff]

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


Ship it!




Ship It!

- Till Toenshoff


On May 20, 2016, 12:56 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47610/
> ---
> 
> (Updated May 20, 2016, 12:56 a.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-5414
> https://issues.apache.org/jira/browse/MESOS-5414
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The libelf library is not a standard library installed on all Linux
> systems, and it is not a library required to build stout in
> general. It is only required for subsystems that happen to use the
>  header. As such, we should only require this
> dependency for those specific subsystems. If we ever decide to build
> something into stout that depends on libelf by default, we can
> revisit this issue then.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/configure.ac 52b4a05e9b27518144e4382d107c9c7834a41015 
> 
> Diff: https://reviews.apache.org/r/47610/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 47822: Removed inconsistancy from routing endpoints in agent code.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47822]

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, 9:23 a.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47822/
> ---
> 
> (Updated May 25, 2016, 9:23 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Anand Mazumdar, 
> Benjamin Bannier, Greg Mann, Jan Schlicht, Till Toenshoff, and Vinod Kone.
> 
> 
> Bugs: MESOS-5293
> https://issues.apache.org/jira/browse/MESOS-5293
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The master and agent code for routing endpoints are made
> consistent. Now they both capture 'this' variable in the
> lambda calls in routing function arguments. Continuations
> are no more need to be static.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp dd1f509e9464af7d02db0182b0eebd63993dd029 
>   src/slave/slave.hpp 0de6a570e8b4699771048295ec3fcedf84593495 
>   src/slave/slave.cpp 2941cf1b6ea1e4deabfcbbe3f4897c06a28531a5 
> 
> Diff: https://reviews.apache.org/r/47822/diff/
> 
> 
> Testing
> ---
> 
> Unit tests.
> 
> On ubuntu 16.04:
> sudo GTEST_FILTER="*SlaveEndpointTest*.*" make -j2 check
> 
> Manual testing.
> 
> 1. Ran master with:
> 
> 
>sudo  ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> 
> 2. ACL File:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "NONE" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   } 
> 
> 
> 3. Ran slave with:
> 
> 
>sudo ./bin/mesos-slave.sh --master=127.0.0.1:5050 --ip=0.0.0.0 
> --acls=file:///home/abhishek/testAcl
> 
> 
> 4. Ran toy-framework with:
> 
> 
>sudo ./no-executor-framework --master=master@127.0.0.1:5050 
> --command="echo hello"
> 
> 
> 5. Output while hitting "http://127.0.0.1:5051/slave(1)/containers" - HTTP 
> error 403: Forbidden
> 
> 6. Changed ACL to:
> 
> 
>   {
> "get_endpoints": [
>   {
> "principals": { "type": "ANY" },
> "paths": { "values": ["/flags", "/monitor/statistics", "/containers"] 
> }
>   }
> ]
>   }
> 
> 
> 7. Ran slave and framework again.
> 
> 8. Output:
> 
> 
> 
> [{"container_id":"2059c6a5-d168-4db7-912d-1c70b61fdb5d","executor_id":"699","executor_name":"Command
>  Executor (Task: 699) (Command: sh -c 'echo 
> hello')","framework_id":"48e2fe51-afba-44c5-958e-0ad3cee6454b-","source":"699","statistics":{"cpus_limit":0.2,"cpus_system_time_secs":0.01,"cpus_user_time_secs":0.04,"mem_limit_bytes":67108864,"mem_rss_bytes":88477696,"timestamp":1464168040.50791},"status":{}},{"container_id":"a0be0340-ca6b-4fc4-9862-4af143a7961e",...
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>