Re: Review Request 61155: Added http::Server.

[Benjamin Mahler]

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



A pretty clean http server!

I held off on a ship it because there were a couple of issues that we should 
discuss.


3rdparty/libprocess/include/process/http.hpp
Line 1020 (original), 1020-1022 (patched)


Fix this separately?



3rdparty/libprocess/include/process/http.hpp
Lines 1047-1050 (patched)


FYI I think this might not work for windows



3rdparty/libprocess/include/process/http.hpp
Lines 1059 (patched)


Issuing a shutdown of reads doesn't seem quite right to me, it will cut off 
any requests that we haven't fully read yet and cut off any pipelined requests 
that are behind it already. Even if shutdown is guaranteed to preserve the data 
in the receive buffer we'll still be cutting off any requests that don't 
completely reside within the receive buffer?

I would expect that graceful shutdown will at least finish reading any 
active requests, and ideally (to gracefully handle pipelined requests) keep 
reading until reading is blocked and no requests are being handled.

As an aside, apparently (since not specified by POSIX) the behavior when 
clients send more data after a shutdown(SHUT_RD) is platform dependent:

```
* Windows will issue an RST which eventually gives the sender a 'connection 
reset'.

* BSD-based Unixes will accept and ignore the data (i.e. ACK it but throw 
it away, rather than entering it into your socket receive buffer), so the 
sender will just keep sending as much as he likes, until you close the socket, 
when he will either read an end-of-stream or get a 'connection reset' on write.

* Linux will accept the data until your socket send buffer fills up; then 
your receive window will close, which blocks the sender; then when you close 
the socket the sender will get a 'connection reset'.
```

E.g. this was from https://stackoverflow.com/a/30317014



3rdparty/libprocess/include/process/http.hpp
Lines 1071 (patched)


FYI I think this might not work for windows



3rdparty/libprocess/include/process/http.hpp
Lines 1076 (patched)


Just curious, because it's not obvious to me, when does the caller need to 
pass a socket instead of the address?



3rdparty/libprocess/include/process/http.hpp
Lines 1124-1130 (patched)


stop seems to suggest a s/run/start/?



3rdparty/libprocess/include/process/http.hpp
Lines 1132-1133 (patched)


I suspect callers will want something synchronous here since the address 
should already be known, you could do something similar to connection:


https://github.com/apache/mesos/blob/f8859b9f6bec0a21584000508d1fbf77aa18ec62/3rdparty/libprocess/include/process/http.hpp#L963-L964

i.e. store the address in Server since you always have it in hand when 
you're constructing the Server object.



3rdparty/libprocess/include/process/http.hpp
Lines 1143 (patched)


Any reason not to store a `PID` and spawn with 
`managed=true`?



3rdparty/libprocess/src/http.cpp
Line 1404 (original), 1406-1407 (patched)


whoops?



3rdparty/libprocess/src/http.cpp
Lines 1705-1713 (original), 1708-1730 (patched)


whoops?



3rdparty/libprocess/src/http.cpp
Lines 1964-1971 (patched)


FYI I think this might not work for windows



3rdparty/libprocess/src/http.cpp
Lines 1973-1978 (patched)


Hm.. can you avoid the copy here and instead move the client into the 
clients map?



3rdparty/libprocess/src/http.cpp
Lines 1987 (patched)


It doesn't look like you need the `-> Future` here given both 
returns have the same type?



3rdparty/libprocess/src/http.cpp
Lines 1997-2003 (patched)


Hm.. isn't this only the DISCARDED case? In the failure case it seems like 
that's a problem with the accept loop. Maybe the logic should be:

```
if discarded:
  check stopping
  return stopping

return shutdown(failure(f.condition))
```



3rdparty/libprocess/src/http.cpp
Lines 2013-2021 (patched)


Seems like we should reverse these? i.e. if stopping is

Re: Review Request 61237: Updated docker executor to return IPv6 address of a container.

[Qian Zhang]

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


Fix it, then Ship it!





src/docker/docker.hpp
Lines 112-113 (original), 112-113 (patched)


Not yours, the word `no IP has been not assigned` seems incorrect, should 
it be `no IP has been assigned`?



src/docker/docker.cpp
Line 342 (original), 345 (patched)


Not yours, should `failback` be `fallback`?



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


It seems we need one more space here for indent.



src/docker/executor.cpp
Lines 236-238 (patched)


I think these 3 lines can be merged into 2 lines, like:
```
  // NOTE: By default the protocol is set to IPv4 and therefore
  // we explicitly set the protocol only for an IPv6 address.

```


- Qian Zhang


On July 29, 2017, 8:51 a.m., Avinash sridharan wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/61237/
> ---
> 
> (Updated July 29, 2017, 8:51 a.m.)
> 
> 
> Review request for mesos, Jie Yu and Qian Zhang.
> 
> 
> Bugs: MESOS-7807
> https://issues.apache.org/jira/browse/MESOS-7807
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> A docker container can have a v4 and a v6 address. Currently the docker
> executor was returning only the IPv4 address. This changes allows the
> executor to return the IPv4 and IPv6 address of the container.
> 
> 
> Diffs
> -
> 
>   src/docker/docker.hpp 5593cb635e073334c6c2566be3d803cd7febb1c3 
>   src/docker/docker.cpp 8081c0203bf62cf62aa3b93d745f0e829ad65509 
>   src/docker/executor.cpp e03f24461ec7b19cacae79c020406f0a475f2d19 
> 
> 
> Diff: https://reviews.apache.org/r/61237/diff/1/
> 
> 
> Testing
> ---
> 
> Start agent:
> sudo ./bin/mesos-agent.sh --ip=10.0.2.15 --master=10.0.2.15:5050 
> --work_dir=/tmp/mesos --containerizers=mesos,docker
> 
> Start master:
> sudo ./bin/mesos-master.sh --ip=10.0.2.15 --work_dir=/tmp/mesos-master
> 
> Use mesos-execute to launch a docker container on an IPv6 network:
> sudo src/mesos-execute --master=10.0.2.15:5050 
> --task=file:///home/vagrant/dev/mesos_tasks/docker.json 2> log.txt
> 
> cat /home/vagrant/dev/mesos_tasks/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": "USER"
> },
> "network_infos": [
> {
>   "name": "ipv6"
> }
> ]
>   }
> }
> 
> 
> Curl the Mesos state to make sure that the right IPv6 addresses are reflected 
> for the container:
>  curl http://10.0.2.15:5051/state | jq
> 
> "role": "*",
>   "statuses": [
> {
>   "state": "TASK_RUNNING",
>   "timestamp": 1501288232.79384,
>   "container_status": {
> "container_id": {
>   "value": "59319a5b-a28b-46bf-a20f-8472e6598abd"
> },
> "network_infos": [
>   {
> "ip_addresses": [
>   {
> "protocol": "IPv4",
> "ip_address": "172.19.0.2"
>   },
>   {
> "protocol": "IPv6",
> "ip_address": "fd00:0:0:1::2"
>   }
> ],
> "name": "ipv6"
>   }
> ]
>   }
> }
>   ],
>   "container": {
> "type": "DOCKER",
> "docker": {
>   "image": "nginx",
>   "network": "USER",
>   "privileged": false
> },
> "network_infos": [
>   {
> "name": "ipv6"
>   }
> ]
>   }
> }
>   ],
> 
> 
> Thanks,
> 
> Avinash sridharan
> 
>

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

[Qian Zhang]

> On July 28, 2017, 8:41 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/cni_isolator_tests.cpp
> > Lines 1226 (patched)
> > 
> >
> > Similar to the `DockerContainerizer` can we add a test for the flags 
> > themselves to check that we don't allow setting DNS for HOST mode? Also, 
> > would be great to test the matching algorithm we use to setup the DNS when 
> > we have multiple DNS entries.
> 
> Qian Zhang wrote:
> I added a test `https://reviews.apache.org/r/61219/` for the wildcard 
> match.

And I added a test https://reviews.apache.org/r/61245/ for the flag validation.


- Qian


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


On July 25, 2017, 2:08 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60793/
> ---
> 
> (Updated July 25, 2017, 2:08 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_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 60761: Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.

[Qian Zhang]

> On July 28, 2017, 8:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > 
> >
> > Can we have a test for checking validation error on setting up the HOST 
> > MODE?
> 
> Qian Zhang wrote:
> Did you mean adding a test to test the validation code in the lambda that 
> we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If 
> so, then I think `docker_containerizer_tests.cpp` may not be a good place for 
> such test because `docker_containerizer_tests.cpp` should contain the 
> end-to-end tests, but for testing the validation code we do not even need to 
> start the agent. Maybe just add a test in `flags_tests.cpp`? And in another 
> hand, for the other agent flags which has the validation code in lambda, it 
> seems there is no related test for them, so do we really need such test for 
> testing the validation code of `--default_container_dns`?
> 
> BTW, I was thinking to add a test for wildcard match of Docker 
> user-defined network, but the problem is we can not assume any user-defined 
> networks in the test env, but there must be a bridge network, so currently I 
> only have the test for the bridge network.
> 
> Avinash sridharan wrote:
> Agreed !! if we can add the flag validation tests for the flags tests 
> that should be good enough.  Wildcard match test for BRIDGE mode is good 
> enough I think.
> 
> Qian Zhang wrote:
> I added a test https://reviews.apache.org/r/61219/ for the flag 
> validation.

Sorry, the patch for the flag validation test should be: 
https://reviews.apache.org/r/61245/.


- Qian


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


On July 25, 2017, 2:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> ---
> 
> (Updated July 25, 2017, 2:07 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 `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 60761: Added a test `DockerContainerizerTest.ROOT_DOCKER_DefaultDNS`.

[Qian Zhang]

> On July 28, 2017, 8:35 a.m., Avinash sridharan wrote:
> > src/tests/containerizer/docker_containerizer_tests.cpp
> > Lines 4357 (patched)
> > 
> >
> > Can we have a test for checking validation error on setting up the HOST 
> > MODE?
> 
> Qian Zhang wrote:
> Did you mean adding a test to test the validation code in the lambda that 
> we added in `src/slave/flags.cpp` for the `--default_container_dns` flag? If 
> so, then I think `docker_containerizer_tests.cpp` may not be a good place for 
> such test because `docker_containerizer_tests.cpp` should contain the 
> end-to-end tests, but for testing the validation code we do not even need to 
> start the agent. Maybe just add a test in `flags_tests.cpp`? And in another 
> hand, for the other agent flags which has the validation code in lambda, it 
> seems there is no related test for them, so do we really need such test for 
> testing the validation code of `--default_container_dns`?
> 
> BTW, I was thinking to add a test for wildcard match of Docker 
> user-defined network, but the problem is we can not assume any user-defined 
> networks in the test env, but there must be a bridge network, so currently I 
> only have the test for the bridge network.
> 
> Avinash sridharan wrote:
> Agreed !! if we can add the flag validation tests for the flags tests 
> that should be good enough.  Wildcard match test for BRIDGE mode is good 
> enough I think.

I added a test https://reviews.apache.org/r/61219/ for the flag validation.


- Qian


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


On July 25, 2017, 2:07 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/60761/
> ---
> 
> (Updated July 25, 2017, 2:07 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 `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
> 
>

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

[Qian Zhang]

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

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