Re: Review Request 54717: Modified isolator for dynamic addition/deletion of CNI networks.

[Avinash sridharan]

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

(Updated Dec. 18, 2016, 2:23 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Qian's comments.


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


Repository: mesos


Description
---

If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.

During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

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


Testing
---

make check

Manual testing with addition of a non-existent CNI configuration.


Thanks,

Avinash sridharan

Re: Review Request 54717: Modified isolator for dynamic addition/deletion of CNI networks.

[Avinash sridharan]

> On Dec. 17, 2016, 3:28 p.m., Qian Zhang wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 1159-1170
> > 
> >
> > Just curious in which case `getNetwork` will succeeds but 
> > `getNetworkConfigJSON()` will fail?

One case I can think of is if the operator ends up removing the file between a 
call to `getNetwork` and `getNetworkConfigJSON` . Highly unlikely, but possible.


- Avinash


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


On Dec. 17, 2016, 8:19 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> ---
> 
> (Updated Dec. 17, 2016, 8:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
> 
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54717/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual testing with addition of a non-existent CNI configuration.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 54716: Modified the initialization logic for `network/cni` isolator.

[Avinash sridharan]

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

(Updated Dec. 18, 2016, 2:03 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Qian's comments.


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


Repository: mesos


Description
---

Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI  plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

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


Testing
---

sudo make check --gtest_filter=*CNI*


Thanks,

Avinash sridharan

Re: Review Request 54839: Updated Mesos process::loop uses with process::ControlFlow.

[Benjamin Hindman]

> On Dec. 17, 2016, 9:53 p.m., Kevin Klues wrote:
> > src/slave/containerizer/mesos/io/switchboard.cpp, lines 1571-1593
> > 
> >
> > Is there a specific reason to reorganize things in this way with 
> > `.then()`, `.repair()` and `.recover()`. I'm mostly curious because using 
> > `.onAny()` tends to be simpler for me to reason about and I don't quite 
> > follow what's going on with your change (plus we lose the discarded case).

The reason is to simplify the logic around not having to create a `Promise`. 
While creating the promise is not that big of a deal, the bigger problem is 
actually linking the promise so as to properly handle discards. In the original 
code you don't propagate any discards into the `process::io::write` call which 
means that even if the code around you was discarded this code won't be. 
Setting up the discard chaining is tedious and complex. When you chain futures 
with `.then`, `.repair`, etc, then you don't have to worry about it. And note 
that in your original code you should have never had the discard case because 
there wasn't any chaining! That being said, adding `.recover` is meant to 
capture the chaining. Going forward, we want to introduce something like 
`.match` that let's you chain correctly regardless of the result.


- Benjamin


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


On Dec. 17, 2016, 9:40 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54839/
> ---
> 
> (Updated Dec. 17, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Mesos process::loop uses with process::ControlFlow.
> 
> 
> Diffs
> -
> 
>   src/common/recordio.hpp 5a22d066bd2bcd40ede5ebf6476b9ceb972dd584 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
>   src/slave/http.cpp e1cea46cb17ea2f24fe457bb06a0a1a55c2a2bc4 
> 
> Diff: https://reviews.apache.org/r/54839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54837: Added a CHECK when adding a framework in the allocator.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54837]

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

- Mesos ReviewBot


On Dec. 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54837/
> ---
> 
> (Updated Dec. 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a CHECK when adding a framework in the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8cbc8db3d1319718765783827f8be1981b56f04 
> 
> Diff: https://reviews.apache.org/r/54837/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 54837: Added a CHECK when adding a framework in the allocator.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54837/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Jay Guo and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a CHECK when adding a framework in the allocator.
> 
> 
> Diffs
> -
> 
>   src/master/allocator/mesos/hierarchical.cpp 
> a8cbc8db3d1319718765783827f8be1981b56f04 
> 
> Diff: https://reviews.apache.org/r/54837/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

[Guangya Liu]

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




include/mesos/resources.hpp (lines 335 - 336)


Can you please elaborate on this? I think that we do not want to allocate a 
resource again if it was already allocated to some other roles? even with 
optimistic offer?



include/mesos/resources.hpp (line 339)


What is the use of the return value here, can you please add more comments 
here for its usage? I think that the `unallocate()` will be called by 
`recoverResources`, and what is the problem if `unalocate` just return void for 
such case?



src/common/resources.cpp (line 1062)


Shall we add a `CHECK` here to make sure this resource was not allocated to 
any role?



src/common/resources.cpp (line 1075)


How about put this in the `if` block?


- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
>   src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
>   src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 54841: Used `loop` in implementation of io::read and io::write.

[Benjamin Hindman]

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

(Updated Dec. 18, 2016, 1:04 a.m.)


Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Used `loop` in implementation of io::read and io::write.


Diffs (updated)
-

  3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54842: Updated Resources to handle Resource.AllocationInfo.

[Guangya Liu]

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


Ship it!




Ship It!

- Guangya Liu


On 十二月 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54842/
> ---
> 
> (Updated 十二月 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> With the addition of `Resource.AllocationInfo`, we must prevent the
> loss of allocation information when performing addition and
> subtraction of resources.
> 
> 
> Diffs
> -
> 
>   src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
>   src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
>   src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 
> 
> Diff: https://reviews.apache.org/r/54842/diff/
> 
> 
> Testing
> ---
> 
> Added tests.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Guangya Liu]

> On 十二月 17, 2016, 8:07 p.m., Benjamin Mahler wrote:
> > src/tests/resources_tests.cpp, lines 2765-2769
> > 
> >
> > Hm.. I didn't follow this assumption, I guess when you're saying "using 
> > star role" you're referring to unreserved resources?

Yes, I mean unreserved resource here.


- Guangya


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


On 十二月 17, 2016, 1:24 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated 十二月 17, 2016, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54836: Added helpers to allocate / unallocate Resources.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54842, 54836]

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

- Mesos ReviewBot


On Dec. 17, 2016, 10:50 p.m., Benjamin Mahler wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54836/
> ---
> 
> (Updated Dec. 17, 2016, 10:50 p.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helpers to allocate / unallocate Resources.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
>   include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
>   src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
>   src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
>   src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 
> 
> Diff: https://reviews.apache.org/r/54836/diff/
> 
> 
> Testing
> ---
> 
> Updated the existing allocation test to incorporate the new helper.
> 
> 
> Thanks,
> 
> Benjamin Mahler
> 
>

Re: Review Request 54803: Fixed `SlaveTests` to pass when `HAS_AUTHENTICATION` is undefined.

[Alex Clemmer]

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

(Updated Dec. 17, 2016, 11:01 p.m.)


Review request for mesos, Adam B, Andrew Schwartzmeyer, Daniel Pravat, Greg 
Mann, John Kordich, Joseph Wu, and Vinod Kone.


Changes
---

Replace instances of `Clock::resume` with `Clock::advance`.


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


Repository: mesos


Description
---

Currently, when `HAS_AUTHENTICATION` is undefined, the Agent will
use `delay` to schedule a random time in the future to register with the
Master, to avoid the thundering herd problem after a Master failover.
The authentication codepath, in contrast, schedules the registration
immediately.

In tests where we have `Clock::pause`'d when we are supposed to be
registering the slave, the authention codepath will succeeed, while
no-authentication codepath will hang forever.

A much more detailed analysis of this situation exists in MESOS-6803.

This commit will resolve this issue for `slave_tests.cpp` by changing
the tests to not use `Clock::pause` when we are waiting for Agent
registration.


Diffs (updated)
-

  src/tests/slave_tests.cpp d956a326ef29bf29837e0587a14bae457147cbca 

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


Testing
---

Added `delay` to the call to `authenticate` in `Slave::detected`, ran tests to 
find failing tests in `SlaveTest.*`, then fixed, then ran again.


Thanks,

Alex Clemmer

Review Request 54836: Added helpers to allocate / unallocate Resources.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.


Repository: mesos


Description
---

Added helpers to allocate / unallocate Resources.


Diffs
-

  include/mesos/resources.hpp c2e6d6680d9687f0bbfb73e84ffb1457072c4412 
  include/mesos/v1/resources.hpp 98f025b68c818c384a1174ef9b969f5c0776a7f1 
  src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
  src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
  src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 

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


Testing
---

Updated the existing allocation test to incorporate the new helper.


Thanks,

Benjamin Mahler

Review Request 54842: Updated Resources to handle Resource.AllocationInfo.

[Benjamin Mahler]

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

Review request for mesos, Benjamin Bannier, Jay Guo, and Guangya Liu.


Repository: mesos


Description
---

With the addition of `Resource.AllocationInfo`, we must prevent the
loss of allocation information when performing addition and
subtraction of resources.


Diffs
-

  src/common/resources.cpp 16f6de6741b548a8a9fff5b70447b1a312f10efe 
  src/tests/resources_tests.cpp c0ff9777ceb1731086179e2966b07c46d035c110 
  src/v1/resources.cpp a1adf1b27964bf6a89d97715cff05878d8c6cae9 

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


Testing
---

Added tests.


Thanks,

Benjamin Mahler

Review Request 54837: Added a CHECK when adding a framework in the allocator.

[Benjamin Mahler]

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

Review request for mesos, Jay Guo and Guangya Liu.


Repository: mesos


Description
---

Added a CHECK when adding a framework in the allocator.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
a8cbc8db3d1319718765783827f8be1981b56f04 

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


Testing
---

make check


Thanks,

Benjamin Mahler

Re: Review Request 54839: Updated Mesos process::loop uses with process::ControlFlow.

[Kevin Klues]

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




src/slave/containerizer/mesos/io/switchboard.cpp (lines 1567 - 1583)


Is there a specific reason to reorganize things in this way with `.then()`, 
`.repair()` and `.recover()`. I'm mostly curious because using `.onAny()` tends 
to be simpler for me to reason about and I don't quite follow what's going on 
with your change (plus we lose the discarded case).


- Kevin Klues


On Dec. 17, 2016, 9:40 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54839/
> ---
> 
> (Updated Dec. 17, 2016, 9:40 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Kevin Klues, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated Mesos process::loop uses with process::ControlFlow.
> 
> 
> Diffs
> -
> 
>   src/common/recordio.hpp 5a22d066bd2bcd40ede5ebf6476b9ceb972dd584 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
>   src/slave/http.cpp e1cea46cb17ea2f24fe457bb06a0a1a55c2a2bc4 
> 
> Diff: https://reviews.apache.org/r/54839/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54840: Used process::loop to avoid stack overflow due to recursion.

[Benjamin Hindman]

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

(Updated Dec. 17, 2016, 9:50 p.m.)


Review request for mesos, Jie Yu and Kevin Klues.


Repository: mesos


Description
---

Used process::loop to avoid stack overflow due to recursion.


Diffs (updated)
-

  3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54358: Introduced ControlFlow for process::loop.

[Benjamin Hindman]

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

(Updated Dec. 17, 2016, 9:50 p.m.)


Review request for mesos, Benjamin Mahler and Michael Park.


Repository: mesos


Description
---

Introduced ControlFlow for process::loop.


Diffs (updated)
-

  3rdparty/libprocess/include/process/loop.hpp 
b35f7e6cc2da9d1c840656f80410756ba80dbc48 
  3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 
  3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
  3rdparty/libprocess/src/tests/loop_tests.cpp 
8435ba872e8d4505c3a9125e5a2dac1c31b9bf9a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54840: Used process::loop to avoid stack overflow due to recursion.

[Kevin Klues]

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


Ship it!




This isn't necessarily relatd to this review, but what values can be returned 
to `ControlFlow`. I'm guessing `Break()`, `Continue()`, and `T`?

- Kevin Klues


On Dec. 17, 2016, 9:39 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54840/
> ---
> 
> (Updated Dec. 17, 2016, 9:39 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::loop to avoid stack overflow due to recursion.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 
> 
> Diff: https://reviews.apache.org/r/54840/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 54838: Used process::io::BUFFERED_READ_SIZE instead of constant.

[Kevin Klues]

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


Ship it!




Was this introduced recently or did I miss it the first time around when 
implementing this? Should this be called "DEFAULT_BUFFERED_READ_SIZE"?

- Kevin Klues


On Dec. 17, 2016, 9:35 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54838/
> ---
> 
> (Updated Dec. 17, 2016, 9:35 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Kevin Klues.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Used process::io::BUFFERED_READ_SIZE instead of constant.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/io/switchboard.cpp 
> c80627986f729255e3b3ad1fc9cfa6213e19c521 
> 
> Diff: https://reviews.apache.org/r/54838/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Review Request 54838: Used process::io::BUFFERED_READ_SIZE instead of constant.

[Benjamin Hindman]

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

Review request for mesos, Jie Yu and Kevin Klues.


Repository: mesos


Description
---

Used process::io::BUFFERED_READ_SIZE instead of constant.


Diffs
-

  src/slave/containerizer/mesos/io/switchboard.cpp 
c80627986f729255e3b3ad1fc9cfa6213e19c521 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 54841: Used `loop` in implementation of io::read and io::write.

[Benjamin Hindman]

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

Review request for mesos, Benjamin Mahler and Jie Yu.


Repository: mesos


Description
---

Used `loop` in implementation of io::read and io::write.


Diffs
-

  3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 54840: Used process::loop to avoid stack overflow due to recursion.

[Benjamin Hindman]

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

Review request for mesos.


Repository: mesos


Description
---

Used process::loop to avoid stack overflow due to recursion.


Diffs
-

  3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Review Request 54839: Updated Mesos process::loop uses with process::ControlFlow.

[Benjamin Hindman]

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

Review request for mesos.


Repository: mesos


Description
---

Updated Mesos process::loop uses with process::ControlFlow.


Diffs
-

  src/common/recordio.hpp 5a22d066bd2bcd40ede5ebf6476b9ceb972dd584 
  src/slave/containerizer/mesos/io/switchboard.cpp 
c80627986f729255e3b3ad1fc9cfa6213e19c521 
  src/slave/http.cpp e1cea46cb17ea2f24fe457bb06a0a1a55c2a2bc4 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54358: Introduced ControlFlow for process::loop.

[Benjamin Hindman]

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

(Updated Dec. 17, 2016, 9:37 p.m.)


Review request for mesos and Benjamin Mahler.


Repository: mesos


Description
---

Introduced ControlFlow for process::loop.


Diffs (updated)
-

  3rdparty/libprocess/include/process/loop.hpp 
b35f7e6cc2da9d1c840656f80410756ba80dbc48 
  3rdparty/libprocess/src/http.cpp 97d1424be20e217401519c2bee79857bcf087023 
  3rdparty/libprocess/src/io.cpp 27da897894e12941a6bba5f5eda04c35100d2d73 
  3rdparty/libprocess/src/tests/loop_tests.cpp 
8435ba872e8d4505c3a9125e5a2dac1c31b9bf9a 

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


Testing
---

make check


Thanks,

Benjamin Hindman

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Benjamin Mahler]

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


Fix it, then Ship it!




Thanks!


include/mesos/resources.hpp (lines 377 - 378)


Maybe a comment that this must be called only on allocated resources, since 
this will otherwise crash the program via the CHECK.



include/mesos/v1/resources.hpp (lines 377 - 378)


Ditto here.



src/tests/resources_tests.cpp (lines 2756 - 2772)


I'll send you a review I have to add `Resources::allocate` and 
`Resources::unallocate`, this will let us avoid having a special helper here 
(for now we can just add a TODO here and I'll clean it up in my patch).



src/tests/resources_tests.cpp (lines 2765 - 2769)


Hm.. I didn't follow this assumption, I guess when you're saying "using 
star role" you're referring to unreserved resources?



src/v1/resources.cpp (line 34)


Since we already have  we have CHECK available, 
 introduces CHECK_SOME, CHECK_NONE, CHECK_ERROR.


- Benjamin Mahler


On Dec. 17, 2016, 1:24 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated Dec. 17, 2016, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54718: Removed dependency of `recover` on cached networks.

[Qian Zhang]

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


Ship it!




Ship It!

- Qian Zhang


On Dec. 17, 2016, 4:36 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54718/
> ---
> 
> (Updated Dec. 17, 2016, 4:36 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since CNI networks can be added and removed on the fly, and for a
> given container the CNI configuration with which the container was
> launched is check-pointed in the containers `network/cni` work
> directory, it doesn't make sense to require the network name that the
> container is associated with to exist in the in-memory cache. Hence,
> removing these dependencies.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54718/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 54717: Modified isolator for dynamic addition/deletion of CNI networks.

[Qian Zhang]

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




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


In the implementation of this method, I do not see you validate the 
existence of the plugin.



src/slave/containerizer/mesos/isolators/network/cni/cni.cpp (lines 1159 - 1170)


Just curious in which case `getNetwork` will succeeds but 
`getNetworkConfigJSON()` will fail?


- Qian Zhang


On Dec. 17, 2016, 4:19 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> ---
> 
> (Updated Dec. 17, 2016, 4:19 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
> 
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54717/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual testing with addition of a non-existent CNI configuration.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 54716: Modified the initialization logic for `network/cni` isolator.

[Qian Zhang]

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




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


I think this comment needs to be updated since you have changed the value 
of this hashmap from network configuration info to the path of network 
configuration file.



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


s/errors/error/



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


Do we need to mention the same network name twice in this message? I think 
for the later one we can just mention it as "it", like:
```
Skipping network xxx from configuration file '/xxx', since we failed to 
find CNI plugin 'yyy' used by it.
```



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


Ditto.


- Qian Zhang


On Dec. 17, 2016, 4:16 p.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> ---
> 
> (Updated Dec. 17, 2016, 4:16 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI  plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54716/diff/
> 
> 
> Testing
> ---
> 
> sudo make check --gtest_filter=*CNI*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54830]

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

- Mesos ReviewBot


On Dec. 17, 2016, 1:24 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated Dec. 17, 2016, 1:24 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Guangya Liu]

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

(Updated 十二月 17, 2016, 1:24 p.m.)


Review request for mesos, Benjamin Mahler and Jay Guo.


Repository: mesos


Description
---

Added helper function to get per-role resource allocations.


Diffs (updated)
-

  include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
  include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
  src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
  src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
  src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 

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


Testing
---

make
make check

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

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


Thanks,

Guangya Liu

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Guangya Liu]

> On 十二月 17, 2016, 10:19 a.m., Jay Guo wrote:
> > src/common/resources.cpp, lines 1082-1083
> > 
> >
> > Maybe "We do not expect allocated and unallocated resource objects 
> > being stored within the same `Resources` object."?

I updated this to "We do not expect unallocated resource objects being stored 
within the this `Resources` object."


- Guangya


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


On 十二月 17, 2016, 4:28 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated 十二月 17, 2016, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Guangya Liu]

> On 十二月 17, 2016, 10:17 a.m., Jay Guo wrote:
> > src/common/resources.cpp, line 1081
> > 
> >
> > I see we sometime use underscore as prefix and sometime suffix. I 
> > wonder if there's a fixed rule for the style? Also, do we necessary need 
> > this underscore here?

It is `Resource_ ` class here, so I was using `resource_` as variable here, 
same as others in this file.


- Guangya


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


On 十二月 17, 2016, 4:28 a.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated 十二月 17, 2016, 4:28 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54834: Fixed a typo in webui.

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [53291, 54833, 54834]

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

- Mesos ReviewBot


On Dec. 17, 2016, 9:41 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54834/
> ---
> 
> (Updated Dec. 17, 2016, 9:41 a.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Joseph Wu, and Vinod Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fixed a typo in webui.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/index.html a4eda3034906d100a20dcd0a2473112c1edd6b42 
>   src/webui/master/static/js/controllers.js 
> 5dcfbdb5ec36590377255d84569861c824738b04 
> 
> Diff: https://reviews.apache.org/r/54834/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 54832: Removed extra space in method definitions in src [2/2].

[Mesos ReviewBot]

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



Patch looks great!

Reviews applied: [54831, 54832]

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

- Mesos ReviewBot


On Dec. 17, 2016, 9:38 a.m., haosdent huang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54832/
> ---
> 
> (Updated Dec. 17, 2016, 9:38 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov, Jie Yu, Joseph Wu, and Vinod 
> Kone.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Removed extra space in method definitions in src [2/2].
> 
> 
> Diffs
> -
> 
>   src/examples/load_generator_framework.cpp 
> 99fa760aa99bb0b95accb49727b312816ac1ff09 
>   src/linux/routing/queueing/htb.cpp faadf32bd48cc6bf968b1229789903c0d01fd75c 
> 
> Diff: https://reviews.apache.org/r/54832/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> haosdent huang
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Jay Guo]

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




src/common/resources.cpp (lines 1082 - 1083)


Maybe "We do not expect allocated and unallocated resource objects being 
stored within the same `Resources` object."?


- Jay Guo


On Dec. 17, 2016, 12:28 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated Dec. 17, 2016, 12:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54830: Added helper function to get per-role resource allocations.

[Jay Guo]

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




src/common/resources.cpp (line 1081)


I see we sometime use underscore as prefix and sometime suffix. I wonder if 
there's a fixed rule for the style? Also, do we necessary need this underscore 
here?



src/common/resources.cpp (line 1084)


Since `role` is `optional` in `allocationInfo`, do we need an explicit 
check for that as well?



src/tests/resources_tests.cpp (lines 2756 - 2757)


Maybe "Helper for creating an allocated resource, indicating resource that 
has been allocated to a framework." ?


- Jay Guo


On Dec. 17, 2016, 12:28 p.m., Guangya Liu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54830/
> ---
> 
> (Updated Dec. 17, 2016, 12:28 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and Jay Guo.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added helper function to get per-role resource allocations.
> 
> 
> Diffs
> -
> 
>   include/mesos/resources.hpp f569c931ff7db8d51dfd7c96f4f2addab05df85d 
>   include/mesos/v1/resources.hpp f60ab794a0c7c24885c49cc47b798c363e3279e7 
>   src/common/resources.cpp 4bb9beffcb3509f4226b4985e05eccec01412d0f 
>   src/tests/resources_tests.cpp d6eb7787bac58c1133a4bab0fc17df49117fed87 
>   src/v1/resources.cpp 46cc00f2f453f5eb4ddc4b0b9b89be2bd89f05d9 
> 
> Diff: https://reviews.apache.org/r/54830/diff/
> 
> 
> Testing
> ---
> 
> make
> make check
> 
> ```
> ./bin/mesos-tests.sh  --gtest_filter="AllocatedResourcesTest.Allocations" 
> --verbose
> [==] Running 1 test from 1 test case.
> [--] Global test environment set-up.
> [--] 1 test from AllocatedResourcesTest
> [ RUN  ] AllocatedResourcesTest.Allocations
> [   OK ] AllocatedResourcesTest.Allocations (0 ms)
> [--] 1 test from AllocatedResourcesTest (0 ms total)
> 
> [--] Global test environment tear-down
> [==] 1 test from 1 test case ran. (17 ms total)
> [  PASSED  ] 1 test.
> ```
> 
> 
> Thanks,
> 
> Guangya Liu
> 
>

Re: Review Request 54833: Fixed the metrics display bug in agent.html.

[haosdent huang]

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

(Updated Dec. 17, 2016, 9:48 a.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, and Vinod Kone.


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


Repository: mesos


Description
---

Fixed the metrics display bug in agent.html.


Diffs
-

  src/webui/master/static/agent.html 3d15d550c15b8e882c2356cab2e1a0ad6291 
  src/webui/master/static/js/controllers.js 
5dcfbdb5ec36590377255d84569861c824738b04 

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


Testing (updated)
---

```
Test in OS X with mesos-local and mesos-execute
Don't flick again after apply the patches.
```


Thanks,

haosdent huang

Review Request 54833: Fixed the metrics display bug in agent.html.

[haosdent huang]

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

Review request for mesos, Benjamin Mahler, Joseph Wu, and Vinod Kone.


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


Repository: mesos


Description
---

Fixed the metrics display bug in agent.html.


Diffs
-

  src/webui/master/static/agent.html 3d15d550c15b8e882c2356cab2e1a0ad6291 
  src/webui/master/static/js/controllers.js 
5dcfbdb5ec36590377255d84569861c824738b04 

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


Testing
---


Thanks,

haosdent huang

Review Request 54834: Fixed a typo in webui.

[haosdent huang]

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

Review request for mesos, Benjamin Mahler, Joseph Wu, and Vinod Kone.


Repository: mesos


Description
---

Fixed a typo in webui.


Diffs
-

  src/webui/master/static/index.html a4eda3034906d100a20dcd0a2473112c1edd6b42 
  src/webui/master/static/js/controllers.js 
5dcfbdb5ec36590377255d84569861c824738b04 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 53291: Fixed incorrect `/monitor/statistics` URL in WebUI.

[haosdent huang]

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

(Updated Dec. 17, 2016, 9:40 a.m.)


Review request for mesos, Benjamin Mahler, Joseph Wu, Vinod Kone, and Jiang Yan 
Xu.


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


Repository: mesos


Description
---

When launches Mesos in local mode, all requests send to agents need to
specific the agent id in URL explicitly.


Diffs (updated)
-

  src/webui/master/static/js/controllers.js 
5dcfbdb5ec36590377255d84569861c824738b04 
  src/webui/master/static/js/services.js 
46cac4ea20217fbee602c496e6525d46831948e1 

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


Testing
---


Thanks,

haosdent huang

Review Request 54831: Removed extra space in method definition in 3rdparty [1/2].

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov, Jie Yu, Joseph Wu, and Vinod 
Kone.


Repository: mesos


Description
---

Removed extra space in method definition in 3rdparty [1/2].


Diffs
-

  3rdparty/stout/include/stout/flags/flags.hpp 
ecadaf8412e8a05f078d2c1e6eeddf159e0ad778 

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


Testing
---


Thanks,

haosdent huang

Review Request 54832: Removed extra space in method definitions in src [2/2].

[haosdent huang]

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

Review request for mesos, Alexander Rukletsov, Jie Yu, Joseph Wu, and Vinod 
Kone.


Repository: mesos


Description
---

Removed extra space in method definitions in src [2/2].


Diffs
-

  src/examples/load_generator_framework.cpp 
99fa760aa99bb0b95accb49727b312816ac1ff09 
  src/linux/routing/queueing/htb.cpp faadf32bd48cc6bf968b1229789903c0d01fd75c 

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


Testing
---


Thanks,

haosdent huang

Re: Review Request 54717: Modified isolator for dynamic addition/deletion of CNI networks.

[Avinash sridharan]

> On Dec. 17, 2016, 12:09 a.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, lines 250-252
> > 
> >
> > In fact, can we just merge this with `getNetwork` below. We have a 
> > single helper `getNetworkConfigJson` which will refresh the cache if 
> > necessary.
> 
> Avinash sridharan wrote:
> I was thinking about that as well. The only reason I kept them separate 
> was because I was using `getNetworkConfigJSON` as is in `detach` ? In 
> `detach` we are relying on the checkpointed configuration and not the 
> configuration learnt from `networkConfigs.`

Would it make sense if we make the `path` and `option` so the helper would be:
```
Try NetworkCniIsolatorProcess::getNetworkConfigJSON(
251 
const string& network,
252 
Option path = None())
```

If path is not set then we will basically search the `networkConfigs` else we 
will simply get the JSON config from the path. We can then have a single helper 
function here.


- Avinash


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


On Dec. 17, 2016, 8:19 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54717/
> ---
> 
> (Updated Dec. 17, 2016, 8:19 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> If the `network/cni` isolator sees a cache-miss during the `prepare`
> phase, it will try to look for the CNI network on disk before giving
> up. This allows for the dynamic addition of CNI networks without the
> need for agent restart.
> 
> During `isolate` or `prepare` if for a given network, the isolator is
> unable to read the corresponding configuration file, or finds and
> error in the existing configuration file, it will remove the network
> from the in-memory cache. This allows dynamic deletion of CNI
> configurations from the `network/cni` isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54717/diff/
> 
> 
> Testing
> ---
> 
> make check
> 
> Manual testing with addition of a non-existent CNI configuration.
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

Re: Review Request 54717: Modified isolator for dynamic addition/deletion of CNI networks.

[Avinash sridharan]

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

(Updated Dec. 17, 2016, 8:19 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

If the `network/cni` isolator sees a cache-miss during the `prepare`
phase, it will try to look for the CNI network on disk before giving
up. This allows for the dynamic addition of CNI networks without the
need for agent restart.

During `isolate` or `prepare` if for a given network, the isolator is
unable to read the corresponding configuration file, or finds and
error in the existing configuration file, it will remove the network
from the in-memory cache. This allows dynamic deletion of CNI
configurations from the `network/cni` isolator.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

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


Testing
---

make check

Manual testing with addition of a non-existent CNI configuration.


Thanks,

Avinash sridharan

Re: Review Request 54716: Modified the initialization logic for `network/cni` isolator.

[Avinash sridharan]

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

(Updated Dec. 17, 2016, 8:16 a.m.)


Review request for mesos, Jie Yu and Qian Zhang.


Changes
---

Addressed Jie's comments.


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


Repository: mesos


Description
---

Since the `network/cni` isolator will soon be able to
add/modify/delete CNI configuration files without the need for agent
restart, we are changing the initialization logic to not bail out if
we see errors while reading a CNI configuration file or don't find a
CNI  plugin for a given CNI configuration. If either of these errors
occur the `network/cni` isolator would simply skip the specific CNI
network and complete the initialization.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
  src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 

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


Testing
---

sudo make check --gtest_filter=*CNI*


Thanks,

Avinash sridharan

Re: Review Request 54716: Modified the initialization logic for `network/cni` isolator.

[Avinash sridharan]

> On Dec. 16, 2016, 11:39 p.m., Jie Yu wrote:
> > src/slave/containerizer/mesos/isolators/network/cni/cni.cpp, line 1183
> > 
> >
> > let's use networkConfigPath consistently

The reason I have `cniConfigPath` here is that we use `networkConfigPath` to 
denote the checkpointed CNI configuration (line 1189 in `attach`). This seemed 
consistent with what we were doing in `detach`.


- Avinash


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


On Dec. 17, 2016, 8:16 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/54716/
> ---
> 
> (Updated Dec. 17, 2016, 8:16 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-6567
> https://issues.apache.org/jira/browse/MESOS-6567
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since the `network/cni` isolator will soon be able to
> add/modify/delete CNI configuration files without the need for agent
> restart, we are changing the initialization logic to not bail out if
> we see errors while reading a CNI configuration file or don't find a
> CNI  plugin for a given CNI configuration. If either of these errors
> occur the `network/cni` isolator would simply skip the specific CNI
> network and complete the initialization.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.hpp 
> b8fc755a8dd4757d904f7e97a71d3cf7f29d2033 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> ddb4d33e9ecd0ffd118e8e68cb7ec90346b08049 
> 
> Diff: https://reviews.apache.org/r/54716/diff/
> 
> 
> Testing
> ---
> 
> sudo make check --gtest_filter=*CNI*
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>