Re: Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Cody Maloney]

> On May 10, 2016, 5:27 a.m., Cody Maloney wrote:
> >

I believe this covers the use cases we need. Be great to have a demo PR of it 
in action for:  https://github.com/dcos/dcos to validate that it does.


- Cody


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


On May 9, 2016, 5:10 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47123/
> ---
> 
> (Updated May 9, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Till Toenshoff.
> 
> 
> Bugs: MESOS-5173
> https://issues.apache.org/jira/browse/MESOS-5173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the operator to use separate manifest JSON files for each
> module.  Previously, one had to merge all module manifest files into a
> single JSON file before passing on to the master/agent.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
>   src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
>   src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
>   src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
>   src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
>   src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
>   src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
>   src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
>   src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 
> 
> Diff: https://reviews.apache.org/r/47123/diff/
> 
> 
> Testing
> ---
> 
> Manual testing with:
> 1. --module_dir
> 2. --modules
> 3. --module_dir in conjunction with --modules
> 
> The first two succeeded while the third failed as expected.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Cody Maloney]

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




src/module/manager.cpp (line 357)


Should do the early exit:
`if (moduleManifests.isNone()) { return Nothing(); }`

That way the rest doesn't need to be indented.



src/module/manager.cpp (line 358)


Should document the sort order / load order based on filenames inside the 
doc changes to accompany this (I don't see it anywhere currently)



src/module/manager.cpp (line 377)


Is there a semantic difference between loading modules one at a time vs. 
just passing them all at once to loadManifest?

I'm worried that the behavior here could differ slightly from the --modules 
flag which calls loadManifest with all the modules at once.


- Cody Maloney


On May 9, 2016, 5:10 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47123/
> ---
> 
> (Updated May 9, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Till Toenshoff.
> 
> 
> Bugs: MESOS-5173
> https://issues.apache.org/jira/browse/MESOS-5173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the operator to use separate manifest JSON files for each
> module.  Previously, one had to merge all module manifest files into a
> single JSON file before passing on to the master/agent.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
>   src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
>   src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
>   src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
>   src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
>   src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
>   src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
>   src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
>   src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 
> 
> Diff: https://reviews.apache.org/r/47123/diff/
> 
> 
> Testing
> ---
> 
> Manual testing with:
> 1. --module_dir
> 2. --modules
> 3. --module_dir in conjunction with --modules
> 
> The first two succeeded while the third failed as expected.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 36440: Enabled docker volume support for DockerContainerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45377, 36440]

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

- Mesos ReviewBot


On May 10, 2016, 3:50 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36440/
> ---
> 
> (Updated May 10, 2016, 3:50 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5341
> https://issues.apache.org/jira/browse/MESOS-5341
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled docker volume support for DockerContainerizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
>   include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
>   src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 
> 
> Diff: https://reviews.apache.org/r/36440/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 1) Start convoy
> 2) Create a volume named as c1
> root@kvm-002605:~# convoy list
> {
> "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad": {
> "UUID": "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Name": "c1",
> "Driver": "devicemapper",
> "MountPoint": "",
> "CreatedTime": "Mon May 09 09:38:52 +0800 2016",
> "DriverInfo": {
> "DevID": "1",
> "Device": 
> "/dev/mapper/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Driver": "devicemapper",
> "MountPoint": "",
> "Size": "107374182400"
> },
> "Snapshots": {}
> }
> }
> 3) Update  mesos-execte to set up volume
> index 4711e80..6c712a2 100644
> --- a/src/cli/execute.cpp
> +++ b/src/cli/execute.cpp
> @@ -72,6 +72,8 @@ using mesos::v1::TaskID;
>  using mesos::v1::TaskInfo;
>  using mesos::v1::TaskState;
>  using mesos::v1::TaskStatus;
> +using mesos::v1::Volume;
> +using mesos::v1::Parameters;
>  
>  using mesos::v1::scheduler::Call;
>  using mesos::v1::scheduler::Event;
> @@ -581,6 +583,18 @@ private:
>  
>containerInfo.set_type(ContainerInfo::DOCKER);
>containerInfo.mutable_docker()->set_image(dockerImage.get());
> +  //containerInfo.mutable_docker()->set_volume_driver("convoy");
> +
> +  Volume* volume1 = containerInfo.add_volumes();
> +  //volume1->set_host_path("c1");
> +  volume1->set_container_path("/tmp");
> +  volume1->mutable_source()->set_type(Volume::Source::DOCKER_VOLUME);
> +  volume1->mutable_source()->mutable_docker_volume()->set_driver(
> +  "convoy");
> +  volume1->mutable_source()->mutable_docker_volume()->set_name(
> +  "c1");
> +  volume1->set_mode(Volume::RW);
> +  cout << "Add Voume 1" << endl;
>  
>if (networks.isSome() && !networks->empty()) {
>  vector tokens = strings::tokenize(networks.get(), ",");
> 4) Start master
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master
> 5) Start agent
> GLOG_v=1 ./bin/mesos-slave.sh --master=10.0.0.65:5050 
> --isolation=filesystem/linux,docker/runtime --containerizers=docker 
> --work_dir=/tmp/mesos --image_providers=docker
> 5) Start mesos-execute
> src/mesos-execute --master=10.0.0.65:5050 --name=test --command="sleep 1" 
> --docker_image=ubuntu:14.04 --containerizer=docker
> I0509 13:14:56.118808  8320 scheduler.cpp:177] Version: 0.29.0
> I0509 13:14:56.120827  8346 scheduler.cpp:479] New master detected at 
> master@10.0.0.65:5050
> Subscribed with ID 'acff7546-e8bf-4fad-a651-307c73297db0-'
> Add Voume 1
> Submitted task 'test' to agent '5454a953-83d0-42a4-b7fe-0825a6667f8c-S0'
> Received status update TASK_RUNNING for task 'test'
>   source: SOURCE_EXECUTOR
> Received status update TASK_LOST for task 'test'
> 
> 6) Inspect container
> docker inspect 
> ...
> {
> "Name": "c1",
> "Source": 
> "/var/lib/convoy/devicemapper/mounts/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Destination": "/tmp",
> "Driver": "convoy",
> "Mode": "rw",
> "RW": true,
> "Propagation": "rprivate"
> }
> ...
> 7) Check sandbox executer log
> I0509 13:14:56.313102  8361 logging.cpp:195] Logging to STDERR
> I0509 13:14:56.315385  8361 process.cpp:999] libprocess is initialized on 
> 10.0.0.65:52853 with 16 worker threads
> I0509 13:14:56.316789  8361 exec.cpp:150] Version: 0.29.0
> I0509 13:14:56.321717  8387 exec.cpp:200] Executor started at: 
> executor(1)@10.0.0.65:52853 with pid 8361
> I0509 

Review Request 47154: Establish TCP connection after backing off.

[David Robinson]

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

Review request for mesos, Ben Mahler, Ian Downes, and Cong Wang.


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


Repository: mesos


Description
---

Establish TCP connection after backing off.


Diffs
-

  src/slave/slave.hpp b72438033708de473046d321c493d9fbcd7a9b43 
  src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 

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


Testing
---

make check

Also started a master and agent and captured a tcpdump to verify the connection 
is only established _after_ the registration backoff.


Thanks,

David Robinson

Re: Review Request 46866: Enabled authorization of libprocess HTTP endpoints (libprocess).

[Greg Mann]

> On May 9, 2016, 10:28 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/include/process/http.hpp, line 85
> > 
> >
> > Is there a design doc or an overall explanation of how authorization 
> > will be performed in libprocess?
> > 
> > I find it cool if we provide an API which can be used in Mesos, similar 
> > to how authentication works, also to keep them consistent.
> > 
> > Of the top of my head, how about changing `route` to use a set of 
> > flags, similar on how streams work in the stdlib so we can define if you 
> > want authentication + authorization, etc.

No, there isn't a design doc, though perhaps there should be. I think we're in 
agreement that we should provide a consistent interface for authentication and 
authorization in libprocess. I was thinking about this after our discussion 
this morning: the key difference between the existing HTTP authentication 
implementation and the authorization implementation proposed here is the way in 
which endpoints are mapped to handlers. For AuthN, both endpoints and 
authenticators get mapped into a space of realms, and thus endpoints are 
implicitly mapped to authenticators. In the current proposal, authorization 
callbacks get directly mapped to endpoints via the full endpoint path.

One question is: will most HTTP endpoints require unique authorization logic? 
If so, then perhaps it's only cumbersome to have an intermediate mapping onto 
something like a "realm". Or perhaps there's another good reason for this 
mapping that I'm not thinking of?

A difficulty in adding a new `route` function to handle these cases is that the 
authorization logic lives in Mesos, but the endpoint is `route`d for the first 
time in libprocess. In order to accomplish this, we would need to allow one 
process to override the `route` call of another process. This is possible, but 
seems less than ideal to me.


- Greg


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


On May 10, 2016, 3:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46866/
> ---
> 
> (Updated May 10, 2016, 3:23 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-5286
> https://issues.apache.org/jira/browse/MESOS-5286
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables libprocess to store and execute
> authorization callbacks provided by a client
> application.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 
>   3rdparty/libprocess/src/process.cpp 
> dcaa64633d1eea330649c563635642928164d73c 
> 
> Diff: https://reviews.apache.org/r/46866/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 36440: Enabled docker volume support for DockerContainerizer.

[Guangya Liu]

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

(Updated 五月 10, 2016, 3:50 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Enabled docker volume support for DockerContainerizer.


Diffs (updated)
-

  include/mesos/mesos.proto 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
  src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 

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


Testing
---

make
make check

1) Start convoy
2) Create a volume named as c1
root@kvm-002605:~# convoy list
{
"f3459d44-bbed-4e01-a7f6-6c6aa4b70cad": {
"UUID": "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Name": "c1",
"Driver": "devicemapper",
"MountPoint": "",
"CreatedTime": "Mon May 09 09:38:52 +0800 2016",
"DriverInfo": {
"DevID": "1",
"Device": 
"/dev/mapper/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Driver": "devicemapper",
"MountPoint": "",
"Size": "107374182400"
},
"Snapshots": {}
}
}
3) Update  mesos-execte to set up volume
index 4711e80..6c712a2 100644
--- a/src/cli/execute.cpp
+++ b/src/cli/execute.cpp
@@ -72,6 +72,8 @@ using mesos::v1::TaskID;
 using mesos::v1::TaskInfo;
 using mesos::v1::TaskState;
 using mesos::v1::TaskStatus;
+using mesos::v1::Volume;
+using mesos::v1::Parameters;
 
 using mesos::v1::scheduler::Call;
 using mesos::v1::scheduler::Event;
@@ -581,6 +583,18 @@ private:
 
   containerInfo.set_type(ContainerInfo::DOCKER);
   containerInfo.mutable_docker()->set_image(dockerImage.get());
+  //containerInfo.mutable_docker()->set_volume_driver("convoy");
+
+  Volume* volume1 = containerInfo.add_volumes();
+  //volume1->set_host_path("c1");
+  volume1->set_container_path("/tmp");
+  volume1->mutable_source()->set_type(Volume::Source::DOCKER_VOLUME);
+  volume1->mutable_source()->mutable_docker_volume()->set_driver(
+  "convoy");
+  volume1->mutable_source()->mutable_docker_volume()->set_name(
+  "c1");
+  volume1->set_mode(Volume::RW);
+  cout << "Add Voume 1" << endl;
 
   if (networks.isSome() && !networks->empty()) {
 vector tokens = strings::tokenize(networks.get(), ",");
4) Start master
./bin/mesos-master.sh --work_dir=/tmp/mesos/master
5) Start agent
GLOG_v=1 ./bin/mesos-slave.sh --master=10.0.0.65:5050 
--isolation=filesystem/linux,docker/runtime --containerizers=docker 
--work_dir=/tmp/mesos --image_providers=docker
5) Start mesos-execute
src/mesos-execute --master=10.0.0.65:5050 --name=test --command="sleep 1" 
--docker_image=ubuntu:14.04 --containerizer=docker
I0509 13:14:56.118808  8320 scheduler.cpp:177] Version: 0.29.0
I0509 13:14:56.120827  8346 scheduler.cpp:479] New master detected at 
master@10.0.0.65:5050
Subscribed with ID 'acff7546-e8bf-4fad-a651-307c73297db0-'
Add Voume 1
Submitted task 'test' to agent '5454a953-83d0-42a4-b7fe-0825a6667f8c-S0'
Received status update TASK_RUNNING for task 'test'
  source: SOURCE_EXECUTOR
Received status update TASK_LOST for task 'test'

6) Inspect container
docker inspect 
...
{
"Name": "c1",
"Source": 
"/var/lib/convoy/devicemapper/mounts/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Destination": "/tmp",
"Driver": "convoy",
"Mode": "rw",
"RW": true,
"Propagation": "rprivate"
}
...
7) Check sandbox executer log
I0509 13:14:56.313102  8361 logging.cpp:195] Logging to STDERR
I0509 13:14:56.315385  8361 process.cpp:999] libprocess is initialized on 
10.0.0.65:52853 with 16 worker threads
I0509 13:14:56.316789  8361 exec.cpp:150] Version: 0.29.0
I0509 13:14:56.321717  8387 exec.cpp:200] Executor started at: 
executor(1)@10.0.0.65:52853 with pid 8361
I0509 13:14:56.325158  8388 exec.cpp:225] Executor registered on agent 
5454a953-83d0-42a4-b7fe-0825a6667f8c-S0
I0509 13:14:56.326740  8388 exec.cpp:237] Executor::registered took 325332ns
I0509 13:14:56.327030  8388 exec.cpp:312] Executor asked to run task 'test'
I0509 13:14:56.327163  8388 exec.cpp:321] Executor::launchTask took 98803ns
I0509 13:14:56.327354  8386 docker.cpp:546] Volume config 'c1:/tmp' driver 
'convoy' for container 
'mesos-5454a953-83d0-42a4-b7fe-0825a6667f8c-S0.d2fb7a57-d364-41b8-a671-30b95c5cebb4'
I0509 13:14:56.327437  8386 docker.cpp:707] Running docker -H 
unix:///var/run/docker.sock run --cpu-shares 1024 --memory 134217728 -e 
MESOS_SANDBOX=/mnt/mesos/sandbox -e 

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 五月 10, 2016, 3:41 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 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
76d0fa183c9099584da6a4ee1e5d474b871310c3 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 46866: Enabled authorization of libprocess HTTP endpoints (libprocess).

[Greg Mann]

> On May 9, 2016, 10:28 a.m., Alexander Rojas wrote:
> > 3rdparty/libprocess/src/process.cpp, line 3289
> > 
> >
> > Why no setting this variable close to were it will be used. I always 
> > liked the concept of data locality, so someone doesn't have to scroll up to 
> > see where a variable was set.

This needs to stay within the enclosing scope, but I moved it to the bottom of 
the `if` block, closer to `principal`'s first usage.


- Greg


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


On May 10, 2016, 3:23 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46866/
> ---
> 
> (Updated May 10, 2016, 3:23 a.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-5286
> https://issues.apache.org/jira/browse/MESOS-5286
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables libprocess to store and execute
> authorization callbacks provided by a client
> application.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 
>   3rdparty/libprocess/src/process.cpp 
> dcaa64633d1eea330649c563635642928164d73c 
> 
> Diff: https://reviews.apache.org/r/46866/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46866: Enabled authorization of libprocess HTTP endpoints (libprocess).

[Greg Mann]

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

(Updated May 10, 2016, 3:23 a.m.)


Review request for mesos, Alexander Rojas and Kapil Arya.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

This patch enables libprocess to store and execute
authorization callbacks provided by a client
application.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 
  3rdparty/libprocess/src/process.cpp dcaa64633d1eea330649c563635642928164d73c 

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


Testing
---

`make check` on OSX.


Thanks,

Greg Mann

Re: Review Request 36440: Enabled docker volume support for DockerContainerizer.

[Guangya Liu]

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

(Updated May 10, 2016, 3:03 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Enabled docker volume support for DockerContainerizer.


Diffs (updated)
-

  include/mesos/mesos.proto 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
  src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 

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


Testing
---

make
make check

1) Start convoy
2) Create a volume named as c1
root@kvm-002605:~# convoy list
{
"f3459d44-bbed-4e01-a7f6-6c6aa4b70cad": {
"UUID": "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Name": "c1",
"Driver": "devicemapper",
"MountPoint": "",
"CreatedTime": "Mon May 09 09:38:52 +0800 2016",
"DriverInfo": {
"DevID": "1",
"Device": 
"/dev/mapper/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Driver": "devicemapper",
"MountPoint": "",
"Size": "107374182400"
},
"Snapshots": {}
}
}
3) Update  mesos-execte to set up volume
index 4711e80..6c712a2 100644
--- a/src/cli/execute.cpp
+++ b/src/cli/execute.cpp
@@ -72,6 +72,8 @@ using mesos::v1::TaskID;
 using mesos::v1::TaskInfo;
 using mesos::v1::TaskState;
 using mesos::v1::TaskStatus;
+using mesos::v1::Volume;
+using mesos::v1::Parameters;
 
 using mesos::v1::scheduler::Call;
 using mesos::v1::scheduler::Event;
@@ -581,6 +583,18 @@ private:
 
   containerInfo.set_type(ContainerInfo::DOCKER);
   containerInfo.mutable_docker()->set_image(dockerImage.get());
+  //containerInfo.mutable_docker()->set_volume_driver("convoy");
+
+  Volume* volume1 = containerInfo.add_volumes();
+  //volume1->set_host_path("c1");
+  volume1->set_container_path("/tmp");
+  volume1->mutable_source()->set_type(Volume::Source::DOCKER_VOLUME);
+  volume1->mutable_source()->mutable_docker_volume()->set_driver(
+  "convoy");
+  volume1->mutable_source()->mutable_docker_volume()->set_name(
+  "c1");
+  volume1->set_mode(Volume::RW);
+  cout << "Add Voume 1" << endl;
 
   if (networks.isSome() && !networks->empty()) {
 vector tokens = strings::tokenize(networks.get(), ",");
4) Start master
./bin/mesos-master.sh --work_dir=/tmp/mesos/master
5) Start agent
GLOG_v=1 ./bin/mesos-slave.sh --master=10.0.0.65:5050 
--isolation=filesystem/linux,docker/runtime --containerizers=docker 
--work_dir=/tmp/mesos --image_providers=docker
5) Start mesos-execute
src/mesos-execute --master=10.0.0.65:5050 --name=test --command="sleep 1" 
--docker_image=ubuntu:14.04 --containerizer=docker
I0509 13:14:56.118808  8320 scheduler.cpp:177] Version: 0.29.0
I0509 13:14:56.120827  8346 scheduler.cpp:479] New master detected at 
master@10.0.0.65:5050
Subscribed with ID 'acff7546-e8bf-4fad-a651-307c73297db0-'
Add Voume 1
Submitted task 'test' to agent '5454a953-83d0-42a4-b7fe-0825a6667f8c-S0'
Received status update TASK_RUNNING for task 'test'
  source: SOURCE_EXECUTOR
Received status update TASK_LOST for task 'test'

6) Inspect container
docker inspect 
...
{
"Name": "c1",
"Source": 
"/var/lib/convoy/devicemapper/mounts/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
"Destination": "/tmp",
"Driver": "convoy",
"Mode": "rw",
"RW": true,
"Propagation": "rprivate"
}
...
7) Check sandbox executer log
I0509 13:14:56.313102  8361 logging.cpp:195] Logging to STDERR
I0509 13:14:56.315385  8361 process.cpp:999] libprocess is initialized on 
10.0.0.65:52853 with 16 worker threads
I0509 13:14:56.316789  8361 exec.cpp:150] Version: 0.29.0
I0509 13:14:56.321717  8387 exec.cpp:200] Executor started at: 
executor(1)@10.0.0.65:52853 with pid 8361
I0509 13:14:56.325158  8388 exec.cpp:225] Executor registered on agent 
5454a953-83d0-42a4-b7fe-0825a6667f8c-S0
I0509 13:14:56.326740  8388 exec.cpp:237] Executor::registered took 325332ns
I0509 13:14:56.327030  8388 exec.cpp:312] Executor asked to run task 'test'
I0509 13:14:56.327163  8388 exec.cpp:321] Executor::launchTask took 98803ns
I0509 13:14:56.327354  8386 docker.cpp:546] Volume config 'c1:/tmp' driver 
'convoy' for container 
'mesos-5454a953-83d0-42a4-b7fe-0825a6667f8c-S0.d2fb7a57-d364-41b8-a671-30b95c5cebb4'
I0509 13:14:56.327437  8386 docker.cpp:707] Running docker -H 
unix:///var/run/docker.sock run --cpu-shares 1024 --memory 134217728 -e 
MESOS_SANDBOX=/mnt/mesos/sandbox -e 

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 May 10, 2016, 2:43 a.m.)


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


Summary (updated)
-

Made "driver" as optional for DockerVolume.


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


Repository: mesos


Description (updated)
---

Made "driver" as optional for DockerVolume.


Diffs (updated)
-

  include/mesos/mesos.proto 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
  include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
  src/slave/containerizer/mesos/isolators/docker/volume/isolator.cpp 
76d0fa183c9099584da6a4ee1e5d474b871310c3 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 46874: Enhanced log message when launch mesos-containerizer.

[Guangya Liu]

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

(Updated 五月 9, 2016, 10:39 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

The mesos-containerizer will help prepare the rootfs, mount and
network conf for a container when launch with some flags, it
would be helpful to print out all of the mesos-containerizer launch
flags for trouble shooting.


Diffs
-

  src/slave/containerizer/mesos/containerizer.cpp 
75e5a32a3e70ec60a6800e21a621673184ea0956 

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


Testing
---

make
make check

Enhanced log message:
I0501 14:12:17.910006  5991 containerizer.cpp:1175] Launching 
'mesos-containerizer' with flags 
'--command="{"shell":true,"value":"\/root\/src\/mesos\/m1\/mesos\/build\/src\/mesos-executor"}"
 --commands="{"commands":[{"shell":true,"value":"#!\/bin\/sh\nset -x 
-e\n\/root\/src\/mesos\/m1\/mesos\/build\/src\/mesos-containerizer mount 
--help=\"false\" --operation=\"make-rslave\" --path=\"\/\"\ngrep -E 
'\/tmp\/mesos\/.+' \/proc\/self\/mountinfo | grep -v 
'a2cc264f-72eb-416c-b876-e4a98484a0ab' | cut -d' ' -f5 | xargs 
--no-run-if-empty umount -l || true \n"}]}" --help="false" --pipe_read="8" 
--pipe_write="9" 
--sandbox="/tmp/mesos/slaves/5763efb9-8e12-4bfc-b06b-2a324881608d-S0/frameworks/425afd24-796d-42dd-98c0-593f1d71e246-/executors/test/runs/a2cc264f-72eb-416c-b876-e4a98484a0ab"
 --user="root"'


Thanks,

Guangya Liu

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

[Anand Mazumdar]

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


Ship it!




Ship It!

- Anand Mazumdar


On May 7, 2016, 11:40 p.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47092/
> ---
> 
> (Updated May 7, 2016, 11:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Some of the methods in the process::network namespace were returning
> the 0 or -1 return code of the underlying socket system call. These
> functions will always return 0 when the Try is Some.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/network.hpp 
> 9cc4acd11dba561f40c33bc9dabb35a452a80e62 
>   3rdparty/libprocess/src/poll_socket.cpp 
> e68c7836b6a79253fa646c1919d0f331f21ec131 
>   3rdparty/libprocess/src/socket.cpp ec0e913aca30f92d4a0543ad1e685b897617bca3 
> 
> Diff: https://reviews.apache.org/r/47092/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

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

[Anand Mazumdar]

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


Fix it, then Ship it!




LGTM. Just a couple of micro nit-picks.


3rdparty/libprocess/src/http.cpp (line 1160)


Nit: Backticks around `ConnectionProcess`.



3rdparty/libprocess/src/http.cpp (line 1161)


s/ConnectionProcess/process



3rdparty/libprocess/src/http.cpp (lines 1164 - 1167)


hmm.. thinking about it more, we should follow this pattern everywhere. 
More often then not this is what our pimpl pattern looks like:

```cpp
process::terminate(...);
process::wait(..);
delete process; // even better have an `Owned` to clean up the pointer and 
eliminate this line.
```

The last 2 lines are redundant and we can easily get away with libprocess 
doing the GC. This modifies the code to be just a one liner with the added 
benefit of having a `PID` member variable for `dispatch` instead of a pointer:

```cpp
process::terminate(...);
```


- Anand Mazumdar


On May 8, 2016, 12:45 a.m., Ben Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47091/
> ---
> 
> (Updated May 8, 2016, 12:45 a.m.)
> 
> 
> Review request for mesos and Anand Mazumdar.
> 
> 
> Bugs: MESOS-4658
> https://issues.apache.org/jira/browse/MESOS-4658
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per the details in MESOS-4658, the `http::Connection` abstraction is
> prone to the deadlock of `process::wait`ing on itself. This occurs
> when the `http::internal::ConnectionProcess` exposes a Future on
> which the caller binds a copy of `http::Connection`. When completing
> the Future, the Future clears its callbacks within the execution
> context of the `http::internal::ConnectionProcess`. If the last copy
> of the `http::Connection` was present within one of the callbacks,
> the `http::internal::ConnectionProcess` will wait on itself, leading
> to a deadlock.
> 
> This surfaces a general pattern of having libprocess manage a
> Process when it cannot be guaranteed that the Process will be
> waited on by another execution context.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 48f91d01555e760b1c4fd2cde684168d65f75e57 
> 
> Diff: https://reviews.apache.org/r/47091/diff/
> 
> 
> Testing
> ---
> 
> Removed the hack put in place within the http::get path to validate the fix.
> 
> 
> Thanks,
> 
> Ben Mahler
> 
>

Re: Review Request 46504: Constructed error string in MethodNotAllowed.

[Jacob Janco]

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

(Updated May 9, 2016, 10:21 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Removed constructor delegation and moved body construction to a static method. 
Also changed log line to reflect more readble response body generation.


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


Repository: mesos


Description (updated)
---

Constructed error string in MethodNotAllowed.


Diffs (updated)
-

  3rdparty/libprocess/include/process/http.hpp 
8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 

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


Testing
---

make check


Thanks,

Jacob Janco

Re: Review Request 47127: Forgot the algorithm is maximizing when it needs to minimize.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On May 9, 2016, 5:20 p.m., Chris wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47127/
> ---
> 
> (Updated May 9, 2016, 5:20 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The algorithm is maximizing when it needs to minimize. Simple fix after some 
> testing in python.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47127/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chris
> 
>

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

[Ben Mahler]

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


Ship it!




Modulo comments.

- Ben Mahler


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

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

[Ben Mahler]

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


Ship it!




Modulo comments.

- Ben Mahler


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

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

[Alexander Rukletsov]

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




src/launcher/http_command_executor.cpp (line 620)


Remove "for some reason"



src/launcher/http_command_executor.cpp (lines 623 - 627)


The escalation grace period can be only decreased. We intentionally do not 
support increasing the total grace period for the terminating task, because we 
do not want users to "slow down" a kill that is in progress. Also note that 
docker does not support this currently.


- Alexander Rukletsov


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

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

[Alexander Rukletsov]

> On May 5, 2016, 8:25 p.m., Ben Mahler wrote:
> > src/launcher/http_command_executor.cpp, lines 644-645
> > 
> >
> > How about:
> > 
> > ```
> > cout << "Received a new kill policy grace period of << gracePeriod << 
> > "; shortening remaining grace period time to " << remaining;
> > ```
> 
> Alexander Rukletsov wrote:
> This is:
>   * Not symmetrical to the log message when we issue a kill for the first 
> time;
>   * Not correct, because the total grace period is `remaining + elapsed`.
>   
> Why do you think this level of detail is important in the log? We've 
> already have a log entry per each kill.

As an altertnative, let's print it when `kill()` is called.


> On May 5, 2016, 8:25 p.m., Ben Mahler wrote:
> > src/launcher/http_command_executor.cpp, lines 569-575
> > 
> >
> > It looks like we have to re-assign kill_policy! Otherwise, if kill is 
> > called internally (e.g. health check failure) after a Kill message with a 
> > kill_policy arrives, we may accidentally change the grace period!
> > 
> > Something like:
> > 
> > ```
> > if (override.isSome()) {
> >   killPolicy = override.get();
> > }
> > ```
> > 
> > If we don't re-assign we will take the old one from TaskInfo when kill 
> > is called by the executor itself.
> 
> Alexander Rukletsov wrote:
> This was intentional. If the health check fails during the grace 
> shutdown, but the task has not been reaped yet, doesn't it makes sense to 
> kill it faster?

Add a TODO in health checks about kill policy.


- Alexander


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


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

Re: Review Request 46928: Added safety fixes to and tests `os::close`.

[Alex Clemmer]

> On May 8, 2016, 9:06 p.m., Joris Van Remoortere wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp, 
> > lines 38-47
> > 
> >
> > https://msdn.microsoft.com/en-us/library/s58ftw19.aspx
> > suggests you use C++ try / catch.
> > Why did you choose the use `__try` & `__except`?

A couple reasons.

(1) No specific advantage to using them. Using C++ exceptions is more flexible, 
but Mesos is largely not supposed to throw exceptions, so we don't need the 
extra flexibility. The guidance here (as I understand it) is meant to encourage 
people to turn on the flag that maps SEH to C++ exceptions so they don't have 
crazy code that awkwardly does both. We don't really have this problem.
(2) But, more to the point, I don't want to add the compiler flag that enables 
them if I can avoid it, because we're not supposed to be using exceptions at 
all. I'd rather be extremely clear at every call site that we expect only to 
catch structured exceptions.


- Alex


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


On May 3, 2016, 5:41 a.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46928/
> ---
> 
> (Updated May 3, 2016, 5:41 a.m.)
> 
> 
> Review request for mesos, Daniel Pravat, Artem Harutyunyan, Joris Van 
> Remoortere, and Michael Park.
> 
> 
> Bugs: MESOS-5318
> https://issues.apache.org/jira/browse/MESOS-5318
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit has three goals:
> 
>   (1) Expand the `os::close` to support closing Windows `HANDLE`
>   objects. This will simplify code in many places, but most notably
>   (and immediately) subprocess, where sharing code between POSIX and
>   Windows implementations is a major priority.
>   (2) Make `os::close` safe on Windows. Unlike its POSIX cousins, the
>   Windows implementation of `::close` is very picky about which file
>   descriptors it will close. If you pass it a `SOCKET` by mistake,
>   for example, the C runtime will throw a structured exception.
>   Since this could potentially halt the process, and since our core
>   socket abstractions (socket.hpp, for example) use `int` to
>   represent sockets, it is worth investing in safety here.
>   (3) Add `os::close` unit tests to verify the safety requirements.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/os/windows/close.hpp 
> 8f7318aecff261b803991f7aa24ad869de8c1162 
>   3rdparty/libprocess/3rdparty/stout/tests/main.cpp 
> 3f3f62769abdbe44b3e37e0ea9e5f59484b52db6 
>   3rdparty/libprocess/3rdparty/stout/tests/os/filesystem_tests.cpp 
> 269079f316b51e19990d3058c1b9a34060e3559b 
> 
> Diff: https://reviews.apache.org/r/46928/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 47080: Agent should backoff when establishing a socket.

[David Robinson]

> On May 9, 2016, 5:20 a.m., Cong Wang wrote:
> > src/slave/slave.cpp, line 954
> > 
> >
> > One thing to note is the authentication could need a connect with 
> > master, so this could impact it. But essentially we need a backoff for 
> > authentication too.

Yeah, perhaps the call to authenticate() should be moved within 
doReliableRegistration() too.


- David


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


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

Re: Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47123]

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

- Mesos ReviewBot


On May 9, 2016, 5:10 p.m., Kapil Arya wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47123/
> ---
> 
> (Updated May 9, 2016, 5:10 p.m.)
> 
> 
> Review request for mesos, Cody Maloney and Till Toenshoff.
> 
> 
> Bugs: MESOS-5173
> https://issues.apache.org/jira/browse/MESOS-5173
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This allows the operator to use separate manifest JSON files for each
> module.  Previously, one had to merge all module manifest files into a
> single JSON file before passing on to the master/agent.
> 
> 
> Diffs
> -
> 
>   src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
>   src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
>   src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
>   src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
>   src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
>   src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
>   src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
>   src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
>   src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
>   src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 
> 
> Diff: https://reviews.apache.org/r/47123/diff/
> 
> 
> Testing
> ---
> 
> Manual testing with:
> 1. --module_dir
> 2. --modules
> 3. --module_dir in conjunction with --modules
> 
> The first two succeeded while the third failed as expected.
> 
> 
> Thanks,
> 
> Kapil Arya
> 
>

Re: Review Request 46920: Added validation hook inside Slave::runTask.

[Joseph Wu]

> On May 6, 2016, 12:37 a.m., Adam B wrote:
> > Please create a JIRA with motivation for this hook. I don't want a new hook 
> > to slip into a release without a JIRA in the changelog to document it.
> 
> Kapil Arya wrote:
> Good point. I meant to add it to the RR, but it slipped :-).

I should have added a "`[WIP]`" to the summary :)


> On May 6, 2016, 12:37 a.m., Adam B wrote:
> > include/mesos/hook.hpp, lines 51-55
> > 
> >
> > Why wouldn't you do this validation at the master? What would a slave 
> > know about validation that the master wouldn't?
> > Would different slaves validate differently? If so, why isn't that 
> > attribute surfaced in the offer rather than failing on task launch?
> > Fail early.

Discussed this with Jie offline, and we determined that Isolators are the 
correct way to validate things with the MesosContainerizer.  And because of 
that, this hook should be DockerContainerizer-only.

To answer your questions:

* Presumably, each hook/isolator would be adding functionality as well as 
validating.  Having the agent do validation keeps the logic in one place.
* Agents will always know about the ExecutorInfo, which the master does not 
construct for command tasks.
* A hetergeneous cluster might have agents with different hardware.  i.e. A 
node with no GPUs probably doesn't need GPU-validation logic.
* Presumably, it's up to the operator to distinguish the node via resources, 
attributes, roles, etc.


- Joseph


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


On May 5, 2016, 9:01 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46920/
> ---
> 
> (Updated May 5, 2016, 9:01 a.m.)
> 
> 
> Review request for mesos, Adam B, Artem Harutyunyan, Jie Yu, and Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a new hook `slaveRunTaskValidatorHook`, which allows a module to 
> inspect the contents of a task and potentially reject the task with a 
> `TASK_ERROR`.
> 
> 
> Diffs
> -
> 
>   include/mesos/hook.hpp 210ffba09f5acae34ca49b888a781f683777f9ca 
>   src/examples/test_hook_module.cpp 4b97f84204934d0e678786fd6cde38b89a6f8f48 
>   src/hook/manager.hpp 528674e36639fe78137ba0a4bb004c99730e7a22 
>   src/hook/manager.cpp 381807d582998043d73e9b8c9d3c1fddbcf73cf1 
>   src/slave/slave.cpp 116ea59b72950db4a7cd721b7ba5bfbb2e1c1454 
>   src/tests/hook_tests.cpp 60d52c5849ba555f6f3070883d87aadf105f05b0 
> 
> Diff: https://reviews.apache.org/r/46920/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

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

[Alexander Rukletsov]

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




src/docker/executor.cpp (lines 396 - 397)


Consider starting health checks even if we have already been killed to 
ensure that tasks are health checked while in their kill grace period.



src/launcher/executor.cpp (lines 516 - 518)


If a kill is in progress, consider adjusting the grace period if a new one 
is provided.


- Alexander Rukletsov


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

Re: Review Request 47100: Process binding using greedy, submodular selection algorithm.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On May 9, 2016, 3:21 p.m., Chris wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47100/
> ---
> 
> (Updated May 9, 2016, 3:21 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Bugs: MESOS-5342
> https://issues.apache.org/jira/browse/MESOS-5342
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Process binding using greedy, submodular selection algorithm. First commit 
> might be a bad commit - had issues with RBTools
> 
> 
> Diffs
> -
> 
>   Makefile.am 2b43854b0d36e4ee9c24a0c6f67480c697b8684a 
>   configure.ac 0b9683be6f8d0805f2c04797e839f3578a57efbe 
>   src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
>   src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
> 74982a610b6c0a74734165a0c6aa8c9f72f54deb 
>   src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
> 52d3ddcf80cb0a3f9eb98c082dfb0216c34a7ffd 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
>  PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47100/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chris
> 
>

Review Request 47127: Forgot the algorithm is maximizing when it needs to minimize.

[Chris]

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

Review request for mesos.


Repository: mesos


Description
---

The algorithm is maximizing when it needs to minimize. Simple fix after some 
testing in python.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
 PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47126: Forgot the algorithm is maximizing when it needs to minimize.

[Chris]

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

Review request for mesos.


Repository: mesos


Description
---

The algorithm is maximizing when it needs to minimize. Simple fix after some 
testing in python.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 

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


Testing
---


Thanks,

Chris

Re: Review Request 46806: Fixed and illustrated process::initialize ordering.

[Joris Van Remoortere]

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


Ship it!




Ship It!

- Joris Van Remoortere


On May 2, 2016, 5:50 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46806/
> ---
> 
> (Updated May 2, 2016, 5:50 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Joerg Schad, Joris Van Remoortere, and 
> Kapil Arya.
> 
> 
> Bugs: MESOS-5304
> https://issues.apache.org/jira/browse/MESOS-5304
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moves `metrics::initialize` below the creation of the `help` process.
> 
> Adds a comment block to explain the ordering of process creation in 
> `process::initialize`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> dcaa64633d1eea330649c563635642928164d73c 
> 
> Diff: https://reviews.apache.org/r/46806/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also double-checked that `/help/metrics/snapshot` shows up on both master and 
> agent.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Kapil Arya]

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

(Updated May 9, 2016, 1:10 p.m.)


Review request for mesos, Cody Maloney and Till Toenshoff.


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


Repository: mesos


Description
---

This allows the operator to use separate manifest JSON files for each
module.  Previously, one had to merge all module manifest files into a
single JSON file before passing on to the master/agent.


Diffs (updated)
-

  src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
  src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
  src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
  src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
  src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
  src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
  src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
  src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
  src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 

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


Testing
---

Manual testing with:
1. --module_dir
2. --modules
3. --module_dir in conjunction with --modules

The first two succeeded while the third failed as expected.


Thanks,

Kapil Arya

Re: Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Kapil Arya]

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

(Updated May 9, 2016, 1:05 p.m.)


Review request for mesos, Cody Maloney and Till Toenshoff.


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


Repository: mesos


Description
---

This allows the operator to use separate manifest JSON files for each
module.  Previously, one had to merge all module manifest files into a
single JSON file before passing on to the master/agent.


Diffs (updated)
-

  src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
  src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
  src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
  src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
  src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
  src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
  src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
  src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
  src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 

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


Testing
---

Manual testing with:
1. --module_dir
2. --modules
3. --module_dir in conjunction with --modules

The first two succeeded while the third failed as expected.


Thanks,

Kapil Arya

Review Request 47123: Added --modules_dir flag to read module manifests from a directory.

[Kapil Arya]

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

Review request for mesos, Cody Maloney and Till Toenshoff.


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


Repository: mesos


Description
---

This allows the operator to use separate manifest JSON files for each
module.  Previously, one had to merge all module manifest files into a
single JSON file before passing on to the master/agent.


Diffs
-

  src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
  src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
  src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
  src/module/manager.hpp 9944af0daf6c9cb5a8ff338099401b1db88ee237 
  src/module/manager.cpp 9f88ec3addab59e4a40b0b40612518178d535aa5 
  src/sched/flags.hpp b4ca12b667283cee1f96a4b421fcf3b06bbe59d7 
  src/sched/sched.cpp 4693d0dc09afc3ddbbf34e166579b6a6d71c3e38 
  src/slave/flags.hpp 4fa3213545d4bd3525d85c3f71749f00f08dc998 
  src/slave/flags.cpp 6fde51fc61cfcad61d4085c208bd2eca2eae8f14 
  src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
  src/tests/flags.hpp ae232b1a087edfaf678bd1c67bc509efd6c740d8 
  src/tests/main.cpp c3ccf918c781bdb25b220c7ef3efa7d3b7c88c91 

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


Testing
---

Manual testing with:
1. --module_dir
2. --modules
3. --module_dir in conjunction with --modules

The first two succeeded while the third failed as expected.


Thanks,

Kapil Arya

Re: Review Request 47120: Value from f() was used when it needs to be the value from map<> cost.

[Mesos ReviewBot]

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



Bad review!

Reviews applied: []

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

- Mesos ReviewBot


On May 9, 2016, 3:20 p.m., Chris wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47120/
> ---
> 
> (Updated May 9, 2016, 3:20 p.m.)
> 
> 
> Review request for mesos.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Value from f() was used when it needs to be the value from map<> cost. small 
> bug, the submodular cost was being used to determine if the budget constraint 
> was being satisfied (<=B). Fixed that issue in this commit.
> 
> 
> Diffs
> -
> 
>   
> src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/47120/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Chris
> 
>

Re: Review Request 47071: Added framework/task filtering to /state and /tasks endpoint.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46613, 47068, 47069, 47070, 47071]

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

- Mesos ReviewBot


On May 9, 2016, 2:47 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47071/
> ---
> 
> (Updated May 9, 2016, 2:47 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The /state and /task endpoints output framework and task level data.
> Hence we need to authorize both frameworks and tasks.
> 
> 
> Diffs
> -
> 
>   src/authorizer/local/authorizer.cpp 
> e59c11269670a7ed72b780913971b421ee17f33f 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
> 
> Diff: https://reviews.apache.org/r/47071/diff/
> 
> 
> Testing
> ---
> 
> make check (osx)
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Review Request 47102: Corrected initial utilization value for cores.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Corrected initial utilization value for cores.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47100: Process binding using greedy, submodular selection algorithm.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Process binding using greedy, submodular selection algorithm. First commit 
might be a bad commit - had issues with RBTools


Diffs
-

  Makefile.am 2b43854b0d36e4ee9c24a0c6f67480c697b8684a 
  configure.ac 0b9683be6f8d0805f2c04797e839f3578a57efbe 
  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
74982a610b6c0a74734165a0c6aa8c9f72f54deb 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
52d3ddcf80cb0a3f9eb98c082dfb0216c34a7ffd 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47104: Remove pid from pid-core tracking map.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Remove pid from pid-core tracking map.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47106: Corrected variable names and 'empty' histogram detection logic.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Corrected variable names and 'empty' histogram detection logic.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47101: Process binding using greedy, submodular selection algorithm.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Process binding using greedy, submodular selection algorithm.


Diffs
-

  Makefile.am 2b43854b0d36e4ee9c24a0c6f67480c697b8684a 
  configure.ac 0b9683be6f8d0805f2c04797e839f3578a57efbe 
  src/CMakeLists.txt e0c538d9e6542fbe279bfbf6f20172e4c611c859 
  src/Makefile.am 710e1644e2f0a8e9b87cc997b2211291f4e055fd 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
74982a610b6c0a74734165a0c6aa8c9f72f54deb 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
52d3ddcf80cb0a3f9eb98c082dfb0216c34a7ffd 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47103: Added reference count decrments to core histograms.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Added reference count decrments to core histograms.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/cpushare.hpp 
74982a610b6c0a74734165a0c6aa8c9f72f54deb 
  src/slave/containerizer/mesos/isolators/cgroups/cpushare.cpp 
52d3ddcf80cb0a3f9eb98c082dfb0216c34a7ffd 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.hpp 
PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/ProcessBinder.cpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.hpp 
PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47105: Decided to change the default initial task value.

[Chris]

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

Review request for mesos.


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


Repository: mesos


Description
---

Decided to change the default initial task value.


Diffs
-

  src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/hwloc.cpp 
PRE-CREATION 

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


Testing
---


Thanks,

Chris

Review Request 47120: Value from f() was used when it needs to be the value from map<> cost.

[Chris]

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

Review request for mesos.


Repository: mesos


Description
---

Value from f() was used when it needs to be the value from map<> cost. small 
bug, the submodular cost was being used to determine if the budget constraint 
was being satisfied (<=B). Fixed that issue in this commit.


Diffs
-

  
src/slave/containerizer/mesos/isolators/cgroups/devices/hwloc/SubmodularScheduler.cpp
 PRE-CREATION 

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


Testing
---


Thanks,

Chris

Re: Review Request 46370: Introduced linux capabilities API.

[Jojy Varghese]

> On May 7, 2016, 7:14 p.m., Jie Yu wrote:
> > src/linux/capabilities.hpp, line 194
> > 
> >
> > When I review the code, I found very confusing. We have 
> > `CapabilityInfo, CapabilityInfo::Capability, Capability, Capabilities, 
> > CapabilityFlags, CapabilitySet`. Also, looking at the interfaces of 
> > `Capabilities`, it's not straightfoward to extend it to support file 
> > capabilities. If possible, I'd like the library here that's easily 
> > extensible to support file capabilities.
> > 
> > In fact, I like the abstraction in the go library
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability.go
> > 
> > So, basically, `Capabilities` will become the struct that contains all 
> > capabilities (permitted, inheritted, etc) if applied. and it has methods 
> > defined allowing callers to modify any of them. Finally, there's an `Apply` 
> > method which can be used to apply the change. The methods will be virtual 
> > so that in the future, we can use that to support file capabilities as well:
> > ```
> > typedef int Capability
> > 
> > constexpr int CHOWN = 0;
> > ...
> > 
> > 
> > enum Type
> > {
> >   EFFECTIVE,
> >   PERMITTED,
> >   INHERITABLE,
> >   BOUNDING
> > };
> > 
> > 
> > class Capabilities
> > {
> > public:
> >   static Try fromPid(const Option& pid = 
> > None());
> >   static Try fromFile(const string& path); // TODO.
> >   
> >   virtual bool test(Type type, Capability cap);
> >   virtual void set(Type type, const hashset& caps);
> >   virtual void unset(Type type, const hashset& caps);
> >   virtual void fill(Type type);
> >   virtual void clear(Type type);
> >   
> >   virtual Try apply();
> > };
> > 
> > 
> > // Returns the set of capabilities specified in the protobuf.
> > hashset parse(const CapabilityInfo& info);
> > ```
> > 
> > In the cpp file, you can create a `class ProcessCapabilities : public 
> > Capabilities` that implements the above virtual methods. 
> > 
> > For version and lastCapability, i think it can be a global variable in 
> > the cpp file which will be set the first time `version` is called. Any 
> > subsequent calling of `version` will use the cached value:
> > 
> > ```
> > Try version();
> > Try lastCapability();
> > 
> > Try ProcessCapabilities::create(const 
> > Option& pid = None())
> > {
> >   // you can save version and lastCap in ProcessCapabilities.
> > }
> > ```
> 
> Jojy Varghese wrote:
> Thanks for the review Jie. 
> 
> The idea behind separate classes is to keep the functionality cohesive 
> which I think is an important property of any API. I believe that `test`, 
> `fill`, `clear` etc is not part of the `capabilities` domain  but they change 
> state of `flags`. `Flags` are first class citizens of the capabilities 
> domain. Also, ideally the `capabilities` API should only have `set` and 
> `get`, i had to add `drop` and others to satisfy some of our other 
> requirements. I think if i pull them out into  functions(not part of the 
> class), the `capabilities` class will have only `set` and `get` of flags.
> 
> Also, `set` is an array of `capabilities`. In linux, libcap is supposed 
> to be the standard portable API and I tried to follow that API more that the 
> `go` API.  In libcap, there are functions like `cap_set_flag` to set a 
> particular capabilility (CHOWN) in a set 
> (`http://linux.die.net/man/3/cap_set_flag`). I have tried to put those as 
> part of the `CapabiityFlag` and `CapabilitySet` classes.
> 
> Also, I believe that we should have `Capability` (NET_RAW, CHOWN etc) as 
> first class citizens of the API. For example libcap uses `cap_value_t` as the 
> type for the capabilities. I added `enum Capability` as the type.
> 
> Jie Yu wrote:
> I understand that your interface is modeled after libcap. I looked at 
> both and found that libcap interfaces are a bit confusing and hard to use. In 
> fact, if you looked at https://people.redhat.com/sgrubb/libcap-ng/, "The 
> libcap-ng library is intended to make programming with posix capabilities 
> much easier than the traditional libcap library."
> 
> Meanwhile, I found the go capability library interfaces are quite easy to 
> understand and readable.
> 
> Jojy Varghese wrote:
> I agree that libcap API is confusing. At the same time I think the the 
> capabilities API in docker is not well designed. An API should not be `wide`. 
> It should expose clean cohsive interfaces. The issue with the docker API is 
> that it has no clear idea of ownership. `clear`, `test` etc which are `flags` 
> concern, are packed along with methods that operate on a process's 
> credentials. 
> 
> I also believe that we 

Re: Review Request 46936: Documented the agent endpoint '/flags'.

[Alexander Rukletsov]

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


Ship it!




I'll resolve outstading issues and commit this for you.

- Alexander Rukletsov


On May 3, 2016, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46936/
> ---
> 
> (Updated May 3, 2016, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/flags.md b2740e6a4ce4bb8c25de07071bafbf174adf9137 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/46936/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46935: Documented the agent endpoint '/metrics/snapshot'.

[Alexander Rukletsov]

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


Ship it!




I'll slightly change the wording and commit that for you.

- Alexander Rukletsov


On May 9, 2016, 9:34 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46935/
> ---
> 
> (Updated May 9, 2016, 9:34 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/monitor/statistics.json.md 
> 89a55add1219278b8a7af050f5a0defb6114e9db 
>   docs/endpoints/slave/monitor/statistics.md 
> 3e9a4d1b9d496e94280fe05788f00e9c8a528bb5 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/46935/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang-trunk w/o optimizations)
> 
> I also ran `generate-endpoints` and inspected the generated documentation.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!
> 
> Jan Schlicht wrote:
> Hmm, but there's a reason why we pass an `http::Request` here: The first 
> version of `authorizeEndpoint` took an `endpoint` parameter. Because there's 
> no type for that or rather endpoints are a part of the string "path", this 
> parameter was passed as a `std::string`. That wasn't explicit enough, hence 
> we decided to extract the endpoint for the `Request.path` explicitly in this 
> function. The best solution would be to have an `Endpoint` type and 
> `authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
> path using the same logic that we currenly apply in the function.
> 
> Jan Schlicht wrote:
> Regarding the `MethodNotAllowed`: IMO that's something that shouldn't be 
> returned by `authorizeEndpoint`, but needs to be validated by the routed 
> function before that. The current behavior (i.e. without authz) of for 
> example the "/flags" endpoint (and other endpoints as well) is not right: 
> Sending a POST request will create the same response as a GET request, 
> because the request method isn't validated. Method validation should become 
> part of _every_ endpoint, it doesn't even matter if that endpoint needs to be 
> authorized or not.

I've created (MESOS-5346)[https://issues.apache.org/jira/browse/MESOS-5346] for 
that, but will also add a method validation as part of this RR.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46501]

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

- Mesos ReviewBot


On May 9, 2016, 10:49 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> ---
> 
> (Updated May 9, 2016, 10:49 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4785
> https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!
> 
> Jan Schlicht wrote:
> Hmm, but there's a reason why we pass an `http::Request` here: The first 
> version of `authorizeEndpoint` took an `endpoint` parameter. Because there's 
> no type for that or rather endpoints are a part of the string "path", this 
> parameter was passed as a `std::string`. That wasn't explicit enough, hence 
> we decided to extract the endpoint for the `Request.path` explicitly in this 
> function. The best solution would be to have an `Endpoint` type and 
> `authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
> path using the same logic that we currenly apply in the function.

Regarding the `MethodNotAllowed`: IMO that's something that shouldn't be 
returned by `authorizeEndpoint`, but needs to be validated by the routed 
function before that. The current behavior (i.e. without authz) of for example 
the "/flags" endpoint (and other endpoints as well) is not right: Sending a 
POST request will create the same response as a GET request, because the 
request method isn't validated. Method validation should become part of _every_ 
endpoint, it doesn't even matter if that endpoint needs to be authorized or not.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 47116: Added a missing blank in "slave/http.cpp".

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On May 9, 2016, 12:38 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47116/
> ---
> 
> (Updated May 9, 2016, 12:38 p.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/47116/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 47116: Added a missing blank in "slave/http.cpp".

[Jan Schlicht]

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

(Updated May 9, 2016, 2:38 p.m.)


Review request for mesos and Till Toenshoff.


Changes
---

Addressed Jörg's issues.


Summary (updated)
-

Added a missing blank in "slave/http.cpp".


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.
> 
> Jan Schlicht wrote:
> That's a good point! I'd would also aid in factoring out the 
> `authorizeEndpoint` function like you're suggesting above. Will work on that!

Hmm, but there's a reason why we pass an `http::Request` here: The first 
version of `authorizeEndpoint` took an `endpoint` parameter. Because there's no 
type for that or rather endpoints are a part of the string "path", this 
parameter was passed as a `std::string`. That wasn't explicit enough, hence we 
decided to extract the endpoint for the `Request.path` explicitly in this 
function. The best solution would be to have an `Endpoint` type and 
`authorizeEndpoint` uses that one. An `Endpoint` could be constructed from a 
path using the same logic that we currenly apply in the function.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 47115: Fixed some issues in newbie-guide.md.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47115]

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

- Mesos ReviewBot


On May 9, 2016, 9:56 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47115/
> ---
> 
> (Updated May 9, 2016, 9:56 a.m.)
> 
> 
> Review request for mesos, Neil Conway and Till Toenshoff.
> 
> 
> Bugs: MESOS-5292
> https://issues.apache.org/jira/browse/MESOS-5292
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review fixes some formating issues with the newbie-guide
> including removing a reference to a non-existent image
> (see Mesos-5292 for details).
> 
> 
> Diffs
> -
> 
>   docs/newbie-guide.md 5375fb0598766c1c8bcba39ce552df1405f94a39 
> 
> Diff: https://reviews.apache.org/r/47115/diff/
> 
> 
> Testing
> ---
> 
> viewed via docker website container.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47116: Added a missing blank.

[Joerg Schad]

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




src/slave/http.cpp (line 375)


Could you maybe be a little more precise in the summary e.g., "Added a 
missing blank line to http.cpp".


- Joerg Schad


On May 9, 2016, 11:54 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47116/
> ---
> 
> (Updated May 9, 2016, 11:54 a.m.)
> 
> 
> Review request for mesos and Till Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/47116/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On April 29, 2016, 11:20 a.m., Benjamin Bannier wrote:
> > src/master/http.cpp, line 868
> > 
> >
> > Could you make this capture list explicit (`[this, request]`)?
> 
> Jan Schlicht wrote:
> The style guide recommends to prefer default capture by value, that's why 
> I'm doing it here. The case here is different from the one in 
> `slave/http.cpp`. There we explicitly captured by value to be sure not to 
> capture `this`, because that would lead to problems there. We don't have this 
> problem here, hence the default capture by value.
> 
> Benjamin Bannier wrote:
> Note that `[this, request]` *does* capture by value. I am just asking you 
> to be explicit about what you capture instead of pulling in everything from 
> the current scope.
> 
> Jan Schlicht wrote:
> As does `[=]`, and that's preferred by the style guide.
> 
> Benjamin Bannier wrote:
> Hm, I read that part of the style guide differently, but well. I believe 
> we value explicit over implicit, but even in this file there seems to be no 
> preference either way, so feel free to drop this issue.

Seems that I'm the only one that read the style guide that way, see the 
discussion with @alexr below. I'll change it to explicit capture.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.
> 
> Alexander Rukletsov wrote:
> So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you 
> think it's right?
> 
> In retrospect, there was a mistake to move request parsing into this 
> function. You know check that the request is correctly formed in 
> `authorizeEndpoint()`, from where you can't return `Response`. I suggest to 
> split this function in two: one for parsing and another for mapping principal 
> to subject.

That's a good point! I'd would also aid in factoring out the 
`authorizeEndpoint` function like you're suggesting above. Will work on that!


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?
> 
> Jan Schlicht wrote:
> See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
> tldr; We can do it for the master, but (currently) not for the agent.
> 
> Alexander Rukletsov wrote:
> But it does not prevent us from following the pattern we used in the 
> agent here, right?

No, it doesn't. But it's the pattern we used in the agent that's the outlier 
here. In other cases we are able to access `this` in the continuation and make 
use of it. IMO this is the pattern we should prefer: Having a non-static 
continuation, that can access `this` and its members and change the 
continuation in `Slave::Http` to that pattern as soon as MESOS-5293 is resolved.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?
> 
> Jan Schlicht wrote:
> Yes, I only need to capture `request` here, but the way I understand the 
> C++ style guide, "default capture by value" ("=") is preferred over "explicit 
> capture by value".
> 
> Alexander Rukletsov wrote:
> I'm not sure this is what the styleguide says. I read "Prefer default 
> capture by value, explicit capture by value, then capture by reference." as 
> explicit and default captures both take precedence over captures by 
> reference. Anyway, if explicit capture is fine, this is the best use case for 
> it: a lambda taking one parameter, which is exactly your case. Please change 
> this.

Good to know. I'm actually a fan of explicit capture -- as long as the 
parameter list isn't too long. I've had a similar discussion with @bbannier 
about that above. There it's about the explicit capture of 2 parameters. I'll 
also change that there.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )
> 
> Jan Schlicht wrote:
> Because `authorizeEndpoint` isn't supposed to create a `http::Response`. 
> Its purpose is to answer the question whether a principal is authorized to do 
> things and it's the caller's responsibility to create a HTTP response from 
> that answer. But to create the answer, the function needs informations that 
> it extracts from the HTTP request. Because these informations may be missing, 
> we need to handle that case here, hence we fail the `Future`.

So now we return "HTTP/1.1 503 Service Unavailable" in this case. Do you think 
it's right?

In retrospect, there was a mistake to move request parsing into this function. 
You know check that the request is correctly formed in `authorizeEndpoint()`, 
from where you can't return `Response`. I suggest to split this function in 
two: one for parsing and another for mapping principal to subject.


> On May 9, 2016, 9:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?
> 
> Jan Schlicht wrote:
> See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
> tldr; We can do it for the master, but (currently) not for the agent.

But it does not prevent us from following the pattern we used in the agent 
here, right?


- Alexander


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


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:18 a.m., Alexander Rukletsov wrote:
> > Also, do you want to update configuration.md?

To add the "get_endpoints" action to the ACL example? Yep, makes sense to do 
that here, will add it.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?
> 
> Jan Schlicht wrote:
> Yes, I only need to capture `request` here, but the way I understand the 
> C++ style guide, "default capture by value" ("=") is preferred over "explicit 
> capture by value".

I'm not sure this is what the styleguide says. I read "Prefer default capture 
by value, explicit capture by value, then capture by reference." as explicit 
and default captures both take precedence over captures by reference. Anyway, 
if explicit capture is fine, this is the best use case for it: a lambda taking 
one parameter, which is exactly your case. Please change this.


> On May 7, 2016, 10:24 a.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?
> 
> Jan Schlicht wrote:
> Indeed, that's quite some code duplication. But there are also some 
> slight differences in both functions. Had a discussion with @arojas about how 
> to handle that case. On one hand it's good to remove code duplication, on the 
> other hand the extracted function would look a bit more complicated to 
> support the different requirements of master and agent. We decided that in 
> this particular case it's okay to stick with the two similar functions.

If we split this function in two (per suggestion below), the copy-paste 
wouldn't be so drastic: we already have a lot of functions that do mapping 
principal->subject (it is unfortunate we have to repeat it again and again) and 
a fucntion that extracts endpoint from an URL path. Maybe we can add a helper 
in `Request` for this one.


- Alexander


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


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 47116: Added a missing blank.

[Jan Schlicht]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 

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


Testing
---

make check


Thanks,

Jan Schlicht

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:14 a.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, line 814
> > 
> >
> > I think we should have used `MethodNotAllowed` here.

See my comment in https://reviews.apache.org/r/46784/. I don't think we should 
use `MethodNotAllowed` because it's not the purpose of this function to return 
an `http::Response`. What do you think?


- Jan


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


On April 27, 2016, 4:32 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 27, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2796a812b72f208b1ae2d65a4ba843b50d70 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636 
>   src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 
> --gtest_filter=*SlaveAuthorization*
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 5, 2016, 7:20 p.m., Till Toenshoff wrote:
> > src/slave/http.cpp, line 373
> > 
> >
> > Two blanks please.

Good catch! I've created https://reviews.apache.org/r/47116/ to fix that.


- Jan


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


On April 27, 2016, 4:32 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 27, 2016, 4:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2796a812b72f208b1ae2d65a4ba843b50d70 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636 
>   src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 
> --gtest_filter=*SlaveAuthorization*
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

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

[Joerg Schad]

> On May 9, 2016, 11:12 a.m., Alexander Rojas wrote:
> > docs/authorization.md, lines 90-101
> > 
> >
> > Do we really need to distinguish this one as a different case of 
> > general authorization? I personally believe no (except for performance 
> > issues), but in that case I would prefer an opt-out flag than an opt-in one.

So you would prefere the default to be set to true? Not sure that makes sense, 
as it is an advanced feature which should be turned on knowing about the 
consequences...
I believe adding a separate note/section to the document makes sense as a) it 
is different from the all/nothing authorization behavior of the other cases 
(where I simply get a "Not Authorized reply"), b) it has performance 
implications I feel are important to understand for users...


- Joerg


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


On May 6, 2016, 8:31 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47068/
> ---
> 
> (Updated May 6, 2016, 8:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a master flag enabling the fine-grained
> authorization based filtering of HTTP endpoints.
> The default for this setting is `false` as the filtering incurs
> a performance penalty.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
> 
> Diff: https://reviews.apache.org/r/47068/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Alexander Rojas]

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



In the description: `/protobug/protobuf/`, also, I would avoid starting the 
description with _As…_


src/common/protobuf_utils.cpp (line 185)


Not yours but I die a little every time I see a variable named with one 
letter.


- Alexander Rojas


On May 6, 2016, 10:33 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47069/
> ---
> 
> (Updated May 6, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As the LocalAuthorizer will use the OS `user` for
> authorization of tasks we add it to the `Task` protobug
> message. Note that the master stores `Task` (as opposed
> to `TaskInfo`) for running and completed tasks.
> 
> 
> Diffs
> -
> 
>   src/common/protobuf_utils.cpp 4f4711d54c471922f1a103310d4d360e41a99870 
>   src/messages/messages.proto e0f1fca92d3ea8c29c095da31653c317873a934c 
> 
> Diff: https://reviews.apache.org/r/47069/diff/
> 
> 
> Testing
> ---
> 
> tested entire check.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

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

[Alexander Rojas]

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




docs/authorization.md (lines 90 - 101)


Do we really need to distinguish this one as a different case of general 
authorization? I personally believe no (except for performance issues), but in 
that case I would prefer an opt-out flag than an opt-in one.


- Alexander Rojas


On May 6, 2016, 10:31 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47068/
> ---
> 
> (Updated May 6, 2016, 10:31 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a master flag enabling the fine-grained
> authorization based filtering of HTTP endpoints.
> The default for this setting is `false` as the filtering incurs
> a performance penalty.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
>   docs/configuration.md 34271c76d10ad930e6cc586c2b820ce8989a053a 
>   src/master/flags.hpp e4cac1f8d688319c804e608b7229f458f779364a 
>   src/master/flags.cpp c0c9e924e876175b75a174e375a4c993d97e18ee 
> 
> Diff: https://reviews.apache.org/r/47068/diff/
> 
> 
> Testing
> ---
> 
> tested entire chain.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 47114: Cleaned up angle bracket style.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [47112, 47113, 47114]

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

- Mesos ReviewBot


On May 9, 2016, 9:10 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/47114/
> ---
> 
> (Updated May 9, 2016, 9:10 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Cleaned up angle bracket style.
> 
> 
> Diffs
> -
> 
>   src/zookeeper/contender.cpp 5eb4b581542f5dca6050fc2d4360fb91f999092b 
> 
> Diff: https://reviews.apache.org/r/47114/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Alexander Rojas]

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

(Updated May 9, 2016, 12:49 p.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
Conway.


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


Repository: mesos


Description
---

The API of the authorization has been changing constantly over the
last few versions. This patch attempts to update the documentation to
the those changes into account.


Diffs (updated)
-

  docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 

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


Testing
---


Thanks,

Alexander Rojas

Re: Review Request 46423: Windows: Forked `subprocess.cpp`.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On May 5, 2016, 3:35 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46423/
> ---
> 
> (Updated May 5, 2016, 3:35 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-3637
> https://issues.apache.org/jira/browse/MESOS-3637
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows: Forked subprocess.cpp, added `Windows` implementation.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/Makefile.am c51c31eb91dd7be4cff8a188942ea77b3b361d45 
>   3rdparty/libprocess/include/Makefile.am 
> 47f5347988a61140c87bcd329e25d5a4d52e17a0 
>   3rdparty/libprocess/include/process/posix/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 6ce1a827e36e0c65985e3928c89f51561fdd70cd 
>   3rdparty/libprocess/include/process/subprocess_base.hpp PRE-CREATION 
>   3rdparty/libprocess/include/process/windows/subprocess.hpp PRE-CREATION 
>   3rdparty/libprocess/src/CMakeLists.txt 
> a8379d323f30037bdd15e151957f885664b5e242 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/subprocess_posix.cpp PRE-CREATION 
>   3rdparty/libprocess/src/subprocess_windows.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46423/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 46869: Allowed tests to authorize libprocess HTTP endpoints.

[Alexander Rojas]

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




src/tests/cluster.cpp (lines 424 - 432)


Since callbacks are a libprocess property and not really additive, wouldn't 
this effectively overwrite the callbacks added by the master or any previously 
created agent?


- Alexander Rojas


On May 6, 2016, 11:16 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46869/
> ---
> 
> (Updated May 6, 2016, 11:16 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-5286
> https://issues.apache.org/jira/browse/MESOS-5286
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The test cluster initialization code was altered
> to enable authorization of libprocess-level HTTP
> endpoints when a master or agent is launched
> with authorization enabled.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp db7262868164b1966a9824b8aa53dbe9c5af7c2f 
> 
> Diff: https://reviews.apache.org/r/46869/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46867: Enabled authorization of libprocess HTTP endpoints (Mesos).

[Alexander Rojas]

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




src/common/http.cpp (line 587)


method name in lowecase.

Perhaps `createEmptyAuthorizationCallbacks`?



src/master/main.cpp (lines 430 - 431)


Does it? The container `Option` is copied, but all copies of that `Option` 
will be refering to the same pointer.



src/slave/main.cpp (lines 320 - 321)


Does it? The container `Option` is copied, but all copies of that `Option` 
will be refering to the same pointer.


- Alexander Rojas


On May 6, 2016, 11:14 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46867/
> ---
> 
> (Updated May 6, 2016, 11:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-5286
> https://issues.apache.org/jira/browse/MESOS-5286
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Code is added to the common headers and the
> master/agent executables which sets authorization
> callbacks for libprocess-level HTTP endpoints.
> This allows these endpoints to be authorized.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp 6c6f2840bbf49d198b1db876cdf4af5ef49b0e27 
>   src/common/http.cpp ccf386898130c966903cb5aae4eaffbc9b63ca1f 
>   src/master/main.cpp 23149d5511d1556f1a885d01ea9380a9669fa8c5 
>   src/slave/main.cpp fee46bafc88f8cdade868aab8c0fee79b8d2fb6d 
> 
> Diff: https://reviews.apache.org/r/46867/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 46866: Enabled authorization of libprocess HTTP endpoints (libprocess).

[Alexander Rojas]

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




3rdparty/libprocess/include/process/http.hpp (line 85)


Is there a design doc or an overall explanation of how authorization will 
be performed in libprocess?

I find it cool if we provide an API which can be used in Mesos, similar to 
how authentication works, also to keep them consistent.

Of the top of my head, how about changing `route` to use a set of flags, 
similar on how streams work in the stdlib so we can define if you want 
authentication + authorization, etc.



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


Why no setting this variable close to were it will be used. I always liked 
the concept of data locality, so someone doesn't have to scroll up to see where 
a variable was set.


- Alexander Rojas


On May 6, 2016, 11:13 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46866/
> ---
> 
> (Updated May 6, 2016, 11:13 p.m.)
> 
> 
> Review request for mesos, Alexander Rojas and Kapil Arya.
> 
> 
> Bugs: MESOS-5286
> https://issues.apache.org/jira/browse/MESOS-5286
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch enables libprocess to store and execute
> authorization callbacks provided by a client
> application.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/http.hpp 
> 8f4eabcbb71ead1f5c28e1d8a2dd40db7af1f297 
>   3rdparty/libprocess/src/process.cpp 
> dcaa64633d1eea330649c563635642928164d73c 
> 
> Diff: https://reviews.apache.org/r/46866/diff/
> 
> 
> Testing
> ---
> 
> `make check` on OSX.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 47115: Fixed some issues in newbie-guide.md.

[Joerg Schad]

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

Review request for mesos and Till Toenshoff.


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


Repository: mesos


Description
---

This review fixes some formating issues with the newbie-guide
including removing a reference to a non-existent image
(see Mesos-5292 for details).


Diffs
-

  docs/newbie-guide.md 5375fb0598766c1c8bcba39ce552df1405f94a39 

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


Testing
---

viewed via docker website container.


Thanks,

Joerg Schad

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [46501]

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

- Mesos ReviewBot


On May 9, 2016, 8:49 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> ---
> 
> (Updated May 9, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4785
> https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 46935: Documented the agent endpoint '/metrics/snapshot'.

[Benjamin Bannier]

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

(Updated May 9, 2016, 11:34 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil Conway.


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  docs/endpoints/slave/monitor/statistics.json.md 
89a55add1219278b8a7af050f5a0defb6114e9db 
  docs/endpoints/slave/monitor/statistics.md 
3e9a4d1b9d496e94280fe05788f00e9c8a528bb5 
  src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 

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


Testing
---

make check (OS X, clang-trunk w/o optimizations)

I also ran `generate-endpoints` and inspected the generated documentation.


Thanks,

Benjamin Bannier

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Neil Conway]

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




docs/authorization.md (line 12)


I'd avoid passive voice here: "The next section explores the concepts 
necessary to successfully configure the local authorizer". Personally I'd opt 
for removing the rest of the paragraph.

Typos: "sections", "finalized", "discussion".

Personally I would opt for using italics around "local authorizer" only the 
first time the term is used, and then not using italics for subsequent uses.



docs/authorization.md (line 14)


"The final section briefly discusses how to implement a custom authorizer."



docs/authorization.md (line 462)


I'd move this section up before the examples.


- Neil Conway


On May 9, 2016, 8:49 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> ---
> 
> (Updated May 9, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4785
> https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/tests/master_authorization_tests.cpp, line 1033
> > 
> >
> > Doesn't it compile without explicit casting to string?

It doesn't. See the discussion with Benjamin above.


> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 2872
> > 
> >
> > Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
> > actually : )

Because `authorizeEndpoint` isn't supposed to create a `http::Response`. Its 
purpose is to answer the question whether a principal is authorized to do 
things and it's the caller's responsibility to create a HTTP response from that 
answer. But to create the answer, the function needs informations that it 
extracts from the HTTP request. Because these informations may be missing, we 
need to handle that case here, hence we fail the `Future`.


> On May 9, 2016, 11:12 a.m., Alexander Rukletsov wrote:
> > src/master/http.cpp, line 879
> > 
> >
> > In r/46203/ you've cached flags and passed them into continuation. Here 
> > you call `master->flags` in the continuation. Why is the approach different 
> > to the one for the agent?

See [MESOS-5293](https://issues.apache.org/jira/browse/MESOS-5293).
tldr; We can do it for the master, but (currently) not for the agent.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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



Also, do you want to update configuration.md?

- Alexander Rukletsov


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46203: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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




src/slave/http.cpp (line 814)


I think we should have used `MethodNotAllowed` here.


- Alexander Rukletsov


On April 27, 2016, 2:32 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46203/
> ---
> 
> (Updated April 27, 2016, 2:32 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5142
> https://issues.apache.org/jira/browse/MESOS-5142
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 2796a812b72f208b1ae2d65a4ba843b50d70 
>   include/mesos/authorizer/acls.proto 
> c50deeb5565dfd5b3e5e7210283d9a36a3bfd579 
>   include/mesos/authorizer/authorizer.proto 
> 40d93ea257d1df8d22eee8a21667db90d579a8fe 
>   src/Makefile.am e024c6d65608a55765e527a8668c415723dcfcca 
>   src/authorizer/local/authorizer.cpp 
> 0a3805fe4ce8eb89e096e8cd4326035513ba892b 
>   src/slave/flags.cpp a319d60c006d1104836c1c40f3617ceac9cb7b1e 
>   src/slave/http.cpp 537736d1fe42e8150bad91326299ef9a17041a8e 
>   src/slave/slave.hpp 57b18882e30e44dcc40449b0e3be8ee970c45bc8 
>   src/tests/authorization_tests.cpp d4ef0f94c86b1287c98671c5d478ce6ac3d90636 
>   src/tests/mesos.hpp 78edab89e6c13ff0892b2f3b5cb6886f08b02c82 
>   src/tests/slave_authorization_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/46203/diff/
> 
> 
> Testing
> ---
> 
> ./bin/mesos-tests.sh --gtest_shuffle
> ./bin/mesos-tests.sh --gtest_shuffle --gtest_repeat=100 
> --gtest_filter=*SlaveAuthorization*
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Alexander Rukletsov]

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



Looks good! One more round and we can commit this.

Do you want to add an integration test like you did in r/46203 with 
`AuthorizeFlagsEndpoint`?


src/master/http.cpp (line 879)


In r/46203/ you've cached flags and passed them into continuation. Here you 
call `master->flags` in the continuation. Why is the approach different to the 
one for the agent?



src/master/http.cpp (line 2872)


Why don't we return `MethodNotAllowed` here? Same applies to the agent, 
actually : )



src/tests/master_authorization_tests.cpp (line 1033)


Doesn't it compile without explicit casting to string?



src/tests/master_authorization_tests.cpp (lines 1036 - 1037)


Please re-wrap to reduce jaggeddness.



src/tests/master_authorization_tests.cpp (lines 1100 - 1101)


Ditto.


- Alexander Rukletsov


On May 3, 2016, 12:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 12:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Review Request 47112: Cleaned up header includes.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


Repository: mesos


Description
---

Avoid depending on transitively included headers.


Diffs
-

  src/zookeeper/contender.cpp 5eb4b581542f5dca6050fc2d4360fb91f999092b 

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


Testing
---

make


Thanks,

Neil Conway

Review Request 47113: Cleaned up `using` statements.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


Repository: mesos


Description
---

Avoid including the entire `process` namespace; instead, only pull
in the particular identifiers from that namespace that are needed.


Diffs
-

  src/zookeeper/contender.cpp 5eb4b581542f5dca6050fc2d4360fb91f999092b 

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


Testing
---

make


Thanks,

Neil Conway

Review Request 47114: Cleaned up angle bracket style.

[Neil Conway]

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

Review request for mesos, Alexander Rukletsov and Ben Mahler.


Repository: mesos


Description
---

Cleaned up angle bracket style.


Diffs
-

  src/zookeeper/contender.cpp 5eb4b581542f5dca6050fc2d4360fb91f999092b 

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


Testing
---

make


Thanks,

Neil Conway

Re: Review Request 46935: Documented the agent endpoint '/metrics/snapshot'.

[Benjamin Bannier]

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

(Updated May 9, 2016, 11:04 a.m.)


Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil Conway.


Changes
---

alexr's comments.


Repository: mesos


Description (updated)
---

See summary.


Diffs (updated)
-

  docs/endpoints/slave/monitor/statistics.json.md 
89a55add1219278b8a7af050f5a0defb6114e9db 
  docs/endpoints/slave/monitor/statistics.md 
3e9a4d1b9d496e94280fe05788f00e9c8a528bb5 
  src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 

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


Testing
---

make check (OS X, clang-trunk w/o optimizations)

I also ran `generate-endpoints` and inspected the generated documentation.


Thanks,

Benjamin Bannier

Re: Review Request 46935: Documented the agent endpoint '/metrics/snapshot'.

[Benjamin Bannier]

> On May 4, 2016, 10:34 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, lines 623-624
> > 
> >
> > How about something like: "The principal should be authorized to query 
> > this endpoint"?
> 
> Neil Conway wrote:
> +1, although I'd say "The current principal ..."
> 
> Alexander Rukletsov wrote:
> I'm ESL, but for me "current" in this context sounds like a principal 
> which is stored or cached by Mesos, and hence "current" to the context. But 
> we are talking about the principal from the request, which may be observed by 
> Mesos for the first time. Maybe we can find a better word, e.g. provided, 
> specified, or "request principal"?

Using `The principal should be authorized to query this endpoint.` for now.


- Benjamin


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


On May 9, 2016, 11:04 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46935/
> ---
> 
> (Updated May 9, 2016, 11:04 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/monitor/statistics.json.md 
> 89a55add1219278b8a7af050f5a0defb6114e9db 
>   docs/endpoints/slave/monitor/statistics.md 
> 3e9a4d1b9d496e94280fe05788f00e9c8a528bb5 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/46935/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang-trunk w/o optimizations)
> 
> I also ran `generate-endpoints` and inspected the generated documentation.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46784: Added authorization of the '/flags' endpoint.

[Jan Schlicht]

> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 869
> > 
> >
> > Did you need to pass all ("=") the in-scope variables through to the 
> > lambda, or just `request`?

Yes, I only need to capture `request` here, but the way I understand the C++ 
style guide, "default capture by value" ("=") is preferred over "explicit 
capture by value".


> On May 7, 2016, 12:24 p.m., Adam B wrote:
> > src/master/http.cpp, line 2846
> > 
> >
> > There's a lot of code duplication between here and 
> > `Slave::Http::authorizeEndpoint()`. Can you share/reuse code between the 
> > two?

Indeed, that's quite some code duplication. But there are also some slight 
differences in both functions. Had a discussion with @arojas about how to 
handle that case. On one hand it's good to remove code duplication, on the 
other hand the extracted function would look a bit more complicated to support 
the different requirements of master and agent. We decided that in this 
particular case it's okay to stick with the two similar functions.


- Jan


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


On May 3, 2016, 2:42 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46784/
> ---
> 
> (Updated May 3, 2016, 2:42 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Benjamin Bannier.
> 
> 
> Bugs: MESOS-5297
> https://issues.apache.org/jira/browse/MESOS-5297
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Access to the '/flags' endpoint is authorized using
> the 'GET_ENDPOINT_WITH_PATH' action.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 4985f24b70a00116caa4bd0335ea51e55d878d19 
>   src/master/master.hpp 3e55114ee7866e06513071e86e15608099dae052 
>   src/tests/master_authorization_tests.cpp 
> 804b39a269c09df9f6c0bbdf6f8b53921ac09ce8 
> 
> Diff: https://reviews.apache.org/r/46784/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Alexander Rojas]

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

(Updated May 9, 2016, 10:49 a.m.)


Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
Conway.


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


Repository: mesos


Description
---

The API of the authorization has been changing constantly over the
last few versions. This patch attempts to update the documentation to
the those changes into account.


Diffs (updated)
-

  docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 

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


Testing
---


Thanks,

Alexander Rojas

Re: Review Request 46501: Updated authorization.md to reflect current changes.

[Alexander Rojas]

> On April 29, 2016, 4:07 p.m., Neil Conway wrote:
> > docs/authorization.md, line 276
> > 
> >
> > For consistency, this needs a newline (``).

using table instead.


- Alexander


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


On May 4, 2016, 10:39 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46501/
> ---
> 
> (Updated May 4, 2016, 10:39 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Greg Mann, and Neil 
> Conway.
> 
> 
> Bugs: MESOS-4785
> https://issues.apache.org/jira/browse/MESOS-4785
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The API of the authorization has been changing constantly over the
> last few versions. This patch attempts to update the documentation to
> the those changes into account.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 0db5c345b3239814b3b9d2e8a87601ff69d0f869 
> 
> Diff: https://reviews.apache.org/r/46501/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 46935: Documented the agent endpoint '/metrics/snapshot'.

[Alexander Rukletsov]

> On May 4, 2016, 8:34 p.m., Alexander Rukletsov wrote:
> > src/slave/http.cpp, lines 623-624
> > 
> >
> > How about something like: "The principal should be authorized to query 
> > this endpoint"?
> 
> Neil Conway wrote:
> +1, although I'd say "The current principal ..."

I'm ESL, but for me "current" in this context sounds like a principal which is 
stored or cached by Mesos, and hence "current" to the context. But we are 
talking about the principal from the request, which may be observed by Mesos 
for the first time. Maybe we can find a better word, e.g. provided, specified, 
or "request principal"?


- Alexander


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


On May 3, 2016, 2:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46935/
> ---
> 
> (Updated May 3, 2016, 2:17 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Documented the agent endpoint '/metrics/snapshot'.
> 
> 
> Diffs
> -
> 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/46935/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang-trunk w/o optimizations)
> 
> I also ran `generate-endpoints` and inspected the generated documentation.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46936: Documented the agent endpoint '/flags'.

[Alexander Rukletsov]

> On May 5, 2016, 4:15 p.m., Neil Conway wrote:
> > src/slave/http.cpp, lines 356-357
> > 
> >
> > +1, although I'd say "The current principal should be ..."

I'm ESL, but for me "current" in this context sounds like a principal which is 
stored or cached by Mesos, and hence "current" to the context. But we are 
talking about the principal from the request, which may be observed by Mesos 
for the first time. Maybe we can find a better word, e.g. provided, specified, 
or "request principal"?


- Alexander


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


On May 3, 2016, 2:36 p.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46936/
> ---
> 
> (Updated May 3, 2016, 2:36 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Alexander Rojas, and Neil 
> Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/slave/flags.md b2740e6a4ce4bb8c25de07071bafbf174adf9137 
>   src/slave/http.cpp 9b558862e025c5caa71e05fc5eeba783c0ad6fd5 
> 
> Diff: https://reviews.apache.org/r/46936/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

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

[haosdent huang]

> On May 7, 2016, 1:22 p.m., haosdent huang wrote:
> > src/docker/executor.cpp, line 398
> > 
> >
> > Should it change to `if (killed || terminated ||..)`?
> 
> Alexander Rukletsov wrote:
> Currently, if `terminated == true` then `killed == true` as well. There 
> is no need to introduce an extra condition.

Got it, thx!


- haosdent


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


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

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

[Alexander Rukletsov]

> On May 7, 2016, 1:22 p.m., haosdent huang wrote:
> > src/docker/executor.cpp, line 398
> > 
> >
> > Should it change to `if (killed || terminated ||..)`?

Currently, if `terminated == true` then `killed == true` as well. There is no 
need to introduce an extra condition.


- Alexander


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


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

Re: Review Request 36440: Enabled docker volume support for DockerContainerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [36440]

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

- Mesos ReviewBot


On May 9, 2016, 5:25 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36440/
> ---
> 
> (Updated May 9, 2016, 5:25 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5341
> https://issues.apache.org/jira/browse/MESOS-5341
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enabled docker volume support for DockerContainerizer.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 73d4bc0ec3f34033eddf04aa41893c1fc66933b3 
>   include/mesos/v1/mesos.proto 44b4f8a059f9dfdcbf02f0c30c1b859898c2e617 
>   src/docker/docker.cpp 19cf424dfd5748ae66a7023840aa2b0652e8f2c0 
> 
> Diff: https://reviews.apache.org/r/36440/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 1) Start convoy
> 2) Create a volume named as c1
> root@kvm-002605:~# convoy list
> {
> "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad": {
> "UUID": "f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Name": "c1",
> "Driver": "devicemapper",
> "MountPoint": "",
> "CreatedTime": "Mon May 09 09:38:52 +0800 2016",
> "DriverInfo": {
> "DevID": "1",
> "Device": 
> "/dev/mapper/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Driver": "devicemapper",
> "MountPoint": "",
> "Size": "107374182400"
> },
> "Snapshots": {}
> }
> }
> 3) Update  mesos-execte to set up volume
> index 4711e80..6c712a2 100644
> --- a/src/cli/execute.cpp
> +++ b/src/cli/execute.cpp
> @@ -72,6 +72,8 @@ using mesos::v1::TaskID;
>  using mesos::v1::TaskInfo;
>  using mesos::v1::TaskState;
>  using mesos::v1::TaskStatus;
> +using mesos::v1::Volume;
> +using mesos::v1::Parameters;
>  
>  using mesos::v1::scheduler::Call;
>  using mesos::v1::scheduler::Event;
> @@ -581,6 +583,18 @@ private:
>  
>containerInfo.set_type(ContainerInfo::DOCKER);
>containerInfo.mutable_docker()->set_image(dockerImage.get());
> +  //containerInfo.mutable_docker()->set_volume_driver("convoy");
> +
> +  Volume* volume1 = containerInfo.add_volumes();
> +  //volume1->set_host_path("c1");
> +  volume1->set_container_path("/tmp");
> +  volume1->mutable_source()->set_type(Volume::Source::DOCKER_VOLUME);
> +  volume1->mutable_source()->mutable_docker_volume()->set_driver(
> +  "convoy");
> +  volume1->mutable_source()->mutable_docker_volume()->set_name(
> +  "c1");
> +  volume1->set_mode(Volume::RW);
> +  cout << "Add Voume 1" << endl;
>  
>if (networks.isSome() && !networks->empty()) {
>  vector tokens = strings::tokenize(networks.get(), ",");
> 4) Start master
> ./bin/mesos-master.sh --work_dir=/tmp/mesos/master
> 5) Start agent
> GLOG_v=1 ./bin/mesos-slave.sh --master=10.0.0.65:5050 
> --isolation=filesystem/linux,docker/runtime --containerizers=docker 
> --work_dir=/tmp/mesos --image_providers=docker
> 5) Start mesos-execute
> src/mesos-execute --master=10.0.0.65:5050 --name=test --command="sleep 1" 
> --docker_image=ubuntu:14.04 --containerizer=docker
> I0509 13:14:56.118808  8320 scheduler.cpp:177] Version: 0.29.0
> I0509 13:14:56.120827  8346 scheduler.cpp:479] New master detected at 
> master@10.0.0.65:5050
> Subscribed with ID 'acff7546-e8bf-4fad-a651-307c73297db0-'
> Add Voume 1
> Submitted task 'test' to agent '5454a953-83d0-42a4-b7fe-0825a6667f8c-S0'
> Received status update TASK_RUNNING for task 'test'
>   source: SOURCE_EXECUTOR
> Received status update TASK_LOST for task 'test'
> 
> 6) Inspect container
> docker inspect 
> ...
> {
> "Name": "c1",
> "Source": 
> "/var/lib/convoy/devicemapper/mounts/f3459d44-bbed-4e01-a7f6-6c6aa4b70cad",
> "Destination": "/tmp",
> "Driver": "convoy",
> "Mode": "rw",
> "RW": true,
> "Propagation": "rprivate"
> }
> ...
> 7) Check sandbox executer log
> I0509 13:14:56.313102  8361 logging.cpp:195] Logging to STDERR
> I0509 13:14:56.315385  8361 process.cpp:999] libprocess is initialized on 
> 10.0.0.65:52853 with 16 worker threads
> I0509 13:14:56.316789  8361 exec.cpp:150] Version: 0.29.0
> I0509 13:14:56.321717  8387 exec.cpp:200] Executor started at: 
> executor(1)@10.0.0.65:52853 with pid 8361
> I0509 13:14:56.325158  8388