Review Request 45093: Added mesos containerizer test DestroyWhileProvisioning.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Added mesos containerizer test DestroyWhileProvisioning.


Diffs
-

  src/tests/containerizer/mesos_containerizer_tests.cpp 
f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 

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


Testing
---

make check


Thanks,

Gilbert Song

Review Request 45092: Fixed containerizer potential race destroy while provisioning.

[Gilbert Song]

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

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


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


Repository: mesos


Description
---

Fixed containerizer potential race destroy while provisioning.


Diffs
-

  src/slave/containerizer/mesos/containerizer.hpp 
3ef6a6752a6656e97be9f48bd4d2d060d1f9cb46 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 

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


Testing
---

make check


Thanks,

Gilbert Song

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45046]

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 March 21, 2016, 3:14 a.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 21, 2016, 3:14 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 761238c48332bcce0bff6c411225fdb4176ddca6 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.hpp 
> bbdce88da6e41dbb88681bc9d604b00923033b3d 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

> On March 19, 2016, 10:03 a.m., Joerg Schad wrote:
> > src/slave/slave.cpp, line 373
> > 
> >
> > Do we actually have to get the authenticator above if this flag is not 
> > set?
> 
> Greg Mann wrote:
> The code directly above this line performs error checking on the value of 
> the `--http_authenticators` flag, which is necessary before this `if` block 
> uses that input. However, we also have the code that reads the 
> `--http_credentials` flag, which only needs to be executed if the user is 
> using the basic HTTP authenticator, so I've moved that code into the 
> appropriate scope.

However, moving this code means that we won't perform error checking on the 
value of the `--http_credentials` flag when `--authenticate_http` is not set. 
Arguably, we should always fail hard and tell a user if they've given us 
invalid input for a flag. I'm going to move this code back to its original 
location for now; let me know what you think.

We should also probably fail hard if a user attempts to set 
`--http_credentials` when `--authenticate_http` is not set. I've added an `else 
if` to handle this case just after the `if` block associated with this comment.


- Greg


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


On March 21, 2016, 4:50 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44515/
> ---
> 
> (Updated March 21, 2016, 4:50 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flags for HTTP authentication.
> 
> Three command-line flags have been added to the agent to enable HTTP 
> authentication: `--authenticate_http`, `--http_credentials`, and 
> `--http_authenticators`.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
> 
> Diff: https://reviews.apache.org/r/44515/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

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

(Updated March 21, 2016, 4:50 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


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


Repository: mesos


Description
---

Added agent flags for HTTP authentication.

Three command-line flags have been added to the agent to enable HTTP 
authentication: `--authenticate_http`, `--http_credentials`, and 
`--http_authenticators`.


Diffs (updated)
-

  src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 

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


Testing
---

`sudo make check` was used to test on both OSX and Ubuntu 14.04.


Thanks,

Greg Mann

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

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

(Updated March 21, 2016, 4:24 a.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added agent flags for HTTP authentication.

Three command-line flags have been added to the agent to enable HTTP 
authentication: `--authenticate_http`, `--http_credentials`, and 
`--http_authenticators`.


Diffs (updated)
-

  src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 

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


Testing
---

`sudo make check` was used to test on both OSX and Ubuntu 14.04.


Thanks,

Greg Mann

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

> On March 19, 2016, 10:03 a.m., Joerg Schad wrote:
> > src/slave/slave.cpp, line 373
> > 
> >
> > Do we actually have to get the authenticator above if this flag is not 
> > set?

The code directly above this line performs error checking on the value of the 
`--http_authenticators` flag, which is necessary before this `if` block uses 
that input. However, we also have the code that reads the `--http_credentials` 
flag, which only needs to be executed if the user is using the basic HTTP 
authenticator, so I've moved that code into the appropriate scope.


- Greg


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


On March 21, 2016, 4:24 a.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44515/
> ---
> 
> (Updated March 21, 2016, 4:24 a.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flags for HTTP authentication.
> 
> Three command-line flags have been added to the agent to enable HTTP 
> authentication: `--authenticate_http`, `--http_credentials`, and 
> `--http_authenticators`.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
> 
> Diff: https://reviews.apache.org/r/44515/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 20, 2016, 7:06 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```
> 
> Qian Zhang wrote:
> Are we going to support the use case that one container joins one CNI 
> network multiple times? If yes, then I think we may need to use `vector` 
> rather than `hashmap` in `struct Info`, like this:
> 
> struct NetworkResultInfo
> {
>   // CNI network name.
>   std::string name;
> 
>   // Interface name.
>   const std::string ifname;
> 
>   // Protobuf of CNI network result returned by CNI plugin
>   Option result;
> };
> 
> struct Info
> {
>   std::vector networkInfos;
> };
> 
> Jie Yu wrote:
> > Are we going to support the use case that one container joins one CNI 
> network multiple times?
> 
> Let's not consider this for now. I don't see a use case at this moment.
> 
> Also, I think the name 'NetworkResult' is weird (what result? what's 
> network result?!). Can we rename it to NetworkInfo?
> 
> ```
> struct NetworkInfo
> {
>   string name;
>   string ifname;
>   Option network;
> }
> 
> struct Info
> {
>   hashmap networkInfos;
> }
> ```
> 
> Qian Zhang wrote:
> Do we need to support NIC bonding (e.g., both eth0 and eth1 of a 
> container are in one CNI network)? If we do not need to support it, then I 
> agree we should use hashmap rather than vector, and I will add a check in 
> `prepare()` to make sure one container only joins a CNI network once, 
> otherwise return Failure.

Let's not consider NIC bonding for now. We can easily s/hashmap/multihashmap/ 
if we want to support it in the future.


- Jie


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


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6: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
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 43712: Documented how the replicated log works.

[haosdent huang]

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




docs/replicated-log-internals.md (line 105)


> Otherwise, we should find at least one VOTING replica in a quorum of 
replicas such that its end position is larger than end.

Does it should be "Otherwise, we should find at least one VOTING replica in 
a quorum of replicas such that its end position is larger than _e_."


- haosdent huang


On Feb. 18, 2016, 6:19 a.m., Neil Conway wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43712/
> ---
> 
> (Updated Feb. 18, 2016, 6:19 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-1471
> https://issues.apache.org/jira/browse/MESOS-1471
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This is closely based on an (unpublished) blog post by Jie Yu.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 3d8236822af688a88a9f9f357c67c03d7d60fdd9 
>   docs/high-availability-framework-guide.md 
> f21f95f24c0e9f3c4376b64e6a5776aafec39172 
>   docs/home.md 982ad28d392570b40b83e9e85d21583b88ff755e 
>   docs/images/log-architecture.png PRE-CREATION 
>   docs/images/log-cluster.png PRE-CREATION 
>   docs/maintenance.md e6bfe0f655581a6a72de4579bd7e5753625c0e51 
>   docs/operational-guide.md 4680ee3397d081acd6f82499703de4456e7ca4f4 
>   docs/replicated-log-internals.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/43712/diff/
> 
> 
> Testing
> ---
> 
> Previewed on github gist.
> 
> 
> Thanks,
> 
> Neil Conway
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

> On March 20, 2016, 7:06 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```
> 
> Qian Zhang wrote:
> Are we going to support the use case that one container joins one CNI 
> network multiple times? If yes, then I think we may need to use `vector` 
> rather than `hashmap` in `struct Info`, like this:
> 
> struct NetworkResultInfo
> {
>   // CNI network name.
>   std::string name;
> 
>   // Interface name.
>   const std::string ifname;
> 
>   // Protobuf of CNI network result returned by CNI plugin
>   Option result;
> };
> 
> struct Info
> {
>   std::vector networkInfos;
> };

> Are we going to support the use case that one container joins one CNI network 
> multiple times?

Let's not consider this for now. I don't see a use case at this moment.

Also, I think the name 'NetworkResult' is weird (what result? what's network 
result?!). Can we rename it to NetworkInfo?

```
struct NetworkInfo
{
  string name;
  string ifname;
  Option network;
}

struct Info
{
  hashmap networkInfos;
}
```


- Jie


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


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6: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
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44622: Introduced a protobuf message "NetworkResult".

[Jie Yu]

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




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


Can we put this under cni::spec namespace instead?


- Jie Yu


On March 17, 2016, 10:06 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44622/
> ---
> 
> (Updated March 17, 2016, 10:06 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
> ---
> 
> Introduced a protobuf message "NetworkResult".
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44622/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[James Peach]

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

(Updated March 21, 2016, 2 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

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


Diffs (updated)
-

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

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


Testing
---

Make check. Manual testing.


Thanks,

James Peach

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

[James Peach]

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

(Updated March 21, 2016, 2 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add XFS disk isolator documentation.


Diffs (updated)
-

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

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


Testing
---

Make check. Source inspection.


Thanks,

James Peach

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

[James Peach]

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

(Updated March 21, 2016, 2 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

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

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

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

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

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

  $ xfs_io -r -c stat PATH


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 
  src/slave/containerizer/mesos/isolators/disk/xfs.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/disk/xfs.cpp PRE-CREATION 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 

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


Testing
---

Make check. Manual testing. Tests in subsequent patches.


Thanks,

James Peach

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

[James Peach]

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

(Updated March 21, 2016, 2 a.m.)


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


Changes
---

Rebased.


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


Repository: mesos


Description
---

Add tests for XFS project quota utilities.


Diffs (updated)
-

  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/tests/containerizer/xfs_quota_tests.cpp PRE-CREATION 
  src/tests/environment.cpp ee1bbe6b4e3dda1e27b63d71a08ef0d1d254741a 

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


Testing
---

Make check. Manual testing. These tests.


Thanks,

James Peach

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

[James Peach]

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

(Updated March 21, 2016, 1:59 a.m.)


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


Changes
---

Rebased and addressed review comments.


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


Repository: mesos


Description
---

Add autoconf tests for XFS project quotas.


Diffs (updated)
-

  configure.ac 9ec4bc1cff3b0b46dd2e7ece2c1f2d19ffb8 

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


Testing
---

Make check. Manual verification.


Thanks,

James Peach

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Qian Zhang]

> On March 21, 2016, 3:06 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.hpp, line 82
> > 
> >
> > s/NetworkResultInfo/Info/
> > 
> > We typically use 'Info' to store information about each container in 
> > isolators.
> > 
> > We can then have a hashmap from network name to a another data 
> > structure in Info to represent the networks the container has joined.
> > 
> > ```
> > // Information about a network that a container has joined.
> > struct NetworkInfo
> > {
> >   string ifname;
> >   cni::NetworkResult result;
> > };
> > 
> > struct Info
> > {
> >   hashmap networkInfos;
> > };
> > ```

Are we going to support the use case that one container joins one CNI network 
multiple times? If yes, then I think we may need to use `vector` rather than 
`hashmap` in `struct Info`, like this:

struct NetworkResultInfo
{
  // CNI network name.
  std::string name;

  // Interface name.
  const std::string ifname;

  // Protobuf of CNI network result returned by CNI plugin
  Option result;
};

struct Info
{
  std::vector networkInfos;
};


- Qian


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


On March 18, 2016, 2:31 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 2:31 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 prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Vinod Kone]

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




src/tests/fetcher_tests.cpp (line 646)


s/fix/Fix/

Also, instead of here this TODO should be located at tests that do 
os::mktemp() (e.g., #431).



src/tests/fetcher_tests.cpp (line 648)


why is this directory named "cutom_gzip_filename" ?

also you are calling mkdtemp() but not using 'XXX' pattern? if you want to 
a well known directory name you can as well just use os::mkdir().



src/tests/fetcher_tests.cpp (line 651)


ditto. this should be os::touch() if you want a well known filename.



src/tests/fetcher_tests.cpp (line 687)


ditto. see above.



src/tests/fetcher_tests.cpp (lines 688 - 692)


ditto. see above.



CHANGELOG (line 1)


s/0.29.0/0.29.0 (WIP)/



CHANGELOG (line 7)


2 blank lines.


- Vinod Kone


On March 21, 2016, 12:51 a.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 21, 2016, 12:51 a.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   CHANGELOG 761238c48332bcce0bff6c411225fdb4176ddca6 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.hpp 
> bbdce88da6e41dbb88681bc9d604b00923033b3d 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Michael Browning]

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

(Updated March 21, 2016, 12:51 a.m.)


Review request for mesos, Vinod Kone and Zhitao Li.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Created URI.filename to name fetched files in sandbox where appropriate.


Diffs (updated)
-

  CHANGELOG 761238c48332bcce0bff6c411225fdb4176ddca6 
  docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
  include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
  include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/slave/containerizer/fetcher.hpp bbdce88da6e41dbb88681bc9d604b00923033b3d 
  src/slave/containerizer/fetcher.cpp 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
  src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

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


Testing
---

There are two paths by which a file gets fetched to the executor sandbox: the 
without-cache path, where the fetcher downloads the file directly from the 
specified URI, and the with-cache path, where it copies it from the cache. In 
both cases, we verify that the file is saved to the sandbox directory with the 
name specified by the "filename" field in the CommandInfo.URI proto.


Thanks,

Michael Browning

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Michael Browning]

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

(Updated March 21, 2016, 12:49 a.m.)


Review request for mesos, Vinod Kone and Zhitao Li.


Changes
---

Address comments and add API changes to CHANGELOG.


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


Repository: mesos


Description
---

Created URI.filename to name fetched files in sandbox where appropriate.


Diffs (updated)
-

  3rdparty/libprocess/include/process/help.hpp 
3f859803f3096d3161fffb6485ce1ce3cb6b04bc 
  3rdparty/libprocess/src/help.cpp e046933975883fb046af8e51c31a742d983a6321 
  3rdparty/libprocess/src/logging.cpp bdbf716ee46a459f1004f0f4b72c23aae135f347 
  CHANGELOG 761238c48332bcce0bff6c411225fdb4176ddca6 
  docs/endpoints/master/api/v1/scheduler.md 
538979977e07b484956be85b08b0dbc6cffcef01 
  docs/endpoints/master/create-volumes.md 
52ae3a8159bb7d26b63f5889ce3f122371afbdc4 
  docs/endpoints/master/destroy-volumes.md 
a0bb1e8d1ce42ab2b96518cd4d325bfc541ad4ff 
  docs/endpoints/master/flags.md e9038fece0d17432f839c1719beac65d1a351550 
  docs/endpoints/master/frameworks.md 15ecabfdd87faee6a1f24895088a3c08edf62b96 
  docs/endpoints/master/health.md dc1c1cd6f78874b9264279dd78c57aa572d048a4 
  docs/endpoints/master/machine/down.md 
f7e81417aa663e287a61124b351f2461b1c106e6 
  docs/endpoints/master/machine/up.md d96be726f5cf524937e3ebc5b046bf5f47886a1c 
  docs/endpoints/master/maintenance/schedule.md 
577e43c5faa961acb75f66e0db6729d7db9d88bf 
  docs/endpoints/master/maintenance/status.md 
249dae2706ccf30d79e0cd2f19821acd6ada74b7 
  docs/endpoints/master/observe.md 9ebd527788bbd2fd2dd833242c24ac561f826c30 
  docs/endpoints/master/quota.md b1e3462bd35c5acfd65323db5d0660277b110b11 
  docs/endpoints/master/redirect.md 833d9935b8fb038977fe08d826d73f03f2059bab 
  docs/endpoints/master/reserve.md 1d481b56d380d45218001513330b225ca4a0a55c 
  docs/endpoints/master/roles.json.md 01e265330c4c30de11301a2c4abbddfee10ac35d 
  docs/endpoints/master/roles.md 9c01d370a86d77eb95fcc157de70304e73f5f1e9 
  docs/endpoints/master/slaves.md 9e9fff7329d6bf05ae78997b1292657ff4dd10c3 
  docs/endpoints/master/state-summary.md 
a0da400b34c202602cb661a19c6199291da4ff4b 
  docs/endpoints/master/state.json.md 7998b80f29042f4b67565b03e2464b6e3a009335 
  docs/endpoints/master/state.md 59518d624a0c5e010f856c900a9d6512a35b21af 
  docs/endpoints/master/tasks.json.md 29bb9738b1519174eaeac4f4db423000fe48d0d3 
  docs/endpoints/master/tasks.md ab9bb09631400668adddeee363dd612c71e98e16 
  docs/endpoints/master/teardown.md 9b62b2656162b4160a9845152aecf7b75231fbae 
  docs/endpoints/master/unreserve.md b9282df659ebb6090ef49ef8fc0f01411cd53103 
  docs/endpoints/master/weights.md 7c4517343aa61bf6156fa54547690930051616a3 
  docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
  include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
  include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
  src/exec/exec.cpp 8f672602daf090dec032d2b684e407e5d043af9c 
  src/executor/executor.cpp 48b00504cac83aa0f4b1fc0b81480d3010edebae 
  src/launcher/executor.cpp 63eb8f1ae5510acdb5b504807a37c4e2e68ad6bd 
  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
  src/slave/containerizer/fetcher.hpp bbdce88da6e41dbb88681bc9d604b00923033b3d 
  src/slave/containerizer/fetcher.cpp 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 
  src/slave/http.cpp 0117827d73bd1b4c96ef261d4573abf70f52c6b3 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 
  src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

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


Testing
---

There are two paths by which a file gets fetched to the executor sandbox: the 
without-cache path, where the fetcher downloads the file directly from the 
specified URI, and the with-cache path, where it copies it from the cache. In 
both cases, we verify that the file is saved to the sandbox directory with the 
name specified by the "filename" field in the CommandInfo.URI proto.


Thanks,

Michael Browning

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Guangya Liu]

> On 三月 20, 2016, 6:36 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 645-647
> > 
> >
> > I know you followed the existing pattern in this file but we shouldn't 
> > do mktemp() like this in the tests because if the test fails for whatever 
> > reason the temp file and the directory are leaked. 
> > 
> > The FetcherTest fixture inherits from TemporaryDirectoryTest so that 
> > the test runs in a temporary sandbox.
> > 
> > So you should do:
> > 
> > ```
> > Try dir = os::mkdtemp("./"));
> > ASSERT_SOME(dir);
> > 
> > Try path = os::mktemp(path::join(dir.get(), ""));
> > ASSERT_SOME(path);
> > ```
> > 
> > Feel free to fix the other tests in this file in a subsequent review or 
> > just add a TODO for now.
> 
> Michael Browning wrote:
> Happy to fix the other tests in this file. I think it'd be simpler to do 
> it in a separate review -- should I also create an issue for that?
> 
> Vinod Kone wrote:
> no need to create an issue.

What about using `path::join(os::getcwd(), "");` to simplify the test? I 
saw that the `ROOT_LocalPullerSimpleCommand` is using such format. 
https://github.com/apache/mesos/blob/master/src/tests/containerizer/provisioner_docker_tests.cpp#L340


- Guangya


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


On 三月 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated 三月 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

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

[James Peach]

> On March 18, 2016, 6:36 p.m., Jiang Yan Xu wrote:
> > configure.ac, line 923
> > 
> >
> > Sorry this should have been a continued discussion on 
> > https://reviews.apache.org/r/44342/#comment184051 but anyways:
> > 
> > Your response to my initial comments:
> > > There's no AC_ARG_WITH. If the dependencies area available we will 
> > build the isolator and the operator can configure it at runtime. I don't 
> > think there's any need to disable this at build time since it is not 
> > enabled by default anyway.
> > 
> > I am still favoring building XFS isolator based on 
> > `--with-xfs-isolator` (AC_ARG_ENABLE is probably better than AC_ARG_WITH) 
> > for the following reasons:
> > 
> > 
> > 1. Explicitness: In general, if we build things solely based on the 
> > availability of headers, users can inadvertently build more things into the 
> > deployed binary which could result in larger binaries and perhaps other 
> > side effects (through bugs, etc.).
> > 2. Preventing user mistake: I am thinking about how we would advise 
> > users on enabling this feature:
> >   1. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages whose name could be different for different 
> > distros and if you haven't made mistakes in finding them, the feature will 
> > be built." vs. 
> >   2. "The XFS isolator feature is disabled by default, to enable it in 
> > build, install these packages (which could be named differently depending 
> > on your distro), if the dependencies are not satisfied, the build aborts 
> > with an message stating such error." The latter feels better to me.
> > 3. Consistency: Other optional features of Mesos follow this pattern.
> 
> Jiang Yan Xu wrote:
> Sorry I meant `--enable-xfs-isolator`.
> 
> Jie Yu wrote:
> For consistency: I think the network isolator is using 
> `--with-network-isolator` (`AC_ARG_WITH([network-isolator],`), so it'e better 
> to be consistent and use 'AC_ARG_WITH' here.
> 
> I agree with Yan that we should be more explicit, instead of relying on 
> headers.

My view is that unless there is a tradeof (performance, security, etc), then 
every optional feature should be available at runtime. The alternative imposes 
an undue burden on operators because they will have to repackage if they want 
to enable a feature.

However, I'm not going to argue this any furthere here. I'll just guard this 
with ``AC_ARG_WITH`` (@xujyan is right this should have been ``AC_ARG_ENABLE``).


- James


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


On March 18, 2016, 3:45 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44945/
> ---
> 
> (Updated March 18, 2016, 3:45 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-4828
> https://issues.apache.org/jira/browse/MESOS-4828
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add autoconf tests for XFS project quotas.
> 
> 
> Diffs
> -
> 
>   configure.ac 8e4f03593df4a8ba13f00292963e351acc3f71c1 
> 
> Diff: https://reviews.apache.org/r/44945/diff/
> 
> 
> Testing
> ---
> 
> Make check. Manual verification.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Vinod Kone]

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



Can you confirm that you did a manual test of this (using curl and multiple 
masters w/ ZK)?


src/master/http.cpp (lines 970 - 972)


looks like this fits in one line, why the wrapping?

```
  return TemporaryRedirect(
  "//" + hostname.get() + ":" + stringify(info.port()) + 
request.url.path);
```


- Vinod Kone


On March 20, 2016, 11:03 p.m., Ashwin Murthy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45000/
> ---
> 
> (Updated March 20, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3902: [Updated] Fix location header in redirect from non-leader. 
> Addressed comments from Vinod and Ben. Removed the scheme but added the path 
> from the original request to the location header. Preserved the link to the 
> RFC in the comments.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
> 
> Diff: https://reviews.apache.org/r/45000/diff/
> 
> 
> Testing
> ---
> 
> Was trying to write unit test in scheduler http api test but it turns out 
> multi master tests cannot be written at this point. Need to manually verify 
> this by setting up multiple masters with ZK.
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Vinod Kone]

> On March 20, 2016, 6:36 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 645-647
> > 
> >
> > I know you followed the existing pattern in this file but we shouldn't 
> > do mktemp() like this in the tests because if the test fails for whatever 
> > reason the temp file and the directory are leaked. 
> > 
> > The FetcherTest fixture inherits from TemporaryDirectoryTest so that 
> > the test runs in a temporary sandbox.
> > 
> > So you should do:
> > 
> > ```
> > Try dir = os::mkdtemp("./"));
> > ASSERT_SOME(dir);
> > 
> > Try path = os::mktemp(path::join(dir.get(), ""));
> > ASSERT_SOME(path);
> > ```
> > 
> > Feel free to fix the other tests in this file in a subsequent review or 
> > just add a TODO for now.
> 
> Michael Browning wrote:
> Happy to fix the other tests in this file. I think it'd be simpler to do 
> it in a separate review -- should I also create an issue for that?

no need to create an issue.


- Vinod


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


On March 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44985, 45000]

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 March 20, 2016, 11:03 p.m., Ashwin Murthy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45000/
> ---
> 
> (Updated March 20, 2016, 11:03 p.m.)
> 
> 
> Review request for mesos and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3902: [Updated] Fix location header in redirect from non-leader. 
> Addressed comments from Vinod and Ben. Removed the scheme but added the path 
> from the original request to the location header. Preserved the link to the 
> RFC in the comments.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
> 
> Diff: https://reviews.apache.org/r/45000/diff/
> 
> 
> Testing
> ---
> 
> Was trying to write unit test in scheduler http api test but it turns out 
> multi master tests cannot be written at this point. Need to manually verify 
> this by setting up multiple masters with ZK.
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Michael Browning]

> On March 20, 2016, 6:36 p.m., Vinod Kone wrote:
> > src/slave/containerizer/fetcher.cpp, lines 156-160
> > 
> >
> > Why are you validating the filename as an URI? IIUC, filename should be 
> > a simple relative path?

In the absence of a URI schema, validateUri basically validates exactly that 
case -- but you're right, that's not really in line with the semantics of the 
validateUri method. I'll factor that logic out into a separate validateFilename 
function.


> On March 20, 2016, 6:36 p.m., Vinod Kone wrote:
> > src/tests/fetcher_tests.cpp, lines 645-647
> > 
> >
> > I know you followed the existing pattern in this file but we shouldn't 
> > do mktemp() like this in the tests because if the test fails for whatever 
> > reason the temp file and the directory are leaked. 
> > 
> > The FetcherTest fixture inherits from TemporaryDirectoryTest so that 
> > the test runs in a temporary sandbox.
> > 
> > So you should do:
> > 
> > ```
> > Try dir = os::mkdtemp("./"));
> > ASSERT_SOME(dir);
> > 
> > Try path = os::mktemp(path::join(dir.get(), ""));
> > ASSERT_SOME(path);
> > ```
> > 
> > Feel free to fix the other tests in this file in a subsequent review or 
> > just add a TODO for now.

Happy to fix the other tests in this file. I think it'd be simpler to do it in 
a separate review -- should I also create an issue for that?


- Michael


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


On March 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Ashwin Murthy]

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

(Updated March 20, 2016, 11:03 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description (updated)
---

MESOS-3902: [Updated] Fix location header in redirect from non-leader. 
Addressed comments from Vinod and Ben. Removed the scheme but added the path 
from the original request to the location header. Preserved the link to the RFC 
in the comments.


Diffs (updated)
-

  src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 

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


Testing
---

Was trying to write unit test in scheduler http api test but it turns out multi 
master tests cannot be written at this point. Need to manually verify this by 
setting up multiple masters with ZK.


Thanks,

Ashwin Murthy

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Ashwin Murthy]

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

(Updated March 20, 2016, 10:07 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description
---

MESOS-3902: Fix in location header during redirect from non-leader. Appended 
the path from original request to the location header


Diffs
-

  src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 

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


Testing
---

Was trying to write unit test in scheduler http api test but it turns out multi 
master tests cannot be written at this point. Need to manually verify this by 
setting up multiple masters with ZK.


Thanks,

Ashwin Murthy

Re: Review Request 45000: MESOS-3902: Fix in location header during redirect from non-leader.

[Ashwin Murthy]

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

(Updated March 20, 2016, 9:57 p.m.)


Review request for mesos and Vinod Kone.


Repository: mesos


Description (updated)
---

MESOS-3902: Fix in location header during redirect from non-leader. Appended 
the path from original request to the location header


Diffs (updated)
-

  src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 
  src/tests/containerizer/mesos_containerizer_tests.cpp 
f3ca32b2d8b8ef9dcfa8f20d9ceaff48b6598a66 

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


Testing
---

Was trying to write unit test in scheduler http api test but it turns out multi 
master tests cannot be written at this point. Need to manually verify this by 
setting up multiple masters with ZK.


Thanks,

Ashwin Murthy

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45057]

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 March 20, 2016, 7:50 p.m., Tomasz Janiszewski wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45057/
> ---
> 
> (Updated March 20, 2016, 7:50 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4885
> https://issues.apache.org/jira/browse/MESOS-4885
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `unzip` overwrite existing files without prompting.
> 
> 
> Diffs
> -
> 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45057/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Tomasz Janiszewski
> 
>

Re: Review Request 45088: Edited master's flag help strings to match the agent.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44678, 44703, 44515, 44523, 44553, 44554, 45088]

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 March 20, 2016, 7:01 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45088/
> ---
> 
> (Updated March 20, 2016, 7:01 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Edited master's flag help strings to match the agent.
> 
> With the addition of the `--authenticate_http` and `--http_credentials` flags 
> for the agent, the flag help strings have been improved slightly. This patch 
> edits the master's help string to match that of the agent.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
>   src/master/flags.cpp e6fea6421ea1a16b9cd78b0e42b830829b95ad61 
> 
> Diff: https://reviews.apache.org/r/45088/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 45057: Made `unzip` overwrite existing files without prompting.

[Tomasz Janiszewski]

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

(Updated March 20, 2016, 7:50 p.m.)


Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Made `unzip` overwrite existing files without prompting.


Diffs (updated)
-

  src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
  src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 

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


Testing
---


Thanks,

Tomasz Janiszewski

Re: Review Request 45078: MESOS-3902: Fix location header in redirect from non-leader.

[Ashwin Murthy]

> On March 20, 2016, 5:51 p.m., Vinod Kone wrote:
> > Looks like you created a new review instead of updating the existing review 
> > (https://reviews.apache.org/r/45000/) ? Can you discard this and update the 
> > old one instead. That way the review/comment history is not lost. You can 
> > use support/post-reviews.py to make this easy (see: 
> > http://mesos.apache.org/documentation/latest/submitting-a-patch/).

ok will do.


- Ashwin


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


On March 19, 2016, 9:57 p.m., Ashwin Murthy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45078/
> ---
> 
> (Updated March 19, 2016, 9:57 p.m.)
> 
> 
> Review request for mesos, Ben Whitehead, Joris Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3902: Fix location header in redirect from non-leader. Adding the path 
> of the original request into the redirect URL.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
> 
> Diff: https://reviews.apache.org/r/45078/diff/
> 
> 
> Testing
> ---
> 
> Manual testing to be done. Testing framework does not support multi-master 
> tests as yet.
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

[Jie Yu]

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



Some initial comments. Will do a pass to finish the rest later.


src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (lines 32 - 56)


Can we introduce paths.hpp|cpp under cni/ directory  for the canonical 
locations.

```
constexpr char ROOT_DIR[] = "...";

string cni::paths::getNamespaceHandle(
const string& rootDir,
const ContainerID& containerId);

string cni::paths::getNetworkPath(
const string& rootDir,
const ContainerID& containerId,
const string& name);

string cni::paths::getIPv4Path(
const string& rootDir,
const ContainerID& containerId,
const string& name,
const string& ifname);

string cni::paths::getIPv6Path(
const string& rootDir,
const ContainerID& containerId,
const string& name,
const string& ifname);
```



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 206 - 212)


I suggest we save a rootDir in the isolator process. We can easily switch 
to use a flag later. Also, we need to call 'realpath' here to make sure it's a 
realpath.

We also need to make sure ROOT_DIR is a self bind mounted directory 
(slave+shared) so that namespace bind mount does not leak into containers.


- Jie Yu


On March 20, 2016, 4:27 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 20, 2016, 4:27 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 isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44514: Implemented prepare() method of "network/cni" isolator.

[Jie Yu]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.hpp (line 82)


s/NetworkResultInfo/Info/

We typically use 'Info' to store information about each container in 
isolators.

We can then have a hashmap from network name to a another data structure in 
Info to represent the networks the container has joined.

```
// Information about a network that a container has joined.
struct NetworkInfo
{
  string ifname;
  cni::NetworkResult result;
};

struct Info
{
  hashmap networkInfos;
};
```


- Jie Yu


On March 18, 2016, 6:31 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44514/
> ---
> 
> (Updated March 18, 2016, 6: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
> ---
> 
> Implemented prepare() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44514/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 45088: Edited master's flag help strings to match the agent.

[Greg Mann]

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

(Updated March 20, 2016, 7:01 p.m.)


Review request for mesos, Adam B and Joerg Schad.


Summary (updated)
-

Edited master's flag help strings to match the agent.


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


Repository: mesos


Description (updated)
---

Edited master's flag help strings to match the agent.

With the addition of the `--authenticate_http` and `--http_credentials` flags 
for the agent, the flag help strings have been improved slightly. This patch 
edits the master's help string to match that of the agent.


Diffs (updated)
-

  docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
  src/master/flags.cpp e6fea6421ea1a16b9cd78b0e42b830829b95ad61 

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 44554: Added agent HTTP authentication to the docs.

[Greg Mann]

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

(Updated March 20, 2016, 6:58 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Edited docs to match flags.cpp.


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


Repository: mesos


Description
---

Added agent HTTP authentication to the docs.


Diffs (updated)
-

  docs/authentication.md e7c0bf3ed331411f607e7622419f14853006a480 
  docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
  docs/home.md fd7794f56b4a95268dbb82288d99cab19a89f5cd 

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


Testing
---

Viewed with the Mesos website container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

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

(Updated March 20, 2016, 6:57 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added agent flags for HTTP authentication.

Three command-line flags have been added to the agent to enable HTTP 
authentication: `--authenticate_http`, `--http_credentials`, and 
`--http_authenticators`.


Diffs (updated)
-

  src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 

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


Testing
---

`sudo make check` was used to test on both OSX and Ubuntu 14.04.


Thanks,

Greg Mann

Re: Review Request 45087: [WIP] Add `CpuSubsystem` for cgroups unified isolator.

[Mesos ReviewBot]

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



Bad patch!

Reviews applied: [45087, 45086, 45085, 45084, 45083]

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

Error:
2016-03-20 18:53:48 URL:https://reviews.apache.org/r/45083/diff/raw/ 
[2114/2114] -> "45083.patch" [1]
Total errors found: 0
Checking 1 files
Error: Commit message summary (the first line) must start with a capital letter.

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

- Mesos ReviewBot


On March 20, 2016, 5:47 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45087/
> ---
> 
> (Updated March 20, 2016, 5:47 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Bugs: MESOS-4697
> https://issues.apache.org/jira/browse/MESOS-4697
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Add `CpuSubsystem` for cgroups unified isolator.
> 
> 
> Diffs
> -
> 
>   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/45087/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Vinod Kone]

> On March 19, 2016, 10:07 a.m., Guangya Liu wrote:
> >

@michael you should mark the issues as "Fixed" or "Drop" once you address them. 
See http://mesos.apache.org/documentation/latest/submitting-a-patch/.


- Vinod


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


On March 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 45046: Created URI.filename to name fetched files in sandbox where appropriate.

[Vinod Kone]

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



Can you also add a CHANGELOG entry for this under "Additonal API changes" 
section for 0.29.0 (create one if it doesn't exist)?


src/launcher/fetcher.cpp (lines 252 - 260)


The way this is written, we will throw an error if basename cannot be 
determined irrespective of whether filename was specified.

How about:

```
Try basename = uri.has_filename()
  ? uri.filename()
  : Fetcher::basename(uri.value());
```



src/launcher/fetcher.cpp (lines 295 - 298)


ditto. see above.



src/launcher/fetcher.cpp (lines 295 - 298)


ditto. see above.



src/slave/containerizer/fetcher.cpp (lines 156 - 160)


Why are you validating the filename as an URI? IIUC, filename should be a 
simple relative path?



src/tests/fetcher_cache_tests.cpp (lines 689 - 697)


//
const string executablePath = path::join(
task.get().runDirectory.value, customFilename);

EXPECT_TRUE(isExecutable(executablePath));

// The script...
const string outputPath = path::join(
  task.get().runDirectory.value, COMMAND_NAME);

EXPECT_TRUE(os::exists(outputPath + taskName(index)));



src/tests/fetcher_tests.cpp (lines 645 - 647)


I know you followed the existing pattern in this file but we shouldn't do 
mktemp() like this in the tests because if the test fails for whatever reason 
the temp file and the directory are leaked. 

The FetcherTest fixture inherits from TemporaryDirectoryTest so that the 
test runs in a temporary sandbox.

So you should do:

```
Try dir = os::mkdtemp("./"));
ASSERT_SOME(dir);

Try path = os::mktemp(path::join(dir.get(), ""));
ASSERT_SOME(path);
```

Feel free to fix the other tests in this file in a subsequent review or 
just add a TODO for now.



src/tests/fetcher_tests.cpp (line 677)


kill this. if you did as suggested above, this will be automatically 
cleanedup as part of the teardown.



src/tests/fetcher_tests.cpp (lines 684 - 685)


ditto. see above.



src/tests/fetcher_tests.cpp (line 716)


kill this.


- Vinod Kone


On March 19, 2016, 10:03 p.m., Michael Browning wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45046/
> ---
> 
> (Updated March 19, 2016, 10:03 p.m.)
> 
> 
> Review request for mesos, Vinod Kone and Zhitao Li.
> 
> 
> Bugs: MESOS-4735
> https://issues.apache.org/jira/browse/MESOS-4735
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Created URI.filename to name fetched files in sandbox where appropriate.
> 
> 
> Diffs
> -
> 
>   docs/fetcher.md f70939d8410516c9387a8cba86b5b75539a5fe9a 
>   include/mesos/mesos.proto deb9c0910a27afd67276f54b3f666a878212727b 
>   include/mesos/v1/mesos.proto a981e750c24cfc48177bbc9ca56f0c3ecfae1a1b 
>   src/launcher/fetcher.cpp f85b118fb19cf9d4563f89847a783be35067e815 
>   src/slave/containerizer/fetcher.cpp 
> 33dfcade6beb53a5a6dbc41a8f3380f5cb30a161 
>   src/tests/fetcher_cache_tests.cpp 645dae208cb2b0aa2d2181d96eb1fd8893975430 
>   src/tests/fetcher_tests.cpp fb47706eb90ae5808bafe13c681d609a808b0c8e 
> 
> Diff: https://reviews.apache.org/r/45046/diff/
> 
> 
> Testing
> ---
> 
> There are two paths by which a file gets fetched to the executor sandbox: the 
> without-cache path, where the fetcher downloads the file directly from the 
> specified URI, and the with-cache path, where it copies it from the cache. In 
> both cases, we verify that the file is saved to the sandbox directory with 
> the name specified by the "filename" field in the CommandInfo.URI proto.
> 
> 
> Thanks,
> 
> Michael Browning
> 
>

Re: Review Request 44554: Added agent HTTP authentication to the docs.

[Joerg Schad]

> On March 19, 2016, 10:49 a.m., Joerg Schad wrote:
> > docs/configuration.md, line 1274
> > 
> >
> > dito.
> 
> Greg Mann wrote:
> Since the master's flag is '--credentials' rather than 
> '--http_credentials', and they have slightly different uses, I think these 
> should remain in the separate sections.

Agree, I got carried away by the dito commenting.


- Joerg


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


On March 20, 2016, 6:12 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44554/
> ---
> 
> (Updated March 20, 2016, 6:12 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
> https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent HTTP authentication to the docs.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md e7c0bf3ed331411f607e7622419f14853006a480 
>   docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
>   docs/home.md fd7794f56b4a95268dbb82288d99cab19a89f5cd 
> 
> Diff: https://reviews.apache.org/r/44554/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the Mesos website container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

> On March 19, 2016, 11:39 a.m., Joerg Schad wrote:
> > src/slave/flags.cpp, line 682
> > 
> >
> > One part I don't like is that the same flag on the master has a 
> > different name credentials.
> > I would suggest to deprecate (in another patch) to deprecate  masters 
> > credentials flag and introduce a new one for that. 
> > Already have a local patch for that, just interested in your opinion 
> > first.

Yes, the difference in naming is unfortunate. However, the master's 
`--credentials` flag is used for several purposes: HTTP authentication, 
framework authentication, and slave authentication, so I think that the more 
general name "credentials" makes sense for that reason.


- Greg


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


On March 20, 2016, 5:27 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44515/
> ---
> 
> (Updated March 20, 2016, 5:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flags for HTTP authentication.
> 
> Three command-line flags have been added to the agent to enable HTTP 
> authentication: `--authenticate_http`, `--http_credentials`, and 
> `--http_authenticators`.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
> 
> Diff: https://reviews.apache.org/r/44515/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 45088: Edited master's flags to match documentation.

[Greg Mann]

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

Review request for mesos, Adam B and Joerg Schad.


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


Repository: mesos


Description
---

Edited master's flags to match documentation.

With the addition of the `--authenticate_http` flag for the agent, the master 
and agent now share this flag. While adding the flag to the agent, the help 
string text was improved slightly, and this patch edits the master's help 
string to match that of the agent.


Diffs
-

  src/master/flags.cpp e6fea6421ea1a16b9cd78b0e42b830829b95ad61 

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


Testing
---


Thanks,

Greg Mann

Re: Review Request 44554: Added agent HTTP authentication to the docs.

[Greg Mann]

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

(Updated March 20, 2016, 6:12 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added agent HTTP authentication to the docs.


Diffs (updated)
-

  docs/authentication.md e7c0bf3ed331411f607e7622419f14853006a480 
  docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
  docs/home.md fd7794f56b4a95268dbb82288d99cab19a89f5cd 

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


Testing
---

Viewed with the Mesos website container: 
https://github.com/mesosphere/mesos-website-container


Thanks,

Greg Mann

Re: Review Request 45078: MESOS-3902: Fix location header in redirect from non-leader.

[Vinod Kone]

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



Looks like you created a new review instead of updating the existing review 
(https://reviews.apache.org/r/45000/) ? Can you discard this and update the old 
one instead. That way the review/comment history is not lost. You can use 
support/post-reviews.py to make this easy (see: 
http://mesos.apache.org/documentation/latest/submitting-a-patch/).

- Vinod Kone


On March 19, 2016, 9:57 p.m., Ashwin Murthy wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45078/
> ---
> 
> (Updated March 19, 2016, 9:57 p.m.)
> 
> 
> Review request for mesos, Ben Whitehead, Joris Van Remoortere, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MESOS-3902: Fix location header in redirect from non-leader. Adding the path 
> of the original request into the redirect URL.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp bfea8fa811c24d37b2d7a8109728e1f40217e02f 
> 
> Diff: https://reviews.apache.org/r/45078/diff/
> 
> 
> Testing
> ---
> 
> Manual testing to be done. Testing framework does not support multi-master 
> tests as yet.
> 
> 
> Thanks,
> 
> Ashwin Murthy
> 
>

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

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [44004, 44200, 44549, 44555, 44622, 44514, 44706, 45082]

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 March 20, 2016, 4:30 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45082/
> ---
> 
> (Updated March 20, 2016, 4:30 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 PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/45082/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44554: Added agent HTTP authentication to the docs.

[Greg Mann]

> On March 19, 2016, 10:49 a.m., Joerg Schad wrote:
> > docs/configuration.md, line 1274
> > 
> >
> > dito.

Since the master's flag is '--credentials' rather than '--http_credentials', 
and they have slightly different uses, I think these should remain in the 
separate sections.


> On March 19, 2016, 10:49 a.m., Joerg Schad wrote:
> > docs/authentication.md, line 85
> > 
> >
> > Do we have documentation for basic authenticator?

We don't have module documentation for the basic authenticator module. This 
makes sense right now, since users won't use the basic authenticator as a 
module, they'll just let it load as the default. If we move to not having a 
default authenticator module in the future, then I think it will make sense to 
explicitly document the basic authenticator as a module.


- Greg


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


On March 18, 2016, 7:30 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44554/
> ---
> 
> (Updated March 18, 2016, 7:30 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4850
> https://issues.apache.org/jira/browse/MESOS-4850
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent HTTP authentication to the docs.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md e7c0bf3ed331411f607e7622419f14853006a480 
>   docs/configuration.md d10fa2e7fc7c477de2f0e30e10da6d817ecbf404 
>   docs/home.md fd7794f56b4a95268dbb82288d99cab19a89f5cd 
> 
> Diff: https://reviews.apache.org/r/44554/diff/
> 
> 
> Testing
> ---
> 
> Viewed with the Mesos website container: 
> https://github.com/mesosphere/mesos-website-container
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Review Request 45087: [WIP] Add `CpuSubsystem` for cgroups unified isolator.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add `CpuSubsystem` for cgroups unified isolator.


Diffs
-

  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/45087/diff/


Testing
---


Thanks,

haosdent huang

Review Request 45084: [WIP] Add `Subsystem` abstraction for cgroups.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add `Subsystem` abstraction for cgroups.


Diffs
-

  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/45084/diff/


Testing
---


Thanks,

haosdent huang

Review Request 45086: [WIP] Enable cgroups unified isolator in isolation.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Enable cgroups unified isolator in isolation.


Diffs
-

  src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
  src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
  src/slave/containerizer/mesos/containerizer.cpp 
ee7a265975323ca891114a286357c8e42901560c 

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


Testing
---


Thanks,

haosdent huang

Review Request 45083: [WIP] Added a any mechanism for futures.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Added a any mechanism for futures.


Diffs
-

  3rdparty/libprocess/include/process/collect.hpp 
5a92b72eb7668494dc832ec446a41b3d673a20cc 

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


Testing
---


Thanks,

haosdent huang

Review Request 45085: [WIP] Add cgroup unified isolator.

[haosdent huang]

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

Review request for mesos and Jie Yu.


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


Repository: mesos


Description
---

Add cgroup unified isolator.


Diffs
-

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

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 45073: Restructured authentication.md to group common flags.

[Greg Mann]

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



Personally, I'm not convinced that this is a useful way to format our flag 
documentation. I've been meaning to propose eliminating it from 
'configuration.md'. I think it's more confusing to have the lists of flags 
broken up into multiple pieces. I'm guessing that most users come to the 
documentation either looking for a specific flag of either the master or agent, 
or they want to look over the entire list of flags for either the master or 
agent. In either case, I think that splitting up the flags makes it a little 
harder for them to find what they want. Thoughts?

- Greg Mann


On March 19, 2016, 11:51 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45073/
> ---
> 
> (Updated March 19, 2016, 11:51 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Restructured authentication.md to group common flags.
> 
> 
> Diffs
> -
> 
>   docs/authentication.md e7c0bf3ed331411f607e7622419f14853006a480 
> 
> Diff: https://reviews.apache.org/r/45073/diff/
> 
> 
> Testing
> ---
> 
> viewed via gist: https://gist.github.com/joerg84/63e5919fef724b5c3508 and 
> docker website container
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44989: Fixed a race in the resource offers tests.

[Greg Mann]

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

(Updated March 20, 2016, 5:35 p.m.)


Review request for mesos, Adam B and Joerg Schad.


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


Repository: mesos


Description
---

Fixed a race in the resource offers tests.

Adding HTTP credentials to `StartSlave` in 'src/tests/mesos.cpp' has exposed a 
race condition in ResourceOffersTest.ResourceOfferWithMultipleSlaves. The test 
quickly runs `StartSlave` 10 times to create 10 agents. Under the covers, 
`StartSlave` writes data to disk, and it seems that with the additional data 
being written to disk for HTTP credentials, the filesystem operations for one 
`StartSlave` call were not completing before the next call.

By settling the clock in between each invocation of `StartSlave`, this patch 
fixes the race. The test is slower, but it is now reliable. Running the test 3 
times, the old implementation gives an average runtime of 265ms, while the new 
one runs in an average of 359ms.


Diffs
-

  src/tests/resource_offers_tests.cpp 1cf292ee7931207596f8f06677386bef5965ef15 

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


Testing
---

`GTEST_FILTER="ResourceOffersTest.ResourceOfferWithMultipleSlaves" 
bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure=1` was used to 
test on both OSX and Ubuntu 14.04.


Thanks,

Greg Mann

Re: Review Request 44553: Added authentication to agent HTTP endpoints.

[Greg Mann]

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

(Updated March 20, 2016, 5:32 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


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


Repository: mesos


Description
---

Added authentication to agent HTTP endpoints.

This patch adds HTTP authentication to the `/state`, `/state.json`, and 
`/flags` endpoints. Tests are also updated to use authentication when hitting 
these endpoints, and a new test was added to probe these endpoints' behavior 
when bad credentials are supplied: `SlaveTest.HTTPEndpointsBadAuthentication`.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/containerizer/docker_containerizer_tests.cpp 
f6fce7df82417e029fadf805d6e0b793f396aa69 
  src/tests/fault_tolerance_tests.cpp f99413f56e96a796d3d45decad1f049e6a238789 
  src/tests/health_check_tests.cpp d32164aeb1eb439bd062afa28614dd919e24f06b 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing
---

Tests were updated to use authentication when hitting the affected agent 
endpoints, and new tests were added to probe the endpoint behavior when invalid 
credentials are supplied.

`sudo make check` was used to test on both OSX and Ubuntu 14.04. The new test 
was run 1000 times to look for flakiness.


Thanks,

Greg Mann

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

> On March 19, 2016, 9:16 a.m., Adam B wrote:
> > src/tests/mesos.cpp, line 177
> > 
> >
> > Why use `os::getcwd()` instead of `directory.get()` like "credential" 
> > and "fetch" above? Seems like `CreateSlaveFlags` wanted to use a temporary 
> > directory removed later.
> > I'm not convinced these two are equivalent.

Whoops - thanks for catching this Adam!!! You are correct, these aren't 
equivalent and this was in fact causing the race that I had to patch up :-) 
Using `directory.get()` gives us a different temporary directory for each of 
the agents created in `ResourceOffersTest.ResourceOfferWithMultipleSlaves`, 
while using `os::getcwd()` uses a common directory for all of them, which 
allows them to step on each others' toes when they're being spun up.

Changing this to `directory.get()` solves the problem, and is clearly what I 
should be doing here. Patch has been updated.


- Greg


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


On March 20, 2016, 5:27 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44515/
> ---
> 
> (Updated March 20, 2016, 5:27 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added agent flags for HTTP authentication.
> 
> Three command-line flags have been added to the agent to enable HTTP 
> authentication: `--authenticate_http`, `--http_credentials`, and 
> `--http_authenticators`.
> 
> 
> Diffs
> -
> 
>   src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
>   src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
>   src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
>   src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
>   src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 
> 
> Diff: https://reviews.apache.org/r/44515/diff/
> 
> 
> Testing
> ---
> 
> `sudo make check` was used to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 44515: Added agent flags for HTTP authentication.

[Greg Mann]

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

(Updated March 20, 2016, 5:27 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comment.


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


Repository: mesos


Description
---

Added agent flags for HTTP authentication.

Three command-line flags have been added to the agent to enable HTTP 
authentication: `--authenticate_http`, `--http_credentials`, and 
`--http_authenticators`.


Diffs (updated)
-

  src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 
  src/slave/flags.hpp feb095da4521f678c96f4cc53bdfda262d350388 
  src/slave/flags.cpp b77afa956834bb5b1f85301d7a5f386ab9da41e3 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/mesos.cpp 90aef6bfe619dc0acdb4ccba6a7180482dd13ce5 

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


Testing
---

`sudo make check` was used to test on both OSX and Ubuntu 14.04.


Thanks,

Greg Mann

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 20, 2016, 4:48 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

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


Testing
---


Thanks,

Greg Mann

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

[Qian Zhang]

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

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 PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

[Qian Zhang]

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

(Updated March 21, 2016, 12:27 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
---

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


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp PRE-CREATION 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp PRE-CREATION 

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


Testing
---

make check


Thanks,

Qian Zhang

Re: Review Request 45081: Excluded reserved resources when got nonRevocable resources in stage 1.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45081]

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 March 20, 2016, 2:06 p.m., Klaus Ma wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45081/
> ---
> 
> (Updated March 20, 2016, 2:06 p.m.)
> 
> 
> Review request for mesos and Alexander Rukletsov.
> 
> 
> Bugs: MESOS-4988
> https://issues.apache.org/jira/browse/MESOS-4988
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allocator will only allocate non-revocable resources to satify quota. As the 
> reserved resources can not be revocable, it's not necessary to call 
> `nonRevocable()` for reserved resources.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> 70291075c00a9a557529c2562dedcfc6c6c3ec32 
> 
> Diff: https://reviews.apache.org/r/45081/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> 
> Thanks,
> 
> Klaus Ma
> 
>

Review Request 44922: Fixed the port mapping tests compile errors.

[Jie Yu]

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

Review request for mesos and Joseph Wu.


Repository: mesos


Description
---

Fixed the port mapping tests compile errors.


Diffs
-

  src/tests/containerizer/port_mapping_tests.cpp 
d89780f0c62e27f18b08fbd99bc25ac62d0f5f3c 

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


Testing
---

make check on fedora 23


Thanks,

Jie Yu

Re: Review Request 44549: Introduced a protobuf message "NetworkConfig".

[Qian Zhang]

> On March 17, 2016, 11:37 a.m., Jie Yu wrote:
> > src/CMakeLists.txt, line 279
> > 
> >
> > I don't think we put headers here.

I had the same concern before, because it seems most source files here are 
.cpp. But I also see the followings, so I guess it should be OK to put headers 
here as well?
`slave/containerizer/composing.cpp`
`slave/containerizer/composing.hpp` 
`slave/containerizer/docker.cpp`
`slave/containerizer/docker.hpp`


- Qian


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


On March 16, 2016, 10:09 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44549/
> ---
> 
> (Updated March 16, 2016, 10:09 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
> ---
> 
> Introduced a protobuf message "NetworkConfig".
> 
> 
> Diffs
> -
> 
>   src/CMakeLists.txt 0517dd1ae7125ac1ae85cc83b6daaca66a10b8b4 
>   src/Makefile.am 9dd21b56af0500f7125b07bf535b45fe5c544aaf 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.cpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni/spec.proto PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44549/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On March 20, 2016, 4:19 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44934/
> ---
> 
> (Updated March 20, 2016, 4:19 a.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 ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 
> 
> Diff: https://reviews.apache.org/r/44934/diff/
> 
> 
> Testing
> ---
> 
> Tested with various Appc images.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 45081: Excluded reserved resources when got nonRevocable resources in stage 1.

[Klaus Ma]

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

(Updated March 20, 2016, 10:06 p.m.)


Review request for mesos and Alexander Rukletsov.


Changes
---

Update the bugs


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


Repository: mesos


Description
---

Allocator will only allocate non-revocable resources to satify quota. As the 
reserved resources can not be revocable, it's not necessary to call 
`nonRevocable()` for reserved resources.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Review Request 45081: Excluded reserved resources when got nonRevocable resources in stage 1.

[Klaus Ma]

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

Review request for mesos and Alexander Rukletsov.


Repository: mesos


Description
---

Allocator will only allocate non-revocable resources to satify quota. As the 
reserved resources can not be revocable, it's not necessary to call 
`nonRevocable()` for reserved resources.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
70291075c00a9a557529c2562dedcfc6c6c3ec32 

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


Testing
---

make
make check


Thanks,

Klaus Ma

Re: Review Request 45010: Fixed newlines for include directives.

[haosdent huang]

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


Ship it!




Ship It!

- haosdent huang


On 三月 18, 2016, 12:44 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45010/
> ---
> 
> (Updated 三月 18, 2016, 12:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed newlines for include directives.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> daa46e05d113fd62ea9dad042ec41aaab28ad003 
> 
> Diff: https://reviews.apache.org/r/45010/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 33174: Fix for docker not configuring CFS quotas correctly

[Jie Yu]

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



This patch breaks all the ROOT DOCKER tests in our internal CI. I've reverted 
it for now. Can you do a sudo make check with docker?

- Jie Yu


On March 16, 2016, 8:10 p.m., Steve Niemitz wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/33174/
> ---
> 
> (Updated March 16, 2016, 8:10 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2617
> https://issues.apache.org/jira/browse/MESOS-2617
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fix for docker containerizer not configuring CFS quotas correctly.
> 
> It would be nice to refactor all this isolation code in a way that can be 
> shared between all containerizers, as this is basically just copied from the 
> CgroupsCpushareIsolator, but that's a much bigger undertaking.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 79cd955e9c241becff52cc4bbef81dcc16802ee7 
>   src/slave/containerizer/docker.cpp fb9188a19a5cd8211d4f36f9647ebb70de560109 
> 
> Diff: https://reviews.apache.org/r/33174/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Steve Niemitz
> 
>

Review Request 44912: Used the same fixture for all related tests.

[Benjamin Bannier]

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

Review request for mesos, Joris Van Remoortere and Yong Tang.


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


Repository: mesos


Description
---

The initial idea was to only use a fixture creating temporary working
directories when really needed. It was felt that this hinders
discoverability of related test.


Diffs
-

  3rdparty/libprocess/src/tests/io_tests.cpp 
87fd3fafc29c01362bae0b77a2a332ec89a8b442 

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


Testing
---

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


Thanks,

Benjamin Bannier

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

Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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



For reference, Ahishek and I talked offline, and he modified his review based 
off of a branch I pushed out to github:
https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices

The old reviews that this review is based on are:
https://reviews.apache.org/r/44439/
https://reviews.apache.org/r/44796/

- Kevin Klues


On March 17, 2016, 7:21 p.m., Abhishek Dasgupta wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44974/
> ---
> 
> (Updated March 17, 2016, 7:21 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and 
> Niklas Nielsen.
> 
> 
> Bugs: MESOS-3368
> https://issues.apache.org/jira/browse/MESOS-3368
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> There are some helper methods added to support device cgroups in cgroups 
> abstraction to aid isolators controlling access to devices.
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 51ccefd1fd2d0989b9bd31342d3d1e3701f88ed2 
>   src/linux/cgroups.cpp df18ed46a2a96871f67c7eb4233c3b4c27b7aa1c 
> 
> Diff: https://reviews.apache.org/r/44974/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Abhishek Dasgupta
> 
>

Re: Review Request 44658: Removed unused signal escalation constant.

[Alexander Rukletsov]

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

(Updated March 18, 2016, 5:20 p.m.)


Review request for mesos and Ben Mahler.


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


Repository: mesos


Description
---

See summary.


Diffs (updated)
-

  src/slave/constants.hpp 4189c07dbad754e51f1a067ecbb4c99ae42a386f 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44755: Added getAppcImage for Appc provisioning tests.

[Guangya Liu]

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




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


add a period to the end.


- Guangya Liu


On 三月 17, 2016, 4:44 p.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44755/
> ---
> 
> (Updated 三月 17, 2016, 4:44 p.m.)
> 
> 
> Review request for mesos and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get an Appc image protobuf object with given
> parameters.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/provisioner_appc_tests.cpp 
> 069af8a0b5e031d9032fd13154cee250f279b404 
> 
> Diff: https://reviews.apache.org/r/44755/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 44706: Implemented isolate() method of "network/cni" isolator.

[Qian Zhang]

> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 277
> > 
> >
> > either s/realpath/`realpath`
> > 
> > or s/realpath/location

I see cgroups.cpp uses the word `canonical path` which I think is good, so 
let's just use it :-)


> On March 12, 2016, 2:45 a.m., Avinash sridharan wrote:
> > src/slave/containerizer/mesos/isolators/network/cni.cpp, line 358
> > 
> >
> > If we block here, it will lock the isolator, and prevent the 
> > `MesosContainerizer` from launching any more containers?
> 
> Qian Zhang wrote:
> I do not think it will prevent `MesosContainerizer` from launching any 
> more containers. Please take a look at: 
> https://github.com/apache/mesos/blob/0.27.2/src/slave/containerizer/mesos/containerizer.cpp#L945:L1034,
>  the whole code to call launcher to fork executor process and call isolator's 
> `isolate()` are already in a `defer()` as a lambda, and it is also a very big 
> lambda.
> 
> Avinash sridharan wrote:
> Hi Qian,
>  Didn't understand your comment. Are you implying calling `await` on the 
> `Future` won't block?

I think it will not block `MesosContainerizerProcess`, but it will indeed block 
`NetworkCniIsolatorProcess`. So yes, I agree we should use `defer`. And I have 
another question, for the container which want to join multiple networks, do we 
want to call plugin to join multiple networks in parallel or sequentially? I 
think joining one network should be independent on joining another, but if we 
do them in parallel, I am afraid that user may see "`eth2, eth0, eth1`" rather 
than "`eth0, eth1, eth2`" in the container. Please let me know your thought :-)


- Qian


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


On March 16, 2016, 3:50 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44706/
> ---
> 
> (Updated March 16, 2016, 3:50 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 isolate() method of "network/cni" isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni.hpp PRE-CREATION 
>   src/slave/containerizer/mesos/isolators/network/cni.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44706/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 44974: Added device support in cgroups abstraction.

[Kevin Klues]

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




src/linux/cgroups.hpp (line 645)


Because a selector major/minor value can either bo the '*' character or an 
unsinged integer, I'd probably change my constructors to reflect this. I.e.:

```
Selector(Type type, unsigned int major, unsigned int minor);
Selector(Type type, char minor, unsigned int minor);
Selector(Type type, unsigned int major, char minor);
Selector(Type type, char major, char minor);
```

And then just stringify them under the hood to store them as strings for a 
common representation.



src/linux/cgroups.hpp (lines 660 - 662)


I would probably just call these read/write/mknod instead of adding 
"device" as a prefix.



src/linux/cgroups.hpp (lines 665 - 666)


I see you copied my comment, but changed the name of the class to 
DeviceEntry :)

We should probaly update one or the other.

I am personaly in favor of simply 'Device', but I'll like @bmahler and 
@nnielsen chime in on their preferences.



src/linux/cgroups.hpp (lines 671 - 673)


We typically don't write accessor's like this.

Also, we don't space things out like this.  We use single spaces between 
the different elements.



src/linux/cgroups.hpp (lines 675 - 684)


I know we kept going back and forth about this offline, but in the end, I 
think it makes sense to just make all of this public and non-const. I'd even go 
so far as to say we just make this a struct instead of a class.

The original reasoning behind making this constructor private and the 
members const was to prevent someone from inputting wrong values for the 
major/minor numbers at initialization (because these values can either be the 
'*' character or an unsigned integer).  I think this is actually better handled 
in the Selector() constructor, as mentioned above.

Moreover, even if they have bad values, this will all be caught by errors 
when reading/writing to the `devices.allow`, `devices.deny`, and `devices.list` 
files.



src/linux/cgroups.hpp (lines 687 - 694)


For the use case of the GPU, I also need `operator==` for all of these 
structs.  They are implemented in my github branch I showed you before:


https://github.com/klueska-mesosphere/mesos/commits/r/a10gupta/cgroups-devices



src/linux/cgroups.cpp (lines 2426 - 2441)


There's probably no need for this extra level of indirection through the 
`type` variable.  I would also consider using a `switch` statement since these 
are all enums.

```
  switch (selector.deviceType) {
case DeviceEntry::Selector::ALL:
  stream << "a";
  break;
case DeviceEntry::Selector::BLOCK:
  stream << "b";
  break;
case DeviceEntry::Selector::CHARACTER:
  stream << "c";
  break;
default:
  LOG(ERROR) << "Unable to parse device type: " + selector.deviceType;
  return stream;
  }

  return stream << " "
<< selector.deviceMajor
<< ":"
<< selector.deviceMinor;
```



src/linux/cgroups.cpp (line 2459)


The style guieds require two newlines between functions in a `.cpp` file. 
Could you do a pass to fix this up?



src/linux/cgroups.cpp (lines 2463 - 2465)


You shouldn't have to call `stringify()` around the `selector` and `access` 
fields because you have already provided `operator<<` operators for them.



src/linux/cgroups.cpp (lines 2523 - 2526)


I would probably move this block up to line 2509.  We typically try to 
error out on cases we know are error conditions as early in the code as 
possible.  

In this case, we know we require 3 arguments as soon as we determine that 
the first token is not an "a", so we shoud error out right away.



src/linux/cgroups.cpp (line 2539)


I would probably reverse this to say:

```
if (deviceNumbers[0] != "*" major.isError())
```

The reason being that the only reason we care about the error is because 
the `deviceNumber != "*"`.



src/linux/cgroups.cpp (line 2546)


Same comment as above.

Re: Review Request 45001: Replaced const ref to temporary in tests for consistency.

[Jie Yu]

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


Ship it!




Ship It!

- Jie Yu


On March 18, 2016, 12:15 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45001/
> ---
> 
> (Updated March 18, 2016, 12:15 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Replaced const ref to temporary in tests for consistency.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> daa46e05d113fd62ea9dad042ec41aaab28ad003 
>   3rdparty/libprocess/3rdparty/stout/tests/os_tests.cpp 
> a69fd2d08838e9f0b5738531b29dbca996b70dcb 
> 
> Diff: https://reviews.apache.org/r/45001/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Review Request 45014: Add /containers endpoint to return ResourceUsage.

[Jay Guo]

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

Review request for mesos and Jie Yu.


Summary (updated)
-

Add /containers endpoint to return ResourceUsage.


Repository: mesos


Description (updated)
---

Add /containers endpoint to return ResourceUsage.


Diffs (updated)
-

  src/slave/http.cpp 4eb1fafdfa72094511b0b2684a3c2705bd8c7c5e 
  src/slave/slave.hpp 7520cc356e2b1b7f5fff15f33071a46a7b05e762 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 
  src/tests/slave_tests.cpp ea1d776077bf638885db8421194aa4427c772169 

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


Testing (updated)
---

make check
`curl agent_ip:port/containers` returns same json content as `curl 
agent_ip:port/monitor/statistics.json`

This is a draft patch of adding /containers endpoint to agent 
[MESOS-4891](https://issues.apache.org/jira/browse/MESOS-4891)

ContainerStatus will be added to response based on this patch.


Thanks,

Jay Guo

Review Request 44906: Update containerizer construction in fetcher_cache_tests.cpp.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update containerizer construction in fetcher_cache_tests.cpp.


Diffs
-

  src/tests/fetcher_cache_tests.cpp 776c95267caff7b27cd70c2fa6149d8158c86750 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44657: Used `KillPolicy` and shutdown grace period in command executor.

[Alexander Rukletsov]

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

(Updated March 18, 2016, 5:19 p.m.)


Review request for mesos, Ben Mahler and Gilbert Song.


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


Repository: mesos


Description
---

The command executor determines how much time it allots the
underlying task to clean up (effectively how long to wait for
the task to comply to SIGTERM before sending SIGKILL) based
on both optional task's `KillPolicy` and optional
`shutdown_grace_period` field in `ExecutorInfo`.


Diffs (updated)
-

  src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 
  src/slave/slave.cpp 840534ff0687e82ed063c386e36bbabada230697 

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


Testing
---

The complete chain was tested. See https://reviews.apache.org/r/44662/.


Thanks,

Alexander Rukletsov

Re: Review Request 44852: Documented existing allocator metrics.

[Benjamin Bannier]

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

(Updated March 18, 2016, 5:08 p.m.)


Review request for mesos, Alexander Rukletsov and Ben Mahler.


Changes
---

Review comments from BenM.


Repository: mesos


Description
---

Documented existing allocator metrics.


Diffs (updated)
-

  docs/monitoring.md 827f7073204fcf8575ca980a5571c8be4f5e4110 

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


Testing
---

Checked rendering with packaged docker image.


Thanks,

Benjamin Bannier

Re: Review Request 44660: Used `KillPolicy` and shutdown grace period in docker executor.

[Timothy Chen]

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




src/docker/executor.cpp (line 226)


This should fit 80 char width?


- Timothy Chen


On March 15, 2016, 4:04 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44660/
> ---
> 
> (Updated March 15, 2016, 4:04 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Gilbert Song, and Timothy Chen.
> 
> 
> 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
> -
> 
>   src/docker/executor.cpp 1921d4a1ce3c45b4e2f81f0ef5914d5830da6866 
> 
> Diff: https://reviews.apache.org/r/44660/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44379: Correctly parse perf stat format for non-vanilla 3.10 kernel.

[Ben Mahler]

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




src/linux/perf.cpp (lines 435 - 437)


Hm.. this comment is really hard for me to understand, if OS vendors 
enhance the format, how did you know that they only use this particular format 
and not others?


- Ben Mahler


On March 10, 2016, 3:26 a.m., fan du wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44379/
> ---
> 
> (Updated March 10, 2016, 3:26 a.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4705
> https://issues.apache.org/jira/browse/MESOS-4705
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> For vanilla kernel <= 3.12, 'perf stat' format is:
>   value,event,cgroup
> Tested with 3.12, 3.11, 3.10 vanilla kernel.
> 
> OS vendors may enhance perf tools with additional format:
>   value,unit,event,cgroup,running,ratio
> Tested on Centos 7 with kernel 3.10.0-{123 OR 229}.el7.x86_64
> 
> 
> Diffs
> -
> 
>   src/linux/perf.cpp 1c113a2b3f57877e132bbd65e01fb2f045132128 
> 
> Diff: https://reviews.apache.org/r/44379/diff/
> 
> 
> Testing
> ---
> 
> 1. {Found and Test} with Serenity, ema filter could get perf event statistics 
> correctly as expected.
> 2. ./bin/mesos-tests.sh --gtest_filter=PerfEventIsolatorTest* 
> --log_dir=/tmp/mesos/
> 
> 
> Thanks,
> 
> fan du
> 
>

Re: Review Request 44654: Fixed hard-coded executor shutdown grace period in executor library.

[Ben Mahler]

> On March 15, 2016, 6:39 p.m., Ben Mahler wrote:
> > src/exec/exec.cpp, line 113
> > 
> >
> > If you'd like to add the `private` qualifier, why isn't `kill` left as 
> > protected?
> 
> Alexander Rukletsov wrote:
> I do not know what is our general guideline here. I think we are 
> inconsitent about access modifiers; I've checked `AwaitProcess`, 
> `CollectProcess`, `ReaperProcess`, `SequenceProcess`. Here I followed this 
> sentence from the Google style guide: "Limit the use of protected to those 
> member functions that might need to be accessed from subclasses. Note that 
> data members should be private." I left `kill` as protected because that was 
> the original intention of the author.
> 
> I'm fine with making it private, because it's unlikely we are going to 
> subclass it. However, it will be inconsistent to what we have in 
> `ShutdownProcess` in "executor/executor.cpp".
> 
> Moreover, I think we should factor out `ShutdownProcess` because it is 
> already used in two places. I suggest to leave the code consistent for now 
> and change to private when we refactor. Does it make sense?

I'd rather not factor out ShutdownProcess because it is not clearly a good 
re-usable abstraction. Both uses could be better served by the following very 
simple code, no need for sharing:

```
delay(gracePeriod,
  []() {
killpg(0, SIGKILL);
os::sleep(Seconds(5));
exit(EXIT_FAILURE);
  });
```

There isn't really a benefit to the ShutdownProcess abstraction on top of just 
doing a simple delay. It is necessary only because there isn't support for 
executing the delay within a synchronous or `__executor__`-based execution 
context (to avoid the ExecutorProcess blocking issue).


- Ben


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


On March 17, 2016, 11:30 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44654/
> ---
> 
> (Updated March 17, 2016, 11:30 p.m.)
> 
> 
> Review request for mesos and Ben Mahler.
> 
> 
> Bugs: MESOS-4911
> https://issues.apache.org/jira/browse/MESOS-4911
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Executor shutdown grace period, which configured on the agent, is
> propagated to executors via the `MESOS_EXECUTOR_SHUTDOWN_GRACE_PERIOD`
> environment variable. The executor library uses this timeout to delay
> the hard shutdown of the related executor.
> 
> 
> Diffs
> -
> 
>   src/exec/exec.cpp 741786132f3a8cc43f5b9ced262429038832a946 
>   src/executor/executor.cpp 87db4e02cbaa778aab0173741bfe066fdee9a48d 
> 
> Diff: https://reviews.apache.org/r/44654/diff/
> 
> 
> Testing
> ---
> 
> The complete chain was tested. See https://reviews.apache.org/r/44662/.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 44850: Added missing include in allocator/mesos/hierarchical.hpp.

[Ben Mahler]

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


Ship it!




Ship It!

- Ben Mahler


On March 15, 2016, 2:51 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44850/
> ---
> 
> (Updated March 15, 2016, 2:51 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Ben Mahler.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added missing include in allocator/mesos/hierarchical.hpp.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.hpp 
> 52b3a9bfbe73d24968f7ddb0672aee1e05636ab0 
> 
> Diff: https://reviews.apache.org/r/44850/diff/
> 
> 
> Testing
> ---
> 
> make check (OS X, clang-trunk)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 44975: Updated cgroups test cases for cgroups device support.

[Abhishek Dasgupta]

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

Review request for mesos, Ben Mahler, Felix Abecassis, Kevin Klues, and Niklas 
Nielsen.


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


Repository: mesos


Description
---

Updated cgroups test cases for cgroups device support.


Diffs
-

  src/tests/containerizer/cgroups_tests.cpp 
acaed9b3f8a04964092cef413133834d0cf5a145 

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


Testing
---

make check


Thanks,

Abhishek Dasgupta

Re: Review Request 44944: Handled chunked responses in docker URI fetcher.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On March 17, 2016, 1:50 a.m., Jie Yu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44944/
> ---
> 
> (Updated March 17, 2016, 1:50 a.m.)
> 
> 
> Review request for mesos, Gilbert Song, Jojy Varghese, James Peach, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-4964
> https://issues.apache.org/jira/browse/MESOS-4964
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Handled chunked responses in docker URI fetcher.
> 
> This is because if the response is chunked, curl will output the dechunked 
> version. That will confuse the http response decoder because the header shows 
> that it is chunked.
> 
> 
> Diffs
> -
> 
>   src/uri/fetchers/docker.cpp 2ceee199c6b1bf92c5eca80c8d3344e97e394a09 
> 
> Diff: https://reviews.apache.org/r/44944/diff/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Jie Yu
> 
>

Re: Review Request 44996: Fixed how we detect C++11 compiler support in libprocess.

[Neil Conway]

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

(Updated March 17, 2016, 11:52 p.m.)


Review request for mesos and Joris Van Remoortere.


Changes
---

Rebase.


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


Repository: mesos


Description
---

Adopt a new version of the M4 macro from the upstream macro archive.
We no longer care about whether the compiler supports particular
C++11 features, so we can discard some previous local modifications
we had to this macro. This should also make it easier to mandate
C++14 support in the future.


Diffs (updated)
-

  3rdparty/libprocess/configure.ac 134f8ef4395d059e2901bbb48826fbfc4e6e3124 
  3rdparty/libprocess/m4/ax_cxx_compile_stdcxx.m4 PRE-CREATION 
  3rdparty/libprocess/m4/ax_cxx_compile_stdcxx_11.m4 
251213a7e5e57da4b367f033865594b3678d53aa 

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


Testing
---

make check with GCC6


Thanks,

Neil Conway

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

[haosdent huang]

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




docs/versioning.md (line 85)


Any reason we need change here?



support/non-ascii.py (line 21)


Seems we have to update this when we update third part dependencies every 
time? For example, if we update protobuf to 2.6, this rule need update as well. 
Is it possible to avoid this?


- haosdent huang


On March 18, 2016, 2:48 p.m., Yong Tang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45033/
> ---
> 
> (Updated March 18, 2016, 2:48 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Bernd Mathiske.
> 
> 
> 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, and .md 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.
> 
> 
> Diffs
> -
> 
>   docs/versioning.md ecacd8433f0fa1643827b36d03154042538c1c6b 
>   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 45010: Fixed newlines for include directives.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [45001, 45002, 45003, 45010]

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 March 18, 2016, 12:44 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/45010/
> ---
> 
> (Updated March 18, 2016, 12:44 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Neil Conway.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed newlines for include directives.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
> daa46e05d113fd62ea9dad042ec41aaab28ad003 
> 
> Diff: https://reviews.apache.org/r/45010/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 43615: Update test suite to use the reworked MesosTest helpers.

[Jie Yu]

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




src/tests/containerizer/port_mapping_tests.cpp 


Why move this down? So we remove the the meta data while the slave is still 
running?


- Jie Yu


On March 16, 2016, 12:21 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/43615/
> ---
> 
> (Updated March 16, 2016, 12:21 a.m.)
> 
> 
> Review request for mesos, Bernd Mathiske and Artem Harutyunyan.
> 
> 
> Bugs: MESOS-4633 and MESOS-4634
> https://issues.apache.org/jira/browse/MESOS-4633
> https://issues.apache.org/jira/browse/MESOS-4634
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Includes the following changes:
> 
> * Added the `` header where appropriate.
> * Added the namespace `using process::Owned;` where appropriate.
> * Generally replaced `Try` with `Try`.  
> And `Try` with `Try`.
> * Added the (now required) `MasterDetector` argument to all slaves.  Before, 
> this was fetched from the first master in `Cluster`.
> * Removed `Shutdown();` from all tests.
> * Replaced `Stop(...)` with the appropriate master/slave destruction calls.
> * Wrap various slave objects in `Owned` (i.e. containerizers, isolators, 
> launchers, etc).
> * Replace `CHECK` in tests with `ASSERT`.
> 
> 
> Diffs
> -
> 
>   src/tests/authentication_tests.cpp 85f14c3d453ca5aeffa1c915f38fe3031c2cf712 
>   src/tests/command_executor_tests.cpp 
> 0d2fcf6d4b8d9a925eb6748e6bd33cf279b8f7f8 
>   src/tests/container_logger_tests.cpp 
> 00f4129e46aa9268fbb66da25b34e61004fa87b2 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 8afaa4dab3984e9866b7b223e8e2e70ef83a39dc 
>   src/tests/containerizer/external_containerizer_test.cpp 
> 8e1dbe306a088eb16cd3b9c6174b95fad5685da4 
>   src/tests/containerizer/filesystem_isolator_tests.cpp 
> e72239a55724f1aeeec5362cc370c93dbeca7164 
>   src/tests/containerizer/isolator_tests.cpp 
> 342037ce0a5f8caa4e3cf1550b8f9a7cc328acf9 
>   src/tests/containerizer/memory_pressure_tests.cpp 
> 03879d99c371f296f8d9904666911b34209c114d 
>   src/tests/containerizer/mesos_containerizer_tests.cpp 
> 15f0f93d2e5c19a22f6cc4a71a7d94be4aaec2c1 
>   src/tests/containerizer/port_mapping_tests.cpp 
> a1427fd0157dee343b643f3272dba8ffea61f7b0 
>   src/tests/containerizer/provisioner_docker_tests.cpp 
> b2663a83afccbb156f10b4703f29ee0f638384bf 
>   src/tests/containerizer/runtime_isolator_tests.cpp 
> 1625614f560a4c9a644b1b9e9568199f30373ab5 
>   src/tests/credentials_tests.cpp 7edcc857e0f6f8e80e265deeec59d6349d392224 
>   src/tests/disk_quota_tests.cpp 413e562026a4fc9779f616e921ae2fa2ca51e012 
>   src/tests/exception_tests.cpp 6b71316d545e97f14a45daa14d0fd95204befd3b 
>   src/tests/executor_http_api_tests.cpp 
> 2fc0893f5f5e80a783296fb31b30abe86d92df1b 
>   src/tests/fault_tolerance_tests.cpp 
> 67f362747ac4301a74693aeaa5631b7dd866e782 
>   src/tests/gc_tests.cpp 61a8abb9581dc4602b197a88a677b19386969cbf 
>   src/tests/health_check_tests.cpp 9605859b665645654bbdb2688455cae2d692a057 
>   src/tests/hook_tests.cpp bb287c77493209e663a37e7f88d09a0459855f7d 
>   src/tests/http_fault_tolerance_tests.cpp 
> 7c7f3d90210148176e83553346100a506f263591 
>   src/tests/master_allocator_tests.cpp 
> cba7c36471f93b678d94e1da0251a28a893696b1 
>   src/tests/master_authorization_tests.cpp 
> 6a4de4709960d7ca505e99396e14a1bb51d6902d 
>   src/tests/master_contender_detector_tests.cpp 
> 255ab8119a04b55bb4f1b61dee19c4be64499376 
>   src/tests/master_quota_tests.cpp f1277f03f5b5cf735fea7ba324a27614a829cd50 
>   src/tests/master_slave_reconciliation_tests.cpp 
> d41178eb41df519073fc0890c5716bbc9fed6ad2 
>   src/tests/master_tests.cpp e8c39e775d6ca218ce74cfc6bb50c7576d73e90e 
>   src/tests/master_validation_tests.cpp 
> c9bc38ce604d2d44d6e6b1286507d1c45e5e9e25 
>   src/tests/metrics_tests.cpp 419d275e0b32817388120222bd433ee6f4835efd 
>   src/tests/monitor_tests.cpp 869c9e032817e8859a968232d4a61556a3d53d45 
>   src/tests/oversubscription_tests.cpp 
> e528476cd83b0e3f7ae8cea7d86dfabc1f66484e 
>   src/tests/partition_tests.cpp 3776a0a104582f60b9f19ea58b011485194399b9 
>   src/tests/persistent_volume_endpoints_tests.cpp 
> b5f425ab7f2b4a540418d761bea145565467bd2b 
>   src/tests/persistent_volume_tests.cpp 
> e9215de2e073025f67cdc73e8a8de38cf030671f 
>   src/tests/rate_limiting_tests.cpp 352a5e7a58b500c25c7c8a421047c4e79434e38e 
>   src/tests/reconciliation_tests.cpp e8f3f29836652d20a6ee1bb5231a15e71eb76990 
>   src/tests/registrar_zookeeper_tests.cpp 
> 3df9779ee5d076e16f6a538326693a36f986b6d0 
>   src/tests/repair_tests.cpp 0dfb557a62f64baf2334dbc9f75ecdfb10473c2d 
>   

Re: Review Request 44709: Allowed unknown flags in command and docker executors.

[Alexander Rukletsov]

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

(Updated March 18, 2016, 5:24 p.m.)


Review request for mesos, Ben Mahler and Jie Yu.


Repository: mesos


Description
---

We allow unknown flags to ensure that when a new flag is added
in Mesos version N, agent of version N works with the executor
of version N-1, i.e. the latter does not segfault when observing
a newly introduced flag.


Diffs (updated)
-

  src/docker/executor.cpp afc769d0887e3842106e4c350e94c95c8ffc085e 
  src/launcher/executor.cpp 4149f084b0b234fb995df04e9d1ca704feec082e 

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


Testing
---

`make check` on Mac OS 10.10.4


Thanks,

Alexander Rukletsov

Re: Review Request 44287: Added MasterContender and MasterDetector abstract classes.

[Anurag Singh]

> On March 11, 2016, 9:40 p.m., Joseph Wu wrote:
> > I think you should send an email to the user and dev mailing lists to ask 
> > for high-level feedback on this interface.  We want to make sure the 
> > interface is broad enough to support different implementations.  (And I'm 
> > no expert on leader election.)
> 
> Anurag Singh wrote:
> I'll do that and address your comments.

I haven't seen any comments from the user and developer communities to my email 
on these changes. Do you want to wait longer?


- Anurag


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


On March 15, 2016, 11:01 p.m., Anurag Singh wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44287/
> ---
> 
> (Updated March 15, 2016, 11:01 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-4610
> https://issues.apache.org/jira/browse/MESOS-4610
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> MasterContender and MasterDetector abstract classes can be subclassed by
> user-provided classes to allow for arbitrary contender/detector 
> implementations.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/contender.hpp PRE-CREATION 
>   include/mesos/master/detector.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/44287/diff/
> 
> 
> Testing
> ---
> 
> See https://reviews.apache.org/r/44670/.
> 
> 
> Thanks,
> 
> Anurag Singh
> 
>

Re: Review Request 44954: Regenerated master endpoint documentation.

[Adam B]

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


Ship it!




Ship It!

- Adam B


On March 17, 2016, 10:33 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44954/
> ---
> 
> (Updated March 17, 2016, 10:33 a.m.)
> 
> 
> Review request for mesos, Adam B and Greg Mann.
> 
> 
> Bugs: MESOS-4934
> https://issues.apache.org/jira/browse/MESOS-4934
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Reran the generate-endpoint-help.py scipt after adding the AUTHENTICATION 
> information to HELP.
> 
> 
> Diffs
> -
> 
>   docs/endpoints/master/api/v1/scheduler.md 
> 7a75fd75eef458f26f9e5067b792329f310d5d24 
>   docs/endpoints/master/create-volumes.md 
> 2ab116a16581fc5cfc0fc5cf70c6e001a28d8493 
>   docs/endpoints/master/destroy-volumes.md 
> 88679fe12baf7642b63e71688b90031aaedab1ef 
>   docs/endpoints/master/flags.md 739d6bfae5ae35223d6bc7c4782ff0f16b868018 
>   docs/endpoints/master/frameworks.md 
> 95c1f0e63f6a61d0e6622a8ec9d821a646656cfc 
>   docs/endpoints/master/health.md 7f9352025a6364b812b91e9ccbe380a3a69ba4e1 
>   docs/endpoints/master/machine/down.md 
> 6eec9ecc6e92fd1299b27499f78055c6f4f68e81 
>   docs/endpoints/master/machine/up.md 
> 7cbbf04859a218f90672453e5037480676f79fe5 
>   docs/endpoints/master/maintenance/schedule.md 
> 5f5267d24e392d6302d126e2367be08e6cc5e174 
>   docs/endpoints/master/maintenance/status.md 
> 4d0c7551acb89fb375834fd703c406d68f8bdcfc 
>   docs/endpoints/master/observe.md fae1ee062350d7b25775bef98a0bebda1892ab62 
>   docs/endpoints/master/quota.md 812874d2dd6c3548887e3044ba1f3c3c8c9d1dd6 
>   docs/endpoints/master/redirect.md ac9d0fa3eae485726b10a3ac756228d0cb5aeb27 
>   docs/endpoints/master/reserve.md 1a4f67961baf761f79a693780f42a1a8ce2244fc 
>   docs/endpoints/master/roles.json.md 
> 863715386791ef9192f38bf390fcd2b31d988547 
>   docs/endpoints/master/roles.md 171e0163c4c2db34bf34c8303846793f3c29bdf5 
>   docs/endpoints/master/slaves.md ec169c15507d3c731b8833f2656949caf0efcc33 
>   docs/endpoints/master/state-summary.md 
> fb10ac7db5b94ea7982c245f1d884312d818c6b5 
>   docs/endpoints/master/state.json.md 
> 0415cfdd63cee0b350b3e33c496bacfc1ba53f8a 
>   docs/endpoints/master/state.md ae1ec718b8d718718727dbd2e7ce4739cca41680 
>   docs/endpoints/master/tasks.json.md 
> 46a1253d33cdfdca0a7158808d1e29da1a634933 
>   docs/endpoints/master/tasks.md 2ada97b5dfdaf5f1f7578fa42eb4e61233e6a753 
>   docs/endpoints/master/teardown.md f68d083a4271ed7dd0ddcc87da104a43eb40c7ec 
>   docs/endpoints/master/unreserve.md e059d8d888343c5036346f7a615f04375f44f517 
>   docs/endpoints/master/weights.md 632be242d85fbd61ded62d846e385009a321533c 
> 
> Diff: https://reviews.apache.org/r/44954/diff/
> 
> 
> Testing
> ---
> 
> viewed docs with docker-website generator.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 44989: Fixed a race in the resource offers tests.

[Neil Conway]

> On March 18, 2016, 9:47 a.m., Adam B wrote:
> > src/tests/resource_offers_tests.cpp, line 63
> > 
> >
> > Why pause so soon? You can wait until after the master is started, but 
> > just before you start calling StartSlave() in the loop
> 
> Joerg Schad wrote:
> I agree with you, but actually we follow this pattern in many other tests 
> as well.
> E.g. 
> // This test ensures that allocation is done per slave. This is done
> // by having 2 slaves and 2 frameworks and making sure each framework
> // gets only one slave's resources during an allocation.
> TEST_F(HierarchicalAllocatorTest, CoarseGrained)
> {
>   // Pausing the clock ensures that the batch allocation does not
>   // influence this test.
> 
> Joerg Schad wrote:
> TEST_F(HierarchicalAllocatorTest, CoarseGrained)
> {
>   // Pausing the clock ensures that the batch allocation does not
>   // influence this test.
>   Clock::pause();

Is there an advantage to delaying the pause? Running the whole test with the 
clock paused (and hence pausing the clock at the beginning of the test) seems 
fine to me.


- Neil


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


On March 18, 2016, 4:35 p.m., Greg Mann wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/44989/
> ---
> 
> (Updated March 18, 2016, 4:35 p.m.)
> 
> 
> Review request for mesos, Adam B and Joerg Schad.
> 
> 
> Bugs: MESOS-4849
> https://issues.apache.org/jira/browse/MESOS-4849
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a race in the resource offers tests.
> 
> Adding HTTP credentials to `StartSlave` in 'src/tests/mesos.cpp' has exposed 
> a race condition in ResourceOffersTest.ResourceOfferWithMultipleSlaves. The 
> test quickly runs `StartSlave` 10 times to create 10 agents. Under the 
> covers, `StartSlave` writes data to disk, and it seems that with the 
> additional data being written to disk for HTTP credentials, the filesystem 
> operations for one `StartSlave` call were not completing before the next call.
> 
> By settling the clock in between each invocation of `StartSlave`, this patch 
> fixes the race. The test is slowed considerably, but it is now reliable.
> 
> 
> Diffs
> -
> 
>   src/tests/resource_offers_tests.cpp 
> 1cf292ee7931207596f8f06677386bef5965ef15 
> 
> Diff: https://reviews.apache.org/r/44989/diff/
> 
> 
> Testing
> ---
> 
> `GTEST_FILTER="ResourceOffersTest.ResourceOfferWithMultipleSlaves" 
> bin/mesos-tests.sh --gtest_repeat=1000 --gtest_break_on_failure=1` was used 
> to test on both OSX and Ubuntu 14.04.
> 
> 
> Thanks,
> 
> Greg Mann
> 
>

Re: Review Request 42386: Updated `createFrameworkInfo` for hierarchical_allocator_tests.cpp.

[Guangya Liu]

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

(Updated 三月 17, 2016, 1:12 a.m.)


Review request for mesos, Ben Mahler, Artem Harutyunyan, Joris Van Remoortere, 
Joseph Wu, Klaus Ma, and Jian Qiu.


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


Repository: mesos


Description
---

Updated `createFrameworkInfo` for hierarchical_allocator_tests.cpp.


Diffs (updated)
-

  src/tests/hierarchical_allocator_tests.cpp 
459e02576f6d05abbbcc83ae5cabac5c66e93f05 

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


Testing
---

make
make tests
GLOG_v=2 ./bin/mesos-tests.sh  --gtest_filter="HierarchicalAllocatorTest.*" 
--verbose


Thanks,

Guangya Liu

Review Request 45002: Added FTS_PHYSICAL option to fts_open for rmdir.

[Jojy Varghese]

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

Review request for mesos, Jie Yu and Neil Conway.


Repository: mesos


Description
---

According to the documentation for fts_open, either FTS_PHYSICAL or FTS_LOGICAL
SHOULD be provided. We need FTS_PHYSICAL for the case of rmdir as we dont want
to resolve the symlink targets while deleting them.


Diffs
-

  3rdparty/libprocess/3rdparty/stout/include/stout/os/posix/rmdir.hpp 
cbc97596cd8ed1e6d4261568fd0086c86e063232 
  3rdparty/libprocess/3rdparty/stout/tests/os/rmdir_tests.cpp 
daa46e05d113fd62ea9dad042ec41aaab28ad003 

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


Testing
---

make check.


Thanks,

Jojy Varghese

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 March 18, 2016, 5:26 p.m.)


Review request for mesos and Jie Yu.


Repository: mesos


Description
---

Updated mesos-execute to add support for Appc.


Diffs (updated)
-

  src/cli/execute.cpp ed42cb568f3d16856f48b3bbd354cb2b0fb83e8e 

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


Testing
---

Tested with various Appc images.


Thanks,

Jojy Varghese

Review Request 44895: Update containerizer construction in container_logger_tests.cpp.

[haosdent huang]

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

Review request for mesos and Till Toenshoff.


Repository: mesos


Description
---

Update containerizer construction in container_logger_tests.cpp.


Diffs
-

  src/tests/container_logger_tests.cpp 00f4129e46aa9268fbb66da25b34e61004fa87b2 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 44703: Added Doxygen docs for basic HTTP authenticator.

[Greg Mann]

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

(Updated March 18, 2016, 6:13 p.m.)


Review request for mesos, Adam B, Alexander Rojas, Joerg Schad, and Till 
Toenshoff.


Changes
---

Addressed comments.


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


Repository: mesos


Description
---

Added Doxygen docs for basic HTTP authenticator.

The structure of the module parameters for the basic HTTP authenticator has 
changed, so Doxygen comments were added to the module's header file to provide 
an example.


Diffs (updated)
-

  include/mesos/authentication/http/basic_authenticator_factory.hpp 
c11bb47c8e02f2e8645cf387d18eb64d1c8cb604 

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


Testing
---


Thanks,

Greg Mann