Re: Review Request 61245: Added a test `SlaveTest.ValidateDefaultContainerDNSFlag`.

[Avinash sridharan]

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




src/tests/slave_tests.cpp
Lines 7878 (patched)


s/inexistent/non-existent



src/tests/slave_tests.cpp
Lines 7898 (patched)


s/inexistent/non-existent



src/tests/slave_tests.cpp
Lines 7934 (patched)


This code seems to be duplicated. I think you can use a parameterized test 
over here similar to this:

https://github.com/apache/mesos/blob/master/src/tests/default_executor_tests.cpp#L104



src/tests/slave_tests.cpp
Lines 7955-7972 (patched)


Ditto for using parameterized tests here for CNI and CNM.


- Avinash sridharan


On July 30, 2017, 3:32 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61245/
> ---
> 
> (Updated July 30, 2017, 3:32 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `SlaveTest.ValidateDefaultContainerDNSFlag`.
> 
> 
> Diffs
> -
> 
>   src/tests/slave_tests.cpp a089cc47eae41cd6baeffd3f4a7ee7c7984aacbd 
> 
> 
> Diff: https://reviews.apache.org/r/61245/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61219: Added a test `CniIsolatorTest.ROOT_VerifyDefaultDNSWildcardMatch`.

[Avinash sridharan]

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


Fix it, then Ship it!




Ship It!


src/tests/containerizer/cni_isolator_tests.cpp
Lines 1383-1385 (patched)


Can we change this to:
A DNS information with `network_mode = CNI`, but without a network name, 
acts as a wild card match making it the default DNS for any CNI network not 
specified in the `--default_container_dns` flag.


- Avinash sridharan


On July 28, 2017, 2:35 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61219/
> ---
> 
> (Updated July 28, 2017, 2:35 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `CniIsolatorTest.ROOT_VerifyDefaultDNSWildcardMatch`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> ae0980bd671849fcd3e19941b33c7d3b09fdae7c 
> 
> 
> Diff: https://reviews.apache.org/r/61219/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61257: Added full authz for non summarized fields of `/slaves` endpoint.

[Greg Mann]

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




src/master/http.cpp
Lines 489-491 (original), 490-493 (patched)


As mentioned in the parent review, perhaps we should authorize on the 
canonical resource format, and then convert afterward.


- Greg Mann


On July 31, 2017, 2:01 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61257/
> ---
> 
> (Updated July 31, 2017, 2:01 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Fields were authorized based on partial elements of each
> resource. Moreover, some fields which required authorization were not
> being authorized at all. This patch enables full authorization of all
> fields.
> 
> 
> Diffs
> -
> 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
> 
> 
> Diff: https://reviews.apache.org/r/61257/diff/1/
> 
> 
> Testing
> ---
> 
> ```shell
> make check
> ```
> 
> 
> Thanks,
> 
> Alexander Rojas
> 
>

Re: Review Request 61171: Enabled filtering of the 'GET_AGENTS' v1 API call.

[Greg Mann]

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




src/common/http.hpp
Lines 274-276 (patched)


Could you add a comment above this declaration, explaining what resource 
format this function accepts?



src/common/http.cpp
Lines 984-1009 (patched)


I left another comment regarding the resource format we use for 
authorization. If we switch to authorize using only the canonical 
post-reservation-refinement resource format, then I think it should be 
sufficient to authorize just the roles in `Resource.reservations`.

Perhaps we should also add a CHECK to ensure that `!resource.has_role()` 
and `!resource.has_reservation()`? (Question: are all resources in the 
canonical format, including the `SlaveInfo` in `recovered` agents?)



src/common/http.cpp
Lines 988-990 (patched)


As written, when authorizing resources in the ENDPOINT format, this 
condition will incorrectly call into the acceptor when `resource.role() == 
"*"`; this actually represents an _unreserved_ resource, so we should not call 
into the acceptor.

If we switch to authorizing only the canonical resource format, this code 
could be removed.



src/common/protobuf_utils.cpp
Lines 900-904 (original), 910-915 (patched)


We discussed this in chat, but leaving a comment here for posterity.

Shouldn't we be authorizing using the internal, canonical resource 
representation, rather than converting the resource format first?



src/master/http.cpp
Lines 2525-2527 (patched)


Indented too far.



src/tests/api_tests.cpp
Lines 1638 (patched)


s/tests/test/



src/tests/api_tests.cpp
Lines 1719-1737 (patched)


While this code ensures that all reservations specify the correct role, it 
doesn't assert that the expected reservations do appear.



src/tests/api_tests.cpp
Lines 1722 (patched)


Note that this should fail for unreserved resources, since in that case 
`role == "*"` in the ENDPOINT resource format.



src/tests/api_tests.cpp
Lines 1725-1727 (patched)


Do we expect any AllocationInfo in this response?



src/tests/api_tests.cpp
Lines 1771 (patched)


Since the default value of `role` is `*`, this should be:

```
if (resource.has_role()) {
  EXPECT_EQ(resource.role(), "*");
}
```



src/tests/api_tests.cpp
Lines 1773-1775 (patched)


Do we expect any AllocationInfo in this response?



src/tests/api_tests.cpp
Lines 1781-1784 (patched)


Since the `ReservationInfo`s in the `reservations` field should _always_ 
have their `role` field set, you can probably just do:

```
EXPECT_EQ(resource.reservations_size(), 0);
```


- Greg Mann


On July 31, 2017, 1:50 p.m., Alexander Rojas wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61171/
> ---
> 
> (Updated July 31, 2017, 1:50 p.m.)
> 
> 
> Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff.
> 
> 
> Bugs: MESOS-7416
> https://issues.apache.org/jira/browse/MESOS-7416
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Enables filtering of the results of calls to the 'GET_AGENTS' v1
> API. It filters the contents of different resources entries based
> on the 'VIEW_ROLE' permissions of the principal doing the request
> based on resource roles, allocation roles and reservations.
> 
> 
> Diffs
> -
> 
>   src/common/http.hpp ba8dda18a02f51d1a28e719f06ee4b51573dfbec 
>   src/common/http.cpp dfd5f335e8a3745d047d4f9f5e8c821b2c22da5a 
>   src/common/protobuf_utils.hpp 80d2edd452f3ffa38c40f9a21f8489799065c401 
>   src/common/protobuf_utils.cpp 49d3a229925f4aa107e3e5f762936c16318aeadb 
>   src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
>   src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
>   src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b 
> 
> 
> Diff: https://reviews.apache.org/r/61171/diff/4/
> 
> 
> Testing
> ---
> 
> ```shell
> make check
> ```
> 
> Manual test:
> 
> ```shell
> mkdir -p /tmp/mesos/master
> mkdir -p /tmp/mesos/agent
> 
> # Create credential

Re: Review Request 60793: Added a test `CniIsolatorTest.ROOT_VerifyDefaultDNS`.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 25, 2017, 6:08 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60793/
> ---
> 
> (Updated July 25, 2017, 6:08 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `CniIsolatorTest.ROOT_VerifyDefaultDNS`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/cni_isolator_tests.cpp 
> ae0980bd671849fcd3e19941b33c7d3b09fdae7c 
> 
> 
> Diff: https://reviews.apache.org/r/60793/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61058: Added a lock-free event queue.

[Dario Rexin]

> On July 31, 2017, 9:05 p.m., Dario Rexin wrote:
> > 3rdparty/libprocess/src/event_queue.hpp
> > Lines 328 (patched)
> > 
> >
> > I don't think this queue matches the requirements for an actor system. 
> > This queue uses sub-queues as a performance optimization and the ordering 
> > between the different queues is not guaranteed. The assumption here is, 
> > that causal ordering only needs to be maintained per thread. In an actor 
> > system however, there is no guarantee on which thread an actor will run. 
> > Think of following scenario:
> > 
> > Actor A receives a burst of messages and takes some time to process 
> > them.
> > 
> > Actor B receives a message gets dispatched to Thread 1 and sends a 
> > message to Actor A. The queue of Actor B is now empty, so it donates the 
> > thread back to the pool. 
> > 
> > Actor A is still busy processing the messages it received earlier and 
> > did not process the message that Actor B sent.
> > 
> > Actor B receives another message and gets dispatched to Thread 2. It 
> > now sends another message to Actor A. 
> > 
> > Now Actor A received 2 messages from Actor B from 2 different threads. 
> > The moodycamel queue does not guarantee the order of those messages. This 
> > violates the arrival ordering defined in the actor model and makes it 
> > impossible to reason about message ordering between two actors.
> 
> Benjamin Hindman wrote:
> For posterity, to account for this "deficiency" in the concurrent queue 
> we've forced the _single_ consumer semantics on the queue and let the 
> consumer reorder any events when the read them out. See the lock-free 
> implementation of `EventQueue::try_dequeue` for more details.
> 
> Dario Rexin wrote:
> Clarified with benh and benm. There's additional code that restores 
> ordering.

-.- I forgot to click publish


- Dario


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


On July 29, 2017, 8:52 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61058/
> ---
> 
> (Updated July 29, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a lock-free event queue.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/configure.ac cb2cf4f32be5cbdf9df1e32f9aaf2bbba0a5ae03 
>   3rdparty/libprocess/src/event_queue.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> b268cdad776a3ca2a87cbe60eb098bde2a70667c 
> 
> 
> Diff: https://reviews.apache.org/r/61058/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 61124: Push metric history entries with their timestamp.

[Benjamin Mahler]

> On July 31, 2017, 8:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp
> > Line 63 (original), 63 (patched)
> > 
> >
> > Can this just be:
> > 
> > ```
> > void push(double value, Time now = Clock::now())
> > ```
> > 
> > ?
> 
> James Peach wrote:
> The common case for metrics is that there is no time series, in which 
> case we don't need the current timestamp. If I add `Clock::now` as a default 
> parameter we would be unnecessarily sampleing the clock (hitting a global 
> lock) on every metrics update.

Ah whoops!


- Benjamin


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


On July 25, 2017, 11:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> ---
> 
> (Updated July 25, 2017, 11:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 
> 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 
> 0a9c0227c457c6c81a59f65f901a5464ee00983d 
> 
> 
> Diff: https://reviews.apache.org/r/61124/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 60761: Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.

[Avinash sridharan]

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


Ship it!




Ship It!

- Avinash sridharan


On July 25, 2017, 6:07 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> ---
> 
> (Updated July 25, 2017, 6:07 a.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.
> 
> 
> Diffs
> -
> 
>   src/tests/containerizer/docker_containerizer_tests.cpp 
> 1e85a79f812399270575ea4a64db10e72f40e648 
> 
> 
> Diff: https://reviews.apache.org/r/60761/diff/3/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60600: Set container DNS with `--default_container_dns` in CNI isolator.

[Avinash sridharan]

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




src/slave/containerizer/mesos/isolators/network/cni/cni.cpp
Lines 1008-1028 (patched)


Just a thought? Storing the CNI network DNS entries in a map and the 
defaultDNS also in an `Option` at startup might simplify this code a lot over 
here?


- Avinash sridharan


On July 28, 2017, 2:34 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60600/
> ---
> 
> (Updated July 28, 2017, 2:34 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set container DNS with `--default_container_dns` in CNI isolator.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/network/cni/cni.cpp 
> 831bc7d0774a0ad3c8cbd7f42d4a3f8bd34d3243 
> 
> 
> Diff: https://reviews.apache.org/r/60600/diff/5/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 1. Start Mesos master.
> ```
> $ sudo ./bin/mesos-master.sh --work_dir=/opt/mesos
> ```
> 
> 2. Start Mesos agent.
> ```
> $ sudo ./bin/mesos-slave.sh --master=192.168.122.216:5050 
> --containerizers=mesos,docker --image_providers=docker 
> --image_provisioner_backend=aufs 
> --isolation=filesystem/linux,docker/runtime,network/cni,cgroups/cpu,cgroups/mem
>  --network_cni_config_dir=/opt/cni/net_configs 
> --network_cni_plugins_dir=/opt/cni/plugins --work_dir=/opt/mesos 
> --docker_store_dir=/opt/mesos/store/docker 
> --executor_registration_timeout=60mins 
> --default_container_dns=file:///home/stack/dns.json
> 
> $ cat /opt/cni/net_configs/net1 
> {
> "name": "net1",
> "type": "bridge",
> "bridge": "br1",
> "isGateway": true,
> "ipMasq": true,
> "ipam": {
> "type": "host-local",
> "subnet": "192.168.1.0/24",
> "routes": [
> { "dst": "0.0.0.0/0" }
> ]
> }
> }
> 
> $ cat /opt/cni/net_configs/net2 
> {
> "name": "net2",
> "type": "bridge",
> "bridge": "br2",
> "isGateway": true,
> "ipMasq": true,
> "ipam": {
> "type": "host-local",
> "subnet": "192.168.2.0/24",
> "routes": [
> { "dst": "0.0.0.0/0" }
> ]
> },
> "dns": {
> "nameservers": [ "8.8.4.4" ],
> "domain": "net2.com",
> "search": [ "yyy.com" ],
> "options": [ "attempts:3" ]
> }
> }
> 
> $ cat /home/stack/dns.json
> {
>   "mesos": [
> {
>   "network_mode": "CNI",
>   "network_name": "net1",
>   "dns": {
> "nameservers": [ "8.8.8.8" ],
> "search": [ "xxx.com" ],
> "options": [ "timeout:4" ]
>   }
> }
>   ]
> }
> ```
> 
> 3. Launch a unified container with `mesos-execute`.
> ```
> $ sudo src/mesos-execute --master=192.168.122.216:5050 
> --task=file:///home/stack/task.json
> 
> $ cat /home/stack/task.json 
> {
>   "name": "test",
>   "task_id": {"value" : "test"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   }
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   }
> }
>   ],
>   "command": {
> "value": "sleep 300"
>   },
>   "container": {
> "type": "MESOS",
> "mesos": {
>   "image": {
> "type": "DOCKER",
> "docker": {
>   "name": "busybox"
> }
>   }
> },
> "network_infos": [
>   {
> "name": "net1"
>   },
>   {
> "name": "net2"
>   }
> ]
>   }
> }
> ```
> 
> 4. Check the DNS configuration of the unified container.
> ```
> $ ps -ef | grep sleep 
> root 20060 20037  2 21:45 ?00:00:00 sh -c sleep 300
> root 20074 20060  0 21:45 ?00:00:00 sleep 300
> 
> $ sudo nsenter -t 20060 -m -u -n cat /etc/resolv.conf   
> domain net2.com
> search yyy.com xxx.com
> options attempts:3 timeout:4
> nameserver 8.8.4.4
> nameserver 8.8.8.8
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 61220: Added MESOS-5116 to the CHANGELOG.

[Jiang Yan Xu]

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


Fix it, then Ship it!





docs/upgrades.md
Lines 350 (patched)


Remove the `#`. This is why we also need "Testing Done" for doc changes. :) 
Normally we just say "Tested with a markdown viewer".


- Jiang Yan Xu


On July 31, 2017, 2:42 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61220/
> ---
> 
> (Updated July 31, 2017, 2:42 p.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added MESOS-5116 (XFS disk isolator support for no-enforce mode) to
> the CHANGELOG.
> 
> 
> Diffs
> -
> 
>   CHANGELOG e091fec10296090123986ca44a86645e5867aa0d 
>   docs/configuration.md a06ac82e7b908601b9ba97011931276e6292102d 
>   docs/upgrades.md 349d8a144097d9be354a95c904ab7209ae3cc442 
> 
> 
> Diff: https://reviews.apache.org/r/61220/diff/2/
> 
> 
> Testing
> ---
> 
> None.
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61260: Added agent garbage collection metrics.

[Jiang Yan Xu]

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




src/slave/gc.cpp
Lines 50 (patched)


s/total/completed/?

Just to distinguish from the ones that are unscheduled/cancelled? Also when 
comparing `gc_path_removals_total` and `gc_path_removals_scheduled` it's not 
clear if the former is a superset of the latter.



src/slave/gc.cpp
Lines 52-57 (patched)


The name `_scheduled` is a bit ambiguous as it can be interpreted as a 
Counter that tracks all paths that have ever been scheduled. (The API is called 
`schedule()`).

"The number of paths that have ever been scheduled" is probably itself an 
interesting metric. Can we add it?

This current metric could be named "slave/gc_path_removals_pending"



src/slave/gc.cpp
Lines 55 (patched)


s/ it//



src/slave/gc.cpp
Lines 199 (patched)


Generally prefer pre-increments for object types: 
https://google.github.io/styleguide/cppguide.html#Preincrement_and_Predecrement

Also, strictly speaking this labmda can be run after the GC is destructed. 
Seems like we have to pass in the two counters as copies directly.



src/slave/gc.cpp
Lines 205 (patched)


Ditto.



src/tests/gc_tests.cpp
Lines 95 (patched)


s/count/expected/?

Also I don't see too much value in this class-level helper. 

It may be reducing `ASSERT_EQ` and `EXPECT_SOME_EQ` into one call but the 
`JSON::Object metrics = Metrics();` line is already shared when multiple 
metrics are checked; and it is more correct semantically. i.e., we are reading 
the metrics once and verifying the consistency between the metrics. One could 
argue that we know in this test the metrics don't change between invocations of 
this helper but this is at the expense of clarity. Overall I think we can 
either 1) continue to inline these or 2) if it makes a strong case for an 
abstraction, provide it as a more general helper (possibly put it where 
`JSON::Object Metrics()` is defined).


- Jiang Yan Xu


On July 31, 2017, 9:31 a.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61260/
> ---
> 
> (Updated July 31, 2017, 9:31 a.m.)
> 
> 
> Review request for mesos and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-7842
> https://issues.apache.org/jira/browse/MESOS-7842
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added some basic sandbox garbage collection metrics to track the number
> of paths removals, the number of failed path removal and the number of
> paths that are still scheduled for removal.
> 
> 
> Diffs
> -
> 
>   src/slave/gc.cpp f270fa47b22904e926f4bd4b0a7693a41b2c8271 
>   src/slave/gc_process.hpp PRE-CREATION 
>   src/tests/gc_tests.cpp d4daad3993bb9a92db2c7add6e74f12aeb71d535 
> 
> 
> Diff: https://reviews.apache.org/r/61260/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 26).
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61124: Push metric history entries with their timestamp.

[James Peach]

> On July 31, 2017, 8:10 p.m., Benjamin Mahler wrote:
> > 3rdparty/libprocess/include/process/metrics/metric.hpp
> > Line 63 (original), 63 (patched)
> > 
> >
> > Can this just be:
> > 
> > ```
> > void push(double value, Time now = Clock::now())
> > ```
> > 
> > ?

The common case for metrics is that there is no time series, in which case we 
don't need the current timestamp. If I add `Clock::now` as a default parameter 
we would be unnecessarily sampleing the clock (hitting a global lock) on every 
metrics update.


- James


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


On July 25, 2017, 11:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> ---
> 
> (Updated July 25, 2017, 11:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 
> 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 
> 0a9c0227c457c6c81a59f65f901a5464ee00983d 
> 
> 
> Diff: https://reviews.apache.org/r/61124/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61258: Graduated HTTP and TCP health checks.

[Gastón Kleiman]

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


Ship it!




Ship It!

- Gastón Kleiman


On July 31, 2017, 2:36 p.m., Alexander Rukletsov wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61258/
> ---
> 
> (Updated July 31, 2017, 2:36 p.m.)
> 
> 
> Review request for mesos, Anand Mazumdar, Gastón Kleiman, haosdent huang, and 
> Kapil Arya.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> See summary.
> 
> 
> Diffs
> -
> 
>   CHANGELOG e091fec10296090123986ca44a86645e5867aa0d 
> 
> 
> Diff: https://reviews.apache.org/r/61258/diff/1/
> 
> 
> Testing
> ---
> 
> None: not a functional change.
> 
> 
> Thanks,
> 
> Alexander Rukletsov
> 
>

Re: Review Request 61182: Sent a resource provider message when providers subscribe.

[Jie Yu]

> On July 28, 2017, 10:03 p.m., Jie Yu wrote:
> > src/resource_provider/message.hpp
> > Lines 29 (patched)
> > 
> >
> > With ERP in mind, maybe it's more approapriate to make this message for 
> > a single resource provider? For instance:
> > 
> > ```
> > struct ResourceProviderMessage
> > {
> >   enum class Type
> >   {
> > UPDATE_TOTAL_RESOURCES,
> >   };
> >   
> >   struct UpdateTotalResources
> >   {
> > Option id;
> > Resources total;
> >   };
> > 
> >   Type type;
> >   
> >   UpdateTotalResources updateTotalResources;
> > };
> > ```
> 
> Benjamin Bannier wrote:
> I am not sure we need to do this right now. Independent of ERP/LRP, 
> splitting this up along RPs would somewhat help us to
> 
> * keep the transfered `total` small, and
> * make it easier to perform RP-specific actions when a RP changes (e.g., 
> to implement simple resource health actions).
> 
> OTOH it should be possible to implement this without complicating the 
> protocol.
> 
> Could we punt on that one for now?
> 
> * * *
> 
> I took your suggestion and renamed `s/TOTAL/UPDATE_TOTAL_RESOURCES` in 
> this patch.

The reason I was pushing for per provider update message is because the agent 
logic will be different. If we agree that per provider update is the way to go 
in the future, we should just do it right now so that we don't need to adjust 
the agent logic too much in the future. what do you think?


- Jie


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


On July 31, 2017, 3:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61182/
> ---
> 
> (Updated July 31, 2017, 3:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to inform users of resource provider managers that the
> managed resources have changed, we add a new 'ResourceProviderMessage'
> type for communicating changes to the managed total resources.
> 
> We add code to trigger sending of that message when a resource
> provider subscribes with the manager.
> 
> In the future this message could also be used to communicate changes
> to an already subscribed resource provider's total resources.
> 
> 
> Diffs
> -
> 
>   src/Makefile.am 5712bad2acc4cf0f8ec9b7febffcdb0fa77578c9 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/tests/CMakeLists.txt 6dd2716de942adf6cefa5a464ef664f3c3ebb7a3 
>   src/tests/resource_provider_http_api_tests.cpp 
> 85906ea5e1bb3516ef264de22913ce0a3c9c58c5 
>   src/tests/resource_provider_manager_tests.cpp PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/61182/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61220: Added MESOS-5116 to the CHANGELOG.

[James Peach]

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

(Updated July 31, 2017, 9:42 p.m.)


Review request for mesos and Jiang Yan Xu.


Repository: mesos


Description
---

Added MESOS-5116 (XFS disk isolator support for no-enforce mode) to
the CHANGELOG.


Diffs (updated)
-

  CHANGELOG e091fec10296090123986ca44a86645e5867aa0d 
  docs/configuration.md a06ac82e7b908601b9ba97011931276e6292102d 
  docs/upgrades.md 349d8a144097d9be354a95c904ab7209ae3cc442 


Diff: https://reviews.apache.org/r/61220/diff/2/

Changes: https://reviews.apache.org/r/61220/diff/1-2/


Testing
---

None.


Thanks,

James Peach

Review Request 61241: Removed superfluous variable assignment in 'mesos-style.py'.

[Armand Grillet]

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

Review request for mesos and Kevin Klues.


Repository: mesos


Description
---

See summary.


Diffs
-

  support/mesos-style.py 77a4e855a5595b3933fd271919a56a869378b742 


Diff: https://reviews.apache.org/r/61241/diff/1/


Testing
---


Thanks,

Armand Grillet

Re: Review Request 61112: Added executor_uris flag to long lived and balloon frameworks.

[Armand Grillet]

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

(Updated July 31, 2017, 9:39 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Rsolved issues and flag added to long lived framework.


Summary (updated)
-

Added executor_uris flag to long lived and balloon frameworks.


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


Repository: mesos


Description
---

Allows to set URIs that will be fetched before running the executor.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
  src/examples/long_lived_framework.cpp 
af5b43ee6994cf703f412436d6d561d9abd3b26d 


Diff: https://reviews.apache.org/r/61112/diff/3/

Changes: https://reviews.apache.org/r/61112/diff/2-3/


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 60934: Implemented blkio subsystem usage() for resource statistics.

[Gilbert Song]

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

(Updated July 31, 2017, 2:38 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


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


Repository: mesos


Description
---

Implemented blkio subsystem usage() for resource statistics.


Diffs (updated)
-

  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
a2c575cc87a9e08612cf417013dac76ad6de873b 
  src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
6be0f9ed4aa8c1a2273e5808ad54d3a4922c5e8d 


Diff: https://reviews.apache.org/r/60934/diff/5/

Changes: https://reviews.apache.org/r/60934/diff/4-5/


Testing
---

make check

Tested with `mesos-execute` and verified that the blkio statistics can be 
collected from the resource statistics endpoint:

Start the master:
sudo ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos

Start the agent:
sudo GLOG_v=1 ./bin/mesos-agent.sh --master=127.0.0.1:5050 
--isolation=cgroups/blkio,docker/runtime,filesystem/linux --work_dir=/tmp 
--image_providers=docker --executor_environment_variables="{}"

Launch `mesos-execute` test framework:
sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
--docker_image=alpine --shell=true --command="while true ; do echo 'hello' > 
test.txt ; done"

Collect the statistics for blkio:
```
vagrant@vagrant-ubuntu-wily-64:~$ curl localhost:5051/monitor/statistics.json | 
python -m json.tool
  % Total% Received % Xferd  Average Speed   TimeTime Time  Current
 Dload  Upload   Total   SpentLeft  Speed
100  1247  100  12470 0  39390  0 --:--:-- --:--:-- --:--:-- 40225
[
{
"executor_id": "test",
"executor_name": "Command Executor (Task: test) (Command: sh -c 'while 
true ;...')",
"framework_id": "39fb6d5c-d5bd-4b18-a632-0a42e417b946-",
"source": "test",
"statistics": {
"blkio": {
"cfq": [
{
"io_merged": [
{
"op": "TOTAL",
"value": 0
}
],
"io_queued": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_bytes": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_time": [
{
"op": "TOTAL",
"value": 0
}
],
"io_serviced": [
{
"op": "TOTAL",
"value": 0
}
],
"io_wait_time": [
{
"op": "TOTAL",
"value": 0
}
]
}
],
"cfq_recursive": [
{
"io_merged": [
{
"op": "TOTAL",
"value": 0
}
],
"io_queued": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_bytes": [
{
"op": "TOTAL",
"value": 0
}
],
"io_service_time": [
{
"op": "TOTAL",
"value": 0
}
],
"io_serviced": [
{
"op": "TOTAL",
"value": 0
}
],
"io_wait_time": [
{
"op": "TOTAL",
"value": 0
}
]
}
],
 

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[Gilbert Song]

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

(Updated July 31, 2017, 2:37 p.m.)


Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
Zhitao Li.


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


Repository: mesos


Description
---

Only statistics information for blkio in protobuf.


Diffs (updated)
-

  include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
  include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 


Diff: https://reviews.apache.org/r/60932/diff/7/

Changes: https://reviews.apache.org/r/60932/diff/6-7/


Testing
---

make


Thanks,

Gilbert Song

Re: Review Request 61111: Extracted strings into constants in example frameworks.

[Armand Grillet]

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

(Updated July 31, 2017, 9:22 p.m.)


Review request for mesos, Alexander Rukletsov and Till Toenshoff.


Changes
---

Included /r/61135 and /r/61136.


Summary (updated)
-

Extracted strings into constants in example frameworks.


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


Repository: mesos


Description (updated)
---

This impacts the balloon, disk full, and docker no executor frameworks.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 
  src/examples/docker_no_executor_framework.cpp 
4a58f11fc8892f23ade1d8c872ab9b4fc580d478 


Diff: https://reviews.apache.org/r/6/diff/3/

Changes: https://reviews.apache.org/r/6/diff/2-3/


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 61110: Added name flag to balloon and disk full frameworks.

[Armand Grillet]

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

(Updated July 31, 2017, 9:21 p.m.)


Review request for mesos, Alexander Rukletsov and Alexander Rojas.


Changes
---

Included /r/61134.


Summary (updated)
-

Added name flag to balloon and disk full frameworks.


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


Repository: mesos


Description
---

Allows to distinguish the framework between different instances.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
  src/examples/disk_full_framework.cpp 215b6d29cf5f52973f2b211e9fc72ca6ec2afa7a 


Diff: https://reviews.apache.org/r/61110/diff/4/

Changes: https://reviews.apache.org/r/61110/diff/3-4/


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 61180: Added a resource providers total resources to the subscribe call.

[Jie Yu]

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


Ship it!





include/mesos/v1/resource_provider/resource_provider.proto
Lines 72 (patched)


You should add a TODO here as well.


- Jie Yu


On July 31, 2017, 3:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61180/
> ---
> 
> (Updated July 31, 2017, 3:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Since resource provider resources are dynamic (as opposed to how e.g.,
> agent total resources are implemented), they are not part of the
> 'ResourceProviderInfo'. Instead they are communicated explicitly.
> 
> This commit adds total resources the resource provider 'SUBSCRIBE'
> call which can be used to communicate the total capacity in both
> subscription and resubscription scenarios.
> 
> 
> Diffs
> -
> 
>   include/mesos/resource_provider/resource_provider.proto 
> a8a27ed09cde7a9f137e76900a3a1c1a547752ed 
>   include/mesos/v1/resource_provider/resource_provider.proto 
> 34bce7511bc74e157277371a7f46111c9537bcf2 
> 
> 
> Diff: https://reviews.apache.org/r/61180/diff/2/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/61182/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61058: Added a lock-free event queue.

[Dario Rexin]

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




3rdparty/libprocess/src/event_queue.hpp
Lines 328 (patched)


I don't think this queue matches the requirements for an actor system. This 
queue uses sub-queues as a performance optimization and the ordering between 
the different queues is not guaranteed. The assumption here is, that causal 
ordering only needs to be maintained per thread. In an actor system however, 
there is no guarantee on which thread an actor will run. Think of following 
scenario:

Actor A receives a burst of messages and takes some time to process them.

Actor B receives a message gets dispatched to Thread 1 and sends a message 
to Actor A. The queue of Actor B is now empty, so it donates the thread back to 
the pool. 

Actor A is still busy processing the messages it received earlier and did 
not process the message that Actor B sent.

Actor B receives another message and gets dispatched to Thread 2. It now 
sends another message to Actor A. 

Now Actor A received 2 messages from Actor B from 2 different threads. The 
moodycamel queue does not guarantee the order of those messages. This violates 
the arrival ordering defined in the actor model and makes it impossible to 
reason about message ordering between two actors.


- Dario Rexin


On July 29, 2017, 8:52 p.m., Benjamin Hindman wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61058/
> ---
> 
> (Updated July 29, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos and Benjamin Mahler.
> 
> 
> Bugs: MESOS-7798
> https://issues.apache.org/jira/browse/MESOS-7798
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Added a lock-free event queue.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/configure.ac cb2cf4f32be5cbdf9df1e32f9aaf2bbba0a5ae03 
>   3rdparty/libprocess/src/event_queue.hpp PRE-CREATION 
>   3rdparty/libprocess/src/process.cpp 
> b268cdad776a3ca2a87cbe60eb098bde2a70667c 
> 
> 
> Diff: https://reviews.apache.org/r/61058/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Benjamin Hindman
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

> On July 28, 2017, 8:42 p.m., Michael Park wrote:
> > 3rdparty/libprocess/include/process/dispatch.hpp
> > Lines 193-197 (original), 201-225 (patched)
> > 
> >
> > Could you explain how this is any different?
> > 
> > It looks to me like the arguemnts are captured by-value in 
> > `Dispatcher`, which is what happens with the lambda anyway.
> > 
> > Here and below.

This is an equivalent of C++14's `[a1=std::forward(a1), ...]() {...}`. `aN` 
is always an lvalue in this context, so lambda should copy-construct a capture. 
`Dispatcher` forwards `aN` parameters to member fields via `INIT_VAR_TEMPLATE`, 
so it could move-construct in certain cases.


- Dmitry


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


On July 31, 2017, 8:31 p.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated July 31, 2017, 8:31 p.m.)
> 
> 
> Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
> Michael Park.
> 
> 
> Bugs: MESOS-7713
> https://issues.apache.org/jira/browse/MESOS-7713
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This reduces number of copies made for each parameter in a code like this:
> future.then(defer(pid, &SomeProcess::someMethod, param1, param2));
> 
> For the objects that do not support move semantics (e.g. protobuf messages), 
> number of copies is reduced from 8-10 to 6. If move semantics is supported, 
> then number of copies is reduced from 6-7 to 1 if parameter is passed with 
> std::move, or 2 otherwise.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/defer.hpp 
> 7f3369e723cb244e97930621cbba89cf7873567d 
>   3rdparty/libprocess/include/process/deferred.hpp 
> e446621be11ac51f5f91c417cc8975e363c2f715 
>   3rdparty/libprocess/include/process/dispatch.hpp 
> 3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
>   3rdparty/libprocess/include/process/future.hpp 
> cce950509f58022e79bb51a6e72ea1a005b9cb50 
>   3rdparty/libprocess/include/process/http.hpp 
> f637999174d92a98208b5fc49a65f9929efb11a0 
>   3rdparty/libprocess/src/tests/benchmarks.cpp 
> 694a842e8e18d82ac551749a71764825ba7cb3a9 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/3/
> 
> 
> Testing
> ---
> 
> make check
> 
> Number of copies was checked by using defer to subscribe process for Future 
> callbacks, and passing parameters that count number of copies made.
> 
> 
> Thanks,
> 
> Dmitry Zhuk
> 
>

Re: Review Request 60003: Reduced copying in defer, dispatch and Future.

[Dmitry Zhuk]

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

(Updated July 31, 2017, 8:31 p.m.)


Review request for mesos, Benjamin Hindman, haosdent huang, James Peach, and 
Michael Park.


Changes
---

Addressed review comments.

Added benchmark.
Before changes:
Movable elapsed: 7.763873472secs
Copyable elapsed: 8.20426283secs

After changes:
Movable elapsed: 4.749548906secs
Copyable elapsed: 6.266501858secs


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


Repository: mesos


Description
---

This reduces number of copies made for each parameter in a code like this:
future.then(defer(pid, &SomeProcess::someMethod, param1, param2));

For the objects that do not support move semantics (e.g. protobuf messages), 
number of copies is reduced from 8-10 to 6. If move semantics is supported, 
then number of copies is reduced from 6-7 to 1 if parameter is passed with 
std::move, or 2 otherwise.


Diffs (updated)
-

  3rdparty/libprocess/include/process/defer.hpp 
7f3369e723cb244e97930621cbba89cf7873567d 
  3rdparty/libprocess/include/process/deferred.hpp 
e446621be11ac51f5f91c417cc8975e363c2f715 
  3rdparty/libprocess/include/process/dispatch.hpp 
3a0793888dc0df5e3ec31b06f47cd920c71e0db9 
  3rdparty/libprocess/include/process/future.hpp 
cce950509f58022e79bb51a6e72ea1a005b9cb50 
  3rdparty/libprocess/include/process/http.hpp 
f637999174d92a98208b5fc49a65f9929efb11a0 
  3rdparty/libprocess/src/tests/benchmarks.cpp 
694a842e8e18d82ac551749a71764825ba7cb3a9 


Diff: https://reviews.apache.org/r/60003/diff/3/

Changes: https://reviews.apache.org/r/60003/diff/2-3/


Testing
---

make check

Number of copies was checked by using defer to subscribe process for Future 
callbacks, and passing parameters that count number of copies made.


Thanks,

Dmitry Zhuk

Re: Review Request 61124: Push metric history entries with their timestamp.

[Benjamin Mahler]

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


Fix it, then Ship it!





3rdparty/libprocess/include/process/metrics/metric.hpp
Line 63 (original), 63 (patched)


Can this just be:

```
void push(double value, Time now = Clock::now())
```

?


- Benjamin Mahler


On July 25, 2017, 11:26 p.m., James Peach wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61124/
> ---
> 
> (Updated July 25, 2017, 11:26 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler, Kevin Klues, and Jiang Yan Xu.
> 
> 
> Bugs: MESOS-6918
> https://issues.apache.org/jira/browse/MESOS-6918
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In the Timer metric, we already have a current timestamp when
> we push the sample into the history buffer. Added an optional
> timestamp to Metric::push() so that we can avoid unnecessary
> overhead of calling Clock::now() in the common case of Timer
> metrics.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/include/process/metrics/metric.hpp 
> 21f162d5b7d9e56dc3289d65b6d86deb4c2fa721 
>   3rdparty/libprocess/include/process/metrics/timer.hpp 
> 0a9c0227c457c6c81a59f65f901a5464ee00983d 
> 
> 
> Diff: https://reviews.apache.org/r/61124/diff/1/
> 
> 
> Testing
> ---
> 
> make check (Fedora 25)
> 
> 
> Thanks,
> 
> James Peach
> 
>

Re: Review Request 61193: Convert resource format for completed frameworks.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On July 27, 2017, 2:57 p.m., Jiang Yan Xu wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61193/
> ---
> 
> (Updated July 27, 2017, 2:57 p.m.)
> 
> 
> Review request for mesos, Adam B and Michael Park.
> 
> 
> Bugs: MESOS-7831
> https://issues.apache.org/jira/browse/MESOS-7831
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This patch is similar to 4ec820709. We need to convert all resources
> from old agents without reservation-refinement capability, including
> task resources from completed frameworks.
> 
> 
> Diffs
> -
> 
>   src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32 
> 
> 
> Diff: https://reviews.apache.org/r/61193/diff/1/
> 
> 
> Testing
> ---
> 
> make check and tested in a testing cluster.
> 
> 
> Thanks,
> 
> Jiang Yan Xu
> 
>

Re: Review Request 60934: Implemented blkio subsystem usage() for resource statistics.

[James Peach]

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


Fix it, then Ship it!





src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp
Lines 82 (patched)


IMHO it is better to omit the `default` case since then the compiler will 
warn when new enum values need to be handled.



src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp
Lines 357 (patched)


In the previous patches we omitted the scoping operator on `major` and 
`minor` for portability reasons (ie. they might be pure macros).

Here and below.


- James Peach


On July 30, 2017, midnight, Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60934/
> ---
> 
> (Updated July 30, 2017, midnight)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Implemented blkio subsystem usage() for resource statistics.
> 
> 
> Diffs
> -
> 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.hpp 
> a2c575cc87a9e08612cf417013dac76ad6de873b 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/blkio.cpp 
> 6be0f9ed4aa8c1a2273e5808ad54d3a4922c5e8d 
> 
> 
> Diff: https://reviews.apache.org/r/60934/diff/4/
> 
> 
> Testing
> ---
> 
> make check
> 
> Tested with `mesos-execute` and verified that the blkio statistics can be 
> collected from the resource statistics endpoint:
> 
> Start the master:
> sudo ./bin/mesos-master.sh --ip=127.0.0.1 --work_dir=/var/lib/mesos
> 
> Start the agent:
> sudo GLOG_v=1 ./bin/mesos-agent.sh --master=127.0.0.1:5050 
> --isolation=cgroups/blkio,docker/runtime,filesystem/linux --work_dir=/tmp 
> --image_providers=docker --executor_environment_variables="{}"
> 
> Launch `mesos-execute` test framework:
> sudo ./src/mesos-execute --master=127.0.0.1:5050 --name=test 
> --docker_image=alpine --shell=true --command="while true ; do echo 'hello' > 
> test.txt ; done"
> 
> Collect the statistics for blkio:
> ```
> vagrant@vagrant-ubuntu-wily-64:~$ curl localhost:5051/monitor/statistics.json 
> | python -m json.tool
>   % Total% Received % Xferd  Average Speed   TimeTime Time  
> Current
>  Dload  Upload   Total   SpentLeft  Speed
> 100  1247  100  12470 0  39390  0 --:--:-- --:--:-- --:--:-- 40225
> [
> {
> "executor_id": "test",
> "executor_name": "Command Executor (Task: test) (Command: sh -c 
> 'while true ;...')",
> "framework_id": "39fb6d5c-d5bd-4b18-a632-0a42e417b946-",
> "source": "test",
> "statistics": {
> "blkio": {
> "cfq": [
> {
> "io_merged": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_queued": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_service_bytes": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_service_time": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_serviced": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_wait_time": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ]
> }
> ],
> "cfq_recursive": [
> {
> "io_merged": [
> {
> "op": "TOTAL",
> "value": 0
> }
> ],
> "io_queued": [
>

Re: Review Request 60057: Made explicit that we dispatch to a process manager.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On July 31, 2017, 5:58 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60057/
> ---
> 
> (Updated July 31, 2017, 5:58 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This member function of 'ProcessManager' was capturing a 'this'
> pointer when dispatching to itself, but did not properly use 'defer'
> or 'dispatch'. While this pattern is usually suspect, it was safe here
> as we can be sure that the process manager lives long enough to safely
> invoke the created callback.
> 
> This patch removes the capture of 'this' and instead explicitly
> references the 'static process_manager' in the created callback to
> signal that we rely on external invariants.
> 
> 
> Diffs
> -
> 
>   3rdparty/libprocess/src/process.cpp 
> 44c3531538691dd9600b9169a83f4fd504f4f089 
> 
> 
> Diff: https://reviews.apache.org/r/60057/diff/2/
> 
> 
> Testing
> ---
> 
> * `make check` (Fedora 25 clang-trunk w/ optimizations)
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 61253: Fixed mesos-execute help text related to tasks.

[Michael Park]

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


Ship it!




Ship It!

- Michael Park


On July 31, 2017, 5:48 a.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61253/
> ---
> 
> (Updated July 31, 2017, 5:48 a.m.)
> 
> 
> Review request for mesos and Michael Park.
> 
> 
> Bugs: MESOS-7805
> https://issues.apache.org/jira/browse/MESOS-7805
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> This commit adjusts task-related help texts for mesos-execute.
> 
> 
> Diffs
> -
> 
>   src/cli/execute.cpp 670a516a206882ab2f066e98432e57c7bc209754 
> 
> 
> Diff: https://reviews.apache.org/r/61253/diff/1/
> 
> 
> Testing
> ---
> 
> Verified that task and taskgroup from help text works against a matching 
> master.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Review Request 61262: Added 'heartbeat' event for the operator API.

[Quinn Leng]

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

Review request for mesos, Anand Mazumdar and Greg Mann.


Repository: mesos


Description
---

Added the 'HEARTBEAT' event for the operator API, modified other
related test cases to accept heartbeats.


Diffs
-

  include/mesos/master/master.proto 7a722a6deac484f28a85f63fd68ef49cb6a47606 
  include/mesos/v1/master/master.proto cf88ea64fa08e4bdcf685de4def28d374eb32fdf 
  src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
  src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
  src/master/master.cpp e12c997dad04f8a4ddb47a993a84b2b05c9e2f32 
  src/tests/api_tests.cpp f22ca28c819712d8797e0c0c69dc1ebf1fe5ac1f 


Diff: https://reviews.apache.org/r/61262/diff/1/


Testing
---

make check -j48


Thanks,

Quinn Leng

Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when 'ResourceProviderManager' updates.

[Benjamin Bannier]

> On July 29, 2017, 12:20 a.m., Jie Yu wrote:
> > src/slave/slave.cpp
> > Lines 6496 (patched)
> > 
> >
> > I think we can still process `UPDATE_TOTAL_RESOURCES` message in 
> > DISCONNECTED state by updating slave's internal data structure about total 
> > resources. We just need to send that new total to the master once the slave 
> > is in RUNNING state.
> > 
> > However, we should only send the message to the master if slave is in 
> > RUNNING state. This logic is indeed very similar to the oversubscription 
> > logic. Take a look at the logics there.

Good point about being able to consume the message at least on the agent side 
-- I have updated the code.

I also now use the same log message when an update is send, regardless on 
whether the message was generated in direct message handling or on 
reregistrated.

I added an assertion for `state == RUNNING` to the sending in reregistered 
agent state; as we do not periodically poll the resource provider manager we 
rely on the fact that every update reaches the master. This is different for 
oversubscription where we poll the resource estimator periodically.


- Benjamin


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


On July 31, 2017, 5:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61183/
> ---
> 
> (Updated July 31, 2017, 5:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> The agent's resource provider manager sends a
> 'ResourceProviderMessage' when its managed resources change. This
> commit adds handling in the agent so that an 'UpdateSlaveMessage' is
> sent to the master to update the total resource available on the
> agent. We also store this total in the agent memory so that it can be
> resent on agent resubscription.
> 
> In order to provide push-like handling of the resource provider
> manager's message queue, we chain recursive calls to the handler for
> continuous processing. Initially, processing is kicked off from
> 'Slave::initialize'. In this simple implementation we e.g., provide no
> direct way to stop processing of messages, yet, but it can be achieved
> by e.g., replacing the manager with a new instance (this would also
> require updating routes).
> 
> Since the agent can only send an 'UpdateSlaveMessage' when it is
> registered with a master, a simple back-off of 5 s is implemented which
> will defer processing of a ready message should the agent not yet have
> registered.
> 
> To facilitate logging we add a stringification function for
> 'ResourceProviderMessage's.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
>   src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
>   src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
>   src/tests/slave_tests.cpp a089cc47eae41cd6baeffd3f4a7ee7c7984aacbd 
> 
> 
> Diff: https://reviews.apache.org/r/61183/diff/2/
> 
> 
> Testing
> ---
> 
> `make check`
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60933: Added stats/control helpers for the Blkio cgroup subsystem.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On July 29, 2017, 11:59 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60933/
> ---
> 
> (Updated July 29, 2017, 11:59 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, James Peach, 
> Qian Zhang, and Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> - Data structure for Blkio entities
> - Stats helpers for blkio.throttle.io* (generic blkio stats)
> - Stats helpers for blkio.io* (CFQ related stats)
> - Comments from the kernel blkio doc for helper functions
> 
> 
> Diffs
> -
> 
>   src/linux/cgroups.hpp eaf0dcad0ed38c507564624f1647e0c731b8b433 
>   src/linux/cgroups.cpp 334005abfc4ec9b20b7dc0212d852ba1f505dbb5 
> 
> 
> Diff: https://reviews.apache.org/r/60933/diff/6/
> 
> 
> Testing
> ---
> 
> make check
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 61181: Stored subscribed resources in resource provider manager.

[Benjamin Bannier]

> On July 28, 2017, 11:42 p.m., Jie Yu wrote:
> > src/resource_provider/manager.cpp
> > Lines 270-274 (patched)
> > 
> >
> > That makes me think if it's easier to always rely on `Update` to report 
> > the total resources of a resource provider. `Subscribe` will just register 
> > with the manager with `ResourceProviderInfo`. The resource provider will 
> > send an update to the manager after subscription is successful?
> > 
> > In that way, we don't have to worry about the ID injection in the 
> > manager any more (we do need to do validation!)

Yes, I believe that would make sense. I would however suggest to not tackle 
this right now, and instead defer this to the time where we have an update call 
available as this protocol requires e.g., slightly more advanced testing tools 
as one would need to maintain a connection (e.g., a proper 
`MockResourceProvider` to maintain connections). The change should be 
non-intrusive as we'd be in control of manager as well as resource providers at 
that point.

I added a `TODO` to https://reviews.apache.org/r/61180/ for now.


- Benjamin


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


On July 31, 2017, 5:08 p.m., Benjamin Bannier wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61181/
> ---
> 
> (Updated July 31, 2017, 5:08 p.m.)
> 
> 
> Review request for mesos, Jie Yu and Jan Schlicht.
> 
> 
> Bugs: MESOS-7837
> https://issues.apache.org/jira/browse/MESOS-7837
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> In order to be able to always communicate the aggregated total
> resources available on all subscribed resource providers, a resource
> provider manager needs to keep track of the resources of all
> subscribed resource providers. This commit adds a field for that the
> manager's internal data structures for that purpose.
> 
> To make assigned 'ResourceProviderID's opaque to users of managers, the
> manager assigns provider ids to all resources added.
> 
> 
> Diffs
> -
> 
>   src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 
> 
> 
> Diff: https://reviews.apache.org/r/61181/diff/2/
> 
> 
> Testing
> ---
> 
> Tested as part of https://reviews.apache.org/r/61182/.
> 
> 
> Thanks,
> 
> Benjamin Bannier
> 
>

Re: Review Request 60558: Set container DNS with `--default_container_dns` in Docker executor.

[Avinash sridharan]

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




src/docker/docker.cpp
Lines 755-765 (patched)


This looks really complicated with the `wildcard entry` thrown into play. 
Can I suggest storing the DNS entries into a private variable within `Docker` 
on the following lines?

Option bridgeDNS;
hashmap userDNS;

For wildcard we use the key as "*" in the userDNS network (or you can keep 
an explicit variable for user nertworks. 

If we have these variables and populate them from the flags passed to 
docker executor, the code here becomes relatively simple which will be 
something along the lines of:

if (dockerInfo.network() == ContainerInfo::DocckerInfo::BRIDGE && 
bridgeDNS.isSome() {
  
} else if (dockerInfo.network() == ContainerInfo::DockerInfo::USER && 
userDNS.contains(options.network.get()) {
  ...
} else if (dockerInfo.network() == ContainerInfo::DockerInfo::USER && 
userDNS.contains("*") {
}



src/docker/docker.cpp
Lines 770 (patched)


s/apples/applies?



src/docker/docker.cpp
Lines 783-793 (patched)


We should convert this into a lambda and then invoke the lambda in the `if` 
and the `else if` conditional below. Will avoid code duplication.


- Avinash sridharan


On July 28, 2017, 2:32 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60558/
> ---
> 
> (Updated July 28, 2017, 2:32 p.m.)
> 
> 
> Review request for mesos, Avinash sridharan and Jie Yu.
> 
> 
> Bugs: MESOS-7709
> https://issues.apache.org/jira/browse/MESOS-7709
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Set container DNS with `--default_container_dns` in Docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 5593cb635e073334c6c2566be3d803cd7febb1c3 
>   src/docker/docker.cpp 8081c0203bf62cf62aa3b93d745f0e829ad65509 
>   src/docker/executor.cpp e03f24461ec7b19cacae79c020406f0a475f2d19 
> 
> 
> Diff: https://reviews.apache.org/r/60558/diff/6/
> 
> 
> Testing
> ---
> 
> sudo make check
> 
> 1. Start Mesos master.
> ```
> $ sudo ./bin/mesos-master.sh --work_dir=/opt/mesos
> ```
> 
> 2. Start Mesos agent.
> ```
> $ sudo ./bin/mesos-slave.sh --master=192.168.122.216:5050 
> --containerizers=mesos,docker --image_providers=docker 
> --image_provisioner_backend=aufs 
> --isolation=filesystem/linux,docker/runtime,network/cni,cgroups/cpu,cgroups/mem
>  --network_cni_config_dir=/opt/cni/net_configs 
> --network_cni_plugins_dir=/opt/cni/plugins --work_dir=/opt/mesos 
> --docker_store_dir=/opt/mesos/store/docker 
> --executor_registration_timeout=60mins 
> --default_container_dns=file:///home/stack/dns.json
> 
> $ cat /home/stack/dns.json
> {
>   "mesos": [
> {
>   "network_mode": "CNI",
>   "network_name": "net1",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ]
>   }
> }
>   ],
>   "docker": [
> {
>   "network_mode": "BRIDGE",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ],
> "search": [ "xxx.com", "yyy.com" ],
> "options": [ "timeout:3", "attempts:2" ]
>   }
> },
> {
>   "network_mode": "USER",
>   "network_name": "net2",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ]
>   }
> }
>   ]
> }
> ```
> 
> 3. Launch a Docker container with `mesos-execute`.
> ```
> $ sudo src/mesos-execute --master=192.168.122.216:5050 
> --task=file:///home/stack/task-docker.json
> 
> $cat /home/stack/task-docker.json 
> {
>   "name": "test",
>   "task_id": {"value" : "test"},
>   "agent_id": {"value" : ""},
>   "resources": [
> {
>   "name": "cpus",
>   "type": "SCALAR",
>   "scalar": {
> "value": 0.1
>   }
> },
> {
>   "name": "mem",
>   "type": "SCALAR",
>   "scalar": {
> "value": 32
>   }
> }
>   ],
>   "command": {
> "shell": false
>   },
>   "container": {
> "type": "DOCKER",
> "docker": {
>   "image": "nginx",
>   "network": "BRIDGE"
> }
>   }
> }
> ```
> 
> 4. Check the DNS configuration of the Docker container.
> ```
> $ docker ps 
> CONTAINER IDIMAGE   COMMAND  CREATED  
> STATUS  PORTS   NAMES
> ca642bf31a9fnginx   "nginx -g 'daemon off"   About a 
> minute ago   Up About a minute   80/tcp, 443/tcp 
> mesos-1d48fc32-a138-4c31-a5a9-fd7d279231da
> 
> $ docker exec ca642bf31a9f cat /etc/resolv.conf 
> search xxx.com yyy.c

Re: Review Request 60991: Changed Device::path to optional and introduced Device::Number.

[James Peach]

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


Ship it!




Ship It!

- James Peach


On July 26, 2017, 1:01 a.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60991/
> ---
> 
> (Updated July 26, 2017, 1:01 a.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhongbo Tian.
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A device in cgroup can be represented as either a path or a major:minor
> number. Need to change the `required` Device::path to `optional` to
> add Number in the device protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 8f8079bd7c2de4e8b2f8f9a56e2731b77b8e1575 
>   include/mesos/v1/mesos.proto 720f307f8d738b0787e7c47be7ee15be38b2c0d0 
>   src/slave/containerizer/mesos/isolators/cgroups/subsystems/devices.cpp 
> ddf2a4d661001a3a5832c21504420223ca60a753 
> 
> 
> Diff: https://reviews.apache.org/r/60991/diff/2/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Re: Review Request 60932: Added protobuf scheme for blkio subsystem in CgroupInfo.

[James Peach]

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


Fix it, then Ship it!





include/mesos/mesos.proto
Line 2918 (original), 2918 (patched)


Does this comment need updating now?



include/mesos/v1/mesos.proto
Line 2901 (original), 2901 (patched)


Does this comment need updating now?


- James Peach


On July 29, 2017, 11:58 p.m., Gilbert Song wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60932/
> ---
> 
> (Updated July 29, 2017, 11:58 p.m.)
> 
> 
> Review request for mesos, haosdent huang, Jason Lai, Jie Yu, Qian Zhang, and 
> Zhitao Li.
> 
> 
> Bugs: MESOS-6162
> https://issues.apache.org/jira/browse/MESOS-6162
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Only statistics information for blkio in protobuf.
> 
> 
> Diffs
> -
> 
>   include/mesos/mesos.proto 41e42b4996235cbee26f580f4a7aa2daed166b7f 
>   include/mesos/v1/mesos.proto 9de282f85b8d0ba91fa3de85acd5d0f4082a47d8 
> 
> 
> Diff: https://reviews.apache.org/r/60932/diff/6/
> 
> 
> Testing
> ---
> 
> make
> 
> 
> Thanks,
> 
> Gilbert Song
> 
>

Review Request 61261: Documented agent garbage collection metrics.

[James Peach]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Documented the following agent sandbox garbage collection metrics:
  - slave/gc_path_removals_total
  - slave/gc_path_removals_failed
  - slave/gc_path_removals_scheduled


Diffs
-

  docs/monitoring.md d1f64d46b22e79c16a680a2e7fff9a01202c5457 


Diff: https://reviews.apache.org/r/61261/diff/1/


Testing
---

None.


Thanks,

James Peach

Review Request 61260: Added agent garbage collection metrics.

[James Peach]

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

Review request for mesos and Jiang Yan Xu.


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


Repository: mesos


Description
---

Added some basic sandbox garbage collection metrics to track the number
of paths removals, the number of failed path removal and the number of
paths that are still scheduled for removal.


Diffs
-

  src/slave/gc.cpp f270fa47b22904e926f4bd4b0a7693a41b2c8271 
  src/slave/gc_process.hpp PRE-CREATION 
  src/tests/gc_tests.cpp d4daad3993bb9a92db2c7add6e74f12aeb71d535 


Diff: https://reviews.apache.org/r/61260/diff/1/


Testing
---

make check (Fedora 26).


Thanks,

James Peach

Re: Review Request 61183: Triggered 'UpdateSlaveMessage' when 'ResourceProviderManager' updates.

[Benjamin Bannier]

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

(Updated July 31, 2017, 5:08 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Addressed comments from Jie.


Repository: mesos


Description
---

The agent's resource provider manager sends a
'ResourceProviderMessage' when its managed resources change. This
commit adds handling in the agent so that an 'UpdateSlaveMessage' is
sent to the master to update the total resource available on the
agent. We also store this total in the agent memory so that it can be
resent on agent resubscription.

In order to provide push-like handling of the resource provider
manager's message queue, we chain recursive calls to the handler for
continuous processing. Initially, processing is kicked off from
'Slave::initialize'. In this simple implementation we e.g., provide no
direct way to stop processing of messages, yet, but it can be achieved
by e.g., replacing the manager with a new instance (this would also
require updating routes).

Since the agent can only send an 'UpdateSlaveMessage' when it is
registered with a master, a simple back-off of 5 s is implemented which
will defer processing of a ready message should the agent not yet have
registered.

To facilitate logging we add a stringification function for
'ResourceProviderMessage's.


Diffs (updated)
-

  src/resource_provider/message.hpp 916926bf278de9ed919384a82a452d3ced06bd04 
  src/slave/slave.hpp 1fe93dab1b2bef24721cc1bcffebe1b259e96d79 
  src/slave/slave.cpp 7381530515f86faf4c3e8f82bcd9483f6cf0498b 
  src/tests/slave_tests.cpp a089cc47eae41cd6baeffd3f4a7ee7c7984aacbd 


Diff: https://reviews.apache.org/r/61183/diff/2/

Changes: https://reviews.apache.org/r/61183/diff/1-2/


Testing
---

`make check`


Thanks,

Benjamin Bannier

Re: Review Request 61179: Added 'devolve' overload for 'ResourceProviderID'.

[Benjamin Bannier]

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

(Updated July 31, 2017, 5:08 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Reordered decls as suggested by Jie.


Repository: mesos


Description
---

This commit adds the default overload to 'devolve'
'ResourceProviderID's.


Diffs (updated)
-

  src/internal/devolve.hpp 656173de0a652431be3c3c3d3796e3f3f7fca278 
  src/internal/devolve.cpp 7eb58e94beb5262a63e2f9b9dfefbd3d288e32ec 


Diff: https://reviews.apache.org/r/61179/diff/2/

Changes: https://reviews.apache.org/r/61179/diff/1-2/


Testing
---

Tested as part of https://reviews.apache.org/r/61182/.


Thanks,

Benjamin Bannier

Re: Review Request 61181: Stored subscribed resources in resource provider manager.

[Benjamin Bannier]

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

(Updated July 31, 2017, 5:08 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Used the correct loop.


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


Repository: mesos


Description
---

In order to be able to always communicate the aggregated total
resources available on all subscribed resource providers, a resource
provider manager needs to keep track of the resources of all
subscribed resource providers. This commit adds a field for that the
manager's internal data structures for that purpose.

To make assigned 'ResourceProviderID's opaque to users of managers, the
manager assigns provider ids to all resources added.


Diffs (updated)
-

  src/resource_provider/manager.cpp 44e1576d4462295d404429d51682134306047462 


Diff: https://reviews.apache.org/r/61181/diff/2/

Changes: https://reviews.apache.org/r/61181/diff/1-2/


Testing
---

Tested as part of https://reviews.apache.org/r/61182/.


Thanks,

Benjamin Bannier

Re: Review Request 61180: Added a resource providers total resources to the subscribe call.

[Benjamin Bannier]

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

(Updated July 31, 2017, 5:08 p.m.)


Review request for mesos, Jie Yu and Jan Schlicht.


Changes
---

Added a `TODO`.


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


Repository: mesos


Description
---

Since resource provider resources are dynamic (as opposed to how e.g.,
agent total resources are implemented), they are not part of the
'ResourceProviderInfo'. Instead they are communicated explicitly.

This commit adds total resources the resource provider 'SUBSCRIBE'
call which can be used to communicate the total capacity in both
subscription and resubscription scenarios.


Diffs (updated)
-

  include/mesos/resource_provider/resource_provider.proto 
a8a27ed09cde7a9f137e76900a3a1c1a547752ed 
  include/mesos/v1/resource_provider/resource_provider.proto 
34bce7511bc74e157277371a7f46111c9537bcf2 


Diff: https://reviews.apache.org/r/61180/diff/2/

Changes: https://reviews.apache.org/r/61180/diff/1-2/


Testing
---

Tested as part of https://reviews.apache.org/r/61182/.


Thanks,

Benjamin Bannier

Review Request 61258: Graduated HTTP and TCP health checks.

[Alexander Rukletsov]

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

Review request for mesos, Anand Mazumdar, Gastón Kleiman, haosdent huang, and 
Kapil Arya.


Repository: mesos


Description
---

See summary.


Diffs
-

  CHANGELOG e091fec10296090123986ca44a86645e5867aa0d 


Diff: https://reviews.apache.org/r/61258/diff/1/


Testing
---

None: not a functional change.


Thanks,

Alexander Rukletsov

Re: Review Request 60438: Updated endpoint help generator script to work inside Docker.

[Benjamin Bannier]

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


Ship it!




Ship It!

- Benjamin Bannier


On July 27, 2017, 2:27 a.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60438/
> ---
> 
> (Updated July 27, 2017, 2:27 a.m.)
> 
> 
> Review request for mesos, Benjamin Bannier, Benjamin Mahler, and haosdent 
> huang.
> 
> 
> Bugs: MESOS-7625
> https://issues.apache.org/jira/browse/MESOS-7625
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Changed agent flags to make this script work inside Docker container.
> This is needed because this script will be run as part of website
> publishing process which runs on ASF CI inside Docker.
> 
> 
> Diffs
> -
> 
>   support/generate-endpoint-help.py 19cff57810863a5050db73647348c28858a9499e 
> 
> 
> Diff: https://reviews.apache.org/r/60438/diff/2/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Review Request 61257: Added full authz for non summarized fields of `/slaves` endpoint.

[Alexander Rojas]

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

Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff.


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


Repository: mesos


Description
---

Fields were authorized based on partial elements of each
resource. Moreover, some fields which required authorization were not
being authorized at all. This patch enables full authorization of all
fields.


Diffs
-

  src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 


Diff: https://reviews.apache.org/r/61257/diff/1/


Testing
---

```shell
make check
```


Thanks,

Alexander Rojas

Re: Review Request 61171: Enabled filtering of the 'GET_AGENTS' v1 API call.

[Alexander Rojas]

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

(Updated July 31, 2017, 3:50 p.m.)


Review request for mesos, Adam B, Greg Mann, Quinn Leng, and Till Toenshoff.


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


Repository: mesos


Description
---

Enables filtering of the results of calls to the 'GET_AGENTS' v1
API. It filters the contents of different resources entries based
on the 'VIEW_ROLE' permissions of the principal doing the request
based on resource roles, allocation roles and reservations.


Diffs (updated)
-

  src/common/http.hpp ba8dda18a02f51d1a28e719f06ee4b51573dfbec 
  src/common/http.cpp dfd5f335e8a3745d047d4f9f5e8c821b2c22da5a 
  src/common/protobuf_utils.hpp 80d2edd452f3ffa38c40f9a21f8489799065c401 
  src/common/protobuf_utils.cpp 49d3a229925f4aa107e3e5f762936c16318aeadb 
  src/master/http.cpp 9df086c417a9392f62d600c7a6486be0a1cf7e70 
  src/master/master.hpp 84465af782d4024f22463d981ef9d0ef7827d043 
  src/tests/api_tests.cpp 1d5b080c809248bdf4c76ddad382d714692c804b 


Diff: https://reviews.apache.org/r/61171/diff/4/

Changes: https://reviews.apache.org/r/61171/diff/3-4/


Testing
---

```shell
make check
```

Manual test:

```shell
mkdir -p /tmp/mesos/master
mkdir -p /tmp/mesos/agent

# Create credentials
cat < /tmp/mesos/credentials.txt
hal-9000 dave
glados potato
skynet connor
EOF

# Create ACLs
cat < /tmp/mesos/acls.json
{
  "permissive": true,
  "view_roles" : [
   {
 "principals" : { "type" : "ANY" },
 "roles" : { "values" : ["*"] }
   },
   {
 "principals" : { "values" : ["hal-9000"] },
 "roles" : { "values" : ["space-odyssey"] }
   },
   {
 "principals" : { "values" : ["hal-9000"] },
 "roles" : { "type" : "NONE" }
   },
   {
 "principals" : { "values" : ["glados"] },
 "roles" : { "values" : ["portal"] }
   },
   {
 "principals" : { "values" : ["glados"] },
 "roles" : { "type" : "NONE" }
   },
   {
 "principals" : { "values" : ["skynet"] },
 "roles" : { "values" : ["terminator"] }
   },
   {
 "principals" : { "values" : ["skynet"] },
 "roles" : { "type" : "NONE" }
   }
  ]
}
EOF

# Launch Master with some predefined roles.
./bin/mesos-master.sh \
--work_dir=/tmp/mesos/master \
--log_dir=/tmp/mesos/master/log \
--authenticate_http \
--credentials=/tmp/mesos/credentials.txt \
--authenticate_http_frameworks \
--http_framework_authenticators=basic \
--http_authenticators=basic \
--authenticate_http_readonly \
--acls=/tmp/mesos/acls.json \
--roles="space-odyssey,portal,terminator" &

# Launch Agent with static reservations for all roles.
sudo ./bin/mesos-agent.sh \
--master=127.0.0.1:5050 \
--work_dir=/tmp/mesos/agent \
--authenticate_http_readwrite \
--http_authenticators=basic \
--http_credentials=/tmp/mesos/credentials.txt \
--acls=/tmp/mesos/acls.json \

--resources='cpus(space-odyssey):2;cpus(portal):2;cpus(*):4;mem(space-odyssey):250;mem(portal):250;mem(*):10360;ports(space-odyssey):[31000-32000];ports(portal):[32001-33000];ports(*):[33001-35000];disk(space-odyssey):250;disk(portal):250;disk(*):1000'
 &

# Launch test framework.
./src/mesos-execute \
--master=127.0.0.1:5050 \
--command='while true; do echo "Hello World"; sleep 5; done;' \
--resources="cpus:1;mem:128;disk:32;ports:[31002-31003]" \
--role=space-odyssey \
--name=hello-discovery \
--principal=hal-9000 \
--secret=dave &

# Create a dynamic reservation.
cat > /tmp/resources.json < /tmp/quota.json <

Re: Review Request 61112: Added uris flag to balloon framework.

[Alexander Rukletsov]

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




src/examples/balloon_framework.cpp
Lines 20 (patched)


Do you still need this?



src/examples/balloon_framework.cpp
Lines 26 (patched)


Why do you need this one?



src/examples/balloon_framework.cpp
Lines 55 (patched)


Don't need this anymore?



src/examples/balloon_framework.cpp
Lines 127 (patched)


Let's call it `--executor_uris` for calrity.



src/examples/balloon_framework.cpp
Lines 132 (patched)


s\URI\CommandInfo::URI because `URI` is not a top level message.



src/examples/balloon_framework.cpp
Lines 498-504 (original), 529-551 (patched)


I'd suggest to use either `--executor_uri` or new `--uris` flag. Let's 
update comments for both flags to mention this, add a `CHECK` above that only a 
single one is set, and leave a deprecation `TODO` for the former flag.



src/examples/balloon_framework.cpp
Lines 540 (patched)


Indent is off.



src/examples/balloon_framework.cpp
Lines 543-545 (patched)


`EXIT(EXIT_FAILURE)` instead?



src/examples/balloon_framework.cpp
Lines 548-550 (patched)


Can we use `CopyFrom` from `RepeatedPtrField` here?


- Alexander Rukletsov


On July 31, 2017, 1:01 p.m., Armand Grillet wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61112/
> ---
> 
> (Updated July 31, 2017, 1:01 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Alexander Rojas.
> 
> 
> Bugs: MESOS-7814
> https://issues.apache.org/jira/browse/MESOS-7814
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Allows to set URIs that will be fetched before running the executor.
> 
> 
> Diffs
> -
> 
>   src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 
> 
> 
> Diff: https://reviews.apache.org/r/61112/diff/2/
> 
> 
> Testing
> ---
> 
> $ make check
> 
> 
> Thanks,
> 
> Armand Grillet
> 
>

Re: Review Request 61112: Added uris flag to balloon framework.

[Armand Grillet]

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

(Updated July 31, 2017, 1:01 p.m.)


Review request for mesos, Alexander Rukletsov and Alexander Rojas.


Changes
---

Suggestion applied.


Summary (updated)
-

Added uris flag to balloon framework.


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


Repository: mesos


Description (updated)
---

Allows to set URIs that will be fetched before running the executor.


Diffs (updated)
-

  src/examples/balloon_framework.cpp b8c8e387bd8592bc1d27ff62fa9fd5397ad71fb9 


Diff: https://reviews.apache.org/r/61112/diff/2/

Changes: https://reviews.apache.org/r/61112/diff/1-2/


Testing
---

$ make check


Thanks,

Armand Grillet

Re: Review Request 60057: Made explicit that we dispatch to a process manager.

[Benjamin Bannier]

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

(Updated July 31, 2017, 2:58 p.m.)


Review request for mesos and Michael Park.


Changes
---

Rebased.


Repository: mesos


Description
---

This member function of 'ProcessManager' was capturing a 'this'
pointer when dispatching to itself, but did not properly use 'defer'
or 'dispatch'. While this pattern is usually suspect, it was safe here
as we can be sure that the process manager lives long enough to safely
invoke the created callback.

This patch removes the capture of 'this' and instead explicitly
references the 'static process_manager' in the created callback to
signal that we rely on external invariants.


Diffs (updated)
-

  3rdparty/libprocess/src/process.cpp 44c3531538691dd9600b9169a83f4fd504f4f089 


Diff: https://reviews.apache.org/r/60057/diff/2/

Changes: https://reviews.apache.org/r/60057/diff/1-2/


Testing
---

* `make check` (Fedora 25 clang-trunk w/ optimizations)


Thanks,

Benjamin Bannier

Review Request 61253: Fixed mesos-execute help text related to tasks.

[Benjamin Bannier]

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

Review request for mesos and Michael Park.


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


Repository: mesos


Description
---

This commit adjusts task-related help texts for mesos-execute.


Diffs
-

  src/cli/execute.cpp 670a516a206882ab2f066e98432e57c7bc209754 


Diff: https://reviews.apache.org/r/61253/diff/1/


Testing
---

Verified that task and taskgroup from help text works against a matching master.


Thanks,

Benjamin Bannier