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

[Guangya Liu]

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

(Updated 四月 2, 2016, 5:56 a.m.)


Review request for mesos, Gilbert Song and Jie Yu.


Summary (updated)
-

Added dvd client for mount and unmount.


Repository: mesos


Description (updated)
---

Added dvd client for mount and unmount.


Diffs (updated)
-

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

make
make check


Thanks,

Guangya Liu

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

[Guangya Liu]

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

(Updated 四月 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 (updated)
-

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

make
make check


Thanks,

Guangya Liu

Re: Review Request 45265: Plugged in dvd isolator into agent.

[Guangya Liu]

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

(Updated 四月 2, 2016, 5:47 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
---

Plugged in dvd isolator into agent.


Diffs (updated)
-

  src/slave/containerizer/mesos/containerizer.cpp 
a5dd22380066aa85de04d485052084e2629681c0 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION 

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


Testing
---


Thanks,

Guangya Liu

Re: Review Request 45217: Implemented docker volume driver isolator interface.

[Guangya Liu]

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

(Updated 四月 2, 2016, 5:45 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 docker volume driver isolator interface.


Diffs (updated)
-

  src/CMakeLists.txt ff225c0d51a32b03a1b5f2ba31718ec2305c7ced 
  src/Makefile.am f22ae5b3bd9336a56c802e0e51d39d6cb675caf2 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/docker/dvd/dvd.cpp PRE-CREATION 

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


Testing
---

make
make check


Thanks,

Guangya Liu

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

[Guangya Liu]

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

(Updated 四月 2, 2016, 5:41 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 (updated)
---

make
make check


Thanks,

Guangya Liu

Re: Review Request 45580: Add some log messages in 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082, 
45383, 45531, 45532, 45546, 45580]

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 2, 2016, 3:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45580/
> ---
> 
> (Updated April 2, 2016, 3:31 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add some log messages in 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 991139fac2b2583a35981a5c85f971510515cfab 
> 
> Diff: https://reviews.apache.org/r/45580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45580: Add some log messages in 'network/cni' isolator.

[Qian Zhang]

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

(Updated April 2, 2016, 11:31 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add some log messages in 'network/cni' isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
991139fac2b2583a35981a5c85f971510515cfab 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45033]

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 2, 2016, 1:41 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> ---
> 
> (Updated April 2, 2016, 1:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Bernd 
> Mathiske, haosdent huang, Kevin Klues, Neil Conway, Vinod Kone, and Deshi 
> Xiao.
> 
> 
> Bugs: MESOS-4033
> https://issues.apache.org/jira/browse/MESOS-4033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review request tries to add addition check in mesos-style.pl
> for checking non-ascii characters. It scans .cpp, .hpp, .cc, .h
> files and report error if non-ascii characters exists.
> 
> As part of this review request, two non-ascii characters are identified
> in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
> accordingly.
> 
> Note: .md scan is skipped based on feedback from review request.
> 
> 
> Diffs
> -
> 
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   support/mesos-style.py 13616065ebe07ca401b385716d9b723f65bb2162 
> 
> Diff: https://reviews.apache.org/r/45033/diff/
> 
> 
> Testing
> ---
> 
> Tested manually and found two non ascii characters in docs/versioning.md 
> (fixed as part of this review request).
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated April 2, 2016, 11:18 a.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


Changes
---

Rebase


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


Repository: mesos


Description
---

Implemented status() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
9fe394a7e837293cb6c9dab7097ddeeb2e38290c 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
991139fac2b2583a35981a5c85f971510515cfab 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

> On April 2, 2016, 6:32 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 93-95
> > 
> >
> > This is very confusing. We have both 'network' and 'networkInfo' in 
> > 'NetworkInfo'!
> > 
> > How about this: we introduce an optional 'ContainerInfo' in 'Info':
> > 
> > ```
> > struct Info
> > {
> >   Option containerInfo;
> > }
> > ```
> 
> Qian Zhang wrote:
> So you mean we will have two fields in `struct Info`? Then what fields 
> will `ContainerInfo` have?
>   struct Info
>   {
> hashmap networkInfos;
> Option containerInfo;
>   }
> 
> How about just rename `struct NetworkInfo` to `struct ContainerNetwork` 
> and rename its field `network` to `cniNetworkInfo` like below?
>   struct ContainerNetwork
>   {
> std::string networkName;
> 
> std::string ifName;
> 
> mesos::NetworkInfo networkInfo;
> 
> Option cniNetworkInfo
>   };
> 
>   struct Info
>   {
> Info (const hashmap& 
> _containerNetworks)
>   : containerNetworks (_containerNetworks) {}
> 
> hashmap containerNetworks;
>   };
> 
> Jie Yu wrote:
> yeah, that's much better!

Great, I'd like to do the renaming in a separate patch since it is not related 
to this one (`status()`). So Jie, please review this one, and I will post a 
separate renaming patch soon.


- Qian


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


On April 1, 2016, 10:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated April 1, 2016, 10:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Jie Yu]

> On April 1, 2016, 10:32 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 93-95
> > 
> >
> > This is very confusing. We have both 'network' and 'networkInfo' in 
> > 'NetworkInfo'!
> > 
> > How about this: we introduce an optional 'ContainerInfo' in 'Info':
> > 
> > ```
> > struct Info
> > {
> >   Option containerInfo;
> > }
> > ```
> 
> Qian Zhang wrote:
> So you mean we will have two fields in `struct Info`? Then what fields 
> will `ContainerInfo` have?
>   struct Info
>   {
> hashmap networkInfos;
> Option containerInfo;
>   }
> 
> How about just rename `struct NetworkInfo` to `struct ContainerNetwork` 
> and rename its field `network` to `cniNetworkInfo` like below?
>   struct ContainerNetwork
>   {
> std::string networkName;
> 
> std::string ifName;
> 
> mesos::NetworkInfo networkInfo;
> 
> Option cniNetworkInfo
>   };
> 
>   struct Info
>   {
> Info (const hashmap& 
> _containerNetworks)
>   : containerNetworks (_containerNetworks) {}
> 
> hashmap containerNetworks;
>   };

yeah, that's much better!


- Jie


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


On April 1, 2016, 2:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated April 1, 2016, 2:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45500: Fix Mesos-1104, linux/fs.hpp remove mesos::internal:: should be enough.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On April 1, 2016, 5:19 a.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45500/
> ---
> 
> (Updated April 1, 2016, 5:19 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Cong Wang.
> 
> 
> Bugs: MESOS-1104
> https://issues.apache.org/jira/browse/MESOS-1104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> follow haosdent's suggest, remove mesos::internal:: should be enough.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp b7420c682970c4838e84973198ac4fe7af5f68f9 
>   src/linux/fs.hpp 4525a5d5566e2bc913894e993ac7350f1bbd9cc0 
>   src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
>   src/slave/containerizer/mesos/mount.cpp 
> bf17b015e45579882fd31248e8609eec6d58a9da 
>   src/tests/containerizer/fs_tests.cpp 
> 020fd8a4bf5911671e038a96f8b50c0f58e22ed5 
>   src/tests/containerizer/port_mapping_tests.cpp 
> e062daa9fcfc776144b48325daa1f1284c5e59a4 
> 
> Diff: https://reviews.apache.org/r/45500/diff/
> 
> 
> Testing
> ---
> 
> make test on localhost(ubuntu)
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

> On April 2, 2016, 6:32 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, lines 93-95
> > 
> >
> > This is very confusing. We have both 'network' and 'networkInfo' in 
> > 'NetworkInfo'!
> > 
> > How about this: we introduce an optional 'ContainerInfo' in 'Info':
> > 
> > ```
> > struct Info
> > {
> >   Option containerInfo;
> > }
> > ```

So you mean we will have two fields in `struct Info`? Then what fields will 
`ContainerInfo` have?
  struct Info
  {
hashmap networkInfos;
Option containerInfo;
  }

How about just rename `struct NetworkInfo` to `struct ContainerNetwork` and 
rename its field `network` to `cniNetworkInfo` like below?
  struct ContainerNetwork
  {
std::string networkName;

std::string ifName;

mesos::NetworkInfo networkInfo;

Option cniNetworkInfo
  };

  struct Info
  {
Info (const hashmap& _containerNetworks)
  : containerNetworks (_containerNetworks) {}

hashmap containerNetworks;
  };


- Qian


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


On April 1, 2016, 10:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated April 1, 2016, 10:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).

[Yong Tang]

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



Thanks for all the input from every one. I updated the review request and 
merged the ascii check with mesos-style.py. I also skipped the .md check for 
now as there is another ticket MESOS-5077 for that. Let me know if there are 
other issues.

- Yong Tang


On April 2, 2016, 1:41 a.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> ---
> 
> (Updated April 2, 2016, 1:41 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Bernd 
> Mathiske, haosdent huang, Kevin Klues, Neil Conway, Vinod Kone, and Deshi 
> Xiao.
> 
> 
> Bugs: MESOS-4033
> https://issues.apache.org/jira/browse/MESOS-4033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review request tries to add addition check in mesos-style.pl
> for checking non-ascii characters. It scans .cpp, .hpp, .cc, .h
> files and report error if non-ascii characters exists.
> 
> As part of this review request, two non-ascii characters are identified
> in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
> accordingly.
> 
> Note: .md scan is skipped based on feedback from review request.
> 
> 
> Diffs
> -
> 
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   support/mesos-style.py 13616065ebe07ca401b385716d9b723f65bb2162 
> 
> Diff: https://reviews.apache.org/r/45033/diff/
> 
> 
> Testing
> ---
> 
> Tested manually and found two non ascii characters in docs/versioning.md 
> (fixed as part of this review request).
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).

[Yong Tang]

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

(Updated April 2, 2016, 1:41 a.m.)


Review request for mesos, Alexander Rukletsov, Benjamin Bannier, Bernd 
Mathiske, haosdent huang, Kevin Klues, Neil Conway, Vinod Kone, and Deshi Xiao.


Changes
---

Merge ascii check into mesos-style.py, leave .md check to MESOS-5077 according 
to feedbacks.


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


Repository: mesos


Description (updated)
---

This review request tries to add addition check in mesos-style.pl
for checking non-ascii characters. It scans .cpp, .hpp, .cc, .h
files and report error if non-ascii characters exists.

As part of this review request, two non-ascii characters are identified
in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
accordingly.

Note: .md scan is skipped based on feedback from review request.


Diffs (updated)
-

  docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
  support/mesos-style.py 13616065ebe07ca401b385716d9b723f65bb2162 

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


Testing
---

Tested manually and found two non ascii characters in docs/versioning.md (fixed 
as part of this review request).


Thanks,

Yong Tang

Re: Review Request 44674: Supported image name specified with private registry prefix.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44672, 44673, 44674]

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 1, 2016, 10:41 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44674/
> ---
> 
> (Updated April 1, 2016, 10:41 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4939
> https://issues.apache.org/jira/browse/MESOS-4939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported image name specified with private registry prefix.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/spec.hpp 5fb6647cc02aa64fa9305b1ec6db9d6d81caf5f7 
>   src/docker/spec.cpp 07c0b87ee1a3b6981adfc96f5a7add6f625374c1 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> d3eccd9902a7896d52e6c79b13bd389bb6380b8d 
> 
> Diff: https://reviews.apache.org/r/44674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh --gtest_filter="ProvisionerDockerRegistryPullerTest"
> 
> Tested with private registry localhost:80/ubuntu
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 45607: Move HTB qdisc out of containers.

[Cong Wang]

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

(Updated April 1, 2016, 11:05 p.m.)


Review request for mesos, Ian Downes and Jie Yu.


Changes
---

Updated counters


Summary (updated)
-

Move HTB qdisc out of containers.


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


Repository: mesos


Description (updated)
---

Move HTB qdisc out of containers.


Diffs (updated)
-

  src/linux/routing/queueing/fq_codel.hpp 
6fcd9a21e821da476e9c16f50ed781424338022a 
  src/linux/routing/queueing/fq_codel.cpp 
26860d1552839b47be2b4cbdc575c0d20e6fcdc5 
  src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 
  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
0fe2f486eb733acf738c1c61fc44f820d7401afc 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
323c84a3d960a196d8ba87f753814e9d43a07957 

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


Testing
---

Manual test with --egress_unique_flow_per_container and/or 
--egress_rate_limit_per_container


Thanks,

Cong Wang

Re: Review Request 45200: Duplicate flags with keywork 'slave' on master side.

[Vinod Kone]

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



Can you call this out in the CHANGELOG too please?

Also, all of the old flags should print a deprecation warning (when a user does 
--help on the binary or starts the binary with a deprecated flag) and direct 
users to the new flags. Can you make that change?

- Vinod Kone


On March 23, 2016, 7:39 a.m., Jay Guo wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45200/
> ---
> 
> (Updated March 23, 2016, 7:39 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> [https://issues.apache.org/jira/browse/MESOS-3781]
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 6552e48eab2708a28dd69adba3ec759cb5aeca4c 
>   src/master/flags.hpp f8d2cc4c6c8dab00e34ca737dbcb5b9ca3870d6d 
>   src/master/flags.cpp e6fea6421ea1a16b9cd78b0e42b830829b95ad61 
>   src/tests/flags_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45200/diff/
> 
> 
> Testing
> ---
> 
> ./configure --with-network-isolator
> make check
> 
> 
> 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/#review126613
---




src/slave/containerizer/mesos/isolators/disk/xfs.cpp (lines 501 - 503)


Chatted offline. 

Feels like the right thing to do here is to treat a container with no disk 
resource as `limit == 0`, whether the container started with zero disk resource 
or is updated to zero.

Given XFS' lack of ability to enforce disk quota below a basic block 
(512bytes), I think it's OK to do so and in the user doc make it clear that 
"with XFS isolator when no disk resource or zero disk resource is given, XFS 
will limit it at 512bytes (the size of a basic block)." In fact, XFS cannot do 
enforcement at a granularity below 512bytes. However since Mesos' disk resource 
granularity is more coarse than it (at 1KB, or 0.001 of 1MB), this shouldn't 
violate any expectations.

Given that this is a pretty strict behavior (in most cases the task will 
fail immediately because of stdout/stderr/downloads) I think we should act this 
way based on a flag, maybe only if `flags.enforce_container_disk_quota == 
true`, but it raises another question of whether XFS isolator should support 
monitoring-only mode. Therefore I think it can be punted to a later review, 
I'll raise a JIRA intead.


- Jiang Yan Xu


On March 31, 2016, 5:01 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44948/
> ---
> 
> (Updated March 31, 2016, 5:01 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 44674: Supported image name specified with private registry prefix.

[Gilbert Song]

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

(Updated April 1, 2016, 3:41 p.m.)


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


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


Repository: mesos


Description
---

Supported image name specified with private registry prefix.


Diffs (updated)
-

  include/mesos/docker/spec.hpp 5fb6647cc02aa64fa9305b1ec6db9d6d81caf5f7 
  src/docker/spec.cpp 07c0b87ee1a3b6981adfc96f5a7add6f625374c1 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
d3eccd9902a7896d52e6c79b13bd389bb6380b8d 

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


Testing
---

make check

sudo ./bin/mesos-test.sh --gtest_filter="ProvisionerDockerRegistryPullerTest"

Tested with private registry localhost:80/ubuntu


Thanks,

Gilbert Song

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 93 - 95)


This is very confusing. We have both 'network' and 'networkInfo' in 
'NetworkInfo'!

How about this: we introduce an optional 'ContainerInfo' in 'Info':

```
struct Info
{
  Option containerInfo;
}
```


- Jie Yu


On April 1, 2016, 2:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated April 1, 2016, 2:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45580: Add some log messages in 'network/cni' isolator.

[Jie Yu]

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


Fix it, then Ship it!





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


I won't add this LOG(INFO) since the bind mount logging implies this.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1032 - 1033)


You should put this log line in the if block.


- Jie Yu


On April 1, 2016, 9:54 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45580/
> ---
> 
> (Updated April 1, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add some log messages in 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44674: Supported image name specified with private registry prefix.

[Jie Yu]

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




src/docker/spec.cpp (lines 103 - 105)


Let's remove this. I think checking if 'registry' is empty or not should be 
done at the upper level.

If 'registry' is empty, just return None().



src/docker/spec.cpp (line 123)


Hum, I am thinking about returning Try here. Return 'https' if port 
!= 80 or not specified.



src/docker/spec.cpp (lines 125 - 127)


This is not needed.



src/docker/spec.cpp (line 142)


I think we should just return string here (instead of a Try). If registry 
is empty, return an empty string.


- Jie Yu


On April 1, 2016, 8:25 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44674/
> ---
> 
> (Updated April 1, 2016, 8:25 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4939
> https://issues.apache.org/jira/browse/MESOS-4939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported image name specified with private registry prefix.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/spec.hpp 5fb6647cc02aa64fa9305b1ec6db9d6d81caf5f7 
>   src/docker/spec.cpp 07c0b87ee1a3b6981adfc96f5a7add6f625374c1 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> d3eccd9902a7896d52e6c79b13bd389bb6380b8d 
> 
> Diff: https://reviews.apache.org/r/44674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh --gtest_filter="ProvisionerDockerRegistryPullerTest"
> 
> Tested with private registry localhost:80/ubuntu
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 45607: Moved HTB out of containers.

[Cong Wang]

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

Review request for mesos, Ian Downes and Jie Yu.


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


Repository: mesos


Description
---

Move HTB out of containers so that containers could borrow bandwidth when idle.


Diffs
-

  src/linux/routing/queueing/fq_codel.hpp 
6fcd9a21e821da476e9c16f50ed781424338022a 
  src/linux/routing/queueing/fq_codel.cpp 
26860d1552839b47be2b4cbdc575c0d20e6fcdc5 
  src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 
  src/slave/containerizer/mesos/isolators/network/port_mapping.hpp 
0fe2f486eb733acf738c1c61fc44f820d7401afc 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
323c84a3d960a196d8ba87f753814e9d43a07957 

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


Testing
---

Manual test with --egress_unique_flow_per_container and/or 
--egress_rate_limit_per_container


Thanks,

Cong Wang

Re: Review Request 44674: Supported image name specified with private registry prefix.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44672, 44673, 44674]

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 1, 2016, 8:25 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44674/
> ---
> 
> (Updated April 1, 2016, 8:25 p.m.)
> 
> 
> Review request for mesos, Artem Harutyunyan, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-4939
> https://issues.apache.org/jira/browse/MESOS-4939
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Supported image name specified with private registry prefix.
> 
> 
> Diffs
> -
> 
>   include/mesos/docker/spec.hpp 5fb6647cc02aa64fa9305b1ec6db9d6d81caf5f7 
>   src/docker/spec.cpp 07c0b87ee1a3b6981adfc96f5a7add6f625374c1 
>   src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
> d3eccd9902a7896d52e6c79b13bd389bb6380b8d 
> 
> Diff: https://reviews.apache.org/r/44674/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> sudo ./bin/mesos-test.sh --gtest_filter="ProvisionerDockerRegistryPullerTest"
> 
> Tested with private registry localhost:80/ubuntu
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 45606: Added default class to htb qdisc.

[Cong Wang]

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

Review request for mesos, Ian Downes and Jie Yu.


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


Repository: mesos


Description
---

Added default class to htb::create().


Diffs
-

  src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 
  src/linux/routing/queueing/htb.cpp faadf32bd48cc6bf968b1229789903c0d01fd75c 

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


Testing
---

make && make check


Thanks,

Cong Wang

Review Request 45605: Introduced HTB class.

[Cong Wang]

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

Review request for mesos, Ian Downes and Jie Yu.


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


Repository: mesos


Description
---

Introduced HTB class API, prepare for per-container bandwidth limit.


Diffs
-

  src/linux/routing/internal.hpp 8f68119819f7c79ece1a13ac1894b1802ddc8e19 
  src/linux/routing/queueing/discipline.hpp 
54d6b214ef6a38fd8279f6d01e6f4e3ccfddf634 
  src/linux/routing/queueing/htb.hpp 857646190d21387f98832f5094128505a52a0776 
  src/linux/routing/queueing/htb.cpp faadf32bd48cc6bf968b1229789903c0d01fd75c 
  src/linux/routing/queueing/internal.hpp 
768ed325f9b259e150779eb3ad74f4e5d4bcc7a2 

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


Testing
---

make && make check


Thanks,

Cong Wang

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

[Vinod Kone]

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




src/examples/long_lived_framework.cpp (line 51)


I think you should add a comment here about what the high level behavior of 
this scheduler is. For example that this schedule tries to pick one slave and 
launch as many tasks on it as possible, using a single multi-task executor. And 
that if the slave or executor fails, it picks another slave and repeats the 
process.



src/examples/long_lived_framework.cpp (line 64)


Any particular reason you are using "cout" instead of LOG? It would be 
great to have timestamps, esp if you are planning to run this in a test cluster.



src/examples/long_lived_framework.cpp (line 86)


Can we use "foreach(const Offer& offer, offers)" here?



src/examples/long_lived_framework.cpp (line 87)


why do you need a vector here? AFAICT, you only ever call launchTasks with 
a single task. If that's the case just pass '{task}' to driver.launchTasks().



src/examples/long_lived_framework.cpp (line 92)


s/offer/offer is/



src/examples/long_lived_framework.cpp (lines 104 - 150)


This is still a bit hard to follow. 

How about:

```
foreach (const Offer& offer : offers) {

  if (slaveID.isNone() {
// No active executor running in the cluster. Launch a new task with 
executor.
  
if (offer.resources < (task + executor).resources()) {
   // Not enough resources. Decline.
   decline();
} else {
   launch();
}
  } else if (slaveID == offer.slaveID() {
// Offer from the same slave that has an active executor. Launch more 
tasks on that executor.

if (offer.resources < task.executors()) {
  // Noe enough resources. Decline.
  decline();
} else {
  launch();
}
  } else {
// Offer from a slave different from the slave that has an active 
executor. Decline.
decline();
  }
}

```



src/examples/long_lived_framework.cpp (line 177)


log a message here.



src/examples/long_lived_framework.cpp (line 186)


log a message here.


- Vinod Kone


On March 29, 2016, 7:08 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45067/
> ---
> 
> (Updated March 29, 2016, 7:08 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
> ---
> 
> 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
> -
> 
>   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 44934: Updated mesos-execute to add support for Appc.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45259, 44934]

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 1, 2016, 8:05 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated April 1, 2016, 8:05 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4978
> https://issues.apache.org/jira/browse/MESOS-4978
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated mesos-execute to add support for Appc.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 44674: Supported image name specified with private registry prefix.

[Gilbert Song]

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

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


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


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


Repository: mesos


Description
---

Supported image name specified with private registry prefix.


Diffs (updated)
-

  include/mesos/docker/spec.hpp 5fb6647cc02aa64fa9305b1ec6db9d6d81caf5f7 
  src/docker/spec.cpp 07c0b87ee1a3b6981adfc96f5a7add6f625374c1 
  src/slave/containerizer/mesos/provisioner/docker/registry_puller.cpp 
d3eccd9902a7896d52e6c79b13bd389bb6380b8d 

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


Testing
---

make check

sudo ./bin/mesos-test.sh --gtest_filter="ProvisionerDockerRegistryPullerTest"

Tested with private registry localhost:80/ubuntu


Thanks,

Gilbert Song

Re: Review Request 44934: Updated mesos-execute to add support for Appc.

[Jojy Varghese]

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

(Updated April 1, 2016, 8:05 p.m.)


Review request for mesos and Jie Yu.


Changes
---

rebased.


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


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp af62f41622e1c43acd8f257c54f8754162c433b8 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45571]

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 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 44544: Moved contender and detector definitions into separate directories.

[Anurag Singh]

> On March 31, 2016, 5:25 p.m., Kapil Arya wrote:
> > src/master/contenders/contender.cpp, lines 58-61
> > 
> >
> > I am not sure if I understand this one here. Can you elaborate a bit?

Dropped this issue since this part of the code was copied verbatim.


- Anurag


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


On March 23, 2016, 11:05 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44544/
> ---
> 
> (Updated March 23, 2016, 11:05 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Kapil Arya.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Moved contender and detector definitions into separate directories.
> 
> 
> Diffs
> -
> 
>   src/master/contenders/contender.hpp PRE-CREATION 
>   src/master/contenders/contender.cpp PRE-CREATION 
>   src/master/detectors/detector.hpp PRE-CREATION 
>   src/master/detectors/detector.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44544/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44670/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 42861: Ensure two Mount Disk resources do not have the same root path.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [42860, 42861]

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 1, 2016, 4:43 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/42861/
> ---
> 
> (Updated April 1, 2016, 4:43 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jie Yu, Joris Van Remoortere, and 
> Neil Conway.
> 
> 
> Bugs: MESOS-4521
> https://issues.apache.org/jira/browse/MESOS-4521
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Ensure two Mount Disk resources do not have the same root path.
> 
> 
> Diffs
> -
> 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
> 
> Diff: https://reviews.apache.org/r/42861/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45537: Updated testing `Environment` to remove temp dirs after each test.

[Gilbert Song]

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


Fix it, then Ship it!




Verified the patch fix the tests teardown issue in a test suite, by testing on 
small-size disk VMs with some out-of-disk fix commits reverted.


src/tests/environment.cpp (line 728)


Should we use 
`listeners.Append(Owned(temporaryDirectoryEventListener));`
 here?


- Gilbert Song


On March 31, 2016, 5:17 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45537/
> ---
> 
> (Updated March 31, 2016, 5:17 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jie Yu, and Vinod Kone.
> 
> 
> Bugs: MESOS-5044
> https://issues.apache.org/jira/browse/MESOS-5044
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated testing `Environment` to remove temp dirs after each test.
> 
> 
> Diffs
> -
> 
>   src/tests/environment.hpp 9932f7c530dee3180bbbdf6c06e1e0ec424b46b9 
>   src/tests/environment.cpp 7617e43587cb81104786d06f753f08565a6c2d0a 
> 
> Diff: https://reviews.apache.org/r/45537/diff/
> 
> 
> Testing
> ---
> 
> Verified that make check doesn't leak any temporary directories.
> The exception seems to be some mesos-X directories created by example 
> frameworks.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 45033: Add a commit hook for checking non-ascii characters (MESOS-4033).

[Benjamin Bannier]

> On April 1, 2016, 6:53 p.m., Kevin Klues wrote:
> > As Neil said, I would also recommend *not* excluding the docs files. In the 
> > rare case that we need non-ascii cahracters in the docs, we should use HTML 
> > character entities.
> > 
> > I would also not create a whole new python script for this.  It should be a 
> > simple addition to the existing mesos-style.py script.

Shouldn't we switch to a markdown converter which can automatically translate 
unicode to HTML entities (kramdown appears to be capable of this) instead of 
writing even  more markdown documentation tailored to just one output format 
(here: HTML)? If we'd stick to clean markdown we might even be able to generate 
documentation in different formats in the future (e.g., man pages or info docs 
with Pandoc come to mind).

I filed MESOS-5077 so we can independently of the issues discussed here verify 
the validity of generated HTML (something worth tackling on its own right), but 
I think a fix there might well be able to address your and Neil's concerns 
expressed here.


- Benjamin


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


On March 23, 2016, 3:46 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> ---
> 
> (Updated March 23, 2016, 3:46 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Bernd Mathiske, haosdent 
> huang, Neil Conway, and Deshi Xiao.
> 
> 
> Bugs: MESOS-4033
> https://issues.apache.org/jira/browse/MESOS-4033
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This review request tries to add a commit hook for checking non-ascii
> characters. It scans .cpp, .hpp, .cc, .h files and report
> error if non-ascii characters exists.
> 
> As part of this review request, two non-ascii characters are identified
> in versioning.md (one in Ln 85 and another in Ln 96) and are corrected
> accordingly.
> 
> Note: .md scan is skipped based on feedback from review request.
> 
> 
> Diffs
> -
> 
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   support/hooks/post-rewrite e3747a320fef0b71c06bcf0f2c5532958c416646 
>   support/hooks/pre-commit 10838a4c99db2a8318d64f95d90d2c2c90150453 
>   support/non-ascii.py PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45033/diff/
> 
> 
> Testing
> ---
> 
> Tested manually and found two non ascii characters in docs/versioning.md 
> (fixed as part of this review request).
> 
> 
> Thanks,
> 
> Yong Tang
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45590: Made `Delegate` and `Handlers` libprocess tests less fragile.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45318, 45319, 45488, 45489, 45590]

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 1, 2016, 4:22 p.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45590/
> ---
> 
> (Updated April 1, 2016, 4:22 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> As previously written, these tests depended on the fact that `post`
> will synchronously deliver a message to a local PID.  That is an
> implementation detail that seems unwise to rely upon. Instead, it is
> quite easy to arrange for both tests to block until the effect of
> the `post` has occurred.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/process_tests.cpp 
> 6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd 
> 
> Diff: https://reviews.apache.org/r/45590/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Without this patch, the libprocess tests fail if the "local message" 
> short-circuit in `transport` is disabled (circa line 556). With the patch, 
> the tests succeed.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?
> 
> Jie Yu wrote:
> Yes, but this is not possible, right? If that happens, human needs to be 
> involved.
> 
> Avinash sridharan wrote:
> Yeah, we can't handle this condition. Should we add a `LOG(ERROR)` here 
> or would this be highlighted upstream? Wanted to see if we can explicitly 
> indicate this condition to the operator.

I think the Error message will be visible to the operator? Why you need a 
LOG(ERROR) here. The agent (or containerizer) will do the LOG(ERROR).


- Jie


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


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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

[James Peach]

> On April 1, 2016, 7:27 a.m., Jiang Yan Xu wrote:
> > src/tests/cluster.cpp, lines 434-436
> > 
> >
> > This can be tackled in another review because a) the tests here don't 
> > use cluster.cpp and b) I see other issuse here as well (see below).

The isolator tests use ``cluster.cpp``. I hit this when I had a bug in those 
tests that passed invalid flags arguments to the Slave. Let me know whether you 
want me to move or just drop this diff. However, it is a fairly annoying 
problem to debug.


- James


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


On April 1, 2016, midnight, James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 1, 2016, midnight)
> 
> 
> 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/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   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 45571: A few cleanups and simplifications in CNI isolator.

[Avinash sridharan]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?
> 
> Jie Yu wrote:
> Yes, but this is not possible, right? If that happens, human needs to be 
> involved.

Yeah, we can't handle this condition. Should we add a `LOG(ERROR)` here or 
would this be highlighted upstream? Wanted to see if we can explicitly indicate 
this condition to the operator.


- Avinash


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


On April 1, 2016, 5:36 p.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 5:36 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

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

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


Review request for mesos, Avinash sridharan and Qian Zhang.


Changes
---

Review comments.


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


Repository: mesos


Description
---

A few cleanups and simplifications in CNI isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3a07540909ed771d1bd3b22888e04d5fb451710d 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
486c382365d5293cd9d53b8b239f70a543c46792 

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


Testing
---

make check


Thanks,

Jie Yu

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

[James Peach]

> On April 1, 2016, 7: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.

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


- James


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


On April 1, 2016, midnight, James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44947/
> ---
> 
> (Updated April 1, 2016, midnight)
> 
> 
> 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/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   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 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 374
> > 
> >
> > s/on/only
> > instead of `non-host network(s)` maybe `joined a CNI network(s) ?
> > 
> > Didn't follow the last part of the sentence "and cleanup _might_ be 
> > required for that container." What does this mean ?

Add a few more to explain that does that mean.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 429
> > 
> >
> > You had mentioned that if we return an error during `recover` the agent 
> > will restart. This state from checkpointed information that is not 
> > changing. After restart would the Agent end up hitting this condition again 
> > ?

Yes, but this is not possible, right? If that happens, human needs to be 
involved.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 475
> > 
> >
> > s/crashes/crashed/
> > Why would cleanup be called on this container? Cause they are `orphans`?

No, the agent hasn't realized that the container cleanup has done. So after 
restart, it'll call cleanup again.


> On April 1, 2016, 4:46 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1016
> > 
> >
> > Maybe "failed to detech container " + containerId + " from network '" 
> > .

'containerId' is not needed here because the caller will print it.


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45319: Cleaned up various code in libprocess.

[Neil Conway]

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

(Updated April 1, 2016, 4:20 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

More cleanup.


Repository: mesos


Description
---

Cleaned up various code in libprocess.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 
  3rdparty/libprocess/src/tests/benchmarks.cpp 
e8bbc55f0f9aeabe7612a2ced5299cc01202b1f6 
  3rdparty/libprocess/src/tests/process_tests.cpp 
6b3aa1bcf20466cdf8f8249988b8b06dec27e5cd 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 45318: Improved libprocess comments.

[Neil Conway]

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

(Updated April 1, 2016, 4:20 p.m.)


Review request for mesos and Ben Mahler.


Changes
---

More comment improvements.


Summary (updated)
-

Improved libprocess comments.


Repository: mesos


Description (updated)
---

Improved libprocess comments.


Diffs (updated)
-

  3rdparty/libprocess/include/process/socket.hpp 
4cb49680d1304899a4ee675ac07379e51d9c55b1 
  3rdparty/libprocess/src/process.cpp feaffa4334422ec3964f8d079f570061eaf390d2 

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


Testing
---

make check


Thanks,

Neil Conway

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Avinash sridharan]

> On March 31, 2016, 6:31 p.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 786
> > 
> >
> > Shouldn't we return a failure?
> 
> Qian Zhang wrote:
> I think we should not return a failure because there can be some 
> containers do not want to opt in CNI networks.

I see your point. Looks like we are not storing container information for 
containers that are not connected to a CNI network. This behavior is different 
compared to other isolators (they store container information for all 
containers). We should definitely add a comment here explaining this behavior.


- Avinash


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


On April 1, 2016, 2:17 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated April 1, 2016, 2:17 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4764
> https://issues.apache.org/jira/browse/MESOS-4764
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45582: Fixed Cmake build by adding CNI_SPEC to MESOS_PROTOBUF_SRC.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On April 1, 2016, 1:11 p.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45582/
> ---
> 
> (Updated April 1, 2016, 1:11 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Alex Clemmer, Jie Yu, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> FIxed Cmake build by adding CNI_SPEC to MESOS_PROTOBUF_SRC.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 366a7ae8f6ef1d55202699df0502a30f15a35e1f 
> 
> Diff: https://reviews.apache.org/r/45582/diff/
> 
> 
> Testing
> ---
> 
> cmake ..
> make
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 45571: A few cleanups and simplifications in CNI isolator.

[Jie Yu]

> On April 1, 2016, 2:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 418-424
> > 
> >
> > I do not think we need this. If agent crashes after removing the 
> > interface directory in `_detach`, then we should not get into this 
> > `foreach` loop because `networkNames` returned by `paths::getNetworkNames` 
> > should be empty.
> 
> Jie Yu wrote:
> Hum, I don't think we delete network dir, do we? We delete ifdir first, 
> if all are successful, we delete containerDir. But it's possible that we 
> delete the ifdir, but agent crashes before the containerDir is deleted, right?
> 
> Qian Zhang wrote:
> Yes, you are right. However, since there is only one ifdir in a network 
> dir, I am thinking we may change `_detach` to remove the network dir rather 
> than remove the ifdir and leave an empty network dir there, and then here we 
> will not need this code:
>   if (interfaces->empty()) {
> continue;
>   }

We will support adding more than one interface to a network dir, so I think the 
approach in my patch is better.


- Jie


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


On April 1, 2016, 1:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 1:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

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



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 1, 2016, 2:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 1, 2016, 2:14 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 44660: Used `KillPolicy` and shutdown grace period in docker executor.

[Alexander Rukletsov]

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

(Updated April 1, 2016, 3:38 p.m.)


Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.


Changes
---

Updated testing section.


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


Repository: mesos


Description
---

The docker executor determines how much time it allots the
underlying container to clean up (via passing the timeout to
the docker daemon) based on both optional task's `KillPolicy`
and optional `shutdown_grace_period` field in `ExecutorInfo`.


Diffs
-

  include/mesos/mesos.proto 59f5d3aadd1cb32c1eb869451ad55d877bdeee28 
  include/mesos/v1/mesos.proto 6556a1c243d227fdb9af0d3c1063fed08d3f6c8b 
  src/docker/executor.cpp afc769d0887e3842106e4c350e94c95c8ffc085e 

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


Testing (updated)
---

On CentOS 7.2.1511:
`make check`

To test the newly introduced protobuf fields, I've modified "mesos-execute" to
support these fields. I've inserted these lines in `offers()` method:
```
task.mutable_kill_policy()->mutable_grace_period()->
set_nanoseconds(Seconds().ns());
```

I've also implemented a "--no-shell" flag to support
setting `CommandInfo.shell` to `false`. An example command used in testing:
```
./src/mesos-execute --master=192.99.40.208:5551 --name=test --no-shell 
--env='{"GLOG_v": 1}' --containerizer=docker 
--docker_image=rukletsov/unresponsive-process
```

Mesos master is launched via:
```
./bin/mesos-master.sh --work_dir=m/work --port=5551
```

Mesos agent is launched via:
```
sudo su
GLOG_v=1 ./bin/mesos-slave.sh --work_dir=/tmp --master=192.99.40.208:5551 
--port=5552 --containerizers=docker
```

In order to test the docker stop timeout, which is what the docker executor
uses currently for kill policy, I've created a tiny application that ignores
SIGTERM (https://github.com/rukletsov/unresponsive-process ), wrapped it in a
docker container and published on docker hub
(https://hub.docker.com/r/rukletsov/unresponsive-process/ ).

Test 1.1

`KillPolicy.grace_period` is **not set**, `shell=false`, framework shutdown.
Framework is asked to shutdown several seconds after the task is started. This
results in agent asking executor to shutdown. From the agent's logs, we see that
the command executor sends a TASK_KILLED update in approx. **3 seconds** (the
default value is **5 seconds**, but we deduct some buffer to make sure the task 
can
be reaped and TASK_KILLED is sent) after it has been asked to shut down:
```
I0401 10:40:08.641249 13266 slave.cpp:2215] Asked to shut down framework 
1615ae6e-e907-4296-8d46-d864fae3359b- by master@192.99.40.208:5551
I0401 10:40:08.641286 13266 slave.cpp:2240] Shutting down framework 
1615ae6e-e907-4296-8d46-d864fae3359b-
I0401 10:40:08.641402 13266 slave.cpp:4398] Shutting down executor 'test' of 
framework 1615ae6e-e907-4296-8d46-d864fae3359b- at 
executor(1)@192.99.40.208:51023
I0401 10:40:11.837626 13271 slave.cpp:3184] Handling status update TASK_KILLED 
(UUID: e3388b10-6bd0-4571-ad08-30b77550392e) for task test of framework 
1615ae6e-e907-4296-8d46-d864fae3359b- from exec
utor(1)@192.99.40.208:51023
```

In an excerpt from the executor's log we see, that docker timeout is set to 3s:
```
I0401 10:40:08.641784 13360 exec.cpp:399] Executor asked to shutdown
I0401 10:40:08.641891 13368 exec.cpp:91] Scheduling shutdown of the executor in 
5secs
I0401 10:40:08.641902 13360 exec.cpp:414] Executor::shutdown took 19573ns
I0401 10:40:08.642026 13365 docker.cpp:134] 'docker -H 
unix:///var/run/docker.sock run --cpu-shares 1024 --memory 134217728 -e 
GLOG_v=2 -e MESOS_SANDBOX=/mnt/mesos/sandbox -e 
MESOS_CONTAINER_NAME=mesos-ef6f8d28-d279-40f7-b66a-f9659ca72299-S1.379cbab7-1ba4-4901-b8fa-b717bd1493f4
 -v 
/tmp/slaves/ef6f8d28-d279-40f7-b66a-f9659ca72299-S1/frameworks/1615ae6e-e907-4296-8d46-d864fae3359b-/executors/test/runs/379cbab7-1ba4-4901-b8fa-b717bd1493f4:/mnt/mesos/sandbox
 --net host --name 
mesos-ef6f8d28-d279-40f7-b66a-f9659ca72299-S1.379cbab7-1ba4-4901-b8fa-b717bd1493f4
 rukletsov/unresponsive-process' is being discarded
I0401 10:40:08.667981 13365 docker.cpp:712] Running docker -H 
unix:///var/run/docker.sock stop -t 3 
mesos-ef6f8d28-d279-40f7-b66a-f9659ca72299-S1.379cbab7-1ba4-4901-b8fa-b717bd1493f4
I0401 10:40:11.836829 13363 exec.cpp:535] Executor sending status update 
TASK_KILLED (UUID: e3388b10-6bd0-4571-ad08-30b77550392e) for task test of 
framework 1615ae6e-e907-4296-8d46-d864fae3359b-
```

Test 1.2

`KillPolicy.grace_period` is **not set**, `shell=true`, framework shutdown.
The only difference to test 1.1 is `shell=true` and as required by Mesos in this
case, `command` is set to container's entrypoint. Similar behavior is observed:
```
I0401 11:00:41.629098 14162 exec.cpp:312] Executor 

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




docs/app-framework-development-guide.md (line 184)


Here and below, I would personally rephrase this slightly.

"Any remaining resources remaining (i.e., those that are not used by the 
launched tasks or their executors) will be considered unused. Note that this 
includes resources used by tasks that the framework attempted to launch but 
failed (with TASK_ERROR) due to a malformed task description."


- Neil Conway


On April 1, 2016, 2:14 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45039/
> ---
> 
> (Updated April 1, 2016, 2:14 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 45571: A few cleanups and simplifications in CNI isolator.

[Qian Zhang]

> On April 1, 2016, 10:55 a.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 418-424
> > 
> >
> > I do not think we need this. If agent crashes after removing the 
> > interface directory in `_detach`, then we should not get into this 
> > `foreach` loop because `networkNames` returned by `paths::getNetworkNames` 
> > should be empty.
> 
> Jie Yu wrote:
> Hum, I don't think we delete network dir, do we? We delete ifdir first, 
> if all are successful, we delete containerDir. But it's possible that we 
> delete the ifdir, but agent crashes before the containerDir is deleted, right?

Yes, you are right. However, since there is only one ifdir in a network dir, I 
am thinking we may change `_detach` to remove the network dir rather than 
remove the ifdir and leave an empty network dir there, and then here we will 
not need this code:
  if (interfaces->empty()) {
continue;
  }


- Qian


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


On April 1, 2016, 9:08 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45571/
> ---
> 
> (Updated April 1, 2016, 9:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Qian Zhang.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A few cleanups and simplifications in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 3a07540909ed771d1bd3b22888e04d5fb451710d 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45571/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated April 1, 2016, 10:17 p.m.)


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented status() method of "network/cni" isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3a07540909ed771d1bd3b22888e04d5fb451710d 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
486c382365d5293cd9d53b8b239f70a543c46792 

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


Testing
---


Thanks,

Qian Zhang

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 1, 2016, 2:14 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 45575: Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45083, 45084, 45350, 45085, 45086, 45087, 45351, 45352, 
45353, 45354, 45362, 45363, 45364, 45472, 45473, 43283, 43284, 45573, 45574, 
45575]

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 1, 2016, 10:45 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45575/
> ---
> 
> (Updated April 1, 2016, 10:45 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5047
> https://issues.apache.org/jira/browse/MESOS-5047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> c4e467c8227f9e4129b05d173812592f39a04e06 
> 
> Diff: https://reviews.apache.org/r/45575/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45582: Fixed Cmake build by adding CNI_SPEC to MESOS_PROTOBUF_SRC.

[Joerg Schad]

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

(Updated April 1, 2016, 11:53 a.m.)


Review request for mesos, Benjamin Bannier, Alex Clemmer, and Till Toenshoff.


Repository: mesos


Description
---

FIxed Cmake build by adding CNI_SPEC to MESOS_PROTOBUF_SRC.


Diffs
-

  src/CMakeLists.txt 366a7ae8f6ef1d55202699df0502a30f15a35e1f 

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


Testing
---

cmake ..
make


Thanks,

Joerg Schad

Re: Review Request 45580: Add some log messages in 'network/cni' isolator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082, 
45383, 45531, 45532, 45546, 45580]

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 1, 2016, 9:54 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45580/
> ---
> 
> (Updated April 1, 2016, 9:54 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add some log messages in 'network/cni' isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 486c382365d5293cd9d53b8b239f70a543c46792 
> 
> Diff: https://reviews.apache.org/r/45580/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45577: Updated a note about quota limit in the documentation.

[Alexander Rukletsov]

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


Ship it!




Ship It!

- Alexander Rukletsov


On April 1, 2016, 10:25 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45577/
> ---
> 
> (Updated April 1, 2016, 10:25 a.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   docs/quota.md 12696bf805d43f997d80149e56281c5e7dc0557e 
> 
> Diff: https://reviews.apache.org/r/45577/diff/
> 
> 
> Testing
> ---
> 
> None: Not a functional change.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 43284: Wait for process:reap in PerfEventIsolatorTest.ROOT_CGROUPS_Sample.

[haosdent huang]

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

(Updated April 1, 2016, 10:47 a.m.)


Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett.


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


Repository: mesos


Description
---

Wait for process:reap in PerfEventIsolatorTest.ROOT_CGROUPS_Sample.


Diffs
-

  src/tests/containerizer/isolator_tests.cpp 
7e16846e97e20a7ef058dd8ae4664fa1c554283d 

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


Testing
---

Without this patch, when running 
```
sudo GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="PerfEventIsolatorTest.ROOT_CGROUPS_Sample" --verbose
```
, would got this error
```
[--] Global test environment tear-down
../../src/tests/environment.cpp:732: Failure
Failed
Tests completed with child processes remaining:
-+- 16501 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests 
--gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
 |-+- 16580 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests 
--gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
 | -+- 16582 perf stat --all-cpus --field-separator , --log-fd 1 --event cycles 
--cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 --event task-clock --cgroup 
mesos/239d30bb-f7a1-413b-9d99-0914149d5899 -- sleep 0.25
 |   --- 16584 sleep 0.25
 --- 16581 ()
[==] 1 test from 1 test case ran. (4095 ms total)
```

This also fix similar error in 
`MesosContainerizerSlaveRecoveryTest.CGROUPS_ROOT_PerfRollForward` and 
`CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`.


Thanks,

haosdent huang

Re: Review Request 43284: Wait for process:reap in PerfEventIsolatorTest.ROOT_CGROUPS_Sample.

[haosdent huang]

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

(Updated April 1, 2016, 10:45 a.m.)


Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett.


Summary (updated)
-

Wait for process:reap in PerfEventIsolatorTest.ROOT_CGROUPS_Sample.


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


Repository: mesos


Description (updated)
---

Wait for process:reap in PerfEventIsolatorTest.ROOT_CGROUPS_Sample.


Diffs (updated)
-

  src/tests/containerizer/isolator_tests.cpp 
7e16846e97e20a7ef058dd8ae4664fa1c554283d 

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


Testing
---

Without this patch, when running 
```
sudo GLOG_v=1 ./bin/mesos-tests.sh 
--gtest_filter="PerfEventIsolatorTest.ROOT_CGROUPS_Sample" --verbose
```
, would got this error
```
[--] Global test environment tear-down
../../src/tests/environment.cpp:732: Failure
Failed
Tests completed with child processes remaining:
-+- 16501 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests 
--gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
 |-+- 16580 /home/haosdent/mesos/build/src/.libs/lt-mesos-tests 
--gtest_filter=PerfEventIsolatorTest.ROOT_CGROUPS_Sample --verbose
 | -+- 16582 perf stat --all-cpus --field-separator , --log-fd 1 --event cycles 
--cgroup mesos/239d30bb-f7a1-413b-9d99-0914149d5899 --event task-clock --cgroup 
mesos/239d30bb-f7a1-413b-9d99-0914149d5899 -- sleep 0.25
 |   --- 16584 sleep 0.25
 --- 16581 ()
[==] 1 test from 1 test case ran. (4095 ms total)
```

This also fix similar error in 
`MesosContainerizerSlaveRecoveryTest.CGROUPS_ROOT_PerfRollForward` and 
`CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`.


Thanks,

haosdent huang

Re: Review Request 45574: Add `PerfEventSubsystem` for cgroups unified isolator.

[haosdent huang]

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

(Updated April 1, 2016, 10:45 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add `PerfEventSubsystem` for cgroups unified isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystem.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/cgroups/subsystem.cpp PRE-CREATION 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45575: Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.

[haosdent huang]

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

(Updated April 1, 2016, 10:45 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.


Diffs (updated)
-

  src/tests/containerizer/cgroups_isolator_tests.cpp 
c4e467c8227f9e4129b05d173812592f39a04e06 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 43283: Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.

[haosdent huang]

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

(Updated April 1, 2016, 10:45 a.m.)


Review request for mesos, Ian Downes, Jan Schlicht, and Paul Brett.


Changes
---

Rebase.


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


Repository: mesos


Description (updated)
---

Fix PerfEventIsolatorTest.ROOT_CGROUPS_Sample failed on CentOS 7.1.


Diffs (updated)
-

  src/linux/perf.cpp 749e676aaf2ce639dd976f2b23e323300c6114c5 

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


Testing
---

This also fix similar error in 
`CgroupsAnyHierarchyWithPerfEventTest.ROOT_CGROUPS_Perf`.

# Note: This patch would discarded eventually.


Thanks,

haosdent huang

Re: Review Request 45573: Add PerfEventHandleManager.

[haosdent huang]

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

(Updated April 1, 2016, 10:45 a.m.)


Review request for mesos and Jie Yu.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Add PerfEventHandleManager.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/perf_event.hpp 
65e731886b9e5cac07ae3ad6398faf8f50de5650 
  src/slave/containerizer/mesos/isolators/cgroups/perf_event.cpp 
5ef4ae5c468580352cd16e7716b9ca4c0acde659 

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


Testing
---


Thanks,

haosdent huang

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 1, 2016, 12:35 p.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 (updated)
-

  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 45213: Change 'slave' to 'agent' in all the logs and standard output.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45213]

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 1, 2016, 6:23 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45213/
> ---
> 
> (Updated April 1, 2016, 6:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: mesos-5055
> https://issues.apache.org/jira/browse/mesos-5055
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the first patch to solve [MESOS-3780]
> 
> 
> Diffs
> -
> 
>   CHANGELOG b90078d41357c29c9102df00a735bde460e797bb 
>   src/docker/executor.cpp 26cc88d35fc7cd913c5105f5fc4e6ae71ce59279 
>   src/examples/balloon_framework.cpp 1665e5bd35ad9e029372e55209802c2827be658f 
>   src/examples/no_executor_framework.cpp 
> 9223d954197ebd097f57efea5299c917b24fe41e 
>   src/examples/persistent_volume_framework.cpp 
> 9ce43d2b7da3f03d24473ff22cb62a88004f5f53 
>   src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
>   src/exec/exec.cpp 8f672602daf090dec032d2b684e407e5d043af9c 
>   src/local/local.cpp e777ea2938a23db8b407676a0f7e635e63d032fa 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
>   src/master/main.cpp 58561cffa440aaf1293e9ffe19b5685e6d2f1952 
>   src/master/master.hpp 1751ee1fc4d3e15df018be8bfd4f8ef2a0e32abd 
>   src/master/master.cpp 655b0b8d0156b44914578a1241beb1fb7c92ca23 
>   src/master/validation.cpp 9c9e42283baa6e49d86af2ce7222131ce53ccaff 
>   src/sched/sched.cpp d989cb5f35d314dc2e03617bfdbd4b18d4efcf91 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
>   src/slave/containerizer/mesos/launch.cpp 
> dc7bebf8d0f3e36c6d41af4e0e8ffc438b6cd86c 
>   src/slave/main.cpp f5f21422c7cb94506b78cebf7f7341b5a078f3b2 
>   src/slave/paths.cpp e0451a8856063f78c8f2aeed11c01f3560b4ed1c 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
>   src/slave/state.cpp 75bdfe144d3307dc74bf48e6a0d403fbea568a6d 
>   src/slave/status_update_manager.cpp 
> f6a9b3b6616abee9c55bf731855ee9b3b98606ac 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/hierarchical_allocator_tests.cpp 
> 8f78a204d296f94f515f21511710a35c33e27255 
>   src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
>   src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 
>   src/watcher/whitelist_watcher.cpp 3d0d5828ef095884ca87156d69f963f04dba40a1 
> 
> Diff: https://reviews.apache.org/r/45213/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 45577: Updated a note about quota limit in the documentation.

[Joerg Schad]

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

(Updated April 1, 2016, 10:25 a.m.)


Review request for mesos and Alexander Rukletsov.


Summary (updated)
-

Updated a note about quota limit in the documentation.


Repository: mesos


Description (updated)
---

See summary.


Diffs
-

  docs/quota.md 12696bf805d43f997d80149e56281c5e7dc0557e 

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


Testing (updated)
---

None: Not a functional change.


Thanks,

Joerg Schad

Re: Review Request 45082: Implemented cleanup() method of "network/cni" isolator.

[Qian Zhang]

> On April 1, 2016, 6:22 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 781
> > 
> >
> > Can you add some LOG(INFO) here. Maybe in a subsequent patch.

Sure, fixed it in a new patch: https://reviews.apache.org/r/45580/


> On April 1, 2016, 6:22 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 788
> > 
> >
> > Ditto on adding some LOG(INFO) here.

Ditto.


- Qian


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


On March 31, 2016, 7:28 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 31, 2016, 7:28 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented cleanup() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> 873e0c52475f4868e611bd24a6782ad5eb261a99 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 45580: Add some log messages in 'network/cni' isolator.

[Qian Zhang]

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

Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Add some log messages in 'network/cni' isolator.


Diffs
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
486c382365d5293cd9d53b8b239f70a543c46792 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

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

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


Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.


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


Repository: mesos


Description
---

Implemented status() method of "network/cni" isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
3a07540909ed771d1bd3b22888e04d5fb451710d 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
486c382365d5293cd9d53b8b239f70a543c46792 

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 45429: Added authentication to the '/registry' endpoint.

[Adam B]

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


Fix it, then Ship it!




Looks great! I'll fix these little bits and commit this.


src/master/registrar.hpp (line 130)


Why a `UPID` instead of a `process::PID pid()`?



src/master/registrar.cpp (lines 230 - 231)


"The authentication realm, if any, into which this process' endpoints will 
be installed."


- Adam B


On March 29, 2016, 8:46 a.m., Jan Schlicht wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45429/
> ---
> 
> (Updated March 29, 2016, 8:46 a.m.)
> 
> 
> Review request for mesos, Adam B, Joerg Schad, and Till Toenshoff.
> 
> 
> Bugs: MESOS-4933
> https://issues.apache.org/jira/browse/MESOS-4933
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/local/local.cpp e777ea2938a23db8b407676a0f7e635e63d032fa 
>   src/master/main.cpp 58561cffa440aaf1293e9ffe19b5685e6d2f1952 
>   src/master/registrar.hpp f087569bd6774ac16093f9204e6870e4d4404420 
>   src/master/registrar.cpp def40b94689c363617a7cfbcce82ea8d357cb345 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 
> 
> Diff: https://reviews.apache.org/r/45429/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jan Schlicht
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

> On April 1, 2016, 6:04 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 791-792
> > 
> >
> > hum, what about labels and groups in NetworkInfo? I think we should 
> > save a copy during launch/recover, and fill the missing ip if needed here.

Agree, will fix it soon.


- Qian


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


On March 31, 2016, 10:55 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated March 31, 2016, 10:55 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45360: WIP: Added dvd client for mount.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45214, 45217, 45265, 45270, 45360]

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 1, 2016, 5:53 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45360/
> ---
> 
> (Updated April 1, 2016, 5:53 a.m.)
> 
> 
> Review request for mesos, Gilbert Song and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added dvd client for mount.
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 366a7ae8f6ef1d55202699df0502a30f15a35e1f 
>   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
> ---
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 45533: Explicitly typed quota role sorter in Mesos allocator.

[Benjamin Bannier]

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

(Updated April 1, 2016, 10:07 a.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Removed accidential trailing whitespace (no functional changes)


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


Repository: mesos


Description
---

Explicitly typed quota role sorter in Mesos allocator.


Diffs (updated)
-

  src/master/allocator/mesos/hierarchical.hpp 
e979fdf60da1409d1c2d08f0e9f03cef067506dd 
  src/master/allocator/mesos/hierarchical.cpp 
5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 

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


Testing
---

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


Thanks,

Benjamin Bannier

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

[Jiang Yan Xu]

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




src/tests/cluster.cpp (lines 434 - 436)


This can be tackled in another review because a) the tests here don't use 
cluster.cpp and b) I see other issuse here as well (see below).



src/tests/cluster.cpp (line 455)


Here AWAIT could result in a failure that terminates the lambda early and 
leave behind orphan containers.



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.



src/tests/containerizer/xfs_quota_tests.cpp (line 75)


No space: `{limit, used};`



src/tests/containerizer/xfs_quota_tests.cpp (line 215)


This test can be extended to include files:

```
directory
  |
  |- file
```

You can call `set|clearProjectId()` on the directory and verify 
`getProjectId()` on the file inside (and the directory).



src/tests/containerizer/xfs_quota_tests.cpp (line 217)


`s/p/prid/` or `s/p/project/` here and elsewhere. `project` is probably 
better because you use `projectA` and `projectB` below in another test.

When there are more variables in scope, single letter variables can be 
confusing: e.g., `p` vs. `path` which also starts with `p`.



src/tests/containerizer/xfs_quota_tests.cpp (line 218)


In your `setUp()` you have `chdir`d so it should save us from needing to 
spell out the full path here.

Fix other occurrences as well.



src/tests/containerizer/xfs_quota_tests.cpp (lines 222 - 223)


This can be done in one line:

```
EXPECT_NONE(getProjectId(path));
```

Please fix other occurences at well.



src/tests/containerizer/xfs_quota_tests.cpp (line 239)


It's now hard to do this test with the public `setProjectId()` no longer 
applicable to individual files but I think it's OK because you can construct 
the file hiearachies like above to achieve all achievable cases we'll run into.

In principle testing against public APIs is sufficient and if we have 
enumerated all possiblilties of public API usage and still cannot cover all 
100% paths in the private methods, then these paths are probably invalid and we 
should actually remove them. :)



src/tests/containerizer/xfs_quota_tests.cpp (lines 263 - 264)


I guess this is saying that when O_NOFOLLOW is used along, `openPath()` 
would fail; when O_NOFOLLOW + O_PATH is used, `openPath()` would succeed but 
the resulting fd is very limited.

I think these comments are better suitable to be put above the definition 
of `openPath()` than here because the test here doesn't have access to the 
`open` flags and the reader of the test aren't necessarily aware of the 
internal details.



src/tests/containerizer/xfs_quota_tests.cpp (line 265)


s/used/use/



src/tests/containerizer/xfs_quota_tests.cpp (lines 273 - 280)


So for this we can also create

```
directory
 |
 |- link
```

Then:
```
getProjectId(link);
setProjectId(directory);
getProjectId(link);
clearProjectId(directory);
getProjectId(link);
```

Even if the reason for `EXPECT_ERROR(getProjectId(path));` to pass is due 
to FTS_PHYSICAL (so we didn't attempt to set it on the link instead of we 
cannot set it on the link), that's the situation we are ever going to run into 
in real scnearios anyways so it's OK.



src/tests/containerizer/xfs_quota_tests.cpp (line 284)


This is the most comprehensive test in this file and it has covered all 
methods and not just for `AssignProjectId`: call it `DirectoryTree`?



src/tests/containerizer/xfs_quota_tests.cpp (lines 292 - 294)


Instead of doing cleanups here, we can register the projectIds with a 
member variable:

```
vector projectIds;
```

This way in the `tearDown()` method we can go through all the them and call 
`clearProjectQuota()` on each.

Re: Review Request 45202: Add test for rescinding offer trriggered by updating weights.

[Adam B]

> On March 30, 2016, 4:58 a.m., Adam B wrote:
> > src/tests/master_allocator_tests.cpp, line 1519
> > 
> >
> > Don't you at least know that one of these parameters is the master's 
> > pid?
> 
> Yongqiao Wang wrote:
> Do you mean the parameters of addSlave function in allocator interface? 
> if yes, then master's pid is not one of them. Cloud you help to make this 
> comment clearly? Thanks.

Nevermind. Dropping. I just meant that all those `_`s you're filling in as 
parameters act as wildcards, and if there's something we definitely know (e.g. 
master pid), it'd be better to explicitly match against that instead of the 
wildcard. But I realize now that this is not an EXPECT_PROTOBUF or 
EXPECT_MESSAGE where one of the parameters is the 'from' pid, but rather this 
is an EXPECT_CALL, and everybody seems to pass it all `_`s.


- Adam


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


On March 31, 2016, 12:15 a.m., Yongqiao Wang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45202/
> ---
> 
> (Updated March 31, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Bugs: MESOS-4881
> https://issues.apache.org/jira/browse/MESOS-4881
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add test for rescinding offer trriggered by updating weights.
> 
> 
> Diffs
> -
> 
>   src/tests/dynamic_weights_tests.cpp 
> 6357cf42ec59c1388e95d6d808978918d5cd4a78 
>   src/tests/master_allocator_tests.cpp 
> b41ba2bda4d680f6fc42f525719973d56c11fe31 
>   src/tests/mesos.hpp a35371d7c1fcb9dbbe8a18c80522deb6bf789ad8 
> 
> Diff: https://reviews.apache.org/r/45202/diff/
> 
> 
> Testing
> ---
> 
> make && make check.
> 
> $ ./src/mesos-tests 
> --gtest_filter=MasterAllocatorTest/0.RebalancedForUpdatedWeights
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from MasterAllocatorTest/0, where TypeParam = 
> mesos::internal::master::allocator::MesosAllocator  mesos::internal::master::allocator::DRFSorter> >
> [ RUN  ] MasterAllocatorTest/0.RebalancedForUpdatedWeights
> [   OK ] MasterAllocatorTest/0.RebalancedForUpdatedWeights (1059 ms)
> [--] 1 test from MasterAllocatorTest/0 (1059 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (1070 ms total)
> [  PASSED  ] 1 test.
> 
> 
> Thanks,
> 
> Yongqiao Wang
> 
>

Re: Review Request 45546: Implemented status() method of "network/cni" isolator.

[Qian Zhang]

> On April 1, 2016, 2:31 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 786
> > 
> >
> > Shouldn't we return a failure?

I think we should not return a failure because there can be some containers do 
not want to opt in CNI networks.


> On April 1, 2016, 2:31 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 790
> > 
> >
> > Maybe s/netInfo/_networkInfo

Agree, and let me fix it in `prepare` method as well.


- Qian


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


On March 31, 2016, 10:55 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45546/
> ---
> 
> (Updated March 31, 2016, 10:55 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan, Gilbert Song, and Jie Yu.
> 
> 
> Bugs: MESOS-4759
> https://issues.apache.org/jira/browse/MESOS-4759
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented status() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 1c8e231813c0579b79681c5d18b1f799a727ead7 
> 
> Diff: https://reviews.apache.org/r/45546/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45500: Fix Mesos-1104, linux/fs.hpp remove mesos::internal:: should be enough.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45500]

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 1, 2016, 5:19 a.m., Deshi Xiao wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45500/
> ---
> 
> (Updated April 1, 2016, 5:19 a.m.)
> 
> 
> Review request for mesos, haosdent huang and Cong Wang.
> 
> 
> Bugs: MESOS-1104
> https://issues.apache.org/jira/browse/MESOS-1104
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> follow haosdent's suggest, remove mesos::internal:: should be enough.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.cpp b7420c682970c4838e84973198ac4fe7af5f68f9 
>   src/linux/fs.hpp 4525a5d5566e2bc913894e993ac7350f1bbd9cc0 
>   src/linux/fs.cpp dbf94759b6382bfafd3f3b8b4c2047af36a53ad5 
>   src/slave/containerizer/mesos/mount.cpp 
> bf17b015e45579882fd31248e8609eec6d58a9da 
>   src/tests/containerizer/fs_tests.cpp 
> 020fd8a4bf5911671e038a96f8b50c0f58e22ed5 
>   src/tests/containerizer/port_mapping_tests.cpp 
> e062daa9fcfc776144b48325daa1f1284c5e59a4 
> 
> Diff: https://reviews.apache.org/r/45500/diff/
> 
> 
> Testing
> ---
> 
> make test on localhost(ubuntu)
> 
> 
> Thanks,
> 
> Deshi Xiao
> 
>

Re: Review Request 45213: Change 'slave' to 'agent' in all the logs and standard output.

[zhou xing]

> On 三月 31, 2016, 8:25 p.m., Vinod Kone wrote:
> > Can you call this out specifically in the CHANGELOG (under "Binary API 
> > Changes) section.
> 
> zhou xing wrote:
> Vinod?thanks for the review. I just resubmit the patch to rebase to the 
> latest code, please take a look.
> Besides, do I need to submit another patch for the CHANGELOG chagnes or I 
> can inlude the CHANGELOG changes in this patch?
> 
> Vinod Kone wrote:
> You can include them here.

Done, please take a look


- zhou


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


On 四月 1, 2016, 6:23 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45213/
> ---
> 
> (Updated 四月 1, 2016, 6:23 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: mesos-5055
> https://issues.apache.org/jira/browse/mesos-5055
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the first patch to solve [MESOS-3780]
> 
> 
> Diffs
> -
> 
>   CHANGELOG b90078d41357c29c9102df00a735bde460e797bb 
>   src/docker/executor.cpp 26cc88d35fc7cd913c5105f5fc4e6ae71ce59279 
>   src/examples/balloon_framework.cpp 1665e5bd35ad9e029372e55209802c2827be658f 
>   src/examples/no_executor_framework.cpp 
> 9223d954197ebd097f57efea5299c917b24fe41e 
>   src/examples/persistent_volume_framework.cpp 
> 9ce43d2b7da3f03d24473ff22cb62a88004f5f53 
>   src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
>   src/exec/exec.cpp 8f672602daf090dec032d2b684e407e5d043af9c 
>   src/local/local.cpp e777ea2938a23db8b407676a0f7e635e63d032fa 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
>   src/master/main.cpp 58561cffa440aaf1293e9ffe19b5685e6d2f1952 
>   src/master/master.hpp 1751ee1fc4d3e15df018be8bfd4f8ef2a0e32abd 
>   src/master/master.cpp 655b0b8d0156b44914578a1241beb1fb7c92ca23 
>   src/master/validation.cpp 9c9e42283baa6e49d86af2ce7222131ce53ccaff 
>   src/sched/sched.cpp d989cb5f35d314dc2e03617bfdbd4b18d4efcf91 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
>   src/slave/containerizer/mesos/launch.cpp 
> dc7bebf8d0f3e36c6d41af4e0e8ffc438b6cd86c 
>   src/slave/main.cpp f5f21422c7cb94506b78cebf7f7341b5a078f3b2 
>   src/slave/paths.cpp e0451a8856063f78c8f2aeed11c01f3560b4ed1c 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
>   src/slave/state.cpp 75bdfe144d3307dc74bf48e6a0d403fbea568a6d 
>   src/slave/status_update_manager.cpp 
> f6a9b3b6616abee9c55bf731855ee9b3b98606ac 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/hierarchical_allocator_tests.cpp 
> 8f78a204d296f94f515f21511710a35c33e27255 
>   src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
>   src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 
>   src/watcher/whitelist_watcher.cpp 3d0d5828ef095884ca87156d69f963f04dba40a1 
> 
> Diff: https://reviews.apache.org/r/45213/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 45213: Change 'slave' to 'agent' in all the logs and standard output.

[zhou xing]

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

(Updated 四月 1, 2016, 6:23 a.m.)


Review request for mesos and Vinod Kone.


Changes
---

Add CHANGELOG


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


Repository: mesos


Description
---

This is the first patch to solve [MESOS-3780]


Diffs (updated)
-

  CHANGELOG b90078d41357c29c9102df00a735bde460e797bb 
  src/docker/executor.cpp 26cc88d35fc7cd913c5105f5fc4e6ae71ce59279 
  src/examples/balloon_framework.cpp 1665e5bd35ad9e029372e55209802c2827be658f 
  src/examples/no_executor_framework.cpp 
9223d954197ebd097f57efea5299c917b24fe41e 
  src/examples/persistent_volume_framework.cpp 
9ce43d2b7da3f03d24473ff22cb62a88004f5f53 
  src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
  src/exec/exec.cpp 8f672602daf090dec032d2b684e407e5d043af9c 
  src/local/local.cpp e777ea2938a23db8b407676a0f7e635e63d032fa 
  src/master/allocator/mesos/hierarchical.cpp 
5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
  src/master/main.cpp 58561cffa440aaf1293e9ffe19b5685e6d2f1952 
  src/master/master.hpp 1751ee1fc4d3e15df018be8bfd4f8ef2a0e32abd 
  src/master/master.cpp 655b0b8d0156b44914578a1241beb1fb7c92ca23 
  src/master/validation.cpp 9c9e42283baa6e49d86af2ce7222131ce53ccaff 
  src/sched/sched.cpp d989cb5f35d314dc2e03617bfdbd4b18d4efcf91 
  src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
323c84a3d960a196d8ba87f753814e9d43a07957 
  src/slave/containerizer/mesos/launch.cpp 
dc7bebf8d0f3e36c6d41af4e0e8ffc438b6cd86c 
  src/slave/main.cpp f5f21422c7cb94506b78cebf7f7341b5a078f3b2 
  src/slave/paths.cpp e0451a8856063f78c8f2aeed11c01f3560b4ed1c 
  src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
  src/slave/state.cpp 75bdfe144d3307dc74bf48e6a0d403fbea568a6d 
  src/slave/status_update_manager.cpp f6a9b3b6616abee9c55bf731855ee9b3b98606ac 
  src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
  src/tests/hierarchical_allocator_tests.cpp 
8f78a204d296f94f515f21511710a35c33e27255 
  src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
  src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 
  src/watcher/whitelist_watcher.cpp 3d0d5828ef095884ca87156d69f963f04dba40a1 

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


Testing
---

make
make check


Thanks,

zhou xing

Re: Review Request 45213: Change 'slave' to 'agent' in all the logs and standard output.

[Vinod Kone]

> On March 31, 2016, 8:25 p.m., Vinod Kone wrote:
> > Can you call this out specifically in the CHANGELOG (under "Binary API 
> > Changes) section.
> 
> zhou xing wrote:
> Vinod?thanks for the review. I just resubmit the patch to rebase to the 
> latest code, please take a look.
> Besides, do I need to submit another patch for the CHANGELOG chagnes or I 
> can inlude the CHANGELOG changes in this patch?

You can include them here.


- Vinod


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


On April 1, 2016, 5:57 a.m., zhou xing wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45213/
> ---
> 
> (Updated April 1, 2016, 5:57 a.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Bugs: mesos-3780
> https://issues.apache.org/jira/browse/mesos-3780
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is the first patch to solve [MESOS-3780]
> 
> 
> Diffs
> -
> 
>   src/docker/executor.cpp 26cc88d35fc7cd913c5105f5fc4e6ae71ce59279 
>   src/examples/balloon_framework.cpp 1665e5bd35ad9e029372e55209802c2827be658f 
>   src/examples/no_executor_framework.cpp 
> 9223d954197ebd097f57efea5299c917b24fe41e 
>   src/examples/persistent_volume_framework.cpp 
> 9ce43d2b7da3f03d24473ff22cb62a88004f5f53 
>   src/examples/test_hook_module.cpp abd132b3f39265683542a9d1533d2a31bd81769a 
>   src/exec/exec.cpp 8f672602daf090dec032d2b684e407e5d043af9c 
>   src/local/local.cpp e777ea2938a23db8b407676a0f7e635e63d032fa 
>   src/master/allocator/mesos/hierarchical.cpp 
> 5a5df5f8287a1f85b8b2a6aac7e6e13d0650a132 
>   src/master/main.cpp 58561cffa440aaf1293e9ffe19b5685e6d2f1952 
>   src/master/master.hpp 1751ee1fc4d3e15df018be8bfd4f8ef2a0e32abd 
>   src/master/master.cpp 655b0b8d0156b44914578a1241beb1fb7c92ca23 
>   src/master/validation.cpp 9c9e42283baa6e49d86af2ce7222131ce53ccaff 
>   src/sched/sched.cpp d989cb5f35d314dc2e03617bfdbd4b18d4efcf91 
>   src/slave/containerizer/mesos/isolators/network/port_mapping.cpp 
> 323c84a3d960a196d8ba87f753814e9d43a07957 
>   src/slave/containerizer/mesos/launch.cpp 
> dc7bebf8d0f3e36c6d41af4e0e8ffc438b6cd86c 
>   src/slave/main.cpp f5f21422c7cb94506b78cebf7f7341b5a078f3b2 
>   src/slave/paths.cpp e0451a8856063f78c8f2aeed11c01f3560b4ed1c 
>   src/slave/slave.cpp fc77f594d16a9fb2ca001e089d74e2c0ffeb5baa 
>   src/slave/state.cpp 75bdfe144d3307dc74bf48e6a0d403fbea568a6d 
>   src/slave/status_update_manager.cpp 
> f6a9b3b6616abee9c55bf731855ee9b3b98606ac 
>   src/tests/cluster.cpp 2da0bd7612d571277e76d0a95ad8e776434af323 
>   src/tests/hierarchical_allocator_tests.cpp 
> 8f78a204d296f94f515f21511710a35c33e27255 
>   src/tests/mesos.cpp 77d49cc65e08f040b0d2010cd083928e4ff8b7cd 
>   src/tests/registrar_tests.cpp 39caf9bb950c0b229a66becb039c7a830a18f6bc 
>   src/watcher/whitelist_watcher.cpp 3d0d5828ef095884ca87156d69f963f04dba40a1 
> 
> Diff: https://reviews.apache.org/r/45213/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> zhou xing
> 
>

Re: Review Request 45575: Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45083, 45084, 45350, 45085, 45086, 45087, 45351, 45352, 
45353, 45354, 45362, 45363, 45364, 45472, 45473, 43283, 45573, 45574, 45575]

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 1, 2016, 2:10 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45575/
> ---
> 
> (Updated April 1, 2016, 2:10 a.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-5047
> https://issues.apache.org/jira/browse/MESOS-5047
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Migrate test cases for `perf_event` subsystem to cgroups_isolator_tests.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cgroups_isolator_tests.cpp 
> c4e467c8227f9e4129b05d173812592f39a04e06 
> 
> Diff: https://reviews.apache.org/r/45575/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>