Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Adam B]

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

Ship it!


A few other minor nits, but I think we can commit this for 0.23.0-rc2.
I'd even be willing to make some of the changes myself before committing.


docs/network-monitoring.md (line 7)


s/running under/on/?
s/which bind/those that bind/



docs/network-monitoring.md (line 15)


> "Mesos will automatically check for those kernel functionalities and will 
abort if they are not supported"
Is this still true?



docs/network-monitoring.md (line 67)


> "If it is necessary to reduce this range to free ports to be allocated by 
the slave, it can be configured by defining the new range in `etc/sysctl.conf` 
and rebooting to eliminate"

In "free ports", I couldn't tell if "free" was supposed to be a verb or 
adjective. 
How about "If ports need to be set aside for slave containers, the host 
ephemeral port range can be updated in `etc/sysctl.conf`. Rebooting after the 
update will eliminate"



docs/network-monitoring.md (lines 69 - 71)


This comment confused me. What you really mean is that the previous range 
was 32768-61000, but then you took away the first chunk for Mesos to use for 
its containers.
```
# Set aside 32768-57344 for Mesos containers.
# net.ipv4.ip_local_port_range = 32768 61000
net.ipv4.ip_local_port_range = 57345 61000
```



docs/network-monitoring.md (line 77)


Maybe insert a  before "`number of ephemeral_ports / 
ephemeral_ports_per_container`" so that it goes on a single line.



docs/network-monitoring.md (line 79)


What is the default value of ephemeral_ports_per_container? (1024)



docs/network-monitoring.md (line 92)


Remove indentation if you want this to render as a header.



docs/network-monitoring.md (line 106)


We should only bother to include this flag if we set a non-default value.



docs/network-monitoring.md (line 107)


Can't you just say `300MB` like in your description? Or does this flag only 
handle KB?


- Adam B


On July 7, 2015, 5:59 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36281]

All tests passed.

- Mesos ReviewBot


On July 8, 2015, 12:59 a.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 8, 2015, 12:59 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36273]

All tests passed.

- Mesos ReviewBot


On July 8, 2015, 12:24 a.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36273/
> ---
> 
> (Updated July 8, 2015, 12:24 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess process headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/process.hpp 
> 59b50af86e059463a01f3c83701bc5fd143d51a4 
> 
> Diff: https://reviews.apache.org/r/36273/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Adam B]

> On July 7, 2015, 5:01 p.m., Adam B wrote:
> > docs/network-isolation.md, line 79
> > 
> >
> > Why is this a fixed constant limit? Seems like we might want to adjust 
> > this depending on how many containers are running, or give some containers 
> > more bandwidth than others. Why isn't this just another resource type 
> > (outbound network bandwidth) with a fixed total, where subsets can be 
> > reserved/claimed by different frameworks/tasks.
> 
> Paul Brett wrote:
> Outbound network bandwidth sounds like a reasonable future enhancement - 
> do you want to raise a ticket?

Sure. Created https://issues.apache.org/jira/browse/MESOS-3014


- Adam


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


On July 7, 2015, 5:59 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36226]

All tests passed.

- Mesos ReviewBot


On July 8, 2015, 12:12 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36226/
> ---
> 
> (Updated July 8, 2015, 12:12 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding missing Apache licence header
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 519e38c 
> 
> Diff: https://reviews.apache.org/r/36226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Paul Brett]

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

(Updated July 8, 2015, 12:59 a.m.)


Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.


Changes
---

Incorporate remaining reviewer comments.


Repository: mesos


Description
---

Document per-container unique egress flows and network queueing statistics.


Diffs (updated)
-

  docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
  docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 

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


Testing
---

Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.


Thanks,

Paul Brett

Re: Review Request 34138: AppC hash computation.

[Jiang Yan Xu]

> On July 7, 2015, 3:56 p.m., Jiang Yan Xu wrote:
> > 1. Agree that this is useful as a utility in libprocess. Not much overhead 
> > to move it over right?
> > 2. It feels like something that could be exposed as a function rather than 
> > class, maybe a TODO.

OK I realized that doing the aforementioned refactorings is not as simple as 
moving the file so probably punting it is the right thing to do for now.

1. As a generic utility it's probably giong to be SHA instead SHA512.
2. OK SHA512::hash() is already static but I meant if made more generic like 
SHA(alorightm).hash() then shorthand functions like `sha1()`, `sha512()` is 
easier to use.


- Jiang Yan


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


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Paul Brett]

> On July 8, 2015, 12:01 a.m., Adam B wrote:
> > docs/network-isolation.md, line 79
> > 
> >
> > Why is this a fixed constant limit? Seems like we might want to adjust 
> > this depending on how many containers are running, or give some containers 
> > more bandwidth than others. Why isn't this just another resource type 
> > (outbound network bandwidth) with a fixed total, where subsets can be 
> > reserved/claimed by different frameworks/tasks.

Outbound network bandwidth sounds like a reasonable future enhancement - do you 
want to raise a ticket?


> On July 8, 2015, 12:01 a.m., Adam B wrote:
> > docs/network-isolation.md, lines 70-73
> > 
> >
> > Is 1024 a reasonable example ephemeral_ports_per_container? Your 
> > current example only allows 24 containers on that slave, which is extremely 
> > low. Maybe 16 ports per container would make more sense, yielding 1536 
> > possible containers.

Its a reasonable number for the kinds of services I see running on mesos, after 
all we don't churn through containers very quickly.


> On July 8, 2015, 12:01 a.m., Adam B wrote:
> > docs/network-isolation.md, lines 24-29
> > 
> >
> > Do these dependencies need to be added to getting-started.md?

I don't believe so since this is not a default configuration.


- Paul


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


On July 8, 2015, 12:03 a.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 8, 2015, 12:03 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Paul Brett]

> On July 7, 2015, 11:31 p.m., Jie Yu wrote:
> > docs/network-isolation.md, line 7
> > 
> >
> > Please do not delete the version information. Network monitoring is 
> > added in mesos 0.20 and network isolation is added in mesos 0.23.
> > 
> > I suggest the following layout for the summary paragraph:
> > 
> > ```
> > # Network Monitoring and Isolation.
> > 
> > Mesos 0.20.0 adds the support for per container network monitoring. ...
> > 
> > Mesos 0.23.0 adds the support for per container network isolation. ...
> > 
> > Our solution is transparent to the tasks running on the slave ...
> > ```

We should address the changes in the changelog and publish historical 
documentation on the website for those running older releaes (raised MESOS-3011 
to addresss).


- Paul


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


On July 8, 2015, 12:03 a.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 8, 2015, 12:03 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 35947: Added a new API call 'updateAvailable' to the allocator.

[Jie Yu]

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

Ship it!



src/master/allocator/mesos/hierarchical.hpp (lines 734 - 737)


Could you please add a NOTE here explaining why this is possible? (e.g., 
it's possible because an allocation can happen after updateAvailable is 
enqueued but before it's actually being processed).


- Jie Yu


On June 26, 2015, 10:55 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35947/
> ---
> 
> (Updated June 26, 2015, 10:55 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Benjamin Hindman, Ben Mahler, 
> and Jie Yu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Needed to implement the master HTTP endpoints: `/reserve`, `/unreserve`, 
> `/create` and `/destroy`.
> 
> This API is similar to `updateSlave` which is currently very specific to 
> oversubscription.
> I considered consolidating `updateAvailable` and `updateSlave` but it will 
> require making offers be generated within the allocator to enable this.
> 
> In specific, `updateAvailable` could fail if there aren't sufficient 
> available resources to update, whereas `updateSlave` avoids failing by 
> keeping the allocator in an over-allocated state. For `updateSlave`, leaving 
> the allocator in an over-allocated state is ok. This is because it does not 
> modify resources therefore `total - allocated` will work out to __empty__. 
> `updateAvailable` cannot leave the allocator in an over-allocated state, 
> because it modifies resources, and therefore `total - allocated` is not 
> guaranteed to yield __empty__.
> 
> 
> Diffs
> -
> 
>   include/mesos/master/allocator.hpp 22992c0c77058af4fcd28aa8e4a1191693a16f44 
>   src/master/allocator/mesos/allocator.hpp 
> 72470ec7f56f84a9a9815c09adb88def90ef672f 
>   src/master/allocator/mesos/hierarchical.hpp 
> 3264d145d52b48852878abf7ab9be29ab98208cc 
>   src/tests/hierarchical_allocator_tests.cpp 
> 3258840135290cd008ca09235d18b7f093dafd2e 
>   src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 
> 
> Diff: https://reviews.apache.org/r/35947/diff/
> 
> 
> Testing
> ---
> 
> (1) Added `HierarchicalAllocatorTest.updateAvailableSuccess` and 
> `HierarchicalAllocatorTest.updateAvailableFail`
> (2) `make check`
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 34139: AppC image discovery.

[Jiang Yan Xu]

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



src/slave/containerizer/provisioners/appc/discovery.hpp (lines 43 - 46)


Some comments explaining that the return value is a URI string would be 
nice.



src/slave/containerizer/provisioners/appc/discovery.hpp (line 46)


The `id` is not use in this review and not specified in 
https://github.com/appc/spec/blob/master/spec/discovery.md
I assume its the sha512 but is it used during discovery?



src/slave/containerizer/provisioners/appc/discovery.hpp (line 79)


No need for the trailing `;`



src/slave/containerizer/provisioners/appc/discovery.cpp (line 21)


Kill empty line.



src/slave/containerizer/provisioners/appc/discovery.cpp (line 60)


canonicalize() not introduced until /r/34142/.
Is there anything else outside discovery that uses the method? Can it be 
moved to class `Discovery` (so we don't have to deal with cyclic review 
dependency)?



src/slave/containerizer/provisioners/appc/discovery.cpp (line 90)


FWIW https://github.com/appc/spec/blob/master/spec/discovery.md uses https 
exclusively.



src/slave/flags.cpp (line 63)


List all supported values?



src/slave/flags.cpp (line 68)


State that this is only for local discovery?

The sentences already mentions 'local images' but I think 
--appc_discovery=local is more explict in telling what the operator should do.


- Jiang Yan Xu


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34139/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Local and simple discovery only.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34139/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36273: Doxygen-ification of comments in libprocess process headers.

[Joseph Wu]

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

(Updated July 7, 2015, 5:24 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad.


Repository: mesos


Description
---

Doxygen-ification of comments in libprocess process headers.


Diffs
-

  3rdparty/libprocess/include/process/process.hpp 
59b50af86e059463a01f3c83701bc5fd143d51a4 

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


Testing
---

`doxygen ../Doxyfile` and visually verified the HTML output.


Thanks,

Joseph Wu

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36282]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 11:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
>   src/slave/containerizer/containerizer.cpp 
> 69dfac04cfd9c388fc908b68ac7abbc14304e621 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty

[Isabel Jimenez]

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

(Updated July 8, 2015, 12:12 a.m.)


Review request for mesos and Benjamin Hindman.


Repository: mesos-incubating


Description
---

Adding missing Apache licence header


Diffs (updated)
-

  3rdparty/libprocess/3rdparty/Makefile.am 519e38c 

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


Testing
---


Thanks,

Isabel Jimenez

Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty

[Isabel Jimenez]

> On July 7, 2015, 11:43 p.m., Till Toenshoff wrote:
> > 3rdparty/libprocess/3rdparty/Makefile.am, lines 1-7
> > 
> >
> > This should be the “Apache License Version 2.0” header instead which 
> > applies to libprocess and stout, no? (see docs/mesos-c++-styleguide.md at 
> > "File Headers").

Yes, Till, used the wrong script for this one. My bad.


- Isabel


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


On July 8, 2015, 12:12 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36226/
> ---
> 
> (Updated July 8, 2015, 12:12 a.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding missing Apache licence header
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 519e38c 
> 
> Diff: https://reviews.apache.org/r/36226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

> On July 7, 2015, 11:24 p.m., Till Toenshoff wrote:
> > src/slave/containerizer/containerizer.hpp, line 152
> > 
> >
> > Shouldnt it be called `includeOSEnvironment` instead? Bit unsure right 
> > now but please consider that :)

I think it's Os since all abbreviations that's included as part of the variable 
name no longer is capped.


- Timothy


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


On July 7, 2015, 11:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
>   src/slave/containerizer/containerizer.cpp 
> 69dfac04cfd9c388fc908b68ac7abbc14304e621 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Paul Brett]

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

(Updated July 8, 2015, 12:03 a.m.)


Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.


Changes
---

Incorporate comments from Ian Downes


Repository: mesos


Description
---

Document per-container unique egress flows and network queueing statistics.


Diffs (updated)
-

  docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
  docs/network-isolation.md PRE-CREATION 
  docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 

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


Testing
---

Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.


Thanks,

Paul Brett

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Adam B]

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


Thanks for writing this up! I had a few minor suggestions and some questions.


docs/network-isolation.md (line 11)


"follow the following" is a little redundant.



docs/network-isolation.md (line 15)


s/kernels versions/kernel versions/



docs/network-isolation.md (lines 24 - 29)


Do these dependencies need to be added to getting-started.md?



docs/network-isolation.md (line 31)


`### Build Configuration`?



docs/network-isolation.md (lines 40 - 42)


I would put the parameter example between these two sentences. Where it is 
now, I expect to see the "appropriate error" just described.



docs/network-isolation.md (line 48)


s/so that service discovery/so that the service discovery/



docs/network-isolation.md (line 68)


s/ephemerlal/ephemeral/



docs/network-isolation.md (lines 70 - 73)


Is 1024 a reasonable example ephemeral_ports_per_container? Your current 
example only allows 24 containers on that slave, which is extremely low. Maybe 
16 ports per container would make more sense, yielding 1536 possible containers.



docs/network-isolation.md (line 72)


`s/\/\/`?



docs/network-isolation.md (line 79)


Why is this a fixed constant limit? Seems like we might want to adjust this 
depending on how many containers are running, or give some containers more 
bandwidth than others. Why isn't this just another resource type (outbound 
network bandwidth) with a fixed total, where subsets can be reserved/claimed by 
different frameworks/tasks.



docs/network-isolation.md (line 85)


Is the `="true"` necessary, or would `--egress_unique_flow_per_container` 
work on its own?



docs/network-isolation.md (line 93)


Maybe leave out cpu/mem/disk to let the slave auto-detect them (and shorten 
this line)?



docs/network-isolation.md (line 178)


lowercase?



docs/network-isolation.md (line 198)


lowercase?



docs/network-isolation.md (line 204)


This footnote `[1]` is never referenced above


- Adam B


On July 7, 2015, 2:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36226: Missing Apache headers for libprocess 3rdparty

[Till Toenshoff]

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



3rdparty/libprocess/3rdparty/Makefile.am (lines 1 - 7)


This should be the “Apache License Version 2.0” header instead which 
applies to libprocess and stout, no? (see docs/mesos-c++-styleguide.md at "File 
Headers").


- Till Toenshoff


On July 6, 2015, 10:17 p.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36226/
> ---
> 
> (Updated July 6, 2015, 10:17 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Repository: mesos-incubating
> 
> 
> Description
> ---
> 
> Adding missing Apache licence header
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/3rdparty/Makefile.am 519e38c 
> 
> Diff: https://reviews.apache.org/r/36226/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Isabel Jimenez
> 
>

Re: Review Request 36037: Adding /call endpoint to Master

[Marco Massenzio]

> On July 3, 2015, 12:29 a.m., Ben Mahler wrote:
> > I chatted with Isabel on IRC and asked her to break apart this change into 
> > more bite-sized chunks, so that we can do smaller reviews and get things 
> > committed incrementally:
> > 
> > (1) Dummy /call handler on the master.
> > (2) Validation.
> > (3) Partial implementation of Call (i.e. parsing logic).
> > 
> > Each part can have its own tests. She will be discarding this review in 
> > favor of smaller chunks, which we can commit incrementally. :)
> > 
> > I also asked her to:
> > 
> > (a) Punt on the constants and remove master/http_constants.hpp, since these 
> > constants aren't adding value (CLOSE -> "close") for the added indirection, 
> > and our existing code doesn't follow this pattern.
> > (b) Pull out the change to src/tests/mesos.hpp, since it is independent.
> 
> Marco Massenzio wrote:
> All good.
> However, I beg to disagree on this point:
> >(a) Punt on the constants and remove master/http_constants.hpp, since 
> these constants aren't adding value (CLOSE -> "close") for the added 
> indirection, and our existing code doesn't follow this pattern.
> 
> We *do* have a `constants.hpp` (and relative .cpp) file and I do believe 
> it does add value (I, for one, certainly appreciated having it when I did the 
> JSON/ZK change ;) ): for example, I've already seen the string 
> `application/x-protobuf` typed up 10 times in just two reviews: there is 
> value in having an APPLICATION_PROTOBUF constant to:
> 
> - avoid difficult-to-spot bugs to typos (`application/x-prolobuf`) that 
> may only surface at runtime in production;
> - avoid typing the same stuff again and again (especially those of us 
> using modern IDEs can take advantage of code-completion ;) )
> - this is anyway common standard good practice and would allow us to not 
> having to agonize too much in case we need to refactor something (say, at 
> some point we want to use `application/x-protobuf-binary` for whatever reason 
> - there's only one place to do so; sure, this is an unlikely example, but 
> there may be cases where it may not be so far-fetched).
> 
> Also, *not* doing it does not save (I think?) any effort in reviewing 
> and/or committing, so seems very low cost for a potential sizeable payoff.
> 
> Ah, yes, and there's also the fact that hard-coded strings sprinkled all 
> over the code base are hard to maintain - I know, I've had to pick up the 
> pieces at least twice in the last 4 years ;)
> 
> PS - I do agree that defining `const string CLOSE = "close"` may be 
> pushing this one step too far... but I'd like to retain it for those more 
> commonly used strings.
> 
> Ben Mahler wrote:
> I don't think we're in disagreement, I just want this to be punted so 
> that we can think carefully about how to improve 'Request' and 'Response' 
> usage, rather than bundling it in this code review. Doing more than one thing 
> in a review tends to drag out the review, and I didn't want Isabel to get 
> distracted with this.
> 
> So let's follow up! In particular, having http constants in 
> master/http_constants.hpp is strange because it isn't master specific (we 
> have common/http.hpp for ones relevant to many components in mesos, http.hpp 
> for libprocess). Also, where possible, I'm hoping to avoid the difficulty in 
> header map manipulation entirely by providing typed members (there's a 
> [TODO](https://github.com/apache/mesos/blob/0.23.0-rc1/3rdparty/libprocess/include/process/http.hpp#L107)
>  which briefly alludes to this). For example, `request.connection` could be 
> an enum to capture the possible connection types.

> I don't think we're in disagreement

Awesome! :)

> Doing more than one thing in a review tends to drag out the review,

Completely agree, I just thought that factoring out the constants at the outset 
was rather uncontroversial and best done now rather than have to refactor later.

> having http constants in master/http_constants.hpp 

yep - I had actually missed that: do you have a better suggestion? (maybe 
`common/api_constants.hpp` could be a better place/name? something else?)


> For example, request.connection could be an enum to capture the possible 
> connection types.

Oh, yes - totally: typed enum >> string constant >> hard-coded string :)


- Marco


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


On July 2, 2015, 8:16 a.m., Isabel Jimenez wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36037/
> ---
> 
> (Updated July 2, 2015, 8:16 a.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Benjamin Hindman, Ben Mahle

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Jie Yu]

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

Ship it!


Thanks Paul! This is great!

It's a little hard to review this change because you change the file name (I 
won't be able to see the diff). It would be better if you change the file name 
in a separate patch. Next time!


docs/home.md (line 24)


I would suggest calling it 'Network Monitoring and Isolation'.

Do not forget to change the file name as well.



docs/network-isolation.md (line 5)


Ditto. Calling Network Monitoring and Isolation is more appropriate.

Please to a sweep to fix all occurances in this doc.



docs/network-isolation.md (line 7)


Please do not delete the version information. Network monitoring is added 
in mesos 0.20 and network isolation is added in mesos 0.23.

I suggest the following layout for the summary paragraph:

```
# Network Monitoring and Isolation.

Mesos 0.20.0 adds the support for per container network monitoring. ...

Mesos 0.23.0 adds the support for per container network isolation. ...

Our solution is transparent to the tasks running on the slave ...
```



docs/network-isolation.md (line 98)


Container Network Statistics?


- Jie Yu


On July 7, 2015, 9:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [36281]

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

Error:
 2015-07-07 23:20:54 URL:https://reviews.apache.org/r/36281/diff/raw/ 
[21625/21625] -> "36281.patch" [1]
36281.patch:59: trailing whitespace.
Network isolation is enabled on the slave by appending `network/port_mapping` 
to the slave command line. If the slave has not been compiled with network 
isolation support, it will refuse to start and print an appropriate error. 
36281.patch:297: new blank line at EOF.
+
warning: 2 lines add whitespace errors.
Successfully applied: Document per-container unique egress flows and network 
queueing statistics.

Document per-container unique egress flows and network queueing statistics.


Review: https://reviews.apache.org/r/36281
docs/network-isolation.md:40: trailing whitespace.
+Network isolation is enabled on the slave by appending `network/port_mapping` 
to the slave command line. If the slave has not been compiled with network 
isolation support, it will refuse to start and print an appropriate error. 
docs/network-isolation.md:278: new blank line at EOF.
Failed to commit patch

- Mesos ReviewBot


On July 7, 2015, 9:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Till Toenshoff]

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

Ship it!


Barring Adams comment.


src/slave/containerizer/containerizer.hpp (line 152)


Shouldnt it be called `includeOSEnvironment` instead? Bit unsure right now 
but please consider that :)


- Till Toenshoff


On July 7, 2015, 11:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
>   src/slave/containerizer/containerizer.cpp 
> 69dfac04cfd9c388fc908b68ac7abbc14304e621 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Cong Wang]

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



docs/network-isolation.md (line 29)


libnl3-devel is the package on Red Hat distribution, not for other disto.



docs/network-isolation.md (line 48)


Please explicitly mention that currently binding to an unassigned port is 
allowed by kernel too (we need a patch to disallow this), just that packets 
will be dropped silently.



docs/network-isolation.md (line 81)


s/Separating container traffic/Egress traffic isolation/



docs/network-isolation.md (line 83)


That is called flow classification and isolation. Please also mention that 
flow is classified based on port ranges.


- Cong Wang


On July 7, 2015, 9:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 9:54 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Adam B]

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

Ship it!


Looks like a simpler, more generic fix. Maybe external_containerizer will want 
to use includeOsEnvironment flag too.


src/slave/containerizer/containerizer.cpp (lines 248 - 253)


Seems like one of these could be an else if, since they both wipe 
`environment` and start from scratch. I would probably start with 
`if(flags.executor_environment_variables)` then `else if(includeOsEnvironment)`.


- Adam B


On July 7, 2015, 4:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 4:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
>   src/slave/containerizer/containerizer.cpp 
> 69dfac04cfd9c388fc908b68ac7abbc14304e621 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36279: Doxygen-ification of comments in libprocess IO headers.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36279]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 10:14 p.m., Joseph Wu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36279/
> ---
> 
> (Updated July 7, 2015, 10:14 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg 
> Schad.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Doxygen-ification of comments in libprocess IO headers.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/io.hpp 
> 63887704743f53c6e49ab504f549419e1d5910ce 
> 
> Diff: https://reviews.apache.org/r/36279/diff/
> 
> 
> Testing
> ---
> 
> `doxygen ../Doxyfile` and visually verified the HTML output.
> 
> 
> Thanks,
> 
> Joseph Wu
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

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


Updated the review to use a new optional boolean flag. The docker bridge test 
passes with this as well. Once I have a new ship it then I can merge.

- Timothy Chen


On July 7, 2015, 11:05 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 11:05 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/containerizer.hpp 
> 0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
>   src/slave/containerizer/containerizer.cpp 
> 69dfac04cfd9c388fc908b68ac7abbc14304e621 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

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

(Updated July 7, 2015, 11:05 p.m.)


Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.


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


Repository: mesos


Description
---

Remove os environment when calling executorEnvironment when running docker 
executors, since all the environment variables will be 
passed into the container and causes bad behavior such as overriding hostname.


Diffs (updated)
-

  src/slave/containerizer/containerizer.hpp 
0ee17e6bc52d1e3acefad6bda3a1b7ba64a8a54b 
  src/slave/containerizer/containerizer.cpp 
69dfac04cfd9c388fc908b68ac7abbc14304e621 
  src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Joerg Schad]

> On July 7, 2015, 10:58 p.m., Joerg Schad wrote:
> > src/slave/containerizer/docker.cpp, line 1544
> > 
> >
> > Wouldn't it be easier --especially considering Adam's comment about 
> > MESOS-2832 and the --executor_environment_variable --- to add an additional 
> > (optional) flag to executorEnvironment intialize=true. if explicitly set to 
> > false (such as in this case, we would not initialize environment with 
> > os::environment()
> 
> Till Toenshoff wrote:
> I like this approach better as well - seems much cleaner to me.

flag -> parameter...


- Joerg


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


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Till Toenshoff]

> On July 7, 2015, 10:58 p.m., Joerg Schad wrote:
> > src/slave/containerizer/docker.cpp, line 1544
> > 
> >
> > Wouldn't it be easier --especially considering Adam's comment about 
> > MESOS-2832 and the --executor_environment_variable --- to add an additional 
> > (optional) flag to executorEnvironment intialize=true. if explicitly set to 
> > false (such as in this case, we would not initialize environment with 
> > os::environment()

I like this approach better as well - seems much cleaner to me.


- Till


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


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Joerg Schad]

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



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


Wouldn't it be easier --especially considering Adam's comment about 
MESOS-2832 and the --executor_environment_variable --- to add an additional 
(optional) flag to executorEnvironment intialize=true. if explicitly set to 
false (such as in this case, we would not initialize environment with 
os::environment()


- Joerg Schad


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 34138: AppC hash computation.

[Jiang Yan Xu]

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


1. Agree that this is useful as a utility in libprocess. Not much overhead to 
move it over right?
2. It feels like something that could be exposed as a function rather than 
class, maybe a TODO.


src/slave/containerizer/provisioners/appc/hash.hpp (lines 22 - 31)


```
#include 
#include 

#include 

#include 

...
```

According to the style guide.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 45 - 55)


I would do 

```
if (!system("sha512sum -h &> /dev/null")) {...}
```

to avoid fixing the binary location.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 89 - 91)


I think we use 4 spaces to continue a left angle  bracket the same way we 
continue an left paren.



src/slave/containerizer/provisioners/appc/hash.hpp (line 97)


The `command` is not necessarily `sha512sum`. Maybe use it a static member 
so we detect once and use it with every hash() invocation?



src/slave/containerizer/provisioners/appc/hash.hpp (line 98)


Misaligned indentation.



src/slave/containerizer/provisioners/appc/hash.hpp (line 102)


The `command` is not necessarily `sha512sum`.



src/slave/containerizer/provisioners/appc/hash.hpp (line 109)


Misaligned indentation.



src/slave/containerizer/provisioners/appc/hash.hpp (lines 112 - 122)


This check doesn't work with openssl.

```
/usr/bin/openssl dgst -sha512 somefile.txt
SHA512(somefile.txt)= 
5a73e55fd845981be5d5b87039c678b87404405d5d054c579cf684a18893d181085b9afde535c034221f858d2bcc2b14978b4d5f4d6facfaa1f81e727a010f3c
```

I think we only need shasum and sha512sum to cover both Linux and OSX.


- Jiang Yan Xu


On July 7, 2015, 12:42 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34138/
> ---
> 
> (Updated July 7, 2015, 12:42 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> AppC hash computation.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/34138/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

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



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


It's actually being used in the header if you see below.


- Timothy Chen


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

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



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


We have to filter even if the flag is set, since we're actually including 
more than what the flags has specified.

What's interesting though is that we might unintentionally filter env 
variables if it's set from the flags and it's also included already in the os 
environment with the same key and value.

I can further check with the flags.executor_environment_variables so I 
don't filter a env variable that's also set in there, but I think it might be a 
lot easier to introduce a optional boolean flag (includeOsEnvironment) on 
executorEnvironment method to be able to not include os::environment(), and 
it's set to true by default?

Then all the DockerContainerizer has to do is to set that extra flag to 
false. What you think Ben?


- Timothy Chen


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36106: cgroups: added cpuacct subsystem

[Jojy Varghese]

> On July 2, 2015, 11:32 p.m., Ben Mahler wrote:
> > src/linux/cgroups.hpp, lines 443-472
> > 
> >
> > Thanks!
> > 
> > (1) Do you mind updating my TODO on cgroups::stat() to reflect that 
> > cpuacct::stat is implemented?
> > 
> > (2) Can we just make this a simple struct with two non-const fields and 
> > remove the parse method and getters? Keep it simple and small, if we want 
> > immutability we can just have a 'const Stat' rather than forcing it on the 
> > caller :)
> > 
> > (3) Any reason not to use 'Duration' for these fields?
> 
> Jojy Varghese wrote:
> 1) Absolutely I can. 
> 2) I wanted to reflect the semantics of the stats call. When you make a 
> stats call, the data you get is immutable. By forcing external const, it 
> would imply that the value is immutable.
> 3) Duration is a "period" ( i think) and the values here are absolute 
> values derived from ticks.
> 
> Joris Van Remoortere wrote:
> Regarding 3):
> Duration indeed represents an amount of time, rather than a specific 
> point in time. I believe this is what we intend to represent in this code as 
> well, right? The comments for your functions suggest an amount of time rather 
> than a point in time.

Regarding 1): I am working on another patch that addresses the containerizer 
(docker) using this API. I will also change the cpushare code along with that 
patch.


- Jojy


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


On July 7, 2015, 12:26 a.m., Jojy Varghese wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36106/
> ---
> 
> (Updated July 7, 2015, 12:26 a.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, Joris Van Remoortere, and 
> Timothy Chen.
> 
> 
> Bugs: MESOS-2961
> https://issues.apache.org/jira/browse/MESOS-2961
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> cgroups implementation does not have a cpuacct subsystem implementation as of
> today. Adding the implementation for stat function.
> 
> Changes:
>   - added Stats class to encapsulate cpuacct.stat data
>   - added implementation for cpuacct::stats
>   - added unit tests
> 
> Jira: MESOS-2961
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp 73b98317880eea3d6a2ba37ac56d1f7e3600ba94 
>   src/linux/cgroups.cpp 4c006d0c7382b940a83359d636c0d48952cdbb00 
>   src/tests/cgroups_tests.cpp 475f48a474eea708f98d8c0300862351a2d4379a 
> 
> Diff: https://reviews.apache.org/r/36106/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Jojy Varghese
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Adam B]

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


Question about MESOS-2832, but otherwise looks good.


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


Should executorEnvironment be static as well?



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


Filter? How does this cooperate with our fix for MESOS-2832 where 
`--executor_environment_variables` which lets an admin specify the environment 
variables that should be passed to an
executor? Should we filter even if that flag is set?


- Adam B


On July 7, 2015, 3:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 3:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Ian Downes]

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



docs/network-isolation.md (line 7)


"which require to listen" is incorrect

"a minimum choose a port in the Linux host ephemeral port range" h, 
what? I presume you really mean they should bind(0) and use what the kernel 
tells them to...

saying "specific ports" here will be interpreted as 'I want my container to 
listen to port 80', not that I want to bind some specific port that I can then 
discover.



docs/network-isolation.md (line 25)


This sentence is not clear, suggest rewording as "> 2.6.39 is advised for 
debugging purposes but is not required"



docs/network-isolation.md (line 27)


s/development package for libnl3/libnl3 development package/



docs/network-isolation.md (line 48)


what does "but only assigned ports will be allocated by the kernel mean"? 
this is not clear. Please also state here that the ephemeral range is split and 
assigned.



docs/network-isolation.md (line 52)


I think it's potentially confusing to call them short-lived (yes, I know 
that's historically how they've been used and how wikipedia categorizes them), 
since applications are free to bind to them as use them for the entirety of the 
job lifetime.



docs/network-isolation.md (line 60)


You can write directly to `/proc/sys/net/ipv4/ip_local_port_range`. Please 
state why the reboot is (strongly) advised.



docs/network-isolation.md (line 70)


Is it also recommended that ephemeral_ports per container be power-2 sized 
and aligned?

Can you be precise in the limit on the number of containers? Can you 
document here the master flag to set a global limit to the number of containers 
to each slave used as a workaround because ephemeral ports are not exposed to 
the master.

s/packets/packet/



docs/network-isolation.md (line 77)


Can you state and explain why there's no shaping/limit on ingress?

State explicitly that shaping delays traffic and will not drop packets.


- Ian Downes


On July 7, 2015, 2:54 p.m., Paul Brett wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36281/
> ---
> 
> (Updated July 7, 2015, 2:54 p.m.)
> 
> 
> Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Document per-container unique egress flows and network queueing statistics.
> 
> 
> Diffs
> -
> 
>   docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
>   docs/network-isolation.md PRE-CREATION 
>   docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 
> 
> Diff: https://reviews.apache.org/r/36281/diff/
> 
> 
> Testing
> ---
> 
> Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.
> 
> 
> Thanks,
> 
> Paul Brett
> 
>

Re: Review Request 36050: Added test authorizer module.

[Till Toenshoff]

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



src/tests/authorization_tests.cpp (line 62)


I think these make sense even though we should never reach this state 
according to the factory implementation.


- Till Toenshoff


On July 7, 2015, 7:34 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36050/
> ---
> 
> (Updated July 7, 2015, 7:34 a.m.)
> 
> 
> Review request for mesos, Adam B and Till Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds a test authorizer module.
> 
> Updates the authorization tests in order to perform typed tests on the 
> default authorizer implementation and on the test authorizer module.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/examples/test_authorizer_module.cpp PRE-CREATION 
>   src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 
>   src/tests/module.hpp 03756a2536ca3e662ba422e96d121a6c39bb8c84 
>   src/tests/module.cpp 61d4753f0f098005f56dd4a24984e30405c32558 
> 
> Diff: https://reviews.apache.org/r/36050/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36048: Updated authorizer to allow for modularized implementations.

[Till Toenshoff]

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



include/mesos/authorizer/authorizer.hpp (lines 39 - 40)


Let us also open a JIRA for this TODO to make sure it is 
1. really wanted
2. properly tracked

Then I would suggest to expand that comment a little;

```
// TODO(arojas): Remove once we have a module only initialization
// which would rely only module specific parameters as supplied via
// the modules JSON blob (see MESOS-).
```


- Till Toenshoff


On July 7, 2015, 2:09 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36048/
> ---
> 
> (Updated July 7, 2015, 2:09 p.m.)
> 
> 
> Review request for mesos, Adam B, Alexander Rukletsov, Kapil Arya, and Till 
> Toenshoff.
> 
> 
> Bugs: MESOS-2946
> https://issues.apache.org/jira/browse/MESOS-2946
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Splits and updates the original declaration of the `Authorizer` into its 
> interface and a default implementation, the `LocalAuthorizer`.
> 
> Following the pattern of the modularized `Authenticator`, it generates a 
> default constructor which is required when writing a `TYPED_TEST` in
> a follow up patch. Additionally, an initialize method has been added, needed 
> for passing in the current ACL definitions as provided via
> flags.
> 
> Other changes are just updates to allow for compilation.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/authorizer/authorizer.proto PRE-CREATION 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   include/mesos/type_utils.hpp e7bfe8ca60af945e76b5d85ab37cc97b17ff1b4a 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.hpp c039d9412780aa199db169b31991bf9f45b07d0f 
>   src/authorizer/authorizer.cpp 21e97e315478a4ca9442af83732665f85eb2f8fc 
>   src/common/parse.hpp 8d7ddd6819dad98cd96d5aaae8fe57caf1ee7098 
>   src/examples/persistent_volume_framework.cpp 
> c6d6ed337bfca91dc146cb31298cabebdbb13509 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/http.cpp 2be613b2e4c913b74c12d0d8f2d0e25da3cd3656 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/master/master.hpp fb4d6fac85e284987ec8fbf6949b5023875573fb 
>   src/master/master.cpp c5a4875f0d43c5091ae9a52c6b1d04105dfa3914 
>   src/tests/authorization_tests.cpp 99bb06c1ee73a90abaeeabb742e45aa188c21a87 
>   src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 
>   src/tests/mesos.hpp 9157ac079808d2686592e54ea26a26e6a0825ed3 
>   src/tests/mesos.cpp 5eab6dea6058865847425ab8d31708c92c6f098a 
> 
> Diff: https://reviews.apache.org/r/36048/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36246, 36275]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 9:02 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36275/
> ---
> 
> (Updated July 7, 2015, 9:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-3005
> https://issues.apache.org/jira/browse/MESOS-3005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We generate the certificate using the hostname associated with 
> INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This 
> way there is no inconsistency with the hostname of the certificate versus the 
> test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 869ed6572392e3cdcf0c0152bcca4b91130e3c04 
> 
> Diff: https://reviews.apache.org/r/36275/diff/
> 
> 
> Testing
> ---
> 
> make check.
> verifying on other dev's systems.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Marco Massenzio]

> On July 7, 2015, 9:32 p.m., Marco Massenzio wrote:
> > support/post-reviews.py, line 173
> > 
> >
> > note that here you will be printing the whole commit message, which in 
> > this case is not appropriate.
> > 
> > I would do instead:
> > ```
> > pos = message.find('Review:')
> > if pos != -1:
> > ...
> > print ...format(message[pos:])
> > ```
> > should make the error message clearer.

ok - to be a bit more pedantic:
```
if pos != -1:
...
if not match:
newline = max(message.find('\n', pos), len(message))
print ...format(message[pos:newline])
```


- Marco


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


On July 7, 2015, 3:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 7, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> (1) Handles the case where the `Review: ...` line isn't the last of the 
> commit message.
> 
> ```
> 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
> (6 seconds ago)
> 
> ReviewBoard URL must be the last line of the commit message!
> 
> Fixed post-reviews.py hanging bug.
> 
> Review: https://reviews.apache.org/r/35771
> 
> abcd
> ```
> 
> (2) Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36282: Remove os environment for docker executor enviornment setup.

[Benjamin Hindman]

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

Ship it!


Looks good but see the issue below about moving this inside the translation 
unit.


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


Let's not expose this in a public header since it's only used internally 
and instead let's just keep it as a static function inside of 
src/slave/containerizer/docker.cpp.



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


Newline above this please!



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


s/the os/the current process/


- Benjamin Hindman


On July 7, 2015, 10:10 p.m., Timothy Chen wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36282/
> ---
> 
> (Updated July 7, 2015, 10:10 p.m.)
> 
> 
> Review request for mesos, Adam B, Benjamin Hindman, and Joerg Schad.
> 
> 
> Bugs: MESOS-2996
> https://issues.apache.org/jira/browse/MESOS-2996
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Remove os environment when calling executorEnvironment when running docker 
> executors, since all the environment variables will be 
> passed into the container and causes bad behavior such as overriding hostname.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
>   src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 
> 
> Diff: https://reviews.apache.org/r/36282/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Timothy Chen
> 
>

Re: Review Request 36279: Doxygen-ification of comments in libprocess IO headers.

[Joseph Wu]

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

(Updated July 7, 2015, 3:14 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Joerg Schad.


Repository: mesos


Description
---

Doxygen-ification of comments in libprocess IO headers.


Diffs
-

  3rdparty/libprocess/include/process/io.hpp 
63887704743f53c6e49ab504f549419e1d5910ce 

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


Testing
---

`doxygen ../Doxyfile` and visually verified the HTML output.


Thanks,

Joseph Wu

Re: Review Request 36049: Added support for modularized Authorizer

[Till Toenshoff]

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


There are some nits and slight inconsistencies but overall I think we are in 
good shape here.


src/local/local.cpp (line 217)


Capital "The" please.



src/local/local.cpp (line 220)


Please start with a capital "Add" after that colon.



src/local/local.cpp (lines 227 - 228)


I think we should rephrase the message here;

```
"Could not create '" << flags.authorizers << "' authorizer: " << 
create.error()
```



src/local/local.cpp (line 232)


For validating the configuration, I always found it very helpful that we 
were showing the activated authenticator name/s in the master log -- hence I 
would like to suggest to do the same here as well;

```
LOG(INFO) << "Using '" << flags.authorizers << "' authorizer";
```



src/local/local.cpp (line 234)


I am assuming that the `LocalAuthorizer` should be considered unusable 
should its initialize function ever fail.

My most favored solution here would be to log the failure and make sure 
that `authorizer` remains unset so that we can operate without any 
authorization. That would be following the approach of the authenticator 
`initialize` failure handling.

```
 Try initialize = authorizer.get()->initialize(flags.acls.get());
 
 if (initialize.isError()) {
  // A failure to initialize the authorizer does lead to unusable 
authorization
  // but allows actions to skip authorization.
  LOG(WARNING) << "Authorization is disabled: Failed to initialize '"
   << flags.authorizers << "' authorizer: " << 
initialize.error();
  delete authorizer.get();
  authorizer = None();
}
```

Inherited from  
https://github.com/apache/mesos/blob/master/src/master/master.cpp#L484



src/master/flags.cpp (line 230)


s/authorizer/authorizers/



src/master/flags.cpp (line 231)


Lets make sure we match the flag name and also replace that "default" by 
the actual  implementation name.

```
  "Note that if the flag --authorizers is provided with a value different\n"
  "than '" + DEFAULT_AUTHORIZER + "', the ACLs contents will be ignored.\n"
  "\n"
```



src/master/flags.cpp (line 421)


s/authorizer/authorizers/

Please sure you check if you properly renamed that flag in all references. 
Thanks Alexander :)



src/master/flags.cpp (lines 423 - 424)


That looks like weird wrapping to me.



src/master/main.cpp (lines 301 - 317)


See my comments on local.cpp starting at line 217 ff. regarding this entire 
block.


- Till Toenshoff


On July 7, 2015, 7:34 a.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36049/
> ---
> 
> (Updated July 7, 2015, 7:34 a.m.)
> 
> 
> Review request for mesos, Adam B and Till Toenshoff.
> 
> 
> Bugs: MESOS-2947
> https://issues.apache.org/jira/browse/MESOS-2947
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Adds and integrates helper classes needed to support an `Authorizer` module. 
> Also adds a flag to the master, allowing the selection of an `Authorizer` 
> module.
> 
> 
> Diffs
> -
> 
>   include/mesos/authorizer/authorizer.hpp PRE-CREATION 
>   include/mesos/module/authorizer.hpp PRE-CREATION 
>   src/Makefile.am addb63f615f16ae6b25f745b2e79fd9fc0e27851 
>   src/authorizer/authorizer.cpp PRE-CREATION 
>   src/local/local.cpp 1953d84c75a83f4ace944d6243456235d8a193ff 
>   src/master/constants.hpp 7cec18b7fdfd3b96cde42a30d217c026b2695dce 
>   src/master/constants.cpp fbcae60c43e835f96ec061bd0e9f7961e31fc341 
>   src/master/flags.hpp f2cd19a6edfaa4e5bb31f024ef8d5beda32fbc2f 
>   src/master/flags.cpp 60ac64d98d53f74f904846b27a3833a7c44a9756 
>   src/master/main.cpp 2624b7ea4920a534c98f5dfbf9286c54c50f11a9 
>   src/module/manager.cpp 909ca56eea85d365cb9ebe1b3cce43051cabb670 
>   src/tests/cluster.hpp cfe7ef0c7a6dc62cddc3e5f5b5b28c8bcb2bed26 
> 
> Diff: https://reviews.apache.org/r/36049/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Benjamin Hindman]

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

Ship it!



3rdparty/libprocess/src/tests/ssl_tests.cpp (line 312)


I think a brief comment here and below on why we need to do a `bind` is 
helpful.


- Benjamin Hindman


On July 7, 2015, 9:02 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36275/
> ---
> 
> (Updated July 7, 2015, 9:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-3005
> https://issues.apache.org/jira/browse/MESOS-3005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We generate the certificate using the hostname associated with 
> INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This 
> way there is no inconsistency with the hostname of the certificate versus the 
> test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 869ed6572392e3cdcf0c0152bcca4b91130e3c04 
> 
> Diff: https://reviews.apache.org/r/36275/diff/
> 
> 
> Testing
> ---
> 
> make check.
> verifying on other dev's systems.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Benjamin Hindman]

> On July 7, 2015, 10:08 p.m., Benjamin Hindman wrote:
> >

Added comment and committed, thanks.


- Benjamin


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


On July 7, 2015, 9:02 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36275/
> ---
> 
> (Updated July 7, 2015, 9:02 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-3005
> https://issues.apache.org/jira/browse/MESOS-3005
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> We generate the certificate using the hostname associated with 
> INADDR_LOOPBACK and explicitly bind the test server on INADDR_LOOPBACK. This 
> way there is no inconsistency with the hostname of the certificate versus the 
> test.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/tests/ssl_tests.cpp 
> 869ed6572392e3cdcf0c0152bcca4b91130e3c04 
> 
> Diff: https://reviews.apache.org/r/36275/diff/
> 
> 
> Testing
> ---
> 
> make check.
> verifying on other dev's systems.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 36282: Remove os environment for docker executor enviornment setup.

[Timothy Chen]

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

Review request for mesos, Benjamin Hindman and Joerg Schad.


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


Repository: mesos


Description
---

Remove os environment when calling executorEnvironment when running docker 
executors, since all the environment variables will be 
passed into the container and causes bad behavior such as overriding hostname.


Diffs
-

  src/slave/containerizer/docker.hpp 646a277be21deaded47324bea5474bd68699f25f 
  src/slave/containerizer/docker.cpp 6eb1c84b1a6ef17c3e2ea2028e2f2d75110176ba 

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


Testing
---

make check


Thanks,

Timothy Chen

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Marco Massenzio]

> On July 7, 2015, 9:42 p.m., Vinod Kone wrote:
> > support/post-reviews.py, line 173
> > 
> >
> > Why 'format' instead of directly printing the message with a %s? Just 
> > curious.

It's a more portable way of printing - also, using `format()` allows for better 
ways of formatting messages.
Personally, now I'm always using (if I still have to support Python 2.7):
```
from future import __print_function__

...

print("foo bar {baz}".format(baz=something))
```
but that's pedantic me :)


- Marco


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


On July 7, 2015, 3:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 7, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> (1) Handles the case where the `Review: ...` line isn't the last of the 
> commit message.
> 
> ```
> 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
> (6 seconds ago)
> 
> ReviewBoard URL must be the last line of the commit message!
> 
> Fixed post-reviews.py hanging bug.
> 
> Review: https://reviews.apache.org/r/35771
> 
> abcd
> ```
> 
> (2) Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36246: SSL: Fix connection issue on OSX.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 7, 2015, 8:04 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36246/
> ---
> 
> (Updated July 7, 2015, 8:04 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> using the protocol based size for the `connect()` argument.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 1ef925cd8b6c6b07a64df9d409b9a25e9be539c9 
> 
> Diff: https://reviews.apache.org/r/36246/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 7, 2015, 5:21 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36267/
> ---
> 
> (Updated July 7, 2015, 5:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2943
> https://issues.apache.org/jira/browse/MESOS-2943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
> 
> Diff: https://reviews.apache.org/r/36267/diff/
> 
> 
> Testing
> ---
> 
> Only adding a comment.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 36281: Document per-container unique egress flows and network queueing statistics.

[Paul Brett]

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

Review request for mesos, Ian Downes, Jie Yu, and Cong Wang.


Repository: mesos


Description
---

Document per-container unique egress flows and network queueing statistics.


Diffs
-

  docs/home.md bc277910907c381c08835b6e9d485b27d6da5002 
  docs/network-isolation.md PRE-CREATION 
  docs/network-monitoring.md 8889fb165cc70bc382be0c99de8d7748328abf57 

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


Testing
---

Rendered at https://www.notehub.org/2015/7/7/network-isolation for review.


Thanks,

Paul Brett

Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 7, 2015, 8:59 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36277/
> ---
> 
> (Updated July 7, 2015, 8:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-3002
> https://issues.apache.org/jira/browse/MESOS-3002
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 2ef79eb9be76803d9a26223ee5763a582ca89736 
> 
> Diff: https://reviews.apache.org/r/36277/diff/
> 
> 
> Testing
> ---
> 
> build with network isolator configured.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Vinod Kone]

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



support/post-reviews.py (line 170)


Jie recently refactored this file to avoid using hard coded strings/urls in 
this file so that this script can be used with internal (org specific) repos 
and review servers.

Is there a way you can get the review board url from reviewboardrc isntead?



support/post-reviews.py (line 173)


Why 'format' instead of directly printing the message with a %s? Just 
curious.


- Vinod Kone


On July 7, 2015, 3:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 7, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> (1) Handles the case where the `Review: ...` line isn't the last of the 
> commit message.
> 
> ```
> 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
> (6 seconds ago)
> 
> ReviewBoard URL must be the last line of the commit message!
> 
> Fixed post-reviews.py hanging bug.
> 
> Review: https://reviews.apache.org/r/35771
> 
> abcd
> ```
> 
> (2) Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36277]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 8:59 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36277/
> ---
> 
> (Updated July 7, 2015, 8:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-3002
> https://issues.apache.org/jira/browse/MESOS-3002
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 2ef79eb9be76803d9a26223ee5763a582ca89736 
> 
> Diff: https://reviews.apache.org/r/36277/diff/
> 
> 
> Testing
> ---
> 
> build with network isolator configured.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Marco Massenzio]

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

Ship it!


Minor nit on error message - fix it and commit.
Thanks for fixing this!


support/post-reviews.py (line 173)


note that here you will be printing the whole commit message, which in this 
case is not appropriate.

I would do instead:
```
pos = message.find('Review:')
if pos != -1:
...
print ...format(message[pos:])
```
should make the error message clearer.


- Marco Massenzio


On July 7, 2015, 3:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 7, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> (1) Handles the case where the `Review: ...` line isn't the last of the 
> commit message.
> 
> ```
> 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
> (6 seconds ago)
> 
> ReviewBoard URL must be the last line of the commit message!
> 
> Fixed post-reviews.py hanging bug.
> 
> Review: https://reviews.apache.org/r/35771
> 
> abcd
> ```
> 
> (2) Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.

[Adam B]

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

Ship it!


Ship It!

- Adam B


On July 7, 2015, 1:59 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36277/
> ---
> 
> (Updated July 7, 2015, 1:59 p.m.)
> 
> 
> Review request for mesos and Benjamin Hindman.
> 
> 
> Bugs: MESOS-3002
> https://issues.apache.org/jira/browse/MESOS-3002
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/isolators/network/port_mapping.cpp 
> 2ef79eb9be76803d9a26223ee5763a582ca89736 
> 
> Diff: https://reviews.apache.org/r/36277/diff/
> 
> 
> Testing
> ---
> 
> build with network isolator configured.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 34141: AppC provsioning backend.

[Timothy Chen]

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



src/slave/containerizer/provisioners/appc/backend.cpp (line 139)


Should we try to keep the logging of each image provisioning to a seperate 
log file in the image folder, instead of just writing out to stdout/stderr?


- Timothy Chen


On July 7, 2015, 7:43 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/34141/
> ---
> 
> (Updated July 7, 2015, 7:43 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Simple copy backend that forms the rootfs by copying each layer.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
>   src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
>   src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
>   src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
> 
> Diff: https://reviews.apache.org/r/34141/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Joris Van Remoortere]

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

(Updated July 7, 2015, 9:02 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park.


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


Repository: mesos


Description
---

We generate the certificate using the hostname associated with INADDR_LOOPBACK 
and explicitly bind the test server on INADDR_LOOPBACK. This way there is no 
inconsistency with the hostname of the certificate versus the test.


Diffs
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
869ed6572392e3cdcf0c0152bcca4b91130e3c04 

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


Testing
---

make check.
verifying on other dev's systems.


Thanks,

Joris Van Remoortere

Review Request 36277: MESOS-3002: Fix getOrElse compilation error for network isolator.

[Joris Van Remoortere]

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

Review request for mesos and Benjamin Hindman.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  src/slave/containerizer/isolators/network/port_mapping.cpp 
2ef79eb9be76803d9a26223ee5763a582ca89736 

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


Testing
---

build with network isolator configured.


Thanks,

Joris Van Remoortere

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36269]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Joris Van Remoortere]

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

(Updated July 7, 2015, 8:04 p.m.)


Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park.


Changes
---

rebased.


Repository: mesos


Description
---

We generate the certificate using the hostname associated with INADDR_LOOPBACK 
and explicitly bind the test server on INADDR_LOOPBACK. This way there is no 
inconsistency with the hostname of the certificate versus the test.


Diffs (updated)
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
869ed6572392e3cdcf0c0152bcca4b91130e3c04 

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


Testing
---

make check.
verifying on other dev's systems.


Thanks,

Joris Van Remoortere

Re: Review Request 36246: SSL: Fix connection issue on OSX.

[Joris Van Remoortere]

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

(Updated July 7, 2015, 8:04 p.m.)


Review request for mesos, Benjamin Hindman and Michael Park.


Changes
---

rebased.


Repository: mesos


Description
---

using the protocol based size for the `connect()` argument.


Diffs (updated)
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
1ef925cd8b6c6b07a64df9d409b9a25e9be539c9 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35986]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 7:07 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35986/
> ---
> 
> (Updated July 7, 2015, 7:07 p.m.)
> 
> 
> Review request for mesos, Adam B and Isabel Jimenez.
> 
> 
> Bugs: MESOS-2868
> https://issues.apache.org/jira/browse/MESOS-2868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow slave attributes flag take a value with ':'.
> 
> 
> Diffs
> -
> 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 
> 
> Diff: https://reviews.apache.org/r/35986/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Review Request 36275: MESOS-3005: Fix SSL test hostname dependency.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park.


Repository: mesos


Description
---

We generate the certificate using the hostname associated with INADDR_LOOPBACK 
and explicitly bind the test server on INADDR_LOOPBACK. This way there is no 
inconsistency with the hostname of the certificate versus the test.


Diffs
-

  3rdparty/libprocess/src/tests/ssl_tests.cpp 
869ed6572392e3cdcf0c0152bcca4b91130e3c04 

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


Testing
---

make check.
verifying on other dev's systems.


Thanks,

Joris Van Remoortere

Re: Review Request 36246: SSL: Fix connection issue on OSX.

[Joris Van Remoortere]

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

(Updated July 7, 2015, 7:51 p.m.)


Review request for mesos, Benjamin Hindman and Michael Park.


Repository: mesos


Description
---

using the protocol based size for the `connect()` argument.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 

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


Testing
---


Thanks,

Joris Van Remoortere

Re: Review Request 34142: AppC provisioner.

[Ian Downes]

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

(Updated July 7, 2015, 12:43 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Repository: mesos


Description
---

Discovers image(s), fetches to the image store, then provisions using
a backend.


Diffs
-

  include/mesos/mesos.proto 8df1211165169c9595e0e6e85b5ddc404345ff70 
  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/mesos/containerizer.cpp 
8c102fb7d1f79ee768cb06de3a976ea12f958712 
  src/slave/containerizer/provisioners/appc.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34141: AppC provsioning backend.

[Ian Downes]

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

(Updated July 7, 2015, 12:43 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

Simple copy backend that forms the rootfs by copying each layer.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/backend.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/backend.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34139: AppC image discovery.

[Ian Downes]

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

(Updated July 7, 2015, 12:42 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

Local and simple discovery only.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/discovery.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/discovery.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34140: AppC image store

[Ian Downes]

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

(Updated July 7, 2015, 12:43 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

Images are fetched into the store (after discovery). Stored images are
currently kept indefinitely.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/store.hpp PRE-CREATION 
  src/slave/containerizer/provisioners/appc/store.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34138: AppC hash computation.

[Ian Downes]

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

(Updated July 7, 2015, 12:42 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

AppC hash computation.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/provisioners/appc/hash.hpp PRE-CREATION 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 34137: Add support for container image provisioners.

[Ian Downes]

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

(Updated July 7, 2015, 12:42 p.m.)


Review request for mesos, Chi Zhang, Paul Brett, Timothy Chen, and Vinod Kone.


Changes
---

Updated dependencies.


Repository: mesos


Description
---

The MesosContainerizer can optionally provision a root filesystem for the 
containers.


Diffs
-

  include/mesos/slave/isolator.hpp 18edc030367e42240090f2f3dbc92aec7a4c6234 
  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/slave/containerizer/mesos/containerizer.hpp 
3ac2387eabded61c897a5016655aae10cd1bca91 
  src/slave/containerizer/mesos/containerizer.cpp 
8c102fb7d1f79ee768cb06de3a976ea12f958712 
  src/slave/containerizer/provisioner.hpp PRE-CREATION 
  src/slave/containerizer/provisioner.cpp PRE-CREATION 
  src/slave/flags.hpp 7634e368c72e83932dcd992d78eaca146326606b 
  src/slave/flags.cpp cbf431eb0627bdaf07241cc0fc4630df06fb20e2 
  src/slave/paths.hpp 00476f107ed8233123a6eab0925c9f28aac0a86f 
  src/slave/paths.cpp 0741616b656e947cb460dd6ee6a9a4852be001c2 
  src/slave/state.hpp fed4b7ecf9572a8dbb1a99dbb1769d3e55ef7bc5 
  src/slave/state.cpp 8eda22a550e5add0f84c46cc2ed762b006c0dcec 
  src/tests/containerizer_tests.cpp 0cdb2d2a3f19a4835e85c6b040759019b03f051e 

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


Testing
---


Thanks,

Ian Downes

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Vinod Kone]

> On July 7, 2015, 6:40 p.m., Vinod Kone wrote:
> > CHANGELOG, line 337
> > 
> >
> > I would just put this under deprecations section.
> > 
> > Also, mind updating MESOS-2058 in deprecation section to do 
> > s/deprecate/remove/ because its been removed.
> 
> Jiang Yan Xu wrote:
> Adam is against putting MESOS-2640 in 0.23.0 because it's committed after 
> rc1 is cut. This review doesn't even need to land now.
> 
> s/deprecate/remove/ on MESOS-2058 can be done separately.

SG. Yea I wanted a WIP 24.0 CHANGELOG too when I originially made that comment 
on the older review. Forgot about that.


- Vinod


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


On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

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

(Updated July 7, 2015, 7:07 p.m.)


Review request for mesos, Adam B and Isabel Jimenez.


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


Repository: mesos


Description
---

Allow slave attributes flag take a value with ':'.


Diffs (updated)
-

  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 

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


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.

[Artem Harutyunyan]

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

Ship it!


Ship It!

- Artem Harutyunyan


On July 7, 2015, 10:21 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36267/
> ---
> 
> (Updated July 7, 2015, 10:21 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2943
> https://issues.apache.org/jira/browse/MESOS-2943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
> 
> Diff: https://reviews.apache.org/r/36267/diff/
> 
> 
> Testing
> ---
> 
> Only adding a comment.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

> On July 7, 2015, 11:40 a.m., Vinod Kone wrote:
> > CHANGELOG, line 337
> > 
> >
> > I would just put this under deprecations section.
> > 
> > Also, mind updating MESOS-2058 in deprecation section to do 
> > s/deprecate/remove/ because its been removed.

Adam is against putting MESOS-2640 in 0.23.0 because it's committed after rc1 
is cut. This review doesn't even need to land now.

s/deprecate/remove/ on MESOS-2058 can be done separately.


- Jiang Yan


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


On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.

[Mesos ReviewBot]

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


Bad patch!

Reviews applied: [32891, 31444]

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

Error:
 2015-07-07 18:42:10 URL:https://reviews.apache.org/r/31444/diff/raw/ 
[12941/12941] -> "31444.patch" [1]
error: patch failed: src/slave/containerizer/mesos/launch.cpp:20
error: src/slave/containerizer/mesos/launch.cpp: patch does not apply
Failed to apply patch

- Mesos ReviewBot


On July 7, 2015, 6:33 p.m., Ian Downes wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/31444/
> ---
> 
> (Updated July 7, 2015, 6:33 p.m.)
> 
> 
> Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, 
> and James Peach.
> 
> 
> Bugs: MESOS-2350
> https://issues.apache.org/jira/browse/MESOS-2350
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Optionally take a path that the launch helper should chroot to before 
> exec'ing the executor. It is assumed that the work directory is mounted to 
> the appropriate location under the chroot. In particular, the path to the 
> executor must be relative to the chroot.
> 
> Configuration that should be private to the chroot is done during the launch, 
> e.g. mounting proc and statically configuring basic devices. It is assumed 
> that other configuration, e.g., preparing the image, mounting in volumes or 
> persistent resources, is done by the caller.
> 
> Mounts can be made to the chroot (e.g., updating the volumes or persistent 
> resources) and they will propagate in to the container but mounts made inside 
> the container will not propagate out to the host.
> 
> It currently assumes that at least {{chroot}}/tmp is writeable and that mount 
> points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.
> 
> This is specific to Linux.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
>   src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263 
>   src/slave/containerizer/mesos/launch.hpp 
> 7c8b535746b5ce9add00afef86fdb6faefb5620e 
>   src/slave/containerizer/mesos/launch.cpp 
> 2f2d60e2011f60ec711d3b29fd2c157e30c83c34 
>   src/tests/launch_tests.cpp PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/31444/diff/
> 
> 
> Testing
> ---
> 
> Manual testing only so far. This is harder to automate because we need a 
> self-contained chroot to execute something in... Suggestions welcome.
> 
> 
> Thanks,
> 
> Ian Downes
> 
>

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Vinod Kone]

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



CHANGELOG (line 337)


I would just put this under deprecations section.

Also, mind updating MESOS-2058 in deprecation section to do 
s/deprecate/remove/ because its been removed.


- Vinod Kone


On July 7, 2015, 5:59 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 5:59 p.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

> On July 7, 2015, 11:35 a.m., Adam B wrote:
> > CHANGELOG, line 337
> > 
> >
> > Do you actually want/need MESOS-2640 to go into 0.23.0?
> > If so, MESOS-2640 should have its Target Version and Fix Version set to 
> > 0.23.0 and the release manager (me) should be notified. Since we've already 
> > cut rc1, we are only cherry-picking select patches into 0.23.0-rc2. Does 
> > this need to be one?
> > 
> > If not, create a new `(WIP) Release Notes - Mesos - Version 0.24.0` 
> > section at the top to place your Cleanup ticket under.

I see. ```(WIP) Release Notes - Mesos - Version 0.24.0``` sounds good.


- Jiang Yan


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


On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Adam B]

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


Do you actually want/need MESOS-2640 to go into 0.23.0?


CHANGELOG (line 337)


Do you actually want/need MESOS-2640 to go into 0.23.0?
If so, MESOS-2640 should have its Target Version and Fix Version set to 
0.23.0 and the release manager (me) should be notified. Since we've already cut 
rc1, we are only cherry-picking select patches into 0.23.0-rc2. Does this need 
to be one?

If not, create a new `(WIP) Release Notes - Mesos - Version 0.24.0` section 
at the top to place your Cleanup ticket under.


- Adam B


On July 7, 2015, 10:59 a.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36269/
> ---
> 
> (Updated July 7, 2015, 10:59 a.m.)
> 
> 
> Review request for mesos and Adam B.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Per Vinod's comment on /r/36005
> 
> The `**Cleanup` section makes sense?
> 
> 
> Diffs
> -
> 
>   CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 
> 
> Diff: https://reviews.apache.org/r/36269/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

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

(Updated July 7, 2015, 6:33 p.m.)


Review request for mesos, Adam B and Isabel Jimenez.


Changes
---

Sorry for so much noisy...


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


Repository: mesos


Description
---

Allow slave attributes flag take a value with ':'.


Diffs (updated)
-

  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 

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


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 31444: Support chrooting in MesosContainerizer launch helper.

[Ian Downes]

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

(Updated July 7, 2015, 11:33 a.m.)


Review request for mesos, Chi Zhang, Dominic Hamon, Jay Buffington, Jie Yu, and 
James Peach.


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


Repository: mesos


Description
---

Optionally take a path that the launch helper should chroot to before exec'ing 
the executor. It is assumed that the work directory is mounted to the 
appropriate location under the chroot. In particular, the path to the executor 
must be relative to the chroot.

Configuration that should be private to the chroot is done during the launch, 
e.g. mounting proc and statically configuring basic devices. It is assumed that 
other configuration, e.g., preparing the image, mounting in volumes or 
persistent resources, is done by the caller.

Mounts can be made to the chroot (e.g., updating the volumes or persistent 
resources) and they will propagate in to the container but mounts made inside 
the container will not propagate out to the host.

It currently assumes that at least {{chroot}}/tmp is writeable and that mount 
points {{chroot}}/{tmp,dev,proc,sys} exist in the chroot.

This is specific to Linux.


Diffs
-

  src/Makefile.am e7de0f3d1a5efeaef47d5074defe3b40db94f573 
  src/linux/fs.cpp 568565f878b34708170a886dc4d62849aa01f263 
  src/slave/containerizer/mesos/launch.hpp 
7c8b535746b5ce9add00afef86fdb6faefb5620e 
  src/slave/containerizer/mesos/launch.cpp 
2f2d60e2011f60ec711d3b29fd2c157e30c83c34 
  src/tests/launch_tests.cpp PRE-CREATION 

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


Testing
---

Manual testing only so far. This is harder to automate because we need a 
self-contained chroot to execute something in... Suggestions welcome.


Thanks,

Ian Downes

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

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

(Updated July 7, 2015, 6:26 p.m.)


Review request for mesos, Adam B and Isabel Jimenez.


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


Repository: mesos


Description
---

Allow slave attributes flag take a value with ':'.


Diffs (updated)
-

  Makefile.am f8e958d376efa55ace7c5727a31b8e747403641e 
  configure.ac 546c9bbf775a4ef481fafb3a58c85c6d80e19500 
  docs/ec2-scripts.md PRE-CREATION 
  docs/persistent-volume.md e3dfe6d785bbfda2489ba5d71cad043f290fb23a 
  docs/tools.md 09619f2c162b1765e7718fc314e77b0f77148b97 
  docs/using-the-mesos-submit-tool.md PRE-CREATION 
  ec2/Makefile.am PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/masters PRE-CREATION 
  ec2/deploy.amazon64-old/root/ephemeral-hdfs/conf/slaves PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/cluster-url PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/copy-dir PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/create-swap PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/core-site.xml 
PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh 
PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml 
PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/haproxy+apache/haproxy.config.template 
PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/Capfile PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/masters PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/mesos-daemon PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/redeploy-mesos PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/setup PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/setup-slave PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/setup-torque PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/slaves PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/ssh-no-keychecking PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/start-hypertable PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/start-mesos PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/stop-hypertable PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/stop-mesos PRE-CREATION 
  ec2/deploy.amazon64-old/root/mesos-ec2/zoo PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/core-site.xml PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hadoop-env.sh PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/hdfs-site.xml PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/mapred-site.xml 
PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/masters PRE-CREATION 
  ec2/deploy.amazon64-old/root/persistent-hdfs/conf/slaves PRE-CREATION 
  ec2/deploy.amazon64-old/root/spark/conf/spark-env.sh PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/core-site.xml PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hadoop-env.sh PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/hdfs-site.xml PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/mapred-site.xml PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/masters PRE-CREATION 
  ec2/deploy.amazon64/root/ephemeral-hdfs/conf/slaves PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/cluster-url PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/copy-dir PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/create-swap PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/core-site.xml 
PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/hadoop-env.sh 
PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/hadoop-framework-conf/mapred-site.xml 
PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/haproxy+apache/haproxy.config.template 
PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/hypertable/Capfile PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/hypertable/hypertable.cfg PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/masters PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/mesos-daemon PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/redeploy-mesos PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/setup PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/setup-slave PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/setup-torque PRE-CREATION 
  ec2/deploy.amazon64/root/mesos-ec2/slave

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

> On July 7, 2015, 1:30 a.m., Adam B wrote:
> > src/common/attributes.cpp, lines 150-152
> > 
> >
> > There's a subtle difference in behavior between strings::tokenize and 
> > strings::split. For tokenize, "Empty tokens will not be included in the 
> > result." whereas for split, "Empty tokens are allowed in the result." so 
> > you need to test not only that `pairs.size() == 2`, but also that 
> > `!pairs[0].empty()` and `!pairs[1].empty()`.
> > Let's create unit tests for handling `":foo"` and `"foo:"`.

Hi, @adam-mesos, could not add test unit tests for handling ":foo" and "foo:". 
Because use LOG(FATAL) in parse.


- haosdent


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


On July 7, 2015, 5:55 p.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35986/
> ---
> 
> (Updated July 7, 2015, 5:55 p.m.)
> 
> 
> Review request for mesos, Adam B and Isabel Jimenez.
> 
> 
> Bugs: MESOS-2868
> https://issues.apache.org/jira/browse/MESOS-2868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow slave attributes flag take a value with ':'.
> 
> 
> Diffs
> -
> 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 
> 
> Diff: https://reviews.apache.org/r/35986/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[Adam B]

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

Ship it!


Looks great! Fix the log message and I'll commit it.


src/common/attributes.cpp (line 154)


`pairs[0]` is not accurate if it was pairs[1] that was empty, and printing 
an empty string is pointless anyway.
How about `LOG(FATAL) << "Invalid attribute key:value pair '" << tokens[i] 
<< "'";` so that the error shows the leading/trailing ':'



src/tests/attributes_tests.cpp (lines 55 - 63)


These could be combined easily enough.


- Adam B


On July 7, 2015, 10:55 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35986/
> ---
> 
> (Updated July 7, 2015, 10:55 a.m.)
> 
> 
> Review request for mesos, Adam B and Isabel Jimenez.
> 
> 
> Bugs: MESOS-2868
> https://issues.apache.org/jira/browse/MESOS-2868
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allow slave attributes flag take a value with ':'.
> 
> 
> Diffs
> -
> 
>   src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
>   src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 
> 
> Diff: https://reviews.apache.org/r/35986/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 36267: MESOS-2943: Add comment for explicit return type.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36267]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 5:21 p.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36267/
> ---
> 
> (Updated July 7, 2015, 5:21 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael 
> Park.
> 
> 
> Bugs: MESOS-2943
> https://issues.apache.org/jira/browse/MESOS-2943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
> 
> Diff: https://reviews.apache.org/r/36267/diff/
> 
> 
> Testing
> ---
> 
> Only adding a comment.
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Review Request 36269: Update CHANGELOG to reflect obsolete code cleanup

[Jiang Yan Xu]

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

Review request for mesos and Adam B.


Repository: mesos


Description
---

Per Vinod's comment on /r/36005

The `**Cleanup` section makes sense?


Diffs
-

  CHANGELOG 433924a0e614f061c448a8e85d0a2825567150dc 

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


Testing
---


Thanks,

Jiang Yan Xu

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

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

(Updated July 7, 2015, 5:55 p.m.)


Review request for mesos, Adam B and Isabel Jimenez.


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


Repository: mesos


Description
---

Allow slave attributes flag take a value with ':'.


Diffs (updated)
-

  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 

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


Testing
---

make check


Thanks,

haosdent huang

Re: Review Request 35981: Added persistent volume user guide.

[Jie Yu]

> On June 30, 2015, 12:14 a.m., Adam B wrote:
> > docs/persistent-volume.md, line 56
> > 
> >
> > Can a volume/reservation created by one framework principal only be 
> > destroyed/unreserved by a framework scheduler using the same principal? If 
> > the principal is different, then the operation would fail, even if the role 
> > or even frameworkId is identical?

We only look at the principal of an framework or an operator. The role does not 
matter because an operator does not have a role. The frameworkId is also 
irrelavent because framework B can unreserve/destroy a reservation/volume 
created by framework B as long as ACL allows it.


> On June 30, 2015, 12:14 a.m., Adam B wrote:
> > docs/persistent-volume.md, lines 67-69
> > 
> >
> > You might need a blank line preceding this, and/or some indentation for 
> > it to show up as a numbered list. Doesn't render properly as is.
> > 
> > Unique per role per slave? So I could use the same volumeId for related 
> > volume instances (e.g. my 'logging' or 'data' volumes), up to a max of one 
> > per slave? But if I happen to launch another executor/task on a slave where 
> > one of those volumes is already created/mounted, then I have to rename it 
> > ('data2')?

Yes, this is in the design doc. Persistent volumes can be shared within a role, 
so it has to be unique per role.


> On June 30, 2015, 12:14 a.m., Adam B wrote:
> > docs/persistent-volume.md, lines 120-122
> > 
> >
> > Any chance it could return the host_path too? Or do I have to go 
> > digging through slave logs to figure out where my volume was created?

Why the framework wants to know that? This is similar to sandbox. Do we have a 
way to let the framework know about the location of the sandbox?


- Jie


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


On July 7, 2015, 12:34 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35981/
> ---
> 
> (Updated July 7, 2015, 12:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2405
> https://issues.apache.org/jira/browse/MESOS-2405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Github rendered version is available [here]( 
> https://github.com/mesosphere/mesos/blob/user-docs/docs/persistent-volume.md)
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35981/diff/
> 
> 
> Testing
> ---
> 
> Documentation.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

[Bartek Plotka]

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

(Updated July 7, 2015, 5:50 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
and Vinod Kone.


Changes
---

Changed totalResources check.


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


Repository: mesos


Description
---

Pass slave's total resources to the ResourceEstimator and QoSController via 
Slave::usage().


Diffs (updated)
-

  include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
  src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 

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


Testing
---

make check.


Thanks,

Bartek Plotka

Re: Review Request 36193: Improved Doxygen-Styleguide.

[Joerg Schad]

> On July 6, 2015, 6:47 p.m., Joseph Wu wrote:
> > docs/mesos-doxygen-style-guide.md, line 17
> > 
> >
> > It might be worthwhile to emphasize that markdown syntax can/should be 
> > used in with the comment blocks.  I think Javadocs generally use plain HTML 
> > for links, tables, etc.

Same as above.


- Joerg


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


On July 6, 2015, 9:01 a.m., Joerg Schad wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36193/
> ---
> 
> (Updated July 6, 2015, 9:01 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Bernd Mathiske, and Joseph Wu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Improved Doxygen-Styleguide for clarifying discussions arising from 
> https://reviews.apache.org/r/36141/.
> 
> 
> Diffs
> -
> 
>   docs/mesos-doxygen-style-guide.md 72156c72fc40f72740e57d47d0436c60f6d05bd7 
> 
> Diff: https://reviews.apache.org/r/36193/diff/
> 
> 
> Testing
> ---
> 
> Checked rendered markdown.
> 
> 
> Thanks,
> 
> Joerg Schad
> 
>

Re: Review Request 35986: Allow slave attributes flag take a value with ':'.

[haosdent huang]

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

(Updated July 7, 2015, 5:46 p.m.)


Review request for mesos, Adam B and Isabel Jimenez.


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


Repository: mesos


Description
---

Allow slave attributes flag take a value with ':'.


Diffs (updated)
-

  src/common/attributes.cpp aab114e1a5932e3f218b850e1afc7f2ef0f10e21 
  src/tests/attributes_tests.cpp 2e10eaf3c33e418603ea19090bb0bf9179af71f7 

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


Testing
---

make check


Thanks,

haosdent huang

Review Request 36267: MESOS-2943: Add comment for explicit return type.

[Joris Van Remoortere]

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

Review request for mesos, Benjamin Hindman, Artem Harutyunyan, and Michael Park.


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


Repository: mesos


Description
---

See summary.


Diffs
-

  3rdparty/libprocess/src/libevent_ssl_socket.cpp 
9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 

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


Testing
---

Only adding a comment.


Thanks,

Joris Van Remoortere

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

[Jie Yu]

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

Ship it!


LGTM! Could you please add a follow up patch to test this code path?


src/slave/slave.cpp (lines 4382 - 4383)


You should just add a CHECK here:
```
CHECK_SOME(totalResources)
  << "Failed to apply checkpointed resource "
  << checkpointedResources << " to slave's resources "
  << info->resources();

usage->mutable_total()->CopyFrom(totalResources.get());
```


- Jie Yu


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> ---
> 
> (Updated July 7, 2015, 4:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2933
> https://issues.apache.org/jira/browse/MESOS-2933
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass slave's total resources to the ResourceEstimator and QoSController via 
> Slave::usage().
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 
> 
> Diff: https://reviews.apache.org/r/36204/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [36204]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> ---
> 
> (Updated July 7, 2015, 4:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2933
> https://issues.apache.org/jira/browse/MESOS-2933
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass slave's total resources to the ResourceEstimator and QoSController via 
> Slave::usage().
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 
> 
> Diff: https://reviews.apache.org/r/36204/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

[Bartek Plotka]

> On July 6, 2015, 5:50 p.m., Jie Yu wrote:
> > src/slave/slave.cpp, line 4379
> > 
> >
> > Per my comments above, the logic here needs to be adjusted. We need to 
> > apply checkpointed resource to info.resources() here.
> > 
> > Please refer to the following code:
> > https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883

Thanks Jie for your feedback! 

I addressed your issue, however I'm just curious if there is any possibility to 
get an error (here in *::usage()*) from *applyCheckpointedResources* function? 
(https://github.com/apache/mesos/blob/master/src/slave/slave.cpp#L3883). IMO 
here we are quite sure that *checkpointedResources* are proper. If yes, would 
it make sense to just put *info.resources()* into *usage->total* in case we 
cannot apply checkpointed resources?


- Bartek


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


On July 7, 2015, 4:20 p.m., Bartek Plotka wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36204/
> ---
> 
> (Updated July 7, 2015, 4:20 p.m.)
> 
> 
> Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
> and Vinod Kone.
> 
> 
> Bugs: MESOS-2933
> https://issues.apache.org/jira/browse/MESOS-2933
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Pass slave's total resources to the ResourceEstimator and QoSController via 
> Slave::usage().
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
>   src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 
> 
> Diff: https://reviews.apache.org/r/36204/diff/
> 
> 
> Testing
> ---
> 
> make check.
> 
> 
> Thanks,
> 
> Bartek Plotka
> 
>

Re: Review Request 36204: Pass slave's total resources in ResourceUsage.

[Bartek Plotka]

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

(Updated July 7, 2015, 4:20 p.m.)


Review request for mesos, Ben Mahler, Jie Yu, Niklas Nielsen, Szymon Konefal, 
and Vinod Kone.


Changes
---

Slave's total resources now includes checkpointed dynamic reservations and 
persistent volumes.


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


Repository: mesos


Description
---

Pass slave's total resources to the ResourceEstimator and QoSController via 
Slave::usage().


Diffs (updated)
-

  include/mesos/mesos.proto 3dd4a5b7a4b3bc56bdc690d6adf05f88c0d28273 
  src/slave/slave.cpp 008170f4ce7b97eda43b50b48083b32ae65925c0 

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


Testing
---

make check.


Thanks,

Bartek Plotka

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [35777]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 3:19 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35777/
> ---
> 
> (Updated July 7, 2015, 3:19 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till 
> Toenshoff.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> (1) Handles the case where the `Review: ...` line isn't the last of the 
> commit message.
> 
> ```
> 01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
> (6 seconds ago)
> 
> ReviewBoard URL must be the last line of the commit message!
> 
> Fixed post-reviews.py hanging bug.
> 
> Review: https://reviews.apache.org/r/35771
> 
> abcd
> ```
> 
> (2) Handles the case where the ReviewBoard URL is invalid.
> 
> ```
> af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
> (29 seconds ago)
> 
> Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
> ```
> 
> 
> Diffs
> -
> 
>   support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 
> 
> Diff: https://reviews.apache.org/r/35777/diff/
> 
> 
> Testing
> ---
> 
> Modified the commit message with `git rebase` and ran 
> `./support/post-reviews.py` to observe the above error messages.
> Used the valid commit message for this patch and created it by running 
> `./support/post-reviews.py`.
> 
> 
> Thanks,
> 
> Michael Park
> 
>

Re: Review Request 35777: Made post-reviews.py handle bad (or not) ReviewBoard URLs.

[Michael Park]

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

(Updated July 7, 2015, 3:19 p.m.)


Review request for mesos, Benjamin Hindman, Marco Massenzio, and Till Toenshoff.


Changes
---

Addressed Marco's comment.


Repository: mesos


Description
---

(1) Handles the case where the `Review: ...` line isn't the last of the commit 
message.

```
01596ce802669e9dac7dd24193f041cef3354830 - Fixed post-reviews.py hanging bug. 
(6 seconds ago)

ReviewBoard URL must be the last line of the commit message!

Fixed post-reviews.py hanging bug.

Review: https://reviews.apache.org/r/35771

abcd
```

(2) Handles the case where the ReviewBoard URL is invalid.

```
af081a07234794f60a2575e0383ce537d7111c18 - Fixed post-reviews.py hanging bug. 
(29 seconds ago)

Invalid ReviewBoard URL : 'https://reviews.apache.org/r/35771 abcd'
```


Diffs (updated)
-

  support/post-reviews.py b04e26b85e6b056098c15078f9b31dc352682351 

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


Testing
---

Modified the commit message with `git rebase` and ran 
`./support/post-reviews.py` to observe the above error messages.
Used the valid commit message for this patch and created it by running 
`./support/post-reviews.py`.


Thanks,

Michael Park

Re: Review Request 36245: Fix compilation error for clang-3.5 type deduction error.

[Benjamin Hindman]

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

Ship it!


Ship It!

- Benjamin Hindman


On July 7, 2015, 5:16 a.m., Joris Van Remoortere wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36245/
> ---
> 
> (Updated July 7, 2015, 5:16 a.m.)
> 
> 
> Review request for mesos, Benjamin Hindman and Michael Park.
> 
> 
> Bugs: MESOS-2943
> https://issues.apache.org/jira/browse/MESOS-2943
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> While we figure out how to avoid this bug in clang-3.5, we can allow people 
> to compile by explicitly specifying the return type of the lambda.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/libevent_ssl_socket.cpp 
> 9424dd421b290bec25b4a7c4dc0071ffef6fdc5e 
> 
> Diff: https://reviews.apache.org/r/36245/diff/
> 
> 
> Testing
> ---
> 
> make check 3rdparty using clang-3.5
> 
> 
> Thanks,
> 
> Joris Van Remoortere
> 
>

Re: Review Request 35981: Added persistent volume user guide.

[Mesos ReviewBot]

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


Patch looks great!

Reviews applied: [32982, 35981]

All tests passed.

- Mesos ReviewBot


On July 7, 2015, 12:34 p.m., Michael Park wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/35981/
> ---
> 
> (Updated July 7, 2015, 12:34 p.m.)
> 
> 
> Review request for mesos, Adam B, Jie Yu, and Timothy Chen.
> 
> 
> Bugs: MESOS-2405
> https://issues.apache.org/jira/browse/MESOS-2405
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The Github rendered version is available [here]( 
> https://github.com/mesosphere/mesos/blob/user-docs/docs/persistent-volume.md)
> 
> 
> Diffs
> -
> 
>   docs/persistent-volume.md PRE-CREATION 
> 
> Diff: https://reviews.apache.org/r/35981/diff/
> 
> 
> Testing
> ---
> 
> Documentation.
> 
> 
> Thanks,
> 
> Michael Park
> 
>