Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 27, 2017, 10:43 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addessed the issue on macOS that the `connect()` function will return the error 
`Invalid argument`.
The root cause of this issue is, on macOS setting the 3rd paramter (`addrlen`) 
of the `connect()` function to the size of `sockaddr_storage` is not correct, 
instead we should set it to `sockaddr_in` (IPv4) or `sockaddr_in6` (IPv6).


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

Changes: https://reviews.apache.org/r/63795/diff/5-6/


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Alexander Rukletsov]

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


Ship it!





src/checks/tcp_connect.cpp
Lines 33-34 (patched)


It is unfortunate that we now introduce a dependecy on process library.


- Alexander Rukletsov


On Nov. 27, 2017, 2:20 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 27, 2017, 2:20 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/5/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 27, 2017, 10:20 a.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

> On Nov. 25, 2017, 11:49 a.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 88 (patched)
> > 
> >
> > Why do we need this? `net::IP` will always be `AF_INET` or `AF_INET6`?
> 
> Qian Zhang wrote:
> That's from Alex's comment 
> (https://reviews.apache.org/r/63795/#comment268744), I think it is because we 
> do not want to be tightly coupled with the internal implementation of 
> `net::IP::parse()`.

I don't think that comment is valid with the use of `net::IP` and 
`network::address`. `net::IP` can only be `AF_INET` or AF_INET6`. Its a 
fundamentatl property of IP addresses. So this check is redundant here. I think 
we should remove it. 

https://github.com/apache/mesos/blob/bd5e874c22f0d16fc5494213d319065cf9107d0f/3rdparty/stout/include/stout/ip.hpp#L417


- Avinash


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


On Nov. 22, 2017, 3:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 22, 2017, 3:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

> On Nov. 25, 2017, 7:49 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 88 (patched)
> > 
> >
> > Why do we need this? `net::IP` will always be `AF_INET` or `AF_INET6`?

That's from Alex's comment (https://reviews.apache.org/r/63795/#comment268744), 
I think it is because we do not want to be tightly coupled with the internal 
implementation of `net::IP::parse()`.


- Qian


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


On Nov. 22, 2017, 11:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 22, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

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


Fix it, then Ship it!




Ship It!


src/checks/tcp_connect.cpp
Lines 88 (patched)


Why do we need this? `net::IP` will always be `AF_INET` or `AF_INET6`?


- Avinash sridharan


On Nov. 22, 2017, 3:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 22, 2017, 3:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

> On Nov. 22, 2017, 8:16 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 90 (patched)
> > 
> >
> > Should have bought this up earlier. I think this code will be a lot 
> > more simpler if you use `network::Address` to represent the ip:port address.
> > 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L277
> > 
> > You wouldn't need to maintain family specific code here and you can 
> > directly use the 
> > `sock_storage` cast operator
> > 
> > https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L377

Good idea!


- Qian


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


On Nov. 22, 2017, 11:14 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 22, 2017, 11:14 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/4/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 22, 2017, 11:14 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

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




src/checks/tcp_connect.cpp
Lines 90 (patched)


Should have bought this up earlier. I think this code will be a lot more 
simpler if you use `network::Address` to represent the ip:port address.

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L277

You wouldn't need to maintain family specific code here and you can 
directly use the 
`sock_storage` cast operator

https://github.com/apache/mesos/blob/master/3rdparty/libprocess/include/process/address.hpp#L377


- Avinash sridharan


On Nov. 17, 2017, 12:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 17, 2017, 12:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

> On Nov. 16, 2017, 6:54 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 90 (patched)
> > 
> >
> > Why do we need this?
> > 
> > `net::IP::in` and `net::IP::in6` already give this functionality we 
> > should just use those?
> 
> Qian Zhang wrote:
> We need to call `parse->family()` to know it is `AF_INET` or `AF_INET6`, 
> and then call `net::IP::in()` or `net::IP::in6()` accordingly.

agreed. Was wondering if we can add a `storage` call to `net::IP` and get 
`storage` from `net::IP` itself instead of higher level code needing to deal 
with `family`.


- Avinash


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


On Nov. 17, 2017, 12:57 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 17, 2017, 12:57 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/3/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 17, 2017, 8:57 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

> On Nov. 17, 2017, 2:54 a.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Lines 90 (patched)
> > 
> >
> > Why do we need this?
> > 
> > `net::IP::in` and `net::IP::in6` already give this functionality we 
> > should just use those?

We need to call `parse->family()` to know it is `AF_INET` or `AF_INET6`, and 
then call `net::IP::in()` or `net::IP::in6()` accordingly.


- Qian


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


On Nov. 16, 2017, 3:21 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 16, 2017, 3:21 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

> On Nov. 16, 2017, 6:54 p.m., Avinash sridharan wrote:
> > src/checks/tcp_connect.cpp
> > Line 86 (original), 84 (patched)
> > 
> >
> > `AF_UNSPEC` is default. You can just do `net::IP::parse`
> > 
> > Also instead of `parse` can we use `_ip`?

Looks like AlexR already vetoed the use of `_ip`, so I am good with that.


- Avinash


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


On Nov. 16, 2017, 7:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 16, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Avinash sridharan]

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




src/checks/tcp_connect.cpp
Line 86 (original), 84 (patched)


`AF_UNSPEC` is default. You can just do `net::IP::parse`

Also instead of `parse` can we use `_ip`?



src/checks/tcp_connect.cpp
Lines 90 (patched)


Why do we need this?

`net::IP::in` and `net::IP::in6` already give this functionality we should 
just use those?


- Avinash sridharan


On Nov. 16, 2017, 7:21 a.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 16, 2017, 7:21 a.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/2/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

(Updated Nov. 16, 2017, 3:21 p.m.)


Review request for mesos, Alexander Rukletsov and Avinash sridharan.


Changes
---

Addressed review comments.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs (updated)
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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

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


Testing
---


Thanks,

Qian Zhang

Re: Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Alexander Rukletsov]

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




src/checks/tcp_connect.cpp
Line 86 (original), 84 (patched)


We usually call these variables like the action we performed, i.e., 
s/ip/parse. In this case you can also restore original argument names : )



src/checks/tcp_connect.cpp
Lines 98-102 (patched)


Let's explicitly check for `AF_INET6` and add another `else` where we error 
out for unsupported family.


- Alexander Rukletsov


On Nov. 14, 2017, 1:30 p.m., Qian Zhang wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/63795/
> ---
> 
> (Updated Nov. 14, 2017, 1:30 p.m.)
> 
> 
> Review request for mesos, Alexander Rukletsov and Avinash sridharan.
> 
> 
> Bugs: MESOS-8050
> https://issues.apache.org/jira/browse/MESOS-8050
> 
> 
> Repository: mesos
> 
> 
> Description
> ---
> 
> Made `mesos-tcp-connect` support IPv6.
> 
> 
> Diffs
> -
> 
>   src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 
> 
> 
> Diff: https://reviews.apache.org/r/63795/diff/1/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Qian Zhang
> 
>

Review Request 63795: Made `mesos-tcp-connect` support IPv6.

[Qian Zhang]

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

Review request for mesos, Alexander Rukletsov and Avinash sridharan.


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


Repository: mesos


Description
---

Made `mesos-tcp-connect` support IPv6.


Diffs
-

  src/checks/tcp_connect.cpp 2514f4aebcc634b18cd2b3c36529222fe456e903 


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


Testing
---


Thanks,

Qian Zhang