date:20170704

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

2017-07-04 Thread Avinash sridharan



> On July 3, 2017, 6:35 p.m., Avinash sridharan wrote:
> > src/docker/docker.hpp
> > Lines 167 (patched)
> > 
> >
> > For consistency can we use `defautlContainerDNS`?
> 
> Qian Zhang wrote:
> Originally I used `defautlContainerDNS`, but that will make this line 
> exceeding 80 chars :-( I think it also makes sense to use `defaultDNS` since 
> here we know we are creating run options for Docker container, so I think we 
> do not have to mention `container` in the parameter name.

You can use this `// NOLINT(whitespace/line_length)` to create exceptions? In 
`executor.cpp` you use `defaultContainerDNS` so seemed a bit odd.


- Avinash


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


On June 30, 2017, 7:09 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60558/
> ---
> 
> (Updated June 30, 2017, 7:09 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
> ---
> 
> Set container DNS with `--default_container_dns` in Docker executor.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 5593cb635e073334c6c2566be3d803cd7febb1c3 
>   src/docker/docker.cpp 8ca0c68836ea5d1a1186e79942f6daccf6c6da46 
>   src/docker/executor.cpp e03f24461ec7b19cacae79c020406f0a475f2d19 
> 
> 
> Diff: https://reviews.apache.org/r/60558/diff/1/
> 
> 
> 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": "net1",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ]
>   }
> }
>   ],
>   "docker": [
> {
>   "network": "bridge",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ],
> "search": [ "xxx.com", "yyy.com" ],
> "options": [ "timeout:3", "attempts:2" ]
>   }
> }
>   ]
> }
> ```
> 
> 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.com
> nameserver 8.8.8.8
> nameserver 8.8.4.4
> options timeout:3 attempts:2
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

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

2017-07-04 Thread Qian Zhang



> On July 4, 2017, 2:35 a.m., Avinash sridharan wrote:
> > src/docker/docker.hpp
> > Lines 167 (patched)
> > 
> >
> > For consistency can we use `defautlContainerDNS`?

Originally I used `defautlContainerDNS`, but that will make this line exceeding 
80 chars :-( I think it also makes sense to use `defaultDNS` since here we know 
we are creating run options for Docker container, so I think we do not have to 
mention `container` in the parameter name.


- Qian


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


On June 30, 2017, 3:09 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60558/
> ---
> 
> (Updated June 30, 2017, 3:09 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 8ca0c68836ea5d1a1186e79942f6daccf6c6da46 
>   src/docker/executor.cpp e03f24461ec7b19cacae79c020406f0a475f2d19 
> 
> 
> Diff: https://reviews.apache.org/r/60558/diff/1/
> 
> 
> 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": "net1",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ]
>   }
> }
>   ],
>   "docker": [
> {
>   "network": "bridge",
>   "dns": {
> "nameservers": [ "8.8.8.8", "8.8.4.4" ],
> "search": [ "xxx.com", "yyy.com" ],
> "options": [ "timeout:3", "attempts:2" ]
>   }
> }
>   ]
> }
> ```
> 
> 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.com
> nameserver 8.8.8.8
> nameserver 8.8.4.4
> options timeout:3 attempts:2
> ```
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

2017-07-04 Thread Avinash sridharan



> On July 3, 2017, 5:22 p.m., Avinash sridharan wrote:
> > docs/configuration.md
> > Lines 1329 (patched)
> > 
> >
> > This is interesting. I think this example highlights a subtle 
> > difference between the behavior of the DNS option for CNI and CNM. For CNI 
> > networks we don't really have a concept of HOST or BRIDGE networks. 
> > However, for CNM networks since we support this option for HOST, BRIDGE and 
> > USER networks, we should be explicit about this information. Also, we need 
> > to emphasize that the `--dns` options for `HOST` and `BRIDGE` networks are 
> > supported only for docker 1.13 and above (for `USER` it has been available 
> > since 1.10). We probably want to enforce these version checks for docker.
> 
> Qian Zhang wrote:
> > However, for CNM networks since we support this option for HOST, BRIDGE 
> and USER networks, we should be explicit about this information. Also, we 
> need to emphasize that the --dns options for HOST and BRIDGE networks are 
> supported only for docker 1.13 and above.
> 
> I have a very old version of Docker (1.5.0) and I have checked its 
> `docker run` command also has the `--dns` option and it can work with 
> `BRIDGE` network but not with `HOST` network. Do you think we need to put 
> such info into this help message (I mean emphasizing the minimal Docker 
> version for `--dns` support with `HOST`, `BRIDGE` and `USER` networks 
> respectively)? That may make this message too big.
> 
> Qian Zhang wrote:
> By checking https://github.com/moby/moby/blob/master/CHANGELOG.md, I got 
> the following info:
> 1. `--dns-opt` was introduced to `docker run` in Docker 1.9.0, see 
> https://github.com/moby/moby/pull/16031.
> 2. `--dns` can be used with `HOST` network in Docker 1.12.0, see 
> https://github.com/moby/moby/pull/22408.
> 3. `--dns-option` was introduced to `docker run` in Docker 1.13.0 and 
> `--dns-opt` was hidden (but it still can be used), see 
> https://github.com/moby/moby/pull/28186.
> 
> I will do Docker version check based on the above info in 
> https://reviews.apache.org/r/60558/.

Sounds good. I think given the variations that docker supports with different 
versions I agree with you that it would be too cryptic to put this message in 
the help. Lets just add the `Docker version` check and be more explicit in our 
error messages when the version check fails.


> On July 3, 2017, 5:22 p.m., Avinash sridharan wrote:
> > src/messages/flags.proto
> > Lines 36 (patched)
> > 
> >
> > Just a thought, should we be more explicit and keep two different DNS 
> > structures for Mesos and Docker? For example, Docker has the concept of 
> > network modes where as Mesos doesn't. With this structure we would have to 
> > use some kind of pattern matching to identify the network type in case of 
> > docker, e.g., `network="host"` for host-mode networking, `network="bridge"` 
> > for bridge-mode networking. This is just a thought, I am fine with these 
> > validation checks being introduced if this seems cleaner.
> > 
> > At the very least we should definitely have a comment here explaining 
> > how the network-modes are selected for `docker` based on the network names.
> 
> Qian Zhang wrote:
> I think we do not need the network type since I will always use the 
> network name specified by framework to match `ContainerDNS.docker.network`. 
> And for sure, I will add some comments to explain how the network will be 
> matched for both CNM networks and CNI networks.

Sounds good. Thanks !!


- Avinash


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


On June 28, 2017, 2:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated June 28, 2017, 2:57 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
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 0eb696a949003ff11831aed5e4f4ab384cf9992e 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c 
>   src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

2017-07-04 Thread Qian Zhang



> On July 4, 2017, 1:22 a.m., Avinash sridharan wrote:
> > docs/configuration.md
> > Lines 1329 (patched)
> > 
> >
> > This is interesting. I think this example highlights a subtle 
> > difference between the behavior of the DNS option for CNI and CNM. For CNI 
> > networks we don't really have a concept of HOST or BRIDGE networks. 
> > However, for CNM networks since we support this option for HOST, BRIDGE and 
> > USER networks, we should be explicit about this information. Also, we need 
> > to emphasize that the `--dns` options for `HOST` and `BRIDGE` networks are 
> > supported only for docker 1.13 and above (for `USER` it has been available 
> > since 1.10). We probably want to enforce these version checks for docker.
> 
> Qian Zhang wrote:
> > However, for CNM networks since we support this option for HOST, BRIDGE 
> and USER networks, we should be explicit about this information. Also, we 
> need to emphasize that the --dns options for HOST and BRIDGE networks are 
> supported only for docker 1.13 and above.
> 
> I have a very old version of Docker (1.5.0) and I have checked its 
> `docker run` command also has the `--dns` option and it can work with 
> `BRIDGE` network but not with `HOST` network. Do you think we need to put 
> such info into this help message (I mean emphasizing the minimal Docker 
> version for `--dns` support with `HOST`, `BRIDGE` and `USER` networks 
> respectively)? That may make this message too big.

By checking https://github.com/moby/moby/blob/master/CHANGELOG.md, I got the 
following info:
1. `--dns-opt` was introduced to `docker run` in Docker 1.9.0, see 
https://github.com/moby/moby/pull/16031.
2. `--dns` can be used with `HOST` network in Docker 1.12.0, see 
https://github.com/moby/moby/pull/22408.
3. `--dns-option` was introduced to `docker run` in Docker 1.13.0 and 
`--dns-opt` was hidden (but it still can be used), see 
https://github.com/moby/moby/pull/28186.

I will do Docker version check based on the above info in 
https://reviews.apache.org/r/60558/.


- Qian


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


On June 28, 2017, 10:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated June 28, 2017, 10:57 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
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 0eb696a949003ff11831aed5e4f4ab384cf9992e 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c 
>   src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread haosdent huang


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


Fix it, then Ship it!




Ship It!


src/webui/master/static/agent.html
Lines 176 (patched)


Should we use * here to keep consistent with other parts? Beside this, 
other changes LGTM!


- haosdent huang


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/5/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread Andrei Budnik



> On July 3, 2017, 6:27 p.m., haosdent huang wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 610 (patched)
> > 
> >
> > How about
> > 
> > ```
> > $scope.agent.resource_reservations = 
> > _($scope.state.reserved_resources).map(function(role, reservation)) {
> >   reservation.role = role
> >   return reservation;
> > }
> > 
> > ```
> 
> Andrei Budnik wrote:
> This code:
> ```javascript
> $scope.agent.resource_reservations = _($scope.state.reserved_resources)
>   .map(function(role, reservation) {
> reservation.role = role;
> return reservation;
>   });
> ```
> causes an exception:
> ```
> angular-1.2.3.min.js:84 TypeError: Cannot create property 'role' on 
> string 'dyn'
> at http://127.0.0.1:5050/static/js/controllers.js:607:32
> at http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:889
> at w.each.w.forEach 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:743)
> at Function.w.map.w.collect 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:855)
> at w.(anonymous function) [as map] 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:11745)
> at http://127.0.0.1:5050/static/js/controllers.js:606:14
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:66:439
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:93:98
> ```
> 
> haosdent huang wrote:
> what's the content of $scope.state.reserved_resources in your side?
> 
> Andrei Budnik wrote:
> From `curl localhost:5051/state | jq`:
> ```javascript
>   "reserved_resources": {
> "dyn": {
>   "disk": 0,
>   "gpus": 0,
>   "mem": 1024,
>   "cpus": 2
> },
> "ads": {
>   "disk": 0,
>   "gpus": 0,
>   "mem": 5120,
>   "cpus": 9
> }
>   }
> ```
> 
> haosdent huang wrote:
> How about 
> 
> ```
> $scope.agent.resource_reservations = _($scope.state.reserved_resources)
>   .map(function(reservation, role) {
> reservation.role = role;
> return reservation;
>   });
> ```

It works!
Updated patch.


- Andrei


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/5/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread haosdent huang



> On July 3, 2017, 6:27 p.m., haosdent huang wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 610 (patched)
> > 
> >
> > How about
> > 
> > ```
> > $scope.agent.resource_reservations = 
> > _($scope.state.reserved_resources).map(function(role, reservation)) {
> >   reservation.role = role
> >   return reservation;
> > }
> > 
> > ```
> 
> Andrei Budnik wrote:
> This code:
> ```javascript
> $scope.agent.resource_reservations = _($scope.state.reserved_resources)
>   .map(function(role, reservation) {
> reservation.role = role;
> return reservation;
>   });
> ```
> causes an exception:
> ```
> angular-1.2.3.min.js:84 TypeError: Cannot create property 'role' on 
> string 'dyn'
> at http://127.0.0.1:5050/static/js/controllers.js:607:32
> at http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:889
> at w.each.w.forEach 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:743)
> at Function.w.map.w.collect 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:855)
> at w.(anonymous function) [as map] 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:11745)
> at http://127.0.0.1:5050/static/js/controllers.js:606:14
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:66:439
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:93:98
> ```
> 
> haosdent huang wrote:
> what's the content of $scope.state.reserved_resources in your side?
> 
> Andrei Budnik wrote:
> From `curl localhost:5051/state | jq`:
> ```javascript
>   "reserved_resources": {
> "dyn": {
>   "disk": 0,
>   "gpus": 0,
>   "mem": 1024,
>   "cpus": 2
> },
> "ads": {
>   "disk": 0,
>   "gpus": 0,
>   "mem": 5120,
>   "cpus": 9
> }
>   }
> ```

How about 

```
$scope.agent.resource_reservations = _($scope.state.reserved_resources)
  .map(function(reservation, role) {
reservation.role = role;
return reservation;
  });
```


- haosdent


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/4/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread Andrei Budnik



> On July 3, 2017, 6:27 p.m., haosdent huang wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 610 (patched)
> > 
> >
> > How about
> > 
> > ```
> > $scope.agent.resource_reservations = 
> > _($scope.state.reserved_resources).map(function(role, reservation)) {
> >   reservation.role = role
> >   return reservation;
> > }
> > 
> > ```
> 
> Andrei Budnik wrote:
> This code:
> ```javascript
> $scope.agent.resource_reservations = _($scope.state.reserved_resources)
>   .map(function(role, reservation) {
> reservation.role = role;
> return reservation;
>   });
> ```
> causes an exception:
> ```
> angular-1.2.3.min.js:84 TypeError: Cannot create property 'role' on 
> string 'dyn'
> at http://127.0.0.1:5050/static/js/controllers.js:607:32
> at http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:889
> at w.each.w.forEach 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:743)
> at Function.w.map.w.collect 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:855)
> at w.(anonymous function) [as map] 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:11745)
> at http://127.0.0.1:5050/static/js/controllers.js:606:14
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:66:439
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:93:98
> ```
> 
> haosdent huang wrote:
> what's the content of $scope.state.reserved_resources in your side?

>From `curl localhost:5051/state | jq`:
```javascript
  "reserved_resources": {
"dyn": {
  "disk": 0,
  "gpus": 0,
  "mem": 1024,
  "cpus": 2
},
"ads": {
  "disk": 0,
  "gpus": 0,
  "mem": 5120,
  "cpus": 9
}
  }
```


- Andrei


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/4/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread haosdent huang



> On July 3, 2017, 6:27 p.m., haosdent huang wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 610 (patched)
> > 
> >
> > How about
> > 
> > ```
> > $scope.agent.resource_reservations = 
> > _($scope.state.reserved_resources).map(function(role, reservation)) {
> >   reservation.role = role
> >   return reservation;
> > }
> > 
> > ```
> 
> Andrei Budnik wrote:
> This code:
> ```javascript
> $scope.agent.resource_reservations = _($scope.state.reserved_resources)
>   .map(function(role, reservation) {
> reservation.role = role;
> return reservation;
>   });
> ```
> causes an exception:
> ```
> angular-1.2.3.min.js:84 TypeError: Cannot create property 'role' on 
> string 'dyn'
> at http://127.0.0.1:5050/static/js/controllers.js:607:32
> at http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:889
> at w.each.w.forEach 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:743)
> at Function.w.map.w.collect 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:855)
> at w.(anonymous function) [as map] 
> (http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:11745)
> at http://127.0.0.1:5050/static/js/controllers.js:606:14
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:66:439
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
> at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:93:98
> ```

what's the content of $scope.state.reserved_resources in your side?


- haosdent


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/4/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Re: Review Request 60546: Harden Mesos when building with cmake.

2017-07-04 Thread Benjamin Bannier

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

cmake/CompilationConfigure.cmake
Lines 207-210 (patched)

Let's not tie support to arbitrary versions (e.g., this condition rejects 
clang-4.0 just like gcc-4.3 which doesn't seem like the intention).

Instead let's use `CHECK_CXX_COMPILER_FLAG` to check compiler support for 
the flags you are passing, and potentially output a warning if we cannot enable 
any stack protection.

cmake/CompilationConfigure.cmake
Lines 213-214 (patched)

Not sure this is needed, see below.

cmake/MesosConfigure.cmake
Lines 65 (patched)

It appears as if just setting

set(CMAKE_POSITION_INDEPENDENT_CODE TRUE)

here as well would take care of both the compiler and linker flags; could 
you check if it does and potentially streamline this part. It would also be 
great if we could have a comment explaining why we enforce position independent 
code.

- Benjamin Bannier

On June 29, 2017, 8:18 p.m., Aaron Wood wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60546/
> ---
> 
> (Updated June 29, 2017, 8:18 p.m.)
> 
> 
> Review request for mesos, Andrew Schwartzmeyer, Benjamin Bannier, and Joseph 
> Wu.
> 
> 
> Bugs: MESOS-7737
> https://issues.apache.org/jira/browse/MESOS-7737
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Apply stack protectors (stronger stack protectors with newer compilers), 
> position independent code suitable for linking into executables, security 
> warnings, and generate position independent executables.
> 
> 
> Diffs
> -
> 
>   cmake/CompilationConfigure.cmake 3fa2e2f3b 
>   cmake/MesosConfigure.cmake 5ecad2c0f 
> 
> 
> Diff: https://reviews.apache.org/r/60546/diff/1/
> 
> 
> Testing
> ---
> 
> `mkdir build && cd build && cmake .. && make -j$(nproc) && make check 
> -j$(nproc)`
> 
> 
> Thanks,
> 
> Aaron Wood
> 
>

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

2017-07-04 Thread Andrei Budnik



> On July 3, 2017, 6:27 p.m., haosdent huang wrote:
> > src/webui/master/static/js/controllers.js
> > Lines 610 (patched)
> > 
> >
> > How about
> > 
> > ```
> > $scope.agent.resource_reservations = 
> > _($scope.state.reserved_resources).map(function(role, reservation)) {
> >   reservation.role = role
> >   return reservation;
> > }
> > 
> > ```

This code:
```javascript
$scope.agent.resource_reservations = _($scope.state.reserved_resources)
  .map(function(role, reservation) {
reservation.role = role;
return reservation;
  });
```
causes an exception:
```
angular-1.2.3.min.js:84 TypeError: Cannot create property 'role' on string 'dyn'
at http://127.0.0.1:5050/static/js/controllers.js:607:32
at http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:889
at w.each.w.forEach 
(http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:743)
at Function.w.map.w.collect 
(http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:855)
at w.(anonymous function) [as map] 
(http://127.0.0.1:5050/static/js/underscore-1.4.3.min.js:1:11745)
at http://127.0.0.1:5050/static/js/controllers.js:606:14
at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:66:439
at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
at D (http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:91:464)
at http://127.0.0.1:5050/static/js/angular-1.2.3.min.js:93:98
```


- Andrei


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


On June 28, 2017, 8:52 p.m., Andrei Budnik wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60370/
> ---
> 
> (Updated June 28, 2017, 8:52 p.m.)
> 
> 
> Review request for mesos, Benjamin Mahler and haosdent huang.
> 
> 
> Bugs: MESOS-6441
> https://issues.apache.org/jira/browse/MESOS-6441
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Updated agent webui page to display allocated resources per each role.
> 
> 
> Diffs
> -
> 
>   src/webui/master/static/agent.html 71e5e702e5e64e6f46c84791247aa5156c046ed9 
>   src/webui/master/static/js/controllers.js 
> 67bfd030649dd21840c16188a4964f814aa232d7 
> 
> 
> Diff: https://reviews.apache.org/r/60370/diff/4/
> 
> 
> Testing
> ---
> 
> See screenshot.
> 
> 
> File Attachments
> 
> 
> agent.html
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/22/66cc61b4-04c8-451e-a434-58f556397724__Resource_Reservations.png
> ui_bug
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/1b7755e3-0b11-46ab-9ae4-71f19bce0cd6__ui_bug.png
> ui_bug2
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/b89990c9-9aa5-432a-a0da-3f90c9428127__ui_bug2.png
> ui_bug3
>   
> https://reviews.apache.org/media/uploaded/files/2017/06/28/51553e9d-7fb9-40c0-b47c-b62fc6c37393__ui_bug3.png
> 
> 
> Thanks,
> 
> Andrei Budnik
> 
>

Review Request 60639: Updated 'HierarchicalAllocator' to interpret updates to agent total.

2017-07-04 Thread Benjamin Bannier


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

Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

This commit introduces code to interpret an agent's 'total' amount of
resources passed with 'updateSlave'. We allow overallocation of the
agent, i.e., the amount of allocated resources can exceed the total
resources.


Diffs
-

  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 
  src/tests/hierarchical_allocator_tests.cpp 
2a312a9af4bae679a0a4e7bf45a3c013513c5da2 


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


Testing
---

`make check`


Thanks,

Benjamin Bannier

Review Request 60638: Adjusted allocator interfaces to allow passing new agent total.

2017-07-04 Thread Benjamin Bannier


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

Review request for mesos, Benjamin Mahler, Jie Yu, and Jan Schlicht.


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


Repository: mesos


Description
---

This change adjust interfaces, implementations and users of the
'updateSlave' method. This allows to pass an additional argument
denoting an updated 'total' amount of resource on the agents.

In a subsequent commit we will update the 'HierarchicalAllocator' to
interpret this value.


Diffs
-

  include/mesos/allocator/allocator.hpp 
bec9e0b8ef8d51ec22f9e8af69407ed5ecaa3e8f 
  src/master/allocator/mesos/allocator.hpp 
2e780c92d5c5132abff32f1ce051c3bab2947f37 
  src/master/allocator/mesos/hierarchical.hpp 
5c58cf401de9205e54300e0ce8433995cdf5cb7a 
  src/master/allocator/mesos/hierarchical.cpp 
eb01d8e6b1108866ebc049f9f4a46157823a3541 
  src/master/master.cpp 95e9691b3e3ef101e3e7ddc7a498d5c5bf2276e3 
  src/tests/allocator.hpp a990788d5218bbcac499613783750ade022811a7 
  src/tests/hierarchical_allocator_tests.cpp 
2a312a9af4bae679a0a4e7bf45a3c013513c5da2 


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


Testing
---

Tested with https://reviews.apache.org/r/60639/.


Thanks,

Benjamin Bannier

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

2017-07-04 Thread Qian Zhang



> On July 4, 2017, 1:22 a.m., Avinash sridharan wrote:
> > docs/configuration.md
> > Lines 1312 (patched)
> > 
> >
> > `ContainerInfo.docker.parameter`

The field name is `parameters` rather than `parameter`.


> On July 4, 2017, 1:22 a.m., Avinash sridharan wrote:
> > docs/configuration.md
> > Lines 1329 (patched)
> > 
> >
> > This is interesting. I think this example highlights a subtle 
> > difference between the behavior of the DNS option for CNI and CNM. For CNI 
> > networks we don't really have a concept of HOST or BRIDGE networks. 
> > However, for CNM networks since we support this option for HOST, BRIDGE and 
> > USER networks, we should be explicit about this information. Also, we need 
> > to emphasize that the `--dns` options for `HOST` and `BRIDGE` networks are 
> > supported only for docker 1.13 and above (for `USER` it has been available 
> > since 1.10). We probably want to enforce these version checks for docker.

> However, for CNM networks since we support this option for HOST, BRIDGE and 
> USER networks, we should be explicit about this information. Also, we need to 
> emphasize that the --dns options for HOST and BRIDGE networks are supported 
> only for docker 1.13 and above.

I have a very old version of Docker (1.5.0) and I have checked its `docker run` 
command also has the `--dns` option and it can work with `BRIDGE` network but 
not with `HOST` network. Do you think we need to put such info into this help 
message (I mean emphasizing the minimal Docker version for `--dns` support with 
`HOST`, `BRIDGE` and `USER` networks respectively)? That may make this message 
too big.


> On July 4, 2017, 1:22 a.m., Avinash sridharan wrote:
> > src/messages/flags.proto
> > Lines 36 (patched)
> > 
> >
> > Just a thought, should we be more explicit and keep two different DNS 
> > structures for Mesos and Docker? For example, Docker has the concept of 
> > network modes where as Mesos doesn't. With this structure we would have to 
> > use some kind of pattern matching to identify the network type in case of 
> > docker, e.g., `network="host"` for host-mode networking, `network="bridge"` 
> > for bridge-mode networking. This is just a thought, I am fine with these 
> > validation checks being introduced if this seems cleaner.
> > 
> > At the very least we should definitely have a comment here explaining 
> > how the network-modes are selected for `docker` based on the network names.

I think we do not need the network type since I will always use the network 
name specified by framework to match `ContainerDNS.docker.network`. And for 
sure, I will add some comments to explain how the network will be matched for 
both CNM networks and CNI networks.


- Qian


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


On June 28, 2017, 10:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60500/
> ---
> 
> (Updated June 28, 2017, 10:57 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
> ---
> 
> Introduced `--default_container_dns` agent flag.
> 
> 
> Diffs
> -
> 
>   docs/configuration.md 0eb696a949003ff11831aed5e4f4ab384cf9992e 
>   src/messages/flags.hpp 70ad58c33067a0b058deafdcc4ffc5f554b6ca72 
>   src/messages/flags.proto e87075f19714739b93d9f4aa33ea8686a1bb5613 
>   src/slave/flags.hpp e75c1b4227b443aedf445921b3f2108d930c112c 
>   src/slave/flags.cpp c84aa6724170bba46bbe8410b71d42a1626e 
> 
> 
> Diff: https://reviews.apache.org/r/60500/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 60636: Added method that returns allocated resources for framework.

2017-07-04 Thread Andrei Budnik


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

Review request for mesos, Benjamin Mahler and haosdent huang.


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


Repository: mesos


Description
---

Added method that returns allocated resources for framework.


Diffs (updated)
-

  src/slave/slave.hpp 5ede32d00c5dbf18dc0639ec7af5d2a86f86f619 
  src/slave/slave.cpp 0e24b8cb8d1020af515e3d1862e121e1daf82ce9 


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


Testing
---

make check


Thanks,

Andrei Budnik

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

2017-07-04 Thread Benjamin Bannier


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


Ship it!




Ship It!

- Benjamin Bannier


On June 29, 2017, 11:58 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60438/
> ---
> 
> (Updated June 29, 2017, 11:58 p.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 6eb2d41b35c5f064839c29a0ba8326c590a33213 
> 
> 
> Diff: https://reviews.apache.org/r/60438/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running a CI job pointing to a branch containing this patch.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60440: Updated local development workflow of mesos website.

2017-07-04 Thread Benjamin Bannier

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

I am confused why we need this at all at this point. With 
https://reviews.apache.org/r/60439/ we should be able to generate the site 
locally and look at its output. Executing the rake rules is already now pretty 
much possible (on macOS `rake` needs to be added to the `Gemfile`) if the 
needed tools are locally available, if not, we provide the container. We seem 
to add a lot of elaborate tooling to isolate us from something which is already 
an abstraction (i.e., docker around rake).

Can we get rid of this container completely and instead streamline the existing 
`support/website` and `site/Rakefile` toolings instead of adding another one?

- Benjamin Bannier

On June 29, 2017, 11:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60440/
> ---
> 
> (Updated June 29, 2017, 11:59 p.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
> ---
> 
> Made the layout and scripts consistent with CI based automatic
> publishing of the website.
> 
> 
> Diffs
> -
> 
>   site/Dockerfile 8ba0be0c28e924f7a2b89e6e5a3237deb3751a41 
>   site/README.md ebd3e6a0fea7ae0fe3b28719bcab28ee8f7c356c 
>   site/build.sh 11f15e15621c4d3db1472e88911787b9b3100f97 
>   site/entrypoint.sh PRE-CREATION 
>   site/mesos-website-dev.sh PRE-CREATION 
> 
> 
> Diff: https://reviews.apache.org/r/60440/diff/1/
> 
> 
> Testing
> ---
> 
> Tested by running the script locally.
> 
> 
> Thanks,
> 
> Vinod Kone
> 
>

Re: Review Request 60439: Added scripts to automate website publishing.

2017-07-04 Thread Benjamin Bannier

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

This is a great start, but I would prefer if we wouldn't directly mount an 
existing source directory into the container and bootstrap, configure and build 
in it (this is also broken for e.g., in-source builds).

What about instead modify mounting the Mesos source tree into the container, 
then inside the container cloning from it, and building in the clean checkout? 
This woudl not only minimize the risk of destructive side-effects outside the 
container, but also e.g., make sure that we do not by accident pick up 
uncommitted changes. This is the approach we also followed in 
`support/mesos-tidy`.

I left some first comments, but will look again once the handling of the source 
tree is under control.

support/jenkins/websitebot.sh
Lines 24 (patched)

Let's quote,

: "${WORKSPACE:?"Environment variable 'WORKSPACE' must be set"}"

support/jenkins/websitebot.sh
Lines 26-27 (patched)

Let's quote,

export MESOS_DIR="$WORKSPACE/mesos"
export MESOS_SITE_DIR="$WORKSPACE/mesos-site"

support/jenkins/websitebot.sh
Lines 29 (patched)

Let's quote,

pushd "$MESOS_DIR"

support/jenkins/websitebot.sh
Lines 31 (patched)

If the source tree contains uncommitted changes, the output will not 
reproducibly correspond to this SHA1.

I'd suggest to instead clone the source tree instead.

support/jenkins/websitebot.sh
Lines 38 (patched)

Let's quote,

pushd "$MESOS_SITE_DIR"

support/mesos-website.sh
Lines 25 (patched)

Let's put this into quotes,

: "${MESOS_SITE_DIR:?"Environment variable 'MESOS_SITE_DIR' must be 
set"}"

support/mesos-website.sh
Lines 27 (patched)

Let's quote here,

pushd "$MESOS_DIR"

support/mesos-website.sh
Lines 33 (patched)

Not crucial here right now, but let's put this string into single quotes, 
see https://github.com/koalaman/shellcheck/wiki/SC2064.

Also please consider working on getting this image published in the Mesos 
dockerhub org so we can pull from there instead of having the rebuild the same 
image over and over again. See e.g., `support/mesos-tidy.sh` how we already do 
this for other images.

support/mesos-website.sh
Lines 43 (patched)

Let's be consistent with other command substitutions in this file, and also 
add quotes,

docker run \
  --rm \
  -e LOCAL_USER_ID="$(id -u "$USER")" \
  -v "$MESOS_DIR":/SRC \
  -v "$MESOS_SITE_DIR"/content:/mesos/site/publish \
  $TAG

support/mesos-website/Dockerfile
Lines 1-11 (patched)

We now have one Docker image to run tests in CI (via `docker_build.sh`), 
one to run clang-tidy checks (`mesos-tidy/Dockerfile`), and are adding another 
one  here to build the website.

We should try to reduce the number of these images or remove duplication, 
e.g., this image could be in principle implemented as a small addon on top of 
`mesos/mesos-tidy`

FROM mesos/mesos-tidy:latest
LABEL Description="This image is used for generating Mesos web site."

# Install ruby and doxygen tools ...

support/mesos-website/build.sh
Lines 35 (patched)

Could we make this consistent with other CI scripts?

In `docker_build.sh` we use `make -j6` w/o e.g., hitting a memory limit, in 
`mesos-tidy/entrypoint.sh` where we do not build the most expensive parts of 
Mesos we even pick `make -j $(nproc)`.

- Benjamin Bannier

On June 29, 2017, 11:59 p.m., Vinod Kone wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60439/
> ---
> 
> (Updated June 29, 2017, 11:59 p.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
> ---
> 
> These scripts are expected to be run by ASF CI on any commit to
> the mesos repo. The directory layout is similar to tidybot CI job.
> 
> 
> Diffs
> -
> 
>   support

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

2017-07-04 Thread Ilya Pronin


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




3rdparty/libprocess/include/process/deferred.hpp
Line 77 (original), 79 (patched)


This is more a question than an issue. Do we plan to forward functor 
references through `_Deferred`, hence the use of `std::forward()` here 
instead of `std::move()`?

Carrying a functor reference is unsafe. `template  _Deferred 
defer(F&&)` overload silently captures references to lvalues in 
`_Deferred`. But as long as `_Deferred` is immediately used for conversion 
to `Deferred` or `std::function` it should be fine.



3rdparty/libprocess/include/process/deferred.hpp
Lines 83-88 (original), 85-90 (patched)


Can we eliminate more copyings here? `f_` is captured by value by lambda 
and then passed as a `const` to `dispatch`. We can make the lambda `mutable` 
and `move()` captured `f_`. Or, for example, we could store `f` value and `pid` 
in a structure, that will be captured by a `std::shared_ptr`.



3rdparty/libprocess/include/process/dispatch.hpp
Lines 203-205 (patched)


Style: `Dispatcher` internals are overindented. Here and in other places 
where it's defined.


- Ilya Pronin


On June 15, 2017, 8:32 a.m., Dmitry Zhuk wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60003/
> ---
> 
> (Updated June 15, 2017, 8:32 a.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
> ---
> 
> Reduced copying in defer, dispatch and Future.
> 
> 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 
> 
> 
> Diff: https://reviews.apache.org/r/60003/diff/1/
> 
> 
> 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 60558: Set container DNS with `--default_container_dns` in Docker executor.

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

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Re: Review Request 60546: Harden Mesos when building with cmake.

Re: Review Request 60370: Updated agent webui page to display allocated resources per each role.

Review Request 60639: Updated 'HierarchicalAllocator' to interpret updates to agent total.

Review Request 60638: Adjusted allocator interfaces to allow passing new agent total.

Re: Review Request 60500: Introduced `--default_container_dns` agent flag.

Review Request 60636: Added method that returns allocated resources for framework.

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

Re: Review Request 60440: Updated local development workflow of mesos website.

Re: Review Request 60439: Added scripts to automate website publishing.

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

19 matches

Site Navigation

Mail list logo

Footer information