Re: Review Request 45746: Fixed the indent for continuation statements.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45746]

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

- Mesos ReviewBot


On April 5, 2016, 1:05 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45746/
> ---
> 
> (Updated April 5, 2016, 1:05 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed the indent for continuation statements.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> af5f49007e0afe57006c9db940611e56eb75a621 
> 
> Diff: https://reviews.apache.org/r/45746/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 37168: Add an example framework using dynamic reservation.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [37168]

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

- Mesos ReviewBot


On April 5, 2016, 9:28 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated April 5, 2016, 9:28 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Joerg Schad, and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 6ddac17bb2ac0330bcc09eaab975ae70e84a7695 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.

[Bernd Mathiske]

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


Ship it!




Ship It!

- Bernd Mathiske


On April 5, 2016, 6:27 a.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 5, 2016, 6:27 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the task does not pass validation,
> its resources are considered declined.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> 1d8bebde67f69fd414509b8861571137d3569b46 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> bf866f5ebece2505eaa27bf39a1382cd1a2a069a 
>   src/python/interface/src/mesos/interface/__init__.py 
> 232890daa6d222ae1c86906bbc484c8e635c4eb7 
> 
> Diff: https://reviews.apache.org/r/45039/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.

[Alexander Rukletsov]

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

(Updated April 5, 2016, 1:27 p.m.)


Review request for mesos, Ben Mahler and Neil Conway.


Repository: mesos


Description
---

If the task does not pass validation,
its resources are considered declined.


Diffs (updated)
-

  docs/app-framework-development-guide.md 
1d8bebde67f69fd414509b8861571137d3569b46 
  include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
  src/java/src/org/apache/mesos/SchedulerDriver.java 
bf866f5ebece2505eaa27bf39a1382cd1a2a069a 
  src/python/interface/src/mesos/interface/__init__.py 
232890daa6d222ae1c86906bbc484c8e635c4eb7 

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


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.

[Neil Conway]

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


Ship it!




Ship It!

- Neil Conway


On April 5, 2016, 1:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 5, 2016, 1:27 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the task does not pass validation,
> its resources are considered declined.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> 1d8bebde67f69fd414509b8861571137d3569b46 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> bf866f5ebece2505eaa27bf39a1382cd1a2a069a 
>   src/python/interface/src/mesos/interface/__init__.py 
> 232890daa6d222ae1c86906bbc484c8e635c4eb7 
> 
> Diff: https://reviews.apache.org/r/45039/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45370: Implemented prepare() for dvd isolator.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp (line 21)


Why need `` and `` here?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp (line 74)


A bit confusing why add `using` here? Because of line limit? How about
```
  multihashmap
containermountmap infos;
```



src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 62)


How about use `which` when it avaiable? 
https://issues.apache.org/jira/browse/MESOS-4576



src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 98)


`vector`? And add `using std::vector` above? And I think 
`vector` without `spec` would be better



src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 110)


```
foreach (const string& driverOption,
 volume.source().docker_volume().driver_options()) {
  driverOptions += " --volumeopts=" + driverOption;
}
```



src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp (line 133)


Add a line above.



src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp (line 29)


I think a method which accept these parameters are enough? This way looks 
more like Java.


- haosdent huang


On April 4, 2016, 9:21 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45370/
> ---
> 
> (Updated April 4, 2016, 9:21 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented prepare() for dvd isolator.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45370/diff/
> 
> 
> Testing
> ---
> 
> The execute.cpp is mainly for test, will remove this file once test finished.
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45690: Ensured the bind mount root is a shared mount in its own peer group.

[Cong Wang]

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


Ship it!




Ship It!

- Cong Wang


On April 5, 2016, 6:25 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45690/
> ---
> 
> (Updated April 5, 2016, 6:25 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Cong Wang.
> 
> 
> Bugs: MESOS-4662
> https://issues.apache.org/jira/browse/MESOS-4662
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is for the port mapping isolator.
> 
> This is a simple test I did on a fresh Fedora23 box:
> ```
> Jies-MacBook-Pro:fedora23 jie$ vagrant ssh
> Last login: Mon Apr  4 17:27:28 2016 from 10.0.2.2
> [vagrant@localhost ~]$ sudo mkdir /var/run/netns
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> ...
> [vagrant@localhost ~]$ sudo mount --bind /var/run/netns /var/run/netns
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> 72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
> rw,seclabel,mode=755
> ...
> [vagrant@localhost ~]$ sudo mount --make-shared /var/run/netns
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> 72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
> rw,seclabel,mode=755
> ...
> [vagrant@localhost ~]$ sudo touch /var/run/netns/$$ && sudo mount --bind 
> /proc/self/ns/net /var/run/netns/$$
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> 72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
> rw,seclabel,mode=755
> 74 72 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
> 75 23 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
> ```
> 
> As you can see above, a single bind mount creates two entries in the mount 
> table (`/run/netns/1526`). This is because /run/netns is in the same peer 
> group as /run. So a single mount operation under /run/netns will be 
> propergated to /run as well, creating two mounts. This will confuse the 
> recovery logic in the port mapping isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
> 
> Diff: https://reviews.apache.org/r/45690/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on Fedora 23
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45734: Fix 'pivot_root is not available' error on powerpc platform.

[Vinod Kone]

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


Ship it!




Ship It!

- Vinod Kone


On April 5, 2016, 9:25 a.m., Zhiwei Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45734/
> ---
> 
> (Updated April 5, 2016, 9:25 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: MESOS-5121
> https://issues.apache.org/jira/browse/MESOS-5121
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix 'pivot_root is not available' error on powerpc platform.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 306beab4c294e6a83e469279f38a8423dabd3f19 
> 
> Diff: https://reviews.apache.org/r/45734/diff/
> 
> 
> Testing
> ---
> 
> ../configure --enable-libevent --enable-ssl
> make -j60
> make dist check -j60
> 
> 
> Thanks,
> 
> Zhiwei Chen
> 
>

Re: Review Request 45455: Added test for recovering orphaned docker containers.

[Timothy Chen]

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


Fix it, then Ship it!




Ship It!


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


I think instead of just checking the previous inspect, can you make sure 
that the container is actually stopped by using exists?


- Timothy Chen


On March 29, 2016, 10:44 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45455/
> ---
> 
> (Updated March 29, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-3573
> https://issues.apache.org/jira/browse/MESOS-3573
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> b9c26b92c44e8ca718a5eb333855199bdf2a8e81 
> 
> Diff: https://reviews.apache.org/r/45455/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=100)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 45620: Adjusted the status semantics in CNI isolator.

[Jie Yu]

> On April 2, 2016, 7:25 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 817
> > 
> >
> > We should have a comment here explaining why we are not returning a 
> > failure. Primarily, because some containers might be on host network 
> > instead of CNI network.

Added a TODO. We should probably set ip addresses here for containers that join 
host network.


- Jie


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


On April 2, 2016, 6:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> ---
> 
> (Updated April 2, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch also does a few minor cleanups.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1173d5adee6b4eb45dbed5e693f40ffb5275210 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> af5f49007e0afe57006c9db940611e56eb75a621 
> 
> Diff: https://reviews.apache.org/r/45620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45620: Adjusted the status semantics in CNI isolator.

[Jie Yu]

> On April 5, 2016, 6:32 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 824
> > 
> >
> > Just wondering if there are any cases that 
> > `containerNetwork.networkInfo` is NONE? I think if we get here then 
> > `containerNetwork.networkInfo` must have been set, so maybe it should be a 
> > `CHECK(containerNetwork.networkInfo.isSome())`?

Agreed. Added a CHECK.


- Jie


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


On April 2, 2016, 6:44 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45620/
> ---
> 
> (Updated April 2, 2016, 6:44 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch also does a few minor cleanups.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b1173d5adee6b4eb45dbed5e693f40ffb5275210 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> af5f49007e0afe57006c9db940611e56eb75a621 
> 
> Diff: https://reviews.apache.org/r/45620/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[James Peach]

> On April 4, 2016, 5:35 p.m., haosdent huang wrote:
> > src/tests/cluster.cpp, line 437
> > 
> >
> > how about
> > 
> > ```
> > if (!containerizer) {
> >   return;
> > }
> > ```
> 
> Jiang Yan Xu wrote:
> +1 this is better.
> 
> James Peach wrote:
> You can't early return because you have to call ``terminate()``.
> 
> Jiang Yan Xu wrote:
> Alright, overlooked it :) However it looks like the lines above this 
> suffer from the same problem:
> 
> ```
> if (!cleanUpContainersInDestructor) {
>   return;
> }
> ```
> 
> In generl I think this is OK:
> 
> 
> How about 
> 
> ```
> if (!containerizer || !cleanUpContainersInDestructor) {
>   terminate();
>   return;
> }
> 
> ...
> ```

AFAICT skipping cleanup tasks when ``cleanUpContainersInDestructor`` is 
``false`` is by design.


- James


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


On April 4, 2016, 5:31 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 5:31 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45690: Ensured the bind mount root is a shared mount in its own peer group.

[Jie Yu]

> On April 5, 2016, 5:03 p.m., Cong Wang wrote:
> > src/slave/containerizer/mesos/isolators/network/port_mapping.cpp, line 1963
> > 
> >
> > It is still not clear why we need to handle this case, at least I don't 
> > see Fedora needs it. If it is really needed by some distro, please add some 
> > comments to explain this.

Updated the description with a simple test.


- Jie


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


On April 5, 2016, 5:25 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45690/
> ---
> 
> (Updated April 5, 2016, 5:25 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Cong Wang.
> 
> 
> Bugs: MESOS-4662
> https://issues.apache.org/jira/browse/MESOS-4662
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is for the port mapping isolator.
> 
> This is a simple test I did on a fresh Fedora23 box:
> ```
> Jies-MacBook-Pro:fedora23 jie$ vagrant ssh
> Last login: Mon Apr  4 17:27:28 2016 from 10.0.2.2
> [vagrant@localhost ~]$ sudo mkdir /var/run/netns
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> ...
> [vagrant@localhost ~]$ sudo mount --bind /var/run/netns /var/run/netns
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> 72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
> rw,seclabel,mode=755
> ...
> [vagrant@localhost ~]$ sudo touch /var/run/netns/$$ && sudo mount --bind 
> /proc/self/ns/net /var/run/netns/$$
> [vagrant@localhost ~]$ cat /proc/self/mountinfo
> ...
> 23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
> 72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
> rw,seclabel,mode=755
> 74 72 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
> 75 23 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
> ```
> 
> As you can see above, a single bind mount creates two entries in the mount 
> table (`/run/netns/1526`). This is because /run/netns is in the same peer 
> group as /run. So a single mount operation under /run/netns will be 
> propergated to /run as well, creating two mounts. This will confuse the 
> recovery logic in the port mapping isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
> 
> Diff: https://reviews.apache.org/r/45690/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on Fedora 23
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45690: Ensured the bind mount root is a shared mount in its own peer group.

[Jie Yu]

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

(Updated April 5, 2016, 5:25 p.m.)


Review request for mesos, Ian Downes and Cong Wang.


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


Repository: mesos


Description (updated)
---

This is for the port mapping isolator.

This is a simple test I did on a fresh Fedora23 box:
```
Jies-MacBook-Pro:fedora23 jie$ vagrant ssh
Last login: Mon Apr  4 17:27:28 2016 from 10.0.2.2
[vagrant@localhost ~]$ sudo mkdir /var/run/netns
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
...
[vagrant@localhost ~]$ sudo mount --bind /var/run/netns /var/run/netns
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
rw,seclabel,mode=755
...
[vagrant@localhost ~]$ sudo touch /var/run/netns/$$ && sudo mount --bind 
/proc/self/ns/net /var/run/netns/$$
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
rw,seclabel,mode=755
74 72 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
75 23 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
```

As you can see above, a single bind mount creates two entries in the mount 
table (`/run/netns/1526`). This is because /run/netns is in the same peer group 
as /run. So a single mount operation under /run/netns will be propergated to 
/run as well, creating two mounts. This will confuse the recovery logic in the 
port mapping isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
323c84a3d960a196d8ba87f753814e9d43a07957 

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


Testing
---

sudo make check on Fedora 23


Thanks,

Jie Yu

Re: Review Request 45726: Fixed a bug in the `flags::parse` function and added a test case.

[Michael Park]

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

(Updated April 5, 2016, 5:33 p.m.)


Review request for mesos and Jie Yu.


Changes
---

Added a test case for `Option` flag with an empty string as the value.


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


Repository: mesos


Description (updated)
---

Fixed a bug in the `flags::parse` function and added a test case.


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
6313b6b1f535822e1dee39f5780895a3691ff78c 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
e87bee24b8c57d15f3903ecd8d55e0e51e00f0ff 

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


Testing
---

Added a new test case that fails before the changes to `parse.hpp`.


Thanks,

Michael Park

Re: Review Request 45717: Added checks to verify that `rootDir` and `pluginDir` `isSome.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 5, 2016, 6:19 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45717/
> ---
> 
> (Updated April 5, 2016, 6:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-5113
> https://issues.apache.org/jira/browse/MESOS-5113
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> `rootDir` and `pluginDir` can be `None` in case the operator does not
> specify the network cni flags. In this case invoking `recover` on the
> isolator without checking the `rootDir` and `pluginDir` for `isSome`
> will cause the Agent to crash.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> af5f49007e0afe57006c9db940611e56eb75a621 
> 
> Diff: https://reviews.apache.org/r/45717/diff/
> 
> 
> Testing
> ---
> 
> make
> 
> Bought a slave and master and verified that the slave does not crash.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 45749: Adjusted lifetimes of member variables.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45749]

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

- Mesos ReviewBot


On April 5, 2016, 4:01 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45749/
> ---
> 
> (Updated April 5, 2016, 4:01 p.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-5000
> https://issues.apache.org/jira/browse/MESOS-5000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This correctly reflects the lifetime dependencies among
> Master::storage, Master::state and Master::registrar.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 
> 
> Diff: https://reviews.apache.org/r/45749/diff/
> 
> 
> Testing
> ---
> 
> Without this patch e.g., `MasterTest.MasterLost` fails under OS X with clang 
> (trunk, not optimized), and 
> `ResourceOffersTest.ResourceOfferWithMultipleSlaves` fails under Ubuntu14.04 
> with stock gcc (not optimized). With this patch they can be executed without 
> failure a couple thousand times.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45690: Ensured the bind mount root is a shared mount in its own peer group.

[Jie Yu]

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

(Updated April 5, 2016, 6:25 p.m.)


Review request for mesos, Ian Downes and Cong Wang.


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


Repository: mesos


Description (updated)
---

This is for the port mapping isolator.

This is a simple test I did on a fresh Fedora23 box:
```
Jies-MacBook-Pro:fedora23 jie$ vagrant ssh
Last login: Mon Apr  4 17:27:28 2016 from 10.0.2.2
[vagrant@localhost ~]$ sudo mkdir /var/run/netns
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
...
[vagrant@localhost ~]$ sudo mount --bind /var/run/netns /var/run/netns
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
rw,seclabel,mode=755
...
[vagrant@localhost ~]$ sudo mount --make-shared /var/run/netns
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
rw,seclabel,mode=755
...
[vagrant@localhost ~]$ sudo touch /var/run/netns/$$ && sudo mount --bind 
/proc/self/ns/net /var/run/netns/$$
[vagrant@localhost ~]$ cat /proc/self/mountinfo
...
23 58 0:20 / /run rw,nosuid,nodev shared:22 - tmpfs tmpfs rw,seclabel,mode=755
72 23 0:20 /netns /run/netns rw,nosuid,nodev shared:22 - tmpfs tmpfs 
rw,seclabel,mode=755
74 72 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
75 23 0:3 / /run/netns/1526 rw shared:28 - nsfs nsfs rw
```

As you can see above, a single bind mount creates two entries in the mount 
table (`/run/netns/1526`). This is because /run/netns is in the same peer group 
as /run. So a single mount operation under /run/netns will be propergated to 
/run as well, creating two mounts. This will confuse the recovery logic in the 
port mapping isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
323c84a3d960a196d8ba87f753814e9d43a07957 

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


Testing
---

sudo make check on Fedora 23


Thanks,

Jie Yu

Re: Review Request 45726: Fixed a bug in the `flags::parse` function and added a test case.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 5, 2016, 5:33 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45726/
> ---
> 
> (Updated April 5, 2016, 5:33 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5114
> https://issues.apache.org/jira/browse/MESOS-5114
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a bug in the `flags::parse` function and added a test case.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
> 6313b6b1f535822e1dee39f5780895a3691ff78c 
>   3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
> e87bee24b8c57d15f3903ecd8d55e0e51e00f0ff 
> 
> Diff: https://reviews.apache.org/r/45726/diff/
> 
> 
> Testing
> ---
> 
> Added a new test case that fails before the changes to `parse.hpp`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45214: Updated protobuf to support external storage.

[haosdent huang]

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




include/mesos/mesos.proto (line 1604)


How about use `repeated Parameter` as type here?


- haosdent huang


On April 4, 2016, 8:39 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45214/
> ---
> 
> (Updated April 4, 2016, 8:39 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5012
> https://issues.apache.org/jira/browse/MESOS-5012
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated protobuf to support external storage.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto e1fc02e05df531e29601c6764a5a48ba2b18569f 
>   include/mesos/v1/mesos.proto 35789e051608ea7f1be3ba5b63eaa1fc4e501c84 
> 
> Diff: https://reviews.apache.org/r/45214/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45749: Adjusted lifetimes of member variables.

[Joseph Wu]

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


Ship it!




Ship It!

- Joseph Wu


On April 5, 2016, 9:01 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45749/
> ---
> 
> (Updated April 5, 2016, 9:01 a.m.)
> 
> 
> Review request for mesos, Joseph Wu and Michael Park.
> 
> 
> Bugs: MESOS-5000
> https://issues.apache.org/jira/browse/MESOS-5000
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This correctly reflects the lifetime dependencies among
> Master::storage, Master::state and Master::registrar.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 
> 
> Diff: https://reviews.apache.org/r/45749/diff/
> 
> 
> Testing
> ---
> 
> Without this patch e.g., `MasterTest.MasterLost` fails under OS X with clang 
> (trunk, not optimized), and 
> `ResourceOffersTest.ResourceOfferWithMultipleSlaves` fails under Ubuntu14.04 
> with stock gcc (not optimized). With this patch they can be executed without 
> failure a couple thousand times.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45731: Introduced '--networks' flag to mesos-execute.

[Avinash sridharan]

> On April 5, 2016, 6:25 a.m., Gilbert Song wrote:
> > src/cli/execute.cpp, line 152
> > 
> >
> > Should we mention that an image has to be provided && for mesos 
> > containerizer only?
> 
> Avinash sridharan wrote:
> Why do we need an image for this? Also, the `name` field is used to 
> configure user-defined networks for `DockerContainerizer` as well 
> (MESOS-4369).
> 
> Qian Zhang wrote:
> Agree with Avinash, a container without image can also join the specified 
> networks. However Gilbert's comment reveals some potential issues in 
> `mesos-execute`.
> 
> The currently logic 
> (https://github.com/apache/mesos/blob/0.28.0/src/cli/execute.cpp#L233:L263) 
> in `mesos-execute` to launch task is:
> 1. Check if `--docker_image` is specified.
> 2. If it is specfied, then construct `ContainerInfo` based on 
> `--containerizer` which can be `mesos` or `docker`.
> 
> The problems I see with this logic are:
> 1. If `--docker_image` is not specified, then we will NOT construct 
> `ContainerInfo` at all. This will not be correct when we introduce this 
> `--networks` flag, because `mesos-execute` should be able to launch a 
> container joins some networks but without an image.
> 2. If `--docker_image` is not specified and `--containerizer` is 
> `docker`, then the result is we will launch a task with `MesosContainerizer` 
> which is not expected since what user specifies is `DockerContainerizer`. And 
> if `MesosContainerizer` is not enabled in agent (e.g., start agent with 
> `--containerizers=docker`), then the container will be failed to create with 
> an error like: `None of the enabled containerizers (docker) could create a 
> container for the provided TaskInfo/ExecutorInfo message`. So I think in this 
> case, we should guard it in `mesos-execute` rather than leaving it to agent, 
> i.e., `mesos-execute` should exit with an error message to let user know if 
> s/he choose `--containerizer=docker`, then s/he must specify `--docker_image` 
> as well.
> 
> To resolve the above issues, I have updated the patch with a new logic, 
> please review it and let me know for any comments.

Had a discussion with Gilbert. Qian, thanks for the explanation. Based on this 
I think Gilbert's comments are valid, since we can't use the --networks 
attribute without the image. We should probably throw an error in --networks is 
specified but --docker_image is not specified.


- Avinash


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


On April 5, 2016, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> ---
> 
> (Updated April 5, 2016, 9:13 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced '--networks' flag to mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45731/diff/
> 
> 
> Testing
> ---
> 
> Ran the following command:
>   `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test 
> --docker_image=library/busybox --command="sleep 30" --shell=true 
> --networks=net1,net2`
> 
> The created container successfully joined the CNI networks `net1` and `net2`.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45594: Introduced a new agent flag docker_config.

[Timothy Chen]

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




src/slave/flags.cpp (line 136)


Can you probably elaborate a bit more why a docker config file 
configuration is needed? 

I know it's most likely for pulling private registry docker files, but not 
quite obvious just by reading the help text.


- Timothy Chen


On April 5, 2016, 7:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45594/
> ---
> 
> (Updated April 5, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-5065
> https://issues.apache.org/jira/browse/MESOS-5065
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a new agent flag docker_config.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
>   src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
>   src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 
> 
> Diff: https://reviews.apache.org/r/45594/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45773: Updated the CHANGELOG for 0.28.1.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On April 5, 2016, 8:13 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45773/
> ---
> 
> (Updated April 5, 2016, 8:13 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Joris Van Remoortere, Michael Park, and 
> Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated the CHANGELOG for 0.28.1.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 6309b0f46d6fc53d483758e6121dabd1b4cfbb92 
> 
> Diff: https://reviews.apache.org/r/45773/diff/
> 
> 
> Testing
> ---
> 
> N/A
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45493: Added namespace option to subprocess [3/5].

[Joris Van Remoortere]

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



What is the high-level reasoning for making these namespace specific flags, as 
opposed to clone flags?


3rdparty/libprocess/include/process/subprocess.hpp (lines 338 - 355)


Can you add a `TODO` here to consider making this `clone_flags` as opposed 
to `namespaces`?


- Joris Van Remoortere


On March 31, 2016, 10:22 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45493/
> ---
> 
> (Updated March 31, 2016, 10:22 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5071
> https://issues.apache.org/jira/browse/MESOS-5071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows namespaces flags to be passed to subprocess.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 8a3fe5526f480187441a8aee2c72636bec3e2b2d 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
> 
> Diff: https://reviews.apache.org/r/45493/diff/
> 
> 
> Testing
> ---
> 
> Tested entire chain see https://reviews.apache.org/r/45495/.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45529: Do not leak roleSorter and quotaRoleSorter in Mesos allocator.

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/master/allocator/mesos/hierarchical.cpp (lines 611 - 612)


Mind adding a `CHECK(frameworkSorters.contains(role))` here?


- Joris Van Remoortere


On April 1, 2016, 10:35 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45529/
> ---
> 
> (Updated April 1, 2016, 10:35 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5073
> https://issues.apache.org/jira/browse/MESOS-5073
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Do not leak roleSorter and quotaRoleSorter in Mesos allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> e979fdf60da1409d1c2d08f0e9f03cef067506dd 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
> 
> Diff: https://reviews.apache.org/r/45529/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang trunk, not optimized)
> 
> In https://reviews.apache.org/r/45534/ I add code requiring cleanup to 
> `Sorter`s. Without this patch the cleanup code is never executed, but is with 
> the patch.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 45595: Implemented slave default docker config file support.

[Gilbert Song]

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

(Updated April 5, 2016, 12:22 p.m.)


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


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


Repository: mesos


Description
---

Implemented slave default docker config file support.


Diffs (updated)
-

  src/docker/docker.hpp 7802f23585121d41c738cc28f6bcfa5e6dc9e972 
  src/docker/docker.cpp 4d35513cdd9c044d37d876a6db7dd9321ceaca53 
  src/slave/containerizer/docker.cpp 6a8a705186b3a309329825a6b75ad038c2db4ac1 
  src/tests/mesos.hpp dc0e4dd6341f7d1931f3871991f6a3f20a477159 
  src/tests/mesos.cpp 543b2a7f3d1a4da28a5901fb238285cea6778c2b 

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


Testing
---

make check

sudo ./bin/mesos-test.sh

Tested with a private repo using ./mesos-execute


Thanks,

Gilbert Song

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Joseph Wu]

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




src/tests/cluster.cpp (lines 364 - 366)


The destructor will only dereference a null `containerizer` if this error 
case is hit (or if you pass in a `nullptr` cast to `slave::Containerizer*`).

At this point in the code, we haven't done anything other than create some 
objects.  So it should be ok to do something like this in the destructor:
```
if (containerizer == nullptr) {
  return;
}
```
The call to `terminate()` only matters if the `cluster::Slave` is created 
successfully.


- Joseph Wu


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Joseph Wu]

> On April 4, 2016, 10:35 a.m., haosdent huang wrote:
> > src/tests/cluster.cpp, line 437
> > 
> >
> > how about
> > 
> > ```
> > if (!containerizer) {
> >   return;
> > }
> > ```
> 
> Jiang Yan Xu wrote:
> +1 this is better.
> 
> James Peach wrote:
> You can't early return because you have to call ``terminate()``.
> 
> Jiang Yan Xu wrote:
> Alright, overlooked it :) However it looks like the lines above this 
> suffer from the same problem:
> 
> ```
> if (!cleanUpContainersInDestructor) {
>   return;
> }
> ```
> 
> In generl I think this is OK:
> 
> 
> How about 
> 
> ```
> if (!containerizer || !cleanUpContainersInDestructor) {
>   terminate();
>   return;
> }
> 
> ...
> ```
> 
> James Peach wrote:
> AFAICT skipping cleanup tasks when ``cleanUpContainersInDestructor`` is 
> ``false`` is by design.

Haosdent's suggestion works in this case.  See explanation in my review posted 
below.


- Joseph


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


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Review Request 45768: Fixed commit message hook to skip over the commented lines.

[Michael Park]

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

Review request for mesos, Joerg Schad and Vinod Kone.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  support/hooks/commit-msg d3d5415bf6a243def05cf79f4af2b136b9866d68 

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


Testing
---

Was able to commit a patch where there were lines longer than 72 characters in 
the commented section.


Thanks,

Michael Park

Re: Review Request 45768: Fixed commit message hook to skip over the commented lines.

[Kevin Klues]

> On April 5, 2016, 8:01 p.m., Kevin Klues wrote:
> > support/hooks/commit-msg, line 20
> > 
> >
> > You should probably quote the $LINE variable here. You should also use 
> > a single "=", not "==". The double equals will not work on all versions of 
> > sh (most notably on mac os).
> > I would also just check the first character to equal "#" instead of 
> > comaping the whole line.
> > 
> > Also, when you hit a line with a comment, you probably shouldn't break. 
> > Instead you should continue (in case there is more text to include in the 
> > commit message later on).
> > 
> > The new loop would look like:
> > 
> > ```
> > while read LINE
> > do
> > if [[ "${LINE:0:1}" == "#" ]]; then continue; fi
> > LENGTH=$(echo $LINE | wc -c)
> > if [ "$LENGTH" -gt "73" ]; then
> > echo >&2 "Error: No line in the commit message summary may 
> > exceed 72 characters."
> > exit 1
> > fi
> > ```

Whoops, I meant the following (this actually has the single "=" and uses single 
"[" "]"'s around the if instead of "[[", which is also non-portable.

```
while read LINE
do
if [ "${LINE:0:1}" = "#" ]; then continue; fi
LENGTH=$(echo $LINE | wc -c)
if [ "$LENGTH" -gt "73" ]; then
echo >&2 "Error: No line in the commit message summary may exceed 72 
characters."
exit 1
fi
done
```


- Kevin


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


On April 5, 2016, 7:50 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45768/
> ---
> 
> (Updated April 5, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Bugs: MESOS-5126
> https://issues.apache.org/jira/browse/MESOS-5126
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/hooks/commit-msg d3d5415bf6a243def05cf79f4af2b136b9866d68 
> 
> Diff: https://reviews.apache.org/r/45768/diff/
> 
> 
> Testing
> ---
> 
> Was able to commit a patch where there were lines longer than 72 characters 
> in the commented section.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Review Request 45773: Updated the CHANGELOG for 0.28.1.

[Jie Yu]

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

Review request for mesos, Ben Mahler, Joris Van Remoortere, Michael Park, and 
Vinod Kone.


Repository: mesos


Description
---

Updated the CHANGELOG for 0.28.1.


Diffs
-

  CHANGELOG 6309b0f46d6fc53d483758e6121dabd1b4cfbb92 

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


Testing
---

N/A


Thanks,

Jie Yu

Re: Review Request 45491: Refactored subprocess options [1/5].

[Joris Van Remoortere]

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




3rdparty/libprocess/include/process/subprocess.hpp (line 80)


Please don't include random fixes in reviews. It makes the review / history 
confusing.



3rdparty/libprocess/include/process/subprocess.hpp (lines 178 - 216)


Why the inconsistency between a call operator and a friendship?



3rdparty/libprocess/include/process/subprocess.hpp (line 184)


backticks around `ChildHook`



3rdparty/libprocess/include/process/subprocess.hpp (line 211)


Do we use this `operator() ()` style elsewhere?



3rdparty/libprocess/include/process/subprocess.hpp (line 218)


Why do we need to friend if we've provided a call operator?



3rdparty/libprocess/include/process/subprocess.hpp 


why did you get rid of this whitespace?



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


2 new lines between function definitions



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


Taking the working directory by reference is extremely dangerous here. Why 
do it? Does the style guide default of `[=]` not work?



3rdparty/libprocess/src/subprocess.cpp (lines 63 - 64)


I don't understand how this comment applies to the `chdir` hook?



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


2 new lines between function definitions


- Joris Van Remoortere


On March 31, 2016, 10:21 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45491/
> ---
> 
> (Updated March 31, 2016, 10:21 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5070
> https://issues.apache.org/jira/browse/MESOS-5070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously the subprocess interface supported a several options for the
> child process such as setsid. In order to make the interface more
> flexible we refactored such options into a vector of ChildHooks.
> In order not to allow arbitrary code inside a ChildHook it has to be
> constructed via pre-defined factory methods.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/ssl/gtest.hpp 
> 5435ddda1fd7dfcff1a0b28f2abe35feb707ceeb 
>   3rdparty/libprocess/include/process/subprocess.hpp 
> 8a3fe5526f480187441a8aee2c72636bec3e2b2d 
>   3rdparty/libprocess/src/subprocess.cpp 
> bb0fcbcd0dfa455c8700247c5b4ca0473fd163c3 
>   3rdparty/libprocess/src/tests/subprocess_tests.cpp 
> 727e940f12643974de4ff2734fba431b285b5de3 
> 
> Diff: https://reviews.apache.org/r/45491/diff/
> 
> 
> Testing
> ---
> 
> Tested entire chain see https://reviews.apache.org/r/45495/.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45495: Removed custom clone functions from Mesos [5/5].

[Joris Van Remoortere]

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




src/slave/containerizer/mesos/linux_launcher.cpp (line 279)


`SIGCHLD` is not a namespace flag, right?
See my earlier comment about renaming the `namespace` flags.


- Joris Van Remoortere


On March 31, 2016, 10:24 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45495/
> ---
> 
> (Updated March 31, 2016, 10:24 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5071
> https://issues.apache.org/jira/browse/MESOS-5071
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> All existing clone calls can be specified by providing namespace flags
> to subprocess directly.
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 749e676aaf2ce639dd976f2b23e323300c6114c5 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1f228806da32832c9ca1ae4defcd1bdc154adc18 
>   src/slave/containerizer/docker.cpp c5007a311ae9c1766dd4522ccbddbdb506d4ae4e 
>   src/slave/containerizer/external_containerizer.cpp 
> cf4384cce44172a028c890f52f71ceb8ae109383 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> dde958b252eae75563003ec15087b4231beb285a 
>   src/slave/containerizer/mesos/launcher.cpp 
> a5c8c31b72773d0bd10b9d02675a01f1d641d41c 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launch_tests.cpp 
> 3e36f2f7ab89b98de2c1a971e4ecca58c13ad642 
>   src/tests/containerizer/ns_tests.cpp 
> cd668ebb3b9461bee00dc338c288e5df6eb8fe31 
> 
> Diff: https://reviews.apache.org/r/45495/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> ../configure --with-network-isolator
> make check
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45594: Introduced a new agent flag docker_config.

[Gilbert Song]

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

(Updated April 5, 2016, 12:21 p.m.)


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


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


Repository: mesos


Description
---

Introduced a new agent flag docker_config.


Diffs
-

  docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45596: Updated docker containerizer private registry doc.

[Gilbert Song]

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

(Updated April 5, 2016, 12:34 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated docker containerizer private registry doc.


Diffs (updated)
-

  docs/docker-containerizer.md 865b2cf0e158b1aab6c384d26ab7a3d9800610ac 

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


Testing
---

gist


Thanks,

Gilbert Song

Re: Review Request 45595: Implemented slave default docker config file support.

[Gilbert Song]

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

(Updated April 5, 2016, 12:34 p.m.)


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


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


Repository: mesos


Description
---

Implemented slave default docker config file support.


Diffs (updated)
-

  src/docker/docker.hpp 7802f23585121d41c738cc28f6bcfa5e6dc9e972 
  src/docker/docker.cpp 4d35513cdd9c044d37d876a6db7dd9321ceaca53 
  src/slave/containerizer/docker.cpp 6a8a705186b3a309329825a6b75ad038c2db4ac1 
  src/tests/mesos.hpp dc0e4dd6341f7d1931f3871991f6a3f20a477159 
  src/tests/mesos.cpp 543b2a7f3d1a4da28a5901fb238285cea6778c2b 

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


Testing
---

make check

sudo ./bin/mesos-test.sh

Tested with a private repo using ./mesos-execute


Thanks,

Gilbert Song

Re: Review Request 45492: Used ChildHooks in Mesos [2/5].

[Joris Van Remoortere]

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


Fix it, then Ship it!





src/slave/containerizer/docker.cpp (lines 1223 - 1224)


```
{A,
 B}
```



src/slave/containerizer/external_containerizer.cpp (lines 1091 - 1092)


why not inline this?


- Joris Van Remoortere


On March 31, 2016, 10:21 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45492/
> ---
> 
> (Updated March 31, 2016, 10:21 a.m.)
> 
> 
> Review request for mesos and Joris Van Remoortere.
> 
> 
> Bugs: MESOS-5070
> https://issues.apache.org/jira/browse/MESOS-5070
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We now use the new ChildHooks instead of explicit options such
> as setsid.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.cpp 386f2f4afa51b3a78a3f0b4711018b38c4513e7b 
>   src/health-check/main.cpp 98ea5d3675f088e3a341037dcee92695e4857999 
>   src/linux/perf.cpp 749e676aaf2ce639dd976f2b23e323300c6114c5 
>   src/slave/container_loggers/lib_logrotate.cpp 
> 1f228806da32832c9ca1ae4defcd1bdc154adc18 
>   src/slave/containerizer/docker.cpp c5007a311ae9c1766dd4522ccbddbdb506d4ae4e 
>   src/slave/containerizer/external_containerizer.cpp 
> cf4384cce44172a028c890f52f71ceb8ae109383 
>   src/slave/containerizer/fetcher.cpp 
> 0992112a3d0f122915a3e7636de17c992610832f 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
>   src/slave/containerizer/mesos/isolators/posix/disk.cpp 
> dde958b252eae75563003ec15087b4231beb285a 
>   src/slave/containerizer/mesos/launcher.cpp 
> a5c8c31b72773d0bd10b9d02675a01f1d641d41c 
>   src/slave/containerizer/mesos/linux_launcher.cpp 
> 5028854fa003615f158120e030866b7ec4402b66 
>   src/tests/containerizer/launch_tests.cpp 
> 3e36f2f7ab89b98de2c1a971e4ecca58c13ad642 
>   src/tests/containerizer/ns_tests.cpp 
> cd668ebb3b9461bee00dc338c288e5df6eb8fe31 
>   src/tests/containerizer/port_mapping_tests.cpp 
> e062daa9fcfc776144b48325daa1f1284c5e59a4 
>   src/tests/slave_tests.cpp 57fc50360eae85819ae6ce714b0c3c4c1867b2b8 
> 
> Diff: https://reviews.apache.org/r/45492/diff/
> 
> 
> Testing
> ---
> 
> Tested entire chain see https://reviews.apache.org/r/45495/.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45594: Introduced a new agent flag docker_config.

[Gilbert Song]

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

(Updated April 5, 2016, 12:20 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Introduced a new agent flag docker_config.


Diffs (updated)
-

  docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
  src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
  src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45768: Fixed commit message hook to skip over the commented lines.

[Kevin Klues]

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




support/hooks/commit-msg (line 20)


You should probably quote the $LINE variable here. You should also use a 
single "=", not "==". The double equals will not work on all versions of sh 
(most notably on mac os).
I would also just check the first character to equal "#" instead of 
comaping the whole line.

Also, when you hit a line with a comment, you probably shouldn't break. 
Instead you should continue (in case there is more text to include in the 
commit message later on).

The new loop would look like:

```
while read LINE
do
if [[ "${LINE:0:1}" == "#" ]]; then continue; fi
LENGTH=$(echo $LINE | wc -c)
if [ "$LENGTH" -gt "73" ]; then
echo >&2 "Error: No line in the commit message summary may exceed 
72 characters."
exit 1
fi
```


- Kevin Klues


On April 5, 2016, 7:50 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45768/
> ---
> 
> (Updated April 5, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Bugs: MESOS-5126
> https://issues.apache.org/jira/browse/MESOS-5126
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/hooks/commit-msg d3d5415bf6a243def05cf79f4af2b136b9866d68 
> 
> Diff: https://reviews.apache.org/r/45768/diff/
> 
> 
> Testing
> ---
> 
> Was able to commit a patch where there were lines longer than 72 characters 
> in the commented section.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 45381: Migrate /monitor/statistics and /monitor/statistics.json to slave.

[Jay Guo]

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

(Updated April 6, 2016, 5:33 a.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

These two endpoints and their underlying logics are moved from
ResourceMonitorProcess to slave process. ResourceMonitor is removed.


Diffs (updated)
-

  src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
  src/Makefile.am 55d3b341361bed25f3aa966d77060c88be29e5b0 
  src/slave/http.cpp 7a872c6e21b115500c5291b7cb9fb240ec9dc8ed 
  src/slave/monitor.hpp 70a7c88fefd4577f53f85e3eccdd6b69ab6f3acd 
  src/slave/monitor.cpp 5c1dd354595e67e5eb603c5d49850f84b84b 
  src/slave/slave.hpp 3ba335fcd31a92af9609023daae328b7f4bf5e59 
  src/slave/slave.cpp 60f93ca2c6251190d5dadfd543f0e5c5fbee1214 
  src/tests/limiter.hpp baf396c138113abbf984879fbc2e8fade5e9feac 
  src/tests/mesos.hpp 3b565b45f45c84aba42aa6fb29b21f8306c49861 
  src/tests/mesos.cpp cf38dbb05908800b3a771318fa6922548c86c1f2 
  src/tests/monitor_tests.cpp 5dcb2481ff2f1a7caf54036bc3e60c78feb982b1 
  src/tests/oversubscription_tests.cpp ba036810758d99a6fb0034c5e2bc7829e2343a44 
  src/tests/slave_tests.cpp 57fc50360eae85819ae6ce714b0c3c4c1867b2b8 

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


Testing
---

make check

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


Thanks,

Jay Guo

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

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




src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 33)


Doesn't look like the utils header is used in this header?

Otherwise add a blank line above.



src/slave/containerizer/mesos/isolators/xfs/disk.hpp (line 100)


const Flags flags;



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 57 - 58)


We should probably also validate the upper bound of the ranges because they 
are `uint64`.

```
static Try getIntervalSet(
const Value::Ranges& ranges)
```

?



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 112 - 114)


There is also the `uid.isNone()` case.

We can just 

```
if (!user.isSome()) {
  return Error("Failed to determine user: " +
   (uid.isError() ? uid.error() : "uid not found"));
}
```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 140 - 144)


I suggested this earlier and this was marked as resolved:

If the utils provdes 

```
Option validateProjectIds(IntervalSet);
```

Then the isolator doesn't need to know about `NON_PROJECT_ID` anymore and 
it can stay as a constant in utils.cpp.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 172 - 199)


We can consolidate this with the orphan case because in both cases we need 
to do all of these to read its project ID and construct an `info`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 216 - 217)


This fits in one line.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 222)


IMO it's pretty clean and explicit to consolicate the recovery for both 
live containers and known orphans if we add comments here to make it clear that

```
// We recover the sandboxes for both live containers and known orphans 
// and only clean up the unknown orphans here.
```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 231 - 234)


This case shouldn't happen anymore if we consolidate. We can do a CHECK 
here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 237)


In the code above you had

```
// We should always be able to get a project ID. Maybe the directory
// is not on an XFS filesystem, or something equally fatal happened.
```

And you return a failure. I think this applies here as well regarless of 
whether the sandbox is alive or known/unkown orphans because this indicates 
problems that affect more than just this one container.

We should return a failure here as well.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 238 - 239)


Single quote the sandbox.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 239)


Move one space to the right.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 243)


Let's move over the comments you had above.

```
// If there is no project ID, don't worry about it. This can happen the
// first time an operator enables the XFS disk isolator and we recover a
// set of containers that we did not isolate.
```



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 253)


I don't think we need to wait for the result of the unknown orphan recovery 
because the cleanup could be expensive as it processes every file in the 
sandbox.

We can call `cleanup(containerId)` for every unknown orphan without 
collecting them. At the bottom we just `return Nothing()`.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (line 258)


2 space indentation here.



src/slave/containerizer/mesos/isolators/xfs/disk.cpp (lines 281 - 286)


This is sort of safe in our case because we expect an empty sandbox. 

In a general case `xfs::setProjectId()` could partially fail which makes it 
unsafe to return the project ID.

I think we can just do a hard CHECK before 

Re: Review Request 45360: Added dvd client for mount and unmount.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 28)


I think add this here looks a bit wired.
```
find . -name '*.hpp'|xargs grep 'using namespace'
./3rdparty/libprocess/3rdparty/stout/include/stout/lambda.hpp:using 
namespace std::placeholders;
./3rdparty/libprocess/include/process/pid.hpp: *using namespace process;
./src/slave/slave.hpp:using namespace process;
```

How about move the class to `messo::internal::slave::dvd` namespace?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp (line 42)


I suggest to follow `doxgen-style-guide.md` to add comments to functions.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 56)


s/how does dvdcli works/how dvdcli works/g.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 58)


I think we could use `strings::format` here?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 59)


Do we need check `dvdcli` version or something first?



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 65)


I think could make it more excatly as well. `s/Mount command/Execute Docker 
Volume Driver mount command:`.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 74)


I suggest use `return Failure("Failed to mount Docker Volume Driver:" +  + 
s.error())`



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 111)


A bit confuse why we continue to launch after unmount.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 125)


I think this msg isn't clear. Actually we failed to launch DVD, but this 
message looks like we failed in some general methods like `os::shell`. Maybe we 
could make it more specific.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 131)


Add a line above.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 146)


I suggest to replace `const Owned& externalMount,` to 
`Owned& externalMount,` which make it more matched.



src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp (line 148)


I think you want to log `externalMount` here. You could 
`jsonify(JSON::Protobuf(*externalMount))` directly.


- haosdent huang


On April 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Review Request 45749: Adjusted lifetimes of member variables.

[Benjamin Bannier]

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

Review request for mesos, Joseph Wu and Michael Park.


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


Repository: mesos


Description
---

This correctly reflects the lifetime dependencies among
Master::storage, Master::state and Master::registrar.


Diffs
-

  src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 

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


Testing
---

Without this patch e.g., `MasterTest.MasterLost` fails under OS X with clang 
(trunk, not optimized), and 
`ResourceOffersTest.ResourceOfferWithMultipleSlaves` fails under Ubuntu14.04 
with stock gcc (not optimized). With this patch they can be executed without 
failure a couple thousand times.


Thanks,

Benjamin Bannier

Re: Review Request 45381: Migrate /monitor/statistics and /monitor/statistics.json to slave.

[Jay Guo]

> On March 31, 2016, 7:14 p.m., Jie Yu wrote:
> > src/slave/slave.hpp, line 550
> > 
> >
> > Can you move this field to Http class because this is a limiter for the 
> > http endpoint. Also, `limiter` sounds too general. Maybe rename it?
> > 
> > ```
> > class Http {
> > private:
> >   Slave* slave;
> >   RateLimiter statisticsLimiter;
> > }
> > 
> > ```
> 
> Jay Guo wrote:
> Renamed it.
> 
> Although there is a problem moving it to http class: `Http http = 
> Http(this);` yields compilation error since RateLimiter class copy operator 
> is deleted.
> 
> Jie Yu wrote:
> You can use Shared instead.

`Shared` takes a pointer to RateLimiter, which needs to be freed 
properly, as well as its underlying `RateLimiterProcess`. I get some 
segmentation fault with it. Some hints would be appreciated. While I'm trying 
to figure it out, there is another concern that `RateLimiter::acquire()` is not 
`const`, yet `Shared` enforces `const`. Is `const_cast` a good practice in 
Mesos codebase?


- Jay


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


On April 2, 2016, 10:31 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45381/
> ---
> 
> (Updated April 2, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two endpoints and their underlying logics are moved from
> ResourceMonitorProcess to slave process. ResourceMonitor is removed.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/http.cpp 7a872c6e21b115500c5291b7cb9fb240ec9dc8ed 
>   src/slave/monitor.hpp 70a7c88fefd4577f53f85e3eccdd6b69ab6f3acd 
>   src/slave/monitor.cpp 5c1dd354595e67e5eb603c5d49850f84b84b 
>   src/slave/slave.hpp 3ba335fcd31a92af9609023daae328b7f4bf5e59 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
>   src/tests/mesos.hpp 98f97101d89cb91ca5e773e7a91f04ee1b8a0d88 
>   src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
>   src/tests/monitor_tests.cpp 5dcb2481ff2f1a7caf54036bc3e60c78feb982b1 
>   src/tests/oversubscription_tests.cpp 
> ba036810758d99a6fb0034c5e2bc7829e2343a44 
>   src/tests/slave_tests.cpp 57fc50360eae85819ae6ce714b0c3c4c1867b2b8 
> 
> Diff: https://reviews.apache.org/r/45381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> https://issues.apache.org/jira/browse/MESOS-4891
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 45039: Updated the comment for launching tasks and accepting offers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45039]

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

- Mesos ReviewBot


On April 5, 2016, 1:27 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 5, 2016, 1:27 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the task does not pass validation,
> its resources are considered declined.
> 
> 
> Diffs
> -
> 
>   docs/app-framework-development-guide.md 
> 1d8bebde67f69fd414509b8861571137d3569b46 
>   include/mesos/scheduler.hpp 14c7ff964aa7b94f439d16e605380661d2279d54 
>   src/java/src/org/apache/mesos/SchedulerDriver.java 
> bf866f5ebece2505eaa27bf39a1382cd1a2a069a 
>   src/python/interface/src/mesos/interface/__init__.py 
> 232890daa6d222ae1c86906bbc484c8e635c4eb7 
> 
> Diff: https://reviews.apache.org/r/45039/diff/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 45620: Adjusted the status semantics in CNI isolator.

[Jie Yu]

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

(Updated April 5, 2016, 4:45 p.m.)


Review request for mesos, Avinash sridharan and Qian Zhang.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

This patch also does a few minor cleanups.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b1173d5adee6b4eb45dbed5e693f40ffb5275210 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
af5f49007e0afe57006c9db940611e56eb75a621 

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


Testing
---

make check


Thanks,

Jie Yu

Re: Review Request 45690: Ensured the bind mount root is a shared mount in its own peer group.

[Cong Wang]

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




src/slave/containerizer/mesos/isolators/network/port_mapping.cpp (line 1963)


It is still not clear why we need to handle this case, at least I don't see 
Fedora needs it. If it is really needed by some distro, please add some 
comments to explain this.


- Cong Wang


On April 4, 2016, 5:30 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45690/
> ---
> 
> (Updated April 4, 2016, 5:30 p.m.)
> 
> 
> Review request for mesos, Ian Downes and Cong Wang.
> 
> 
> Bugs: MESOS-4662
> https://issues.apache.org/jira/browse/MESOS-4662
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is for the port mapping isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
> 
> Diff: https://reviews.apache.org/r/45690/diff/
> 
> 
> Testing
> ---
> 
> sudo make check on Fedora 23
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 42551: Porting mesos on ppc64le.

[Vinod Kone]

> On March 24, 2016, 2:14 p.m., Mesos ReviewBot wrote:
> > Patch looks great!
> > 
> > Reviews applied: [42551]
> > 
> > Passed command: export OS='ubuntu:14.04' CONFIGURATION='--verbose' 
> > COMPILER='gcc' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; 
> > ./support/docker_build.sh
> 
> Zhiwei Chen wrote:
> This seems not the right way to solve the powerpc make issue, please 
> check https://issues.apache.org/jira/browse/MESOS-5121

I committed Zhiwei's patch. Should this review be discarded?


- Vinod


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


On March 24, 2016, 5:10 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42551/
> ---
> 
> (Updated March 24, 2016, 5:10 a.m.)
> 
> 
> Review request for mesos, Ben Mahler and Vinod Kone.
> 
> 
> Bugs: MESOS-4312
> https://issues.apache.org/jira/browse/MESOS-4312
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Porting mesos on ppc64le.
> 
> 
> Diffs
> -
> 
>   src/linux/fs.cpp 0df194207a66bf4c417349bebcc8912acbf5976e 
> 
> Diff: https://reviews.apache.org/r/42551/diff/
> 
> 
> Testing
> ---
> 
> make check (OSX 10.10.5, Ubuntu 14.04.3 LTS ppc64le)
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45381: Migrate /monitor/statistics and /monitor/statistics.json to slave.

[Jie Yu]

> On March 31, 2016, 7:14 p.m., Jie Yu wrote:
> > src/slave/slave.hpp, line 550
> > 
> >
> > Can you move this field to Http class because this is a limiter for the 
> > http endpoint. Also, `limiter` sounds too general. Maybe rename it?
> > 
> > ```
> > class Http {
> > private:
> >   Slave* slave;
> >   RateLimiter statisticsLimiter;
> > }
> > 
> > ```
> 
> Jay Guo wrote:
> Renamed it.
> 
> Although there is a problem moving it to http class: `Http http = 
> Http(this);` yields compilation error since RateLimiter class copy operator 
> is deleted.
> 
> Jie Yu wrote:
> You can use Shared instead.
> 
> Jay Guo wrote:
> `Shared` takes a pointer to RateLimiter, which needs to be 
> freed properly, as well as its underlying `RateLimiterProcess`. I get some 
> segmentation fault with it. Some hints would be appreciated. While I'm trying 
> to figure it out, there is another concern that `RateLimiter::acquire()` is 
> not `const`, yet `Shared` enforces `const`. Is `const_cast` a good practice 
> in Mesos codebase?

Shared will free the pointer properly when all references go way.

> RateLimiter::acquire() is not const

You can make 'acquire()' a const function.


- Jie


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


On April 2, 2016, 10:31 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45381/
> ---
> 
> (Updated April 2, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> These two endpoints and their underlying logics are moved from
> ResourceMonitorProcess to slave process. ResourceMonitor is removed.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/http.cpp 7a872c6e21b115500c5291b7cb9fb240ec9dc8ed 
>   src/slave/monitor.hpp 70a7c88fefd4577f53f85e3eccdd6b69ab6f3acd 
>   src/slave/monitor.cpp 5c1dd354595e67e5eb603c5d49850f84b84b 
>   src/slave/slave.hpp 3ba335fcd31a92af9609023daae328b7f4bf5e59 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
>   src/tests/mesos.hpp 98f97101d89cb91ca5e773e7a91f04ee1b8a0d88 
>   src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
>   src/tests/monitor_tests.cpp 5dcb2481ff2f1a7caf54036bc3e60c78feb982b1 
>   src/tests/oversubscription_tests.cpp 
> ba036810758d99a6fb0034c5e2bc7829e2343a44 
>   src/tests/slave_tests.cpp 57fc50360eae85819ae6ce714b0c3c4c1867b2b8 
> 
> Diff: https://reviews.apache.org/r/45381/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> https://issues.apache.org/jira/browse/MESOS-4891
> 
> 
> Thanks,
> 
> Jay Guo
> 
>

Re: Review Request 45454: Cleanup orphaned docker containers owned by previous agent instance.

[Timothy Chen]

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


Ship it!




Ship It!

- Timothy Chen


On March 29, 2016, 10:44 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45454/
> ---
> 
> (Updated March 29, 2016, 10:44 p.m.)
> 
> 
> Review request for mesos, Jie Yu, Joris Van Remoortere, and Timothy Chen.
> 
> 
> Bugs: MESOS-3573
> https://issues.apache.org/jira/browse/MESOS-3573
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This change modifies the docker containerizer to cleanup docker
> containers left from another agent instance. The containers can
> become orphans due to any of the scenarios mentioned here:
> http://bit.ly/1RxCpPl
> 
> This change modifies the logic to invoke docker `ps` on all
> containers on the agent instead of limiting itself to the
> current slaveID. This change also means that running multiple
> agent instances on the same host might not work well for docker
> containers from now on i.e. another agent instance might
> cleanup the docker containers that belong to another instance.
> The cgroup isolators/linux launcher for the Mesos containerizer
> anyways don't recommend running multiple instances of the agent
> on the same host.
> 
> In case one still wants to run multiple agent instances on a
> test cluster using the docker containerizer, we can use the
> `--no-docker_kill_orphans` flag and then kill the docker
> containers manually using a script.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 89d450e10a84f24ddd46d517e2b4b46ab02c4fda 
>   src/slave/containerizer/docker.cpp c5007a311ae9c1766dd4522ccbddbdb506d4ae4e 
> 
> Diff: https://reviews.apache.org/r/45454/diff/
> 
> 
> Testing
> ---
> 
> make check (Test is added as part of the next review in the chain)
> 
> Would follow up with an email on user@ to notify them about the potential
> gotchas with running multiple agent instances in production.
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44948: Add XFS disk resource isolator.

[Jiang Yan Xu]

> On March 24, 2016, 9:54 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, line 568
> > 
> >
> > It seems that we don't need to modify `totalProjectIds` (which can be a 
> > const) here, this change doesn't survive agent restart anyways and we use 
> > `freeProjectIds` to assign IDs.
> 
> James Peach wrote:
> I think it is better to handle this consistently. We use 
> ``totalProjectIds`` to bound the set of possible IDs and this ID is not 
> longer possible.

I am not sure about the intended consistency here because I can't think of a 
case where in the following place:

```
if (totalProjectIds.contains(projectId)) {
  freeProjectIds += projectId;
}
```

`projectId` could be in `totalProjectIds` (and therefore incorrectly returned) 
if `totalProjectIds` was always the initial value but would not be in 
`totalProjectIds` if we modify `totalProjectIds` like we currently do in the 
code.

Can you think of any?


> On March 24, 2016, 9:54 a.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/disk/xfs.cpp, lines 501-503
> > 
> >
> > If the new resources have no disk resources, shouldn't we cleanup the 
> > sandbox?
> 
> James Peach wrote:
> It's not clear to me what this means. If you had a disk resource and now 
> you don't are you unrestricted or do you have zero disk? I think it is better 
> to only remove the project ID in cleanup, but maybe we should still adjust 
> the quota here?

Dropping this for a new suggestion in the new review.


- Jiang Yan


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


On April 4, 2016, 10:28 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 4, 2016, 10:28 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45360: Added dvd client for mount and unmount.

[haosdent huang]

> On April 5, 2016, 4:55 p.m., haosdent huang wrote:
> >

By the way, I saw you didn't support `create`, `remove` and `path` methods 
while it exists in dvdcli. Are they unnecessary here?


- haosdent


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


On April 2, 2016, 5:56 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 2, 2016, 5:56 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount and unmount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.hpp 
> PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/dvd_client.cpp 
> PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45360/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45270: Added spec protobut for external mount.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/docker/dvd/spec.proto (line 32)


Does it should be repeated? And because the options looks like:

```
--volumeopts=k1=v1 --volumeopts=k2=v2
```

I think the type should be `Parameter`?


- haosdent huang


On April 2, 2016, 5:52 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45270/
> ---
> 
> (Updated April 2, 2016, 5:52 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added spec protobut for external mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/docker/dvd/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/docker/dvd/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45270/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45373: Ignored the DOCKER_VOLUME volume source.

[haosdent huang]

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




src/slave/containerizer/mesos/isolators/filesystem/linux.cpp (line 471)


I think this message not enough. Usually I would like to know this ignore 
for which container or which executor.


- haosdent huang


On April 4, 2016, 8:49 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45373/
> ---
> 
> (Updated April 4, 2016, 8:49 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Bugs: MESOS-5013
> https://issues.apache.org/jira/browse/MESOS-5013
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ignored the DOCKER_VOLUME volume source.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/filesystem/linux.cpp 
> 9fc7c48f99155750fd3c18c7c102507e2726362b 
> 
> Diff: https://reviews.apache.org/r/45373/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45594: Introduced a new agent flag docker_config.

[Gilbert Song]

> On April 5, 2016, 12:28 p.m., Timothy Chen wrote:
> > src/slave/flags.cpp, line 136
> > 
> >
> > Can you probably elaborate a bit more why a docker config file 
> > configuration is needed? 
> > 
> > I know it's most likely for pulling private registry docker files, but 
> > not quite obvious just by reading the help text.

Yeah, should mention private registry docker image pulling explicitly.


- Gilbert


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


On April 5, 2016, 12:21 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45594/
> ---
> 
> (Updated April 5, 2016, 12:21 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-5065
> https://issues.apache.org/jira/browse/MESOS-5065
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced a new agent flag docker_config.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 9ad0c2a201130ac23b22b1a844396dc97ec5ff70 
>   src/slave/flags.hpp 9ee7f349cc2bb3fa76141645f4a06fad57664367 
>   src/slave/flags.cpp fd9fbbaa911cc77a21574ba314c50ac226fa49ce 
> 
> Diff: https://reviews.apache.org/r/45594/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45596: Updated docker containerizer private registry doc.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45594, 45595, 45596]

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

- Mesos ReviewBot


On April 5, 2016, 7:34 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45596/
> ---
> 
> (Updated April 5, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker containerizer private registry doc.
> 
> 
> Diffs
> -
> 
>   docs/docker-containerizer.md 865b2cf0e158b1aab6c384d26ab7a3d9800610ac 
> 
> Diff: https://reviews.apache.org/r/45596/diff/
> 
> 
> Testing
> ---
> 
> gist
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45596: Updated docker containerizer private registry doc.

[Jie Yu]

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




docs/docker-containerizer.md (lines 64 - 65)


I won't change this original paragraph.

```
// The original paragraph.

Starting from 0.29, we provide an alternative way to specify docker config 
file for pulling images from private registries. We allow operators to specify 
a shared docker config file using an agent flag. This docker config file will 
be used to pull images from private registries for all containers. See ...
```


- Jie Yu


On April 5, 2016, 7:34 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45596/
> ---
> 
> (Updated April 5, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated docker containerizer private registry doc.
> 
> 
> Diffs
> -
> 
>   docs/docker-containerizer.md 865b2cf0e158b1aab6c384d26ab7a3d9800610ac 
> 
> Diff: https://reviews.apache.org/r/45596/diff/
> 
> 
> Testing
> ---
> 
> gist
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 44080: Windows:[1/2] Implemented `os::gmtime_r`.

[Michael Park]

> On March 18, 2016, 5:36 p.m., Michael Park wrote:
> > 3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp, line 177
> > 
> >
> > Can we remove the `struct` in `struct tm`?

Submitted with this fix.


- Michael


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


On March 31, 2016, 4:40 p.m., Alex Clemmer wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44080/
> ---
> 
> (Updated March 31, 2016, 4:40 p.m.)
> 
> 
> Review request for mesos, Alex Naparu, Daniel Pravat, Artem Harutyunyan, 
> Joris Van Remoortere, Michael Park, M Lawindi, and Yi Sun.
> 
> 
> Bugs: MESOS-4462
> https://issues.apache.org/jira/browse/MESOS-4462
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Windows:[1/2] Implemented `os::gmtime_r`.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
> 9ee233b988c08d953e70345c55bcdd5c2f7c101b 
>   3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
> 6a391ff198ab724f689bcef79d4e2e05a786cbc2 
> 
> Diff: https://reviews.apache.org/r/44080/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Alex Clemmer
> 
>

Re: Review Request 45595: Implemented slave default docker config file support.

[Jie Yu]

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


Ship it!




- Jie Yu


On April 5, 2016, 7:34 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45595/
> ---
> 
> (Updated April 5, 2016, 7:34 p.m.)
> 
> 
> Review request for mesos, Guangya Liu, Artem Harutyunyan, Jie Yu, and Timothy 
> Chen.
> 
> 
> Bugs: MESOS-5065
> https://issues.apache.org/jira/browse/MESOS-5065
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented slave default docker config file support.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 7802f23585121d41c738cc28f6bcfa5e6dc9e972 
>   src/docker/docker.cpp 4d35513cdd9c044d37d876a6db7dd9321ceaca53 
>   src/slave/containerizer/docker.cpp 6a8a705186b3a309329825a6b75ad038c2db4ac1 
>   src/tests/mesos.hpp dc0e4dd6341f7d1931f3871991f6a3f20a477159 
>   src/tests/mesos.cpp 543b2a7f3d1a4da28a5901fb238285cea6778c2b 
> 
> Diff: https://reviews.apache.org/r/45595/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh
> 
> Tested with a private repo using ./mesos-execute
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45529: Do not leak roleSorter and quotaRoleSorter in Mesos allocator.

[Benjamin Bannier]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Addressed review comment from Joris.


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


Repository: mesos


Description
---

Do not leak roleSorter and quotaRoleSorter in Mesos allocator.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
e979fdf60da1409d1c2d08f0e9f03cef067506dd 
  src/master/allocator/mesos/hierarchical.cpp 
cd6b91edf3dde9bc4c978daefdfc83d7c3c69304 

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


Testing
---

make check (OS X, clang trunk, not optimized)

In https://reviews.apache.org/r/45534/ I add code requiring cleanup to 
`Sorter`s. Without this patch the cleanup code is never executed, but is with 
the patch.


Thanks,

Benjamin Bannier

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 5, 2016, 3:40 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Missed a line while refactoring the `resourceOffers` method.


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


Repository: mesos


Description
---

This gives the example `long-lived-framework` enough options to run outside of 
the build environment.

This also updates:

* The style of the framework code.
* Gives the `ExecutorInfo` some resources (needed for some cgroups isolators).
* Restricts the framework to one agent.  Otherwise, it would grab a small chunk 
of every machine in the cluster.
* Adds filters for declined offers.


Diffs (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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


Testing
---

make check

Ran this on the master node on a Mesos cluster:
```
./long-lived-framework --master=zk://localhost:2181/mesos 
--executor_uri="https://s3.amazonaws.com/url/to/long-lived-executor; 
--executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./long-lived-executor"
```


Thanks,

Joseph Wu

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

> On April 5, 2016, 12:52 p.m., Joseph Wu wrote:
> > src/tests/cluster.cpp, lines 364-366
> > 
> >
> > The destructor will only dereference a null `containerizer` if this 
> > error case is hit (or if you pass in a `nullptr` cast to 
> > `slave::Containerizer*`).
> > 
> > At this point in the code, we haven't done anything other than create 
> > some objects.  So it should be ok to do something like this in the 
> > destructor:
> > ```
> > if (containerizer == nullptr) {
> >   return;
> > }
> > ```
> > The call to `terminate()` only matters if the `cluster::Slave` is 
> > created successfully.

I agree that Haosdent's suggestion works for the purpose of this fix. But if 
"The call to terminate() only matters if the cluster::Slave is created 
successfully", then we should call terminate() under the condition that `pid` 
is not empty in `~Slave()`. We should fix it in a separate review.


- Jiang Yan


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


On April 4, 2016, 10:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 4, 2016, 10:31 a.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.cpp 14d0d34fcb4c408ad996672394c39c84fd2be918 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  3rdparty/Makefile.am 0523151d924a1398334767bd401ae84e41bdceb7 
  3rdparty/cmake/Versions.cmake 86c51edb3aa2daf6451459aaf18278f09b91b000 
  3rdparty/libprocess/3rdparty/http-parser-2.6.1.tar.gz PRE-CREATION 
  3rdparty/libprocess/3rdparty/http-parser-2.6.2.patch  
  3rdparty/libprocess/3rdparty/http-parser-2.6.2.tar.gz 
f42342a03b351ab223615883583e79de62d81ee1 
  3rdparty/libprocess/3rdparty/stout/include/stout/errorbase.hpp 
96d395d37cdad706c2f23fdd9caeefc2a33e0541 
  3rdparty/libprocess/3rdparty/stout/include/stout/flags/parse.hpp 
ef365e4d714a2c25d358a702722c5f1216869382 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/grp.hpp 
232588c46c8207c6a002f60d00aa675f9e505083 
  3rdparty/libprocess/3rdparty/stout/include/stout/internal/windows/pwd.hpp 
46e3ec25cbf379fb2b82297849216b0866baf333 
  3rdparty/libprocess/3rdparty/stout/include/stout/posix/os.hpp 
edaa76a5322d0bf60b7172405aa754b5aca95458 
  3rdparty/libprocess/3rdparty/stout/include/stout/try.hpp 
89dedec0c818263f17f220a2e71ed470bf75a42f 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/error.hpp 
d634491b301632ce6468f86d04f21fa25afbbaaa 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/format.hpp 
872ddbc6e3cd81ac5976ee12df2db32905a9fe7b 
  3rdparty/libprocess/3rdparty/stout/include/stout/windows/os.hpp 
c48106e5905e3be0faeba7177ef534766089faff 
  3rdparty/libprocess/3rdparty/stout/tests/flags_tests.cpp 
66e16abe914e2a1ee7599bab857ff478b7ec20dc 
  3rdparty/libprocess/3rdparty/versions.am 
5506eb3aeafb09226ba5b90dae194ece31285412 
  3rdparty/libprocess/include/process/network.hpp 
2bc940fb3ba2bc6b2fed281a3920d13fdb1277e9 
  3rdparty/libprocess/src/encoder.hpp 69163830eaa9f77132a16fc14351b309144827bf 
  3rdparty/libprocess/src/poll_socket.cpp 
cb2878565a112017b190b4ff83dc65a876ea45f9 
  3rdparty/libprocess/src/process.cpp d2c458ed93307f75358bb642aaf2ed8e17b2fe97 
  3rdparty/libprocess/src/time.cpp a6c3f3de69e056544406f4416c2d0aea06adc34d 
  3rdparty/versions.am a94723e213dbbbd1df7e1b784386023412f7b961 
  3rdparty/zookeeper-3.4.5.patch PRE-CREATION 
  3rdparty/zookeeper-3.4.8.patch 486df1ae96af3426835c9d47ff2e36dd47ccde3f 
  3rdparty/zookeeper-3.4.8.tar.gz a23d68becf596b0246371fc244f018e4c71ca5d9 
  CHANGELOG b5dd490a064a226d242448093a306b9485a88de3 
  LICENSE 19130d977986dc9e23872817251ffcaa5a3c2c7d 
  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 
  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  docs/mesos-containerizer.md 2fde74362f35568bb944da80a1db17e6d3ba9d7b 
  docs/modules.md 28eb233a30b844b302fd95c03e4ff6647355cdfa 
  docs/versioning.md 800debe5ddb40f941a3b839aa79dddbd2dbf1462 
  include/mesos/docker/spec.hpp 2ebacc70d92a593c8dd006b34519c3a2a5225481 
  src/docker/docker.hpp da6e9a25b99079940958001128ee949cdb9b931b 
  src/docker/docker.cpp bb9ddde27464aa4d9215e3ca673fa87c6e00e326 
  src/docker/executor.cpp 579905f6c2ece7db9c853abf481541fdbc3116b7 
  src/docker/spec.cpp ac28331a17edb8c2ff81d5a2f79a794f869a3e5d 
  src/examples/balloon_framework.cpp 15c45612b777edaf97aea9b953439d4ad56920f3 
  src/examples/java/test-log.in 4132617ccbe5067c22d959a58640be825cedfe67 
  src/examples/no_executor_framework.cpp 
f578edfd99f3b7adf19cf06eab20696532c7b67d 
  src/examples/persistent_volume_framework.cpp 
b4faa0ee25dc3a72c17ef2b0640a3695423ef79a 
  src/examples/test_hook_module.cpp 3ff9fd71c275fd1a705ab3aca7a8041f29289bb0 
  src/exec/exec.cpp a2e6d86fd0b1f3d688d17296151db74bcb9b3418 
  src/launcher/executor.cpp 4658b9e5bb48b09e403991c8bdfdbf4d4dae0416 
  src/linux/cgroups.hpp 5f4010734ed9e3295dcc3a4390123e4f4ce99c16 
  src/linux/fs.cpp 2087b4ac1503e0fd085319b1017389f1f947536f 
  src/local/local.cpp 0d980188f933a8d543af696d8addd7ca5855413e 
  src/master/allocator/mesos/hierarchical.cpp 
cd6b91edf3dde9bc4c978daefdfc83d7c3c69304 
  src/master/main.cpp 181bbcb1758c0e9b83ef46496e990ce3d8c2195c 
  src/master/master.hpp e5b16f94fa5650b9db44cd7c975adeb5871e16f6 
  src/master/master.cpp 2cfa5075991bbb5df8a7e33b14767de558bda4ac 
  src/master/validation.cpp 504cd9b8bd5d40bb591b7aa5a23bd74cc210c2fc 
  src/module/manager.cpp 7850fd3e1e574e1a92289118e2e27c62b68eaf05 
  src/python/native_common/ext_modules.py.in 
ad98a576ee3662ccb5b8900a339e530089f41355 
  src/sched/sched.cpp d740ef630683b61554db58e628333e8a19e9aa27 
  src/slave/containerizer/docker.cpp 0576eab65a1f3556644472d5c9baba4ea9c34346 
  src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44949: Add XFS disk isolator tests.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add basic XFS disk isolator tests by cloning the POSIX disk isolator
tests and making minor changes for the differences in semantics.


Diffs (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 

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


Testing
---

Make check. Manual testing.


Thanks,

James Peach

Re: Review Request 45455: Added test for recovering orphaned docker containers.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45453, 45454, 45455]

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

- Mesos ReviewBot


On April 5, 2016, 10:05 p.m., Anand Mazumdar wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45455/
> ---
> 
> (Updated April 5, 2016, 10:05 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Timothy Chen.
> 
> 
> Bugs: MESOS-3573
> https://issues.apache.org/jira/browse/MESOS-3573
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> a2ac254423449a6e5185fe83177c45ce0d08420f 
> 
> Diff: https://reviews.apache.org/r/45455/diff/
> 
> 
> Testing
> ---
> 
> make check (gtest_repeat=100)
> 
> 
> Thanks,
> 
> Anand Mazumdar
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

> On April 5, 2016, 10:25 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.cpp, line 286
> > 
> >
> > `s/directory/path`.

As discussed, we only accept directories now.


> On April 5, 2016, 10:25 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/utils.hpp, lines 61-62
> > 
> >
> > s/directory/path/, this breaks the symmetry in the `*projectId` methods 
> > but this doesn't need to be a directory and we actually use it to test 
> > files in tests.

As discussed we only accept directories now.


- James


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


On April 5, 2016, 11:15 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 5, 2016, 11:15 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11:40 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Track sandbox directory usage by dynamically assigning XFS project
quotas. We track a range of XFS project IDs, assigning a project ID
and a project quota to each sandbox as it is created. When the task
reaches the quota, writes will fail with EDQUOT, and the task will have
an opportunity to handle that.

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

  $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

  $ xfs_io -r -c stat PATH


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
  src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 

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


Testing
---

Make check. Manual testing. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:40 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

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


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44950: Add XFS disk isolator documentation.

[James Peach]

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

(Updated April 5, 2016, 10:59 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Add XFS disk isolator documentation.


Diffs (updated)
-

  docs/configuration.md da42eaf7069a016fa7eaf929fc285e1fa1f144e9 
  docs/mesos-containerizer.md 15fb5bdbe74e059614b8948108f32cd04b623305 

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


Testing
---

Make check. Source inspection.


Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

[James Peach]

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

(Updated April 5, 2016, 10:59 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addresses review comments.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Track sandbox directory usage by dynamically assigning XFS project
quotas. We track a range of XFS project IDs, assigning a project ID
and a project quota to each sandbox as it is created. When the task
reaches the quota, writes will fail with EDQUOT, and the task will have
an opportunity to handle that.

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

  $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

  $ xfs_io -r -c stat PATH


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
  src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 

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


Testing
---

Make check. Manual testing. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44949: Add XFS disk isolator tests.

[James Peach]

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

(Updated April 5, 2016, 10:59 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add basic XFS disk isolator tests by cloning the POSIX disk isolator
tests and making minor changes for the differences in semantics.


Diffs (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 

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


Testing
---

Make check. Manual testing.


Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

[James Peach]

> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.hpp, line 33
> > 
> >
> > Doesn't look like the utils header is used in this header?
> > 
> > Otherwise add a blank line above.

Added the blank. The header is needed for the definition of ``prid_t``.


> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, lines 112-114
> > 
> >
> > There is also the `uid.isNone()` case.
> > 
> > We can just 
> > 
> > ```
> > if (!user.isSome()) {
> >   return Error("Failed to determine user: " +
> >(uid.isError() ? uid.error() : "uid not found"));
> > }
> > ```

``getuid(2)`` can't fail or return ``None``.


> On April 5, 2016, 4:52 p.m., Jiang Yan Xu wrote:
> > src/slave/containerizer/mesos/isolators/xfs/disk.cpp, lines 281-286
> > 
> >
> > This is sort of safe in our case because we expect an empty sandbox. 
> > 
> > In a general case `xfs::setProjectId()` could partially fail which 
> > makes it unsafe to return the project ID.
> > 
> > I think we can just do a hard CHECK before calling to make sure the 
> > directory is empty. If no such method is directory usable right now, add a 
> > TODO and let's add one to stout.

As discussed, we keep the container Info record and depend on cleanup to return 
the project ID.


- James


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


On April 5, 2016, 10:59 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated April 5, 2016, 10:59 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOs-4828
> https://issues.apache.org/jira/browse/MESOs-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Track sandbox directory usage by dynamically assigning XFS project
> quotas. We track a range of XFS project IDs, assigning a project ID
> and a project quota to each sandbox as it is created. When the task
> reaches the quota, writes will fail with EDQUOT, and the task will have
> an opportunity to handle that.
> 
> Quotas are not applied to volume resources since the isolator interface
> has no insight into the volume lifecycle. Thus it is not currently
> possible to accurately assign and reclaim project IDs.
> 
> If LOW is the lower bound of the project ID range and HIGH is the upper
> bound, you can show the currently allocated project quotas using the
> xfs_quota command:
> 
>   $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"
> 
> To show the project ID assigned to the file PATH, use the xfs_io command:
> 
>   $ xfs_io -r -c stat PATH
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/containerizer.cpp 
> a5dd22380066aa85de04d485052084e2629681c0 
>   src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/slave/flags.hpp d0c606eea74e1a2e69067c43a267047e65a22a04 
>   src/slave/flags.cpp 0551ec334c6747507bf7bb068d27d67f3fdd6c83 
> 
> Diff: https://reviews.apache.org/r/44948/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44949: Add XFS disk isolator tests.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add basic XFS disk isolator tests by cloning the POSIX disk isolator
tests and making minor changes for the differences in semantics.


Diffs (updated)
-

  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 

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


Testing
---

Make check. Manual testing.


Thanks,

James Peach

Re: Review Request 44950: Add XFS disk isolator documentation.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Add XFS disk isolator documentation.


Diffs (updated)
-

  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  docs/mesos-containerizer.md 2fde74362f35568bb944da80a1db17e6d3ba9d7b 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 

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


Testing
---

Make check. Source inspection.


Thanks,

James Peach

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add utility functions to manipulate XFS project quotas.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 

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


Testing
---

Make check. Manual verification. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[James Peach]

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

(Updated April 5, 2016, 11:41 p.m.)


Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.


Changes
---

Rebased and addressed review comments.


Repository: mesos


Description
---

If Cluster::Slave::start() fails, make sure we don't crash in the
destructor.


Diffs (updated)
-

  src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 
  src/tests/cluster.cpp eefc2fa55bca1ad6a53047046fa4f5996d5c3fef 

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


Testing
---

Make check.


Thanks,

James Peach

Re: Review Request 45455: Added test for recovering orphaned docker containers.

[Anand Mazumdar]

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

(Updated April 5, 2016, 10:05 p.m.)


Review request for mesos, Jie Yu and Timothy Chen.


Changes
---

Review comment from Tim


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/tests/containerizer/docker_containerizer_tests.cpp 
a2ac254423449a6e5185fe83177c45ce0d08420f 

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


Testing
---

make check (gtest_repeat=100)


Thanks,

Anand Mazumdar

Re: Review Request 45622: Fixed Nvidia GPU isolator build for gcc.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On April 2, 2016, 9:35 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45622/
> ---
> 
> (Updated April 2, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Ben Mahler and switched to 'mcypark'.
> 
> 
> Bugs: MESOS-5082
> https://issues.apache.org/jira/browse/MESOS-5082
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There appears to be a discrepancy between clang and gcc, which allows
> clang to accept `using` declarations of the form `using ns_name::name;`
> that contain nested classes, structs, and enums after the `name` field
> in the declaration (e.g. `using ns_name::name::enum;`).
> 
> The language for describing this functionality is ambiguous in the
> C++11 specification as referenced here:
> http://en.cppreference.com/w/cpp/language/namespace#Using-declarations
> 
> This patch fixes a bug where we relied on this functionality in clang
> and had build errors in gcc.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 53397fc1da11a71259df1c38526a613676a60301 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> c2cdc8fde7a85741be6494ea664d3719d1f13a43 
> 
> Diff: https://reviews.apache.org/r/45622/diff/
> 
> 
> Testing
> ---
> 
> Rebuilt with nvidia gpu support for both clang and gcc.
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 45440: Added some metrics to the long-lived-framework example.

[Joseph Wu]

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

(Updated April 5, 2016, 3:33 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Rebased on previous patch (interleaved changes).


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


Repository: mesos


Description
---

Adds metrics to gauge the health of the framework.  This includes:

* uptime_secs = How long the framework has been running.
* registered  = If the framework is registered.
* offers_received = A counter used to determine if the framework is starved or 
not.
* tasks_launched  = Number of tasks launched.
* abnormal_terminations = Number of terminal status updates which were not 
`TASK_FINISHED`.

Also adds an endpoint `/framework/counters` which returns the list of metrics 
which are "counters".


Diffs (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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


Testing
---

make check

Also deployed this version on a test cluster.  See the previous review.


Thanks,

Joseph Wu

Re: Review Request 45768: Fixed commit message hook to skip over the commented lines.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45767, 45768]

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

- Mesos ReviewBot


On April 5, 2016, 7:50 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45768/
> ---
> 
> (Updated April 5, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos, Joerg Schad and Vinod Kone.
> 
> 
> Bugs: MESOS-5126
> https://issues.apache.org/jira/browse/MESOS-5126
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   support/hooks/commit-msg d3d5415bf6a243def05cf79f4af2b136b9866d68 
> 
> Diff: https://reviews.apache.org/r/45768/diff/
> 
> 
> Testing
> ---
> 
> Was able to commit a patch where there were lines longer than 72 characters 
> in the commented section.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 44946: Add utility functions to manipulate XFS project quotas.

[Jiang Yan Xu]

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


Fix it, then Ship it!




Looking good. Just a few minor comments, plus one mentioned in [another 
review](https://reviews.apache.org/r/44948/diff/11/?file=1321656#file1321656line140)
 and we are good to go.


src/slave/containerizer/mesos/isolators/xfs/utils.hpp (lines 61 - 62)


s/directory/path/, this breaks the symmetry in the `*projectId` methods but 
this doesn't need to be a directory and we actually use it to test files in 
tests.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 72)


Kill line.



src/slave/containerizer/mesos/isolators/xfs/utils.hpp (line 78)


`s/__XFS_HPP__/__XFS_UTILS_HPP__`.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 59)


Add an empty line.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (lines 154 - 155)


I didn't mention this earlier but it would be nice if all these internal 
arithmetics use the Bytes object and only convert to a uint when it calls 
underlying systems calls.

A TODO would be fine.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 265)


"larger than or equal to" or ">=" so it matches the condition check.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 266)


Nit: `stringify(Bytes(BASIC_BLOCK_SIZE))`. 

I have another comment about a TODO for using Bytes consistently, including 
the type of BASIC_BLOCK_SIZE. But OK to not do it in this review.



src/slave/containerizer/mesos/isolators/xfs/utils.cpp (line 286)


`s/directory/path`.



src/tests/environment.cpp (lines 468 - 471)


This is unrelated?


- Jiang Yan Xu


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44946/
> ---
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add utility functions to manipulate XFS project quotas.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44946/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification. Tests in subsequent patches.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45067: Updated the long-lived-framework example.

[Joseph Wu]

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

(Updated April 5, 2016, 3:33 p.m.)


Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and Vinod 
Kone.


Changes
---

Addressed Vinod's comments.


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


Repository: mesos


Description
---

This gives the example `long-lived-framework` enough options to run outside of 
the build environment.

This also updates:

* The style of the framework code.
* Gives the `ExecutorInfo` some resources (needed for some cgroups isolators).
* Restricts the framework to one agent.  Otherwise, it would grab a small chunk 
of every machine in the cluster.
* Adds filters for declined offers.


Diffs (updated)
-

  src/examples/long_lived_framework.cpp 
ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 

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


Testing
---

make check

Ran this on the master node on a Mesos cluster:
```
./long-lived-framework --master=zk://localhost:2181/mesos 
--executor_uri="https://s3.amazonaws.com/url/to/long-lived-executor; 
--executor_command="LD_LIBRARY_PATH=/path/to/libmesos && ./long-lived-executor"
```


Thanks,

Joseph Wu

Re: Review Request 44947: Add tests for XFS project quota utilities.

[Jiang Yan Xu]

> On April 1, 2016, 12:27 a.m., Jiang Yan Xu wrote:
> > src/tests/containerizer/xfs_quota_tests.cpp, line 71
> > 
> >
> > We have 
> > 
> > `makeQuotaInfo` vs. `mkfile` & `mkloop`. Can we make the use of the 
> > word `make` or `mk` consistent? Generally we avoid abbreviations so using 
> > `make` (and camelCasing) is preferred.
> 
> James Peach wrote:
> ``mkfile`` and ``mkloop`` are named after ``mkfile(1)``, ``mknod(1)``, 
> etc. I expected that would be a fairly familiar nomenclature.
> 
> ``makeQuotaInfo`` is substantially different so there's no good reason to 
> use the same naming scheme. It is following the ``std::make_pair`` pattern, 
> but using Mesos naming conventions.

SGTM!


- Jiang Yan


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


On April 4, 2016, 10:27 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 4, 2016, 10:27 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add tests for XFS project quota utilities.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44947/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual testing. These tests.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44950: Add XFS disk isolator documentation.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Add XFS disk isolator documentation.


Diffs (updated)
-

  docs/configuration.md 309a5a05eab386c8943ba6bdee8d5efeb448aa0c 
  docs/mesos-containerizer.md 2fde74362f35568bb944da80a1db17e6d3ba9d7b 

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


Testing
---

Make check. Source inspection.


Thanks,

James Peach

Re: Review Request 44947: Add tests for XFS project quota utilities.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 2afaa328a0fb226a2d1ca35a4754ccb274bc075d 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

Re: Review Request 44948: Add XFS disk resource isolator.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


Bugs: MESOs-4828
https://issues.apache.org/jira/browse/MESOs-4828


Repository: mesos


Description
---

Track sandbox directory usage by dynamically assigning XFS project
quotas. We track a range of XFS project IDs, assigning a project ID
and a project quota to each sandbox as it is created. When the task
reaches the quota, writes will fail with EDQUOT, and the task will have
an opportunity to handle that.

Quotas are not applied to volume resources since the isolator interface
has no insight into the volume lifecycle. Thus it is not currently
possible to accurately assign and reclaim project IDs.

If LOW is the lower bound of the project ID range and HIGH is the upper
bound, you can show the currently allocated project quotas using the
xfs_quota command:

  $ xfs_quota -x -c "report -a -n -L LOW -U HIGH"

To show the project ID assigned to the file PATH, use the xfs_io command:

  $ xfs_io -r -c stat PATH


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/slave/containerizer/mesos/isolators/xfs/disk.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/disk.cpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/xfs/utils.cpp PRE-CREATION 
  src/slave/flags.hpp 69e1b01e09d2a15bee5e0745b751f47aaefe3fbe 
  src/slave/flags.cpp 315cf47d268bce0a0255a061d64e414c736c8125 

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


Testing
---

Make check. Manual testing. Tests in subsequent patches.


Thanks,

James Peach

Re: Review Request 44945: Add autoconf tests for XFS project quotas.

[James Peach]

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

(Updated April 5, 2016, 11:15 p.m.)


Review request for mesos, Jie Yu and Jiang Yan Xu.


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac c693b825294f82ace8a14563cf2229820e159e3c 

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


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 37168: Add an example framework using dynamic reservation.

[Michael Park]

> On April 4, 2016, 1:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 311-320
> > 
> >
> > Conceptutally, `Try` is not something we pass around like this. A 
> > function returning a `Try` is emulating a function that throws an 
> > exception. So if a `Try` results in an error, we need to handle it 
> > immediately, or propagate the error. We do not pass it along to a 
> > subsequent function.
> > 
> > There are other issues here as well. Even if we were to pass it around, 
> > it should have been passed by `const &`. What does `rc` stand for?
> > 
> > Functionally speaking, this says that if `rc` is an error, we log 
> > saying we failed to update the state of an agent. But `reserveResources` 
> > returns `Error` if the offer doesn't contain sufficient amount of 
> > resources. This has really little to do with "failing to update the state 
> > of an agent."
> 
> Klaus Ma wrote:
> Thanks very much for your explaintation on `Try`; that's clear to me :).
> 
> BTW, `rc` means `return code`.

I see. Thanks.


> On April 4, 2016, 1:40 a.m., Michael Park wrote:
> > src/examples/dynamic_reservation_framework.cpp, lines 350-364
> > 
> >
> > We should be able to just use the ones in `src/tests/mesos.hpp`.
> 
> Klaus Ma wrote:
> I'd like to de-couple example from Mesos internal code, e.g. 
> `tests/mesos.hpp`. Ideally, the user can build example with installed Mesos 
> binaries & headers.

I see. Ok, we can keep these then. Thanks!


- Michael


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


On April 5, 2016, 9:28 a.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/37168/
> ---
> 
> (Updated April 5, 2016, 9:28 a.m.)
> 
> 
> Review request for mesos, Greg Mann, Joerg Schad, and Michael Park.
> 
> 
> Bugs: MESOS-3063
> https://issues.apache.org/jira/browse/MESOS-3063
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Provide example for dynamic reservation features.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/examples/dynamic_reservation_framework.cpp PRE-CREATION 
>   src/tests/dynamic_reservation_framework_test.sh PRE-CREATION 
>   src/tests/examples_tests.cpp 6ddac17bb2ac0330bcc09eaab975ae70e84a7695 
> 
> Diff: https://reviews.apache.org/r/37168/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Re: Review Request 45715: Added support to grant access to /dev/nvidiactl in Nvidia GPU isolator.

[Ben Mahler]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp (lines 
65 - 78)


Could we avoid the static non-POD?


- Ben Mahler


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45715/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5115
> https://issues.apache.org/jira/browse/MESOS-5115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, calls to 'nvidia-smi' would fail inside a container even
> if access to a GPU had been granted. Moreover, access to
> /dev/nvidiactl is actually required for a container to do anything
> useful with a GPU even if it has access to it.
> 
> This patch explicitly grants/revokes access to /dev/nvidiactl as GPUs
> are added and removed from a container in the Nvidia GPU isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> b0f58035c7c819b42e5f249fadd97312f9e3ac7b 
> 
> Diff: https://reviews.apache.org/r/45715/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 45715: Added support to grant access to /dev/nvidiactl in Nvidia GPU isolator.

[Ben Mahler]

> On April 5, 2016, 11:53 p.m., Ben Mahler wrote:
> >

Do we also need the uvm device?


- Ben


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


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45715/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-5115
> https://issues.apache.org/jira/browse/MESOS-5115
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Previously, calls to 'nvidia-smi' would fail inside a container even
> if access to a GPU had been granted. Moreover, access to
> /dev/nvidiactl is actually required for a container to do anything
> useful with a GPU even if it has access to it.
> 
> This patch explicitly grants/revokes access to /dev/nvidiactl as GPUs
> are added and removed from a container in the Nvidia GPU isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/devices/gpus/nvidia.cpp 
> b0f58035c7c819b42e5f249fadd97312f9e3ac7b 
> 
> Diff: https://reviews.apache.org/r/45715/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 44364: Added tests for the Nvidia GPU isolator.

[Kevin Klues]

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

(Updated April 6, 2016, 12:38 a.m.)


Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.


Changes
---

Addressed all of bmahler's comments.


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


Repository: mesos


Description
---

This commit also includes a test filter called 'NvidiaGpuFilter' that
checks for the existence of 'nvidia-smi' before running any tests that
contain the 'NVIDIA_GPU_' prefix.


Diffs (updated)
-

  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/tests/containerizer/nvidia_gpu_isolator_tests.cpp PRE-CREATION 
  src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 

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


Testing
---

On an Nvidia GPU equipped machine:

```
sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
bin/mesos-tests.sh

SUCCESS
```

On a **non** Nvidia GPU equipped machine:

```
sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
bin/mesos-tests.sh

which: no nvidia-smi in (/sbin:/bin:/usr/sbin:/usr/bin)
--
No 'nvidia-smi' command found so no 'NvidiaGpu' tests will run
--
```


Thanks,

Kevin Klues

Re: Review Request 44364: Added tests for the Nvidia GPU isolator.

[Ben Mahler]

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


Fix it, then Ship it!




Nice test!


src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 62)


Could we have a summary comment?



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 70)


s/std:://



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 73 - 79)


We shouldn't need the fetcher and containerizer here.



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (lines 137 - 140)


Could we put newlines in the string so that it prints pretty?



src/tests/containerizer/nvidia_gpu_isolator_tests.cpp (line 150)


Can you sweep for s/.get()./->/ ?



src/tests/environment.cpp (lines 254 - 276)


It's unfortunate that the curl filter used "error" to express existence, 
could we do a s/error/exists/ here? I'd also be ok shipping a patch for the 
curl filter if you want to take that on.


- Ben Mahler


On April 4, 2016, 11:41 p.m., Kevin Klues wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44364/
> ---
> 
> (Updated April 4, 2016, 11:41 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Rob Todd, and Vikrama Ditya.
> 
> 
> Bugs: MESOS-4863
> https://issues.apache.org/jira/browse/MESOS-4863
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit also includes a test filter called 'NvidiaGpuFilter' that
> checks for the existence of 'nvidia-smi' before running any tests that
> contain the 'NVIDIA_GPU_' prefix.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
>   src/tests/containerizer/nvidia_gpu_isolator_tests.cpp PRE-CREATION 
>   src/tests/environment.cpp 90dbe9488bda6af26143934e196aab0d69dccec3 
> 
> Diff: https://reviews.apache.org/r/44364/diff/
> 
> 
> Testing
> ---
> 
> On an Nvidia GPU equipped machine:
> 
> ```
> sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
> bin/mesos-tests.sh
> 
> SUCCESS
> ```
> 
> On a **non** Nvidia GPU equipped machine:
> 
> ```
> sudo GTEST_FILTER="NvidiaGpuTest.ROOT_CGROUPS_NVIDIA_GPU_VerifyDeviceAccess" 
> bin/mesos-tests.sh
> 
> which: no nvidia-smi in (/sbin:/bin:/usr/sbin:/usr/bin)
> --
> No 'nvidia-smi' command found so no 'NvidiaGpu' tests will run
> --
> ```
> 
> 
> Thanks,
> 
> Kevin Klues
> 
>

Re: Review Request 45689: Make Cluster::Slave more tolerant of start failures.

[Jiang Yan Xu]

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


Ship it!





src/tests/cluster.hpp (line 188)


These are default initialized anyways but explicitness is good.


- Jiang Yan Xu


On April 5, 2016, 4:41 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45689/
> ---
> 
> (Updated April 5, 2016, 4:41 p.m.)
> 
> 
> Review request for mesos, Joris Van Remoortere, Joseph Wu, and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If Cluster::Slave::start() fails, make sure we don't crash in the
> destructor.
> 
> 
> Diffs
> -
> 
>   src/tests/cluster.hpp 91d3998176115002ea22a0825b1aaaee3fe0d768 
>   src/tests/cluster.cpp eefc2fa55bca1ad6a53047046fa4f5996d5c3fef 
> 
> Diff: https://reviews.apache.org/r/45689/diff/
> 
> 
> Testing
> ---
> 
> Make check.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45731: Introduced '--networks' flag to mesos-execute.

[Avinash sridharan]

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




src/cli/execute.cpp (line 354)


It would much simpler to have a single if conditional here as follows:
if (networks.isSome()) {
ContainerInfo* containerInfo =  
task.mutable_container();

 foreach (const string& network,
  strings::split(networks.get(), ","))   {
 containerInfo->add_network_infos()->set_name(network);
  }
}


- Avinash sridharan


On April 5, 2016, 9:13 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45731/
> ---
> 
> (Updated April 5, 2016, 9:13 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-5117
> https://issues.apache.org/jira/browse/MESOS-5117
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Introduced '--networks' flag to mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/45731/diff/
> 
> 
> Testing
> ---
> 
> Ran the following command:
>   `sudo src/mesos-execute --master=192.168.122.171:5050 --name=test 
> --docker_image=library/busybox --command="sleep 30" --shell=true 
> --networks=net1,net2`
> 
> The created container successfully joined the CNI networks `net1` and `net2`.
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45440: Added some metrics to the long-lived-framework example.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45067, 45440]

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

- Mesos ReviewBot


On April 5, 2016, 10:33 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45440/
> ---
> 
> (Updated April 5, 2016, 10:33 p.m.)
> 
> 
> Review request for mesos, Greg Mann, Artem Harutyunyan, Kevin Klues, and 
> Vinod Kone.
> 
> 
> Bugs: MESOS-5062
> https://issues.apache.org/jira/browse/MESOS-5062
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds metrics to gauge the health of the framework.  This includes:
> 
> * uptime_secs = How long the framework has been running.
> * registered  = If the framework is registered.
> * offers_received = A counter used to determine if the framework is starved 
> or not.
> * tasks_launched  = Number of tasks launched.
> * abnormal_terminations = Number of terminal status updates which were not 
> `TASK_FINISHED`.
> 
> Also adds an endpoint `/framework/counters` which returns the list of metrics 
> which are "counters".
> 
> 
> Diffs
> -
> 
>   src/examples/long_lived_framework.cpp 
> ef498d63bc5f0a8deb46d71edd85a76a1d38fdd0 
> 
> Diff: https://reviews.apache.org/r/45440/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Also deployed this version on a test cluster.  See the previous review.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>