subject:"\[ovs\-dev\] \[PATCH v2 ovn\] Add IPv6 support for lb health\-check"

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

2023-02-10 Thread Lorenzo Bianconi

> On Fri, Dec 16, 2022 at 6:06 PM Lorenzo Bianconi <
> lorenzo.bianc...@redhat.com> wrote:
> 
> > Add Similar to IPv4 counterpart, introduce IPv6 load-balancer health
> > check support.
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2136094
> > Signed-off-by: Lorenzo Bianconi 
> >
> 
> Hi Lorenzo,
> I have a couple of small comments, see down below.
> Other than that it looks good.

Hi Ales,

thx for the review. Few comments inline.

Regards,
Lorenzo

> 
> 
> > ---
> > Changes since v1:
> > - fix potential crash in ovn-northd
> > - improve documentation
> > ---
> >  controller/pinctrl.c| 213 -
> >  northd/northd.c |  79 ++
> >  northd/ovn-northd.8.xml |  17 +++
> >  ovn-nb.xml  |  21 ++--
> >  tests/ovn.at| 201 ++-
> >  tests/system-ovn.at | 230 +++-
> >  6 files changed, 658 insertions(+), 103 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index f1176a2f2..d7dbdab0e 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -6740,7 +6740,7 @@ sync_svc_monitors(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> >  ovs_be32 ip4;
> >  if (ip_parse(sb_svc_mon->ip, )) {
> >  ip_addr = in6_addr_mapped_ipv4(ip4);
> > -} else {
> > +} else if (!ipv6_parse(sb_svc_mon->ip, _addr)) {
> >  continue;
> >  }
> >
> 
> It looks to me that it would be beneficial to store is_ipv6 boolean after
> parsing.
> You could avoid multiple checks IN6_IS_ADDR_V4MAPPED(_addr).
> See down below.

I would say it is better something like:

is_ipv4 = ip_parse(sb_svc_mon->ip, );
...

> 
> 
> 
> >
> > @@ -6753,16 +6753,27 @@ sync_svc_monitors(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> >  continue;
> >  }
> >
> > -for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
> > -if (ip4 == laddrs.ipv4_addrs[j].addr) {
> > -ea = laddrs.ea;
> > -mac_found = true;
> > -break;
> > +if (IN6_IS_ADDR_V4MAPPED(_addr)) {
> >
> 
> This check could be replaced with "if(i!s_ipv6)".

ack, I will fix it.

> 
> 
> > +for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
> > +if (ip4 == laddrs.ipv4_addrs[j].addr) {
> > +ea = laddrs.ea;
> > +mac_found = true;
> > +break;
> > +}
> > +}
> > +} else {
> > +for (size_t j = 0; j < laddrs.n_ipv6_addrs; j++) {
> > +if (IN6_ARE_ADDR_EQUAL(_addr,
> > +   _addrs[j].addr)) {
> > +ea = laddrs.ea;
> > +mac_found = true;
> > +break;
> > +}
> >  }
> >  }
> >
> > -if (!mac_found && !laddrs.n_ipv4_addrs) {
> > -/* IPv4 address(es) are not configured. Use the first
> > mac. */
> > +if (!mac_found && !laddrs.n_ipv4_addrs &&
> > !laddrs.n_ipv6_addrs) {
> > +/* IP address(es) are not configured. Use the first mac.
> > */
> >  ea = laddrs.ea;
> >  mac_found = true;
> >  }
> > @@ -6796,7 +6807,7 @@ sync_svc_monitors(struct ovsdb_idl_txn
> > *ovnsb_idl_txn,
> >  svc_mon->port_key = port_key;
> >  svc_mon->proto_port = sb_svc_mon->port;
> >  svc_mon->ip = ip_addr;
> > -svc_mon->is_ip6 = false;
> > +svc_mon->is_ip6 = !IN6_IS_ADDR_V4MAPPED(_addr);
> >
> 
> You could store the "is_ipv6" here directly.

ack, I will fix it.

> 
> 
> >  svc_mon->state = SVC_MON_S_INIT;
> >  svc_mon->status = SVC_MON_ST_UNKNOWN;
> >  svc_mon->protocol = protocol;
> > @@ -7564,26 +7575,30 @@ svc_monitor_send_tcp_health_check__(struct rconn
> > *swconn,
> >  ovs_be32 tcp_ack,
> >  ovs_be16 tcp_src)
> >  {
> > -if (svc_mon->is_ip6) {
> > -return;
> > -}
> > -
> >  /* Compose a TCP-SYN packet. */
> >  uint64_t packet_stub[128 / 8];
> >  struct dp_packet packet;
> > +dp_packet_use_stub(, packet_stub, sizeof packet_stub);
> >
> >  struct eth_addr eth_src;
> >  eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, _src);
> > -ovs_be32 ip4_src;
> > -ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
> > -
> > -dp_packet_use_stub(, packet_stub, sizeof packet_stub);
> > -pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
> > - ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
> > - IPPROTO_TCP, 63, TCP_HEADER_LEN);
> > +if (svc_mon->is_ip6) {
> > +struct in6_addr ip6_src;

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

2023-02-10 Thread Ales Musil

On Fri, Dec 16, 2022 at 6:06 PM Lorenzo Bianconi <
lorenzo.bianc...@redhat.com> wrote:

> Add Similar to IPv4 counterpart, introduce IPv6 load-balancer health
> check support.
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2136094
> Signed-off-by: Lorenzo Bianconi 
>

Hi Lorenzo,
I have a couple of small comments, see down below.
Other than that it looks good.


> ---
> Changes since v1:
> - fix potential crash in ovn-northd
> - improve documentation
> ---
>  controller/pinctrl.c| 213 -
>  northd/northd.c |  79 ++
>  northd/ovn-northd.8.xml |  17 +++
>  ovn-nb.xml  |  21 ++--
>  tests/ovn.at| 201 ++-
>  tests/system-ovn.at | 230 +++-
>  6 files changed, 658 insertions(+), 103 deletions(-)
>
> diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> index f1176a2f2..d7dbdab0e 100644
> --- a/controller/pinctrl.c
> +++ b/controller/pinctrl.c
> @@ -6740,7 +6740,7 @@ sync_svc_monitors(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  ovs_be32 ip4;
>  if (ip_parse(sb_svc_mon->ip, )) {
>  ip_addr = in6_addr_mapped_ipv4(ip4);
> -} else {
> +} else if (!ipv6_parse(sb_svc_mon->ip, _addr)) {
>  continue;
>  }
>

It looks to me that it would be beneficial to store is_ipv6 boolean after
parsing.
You could avoid multiple checks IN6_IS_ADDR_V4MAPPED(_addr).
See down below.



>
> @@ -6753,16 +6753,27 @@ sync_svc_monitors(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  continue;
>  }
>
> -for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
> -if (ip4 == laddrs.ipv4_addrs[j].addr) {
> -ea = laddrs.ea;
> -mac_found = true;
> -break;
> +if (IN6_IS_ADDR_V4MAPPED(_addr)) {
>

This check could be replaced with "if(i!s_ipv6)".


> +for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
> +if (ip4 == laddrs.ipv4_addrs[j].addr) {
> +ea = laddrs.ea;
> +mac_found = true;
> +break;
> +}
> +}
> +} else {
> +for (size_t j = 0; j < laddrs.n_ipv6_addrs; j++) {
> +if (IN6_ARE_ADDR_EQUAL(_addr,
> +   _addrs[j].addr)) {
> +ea = laddrs.ea;
> +mac_found = true;
> +break;
> +}
>  }
>  }
>
> -if (!mac_found && !laddrs.n_ipv4_addrs) {
> -/* IPv4 address(es) are not configured. Use the first
> mac. */
> +if (!mac_found && !laddrs.n_ipv4_addrs &&
> !laddrs.n_ipv6_addrs) {
> +/* IP address(es) are not configured. Use the first mac.
> */
>  ea = laddrs.ea;
>  mac_found = true;
>  }
> @@ -6796,7 +6807,7 @@ sync_svc_monitors(struct ovsdb_idl_txn
> *ovnsb_idl_txn,
>  svc_mon->port_key = port_key;
>  svc_mon->proto_port = sb_svc_mon->port;
>  svc_mon->ip = ip_addr;
> -svc_mon->is_ip6 = false;
> +svc_mon->is_ip6 = !IN6_IS_ADDR_V4MAPPED(_addr);
>

You could store the "is_ipv6" here directly.


>  svc_mon->state = SVC_MON_S_INIT;
>  svc_mon->status = SVC_MON_ST_UNKNOWN;
>  svc_mon->protocol = protocol;
> @@ -7564,26 +7575,30 @@ svc_monitor_send_tcp_health_check__(struct rconn
> *swconn,
>  ovs_be32 tcp_ack,
>  ovs_be16 tcp_src)
>  {
> -if (svc_mon->is_ip6) {
> -return;
> -}
> -
>  /* Compose a TCP-SYN packet. */
>  uint64_t packet_stub[128 / 8];
>  struct dp_packet packet;
> +dp_packet_use_stub(, packet_stub, sizeof packet_stub);
>
>  struct eth_addr eth_src;
>  eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, _src);
> -ovs_be32 ip4_src;
> -ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
> -
> -dp_packet_use_stub(, packet_stub, sizeof packet_stub);
> -pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
> - ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
> - IPPROTO_TCP, 63, TCP_HEADER_LEN);
> +if (svc_mon->is_ip6) {
> +struct in6_addr ip6_src;
> +ipv6_parse(svc_mon->sb_svc_mon->src_ip, _src);
>

Do we need to parse the address again? The svc_mon has an ip field which
should contain already parsed mapped IP, so we would just need to extract
it.



> +pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
> + _src, _mon->ip, IPPROTO_TCP,
> + 63, TCP_HEADER_LEN);
> +} else {
> +ovs_be32 ip4_src;
> +

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

2023-01-23 Thread Mark Michelson


Thanks for adding this Lorenzo,

Acked-by: Mark Michelson 

On 12/16/22 12:05, Lorenzo Bianconi wrote:

Add Similar to IPv4 counterpart, introduce IPv6 load-balancer health
check support.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2136094
Signed-off-by: Lorenzo Bianconi 
---
Changes since v1:
- fix potential crash in ovn-northd
- improve documentation
---
  controller/pinctrl.c| 213 -
  northd/northd.c |  79 ++
  northd/ovn-northd.8.xml |  17 +++
  ovn-nb.xml  |  21 ++--
  tests/ovn.at| 201 ++-
  tests/system-ovn.at | 230 +++-
  6 files changed, 658 insertions(+), 103 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f1176a2f2..d7dbdab0e 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -6740,7 +6740,7 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
  ovs_be32 ip4;
  if (ip_parse(sb_svc_mon->ip, )) {
  ip_addr = in6_addr_mapped_ipv4(ip4);
-} else {
+} else if (!ipv6_parse(sb_svc_mon->ip, _addr)) {
  continue;
  }
  
@@ -6753,16 +6753,27 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,

  continue;
  }
  
-for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {

-if (ip4 == laddrs.ipv4_addrs[j].addr) {
-ea = laddrs.ea;
-mac_found = true;
-break;
+if (IN6_IS_ADDR_V4MAPPED(_addr)) {
+for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
+if (ip4 == laddrs.ipv4_addrs[j].addr) {
+ea = laddrs.ea;
+mac_found = true;
+break;
+}
+}
+} else {
+for (size_t j = 0; j < laddrs.n_ipv6_addrs; j++) {
+if (IN6_ARE_ADDR_EQUAL(_addr,
+   _addrs[j].addr)) {
+ea = laddrs.ea;
+mac_found = true;
+break;
+}
  }
  }
  
-if (!mac_found && !laddrs.n_ipv4_addrs) {

-/* IPv4 address(es) are not configured. Use the first mac. */
+if (!mac_found && !laddrs.n_ipv4_addrs && !laddrs.n_ipv6_addrs) {
+/* IP address(es) are not configured. Use the first mac. */
  ea = laddrs.ea;
  mac_found = true;
  }
@@ -6796,7 +6807,7 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
  svc_mon->port_key = port_key;
  svc_mon->proto_port = sb_svc_mon->port;
  svc_mon->ip = ip_addr;
-svc_mon->is_ip6 = false;
+svc_mon->is_ip6 = !IN6_IS_ADDR_V4MAPPED(_addr);
  svc_mon->state = SVC_MON_S_INIT;
  svc_mon->status = SVC_MON_ST_UNKNOWN;
  svc_mon->protocol = protocol;
@@ -7564,26 +7575,30 @@ svc_monitor_send_tcp_health_check__(struct rconn 
*swconn,
  ovs_be32 tcp_ack,
  ovs_be16 tcp_src)
  {
-if (svc_mon->is_ip6) {
-return;
-}
-
  /* Compose a TCP-SYN packet. */
  uint64_t packet_stub[128 / 8];
  struct dp_packet packet;
+dp_packet_use_stub(, packet_stub, sizeof packet_stub);
  
  struct eth_addr eth_src;

  eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, _src);
-ovs_be32 ip4_src;
-ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
-
-dp_packet_use_stub(, packet_stub, sizeof packet_stub);
-pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
- ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
- IPPROTO_TCP, 63, TCP_HEADER_LEN);
+if (svc_mon->is_ip6) {
+struct in6_addr ip6_src;
+ipv6_parse(svc_mon->sb_svc_mon->src_ip, _src);
+pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
+ _src, _mon->ip, IPPROTO_TCP,
+ 63, TCP_HEADER_LEN);
+} else {
+ovs_be32 ip4_src;
+ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
+pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
+ ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
+ IPPROTO_TCP, 63, TCP_HEADER_LEN);
+}
  
  struct tcp_header *th = dp_packet_l4();

  dp_packet_set_l4(, th);
+th->tcp_csum = 0;
  th->tcp_dst = htons(svc_mon->proto_port);
  th->tcp_src = tcp_src;
  
@@ -7594,7 +7609,11 @@ svc_monitor_send_tcp_health_check__(struct rconn *swconn,

  th->tcp_winsz = htons(65160);
  
  uint32_t csum;

-csum = packet_csum_pseudoheader(dp_packet_l3());
+if (svc_mon->is_ip6) {
+csum = packet_csum_pseudoheader6(dp_packet_l3());
+

[ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

2022-12-16 Thread Lorenzo Bianconi

Add Similar to IPv4 counterpart, introduce IPv6 load-balancer health
check support.

Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2136094
Signed-off-by: Lorenzo Bianconi 
---
Changes since v1:
- fix potential crash in ovn-northd
- improve documentation
---
 controller/pinctrl.c| 213 -
 northd/northd.c |  79 ++
 northd/ovn-northd.8.xml |  17 +++
 ovn-nb.xml  |  21 ++--
 tests/ovn.at| 201 ++-
 tests/system-ovn.at | 230 +++-
 6 files changed, 658 insertions(+), 103 deletions(-)

diff --git a/controller/pinctrl.c b/controller/pinctrl.c
index f1176a2f2..d7dbdab0e 100644
--- a/controller/pinctrl.c
+++ b/controller/pinctrl.c
@@ -6740,7 +6740,7 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
 ovs_be32 ip4;
 if (ip_parse(sb_svc_mon->ip, )) {
 ip_addr = in6_addr_mapped_ipv4(ip4);
-} else {
+} else if (!ipv6_parse(sb_svc_mon->ip, _addr)) {
 continue;
 }
 
@@ -6753,16 +6753,27 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
 continue;
 }
 
-for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
-if (ip4 == laddrs.ipv4_addrs[j].addr) {
-ea = laddrs.ea;
-mac_found = true;
-break;
+if (IN6_IS_ADDR_V4MAPPED(_addr)) {
+for (size_t j = 0; j < laddrs.n_ipv4_addrs; j++) {
+if (ip4 == laddrs.ipv4_addrs[j].addr) {
+ea = laddrs.ea;
+mac_found = true;
+break;
+}
+}
+} else {
+for (size_t j = 0; j < laddrs.n_ipv6_addrs; j++) {
+if (IN6_ARE_ADDR_EQUAL(_addr,
+   _addrs[j].addr)) {
+ea = laddrs.ea;
+mac_found = true;
+break;
+}
 }
 }
 
-if (!mac_found && !laddrs.n_ipv4_addrs) {
-/* IPv4 address(es) are not configured. Use the first mac. */
+if (!mac_found && !laddrs.n_ipv4_addrs && !laddrs.n_ipv6_addrs) {
+/* IP address(es) are not configured. Use the first mac. */
 ea = laddrs.ea;
 mac_found = true;
 }
@@ -6796,7 +6807,7 @@ sync_svc_monitors(struct ovsdb_idl_txn *ovnsb_idl_txn,
 svc_mon->port_key = port_key;
 svc_mon->proto_port = sb_svc_mon->port;
 svc_mon->ip = ip_addr;
-svc_mon->is_ip6 = false;
+svc_mon->is_ip6 = !IN6_IS_ADDR_V4MAPPED(_addr);
 svc_mon->state = SVC_MON_S_INIT;
 svc_mon->status = SVC_MON_ST_UNKNOWN;
 svc_mon->protocol = protocol;
@@ -7564,26 +7575,30 @@ svc_monitor_send_tcp_health_check__(struct rconn 
*swconn,
 ovs_be32 tcp_ack,
 ovs_be16 tcp_src)
 {
-if (svc_mon->is_ip6) {
-return;
-}
-
 /* Compose a TCP-SYN packet. */
 uint64_t packet_stub[128 / 8];
 struct dp_packet packet;
+dp_packet_use_stub(, packet_stub, sizeof packet_stub);
 
 struct eth_addr eth_src;
 eth_addr_from_string(svc_mon->sb_svc_mon->src_mac, _src);
-ovs_be32 ip4_src;
-ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
-
-dp_packet_use_stub(, packet_stub, sizeof packet_stub);
-pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
- ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
- IPPROTO_TCP, 63, TCP_HEADER_LEN);
+if (svc_mon->is_ip6) {
+struct in6_addr ip6_src;
+ipv6_parse(svc_mon->sb_svc_mon->src_ip, _src);
+pinctrl_compose_ipv6(, eth_src, svc_mon->ea,
+ _src, _mon->ip, IPPROTO_TCP,
+ 63, TCP_HEADER_LEN);
+} else {
+ovs_be32 ip4_src;
+ip_parse(svc_mon->sb_svc_mon->src_ip, _src);
+pinctrl_compose_ipv4(, eth_src, svc_mon->ea,
+ ip4_src, in6_addr_get_mapped_ipv4(_mon->ip),
+ IPPROTO_TCP, 63, TCP_HEADER_LEN);
+}
 
 struct tcp_header *th = dp_packet_l4();
 dp_packet_set_l4(, th);
+th->tcp_csum = 0;
 th->tcp_dst = htons(svc_mon->proto_port);
 th->tcp_src = tcp_src;
 
@@ -7594,7 +7609,11 @@ svc_monitor_send_tcp_health_check__(struct rconn *swconn,
 th->tcp_winsz = htons(65160);
 
 uint32_t csum;
-csum = packet_csum_pseudoheader(dp_packet_l3());
+if (svc_mon->is_ip6) {
+csum = packet_csum_pseudoheader6(dp_packet_l3());
+} else {
+csum = packet_csum_pseudoheader(dp_packet_l3());
+}
 csum = csum_continue(csum, th, dp_packet_size() -

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

Re: [ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

[ovs-dev] [PATCH v2 ovn] Add IPv6 support for lb health-check

4 matches

Site Navigation

Mail list logo

Footer information