Re: Review Request 51446: Fixed typo in health_checker.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51446]

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

- Mesos ReviewBot


On Aug. 26, 2016, 1:33 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51446/
> ---
> 
> (Updated Aug. 26, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo in health_checker.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> 45a5fe00a95a6e88b1990c1396e03082feb202bc 
> 
> Diff: https://reviews.apache.org/r/51446/diff/
> 
> 
> Testing
> ---
> 
> make && make check on OS X
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Re: Review Request 51097: Added a `PortMapper` class.

[Avinash sridharan]

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

(Updated Aug. 26, 2016, 5:21 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This class will embody the logic for implementing the CNI port-mapper
plugin.


Diffs (updated)
-

  src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 PRE-CREATION 

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


Testing
---

Tested the port-mapper with the following CNI config:
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS",
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "10.22.0.0/16",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 80,
"container_port" : 80
  }
}
  }
}
}

and the following environment variables:
export CNI_COMMAND="ADD"
export CNI_CONTAINERID="00001110"
export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
export CNI_IFNAME="eth0"
export CNI_NETNS="/etc/netns"

If we remove fields from the above CNI config, or remove certain environment 
variables the creation of the `PortMapper` correctly fails. However, if config 
and environment variables are passed as is it will create the `PortMapper` 
correctly.


Thanks,

Avinash sridharan

Review Request 51450: Fixed a capabilities test failure on Fedora 23.

[Jie Yu]

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

Review request for mesos and Benjamin Bannier.


Repository: mesos


Description
---

Fixed a capabilities test failure on Fedora 23.


Diffs
-

  src/tests/containerizer/capabilities_tests.cpp 
ec75698cb969d745c82a8358e54e8d8b24f14f7c 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 51097: Added a `PortMapper` class.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [51097, 51096, 51095]

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

Error:
2016-08-26 03:49:57 URL:https://reviews.apache.org/r/51097/diff/raw/ 
[15668/15668] -> "51097.patch" [1]
error: patch failed: src/Makefile.am:1061
error: src/Makefile.am: patch does not apply
error: patch failed: 
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp:23
error: 
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp:
 patch does not apply

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

- Mesos ReviewBot


On Aug. 25, 2016, 10:04 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51097/
> ---
> 
> (Updated Aug. 25, 2016, 10:04 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This class will embody the logic for implementing the CNI port-mapper
> plugin.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am d389e02584cfb1a00067cd30de1364118d1c46c7 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
>  PRE-CREATION 
>   
> src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
>  PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51097/diff/
> 
> 
> Testing
> ---
> 
> Tested the port-mapper with the following CNI config:
> {
> "name": "mynet",
> "type": "port-mapper",
> "chain": "MESOS",
> "delegate": {
>   "type" : "bridge",
>   "bridge": "cni0",
>   "isGateway": true,
>   "ipMasq": true,
>   "ipam": {
>   "type": "host-local",
>   "subnet": "10.22.0.0/16",
>   "routes": [
> { "dst": "0.0.0.0/0" }
>   ]
>   }
> },
> "args" : {
>   "org.apache.mesos" : {
> "network_info" : {
>   "port_mappings": {
> "host_port" : 80,
> "container_port" : 80
>   }
> }
>   }
> }
> }
> 
> and the following environment variables:
> export CNI_COMMAND="ADD"
> export CNI_CONTAINERID="00001110"
> export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
> export CNI_IFNAME="eth0"
> export CNI_NETNS="/etc/netns"
> 
> If we remove fields from the above CNI config, or remove certain environment 
> variables the creation of the `PortMapper` correctly fails. However, if 
> config and environment variables are passed as is it will create the 
> `PortMapper` correctly.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Review Request 51447: Killed a line in master/validation.cpp.

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

Killed a line in master/validation.cpp.


Diffs
-

  src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 

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


Testing
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 51405: Updated some tests to use the CreateSlaveFlags() helper.

[Kevin Klues]

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

(Updated Aug. 26, 2016, 1:53 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Updated based on Jie's comments.


Summary (updated)
-

Updated some tests to use the CreateSlaveFlags() helper.


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


Repository: mesos


Description (updated)
---

This change was motivated by the desire to set the new `runtime_dir`
flag properly in these tests.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
2725eb0f76f4e2668381c0d6686a99649f4f9f25 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
c0dda11c18a770f594b9dcf2e506ec624706dbbd 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Kevin Klues]

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

(Updated Aug. 26, 2016, 1:52 a.m.)


Review request for mesos and Jie Yu.


Changes
---

The error handlig for `forwardSignals()` from the last revision caused failures 
because I wasn't filtering out signals properly.


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


Repository: mesos


Description
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--exit_status_path' parameter, which indicates where
the exit status of our command should be checkpointed. In order to
do this checkpointing, however, we cannot simply exec into the command
anymore. Instead we now fork-exec the command and reap it once it
completes. We then checkpoint its exit status and return it as our
own. We call the original process the 'init' process of the container
and the fork-exec'd process its child.  As a side effect of doing
things this way, we also have to be careful to forward all signals
sent to the init process down to the child.

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.hpp 
0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
  src/slave/containerizer/mesos/launch.cpp 
2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51446: Fixed typo in health_checker.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Aug. 26, 2016, 1:33 a.m., Gastón Kleiman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51446/
> ---
> 
> (Updated Aug. 26, 2016, 1:33 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and haosdent huang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed typo in health_checker.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.cpp 
> 45a5fe00a95a6e88b1990c1396e03082feb202bc 
> 
> Diff: https://reviews.apache.org/r/51446/diff/
> 
> 
> Testing
> ---
> 
> make && make check on OS X
> 
> 
> Thanks,
> 
> Gastón Kleiman
> 
>

Review Request 51446: Fixed typo in health_checker.

[Gastón Kleiman]

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

Review request for mesos, Alexander Rukletsov and haosdent huang.


Repository: mesos


Description
---

Fixed typo in health_checker.


Diffs
-

  src/health-check/health_checker.cpp 45a5fe00a95a6e88b1990c1396e03082feb202bc 

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


Testing
---

make && make check on OS X


Thanks,

Gastón Kleiman

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [46187]

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

Error:
2016-08-26 00:58:46 URL:https://reviews.apache.org/r/46187/diff/raw/ 
[2687/2687] -> "46187.patch" [1]
error: src/launcher/http_command_executor.cpp: does not exist in index

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

- Mesos ReviewBot


On Aug. 25, 2016, 8:43 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated Aug. 25, 2016, 8:43 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-5276
> https://issues.apache.org/jira/browse/MESOS-5276
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> 0b4136c040ec9cf585c0d38f8471495a855cd640 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 51421: Added provisioner appc unit test for recovering nested container.

[Jie Yu]

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




src/tests/containerizer/provisioner_appc_tests.cpp (line 504)


Ditto.



src/tests/containerizer/provisioner_appc_tests.cpp (lines 538 - 543)


Hum, I don't think legitimate case. ContainerState will only container top 
level executor containers.



src/tests/containerizer/provisioner_appc_tests.cpp (lines 547 - 548)


The comment here is confusing. I think you want to see if a container can 
have multiple rootfses (for image volumes), right?



src/tests/containerizer/provisioner_appc_tests.cpp (lines 558 - 567)


I don't know if this check is useful. I think the destroy check below 
sounds sufficient to me.


- Jie Yu


On Aug. 25, 2016, 8:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51421/
> ---
> 
> (Updated Aug. 25, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added provisioner appc unit test for recovering nested container.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> b3ba176e506a6d1528290c07a8a0555b12c8cf70 
> 
> Diff: https://reviews.apache.org/r/51421/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51420: Added provisioner appc unit test for provisioning nested container.

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/provisioner_appc_tests.cpp (line 353)


Can you use absolute path:
```
path::join(sandbox.get(), "work_dir");
```

Can you fix the rest in this file?


- Jie Yu


On Aug. 25, 2016, 8:15 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51420/
> ---
> 
> (Updated Aug. 25, 2016, 8:15 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added provisioner appc unit test for provisioning nested container.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> b3ba176e506a6d1528290c07a8a0555b12c8cf70 
> 
> Diff: https://reviews.apache.org/r/51420/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51405: Updated a few more tests to create a temporary 'runtime_dir'.

[Jie Yu]

> On Aug. 26, 2016, 12:31 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 215-218
> > 
> >
> > Can you call CreateSlaveFlags instead?
> 
> Kevin Klues wrote:
> I tried doing this and it gave me the error:
> ```
> ../../src/tests/containerizer/isolator_tests.cpp: In member function 
> ‘virtual void 
> mesos::internal::tests::CpuIsolatorTest_UserCpuUsage_Test::TestBody()’:
> ../../src/tests/containerizer/isolator_tests.cpp:212:41: error: there are 
> no arguments to ‘CreateSlaveFlags’ that depend on a template parameter, so a 
> declaration of ‘CreateSlaveFlags’ must be available [-fpermissive]
>slave::Flags flags = CreateSlaveFlags();
>  ^
> ../../src/tests/containerizer/isolator_tests.cpp:212:41: note: (if you 
> use ‘-fpermissive’, G++ will accept your code, but allowing the use of an 
> undeclared name is deprecated)
> ```

Try `this->CreateSlaveFlags()`


- Jie


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


On Aug. 25, 2016, 11:28 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51405/
> ---
> 
> (Updated Aug. 25, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6050
> https://issues.apache.org/jira/browse/MESOS-6050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a few more tests to create a temporary 'runtime_dir'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 2725eb0f76f4e2668381c0d6686a99649f4f9f25 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> c0dda11c18a770f594b9dcf2e506ec624706dbbd 
> 
> Diff: https://reviews.apache.org/r/51405/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51405: Updated a few more tests to create a temporary 'runtime_dir'.

[Kevin Klues]

> On Aug. 26, 2016, 12:31 a.m., Jie Yu wrote:
> > src/tests/containerizer/isolator_tests.cpp, lines 215-218
> > 
> >
> > Can you call CreateSlaveFlags instead?

I tried doing this and it gave me the error:
```
../../src/tests/containerizer/isolator_tests.cpp: In member function ‘virtual 
void 
mesos::internal::tests::CpuIsolatorTest_UserCpuUsage_Test::TestBody()’:
../../src/tests/containerizer/isolator_tests.cpp:212:41: error: there are no 
arguments to ‘CreateSlaveFlags’ that depend on a template parameter, so a 
declaration of ‘CreateSlaveFlags’ must be available [-fpermissive]
   slave::Flags flags = CreateSlaveFlags();
 ^
../../src/tests/containerizer/isolator_tests.cpp:212:41: note: (if you use 
‘-fpermissive’, G++ will accept your code, but allowing the use of an 
undeclared name is deprecated)
```


- Kevin


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


On Aug. 25, 2016, 11:28 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51405/
> ---
> 
> (Updated Aug. 25, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6050
> https://issues.apache.org/jira/browse/MESOS-6050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a few more tests to create a temporary 'runtime_dir'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 2725eb0f76f4e2668381c0d6686a99649f4f9f25 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> c0dda11c18a770f594b9dcf2e506ec624706dbbd 
> 
> Diff: https://reviews.apache.org/r/51405/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Kevin Klues]

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

(Updated Aug. 26, 2016, 12:34 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Handle error when calling `forwardSignals()`


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


Repository: mesos


Description
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--exit_status_path' parameter, which indicates where
the exit status of our command should be checkpointed. In order to
do this checkpointing, however, we cannot simply exec into the command
anymore. Instead we now fork-exec the command and reap it once it
completes. We then checkpoint its exit status and return it as our
own. We call the original process the 'init' process of the container
and the fork-exec'd process its child.  As a side effect of doing
things this way, we also have to be careful to forward all signals
sent to the init process down to the child.

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.hpp 
0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
  src/slave/containerizer/mesos/launch.cpp 
2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Jie Yu]

> On Aug. 25, 2016, 5:34 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/launch.cpp, line 502
> > 
> >
> > I'd avoid using subprocess here. Using subprocess here means that we'll 
> > initialize libprocess, creating many unnecessary threads. I would simply 
> > fork-exec here. We don't have two worry about async signal safe problem 
> > here because it's single threaded process.
> > 
> > We need to fix the `pre_exec_comands` above for the same problem.

Not resolved? I'd still suggest you use 'fork-exec' as the code shown above. 
You can avoid the subprocess usage in that way.


- Jie


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


On Aug. 25, 2016, 11:32 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51406/
> ---
> 
> (Updated Aug. 25, 2016, 11:32 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the 'mesos-containerizer launch' binary would simply exec
> into the actual command we wanted to launch after doing some set of
> preperatory work. The problem with this approach, however, is that
> this gives us no opportunity to checkpoint the exit status of the
> command so the agent can recover it in cases where it is offline at
> the time the command completes.
> 
> To compensate for this, we now allow 'mesos-containerizer launch' to
> take an optional '--exit_status_path' parameter, which indicates where
> the exit status of our command should be checkpointed. In order to
> do this checkpointing, however, we cannot simply exec into the command
> anymore. Instead we now fork-exec the command and reap it once it
> completes. We then checkpoint its exit status and return it as our
> own. We call the original process the 'init' process of the container
> and the fork-exec'd process its child.  As a side effect of doing
> things this way, we also have to be careful to forward all signals
> sent to the init process down to the child.
> 
> In a subsequent commit we will update the mesos containerizer to use
> this new functionality.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
>   src/slave/containerizer/mesos/launch.cpp 
> 2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
>   src/slave/containerizer/mesos/launcher.cpp 
> 413a8afdc56127a58c9599c436d17d6c98e62434 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 7377316776646e3d660086da3c0d5b494ce8ace4 
> 
> Diff: https://reviews.apache.org/r/51406/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51405: Updated a few more tests to create a temporary 'runtime_dir'.

[Jie Yu]

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




src/tests/containerizer/isolator_tests.cpp (lines 215 - 218)


Can you call CreateSlaveFlags instead?



src/tests/containerizer/mesos_containerizer_tests.cpp (line 142)


I'd suggest we make MesosContainerizerIsolatorPreparationTest inherit from 
MesosTest and use CreateSlaveFlags



src/tests/containerizer/mesos_containerizer_tests.cpp (lines 449 - 460)


Ditto.


- Jie Yu


On Aug. 25, 2016, 11:28 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51405/
> ---
> 
> (Updated Aug. 25, 2016, 11:28 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6050
> https://issues.apache.org/jira/browse/MESOS-6050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated a few more tests to create a temporary 'runtime_dir'.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/isolator_tests.cpp 
> 2725eb0f76f4e2668381c0d6686a99649f4f9f25 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> c0dda11c18a770f594b9dcf2e506ec624706dbbd 
> 
> Diff: https://reviews.apache.org/r/51405/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51096: Added the `mesos-port-mapper` binary.

[Avinash sridharan]

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

(Updated Aug. 26, 2016, 12:29 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Replaced spec::CniError with spec::Error


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


Repository: mesos


Description
---

The `mesos-port-mapper` is a CNI plugin that can be used to install
port-forwarding rules for a container. The `mesos-port-mapper` is a
wrapper CNI plugin that should always be used in conjunction with
other CNI plugins.


Diffs (updated)
-

  src/Makefile.am 120a715eeeb7fb222d1169500950b5c7643df554 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 PRE-CREATION 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51095: Adding protobuf to represent errors returned by CNI plugins.

[Avinash sridharan]

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

(Updated Aug. 26, 2016, 12:26 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description (updated)
---

Adding protobuf to represent errors returned by CNI plugins.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/spec.cpp 
938a9f3bff2a95c8591450f0edafaddb01833e59 
  src/slave/containerizer/mesos/isolators/network/cni/spec.proto 
8419bc11d14aa29110377248a92ebb1aa4baf486 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 51442: Added missing Launcher mock for getExitStatusCheckpointPath.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 25, 2016, 11:35 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51442/
> ---
> 
> (Updated Aug. 25, 2016, 11:35 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6051
> https://issues.apache.org/jira/browse/MESOS-6051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing Launcher mock for getExitStatusCheckpointPath.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/launcher.cpp 
> a92d9890f0931425d69ef8ce0896d081b8722079 
> 
> Diff: https://reviews.apache.org/r/51442/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51434: Use `continue` when breaking out of LAUNCH_GROUP case due to errors.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51434]

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

- Mesos ReviewBot


On Aug. 25, 2016, 5:58 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51434/
> ---
> 
> (Updated Aug. 25, 2016, 5:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the convention established by the rest of the switch cases in
> `_accept()` and it will allow common code to be put after the switch
> for each *valid* operation.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
> 
> Diff: https://reviews.apache.org/r/51434/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51217: Added `os::execvp' in `stout` library.

[Daniel Pravat]

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

(Updated Aug. 25, 2016, 11:59 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Summary (updated)
-

Added `os::execvp' in `stout` library.


Repository: mesos


Description (updated)
---

Added `os::execvp' in `stout` library.


Diffs (updated)
-

  3rdparty/stout/include/stout/os/posix/shell.hpp 
1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
  3rdparty/stout/include/stout/os/windows/shell.hpp 
c6e141aba0abe2c7fe5410e867f7db47d632e765 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 51213: Used `os::execvp` instead of `::execvp`.

[Daniel Pravat]

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

(Updated Aug. 25, 2016, 11:59 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Summary (updated)
-

Used `os::execvp` instead of `::execvp`.


Repository: mesos


Description (updated)
---

Used `os::execvp` instead of `::execvp`.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.cpp 
2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 51217: Added `os::execlp' in `stout` library.

[Daniel Pravat]

> On Aug. 22, 2016, 6:39 p.m., Joseph Wu wrote:
> > Note: Your summary/description says `execlp`, but the code is for `execvp` 
> > :P

I will change the summary.


- Daniel


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


On Aug. 19, 2016, 5:38 p.m., Daniel Pravat wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51217/
> ---
> 
> (Updated Aug. 19, 2016, 5:38 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, 
> Joseph Wu, and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added `os::execlp' in `stout` library.
> 
> 
> Diffs
> -
> 
>   3rdparty/stout/include/stout/os/posix/shell.hpp 
> 1d73ae5fa7182ba433eaee0cc0034af6b36d77db 
>   3rdparty/stout/include/stout/os/windows/shell.hpp 
> c6e141aba0abe2c7fe5410e867f7db47d632e765 
> 
> Diff: https://reviews.apache.org/r/51217/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Daniel Pravat
> 
>

Re: Review Request 51210: Update mesos-executor name for Windows.

[Daniel Pravat]

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

(Updated Aug. 25, 2016, 11:50 p.m.)


Review request for mesos, Alex Naparu, Artem Harutyunyan, Alex Clemmer, Joseph 
Wu, and Michael Park.


Repository: mesos


Description
---

Update mesos-executor name for Windows.


Diffs (updated)
-

  src/slave/slave.cpp 5d162d00a8a9a0e318c39bda93dd584f23c87987 

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


Testing
---


Thanks,

Daniel Pravat

Re: Review Request 51437: Added validation test for task group task using NetworkInfos.

[Guangya Liu]

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




src/tests/master_validation_tests.cpp (line 1811)


Here also needs an update if you quota `NetworkInfos`.


- Guangya Liu


On 八月 25, 2016, 7:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51437/
> ---
> 
> (Updated 八月 25, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> NetworkInfos can only be set on the task group executor.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 
>   src/tests/master_validation_tests.cpp 
> 56e931c348ee7b4923e16ab8664e2b0d93bce8fc 
> 
> Diff: https://reviews.apache.org/r/51437/diff/
> 
> 
> Testing
> ---
> 
> make check -j20 GTEST_FILTER="*Validation*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 51442: Added missing Launcher mock for getExitStatusCheckpointPath.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added missing Launcher mock for getExitStatusCheckpointPath.


Diffs
-

  src/tests/containerizer/launcher.cpp a92d9890f0931425d69ef8ce0896d081b8722079 

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


Testing
---

make check


Thanks,

Kevin Klues

Re: Review Request 51407: Updated mesos containerizer to checkpoint the container exit status.

[Kevin Klues]

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

(Updated Aug. 25, 2016, 11:34 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Merged test changes into this patch to make the commit *atomic*


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


Repository: mesos


Description (updated)
---

This includes an updated to tests to account for new 'init' process
semantics in a container. That is, the init process of the new
container is now "mesos-containerizer" not "sh".


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
  src/tests/containerizer/isolator_tests.cpp 
2725eb0f76f4e2668381c0d6686a99649f4f9f25 

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


Testing
---


Thanks,

Kevin Klues

Review Request 51443: Updated 'launcher' to properly reap checkpointed exit status.

[Kevin Klues]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

In order to properly reap the new container 'init' process, we need
introduce a new 'Launcher::wait' command that knows how to reap the
'init' process directly and then retreive its exit status from the
checkpoint (if necessary).

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs
-

  src/slave/containerizer/mesos/launcher.hpp 
0eae09515d550a2c71ae1414d4a22943f1d09db9 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.hpp 
8fbe1e9742df85b0fc6de46ac81a0c064c845a63 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 
  src/tests/containerizer/launcher.hpp 94c62b761a17436841bd19f3bf622cc8f1047876 
  src/tests/containerizer/launcher.cpp a92d9890f0931425d69ef8ce0896d081b8722079 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Kevin Klues]

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

(Updated Aug. 25, 2016, 11:32 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Addressed all of Jie's comments except "I'd avoid using subprocess here." That 
should come in a seperate commit and already has a JIRA for it: `MESOS-6075`


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


Repository: mesos


Description (updated)
---

Previously the 'mesos-containerizer launch' binary would simply exec
into the actual command we wanted to launch after doing some set of
preperatory work. The problem with this approach, however, is that
this gives us no opportunity to checkpoint the exit status of the
command so the agent can recover it in cases where it is offline at
the time the command completes.

To compensate for this, we now allow 'mesos-containerizer launch' to
take an optional '--exit_status_path' parameter, which indicates where
the exit status of our command should be checkpointed. In order to
do this checkpointing, however, we cannot simply exec into the command
anymore. Instead we now fork-exec the command and reap it once it
completes. We then checkpoint its exit status and return it as our
own. We call the original process the 'init' process of the container
and the fork-exec'd process its child.  As a side effect of doing
things this way, we also have to be careful to forward all signals
sent to the init process down to the child.

In a subsequent commit we will update the mesos containerizer to use
this new functionality.


Diffs (updated)
-

  src/slave/containerizer/mesos/launch.hpp 
0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
  src/slave/containerizer/mesos/launch.cpp 
2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
  src/slave/containerizer/mesos/launcher.cpp 
413a8afdc56127a58c9599c436d17d6c98e62434 
  src/slave/containerizer/mesos/linux_launcher.cpp 
7377316776646e3d660086da3c0d5b494ce8ace4 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 51437: Added validation test for task group task using NetworkInfos.

[Guangya Liu]

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


Fix it, then Ship it!





src/master/validation.cpp (line 1003)


How about

```
return Error("'NetworkInfos' must not be set on the task");
```



src/tests/master_validation_tests.cpp (line 1776)


What about define a variable first and use this `resource` in both executor 
and task (#1784)?


- Guangya Liu


On 八月 25, 2016, 7:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51437/
> ---
> 
> (Updated 八月 25, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> NetworkInfos can only be set on the task group executor.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 
>   src/tests/master_validation_tests.cpp 
> 56e931c348ee7b4923e16ab8664e2b0d93bce8fc 
> 
> Diff: https://reviews.apache.org/r/51437/diff/
> 
> 
> Testing
> ---
> 
> make check -j20 GTEST_FILTER="*Validation*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 51405: Updated a few more tests to create a temporary 'runtime_dir'.

[Kevin Klues]

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

(Updated Aug. 25, 2016, 11:28 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Pulled in a few more tests that were failing once I added the `mkdir` for the 
checkpoint dir in the posix launcher.


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


Repository: mesos


Description
---

Updated a few more tests to create a temporary 'runtime_dir'.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
2725eb0f76f4e2668381c0d6686a99649f4f9f25 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
c0dda11c18a770f594b9dcf2e506ec624706dbbd 

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


Testing
---


Thanks,

Kevin Klues

Re: Review Request 50868: Using `OfferedResources` for benchmark test of `SuppressOffers`.

[Jiang Yan Xu]

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



I don't know. Making such distinction feels like splitting hairs to me. Yeah 
there are subtle differences between the two concepts but to me:

- `offerCallback` may not be the best name what we could have come up with even 
though in `Master::offer()` we do construct offers.
- When offer objects are constructed, we call the resources in them "offered 
resources". 
- When we parse resources from offer objects, we call them "offered resources".
- In this test, namely, the "Allocator" tests, we are indeed only allocating 
resources. (I don't see "Offer" objects being created).

Later if the allocator actually hands out offer objects, then we probably need 
to change the tests to use offers directly but right now I think 1) 
"allocation" is most correct, 2) it's not a big deal if people treat them as 
synonymous terms. 

That said, I think reusing the "Allocation" defined at the top of the test 
makes sense.

- Jiang Yan Xu


On Aug. 5, 2016, 2:50 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50868/
> ---
> 
> (Updated Aug. 5, 2016, 2:50 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current benchmark test of `SuppressOffers` is using `Allocation`
> to specify the resources from an `OfferCallback`, but here seems
> using `OfferedResources` maybe better, as here we are actually
> constructing the offers on each agent, but the `Allocation` is an
> allocator concept and it means the resources allocated for a specified
> framework.
> 
> 
> Diffs
> -
> 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
> 
> Diff: https://reviews.apache.org/r/50868/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh --benchmark  
> --gtest_filter="*HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test
> [ RUN  ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0
> Using 1000 agents and 1 frameworks
> Added 1 frameworks in 841us
> Added 1000 agents in 480991us
> allocate() took 264196us to make 1000 offers with 0 out of 1 frameworks 
> suppressing offers
> allocate() took 270110us to make 1000 offers with 0 out of 1 frameworks 
> suppressing offers
> allocate() took 270098us to make 1000 offers with 0 out of 1 frameworks 
> suppressing offers
> allocate() took 264314us to make 1000 offers with 0 out of 1 frameworks 
> suppressing offers
> allocate() took 268732us to make 1000 offers with 0 out of 1 frameworks 
> suppressing offers
> [   OK ] 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test.SuppressOffers/0 
> (4511 ms)
> [--] 1 test from 
> SlaveAndFrameworkCount/HierarchicalAllocator_BENCHMARK_Test (4511 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (4522 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 46822: Avoided slicing of flags in subprocess.

[Michael Park]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (line 1423)


```
add(, "hostname", "Hostname of the container");
```


- Michael Park


On Aug. 25, 2016, 5:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46822/
> ---
> 
> (Updated Aug. 25, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While `FlagsBase` internally stores just maps of names and
> flags, the functions stored in a `Flag` rely on the original type of
> the `Flags` containing them (e.g., we perform dynamic casts to detect
> their types).
> 
> Since `Option` stores `T` as a value (i.e., it cannot contain
> reference types) any interface taking an `Option` cannot rely on
> polymorphic behavior of `T`. To make use of polymorphism we should
> instead store e.g., a pointer type to avoid slicing.
> 
> Here we change `Flags` arguments of `subprocess` to allow preserving
> the original type so `Flag` functions can work reliably; we do this by
> passing a non-owning pointer to a `Flags` so we do not restrict copying.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp e07c4aa723f57db7f8bc31d4af5d32a30ebf98f2 
>   src/health-check/health_checker.cpp 
> d43cb0568c120cbcec6a73d232396ccc54cf3e58 
>   src/launcher/posix/executor.cpp 43573cacee4e681d4327a7ed7c43b4ee263aa175 
>   src/linux/perf.cpp 9455210064779b59ad56637d846fe7584b21340f 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1fca4864457388b265779fbca72296336f1aa5a9 
>   src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 86a8d8a85b6a33c87798108cb65af85bb9bbbc77 
>   src/slave/containerizer/mesos/isolators/docker/volume/driver.cpp 
> 842f2b5d4037e076cac4fd9c2eeb8f69786cffa7 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 760d32bf3dc09f3b715b378f5ded41556f15fe41 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 92f3c07e285ad3b8ef26692aa6475d755188b469 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> f97ace9579879b35cb8050c56e968a31bb741c55 
>   src/slave/containerizer/mesos/launcher.hpp 
> bf435e3a9c150648336a1becf2f075fa183428bd 
>   src/slave/containerizer/mesos/launcher.cpp 
> 9efe8474a2210957ce256fc08cb35694194213c3 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> c1852226c74bc611d045be721e284141e59adcd9 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 95dee95c5e6e613e526c92d8729ae5583c8b58f1 
>   src/tests/containerizer/isolator_tests.cpp 
> 4f047ae6b2e85e177e8b73d60b9dfca913c832a5 
>   src/tests/containerizer/launch_tests.cpp 
> ef0c87c96e6a8379d119246a8ad044248522e67e 
>   src/tests/containerizer/launcher.hpp 
> 7e5c243efad11d04e70b36876b2ed4db82666d31 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> e3c8daab4fc688150a4f222e05f9b1bd9aee1912 
>   src/tests/containerizer/ns_tests.cpp 
> cd668ebb3b9461bee00dc338c288e5df6eb8fe31 
>   src/tests/containerizer/port_mapping_tests.cpp 
> fd181cae5540de1fdd631367aba5cce249f1b72c 
> 
> Diff: https://reviews.apache.org/r/46822/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 46821: Avoided slicing of flags in subprocess in libprocess and stout.

[Michael Park]

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


Fix it, then Ship it!





3rdparty/libprocess/src/subprocess.cpp (line 149)


`if (flags != nullptr) {`


- Michael Park


On Aug. 25, 2016, 5:53 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46821/
> ---
> 
> (Updated Aug. 25, 2016, 5:53 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Joris Van Remoortere, and 
> Michael Park.
> 
> 
> Bugs: MESOS-3335
> https://issues.apache.org/jira/browse/MESOS-3335
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While `FlagsBase` internally stores just maps of names and
> flags, the functions stored in a `Flag` rely on the original type of
> the `Flags` containing them (e.g., we perform dynamic casts to detect
> their types).
> 
> Since `Option` stores `T` as a value (i.e., it cannot contain
> reference types) any interface taking an `Option` cannot rely on
> polymorphic behavior of `T`. To make use of polymorphism we should
> instead store e.g., a pointer type to avoid slicing.
> 
> Here we change `Flags` arguments of `subprocess` to allow preserving
> the original type so `Flag` functions can work reliably; we do this by
> passing a non-owning pointer to a `Flags` so we do not restrict copying.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 83778bbfcc75f3f39b98d03a3580398e7d1062ea 
>   3rdparty/libprocess/include/process/subprocess_base.hpp 
> e0c6b2d930fbd2cfe011e6faf44843b83ab1db27 
>   3rdparty/libprocess/src/subprocess.cpp 
> 44073146118b6c6e9e730b8c901852594080a3eb 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 3f3e21514bd5e2e388165eb64d540764097557ac 
> 
> Diff: https://reviews.apache.org/r/46821/diff/
> 
> 
> Testing
> ---
> 
> Tested on various platforms in internal CI.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51095: Adding protobuf to represent errors returned by CNI plugins.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 71)


s/Cni//

`cni` is already part of the namespace prefix



src/slave/containerizer/mesos/isolators/network/cni/spec.proto (line 72)


`s/cni_version/cniVersion`

looking at CNI spec:
```
{
  "cniVersion": "0.2.0",
  "code": ,
  "msg": ,
  "details":  (optional)
}
```


- Jie Yu


On Aug. 15, 2016, 5:24 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51095/
> ---
> 
> (Updated Aug. 15, 2016, 5:24 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6023
> https://issues.apache.org/jira/browse/MESOS-6023
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adding protobuf to represent errors returned by CNI plugins.
> 
> This will be used by CNI plugins such as `PortMapper` to report errors to the 
> `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto 
> 8419bc11d14aa29110377248a92ebb1aa4baf486 
> 
> Diff: https://reviews.apache.org/r/51095/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 51437: Added validation test for task group task using NetworkInfos.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51437]

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

- Mesos ReviewBot


On Aug. 25, 2016, 7:34 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51437/
> ---
> 
> (Updated Aug. 25, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> NetworkInfos can only be set on the task group executor.
> 
> 
> Diffs
> -
> 
>   src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 
>   src/tests/master_validation_tests.cpp 
> 56e931c348ee7b4923e16ab8664e2b0d93bce8fc 
> 
> Diff: https://reviews.apache.org/r/51437/diff/
> 
> 
> Testing
> ---
> 
> make check -j20 GTEST_FILTER="*Validation*"
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 51097: Added a `PortMapper` class.

[Avinash sridharan]

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

(Updated Aug. 25, 2016, 10:04 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

RB patch update.


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


Repository: mesos


Description
---

This class will embody the logic for implementing the CNI port-mapper
plugin.


Diffs (updated)
-

  src/Makefile.am d389e02584cfb1a00067cd30de1364118d1c46c7 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.hpp
 PRE-CREATION 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/port_mapper.cpp
 PRE-CREATION 

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


Testing
---

Tested the port-mapper with the following CNI config:
{
"name": "mynet",
"type": "port-mapper",
"chain": "MESOS",
"delegate": {
  "type" : "bridge",
  "bridge": "cni0",
  "isGateway": true,
  "ipMasq": true,
  "ipam": {
  "type": "host-local",
  "subnet": "10.22.0.0/16",
  "routes": [
{ "dst": "0.0.0.0/0" }
  ]
  }
},
"args" : {
  "org.apache.mesos" : {
"network_info" : {
  "port_mappings": {
"host_port" : 80,
"container_port" : 80
  }
}
  }
}
}

and the following environment variables:
export CNI_COMMAND="ADD"
export CNI_CONTAINERID="00001110"
export CNI_PATH="$MESOS_INSTALL:/home/vagrant/dev/go/cni/bin"
export CNI_IFNAME="eth0"
export CNI_NETNS="/etc/netns"

If we remove fields from the above CNI config, or remove certain environment 
variables the creation of the `PortMapper` correctly fails. However, if config 
and environment variables are passed as is it will create the `PortMapper` 
correctly.


Thanks,

Avinash sridharan

Re: Review Request 50523: Updated docker recovery to account for GPU resources.

[Kevin Klues]

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




src/slave/containerizer/docker.cpp (lines 1028 - 1034)


I feel like failing to recover devices warrants more than just a LOG 
message.

Consider the situation where we fail to recover them, and we just continue. 

From Mesos's perspective, these devices are now free to hand out to other 
containers.  This will cause conflicts -- especially in the case of GPUs.

We need to think through the failure scenario here a bit more.



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


I don't think you need to save a temporary variable here. Just return the 
call to `docker->inspect` directly, i.e.:

```
return docker->inspect(containerName, slave::DOCKER_INSPECT_DELAY)
  .then(defer(self(), [=](const Docker::Container& container) {
...
  }));
```



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


We typically write out full words for our variable names.

I'd either change this to `nvidiaDevicePrefix` or just simply `prefix.



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


I probably wouldn't use an `Option` here, but rather just an (initially) 
empty set.



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


There should be a line break here.



src/slave/containerizer/docker.cpp (lines 1117 - 1120)


Indentation is wrong here.



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


You should use `strings::remove(deviceString, prefix)` here.



src/slave/containerizer/docker.cpp (lines 1123 - 1125)


You should use `numify()` with error checking here.



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


We should (at a minimum) LOG an error here.

My preference though, would be to return an error at all error points in 
this function and then do the logging in the caller.



src/slave/containerizer/docker.cpp (lines 1137 - 1138)


If you follow the suggesstion above, then this code is unnecessary.



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


If you follow the suggesstion above, then this would turn into:

```
if (!gpus.empty()) {
```



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


With all of the other comments above being addressed, this line should 
become:

```
return nvidiaGpuAllocator->allocate(gpus);
```



src/tests/containerizer/docker_containerizer_tests.cpp (line 3980)


You don't need this (in fact, you shouldn't have this) for the docker 
containerizer. These flags are only relevant for the mesos containerizer.

Instead you should have:
`flags.containerizers = docker`.



src/tests/containerizer/docker_containerizer_tests.cpp (line 4105)


It looks like you wait for recovery to complete, but then you don't 
actually verify that the recovery was successful.

I think after recovery you should verify that the the container was 
actually recovered properly -- devices an all.


- Kevin Klues


On Aug. 24, 2016, 12:56 a.m., Rajat Phull wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50523/
> ---
> 
> (Updated Aug. 24, 2016, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, Yubo Li, and Vikrama 
> Ditya.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker recovery to account for GPU resources.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 42d4364b6fcbc94c7852721511001c103cb5a90d 
> 
> Diff: https://reviews.apache.org/r/50523/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_LaunchWithGpuRecovery"
>  make -j check
> 
> 
> Thanks,
> 
> Rajat Phull
> 
>

Re: Review Request 50523: Updated docker recovery to account for GPU resources.

[Kevin Klues]

> On Aug. 21, 2016, 3 p.m., Guangya Liu wrote:
> > src/slave/containerizer/docker.cpp, line 1055
> > 
> >
> > We should use a `continuation` here, you can refer to 
> > https://reviews.apache.org/r/50841/diff/5#index_header to get some hints.

At first glance, it looks like this line is blocking because of the `.get()`. 
However, the line is just confusing because both `allocator` and `gpus` are of 
type `Option`.  While there are still problems with the line (i.e. it's return 
value is ignored!), it does not seem to block anywhere. See my comments later 
on for how to address this.


- Kevin


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


On Aug. 24, 2016, 12:56 a.m., Rajat Phull wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50523/
> ---
> 
> (Updated Aug. 24, 2016, 12:56 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, Yubo Li, and Vikrama 
> Ditya.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker recovery to account for GPU resources.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 42d4364b6fcbc94c7852721511001c103cb5a90d 
> 
> Diff: https://reviews.apache.org/r/50523/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_LaunchWithGpuRecovery"
>  make -j check
> 
> 
> Thanks,
> 
> Rajat Phull
> 
>

Re: Review Request 51374: Change registry update order on removal, mark-unreachable.

[Neil Conway]

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

(Updated Aug. 25, 2016, 9 p.m.)


Review request for mesos and Vinod Kone.


Changes
---

Fix thinko in commit message.


Summary (updated)
-

Change registry update order on removal, mark-unreachable.


Bugs: MESOS-4049 and MESOS-6090
https://issues.apache.org/jira/browse/MESOS-4049
https://issues.apache.org/jira/browse/MESOS-6090


Repository: mesos


Description (updated)
---

This commit changes the master so that we always update the registry
for an important change (e.g., removing an agent or marking an agent
unreachable), before updating the important parts of the master's
in-memory state (e.g., the in-memory list of registered agents).
Previously, the registry was updated first for registration and
reregistration, but second for removal and marking unreachable.
Updating the registry first is simpler, and also avoids possibly
leaking inaccurate information (e.g., via HTTP endpoints) if the
registry operation fails.


Diffs (updated)
-

  src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
  src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
  src/tests/master_tests.cpp 398164d09b8ef14f916122ed8780023c4a3cd0f6 
  src/tests/partition_tests.cpp 0a72b345538ca3b9510fccf38ceb68ac71c2b473 
  src/tests/slave_tests.cpp 84ee37cae97004914b3f0a060dc854e531737dce 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 51009: Collect throttle related cpu.stat for Docker Containerizer.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51009]

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

- Mesos ReviewBot


On Aug. 22, 2016, 7:15 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51009/
> ---
> 
> (Updated Aug. 22, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, Jie Yu, and Steve 
> Niemitz.
> 
> 
> Bugs: MESOS-6031
> https://issues.apache.org/jira/browse/MESOS-6031
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Collect throttle related cpu.stat for Docker Containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp e447c58bd46ba080529e8f349347eccf5a54110a 
> 
> Diff: https://reviews.apache.org/r/51009/diff/
> 
> 
> Testing
> ---
> 
> Run agent with cfs quota enabled, and observe that throttle related metrics 
> are in `/containers` and `/monitoring/statistics`
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 50661: Documentation: fix broken links.

[Vinod Kone]

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



Pierre, still working on this? If yes, can you please address issues raised by 
haosdent? If not, please discard the review.

- Vinod Kone


On Aug. 1, 2016, 8:09 p.m., Pierre Cheynier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50661/
> ---
> 
> (Updated Aug. 1, 2016, 8:09 p.m.)
> 
> 
> Review request for mesos, Dave Lester, haosdent huang, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> * Remove references to github.com (contains /blob/master..)
> * site Rakefile now handle links contained in HTML table
> 
> The link to github.com in `submitting-a-patch.md` was let on purpose
> as it seems that some users submit PRs by directly editting the file.
> 
> 
> Diffs
> -
> 
>   docs/authorization.md 5456f1be5ff1c7abbc7efd4f6e4130f49b1d6f90 
>   docs/docker-volume.md 2dbd0441a924c9360c1d6432bcc36febe925268b 
>   docs/working-groups.md e9e1076ae809d4a7e198d7eb7a612691912f20c3 
>   site/Rakefile 39aeb3cfec2a419000fb8c9625b79683eff92bdc 
> 
> Diff: https://reviews.apache.org/r/50661/diff/
> 
> 
> Testing
> ---
> 
> Check that doc links still work both in github.com and on the website by:
> * Using the `mesos/website` container
> * Using a Markdown editor
> 
> 
> Thanks,
> 
> Pierre Cheynier
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Qian Zhang]

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

(Updated Aug. 25, 2016, 8:43 p.m.)


Review request for mesos, Anand Mazumdar and Vinod Kone.


Changes
---

fixed bug id -- @vinodkone


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


Repository: mesos


Description
---

Terminate when receiving the ACK of terminal status update.


Diffs
-

  src/launcher/http_command_executor.cpp 
0b4136c040ec9cf585c0d38f8471495a855cd640 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 51434: Use `continue` when breaking out of LAUNCH_GROUP case due to errors.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On Aug. 25, 2016, 5:58 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51434/
> ---
> 
> (Updated Aug. 25, 2016, 5:58 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the convention established by the rest of the switch cases in
> `_accept()` and it will allow common code to be put after the switch
> for each *valid* operation.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
> 
> Diff: https://reviews.apache.org/r/51434/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 51433: Added test case MasterAuthorizationTest, UnauthorizedTaskGroup.

[Vinod Kone]

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


Fix it, then Ship it!




I'll fix the issues while committing.


src/tests/master_authorization_tests.cpp (line 235)


// This test verifies that if one of the tasks in a task group is 
unauthorized, all the tasks in
// the task group are rejected.



src/tests/master_authorization_tests.cpp (line 292)


// Create an authorized task.


- Vinod Kone


On Aug. 25, 2016, 6:19 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51433/
> ---
> 
> (Updated Aug. 25, 2016, 6:19 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6087
> https://issues.apache.org/jira/browse/MESOS-6087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case MasterAuthorizationTest, UnauthorizedTaskGroup.
> 
> 
> Diffs
> -
> 
>   src/tests/master_authorization_tests.cpp 
> 92bd2a9463f861e4b479f76927dd01324765a411 
> 
> Diff: https://reviews.apache.org/r/51433/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  
> --gtest_filter="MasterAuthorizationTest.UnauthorizedTaskGroup"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from MasterAuthorizationTest
> [ RUN  ] MasterAuthorizationTest.UnauthorizedTaskGroup
> [   OK ] MasterAuthorizationTest.UnauthorizedTaskGroup (85 ms)
> [--] 1 test from MasterAuthorizationTest (86 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (105 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51412: Add offeredResources to Allocator::updateAllocation() API.

[Jiang Yan Xu]

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


Fix it, then Ship it!




> This is being added to allow for handling of ACCEPT call within the
allocator in the future. For now, we expose the offered resources
in this API to account for additional copies of shared resources in
the allocator when there are more tasks being launched in a single
ACCEPT call for a single copy of shared resource being offered.

I feel the summary too specific and it sounds like we are changing a generic 
API for only one purpose. One of the main reasons we chose to add this is 
because it can be used for more than shared resources. So how about this as the 
summary:

```
Operations are applied to specific offered resources which are a subset of all 
resources allocated to the framework. 
For RESERVE/UNRESERVE/CREATE/DESTROY it's not necessary to make such 
distinction but we need it for future work such as supporting shared resoruces.
```


include/mesos/allocator/allocator.hpp (lines 255 - 257)


> The offeredResources is passed to this function to allow the allocator 
handle ACCEPT calls.

The same to true for every other argument?

I think it suffices to simply state:

```
The `offeredResources` are the resources that these operations are applied 
to.
```



include/mesos/allocator/allocator.hpp (lines 262 - 263)


Can we swap the order of the two arguments?

Offered resources (or offer ID) go before the list of operations in the 
protobuf `Accept` and master method `_accept()`. It's probably more important 
when the method is changed to `Allocator::accept()` but if we do it now we 
don't need to swap them later.



src/master/allocator/mesos/hierarchical.cpp (lines 616 - 617)


It's not clear to me what "handle ACCEPT related operations" implies. If we 
want to use a TODO to justify the `offeredResources` I think we can just say:

```
// TODO(anindya_sinha): `offeredResources` will be useful for supporting 
LAUNCH operations involving shared resources.
```



src/master/allocator/mesos/hierarchical.cpp (line 617)


> ACCEPT

You meant LAUNCH?



src/master/master.hpp 


Move the comments to `_apply()`?

```
  // Updates the slave's resources by
  // applying the given operation. It also sends a
  // 'CheckpointResourcesMessage' to the slave with the updated
  // checkpointed resources.
```

I think it's worth documenting `_apply()` because of the "side effect": 
sending CheckpointResourcesMessage.



src/master/master.cpp (line 3556)


Add the following?

```
// Note that this list could be different than `accept.operations()` as we 
drop invalid operations. However the order should remain unchanged.
```



src/master/master.cpp (line 4095)


s/the operations/the valid operations/


- Jiang Yan Xu


On Aug. 25, 2016, 8:51 a.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51412/
> ---
> 
> (Updated Aug. 25, 2016, 8:51 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4431
> https://issues.apache.org/jira/browse/MESOS-4431
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is being added to allow for handling of ACCEPT call within the
> allocator in the future. For now, we expose the offered resources
> in this API to account for additional copies of shared resources in
> the allocator when there are more tasks being launched in a single
> ACCEPT call for a single copy of shared resource being offered.
> 
> 
> Diffs
> -
> 
>   include/mesos/allocator/allocator.hpp 
> 41a9d457286e30431490ca626e680d85684b48d6 
>   src/master/allocator/mesos/allocator.hpp 
> 69639be9c14b83157df29f767e819db719588053 
>   src/master/allocator/mesos/hierarchical.hpp 
> bdbc6d3b5b959990538f4e3b7b1a3b031d9aea05 
>   src/master/allocator/mesos/hierarchical.cpp 
> 234ef98529964a0b6d3f132426a6c8ccbb1263ee 
>   src/master/master.hpp 6decff6f4b9c3434de030fd5c06df4c683a7abad 
>   src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 
>   src/tests/allocator.hpp 5ca3057265eb2c00c965132ac7922019e1cc77b8 
>   src/tests/hierarchical_allocator_tests.cpp 
> cbed333f497016fe2811f755028796012b41db77 
>   

Re: Review Request 51043: Added support for reading out the bounding capability set.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 25, 2016, 3:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51043/
> ---
> 
> (Updated Aug. 25, 2016, 3:18 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 
> 
> Diff: https://reviews.apache.org/r/51043/diff/
> 
> 
> Testing
> ---
> 
> make check and sudo make check (Debian jessie, gcc-4.9.2, w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51428: Removed friendship between ProcessCapabilities and Capabilities.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On Aug. 25, 2016, 3:17 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51428/
> ---
> 
> (Updated Aug. 25, 2016, 3:17 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.hpp fd7a571a62e3bbdd8a0453f800bf622457891bf8 
>   src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 
> 
> Diff: https://reviews.apache.org/r/51428/diff/
> 
> 
> Testing
> ---
> 
> Tested with the full chain leading up to r/50271.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51402: Added nested container check in provisioner destroy.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 429)


i think there is a third case in the normal destroy path, right?



src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 441)


no need for the extra `()`



src/slave/containerizer/mesos/provisioner/provisioner.cpp (lines 442 - 445)


I would return a Failure here instead.


- Jie Yu


On Aug. 25, 2016, 12:52 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51402/
> ---
> 
> (Updated Aug. 25, 2016, 12:52 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added nested container check in provisioner destroy.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51402/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 51437: Added validation test for task group task using NetworkInfos.

[Vinod Kone]

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

Review request for mesos, Anand Mazumdar and Guangya Liu.


Repository: mesos


Description
---

NetworkInfos can only be set on the task group executor.


Diffs
-

  src/master/validation.cpp 3d2965e4cfaae075f988fd43386bd00c7d807b2e 
  src/tests/master_validation_tests.cpp 
56e931c348ee7b4923e16ab8664e2b0d93bce8fc 

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


Testing
---

make check -j20 GTEST_FILTER="*Validation*"


Thanks,

Vinod Kone

Re: Review Request 50127: Added DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch.

[Kevin Klues]

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




src/tests/containerizer/docker_containerizer_tests.cpp (lines 3893 - 3894)


I don't understand this comment. Can you clarify? Why do we want to make 
sure we are not running in privileged mode?



src/tests/containerizer/docker_containerizer_tests.cpp (lines 3897 - 3912)


Why do you do this twice?



src/tests/containerizer/docker_containerizer_tests.cpp (lines 3914 - 3917)


What does this have to do with the GPU test? Do we have to check this here?


- Kevin Klues


On Aug. 22, 2016, 10:13 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50127/
> ---
> 
> (Updated Aug. 22, 2016, 10:13 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This added a testing case for end-to-end GPU support for docker
> containerizer.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 42d4364b6fcbc94c7852721511001c103cb5a90d 
> 
> Diff: https://reviews.apache.org/r/50127/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="DockerContainerizerTest.ROOT_NVIDIA_GPU_DOCKER_Launch" make -j 
> check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 51393: Added unit test for provisioner recursive listContainers().

[Jie Yu]

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


Fix it, then Ship it!





src/tests/containerizer/provisioner_paths_tests.cpp (lines 90 - 94)


I would add some comments about the id:
```
ContainerID child1;  // parent1/child1
ContainerID child2;  // parent1/child2
ContainerID child3;  // parent2/child3
ContainerID parent1; // parent1
ContainerID parent2; // parent2
```

Adjust below accordingly.



src/tests/containerizer/provisioner_paths_tests.cpp (lines 109 - 114)


```
ASSERT_SOME(os::mdir(containerDir1));
```

No need for the temp variable.


- Jie Yu


On Aug. 24, 2016, 9:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51393/
> ---
> 
> (Updated Aug. 24, 2016, 9:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added unit test for provisioner recursive listContainers().
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_paths_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/51393/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51392: Supported provisioner listContainers() to be recursive.

[Gilbert Song]

> On Aug. 25, 2016, 12:04 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/paths.hpp, line 83
> > 
> >
> > What does 'parentId' mean here? I'd suggest we don't expose that 
> > parameter to the caller. To do proper recursion, you can introduce a 
> > inlined lambda which takes this extra parameter.
> > 
> > ```
> > Try listContainers(
> > const string& provisionerDir)
> > {
> >   auto helper = [](
> >   const string& containersDir,
> >   const Option& parentContainerId) {
> > hashset results;
> > 
> > ...
> >   }
> >   
> >   return helper(getContainersDir(provisionerDir), None());
> > }
> > ```

yes, I can do that.


- Gilbert


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


On Aug. 24, 2016, 2:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51392/
> ---
> 
> (Updated Aug. 24, 2016, 2:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch supports collecting all containerIds (all containers in the
> nested hierarchy) recursively.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 9829d6b52c8547ae22297a5bc47852ce5a219e4c 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 
> 
> Diff: https://reviews.apache.org/r/51392/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 50947: Removed isolator flag 'gpu/nvidia' for docker containerizer using GPU.

[Kevin Klues]

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




src/slave/containerizer/mesos/isolators/gpu/allocator.cpp (lines 146 - 149)


This comment is confusing. I'd probably just leave it as it was, but say 
that this is only required for the mesos containerizer, i.e.:

`` Don't allow the `--nvidia_gpu_devices` flag without the GPU isolator 
when using the mesos containerizer. For the docker containerizers, this is not 
required.```


- Kevin Klues


On Aug. 15, 2016, 7:29 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50947/
> ---
> 
> (Updated Aug. 15, 2016, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5975
> https://issues.apache.org/jira/browse/MESOS-5975
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Mesos containerizer uses isolator 'gpu/nvidia' for GPU isolation while
> docker and external containerizers do not need it anymore. This removed
> the 'gpu/nvidia' isolator check for docker and external containerizers.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/gpu/allocator.cpp 
> c1a87e9e5c07529bc1d077f68477108a40506806 
> 
> Diff: https://reviews.apache.org/r/50947/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 51257: Add external process container logger.

[Joseph Wu]

> On Aug. 22, 2016, 11:03 a.m., Joseph Wu wrote:
> > src/CMakeLists.txt, line 171
> > 
> >
> > Can you un-remove all these un-related `cpuacct` changes?
> 
> Will Rouesnel wrote:
> Weird, my local branch history doesn't show these in it. I think rbt has 
> done something odd.

Make sure you publish your updated review before resolving these issues :)

I'll un-resolve this one in the meantime.


- Joseph


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


On Aug. 20, 2016, 9:57 a.m., Will Rouesnel wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51257/
> ---
> 
> (Updated Aug. 20, 2016, 9:57 a.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Bugs: MESOS-6003
> https://issues.apache.org/jira/browse/MESOS-6003
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds the external process container logger. This functions like the
> logrotate container logger, but instead calls a custom host binary
> (or script) and passes the executorInfo as JSON and base64 protobuf
> via environment variables. This makes it very easy for users to
> configure custom logging solutions, without needing to write and
> maintain logging modules.
> 
> Tests passing and complete.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 85eb2b855e87d432b9f9c9baa9a5b243928547f7 
>   src/Makefile.am d0f937d8879b11fac793b73f529762e95a9ff1e4 
>   src/slave/container_loggers/lib_externallogger.hpp PRE-CREATION 
>   src/slave/container_loggers/lib_externallogger.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/cgroups/cgroups.cpp 
> 1b84a561cd9498b85d03e901c4372e4f1293f185 
>   src/slave/containerizer/mesos/isolators/cgroups/constants.hpp 
> 3146cc6ee8532f7bd2a98881d0dc9c9cf5c607df 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp 
> 9789cf6a0f87eef5293331832369e6fb46e417e4 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.hpp 
> 10fbbc9cb39a3b41d64cc090b0e890a2416b2306 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/cpuacct.cpp 
> a35ecb073acefb903d7e8c4737d6cd048a2b494d 
>   src/tests/container_logger_external.sh PRE-CREATION 
>   src/tests/container_logger_tests.cpp 
> efadceafca5721bce4dbffadb35f54fd5365abb0 
>   src/tests/module.hpp e661d95fa44fc1aedfe83c564c826d5b7d32c85b 
>   src/tests/module.cpp 5b83fd6358ddea4c9d849b8992e1a6040ef74505 
> 
> Diff: https://reviews.apache.org/r/51257/diff/
> 
> 
> Testing
> ---
> 
> Adds ContainerLoggerTest.EXTERNAL_RecieveEnvironment which tests all major 
> parameters of the change.
> 
> A synthetic external container logger is provided by the script 
> tests/container_logger_external.sh which is setup to fail if any important 
> output is unavailable to the logging process. 
> 
> The other basic checks are duplicated from the Logrotate container logger, 
> from where this change inherits a lot of its code.
> 
> 
> Thanks,
> 
> Will Rouesnel
> 
>

Re: Review Request 51392: Supported provisioner listContainers() to be recursive.

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/paths.hpp (line 83)


What does 'parentId' mean here? I'd suggest we don't expose that parameter 
to the caller. To do proper recursion, you can introduce a inlined lambda which 
takes this extra parameter.

```
Try listContainers(
const string& provisionerDir)
{
  auto helper = [](
  const string& containersDir,
  const Option& parentContainerId) {
hashset results;

...
  }
  
  return helper(getContainersDir(provisionerDir), None());
}
```


- Jie Yu


On Aug. 24, 2016, 9:32 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51392/
> ---
> 
> (Updated Aug. 24, 2016, 9:32 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch supports collecting all containerIds (all containers in the
> nested hierarchy) recursively.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/paths.hpp 
> 9829d6b52c8547ae22297a5bc47852ce5a219e4c 
>   src/slave/containerizer/mesos/provisioner/paths.cpp 
> 86a45f30c22dc1e41f4779c0ce8c11d02dcc46bb 
> 
> Diff: https://reviews.apache.org/r/51392/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51124: Support more layers through symlink for overlay backend.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [51124]

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

- Mesos ReviewBot


On Aug. 25, 2016, 4:22 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Aug. 25, 2016, 4:22 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6000
> https://issues.apache.org/jira/browse/MESOS-6000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support more layers through symlink for overlay backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 93819c887f2f801f06aae7020084b56cc81ce818 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 4c5cdb660dea205aea29d217ba80d845d1108fdf 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> e57bb3d2fb4e67e9caae416824a6a748db1460a1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> ad19feccf08112f94d6514a134963c54ca541e88 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 7c9a7cd5c733f74e10316fc1234115e6820cc2cb 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)
> I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
> '/tmp/NcmRZt' pointed by 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 51343: Refactored the redundant logic in provisioner recover().

[Gilbert Song]

> On Aug. 25, 2016, 10:57 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/provisioner/provisioner.cpp, line 204
> > 
> >
> > Why this change? Where is the error checking?

I removed the error check because it seems redundant to me. We construct the 
`backends` hashmap from `Backend::Create()`. The backends information from the 
filesystem (returned from `listContainerRootfses`) should only contain the 
backends that are included in `Backend::create()`. So this error should be 
never triggered.


- Gilbert


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


On Aug. 23, 2016, 10:19 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51343/
> ---
> 
> (Updated Aug. 23, 2016, 10:19 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the unnecessary loop to construct an identical
> hashmap for info->rootfses.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51391: Fixed a bug related to logger in MesosContainerizer.

[Joseph Wu]

> On Aug. 24, 2016, 4:49 p.m., Joseph Wu wrote:
> > Looks like the same code in the docker containerizer is missing the same 
> > check.
> 
> Jie Yu wrote:
> Can you create a ticket to track? :)

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


- Joseph


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


On Aug. 24, 2016, 2:09 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51391/
> ---
> 
> (Updated Aug. 24, 2016, 2:09 p.m.)
> 
> 
> Review request for mesos and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> It's possible that 'destroy' is called while logger 'prepare' is being
> called. If that happens, when logger 'prepare' finishes, it'll trigger
> an assertion failure.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51391/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 50125: Added mesos-docker-executor support for devices control.

[Kevin Klues]

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




src/docker/executor.hpp (lines 80 - 83)


This comment needs rewording as its hard to understand.

Maybe something like:
```
A comma separated list of devices to expose to the docker container. The 
format of this flag mimics that of the `--devices` flag to docker run, i.e: 
`host_path:container_path:permissions`. For example, `/dev/tty:/dev/tty:mrw'.
```



src/docker/executor.cpp (line 89)


By the time the devices are passed in here they should be of type 
`vector`, not just a string. 

This is similar to how `task_environment` comes in as a simple string via 
`Flags`, but is converted to a `map` by the time it is passed 
into `DockerExecutorProcess`.



src/docker/executor.cpp (lines 159 - 174)


With the change suggessted above, this entire code block disappears.



src/docker/executor.cpp (line 189)


With the change suggested above, this becomes:

```
devices,
```



src/docker/executor.cpp (line 594)


`vector devices`



src/docker/executor.cpp (line 618)


`const vector& devices`



src/docker/executor.cpp (lines 819 - 826)


Here is where you should convert the input string to a `vector`, 
i.e.:

```
vector devices;

if (flags.devices.isSome()) {
  const vector deviceList = strings::split(devices, ",");
  
  foreach (const string& device, deviceList) {
Try parsed = Device::parse(device);

if(parse.isError()) {
  cerr << flags.usage("Failed to parse --devices: " + parse.error())
   << endl;
  return EXIT_FAILURE; 
}

devices->push_back(parsed);
  }
}
```



src/docker/executor.cpp (line 822)


I don't think we want to `cout` anything here since this is an optional 
flag.


- Kevin Klues


On Aug. 15, 2016, 7:29 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50125/
> ---
> 
> (Updated Aug. 15, 2016, 7:29 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new flag '--devices' to mesos-docker-executor, and gave its
> feature to control devices exposition, isolation, and access permission.
> 
> 
> Diffs
> -
> 
>   src/docker/executor.hpp 7b63d784d6b8685912598b77fb38cf6e70646ae3 
>   src/docker/executor.cpp 8d679cd33b6ddf3a5c11bb8c458a97b8809473ac 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
> 
> Diff: https://reviews.apache.org/r/50125/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 51409: Printed all the isolator cleanup errors during destory.

[Jie Yu]

> On Aug. 25, 2016, 8:22 a.m., Gilbert Song wrote:
> > src/slave/containerizer/mesos/containerizer.cpp, lines 1768-1769
> > 
> >
> > We should still keep the `ContainerID`, right?

Nope, caller will (and should) print the container Id.


- Jie


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


On Aug. 25, 2016, 5:22 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51409/
> ---
> 
> (Updated Aug. 25, 2016, 5:22 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The current code only prints the first error.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51409/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 51416: Fixed a TODO in the MesosContainerizer.

[Gilbert Song]

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




src/slave/containerizer/mesos/containerizer.cpp (lines 1641 - 1644)


Is it possible the `reap()` takes forever?


- Gilbert Song


On Aug. 24, 2016, 10:20 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51416/
> ---
> 
> (Updated Aug. 24, 2016, 10:20 p.m.)
> 
> 
> Review request for mesos, Gilbert Song and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch fixed a bug in the destory path. It correctly handles a
> race where the container is destroyed while it is being launched. The
> idea is to make 'container->status' optional and don't set it until
> the container is actually forked.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/containerizer.hpp 
> 1f414cfa332d9a3f8b8f04343249e02924e39d89 
>   src/slave/containerizer/mesos/containerizer.cpp 
> 8a8985a6627859365c4f87ea0cbb5f89190cd4b3 
> 
> Diff: https://reviews.apache.org/r/51416/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Review Request 51433: Added test case MasterAuthorizationTest, UnauthorizedTaskGroup.

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added test case MasterAuthorizationTest, UnauthorizedTaskGroup.


Diffs
-

  src/tests/master_authorization_tests.cpp 
92bd2a9463f861e4b479f76927dd01324765a411 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="MasterAuthorizationTest.UnauthorizedTaskGroup"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from MasterAuthorizationTest
[ RUN  ] MasterAuthorizationTest.UnauthorizedTaskGroup
[   OK ] MasterAuthorizationTest.UnauthorizedTaskGroup (85 ms)
[--] 1 test from MasterAuthorizationTest (86 ms total)

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


Thanks,

Guangya Liu

Re: Review Request 50599: Passed allocated GPUs to 'devices' entry of 'docker::Flags'.

[Kevin Klues]

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




src/slave/containerizer/docker.cpp (lines 230 - 249)


I would probably move this into the next patch alongside the other changes 
to the flags and not even bother with the comment.


- Kevin Klues


On Aug. 22, 2016, 10:12 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated Aug. 22, 2016, 10:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new string entry 'devices' to 'docker::Flags' to
> indicate host devices exposed into docker container, and
> passed allocated GPUs to it.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Vinod Kone]

> On April 14, 2016, 4:49 p.m., Vinod Kone wrote:
> > src/launcher/http_command_executor.cpp, line 749
> > 
> >
> > Looking at slave::statusUpdate() code there are several scenarios where 
> > the slave ignores a status update sent by the executor; this means this 
> > executor could end up not terminating forever.
> > 
> > Can you do the following:
> > 
> > --> Enque a message in the queue to self terminate after some timeout 
> > (you can use the delay() function) to be safe.
> > 
> > --> Add a TODO that we do this to be safe and also because slave 
> > sometimes doesn't ACK a status update. Link to a ticket that fixes the 
> > slave status update semantics to always ACK a status update sent by an 
> > executor.
> > 
> > sounds good?
> 
> Vinod Kone wrote:
> @Qian, any update on this? If this particular review is going to take 
> some time, I think it is still useful two commit the other 2 reviews in this 
> chain. AFAICT, they are independent of this review?
> 
> Qian Zhang wrote:
> @Vinod, sorry for the late. I have filed a ticket 
> (https://issues.apache.org/jira/browse/MESOS-5262) for enhancing 
> `slave::statusUpdate()` to always ACK the status update sent by executor.
> 
> And can you please elaborate about the specific scenarios this executor 
> could not terminate forever. Originially I thought the scenario should be: 
> executor sends a terminal status upate to slave when the corresponding 
> framework is in `TERMINATING` state (e.g., operator tears down the 
> framework), then in `Slave::statusUpdate()`, this status update will be 
> ignored, so the executor will not get the ACK. But after testing, I found in 
> this case the executor can still terminate, because the container 
> corresponded to this executor will be destroyed by 
> `Slave::shutdownExecutorTimeout()` -> `MesosContainerizer::destroy()`, so 
> after `--executor_shutdown_grace_period`, the executor can still terminate. 
> So I am not in which case the executor will never terminate.
> 
> And yes, the other 2 patches are independent of this one, I will make 
> them not depending on this one in the review board, thanks!
> 
> Qian Zhang wrote:
> After more thinking, I see one scenario the executor could never 
> terminate is: agent is down right after it sends SHUTDOWN event to executor. 
> In this case, when handling the SHUTDOWN event, executor will kill the task 
> and send TASK_KILLED status update to agent, but it will not get ACK since 
> agent is already down, so the executor will still run. But I think once agent 
> is started again, executor will receive the ACK and then terminate. I am not 
> sure if this behavior is OK, or we want executor to terminate once it 
> receives the SHUTDOWN event rather than wait for agent is started again?
> 
> Vinod Kone wrote:
> I think it is the right thing for the executor to stay up until the agent 
> comes back up and sends an ACK. Otherwise, which is currently the case, the 
> agent has no idea about the exit status of the executor.
> 
> So the cases where I see no ACKs from the agent are:
> 
> 1) `update` doesn't have UUID.
>   --> Is this possible with the HTTP executor? If not, we should probably 
> shutdown the executor instead of just ignoring.
> 
> 2) Framework is unknown.
>   --> Don't think we can do much here because we don't have access to 
> Executor object?
> 
> 3) Framework is terminating.
>   --> Per your observation the agent is guaranteed to destroy the 
> container. We need to add a comment here explainining this.
> 
> 4) Executor is unknown. While we forward the status update to the 
> framework, the ACK is dropped by `_statusUpdate` and `___statusUpdate()`
>--> If the update is generated by the agent (`killTask()`, 
> `_runTask()`) we should be fine because there is no executor.
>--> If the update is sent by an executor for a task belonging to 
> another executor, that another executor is hopefully already dead. So we 
> should be fine.
>--> Is it possible for the update to be sent by an executor that is 
> not known to the agent? Don't think we can do much here since we don't have 
> access to the Executor object.
> 
> Qian Zhang wrote:
> > I think it is the right thing for the executor to stay up until the 
> agent comes back up and sends an ACK. Otherwise, which is currently the case, 
> the agent has no idea about the exit status of the executor.
> 
> But that seems conflict with your original comment. My understanding to 
> your original comment is, to avoid executor not terminating forever, executor 
> needs to enque a message to self terminate after some timeout (I think we can 
> do it via delay() in `reaped()` after terminal status update is sent), if so, 
> then when executor handles SHUTDOWN event, it will self terminate anyway 

Re: Review Request 50599: Passed allocated GPUs to 'devices' entry of 'docker::Flags'.

[Kevin Klues]

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




src/slave/containerizer/docker.hpp (line 509)


I don't think this should just be a string. It should be an actual vector 
of type `Device`.

Also, the comment should read:
```
// Devices attached to the container.
```



src/slave/containerizer/docker.cpp (lines 685 - 719)


If you store `devices` as a vector of type `Device`, and you use the 
suggestion from the previous patch to add a static `parse()` function to 
`Device`, this whole code block becomes:

```
container->devices.push_back(
Device::parse("/dev/nvidiactl:/dev/nvidiactl:rwm"));

container->devices.push_back(
Device::parse("/dev/nvidia-uvm:/dev/nvidia-uvm:rwm"));

if (os::exists("/dev/nvidia-uvm-tools")) {
  container->devices.push_back(
  Device::parse("/dev/nvidia-uvm-tools:/dev/nvidia-uvm-tools:rwm"));
}
```

The join as a single comma-separated string should happen only when you are 
ready to add it to the flags.



src/slave/containerizer/docker.cpp (lines 685 - 686)


You also need to include `/dev/nvidia-uvm-tools` if it exists.


- Kevin Klues


On Aug. 22, 2016, 10:12 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50599/
> ---
> 
> (Updated Aug. 22, 2016, 10:12 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a new string entry 'devices' to 'docker::Flags' to
> indicate host devices exposed into docker container, and
> passed allocated GPUs to it.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
> 
> Diff: https://reviews.apache.org/r/50599/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 46187: Terminate when receiving the ACK of terminal status update.

[Vinod Kone]

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



qian, sorry for the delay on getting back on this one. are you still interested 
in seeing this through? if yes, can you please rebase?

- Vinod Kone


On April 27, 2016, 1:23 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/46187/
> ---
> 
> (Updated April 27, 2016, 1:23 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar and Vinod Kone.
> 
> 
> Bugs: MESOS-4855
> https://issues.apache.org/jira/browse/MESOS-4855
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Terminate when receiving the ACK of terminal status update.
> 
> 
> Diffs
> -
> 
>   src/launcher/http_command_executor.cpp 
> 0b4136c040ec9cf585c0d38f8471495a855cd640 
> 
> Diff: https://reviews.apache.org/r/46187/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 51434: Use `continue` when breaking out of LAUNCH_GROUP case due to errors.

[Jiang Yan Xu]

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

Review request for mesos, Benjamin Mahler and Vinod Kone.


Repository: mesos


Description
---

This is the convention established by the rest of the switch cases in
`_accept()` and it will allow common code to be put after the switch
for each *valid* operation.


Diffs
-

  src/master/master.cpp 789fb9760a5ea60ce3b7b08f17e36cec69349c60 

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


Testing
---

make check.


Thanks,

Jiang Yan Xu

Re: Review Request 51343: Refactored the redundant logic in provisioner recover().

[Jie Yu]

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




src/slave/containerizer/mesos/provisioner/provisioner.cpp (line 204)


Why this change? Where is the error checking?


- Jie Yu


On Aug. 23, 2016, 5:19 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51343/
> ---
> 
> (Updated Aug. 23, 2016, 5:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, Jie Yu, and 
> Kevin Klues.
> 
> 
> Bugs: MESOS-6067
> https://issues.apache.org/jira/browse/MESOS-6067
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch removed the unnecessary loop to construct an identical
> hashmap for info->rootfses.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> 8e35ff49ec99a242e764095dcfbb541c5e41ec71 
> 
> Diff: https://reviews.apache.org/r/51343/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Jie Yu]

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




src/slave/containerizer/mesos/launcher.cpp (lines 91 - 114)


No need to use os::open. Simply call os::read to get the string from the 
file. For such IO operations, no need to make it async (e.g., LOG(INFO) is 
synchrounous as well).

BTW: you forgot to close the fd.


- Jie Yu


On Aug. 25, 2016, 1:21 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51406/
> ---
> 
> (Updated Aug. 25, 2016, 1:21 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the 'mesos-containerizer launch' binary would simply exec
> into the actual command we wanted to launch after doing some set of
> preperatory work. The problem with this approach, however, is that
> this gives us no opportunity to checkpoint the exit status of the
> command so the agent can recover it in cases where it is offline at
> the time the command completes.
> 
> To compensate for this, we now allow 'mesos-containerizer launch' to
> take an optional '--exit_status_path' parameter, which indicates where
> the exit status of our command should be checkpointed. In order to
> do this checkpointing, however, we cannot simply exec into the command
> anymore. Instead we now fork-exec the command and reap it once it
> completes. We then checkpoint its exit status and return it as our
> own. We call the original process the 'init' process of the container
> and the fork-exec'd process its child.  As a side effect of doing
> things this way, we also have to be careful to forward all signals
> sent to the init process down to the child.
> 
> In order to properly reap the 'init' process, we also introduce a new
> 'Launcher::wait' command that knows how to reap the 'init' process
> directly and then retreive its exit status from the checkpoint (if
> necessary).
> 
> In a subsequent commit we will update the mesos containerizer to use
> this new functionality.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/launch.hpp 
> 0e86da9c7bd9c7fbedd7102d66b902d1c10e5e0b 
>   src/slave/containerizer/mesos/launch.cpp 
> 2db8db5ac2f4614d406b950a1b4e8098a0d90e7b 
>   src/slave/containerizer/mesos/launcher.hpp 
> 0eae09515d550a2c71ae1414d4a22943f1d09db9 
>   src/slave/containerizer/mesos/launcher.cpp 
> 413a8afdc56127a58c9599c436d17d6c98e62434 
>   src/slave/containerizer/mesos/linux_launcher.hpp 
> 8fbe1e9742df85b0fc6de46ac81a0c064c845a63 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 7377316776646e3d660086da3c0d5b494ce8ace4 
>   src/tests/containerizer/launcher.hpp 
> 94c62b761a17436841bd19f3bf622cc8f1047876 
>   src/tests/containerizer/launcher.cpp 
> a92d9890f0931425d69ef8ce0896d081b8722079 
> 
> Diff: https://reviews.apache.org/r/51406/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 51432: Added test case for TaskGroupValidationTest.ExecutorWithoutFrameworkId.

[Vinod Kone]

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


Fix it, then Ship it!





src/tests/master_validation_tests.cpp (line 1665)


s/executors/an executor/

i'll fix this while committing.


- Vinod Kone


On Aug. 25, 2016, 5:25 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51432/
> ---
> 
> (Updated Aug. 25, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-6087
> https://issues.apache.org/jira/browse/MESOS-6087
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added test case for TaskGroupValidationTest.ExecutorWithoutFrameworkId.
> 
> 
> Diffs
> -
> 
>   src/tests/master_validation_tests.cpp 
> 86b4b22350175af592876fd2f6d3fecca7acabce 
> 
> Diff: https://reviews.apache.org/r/51432/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  
> --gtest_filter="TaskGroupValidationTest.ExecutorWithoutFrameworkId"
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from TaskGroupValidationTest
> [ RUN  ] TaskGroupValidationTest.ExecutorWithoutFrameworkId
> [   OK ] TaskGroupValidationTest.ExecutorWithoutFrameworkId (92 ms)
> [--] 1 test from TaskGroupValidationTest (92 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (105 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 51406: Updated 'launcher' to optionally spawn an 'init' process.

[Jie Yu]

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



Can you split the Launcher::wait support to a separate patch?


src/slave/containerizer/mesos/launch.cpp (line 124)


We use the same binary for nested container as well. So it might not be an 
executor. I'd rename it to `containerPid`



src/slave/containerizer/mesos/launch.cpp (lines 130 - 142)


indentation here should be 2 :)



src/slave/containerizer/mesos/launch.cpp (lines 130 - 131)


You can do the following:
```
bufferedSignals[sig].fetch_add(1)
```



src/slave/containerizer/mesos/launch.cpp (line 132)


defer is not async singal safe because it'll try to acquire a lock.

os::kill is async signal safe
http://man7.org/linux/man-pages/man7/signal.7.html

Looking at possible failure scenarios of os::kill, none of them looks 
possible except ESRCH. But in that case, the container pid is gone, and the 
process will exit soon. So no need to print the error message.

Therefore, i'd suggest we simply call os::kill in the signal handler and 
ignore the error. No need to use defer.



src/slave/containerizer/mesos/launch.cpp (line 133)


What if bufferedSignals[i] > 1?



src/slave/containerizer/mesos/launch.cpp (lines 161 - 172)


We have os::signal::install in stout. Please use that



src/slave/containerizer/mesos/launch.cpp (lines 220 - 225)


I'd suggest move this whole block of code after we receive the control pipe 
signal from the parent to continue.

If parent dies, there no point to open and create the exit_status file.



src/slave/containerizer/mesos/launch.cpp (line 224)


even if we pivot_root below.



src/slave/containerizer/mesos/launch.cpp (lines 229 - 239)


I suggest that we create the directory in `Launcher::fork` because it also 
needs to checkpoint the pid there.

This helper binary should just open the file at `exit_status_path`.



src/slave/containerizer/mesos/launch.cpp (line 244)


can you make sure fd has cloexec flag, we don't want to leak this fd to the 
subprocess.



src/slave/containerizer/mesos/launch.cpp (line 245)


I'd insert a new line above.



src/slave/containerizer/mesos/launch.cpp (lines 493 - 497)


I'd suggest we restructure the code like the following. Since we cannot do 
signal forwarding on windows, i'd suggest we simply does not support this 
feature on Windows. This flag will only be used in LinuxLauncher.

```
#ifndef __WINDOWS__
if (exitStatusFd.isSome()) {
  pid_t pid = fork();
  if (pid < 0) {
cerr << ... << endl;
return EXIT_FAILURE;
  } else if (pid > 0) {
// Parent.
forwardSingals(pid);

...
  }
}
#endif // __WINDOWS__

// Child.
if (command->shell()) {
  os::execlp(...);
} else {
  execvp(...);
}
```



src/slave/containerizer/mesos/launch.cpp (line 502)


I'd avoid using subprocess here. Using subprocess here means that we'll 
initialize libprocess, creating many unnecessary threads. I would simply 
fork-exec here. We don't have two worry about async signal safe problem here 
because it's single threaded process.

We need to fix the `pre_exec_comands` above for the same problem.



src/slave/containerizer/mesos/launch.cpp (lines 542 - 544)


else if



src/slave/containerizer/mesos/launch.cpp (line 550)


kill one extra line



src/slave/containerizer/mesos/launch.cpp (line 562)


better to print the exit status path as well here


- Jie Yu


On Aug. 25, 2016, 1:21 a.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51406/
> ---
> 
> (Updated Aug. 25, 2016, 1:21 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-6088
> https://issues.apache.org/jira/browse/MESOS-6088

Re: Review Request 50123: Added GPU scheduler for docker containerizer process.

[Kevin Klues]

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


Fix it, then Ship it!





src/slave/containerizer/docker.cpp (lines 1275 - 1289)


This should probably just be moved into the next patch since it doesn't 
really do much here other than check the vailidity of the GPU resources.

That said, I'm fine with keeping it in this one so long as @bmahler is too.


- Kevin Klues


On Aug. 25, 2016, 9:38 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50123/
> ---
> 
> (Updated Aug. 25, 2016, 9:38 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This added 'NvidiaGpuAllocator' to docker containerizer process so that
> docker containerizer process is ready to use it to allocate GPUs to task
> with 'gpus' resource.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
>   src/tests/mesos.hpp ad31276aeb2cb7ed5ba3e091a9085f35addf17c4 
>   src/tests/mesos.cpp 62e8fcc6fa7bd856aab6148ca6e6cad66b436f04 
> 
> Diff: https://reviews.apache.org/r/50123/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Review Request 51432: Added test case for TaskGroupValidationTest.ExecutorWithoutFrameworkId.

[Guangya Liu]

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

Review request for mesos and Vinod Kone.


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


Repository: mesos


Description
---

Added test case for TaskGroupValidationTest.ExecutorWithoutFrameworkId.


Diffs
-

  src/tests/master_validation_tests.cpp 
86b4b22350175af592876fd2f6d3fecca7acabce 

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


Testing
---

make
make check

```
./bin/mesos-tests.sh  
--gtest_filter="TaskGroupValidationTest.ExecutorWithoutFrameworkId"
[==] Running 1 test from 1 test case.
[--] Global test environment set-up.
[--] 1 test from TaskGroupValidationTest
[ RUN  ] TaskGroupValidationTest.ExecutorWithoutFrameworkId
[   OK ] TaskGroupValidationTest.ExecutorWithoutFrameworkId (92 ms)
[--] 1 test from TaskGroupValidationTest (92 ms total)

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


Thanks,

Guangya Liu

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

[haosdent huang]

> On Aug. 25, 2016, 12:49 p.m., Tomasz Janiszewski wrote:
> > src/health-check/health_checker.cpp, line 452
> > 
> >
> > Will it work on all supported systems?

Windows also have bash now but require external installation. But bash is a 
temporary solution. We are plan to implement a tiny program which suppport TCP 
half open and replace bash with it.


- haosdent


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


On Aug. 25, 2016, 1:25 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/49360/
> ---
> 
> (Updated Aug. 25, 2016, 1:25 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Mahler, Gastón 
> Kleiman, Gilbert Song, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-3567
> https://issues.apache.org/jira/browse/MESOS-3567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported TCP in health check.
> 
> 
> Diffs
> -
> 
>   src/health-check/health_checker.hpp 
> b4548f385e6bdf12f6bbc402a5d59ba8e165b8a5 
>   src/health-check/health_checker.cpp 
> 45a5fe00a95a6e88b1990c1396e03082feb202bc 
> 
> Diff: https://reviews.apache.org/r/49360/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 50841: Added GPU scheduling logic to docker containerizer process.

[Kevin Klues]

> On Aug. 25, 2016, 12:32 a.m., Kevin Klues wrote:
> > src/slave/containerizer/docker.cpp, line 1555
> > 
> >
> > I wouldn't just blindly call this function here. It should be wrapped 
> > in some logic that makes sure it's OK to call it (i.e. checks to make sure 
> > that we have the nvidia->allocator component passed in).
> > 
> > Again, you could have some logic above which saves a temporary `Future` 
> > that is set to `Nothing()` by default and is the result of calling 
> > `deallocateNvidiaGpu()` otherwise.
> 
> Guangya Liu wrote:
> The `deallocateNvidiaGpu` already have some checking for 
> `nvidiaGpuAllocator`, is this enough?

I don't think we want to call any `nvidia*` functions anywhere in the code 
without checks around them at the call site. For someone reading the code top 
to bottom it makes it look like we are *always* allocating / deallocating / 
etc. GPUs, which is cnfusing. The checks inside the function are to make sure 
that we don't accidentally call them somewhere without the proper check at the 
call site.


- Kevin


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


On Aug. 22, 2016, 10:11 a.m., Yubo Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50841/
> ---
> 
> (Updated Aug. 22, 2016, 10:11 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Guangya Liu, Kevin Klues, and 
> Rajat Phull.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added control logic to allocate/deallocate GPUs to GPU-related task
> when the task is started/terminated.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp f2a06065cf99fed934c2c1ffc47461ec8a97f50d 
>   src/slave/containerizer/docker.cpp 5c1ee8e467d1c54c60b67dc5275ef71e1bb90723 
> 
> Diff: https://reviews.apache.org/r/50841/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Yubo Li
> 
>

Re: Review Request 51009: Collect throttle related cpu.stat for Docker Containerizer.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On Aug. 22, 2016, 7:15 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51009/
> ---
> 
> (Updated Aug. 22, 2016, 7:15 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, Jie Yu, and Steve 
> Niemitz.
> 
> 
> Bugs: MESOS-6031
> https://issues.apache.org/jira/browse/MESOS-6031
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Collect throttle related cpu.stat for Docker Containerizer.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.cpp e447c58bd46ba080529e8f349347eccf5a54110a 
> 
> Diff: https://reviews.apache.org/r/51009/diff/
> 
> 
> Testing
> ---
> 
> Run agent with cfs quota enabled, and observe that throttle related metrics 
> are in `/containers` and `/monitoring/statistics`
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 50524: Updated docker recovery to use abstraction provided by docker inspect.

[Rajat Phull]

> On Aug. 21, 2016, 3:15 p.m., Guangya Liu wrote:
> > Why not merge this with https://reviews.apache.org/r/50523 ?

Agreed, this is now merged with https://reviews.apache.org/r/50523. Can be 
dropped from review.


- Rajat


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


On July 29, 2016, 6:16 p.m., Rajat Phull wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/50524/
> ---
> 
> (Updated July 29, 2016, 6:16 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Yubo Li.
> 
> 
> Bugs: MESOS-5795
> https://issues.apache.org/jira/browse/MESOS-5795
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker recovery to use abstraction provided by docker inspect.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 43ca4317d608b3b43dd7bd0d1b55c721e7364885 
>   src/slave/containerizer/docker.cpp 12bad2db03bcf755317c654f028b628c5c407a62 
> 
> Diff: https://reviews.apache.org/r/50524/diff/
> 
> 
> Testing
> ---
> 
> GTEST_FILTER="NvidiaGpuDockerContainerizerTest.ROOT_DOCKER_LaunchWithGpuRecovery"
>  make -j check
> 
> 
> Thanks,
> 
> Rajat Phull
> 
>

Review Request 51431: Added health check support to mesos-execute.

[haosdent huang]

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

Review request for mesos.


Repository: mesos


Description
---

Only used for test.


Diffs
-

  src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 51124: Support more layers through symlink for overlay backend.

[Zhitao Li]

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

(Updated Aug. 25, 2016, 4:22 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Support more layers through symlink for overlay backend.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/backend.hpp 
93819c887f2f801f06aae7020084b56cc81ce818 
  src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
  src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
4c5cdb660dea205aea29d217ba80d845d1108fdf 
  src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
  src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
  src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
  src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
e57bb3d2fb4e67e9caae416824a6a748db1460a1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
387f28a331813c75a509b4a31dbbdc764080b8c1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
ad19feccf08112f94d6514a134963c54ca541e88 
  src/tests/containerizer/provisioner_backend_tests.cpp 
7c9a7cd5c733f74e10316fc1234115e6820cc2cb 

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


Testing
---

1. Make sure `OverlayBackendTest.*` passes;
2. Use mesos-execute to provision a container using overlay backend. Observed 
following log lines:

```
I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
'/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
 to '/tmp/NcmRZt'
I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
overlayfs: 
'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d
 9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
...
(after executor exited)
I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
'/tmp/NcmRZt' pointed by 
'/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
```


Thanks,

Zhitao Li

Re: Review Request 51124: Support more layers through symlink for overlay backend.

[Zhitao Li]

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

(Updated Aug. 25, 2016, 4:13 p.m.)


Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu.


Changes
---

- Clean up symlink more aggressively;
- Add test for temp dir and symlink;
- Pass backendDir to `Backend::destroy` and `OverlayBackendProcess::destroy`.


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


Repository: mesos


Description
---

Support more layers through symlink for overlay backend.


Diffs (updated)
-

  src/slave/containerizer/mesos/provisioner/backend.hpp 
93819c887f2f801f06aae7020084b56cc81ce818 
  src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
  src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
4c5cdb660dea205aea29d217ba80d845d1108fdf 
  src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
  src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
  src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
  src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
e57bb3d2fb4e67e9caae416824a6a748db1460a1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
387f28a331813c75a509b4a31dbbdc764080b8c1 
  src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
  src/slave/containerizer/mesos/provisioner/provisioner.cpp 
ad19feccf08112f94d6514a134963c54ca541e88 
  src/tests/containerizer/provisioner_backend_tests.cpp 
7c9a7cd5c733f74e10316fc1234115e6820cc2cb 

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


Testing
---

1. Make sure `OverlayBackendTest.*` passes;
2. Use mesos-execute to provision a container using overlay backend. Observed 
following log lines:

```
I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
'/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
 to '/tmp/NcmRZt'
I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
overlayfs: 
'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d
 9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
...
(after executor exited)
I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
'/tmp/NcmRZt' pointed by 
'/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
```


Thanks,

Zhitao Li

Re: Review Request 51124: Support more layers through symlink for overlay backend.

[Zhitao Li]

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




src/slave/containerizer/mesos/provisioner/backends/overlay.cpp (lines 285 - 286)


Good point. I think symlink will be removed anyway when the entire 
`backendDir` is cleaned up, but I think it's better practice to explicitly 
remove it here. Will fix.


- Zhitao Li


On Aug. 25, 2016, 4:13 p.m., Zhitao Li wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51124/
> ---
> 
> (Updated Aug. 25, 2016, 4:13 p.m.)
> 
> 
> Review request for mesos, Xiaojian Huang, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-6000
> https://issues.apache.org/jira/browse/MESOS-6000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Support more layers through symlink for overlay backend.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/provisioner/backend.hpp 
> 93819c887f2f801f06aae7020084b56cc81ce818 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.hpp 
> a5ac13bc17b8f291472b622ebfdd0cd1dd072273 
>   src/slave/containerizer/mesos/provisioner/backends/aufs.cpp 
> 4c5cdb660dea205aea29d217ba80d845d1108fdf 
>   src/slave/containerizer/mesos/provisioner/backends/bind.hpp 
> 19d44c6a9a2cd5f8de79f33c76e9d79900ab003f 
>   src/slave/containerizer/mesos/provisioner/backends/bind.cpp 
> 62ccaec1a5cfb466e929f4db7bf7e5376f2a0c2d 
>   src/slave/containerizer/mesos/provisioner/backends/copy.hpp 
> 5bd6a5279d8dd6d088afcd58e4839bf6087ccd1e 
>   src/slave/containerizer/mesos/provisioner/backends/copy.cpp 
> e57bb3d2fb4e67e9caae416824a6a748db1460a1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.hpp 
> 387f28a331813c75a509b4a31dbbdc764080b8c1 
>   src/slave/containerizer/mesos/provisioner/backends/overlay.cpp 
> e20cd48463a78ecd8a652a4d5ac923dc02cca4d9 
>   src/slave/containerizer/mesos/provisioner/provisioner.cpp 
> ad19feccf08112f94d6514a134963c54ca541e88 
>   src/tests/containerizer/provisioner_backend_tests.cpp 
> 7c9a7cd5c733f74e10316fc1234115e6820cc2cb 
> 
> Diff: https://reviews.apache.org/r/51124/diff/
> 
> 
> Testing
> ---
> 
> 1. Make sure `OverlayBackendTest.*` passes;
> 2. Use mesos-execute to provision a container using overlay backend. Observed 
> following log lines:
> 
> ```
> I0816 01:04:27.823420 46584 overlay.cpp:167] Created tempLink at 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
>  to '/tmp/NcmRZt'
> I0816 01:04:27.824834 46584 overlay.cpp:191] Provisioning image rootfs with 
> overlayfs: 
> 'lowerdir=/tmp/NcmRZt/42:/tmp/NcmRZt/41:/tmp/NcmRZt/40:/tmp/NcmRZt/39:/tmp/NcmRZt/38:/tmp/NcmRZt/37:/tmp/NcmRZt/36:/tmp/NcmRZt/35:/tmp/NcmRZt/34:/tmp/NcmRZt/33:/tmp/NcmRZt/32:/tmp/NcmRZt/31:/tmp/NcmRZt/30:/tmp/NcmRZt/29:/tmp/NcmRZt/28:/tmp/NcmRZt/27:/tmp/NcmRZt/26:/tmp/NcmRZt/25:/tmp/NcmRZt/24:/tmp/NcmRZt/23:/tmp/NcmRZt/22:/tmp/NcmRZt/21:/tmp/NcmRZt/20:/tmp/NcmRZt/19:/tmp/NcmRZt/18:/tmp/NcmRZt/17:/tmp/NcmRZt/16:/tmp/NcmRZt/15:/tmp/NcmRZt/14:/tmp/NcmRZt/13:/tmp/NcmRZt/12:/tmp/NcmRZt/11:/tmp/NcmRZt/10:/tmp/NcmRZt/9:/tmp/NcmRZt/8:/tmp/NcmRZt/7:/tmp/NcmRZt/6:/tmp/NcmRZt/5:/tmp/NcmRZt/4:/tmp/NcmRZt/3:/tmp/NcmRZt/2:/tmp/NcmRZt/1:/tmp/NcmRZt/0,upperdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/25d9b6e4-fd0e-401e-bb9e-754dbaec4f55/upperdir,workdir=/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/scratch/2
 5d9b6e4-fd0e-401e-bb9e-754dbaec4f55/workdir'
> ...
> (after executor exited)
> I0816 01:04:34.859851 46584 overlay.cpp:281] Removed temporary directory 
> '/tmp/NcmRZt' pointed by 
> '/var/lib/mesos/provisioner/containers/fd574bbf-4bc0-4538-9ce5-8c2cc93b94c7/backends/overlay/links'
> ```
> 
> 
> Thanks,
> 
> Zhitao Li
> 
>

Re: Review Request 50956: Added name to uri fetcher plugins.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added name to uri fetcher plugins.


Diffs (updated)
-

  include/mesos/uri/fetcher.hpp 3add35c8c0e559203acb540a288d0b51ac817519 
  src/uri/fetchers/copy.hpp 64e686c3249a34c9c68f1bf0ccde1683516d9f26 
  src/uri/fetchers/copy.cpp f095ad65b50405c5e0869e74652ce9529332dd0c 
  src/uri/fetchers/curl.hpp 447e01b9a566df133fd397cc4ea80150761f4653 
  src/uri/fetchers/curl.cpp cc3f9eec42eb841ecd320ffc83aa7884dc67c415 
  src/uri/fetchers/docker.hpp 6cb57be97d9494ea59ce9759e3d52e37b19d6c43 
  src/uri/fetchers/docker.cpp 72f70b8d17c947d5e9d8eb70c1c6bd6046bf1cd2 
  src/uri/fetchers/hadoop.hpp 1689f607a2bcff09887e5aa4d6e03de0ac020260 
  src/uri/fetchers/hadoop.cpp 3c69d43d7bc21061adb9c392da976dc5584e74d4 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50957: Added fetch method based on plugin name.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description (updated)
---

Added plugins by name into the Fetcher. Added a new fetch
method to select plugin by name which ignores scheme and
directly invokes fetch on the plugin.


Diffs (updated)
-

  include/mesos/uri/fetcher.hpp 3add35c8c0e559203acb540a288d0b51ac817519 
  src/uri/fetcher.cpp 198bd29993381758183edfce8faafba36da2d9ae 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50960: Added appc uri fetcher tests.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description (updated)
---

Added tests to fetch using all three modes that 'rkt fetch' supports,
fetch with meta discovery  ex: 'coreos.com/hello:latest',
fetch from a specific  location ex: 'https://github.com/xxx/hello.aci'
fetch from docker registry ex: 'docker://hello-world'.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 072c09b8081e5d5dded907b1a0ffb6d739d86911 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 50958: Added tests to invoke the fetcher plugins by name.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Add a test case for each of the existing fetcher plugins to
invoke them by name.


Diffs (updated)
-

  src/tests/uri_fetcher_tests.cpp 072c09b8081e5d5dded907b1a0ffb6d739d86911 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51096: Added the `mesos-port-mapper` binary.

[Avinash sridharan]

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

(Updated Aug. 25, 2016, 3:21 p.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Fixed comments.


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


Repository: mesos


Description
---

The `mesos-port-mapper` is a CNI plugin that can be used to install
port-forwarding rules for a container. The `mesos-port-mapper` is a
wrapper CNI plugin that should always be used in conjunction with
other CNI plugins.


Diffs (updated)
-

  src/Makefile.am d389e02584cfb1a00067cd30de1364118d1c46c7 
  
src/slave/containerizer/mesos/isolators/network/cni/plugins/port_mapper/main.cpp
 PRE-CREATION 

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


Testing
---

make


Thanks,

Avinash sridharan

Re: Review Request 50959: Added appc fetcher plugin to use rkt tool.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:22 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description (updated)
---

AppcFetcherPlugin returns a '.aci' file of a container image.

Fetch supports all three modes that 'rkt fetch' supports, fetch with
meta discovery  ex: 'coreos.com/hello:latest', fetch from a specific
location ex: 'https://github.com/xxx/hello.aci' or fetch from docker
registry ex: 'docker://hello-world'.

Added new fetched plugin for Appc fetch algorithm. The new
AppcFetcherPlugin implements two methods. Method 'rktFetch' is used to
fetch a container image into a store specified at a directory location
using 'rkt fetch' command and method 'rktExport' exports the image
fetched into the directory location as a '.aci' file that is in tar
file format, usinf 'rkt image export' command.


Diffs (updated)
-

  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/uri/fetcher.hpp 949363ecdb15f5b225db7f3ed67eda52c351bfa1 
  src/uri/fetcher.cpp 198bd29993381758183edfce8faafba36da2d9ae 
  src/uri/fetchers/appc.hpp PRE-CREATION 
  src/uri/fetchers/appc.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51410: Enabled meta discovery using appc labels.

[Srinivas Brahmaroutu]

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

(Updated Aug. 25, 2016, 3:21 p.m.)


Review request for mesos, Gilbert Song and Jie Yu.


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


Repository: mesos


Description
---

Added appc_labels to mesos-execute so that we can indicate meta
discovery using useMetaDiscovery="true" along with other appc labels
that determines the os, arch of the image we are discovering. This logic
needs to be documented so that user can switch between simple and meta
discovery methods. This implementation is necessary as there is no other
way to determine from the URI.


Diffs
-

  src/cli/execute.cpp b752d057a3d86482ef1a4baaf31052469e38dc76 
  src/slave/containerizer/mesos/provisioner/appc/fetcher.cpp 
907e761856e8996b72e4231de27fa15e884d52e3 

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


Testing
---

make check


Thanks,

Srinivas Brahmaroutu

Re: Review Request 51043: Added support for reading out the bounding capability set.

[Benjamin Bannier]

> On Aug. 17, 2016, 8:20 p.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 229-231
> > 
> >
> > Hum, this is not very performant. If lastCap == 37, a single get() here 
> > will issue 37 system calls. Can you at least add a TODO to address it.
> > 
> > Looks like other library reads `/proc/self/status`:
> > 
> > https://github.com/syndtr/gocapability/blob/master/capability/capability_linux.go#L390-L414

I left the implementation as is for the moment as it has a very limited set of 
error cases. I added a `TODO` to possibly add a more performant implementation 
at a later point.


- Benjamin


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


On Aug. 25, 2016, 5:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51043/
> ---
> 
> (Updated Aug. 25, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 
> 
> Diff: https://reviews.apache.org/r/51043/diff/
> 
> 
> Testing
> ---
> 
> make check and sudo make check (Debian jessie, gcc-4.9.2, w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 51042: Added function to apply capabilities to process exec'd next.

[Benjamin Bannier]

> On Aug. 19, 2016, 1:36 a.m., Jie Yu wrote:
> > src/linux/capabilities.cpp, lines 297-313
> > 
> >
> > Should this just be a `set`?
> > 
> > Also, we should just assume we have SETPCAP as agent is typically 
> > running as root.

We want to be able to call this e.g., after changing the UID which always 
clears the effective set, so I left this code in.


- Benjamin


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


On Aug. 25, 2016, 5:18 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/51042/
> ---
> 
> (Updated Aug. 25, 2016, 5:18 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5051
> https://issues.apache.org/jira/browse/MESOS-5051
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/linux/capabilities.hpp fd7a571a62e3bbdd8a0453f800bf622457891bf8 
>   src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 
> 
> Diff: https://reviews.apache.org/r/51042/diff/
> 
> 
> Testing
> ---
> 
> make check and sudo make check (Debian jessie, gcc-4.9.2, w/o optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

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

[Benjamin Bannier]

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

(Updated Aug. 25, 2016, 5:20 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Simplified test helper implementation.


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


Repository: mesos


Description
---

This test is based on the work in https://reviews.apache.org/r/46371/.


Diffs (updated)
-

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

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


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Review Request 51429: Added proper constructor for internal SyscallPayload class and used it.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 

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


Testing
---

Tested with the full chain leading up to r/50271.


Thanks,

Benjamin Bannier

Re: Review Request 51043: Added support for reading out the bounding capability set.

[Benjamin Bannier]

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

(Updated Aug. 25, 2016, 5:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 

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


Testing
---

make check and sudo make check (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50271: Created an isolator for Linux capabilities.

[Benjamin Bannier]

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

(Updated Aug. 25, 2016, 5:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This isolator evaluates agent allowed capabilities and passes net
capabilities on to `mesos-containerizer` which enforces the
capabilities.

Capability information is passed via a new field in
`ContainerLaunchInfo`.


Diffs (updated)
-

  include/mesos/slave/containerizer.proto 
16dd3a19145b9764273cdb9a8899e353c98730e5 
  src/Makefile.am 69e56551f0adca6d6a9811cafea9a8d3c56d1df9 
  src/slave/containerizer/mesos/containerizer.cpp 
e87292f7dc3d51f8d4a5ff2e1b9e0090748cdee6 
  src/slave/containerizer/mesos/isolators/capabilities.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/capabilities.cpp PRE-CREATION 
  src/tests/containerizer/isolator_tests.cpp 
2725eb0f76f4e2668381c0d6686a99649f4f9f25 

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


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 50270: Introduced linux capabilities support for mesos containerizer.

[Benjamin Bannier]

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

(Updated Aug. 25, 2016, 5:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

This change introduces linux capability based security for unified
containerizer. A new agent flag \`allowed_capabilities\` has been
introduced to override the default capabilities of the user or the
capabilities requested by the user.

This feature is only available on linux.

This patch is based on https://reviews.apache.org/r/46798/.


Diffs (updated)
-

  src/launcher/executor.cpp 71ede1ea4f4e97fe94bd2bd136f17f231cedbce6 
  src/slave/flags.hpp 1a006663e7cc58ee548b3dda686cfbac0c240baa 
  src/slave/flags.cpp 0f2be1700f41b74da4ea1ce699a81ec33cf92a9a 

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


Testing
---

`make check` and `sudo make check` (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Re: Review Request 51042: Added function to apply capabilities to process exec'd next.

[Benjamin Bannier]

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

(Updated Aug. 25, 2016, 5:18 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Renamed & Rebased.


Summary (updated)
-

Added function to apply capabilities to process exec'd next.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/linux/capabilities.hpp fd7a571a62e3bbdd8a0453f800bf622457891bf8 
  src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 

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


Testing
---

make check and sudo make check (Debian jessie, gcc-4.9.2, w/o optimizations)


Thanks,

Benjamin Bannier

Review Request 51428: Removed friendship between ProcessCapabilities and Capabilities.

[Benjamin Bannier]

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

See summary.


Diffs
-

  src/linux/capabilities.hpp fd7a571a62e3bbdd8a0453f800bf622457891bf8 
  src/linux/capabilities.cpp e58cb981f9cf7880524114e94ba42cbacfe45630 

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


Testing
---

Tested with the full chain leading up to r/50271.


Thanks,

Benjamin Bannier

Re: Review Request 45964: Add unit tests for sharing of resources.

[Jiang Yan Xu]

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




src/tests/hierarchical_allocator_tests.cpp (lines 1269 - 1270)


This test specifically tests CREATE/DESTROY for shared volumes so let's be 
very specific:

```
// This test verifies that `updateAllocation()` supports creating and 
destroying shared persistent volumes.
```



src/tests/hierarchical_allocator_tests.cpp (line 1271)


s/UpdateShareAllocation/UpdateAllocationSharedPersistentVolume/



src/tests/hierarchical_allocator_tests.cpp (line 1277)


Use `{}` below.



src/tests/hierarchical_allocator_tests.cpp (line 1280)


i.e., use

```
allocator->addSlave(slave.id(), slave, None(), slave.resources(), {});
```



src/tests/hierarchical_allocator_tests.cpp (lines 1305 - 1306)


s/update1/update/



src/tests/hierarchical_allocator_tests.cpp (lines 1349 - 1350)


You don't need `update2`, you *expect* `update2` to be `slave.resources`, 
so just use `slave.resources`. We are not verifying `Resources::apply()` here 
in this test. `updated1` was necessary because you need it as the expected 
value.



src/tests/hierarchical_allocator_tests.cpp (lines 1376 - 1377)


Something more clear would be:

```
// The allocation should be the slave's original resources now that the 
volume is created and then destroyed.
```



src/tests/master_validation_tests.cpp (line 194)


It's not a volume? How about calling the test `UnshareableResource`?

The *shareability* concept comes in handy here.



src/tests/master_validation_tests.cpp (line 203)


s/Shareable/Shared/



src/tests/mesos.hpp (line 754)


The original comment was intended for the following three methods. Do you 
think we need to comment on each one? If so at least do it for LAUNCH as well 
and change plurals to singulars.



src/tests/persistent_volume_tests.cpp (line 547)


It's unfortunate that we duplicate all the code from 
`PreparePersistentVolume` with only two lines of change.

This applies to `AccessSharedPersistentVolume` as well. 

If we want to test whether shared volume works on the slave, we should 
create a test for the more interesting case: multiple tasks accessing the same 
persistent volume. This test would be both necessary and also would cover both 
"preparing" and "accessing" the shared persistent volume therefore we don't 
need to test them separately.



src/tests/persistent_volume_tests.cpp (line 586)


Add a trailing comment:

// Shared.



src/tests/resources_tests.cpp (line 790)


This test should be much simpler: just create a persistent volume using 
`createPersistentVolume()` and verify the output of one copy and two copies, 
and that's it.



src/tests/resources_tests.cpp (lines 2366 - 2367)


We don't use indentation style like this.

The following would be fine:

```
Resource volume1 = createDiskResource(
"200", "role", "1", "path", None(), true);
```

Here and below.



src/tests/resources_tests.cpp (lines 2378 - 2379)


Ditto.



src/tests/resources_tests.cpp (line 2390)


Also verify `nonShared()`?

Plus, there is no operation (therefore shouldn't be in 
ResourcesOperationTest)?



src/tests/resources_tests.cpp (line 2871)


This test came up before IIRC. I don't think we need this as the existing 
tests already have "multiple consumers".



src/tests/resources_tests.cpp (line 2905)


This is the same as "FilterSharedPersistentVolumes" test above? We need 
only one and the one above is simpler.


- Jiang Yan Xu


On Aug. 24, 2016, 9:15 p.m., Anindya Sinha wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45964/
> ---
> 
>