Re: [ovs-dev] [PATCH ovn 4/4] multicast: Properly flood IGMP queries and reports.

[Lucas Alvares Gomes]

On Thu, Jul 7, 2022 at 12:24 PM Lucas Alvares Gomes
 wrote:
>
> Hi,
>
> I tested this series of patches with OpenStack upstream [0] since we
> have some automated tests for IGMP and it seems good:
>
> test_multicast_between_vms_on_same_network[id-113486fc-24c9-4be4-8361-03b1c9892867]
> pass (link: 
> https://98794ab76263ee253bc7-6b6bf6aa2495d781bdba3c8c61916451.ssl.cf1.rackcdn.com/848934/2/check/neutron-tempest-plugin-ovn/b3191bf/testr_results.html,
> this will expire at some point)
>
> What I did was to change ML2/OVN to set mcast_flood to False on the
> LSPs, apply the series of patches in OVN and then run the tests.
>
> Tested-By: 
> https://review.opendev.org/c/openstack/neutron-tempest-plugin/+/848934/
>

Tested-By: Lucas Alvares Gomes 

> [0] https://review.opendev.org/q/topic:test-igmp
>
> Cheers,
> Lucas
>
> On Tue, Jul 5, 2022 at 1:13 PM Dumitru Ceara  wrote:
> >
> > RFC 4541 "Considerations for Internet Group Management Protocol (IGMP)
> > and Multicast Listener Discovery (MLD) Snooping Switches" [0] states:
> >
> > For IGMP packets:
> > 2.1.1.  IGMP Forwarding Rules
> >1) A snooping switch should forward IGMP Membership Reports only to
> >   those ports where multicast routers are attached.
> >
> >   Alternatively stated: a snooping switch should not forward IGMP
> >   Membership Reports to ports on which only hosts are attached.  An
> >   administrative control may be provided to override this
> >   restriction, allowing the report messages to be flooded to other
> >   ports.
> >
> >   This is the main IGMP snooping functionality for the control path.
> >
> >   Sending membership reports to other hosts can result, for IGMPv1
> >   and IGMPv2, in unintentionally preventing a host from joining a
> >   specific multicast group.
> >
> >   When an IGMPv1 or IGMPv2 host receives a membership report for a
> >   group address that it intends to join, the host will suppress its
> >   own membership report for the same group.  This join or message
> >   suppression is a requirement for IGMPv1 and IGMPv2 hosts.
> >   However, if a switch does not receive a membership report from the
> >   host it will not forward multicast data to it.
> >
> > In OVN this translates to forwarding reports only on:
> > a. ports where mrouters have been learned (IGMP queries were received on
> >those ports)
> > b. ports connecting a multicast enabled logical switch to a multicast
> >relay enabled logical router (OVN mrouter)
> > c. ports configured with mcast_flood_reports=true
> >
> > 2.1.2.  Data Forwarding Rules
> >
> >1) Packets with a destination IP address outside 224.0.0.X which are
> >   not IGMP should be forwarded according to group-based port
> >   membership tables and must also be forwarded on router ports.
> >
> > In OVN this translates to forwarding traffic for a group G to:
> > a. all ports where G was learned
> > b. all ports where an (external or OVN) mrouter was learned.
> > c. all ports configured with mcast_flood=true
> >
> > IGMP queries are already snooped by ovn-controller.  Just propagate the
> > information about where mrouters are to the Southbound DB through means
> > of a custom IGMP_Group (name="mrouters").
> >
> > Snooped queries are then re-injected in the OVS pipeline with outport
> > set to MC_FLOOD_L2 (only flood them in the L2 domain).
> >
> > Snooped reports are re-injected in the OVS pipeline with outport set to
> > MC_MROUTER_FLOOD (flood them to snooped mrouters and OVN mrouters).
> >
> > The same behavior applies to IPv6 too (MLD).
> >
> > [0] https://datatracker.ietf.org/doc/html/rfc4541
> >
> > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1933990
> > Reported-at: https://github.com/ovn-org/ovn/issues/126
> > Signed-off-by: Dumitru Ceara 
> > ---
> >  controller/ip-mcast.c   |  129 -
> >  controller/ip-mcast.h   |   14 
> >  controller/pinctrl.c|  129 +
> >  lib/ip-mcast-index.h|5 +
> >  lib/mcast-group-index.h |4 -
> >  northd/northd.c |   54 +++
> >  northd/ovn-northd.8.xml |6 --
> >  tests/ovn-macros.at |   25 +++
> >  tests/ovn.at|  164 
> > ++-
> >  9 files changed, 472 inser

Re: [ovs-dev] [PATCH ovn 4/4] multicast: Properly flood IGMP queries and reports.

[Lucas Alvares Gomes]

Hi,

I tested this series of patches with OpenStack upstream [0] since we
have some automated tests for IGMP and it seems good:

test_multicast_between_vms_on_same_network[id-113486fc-24c9-4be4-8361-03b1c9892867]
pass (link: 
https://98794ab76263ee253bc7-6b6bf6aa2495d781bdba3c8c61916451.ssl.cf1.rackcdn.com/848934/2/check/neutron-tempest-plugin-ovn/b3191bf/testr_results.html,
this will expire at some point)

What I did was to change ML2/OVN to set mcast_flood to False on the
LSPs, apply the series of patches in OVN and then run the tests.

Tested-By: 
https://review.opendev.org/c/openstack/neutron-tempest-plugin/+/848934/

[0] https://review.opendev.org/q/topic:test-igmp

Cheers,
Lucas

On Tue, Jul 5, 2022 at 1:13 PM Dumitru Ceara  wrote:
>
> RFC 4541 "Considerations for Internet Group Management Protocol (IGMP)
> and Multicast Listener Discovery (MLD) Snooping Switches" [0] states:
>
> For IGMP packets:
> 2.1.1.  IGMP Forwarding Rules
>1) A snooping switch should forward IGMP Membership Reports only to
>   those ports where multicast routers are attached.
>
>   Alternatively stated: a snooping switch should not forward IGMP
>   Membership Reports to ports on which only hosts are attached.  An
>   administrative control may be provided to override this
>   restriction, allowing the report messages to be flooded to other
>   ports.
>
>   This is the main IGMP snooping functionality for the control path.
>
>   Sending membership reports to other hosts can result, for IGMPv1
>   and IGMPv2, in unintentionally preventing a host from joining a
>   specific multicast group.
>
>   When an IGMPv1 or IGMPv2 host receives a membership report for a
>   group address that it intends to join, the host will suppress its
>   own membership report for the same group.  This join or message
>   suppression is a requirement for IGMPv1 and IGMPv2 hosts.
>   However, if a switch does not receive a membership report from the
>   host it will not forward multicast data to it.
>
> In OVN this translates to forwarding reports only on:
> a. ports where mrouters have been learned (IGMP queries were received on
>those ports)
> b. ports connecting a multicast enabled logical switch to a multicast
>relay enabled logical router (OVN mrouter)
> c. ports configured with mcast_flood_reports=true
>
> 2.1.2.  Data Forwarding Rules
>
>1) Packets with a destination IP address outside 224.0.0.X which are
>   not IGMP should be forwarded according to group-based port
>   membership tables and must also be forwarded on router ports.
>
> In OVN this translates to forwarding traffic for a group G to:
> a. all ports where G was learned
> b. all ports where an (external or OVN) mrouter was learned.
> c. all ports configured with mcast_flood=true
>
> IGMP queries are already snooped by ovn-controller.  Just propagate the
> information about where mrouters are to the Southbound DB through means
> of a custom IGMP_Group (name="mrouters").
>
> Snooped queries are then re-injected in the OVS pipeline with outport
> set to MC_FLOOD_L2 (only flood them in the L2 domain).
>
> Snooped reports are re-injected in the OVS pipeline with outport set to
> MC_MROUTER_FLOOD (flood them to snooped mrouters and OVN mrouters).
>
> The same behavior applies to IPv6 too (MLD).
>
> [0] https://datatracker.ietf.org/doc/html/rfc4541
>
> Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1933990
> Reported-at: https://github.com/ovn-org/ovn/issues/126
> Signed-off-by: Dumitru Ceara 
> ---
>  controller/ip-mcast.c   |  129 -
>  controller/ip-mcast.h   |   14 
>  controller/pinctrl.c|  129 +
>  lib/ip-mcast-index.h|5 +
>  lib/mcast-group-index.h |4 -
>  northd/northd.c |   54 +++
>  northd/ovn-northd.8.xml |6 --
>  tests/ovn-macros.at |   25 +++
>  tests/ovn.at|  164 
> ++-
>  9 files changed, 472 insertions(+), 58 deletions(-)
>
> diff --git a/controller/ip-mcast.c b/controller/ip-mcast.c
> index 9b0b4465a..a870fb29e 100644
> --- a/controller/ip-mcast.c
> +++ b/controller/ip-mcast.c
> @@ -16,6 +16,7 @@
>  #include 
>
>  #include "ip-mcast.h"
> +#include "ip-mcast-index.h"
>  #include "lport.h"
>  #include "lib/ovn-sb-idl.h"
>
> @@ -27,6 +28,18 @@ struct igmp_group_port {
>  const struct sbrec_port_binding *port;
>  };
>
> +static const struct sbrec_igmp_group *
> +igmp_group_lookup_(struct ovsdb_idl_index *igmp_groups,
> +   const char *addr_str,
> +   const struct sbrec_datapath_binding *datapath,
> +   const struct sbrec_chassis *chassis);
> +
> +static struct sbrec_igmp_group *
> +igmp_group_create_(struct ovsdb_idl_txn *idl_txn,
> +   const char *addr_

Re: [ovs-dev] [PATCH ovn] Allow for setting the Next server IP in the DHCP header

[Lucas Alvares Gomes]

On Thu, May 19, 2022 at 10:05 PM Numan Siddique  wrote:
>
> On Thu, May 19, 2022 at 4:11 AM Lucas Alvares Gomes  
> wrote:
> >
> > Hi,
> >
> > Thanks Numan for the review. See the replies below.
> >
> > On Thu, May 19, 2022 at 12:36 AM Numan Siddique  wrote:
> > >
> > > On Wed, May 11, 2022 at 11:34 AM  wrote:
> > > >
> > > > From: Lucas Alvares Gomes 
> > > >
> > > > In order to PXE boot a baremetal server using the OVN DHCP server we
> > > > need to allow users to set the "next-server" (siaddr) [0] field in the
> > > > DHCP header.
> > > >
> > > > While investigating this issue by comparing the DHCPOFFER and DHCPACK
> > > > packets sent my dnsmasq and OVN we saw that the "next-server" field
> > > > was the problem for OVN, without it PXE booting was timing out while
> > > > fetching the iPXE image from the TFTP server (see the bugzilla ticket
> > > > below for reference).
> > > >
> > > > To confirm this problem we created a bogus patch hardcoding the TFTP
> > > > address in the siaddr of the DHCP header (see the discussion in the
> > > > maillist below) and with this in place we were able to deploy a
> > > > baremetal node using the OVN DHCP end-to-end.
> > > >
> > > > This patch is a proper implementation that creates a new DHCP
> > > > configuration option called "next_server" to allow users to set this
> > > > field dynamically. This patch uses the DHCP code 253 which is a unsed
> > > > code for DHCP specification as this is not a normal DHCP option but a
> > > > special use case in OVN.
> > > >
> > > > [0]
> > > > https://github.com/openvswitch/ovs/blob/9dd3031d2e0e9597449e95428320ccaaff7d8b3d/lib/dhcp.h#L42
> > > >
> > > > Reported-by: https://bugzilla.redhat.com/show_bug.cgi?id=2083629
> > > > Reported-by:
> > > > https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051821.html
> > > > Signed-off-by: Lucas Alvares Gomes 
> > > > ---
> > > >  controller/pinctrl.c | 69 ++--
> > > >  lib/actions.c| 13 -
> > > >  lib/ovn-l7.h |  8 +
> > > >  northd/ovn-northd.c  |  1 +
> > > >  ovn-nb.xml   |  7 +
> > > >  tests/ovn.at |  6 ++--
> > > >  tests/test-ovn.c |  1 +
> > > >  7 files changed, 80 insertions(+), 25 deletions(-)
> > > >
> > > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > > index ae3da332c..428863293 100644
> > > > --- a/controller/pinctrl.c
> > > > +++ b/controller/pinctrl.c
> > > > @@ -2203,30 +2203,56 @@ pinctrl_handle_put_dhcp_opts(
> > > >   *| 4 Bytes padding | 1 Byte (option end 0xFF ) | 4 Bytes padding|
> > > >   * --
> > > >   */
> > > > -struct dhcp_opt_header *in_dhcp_opt =
> > > > -(struct dhcp_opt_header *)reply_dhcp_opts_ptr->data;
> > > > -if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_CODE) {
> > > > -unsigned char *ptr = (unsigned char *)in_dhcp_opt;
> > > > -int len = sizeof *in_dhcp_opt + in_dhcp_opt->len;
> > > > -struct dhcp_opt_header *next_dhcp_opt =
> > > > -(struct dhcp_opt_header *)(ptr + len);
> > > > -
> > > > -if (next_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > > > -if (!ipxe_req) {
> > > > -ofpbuf_pull(reply_dhcp_opts_ptr, len);
> > > > -next_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > > > -} else {
> > > > -char *buf = xmalloc(len);
> > > > +ovs_be32 next_server = in_dhcp_data->siaddr;
> > > > +bool bootfile_name_set = false;
> > > > +in_dhcp_ptr = reply_dhcp_opts_ptr->data;
> > > > +end = (const char *)reply_dhcp_opts_ptr->data + 
> > > > reply_dhcp_opts_ptr->size;
> > > > +
> > >
> > > Hi Lucas,
> > >
> > > Thanks for adding this support.
> > >
> > > It seems to me this while loop can be avoided since lib/actions.c
> > > always encodes offer_ip, DHCP_OPT_BOOTFILE_CODE,
> > > DHCP_OPT_BOOTFILE_ALT_CODE
> > > and no

Re: [ovs-dev] [PATCH ovn] Allow for setting the Next server IP in the DHCP header

[Lucas Alvares Gomes]

Hi,

Thanks Numan for the review. See the replies below.

On Thu, May 19, 2022 at 12:36 AM Numan Siddique  wrote:
>
> On Wed, May 11, 2022 at 11:34 AM  wrote:
> >
> > From: Lucas Alvares Gomes 
> >
> > In order to PXE boot a baremetal server using the OVN DHCP server we
> > need to allow users to set the "next-server" (siaddr) [0] field in the
> > DHCP header.
> >
> > While investigating this issue by comparing the DHCPOFFER and DHCPACK
> > packets sent my dnsmasq and OVN we saw that the "next-server" field
> > was the problem for OVN, without it PXE booting was timing out while
> > fetching the iPXE image from the TFTP server (see the bugzilla ticket
> > below for reference).
> >
> > To confirm this problem we created a bogus patch hardcoding the TFTP
> > address in the siaddr of the DHCP header (see the discussion in the
> > maillist below) and with this in place we were able to deploy a
> > baremetal node using the OVN DHCP end-to-end.
> >
> > This patch is a proper implementation that creates a new DHCP
> > configuration option called "next_server" to allow users to set this
> > field dynamically. This patch uses the DHCP code 253 which is a unsed
> > code for DHCP specification as this is not a normal DHCP option but a
> > special use case in OVN.
> >
> > [0]
> > https://github.com/openvswitch/ovs/blob/9dd3031d2e0e9597449e95428320ccaaff7d8b3d/lib/dhcp.h#L42
> >
> > Reported-by: https://bugzilla.redhat.com/show_bug.cgi?id=2083629
> > Reported-by:
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2022-May/051821.html
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> >  controller/pinctrl.c | 69 ++--
> >  lib/actions.c| 13 -
> >  lib/ovn-l7.h |  8 +
> >  northd/ovn-northd.c  |  1 +
> >  ovn-nb.xml   |  7 +
> >  tests/ovn.at |  6 ++--
> >  tests/test-ovn.c |  1 +
> >  7 files changed, 80 insertions(+), 25 deletions(-)
> >
> > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > index ae3da332c..428863293 100644
> > --- a/controller/pinctrl.c
> > +++ b/controller/pinctrl.c
> > @@ -2203,30 +2203,56 @@ pinctrl_handle_put_dhcp_opts(
> >   *| 4 Bytes padding | 1 Byte (option end 0xFF ) | 4 Bytes padding|
> >   * --
> >   */
> > -struct dhcp_opt_header *in_dhcp_opt =
> > -(struct dhcp_opt_header *)reply_dhcp_opts_ptr->data;
> > -if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_CODE) {
> > -unsigned char *ptr = (unsigned char *)in_dhcp_opt;
> > -int len = sizeof *in_dhcp_opt + in_dhcp_opt->len;
> > -struct dhcp_opt_header *next_dhcp_opt =
> > -(struct dhcp_opt_header *)(ptr + len);
> > -
> > -if (next_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
> > -if (!ipxe_req) {
> > -ofpbuf_pull(reply_dhcp_opts_ptr, len);
> > -next_dhcp_opt->code = DHCP_OPT_BOOTFILE_CODE;
> > -} else {
> > -char *buf = xmalloc(len);
> > +ovs_be32 next_server = in_dhcp_data->siaddr;
> > +bool bootfile_name_set = false;
> > +in_dhcp_ptr = reply_dhcp_opts_ptr->data;
> > +end = (const char *)reply_dhcp_opts_ptr->data + 
> > reply_dhcp_opts_ptr->size;
> > +
>
> Hi Lucas,
>
> Thanks for adding this support.
>
> It seems to me this while loop can be avoided since lib/actions.c
> always encodes offer_ip, DHCP_OPT_BOOTFILE_CODE,
> DHCP_OPT_BOOTFILE_ALT_CODE
> and now DHCP_OPT_NEXT_SERVER_CODE  in the userdata buffer before
> encoding other dhcp options.
>
> Since it is deterministic,  can't we do something like below instead
> of the while loop ?
>
> struct dhcp_opt_header *in_dhcp_opt =
> (struct dhcp_opt_header *)reply_dhcp_opts_ptr->data;
> if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_CODE) {
>
>advance in_dhcp_opt to the next option
> } else if (in_dhcp_opt->code == DHCP_OPT_BOOTFILE_ALT_CODE) {
>   ..
>  advance in_dhcp_opt to the next option
> }
>
> if (in_dhcp_opt->code == DHCP_OPT_NEXT_SERVER_CODE) {
> next_server = get_unaligned_be32(DHCP_OPT_PAYLOAD(in_dhcp_opt));
> }
>
> Let me know if this gets complicated because of my above suggestion.
> In that case,I'm fine to run the below while loop.
>

That's how I coded it the first time when I was testing the patch, but
I found a problem where if more than one option was set, f

Re: [ovs-dev] [PATCH ovn] ovn-northd: Don't add arp responder flows for lports with 'unknown' address.

[Lucas Alvares Gomes]

Hi Numan,

Can we backport this change to 20.03 please ? I did locally, and
should be a clean merge.

The OVN packages from Ubuntu LTS 20.04 (focal) which is used upstream
to test OpenStack is based on 20.03 and this patch fixes an issue we
have with one tempest test [0]

[0] https://bugs.launchpad.net/tempest/+bug/1728886

Cheers,
Lucas

On Fri, Mar 20, 2020 at 8:58 AM Numan Siddique  wrote:
>
> On Thu, Mar 19, 2020 at 9:51 PM Han Zhou  wrote:
> >
> > On Thu, Mar 19, 2020 at 5:27 AM  wrote:
> > >
> > > From: Numan Siddique 
> > >
> > > If a logical port has 'unknown' address, it means it can send and receive
> > > packet with any IP and MAC and generally port security is not set for
> > > such logical ports. If an lport has addresses set to - ["MAC1 IP1",
> > unknown],
> > > right now we add arp responder flows for IP1 and respond MAC1 in the arp
> > > response. But it's possible that the VIF of the logical port can use the
> > IP1
> > > with a different MAC. This patch supports this usecase. When another
> > logical port
> > > sends ARP request for IP1, the VIF of the logical port will anyway
> > respond.
> > >
> > > Reported-by: Maciej Józefczyk 
> > > Signed-off-by: Numan Siddique 
> > > ---
> > >  northd/ovn-northd.8.xml |  5 +++--
> > >  northd/ovn-northd.c | 13 -
> > >  tests/ovn.at| 16 
> > >  3 files changed, 23 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> > > index 9b44720d1..7d03cbc83 100644
> > > --- a/northd/ovn-northd.8.xml
> > > +++ b/northd/ovn-northd.8.xml
> > > @@ -699,8 +699,9 @@ output;
> > >
> > >  
> > >These flows are omitted for logical ports (other than router
> > ports or
> > > -  localport ports) that are down and for logical
> > ports of
> > > -  type virtual.
> > > +  localport ports) that are down, for logical ports
> > of
> > > +  type virtual and for logical ports with 'unknown'
> > > +  address set.
> > >  
> > >
> > >
> > > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > > index 4f94680b5..f648d2ea7 100644
> > > --- a/northd/ovn-northd.c
> > > +++ b/northd/ovn-northd.c
> > > @@ -1152,7 +1152,7 @@ struct ovn_port {
> > >
> > >  bool derived; /* Indicates whether this is an additional port
> > > * derived from nbsp or nbrp. */
> > > -
> > > +bool has_unknown; /* If the addresses have 'unknown' defined. */
> > >  /* The port's peer:
> > >   *
> > >   * - A switch port S of type "router" has a router port R as a
> > peer,
> > > @@ -2059,8 +2059,11 @@ join_logical_ports(struct northd_context *ctx,
> > >  op->lsp_addrs
> > >  = xmalloc(sizeof *op->lsp_addrs * nbsp->n_addresses);
> > >  for (size_t j = 0; j < nbsp->n_addresses; j++) {
> > > -if (!strcmp(nbsp->addresses[j], "unknown")
> > > -|| !strcmp(nbsp->addresses[j], "router")) {
> > > +if (!strcmp(nbsp->addresses[j], "unknown")) {
> > > +op->has_unknown = true;
> > > +continue;
> > > +}
> > > +if (!strcmp(nbsp->addresses[j], "router")) {
> > >  continue;
> > >  }
> > >  if (is_dynamic_lsp_address(nbsp->addresses[j])) {
> > > @@ -6127,7 +6130,7 @@ build_lswitch_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > >  } else {
> > >  /*
> > >   * Add ARP/ND reply flows if either the
> > > - *  - port is up or
> > > + *  - port is up and it doesn't have 'unknown' address
> > defined or
> > >   *  - port type is router or
> > >   *  - port type is localport
> > >   */
> > > @@ -6136,7 +6139,7 @@ build_lswitch_flows(struct hmap *datapaths, struct
> > hmap *ports,
> > >  continue;
> > >  }
> > >
> > > -if (lsp_is_external(op->nbsp)) {
> > > +if (lsp_is_external(op->nbsp) || op->has_unknown) {
> > >  continue;
> > >  }
> > >
> > > diff --git a/tests/ovn.at b/tests/ovn.at
> > > index 8cdbad743..1b6073ff0 100644
> > > --- a/tests/ovn.at
> > > +++ b/tests/ovn.at
> > > @@ -1758,11 +1758,13 @@ for is in 1 2 3; do
> > >  sip=`ip_to_hex 192 168 0 $is$js`
> > >  tip=`ip_to_hex 192 168 0 $id$jd`
> > >  tip_unknown=`ip_to_hex 11 11 11 11`
> > > +reply_ha=;
> > >  if test $d != $s; then
> > > -reply_ha=f0$d
> > > -else
> > > -reply_ha=
> > > +if test $jd != 1; then
> > > +reply_ha=f0$d
> > > +fi
> > >  fi
> > > +
> > >  test_arp $

Re: [ovs-dev] [PATCH ovn v3] Avoid nb_cfg update notification flooding

[Lucas Alvares Gomes]

Thanks Han for picking up this work again.

I tested it in a local environment and it works.

On Fri, Jul 31, 2020 at 7:40 AM Han Zhou  wrote:
>
> nb_cfg as a mechanism to "ping" OVN control plane is very useful
> in many ways. However, the current implementation will trigger
> update notifications flooding in the whole control plane. Each
> HV updates to SB the nb_cfg number and all these updates are
> notified to all the other HVs, which is O(n^2). Although updates
> are batched in fewers notifications than n^2, it still generates
> significant load on SB DB, ovn-controllers and ovn-northd as well.
>
> To solve this problem and make the mechanism more useful in large
> scale producation deployment, this patch separates the per HV
> *private* data (write only by the owning chassis and not
> interesting to any other HVs) from the Chassis table to a separate
> table, so that each HV can conditionally monitor and get updates
> only for its own record.
>
> Test result shows great improvement:
> In a test environment with 1200 sandbox HVs, and 12K ports created
> on 80 lswitches and 1 lrouter, do the sync test when the system
> is idle, with command:
>
> time ovn-nbctl --wait=hv sync
>
> Original result:
> real0m13.724s
> user0m0.295s
> sys 0m0.012s
>
> With this patch:
> real0m3.255s
> user0m0.248s
> sys 0m0.020s
>
> Also, regarding backwards compatibility note that the nb_cfg from the
> Chassis table is no longer updated. If any system is relying on this
> mechanism they should start using the nb_cfg from the Chassis_Private
> table from now on.
>
> Co-authored-by: Lucas Alvares Gomes 
> Signed-off-by: Lucas Alvares Gomes 
> Signed-off-by: Han Zhou 
> ---
> v2 -> v3:
> - Rebase on master.
> - Fixed the RBAC problem found by Numan in v2.
> - Took care of "is-remote" check.
> - Renamed column "name" in Chassis_Private table to "chassis_name" to
>   be more clear that it is a reference to the Chassis table.
> - Removed TODO about ovsdb monitor_cond change since there is no plan
>   to do that.
> - Updated test result in commit message based on latest scale test,
>   which is quite different from the old one because I-P has been added.
>
>  NEWS|  5 +
>  controller/chassis.c| 20 +++--
>  controller/chassis.h|  8 +--
>  controller/ovn-controller.c | 37 ++--
>  lib/chassis-index.c | 26 +++
>  lib/chassis-index.h |  6 ++
>  northd/ovn-northd.c | 52 
> +
>  ovn-sb.ovsschema| 13 ++--
>  ovn-sb.xml  | 37 
>  tests/ovn-controller.at | 26 +++
>  10 files changed, 205 insertions(+), 25 deletions(-)
>
> diff --git a/NEWS b/NEWS
> index f89a93a..a1ce4e8 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -6,6 +6,11 @@ Post-v20.06.0
>   OVN DHCPv4 responder.
> - Added support for DHCP domain search option (119) in native
>   OVN DHCPv4 responder.
> +   - The nb_cfg column from the Chassis table in the OVN Southbound
> + database has been deprecated and is no longer updated. A new table
> + called Chassis_Private now contains the nb_cfg column which is updated
> + by incrementing the value in the NB_Global table, CMSes relying on
> + this mechanism should update their code to use this new table.
>
>  OVN v20.06.0
>  --
> diff --git a/controller/chassis.c b/controller/chassis.c
> index eec270e..7a1d5b5 100644
> --- a/controller/chassis.c
> +++ b/controller/chassis.c
> @@ -604,14 +604,18 @@ chassis_update(const struct sbrec_chassis *chassis_rec,
>  const struct sbrec_chassis *
>  chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>  struct ovsdb_idl_index *sbrec_chassis_by_name,
> +struct ovsdb_idl_index *sbrec_chassis_private_by_name,
>  const struct ovsrec_open_vswitch_table *ovs_table,
>  const struct sbrec_chassis_table *chassis_table,
>  const char *chassis_id,
>  const struct ovsrec_bridge *br_int,
> -const struct sset *transport_zones)
> +const struct sset *transport_zones,
> +const struct sbrec_chassis_private **chassis_private)
>  {
>  struct ovs_chassis_cfg ovs_cfg;
>
> +*chassis_private = NULL;
> +
>  /* Get the chassis config from the ovs table. */
>  ovs_chassis_cfg_init(&ovs_cfg);
>  if (!chassis_parse_ovs_config(ovs_table, br_int, &ovs_cfg)) 

Re: [ovs-dev] [PATCH ovn v2] Avoid nb_cfg update notification flooding

[Lucas Alvares Gomes Martins]

On Wed, Jul 29, 2020 at 8:03 PM Han Zhou  wrote:
>
>
>
> On Wed, Jul 29, 2020 at 2:46 AM Lucas Alvares Gomes  
> wrote:
> >
> > Hi Han,
> >
> > >
> > > Just to follow up. I didn't find any new versions for this patch and it
> > > doesn't seem to be merged. Did you send any new versions or do you still
> > > plan to work on this?
> > >
> >
> > Thanks for this follow up.
> >
> > So I am not currently working on it but the work is still in our
> > backlog. Many of the problems that we had with the mechanism
> > incrementing the "nb_cfg" has been mitigated by some code in the
> > OpenStack side as well as this commit for core OVN [0] that disabled
> > flow recomputation upon updates to the Chassis' external_ids column
> > (we had a few updates to that column every time the nb_cfg was
> > bumped).
> >
> > But, I can try to check if I can get some prioritization and start
> > working again on this patch. Is this something that you need as well ?
> >
> > [0] 
> > https://github.com/ovn-org/ovn/commit/74d90c2223d0a8c123823fb849b4c2de58c296e4
> >
> > Cheers,
> > Lucas
>
> Thanks Lucas for the update.
> Yes, I need it for latency analysis in scale test. I can pick it up. I 
> rebased on master and also fixed the RBAC issue and some minor updates. I 
> will post it later this week.
>

Thank you very much Han!

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH ovn v2] Avoid nb_cfg update notification flooding

[Lucas Alvares Gomes]

Hi Han,

>
> Just to follow up. I didn't find any new versions for this patch and it
> doesn't seem to be merged. Did you send any new versions or do you still
> plan to work on this?
>

Thanks for this follow up.

So I am not currently working on it but the work is still in our
backlog. Many of the problems that we had with the mechanism
incrementing the "nb_cfg" has been mitigated by some code in the
OpenStack side as well as this commit for core OVN [0] that disabled
flow recomputation upon updates to the Chassis' external_ids column
(we had a few updates to that column every time the nb_cfg was
bumped).

But, I can try to check if I can get some prioritization and start
working again on this patch. Is this something that you need as well ?

[0] 
https://github.com/ovn-org/ovn/commit/74d90c2223d0a8c123823fb849b4c2de58c296e4

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] OVN: configuration in Neutron DB?

[Lucas Alvares Gomes]

Hi,

On Wed, Jul 29, 2020 at 7:42 AM Numan Siddique  wrote:
>
> Adding Daniel and Lucas. Maybe you can also include opendev ML to get
> appropriate responses from the OpenStack side.
>
> Please see below for few comments.
>
>
> On Wed, Jul 29, 2020 at 12:02 PM Tony Liu  wrote:
>
> > Quick update. I changed the script to create 256 routers first, then set
> > each of them as gateway.
> > There is no create and set back to back. It seems working fine now.
> >
> > It would be good someone can clarify my questions. It seems that it's
> > not guaranteed that the
> > object is ready when client get OK response of creation request. Is this
> > expected?
> >
> >
> > Thanks!
> >
> > Tony
> >
> > --
> > *From:* dev  on behalf of Tony Liu <
> > tonyliu0...@hotmail.com>
> > *Sent:* July 28, 2020 10:37 PM
> > *To:* ovs-disc...@openvswitch.org ;
> > ovs-dev@openvswitch.org 
> > *Subject:* [ovs-dev] OVN: configuration in Neutron DB?
> >
> > Hi,
> >
> > In case of integration with OpenStack, for example, when a client requests
> > to create a network,
> > is this network configuration saved in both Neutron DB and OVN DB, or OVN
> > DB only?
> >
>
> The neutron API first saves in the neutron db and the neutron OVN mechanism
> driver will talk
> to the Northbound ovsdb-server and create corresponding OVN logical
> resources.
>

Both as numans said, it's first created in the Neutron database and
then the OVN plugin is invoked to create that resource in the OVN NB
database. We usually use the "name" column in the OVN NB DB to store
the correspondent Neutron UUID, if the resource has no "name" column
we then use the external_ids columns to do it.

In your example, a Neutron network corresponds to a Logical_Switch in OVN:

stack@lucas-devstack-2:~/neutron$ openstack network create test
| id| 702c4ccd-2f9e-43c8-99a8-4b07a509e105 |
...

stack@lucas-devstack-2:~/neutron$ ovn-nbctl list logical_switch
external_ids: {"neutron:mtu"="1442",
"neutron:network_name"=test, "neutron:revision_number"="1"}
name: neutron-702c4ccd-2f9e-43c8-99a8-4b07a509e105\
...

So the correspondent Logical Switch in OVN is named as "neutron-"

>
> > Also, when a client gets a network from Neutron API, is the configuration
> > read from Neutron DB
> > or OVN DB?
> >
>
> I think its read from the neutron DB.
>

That's right, it reads from the Neutron database.

>
>
> >
> > Other than coding, is there any doc about how Neutron OVN ML2 driver works?
> >
>
> You can refer here -
> https://docs.openstack.org/neutron/latest/admin/ovn/refarch/refarch.html
>

Yeah that's the part of the documentation we have that matches with
your questions, let us know if you have more questions and perhaps we
can enhance that document with the answers.


>
> >
> > I have this script to create 256 routers and set each of them as gateway.
> > router()
> > {
> > local op=$1
> >
> > for c in `seq 0 1 255`; do
> > echo "INFO: $op router-$c..."
> > openstack router $op router-$c
> > if [ "$op" == "create" ]; then
> > openstack router set \
> > --external-gateway public \
> > --fixed-ip ip-address=10.6.33.$c \
> > --disable-snat \
> > router-$c
> > fi
> > done
> > }
> > I see lots failures from Neutron log when get/show a router. It seems like
> > that, when setting a router,
> > the router is not completely ready yet. Is it possible?
> >
> > After running that script, I see some logical routers in ovn-nb-db don't
> > have gw_port_id. And there
> > are some duplications. Here is an example. Each of them has unique UUID.
> >
> > external_ids: {"neutron:gw_port_id"="",
> > "neutron:revision_number"="1", "neutron:router_name"=router-255}
> > external_ids: {"neutron:gw_port_id"="",
> > "neutron:revision_number"="1", "neutron:router_name"=router-232}
> > external_ids: {"neutron:gw_port_id"="",
> > "neutron:revision_number"="0", "neutron:router_name"=router-158}
> > external_ids: {"neutron:gw_port_id"="",
> > "neutron:revision_number"="0", "neutron:router_name"=router-158}
> > external_ids:
> > {"neutron:gw_port_id"="e52dda53-c914-4ea7-840b-8632a5770680",
> > "neutron:revision_number"="2", "neutron:router_name"=router-158}
> >
> > I enabled nb-db debug logging and searched, eg. router-158, it only shows
> > in a jsonrpc reply message
> > including 3 router-158, as the above.
> >
> > Any clues?
> >
> >
> Maybe Daniel/Lucas can comment.
>
> Thanks
> Numan
>
>
> >
> > Thanks!
> >
> > Tony
> >
> >
> >
> > ___
> > dev mailing list
> > d...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> > ___
> > discuss mailing list
> > disc...@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
> >
> __

Re: [ovs-dev] [PATCH ovn] ovn-controller to no longer monitor Chassis' external_ids

[Lucas Alvares Gomes]

Thank you all for looking into this.

On Tue, May 12, 2020 at 1:37 PM Dumitru Ceara  wrote:
>
> On 5/7/20 12:12 PM, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > Prior to this patch, the external_ids column from the Chassis table were
> > being used for two purposes:
> >
> > 1. Holding configuration for the OVN (copied by ovn-controller from the
> > local OVS database)
> >
> > 2. Holding information from external systems (the main purpose of the
> >external_ids column across other resources).
> >
> > The problem with mixing these two use cases is that, ovn-controller
> > should be aware of changes to the configuration and it would trigger
> > flow re-computations upon those changes. It shouldn't care tho about the
> > information from external systems but, since these two things were put
> > together, CMSs writing things to the external_ids column of the Chassis
> > were waking up ovn-controller to recompute flows.
> >
> > This patch is separating these two things by creating another column in
> > the Chassis table called "other_config". This new table holds the OVN
> > configuration that is copied over from the local OVS db leaving the
> > external_ids column unmonitored and free for other systems to make use
> > of it.
> >
> > This change also keeps things backward compatible by continuing to
> > write the OVN configuration to the external_ids column for external
> > systems that may be reading them from there but, that column is no
> > longer monitored so it won't generate any events. This behavior should
> > be temporary and removed in the future. The note in the NEWS file and
> > comments in the code itself points to the future removal of this
> > behavior.
>
> Hi Lucas,
>
> I would add a note to TODO.rst to track the fact that we need to stop
> writing to chassis.external_ids in the future and that that part should
> be removed.

Oh indeed, I will add it.

>
> Apart from that I have a few comments further down.
>
> >
> > Reported-At: https://bugzilla.redhat.com/show_bug.cgi?id=1824220
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> >  NEWS| 10 ++-
> >  controller/bfd.c|  2 +-
> >  controller/chassis.c| 48 ++---
> >  controller/encaps.c |  4 +--
> >  controller/ovn-controller.8.xml |  2 +-
> >  controller/ovn-controller.c |  9 ---
> >  controller/physical.c   |  4 +--
> >  ic/ovn-ic.c |  6 ++---
> >  northd/ovn-northd.c |  4 +--
> >  ovn-sb.ovsschema|  7 +++--
> >  ovn-sb.xml  | 14 +-
> >  tests/ovn-controller.at | 16 +--
> >  12 files changed, 71 insertions(+), 55 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c2b7945df..1b33be249 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,7 +1,15 @@
> >  Post-v20.03.0
> >  --
> > - Added support for external_port_range in NAT table.
> > -
> > +   - ovn-controller no longer monitor the external_ids column from
> > + the Chassis table. This was done to avoid having to do a flow
> > + recalculation every time external systems wrote to this column. The
> > + chassis configuration has now being moved to a new column called
> > + "other_config". As a note, the configurations are still be written to
> > + the external_ids column (but no longer triggers an alert) to
> > + keep the backward compatibility with current systems that may be
> > + reading it from that column but, this behavior will be removed
> > + in the future.
> >
> >  OVN v20.03.0 - xx xxx 
> >  --
> > diff --git a/controller/bfd.c b/controller/bfd.c
> > index 2b1e87f6d..b305eb158 100644
> > --- a/controller/bfd.c
> > +++ b/controller/bfd.c
> > @@ -152,7 +152,7 @@ bfd_calculate_chassis(
> >  /* It's an HA chassis. So add the ref_chassis to the bfd set. 
> > */
> >  for (size_t i = 0; i < ha_chassis_grp->n_ref_chassis; i++) {
> >  struct sbrec_chassis *ref_ch = 
> > ha_chassis_grp->ref_chassis[i];
> > -if (smap_get_bool(&ref_ch->external_ids, "is-remote", 
> > false)) {
> > +if (smap_get_bool(&ref_ch->other_config, "is-remote", 
> > false)) {>  continue;
> > 

Re: [ovs-dev] [PATCH ovn] ovn-controller to no longer monitor Chassis' external_ids

[Lucas Alvares Gomes]

Hi,

On Thu, May 7, 2020 at 12:09 PM Daniel Alvarez Sanchez
 wrote:
>
> Thanks Lucas!
>
> The only problem I see is that we still need something like the
> Private_Chassis approach that we discussed or otherwise bumping nb_cfg will
> still be generating N notifications (N == num_chassis) on every write.
> Still, this solves part of the problem.
>

Yeah that's right, because in this case the events will be just
dropped on the client side as far as I understand it (omit_alert() is
being used on the nb_cfg and external_ids column with this patch), so
something like the Private_Chassis is still needed I believe for
preventing the server from sending such events across.

That said, this patch still fixes a valuable problem and makes the
"external_ids" column more consistent across OVN, which is meant to be
used by external systems and not OVN itself. We use external_ids a lot
in OpenStack and triggering flow computation whenever we update is not
ideal.

> I was wondering if we could leverage the Controller_Event table [0] for
> this purpose and generate an event to health check the chassis. Looks like
> ovn-controller monitors only its own chassis so we could be good here. What
> do you think?
>
> [0] https://mail.openvswitch.org/pipermail/ovs-dev/2019-June/359826.html
>

Yeah that looked promising! I've reached out to Dumitru and Lorenzo to
try to understand it better but. it seems like it is the other way
around. The ovn-controller process is the one writing to that table
and the CMSs are suppose to handle such events (and get rid of the
entry when they are done). So I'm afraid we can not leverage this for
the health checks :-(

Cheers,
Lucas

> On Thu, May 7, 2020 at 12:12 PM  wrote:
>
> > From: Lucas Alvares Gomes 
> >
> > Prior to this patch, the external_ids column from the Chassis table were
> > being used for two purposes:
> >
> > 1. Holding configuration for the OVN (copied by ovn-controller from the
> > local OVS database)
> >
> > 2. Holding information from external systems (the main purpose of the
> >external_ids column across other resources).
> >
> > The problem with mixing these two use cases is that, ovn-controller
> > should be aware of changes to the configuration and it would trigger
> > flow re-computations upon those changes. It shouldn't care tho about the
> > information from external systems but, since these two things were put
> > together, CMSs writing things to the external_ids column of the Chassis
> > were waking up ovn-controller to recompute flows.
> >
> > This patch is separating these two things by creating another column in
> > the Chassis table called "other_config". This new table holds the OVN
> > configuration that is copied over from the local OVS db leaving the
> > external_ids column unmonitored and free for other systems to make use
> > of it.
> >
> > This change also keeps things backward compatible by continuing to
> > write the OVN configuration to the external_ids column for external
> > systems that may be reading them from there but, that column is no
> > longer monitored so it won't generate any events. This behavior should
> > be temporary and removed in the future. The note in the NEWS file and
> > comments in the code itself points to the future removal of this
> > behavior.
> >
> > Reported-At: https://bugzilla.redhat.com/show_bug.cgi?id=1824220
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> >  NEWS| 10 ++-
> >  controller/bfd.c|  2 +-
> >  controller/chassis.c| 48 ++---
> >  controller/encaps.c |  4 +--
> >  controller/ovn-controller.8.xml |  2 +-
> >  controller/ovn-controller.c |  9 ---
> >  controller/physical.c   |  4 +--
> >  ic/ovn-ic.c |  6 ++---
> >  northd/ovn-northd.c |  4 +--
> >  ovn-sb.ovsschema|  7 +++--
> >  ovn-sb.xml  | 14 +-
> >  tests/ovn-controller.at | 16 +--
> >  12 files changed, 71 insertions(+), 55 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c2b7945df..1b33be249 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -1,7 +1,15 @@
> >  Post-v20.03.0
> >  --
> > - Added support for external_port_range in NAT table.
> > -
> > +   - ovn-controller no longer monitor the external_ids column from
> > + the Chassis table. This was done to avoid having to do a flow
> > + recalculation every time external system

Re: [ovs-dev] [PATCH ovn] Add support for DHCP options 35 and 38

[Lucas Alvares Gomes]

Hi,

On Thu, Apr 23, 2020 at 4:08 PM Flavio Fernandes  wrote:
>
> Acked-by: Flavio Fernandes mailto:fla...@flaviof.com>>
>
>
> > On Apr 23, 2020, at 10:32 AM, lmart...@redhat.com wrote:
> >
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for two new DHCP options:
> >
> > * Option 35, ARP timeout:
>
> One nit: call it "ARP Cache Timeout" as stated in rfc1232
>
> I would add 'cache' throughout, to be consistent with rfc.
>

Good point. I will send a new patch set changing this option name to
be more aligned with the RFC as u suggested.

Thanks,
Lucas

> >  http://www.networksorcery.com/enp/protocol/bootp/option035.htm
> > * Option 38, TCP Keepalive interval:
> >  http://www.networksorcery.com/enp/protocol/bootp/option038.htm
> >
> > I noticed that these two options were missing in OVN while working in
> > DHCP related code for OpenStack.
> >
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> > lib/ovn-l7.h|  4 
> > northd/ovn-northd.c |  2 ++
> > ovn-nb.xml  | 14 ++
> > tests/ovn.at|  6 +++---
> > tests/test-ovn.c|  2 ++
> > 5 files changed, 25 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/ovn-l7.h b/lib/ovn-l7.h
> > index cbea2a0c8..10569264a 100644
> > --- a/lib/ovn-l7.h
> > +++ b/lib/ovn-l7.h
> > @@ -82,6 +82,10 @@ struct gen_opts_map {
> > #define DHCP_OPT_TFTP_SERVER_ADDRESS \
> > DHCP_OPTION("tftp_server_address", 150, "ipv4")
> >
> > +#define DHCP_OPT_ARP_TIMEOUT DHCP_OPTION("arp_timeout", 35, "uint32")
> > +#define DHCP_OPT_TCP_KEEPALIVE_INTERVAL \
> > +DHCP_OPTION("tcp_keepalive_interval", 38, "uint32")
> > +
> > static inline uint32_t
> > gen_opt_hash(char *opt_name)
> > {
> > diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> > index c4675bd68..d790f9152 100644
> > --- a/northd/ovn-northd.c
> > +++ b/northd/ovn-northd.c
> > @@ -11300,6 +11300,8 @@ static struct gen_opts_map supported_dhcp_opts[] = {
> > DHCP_OPT_PATH_PREFIX,
> > DHCP_OPT_TFTP_SERVER_ADDRESS,
> > DHCP_OPT_DOMAIN_NAME,
> > +DHCP_OPT_ARP_TIMEOUT,
>
> DHCP_OPT_ARP_CACHE_TIMEOUT
>
> > +DHCP_OPT_TCP_KEEPALIVE_INTERVAL,
> > };
> >
> > static struct gen_opts_map supported_dhcpv6_opts[] = {
> > diff --git a/ovn-nb.xml b/ovn-nb.xml
> > index 163dd12ee..6bf54b1f5 100644
> > --- a/ovn-nb.xml
> > +++ b/ovn-nb.xml
> > @@ -2852,6 +2852,19 @@
> >   client begins trying to rebind its address.  The DHCPv4 option 
> > code
> >   for this option is 59.
> > 
> > +
> > + > +type='{"type": "integer", "minInteger": 0, "maxInteger": 
> > 255}'>
> > +  The DHCPv4 option code for this option is 35. This option
> > +  specifies the timeout in seconds for ARP cache entries.
> > +
> > +
> > + > +type='{"type": "integer", "minInteger": 0, "maxInteger": 
> > 255}'>
> > +  The DHCPv4 option code for this option is 38. This option
> > +  specifies the interval that the client TCP should wait before
> > +  sending a keepalive message on a TCP connection.
> > +
> >   
> >
> >   
> > @@ -2898,6 +2911,7 @@
> > resolving hostnames via the Domain Name System.
> >   
> > 
> > +
> >   
> > 
> >
> > diff --git a/tests/ovn.at b/tests/ovn.at
> > index 2a7ee7528..6e1436f97 100644
> > --- a/tests/ovn.at
> > +++ b/tests/ovn.at
> > @@ -1188,9 +1188,9 @@ reg1[0] = put_dhcp_opts(offerip = 1.2.3.4, router = 
> > 10.0.0.1);
> > reg2[5] = 
> > put_dhcp_opts(offerip=10.0.0.4,router=10.0.0.1,netmask=255.255.254.0,mtu=1400,domain_name="ovn.org",wpad="https://example.org",bootfile_name="https://127.0.0.1/boot.ipxe",path_prefix="/tftpboot";);
> > formats as reg2[5] = put_dhcp_opts(offerip = 10.0.0.4, router = 
> > 10.0.0.1, netmask = 255.255.254.0, mtu = 1400, domain_name = "ovn.org", 
> > wpad = "https://example.org";, bootfile_name = 
> > "https://127.0.0.1/boot.ipxe";, path_prefix = "/tftpboot");
> > encodes as 
> > controller(userdata=00.00.00.02.00.00.00.00.00.01.de.10.00.00.00.25.0a.00.00.04.03.04.0a.00.00.01.0

Re: [ovs-dev] [PATCH ovn] [OVN] Avoid nb_cfg update notification flooding

[Lucas Alvares Gomes]

Hi,

On Fri, Mar 20, 2020 at 10:07 AM Dumitru Ceara  wrote:
>
> On 3/13/20 2:18 PM, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > nb_cfg as a mechanism to "ping" OVN control plane is very useful
> > in many ways. However, the current implementation will trigger
> > update notifications flooding in the whole control plane. Each
> > HV updates to SB the nb_cfg number and all these updates are
> > notified to all the other HVs, which is O(n^2). Although updates
> > are batched in fewers notifications than n^2, it still generates
> > significant load on SB DB and also on ovn-controllers.
> >
> > To solve this problem and make the mechanism more useful in large
> > scale producation deployment, this patch separates the per HV
> > *private* data (write only by the owning chassis and not
> > interesting to any other HVs) from the Chassis table to a separate
> > table, so that each HV can conditionally monitor and get updates
> > only for its own record.
> >
>
> Hi Lucas,
>
> Overall this looks good to me. I do have a few small comments/questions,
> please see below.
>

Thank you very much for taking a look into this

> > Test result shows great improvement:
> > In a test environment with 1K sandbox HVs, and 10K ports created
> > on 40 lswitches and 8 lrouters, do the sync test when the system
> > is idle, with command:
> >
> > time ovn-nbctl --wait=hv sync
> >
> > Original result:
> > real4m52.926s
> > user0m0.328s
> > sys 0m0.004s
> >
> > With this patch:
> > real0m11.405s
> > user0m0.316s
> > sys 0m0.016s
> >
> > Also, regarding backwards compatibility note that the nb_cfg from the
> > Chassis table is no longer updated. If any system is relying on this
> > mechanism they should start using the nb_cfg from the Chassis_Private
> > table from now on.
>
> I see we mention it in the man pages but should we also add an item to
> NEWS to make it clear that CMSs must use this new mechanism?
>

Will do

> >
> > Co-authored-by: Han Zhou 
> > Signed-off-by: Han Zhou 
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> >  controller/chassis.c| 19 +--
> >  controller/chassis.h|  8 ++--
> >  controller/ovn-controller.c | 37 ++--
> >  lib/chassis-index.c | 25 
> >  lib/chassis-index.h |  6 ++
> >  northd/ovn-northd.c | 27 +-
> >  ovn-sb.ovsschema| 13 +++--
> >  ovn-sb.xml  | 38 +
> >  tests/ovn-controller.at | 26 +
> >  9 files changed, 178 insertions(+), 21 deletions(-)
> >
> > diff --git a/controller/chassis.c b/controller/chassis.c
> > index 522893ead..99ea6b8fc 100644
> > --- a/controller/chassis.c
> > +++ b/controller/chassis.c
> > @@ -585,14 +585,18 @@ chassis_update(const struct sbrec_chassis 
> > *chassis_rec,
> >  const struct sbrec_chassis *
> >  chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >  struct ovsdb_idl_index *sbrec_chassis_by_name,
> > +struct ovsdb_idl_index *sbrec_chassis_private_by_name,
> >  const struct ovsrec_open_vswitch_table *ovs_table,
> >  const struct sbrec_chassis_table *chassis_table,
> >  const char *chassis_id,
> >  const struct ovsrec_bridge *br_int,
> > -const struct sset *transport_zones)
> > +const struct sset *transport_zones,
> > +const struct sbrec_chassis_private **chassis_private)
> >  {
> >  struct ovs_chassis_cfg ovs_cfg;
> >
> > +*chassis_private = NULL;
> > +
> >  /* Get the chassis config from the ovs table. */
> >  ovs_chassis_cfg_init(&ovs_cfg);
> >  if (!chassis_parse_ovs_config(ovs_table, br_int, &ovs_cfg)) {
> > @@ -616,6 +620,15 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >chassis_id);
> >  }
> >
> > +const struct sbrec_chassis_private *chassis_private_rec =
> > +chassis_private_lookup_by_name(sbrec_chassis_private_by_name,
> > +   chassis_id);
> > +if (!chassis_private_rec && ovnsb_idl_txn) {
> > +chassis_private_rec = sbrec_chassis_private_insert(ovnsb_idl_txn);
> > +sbrec_chassis_private_set_name(chassis_private_rec, chassis_id);
>

Re: [ovs-dev] [PATCH ovn] [RFC] Add chassis liveness monitoring mechanism

[Lucas Alvares Gomes]

Hi Han,

Thank you very much for the feedback, much appreciated!

On Wed, Mar 11, 2020 at 7:06 AM Han Zhou  wrote:
>
> Thanks Lucas for working on this! Please see my comments below.
>
> On Thu, Feb 20, 2020 at 1:56 AM Lucas Alvares Gomes  
> wrote:
> >
> > Thanks for the review Dumitry!
> >
> > On Thu, Feb 20, 2020 at 9:19 AM Dumitru Ceara  wrote:
> > >
> > > On 2/19/20 4:37 PM, lmart...@redhat.com wrote:
> > > > From: Lucas Alvares Gomes 
> > > >
> > > > NOTE: SENDING THIS PATCH AS A RFC TO SEE WHAT OTHERS MIGHT THINK ABOUT
> > > > THE IDEA PRIOR TO WRITTING TESTS TO IT.
> > > >
> > > > CMSes integrating with OVN often uses the nb_cfg mechanism as a way to
> > > > check the health status of the ovn-controller process but, the current
> > > > implementation isn't ideal for that purpose because it floods the 
> > > > control
> > > > plane with update notifications every time the nb_cfg value is
> > > > incremented.
> > > >
> > > > This patch is merging two ideas:
> > > >
> > > > 1) Han Zhou proposed a patch [0] creating a table called Chassis_Private
> > > >where each hypervisor *only* writes and monitors its own record to
> > > >avoid this flooding problem.
> > > >
> > > > 2) Having ovn-controller to periodically log that it's alive instead of
> > > >relying on the nb_cfg mechanism.
> > > >
> > > > By using this mechanism, a CMS can more easily just read this new
> > > > Chassis_Private table and figure out the status of each Chassis
> > > > (ovn-controller) in the cluster.
> > > >
> > >
> > > Hi Lucas,
> > >
> > > Thanks a lot for working on this!
> > >
> > > > Here's some reasons why I believe this approach is better than having
> > > > to bump the  nb_cfg value:
> > > >
> > > > 1) Simple integration. Before, the CMS had to increment the nb_cfg
> > > >value in the NB_Global table and wait for it to be propagated to the
> > > >Southbound database (ovn-northd and then ovn-controllers) Chassis
> > > >table in order to know the status of the Chassis.
> > > >
> > > >Now, it can just read the Chassis_Private table and compare the
> > > >alive_at column against the current time.
>
> I think the nb_cfg mechanism doesn't have to wait. There can be one monitor 
> updating the counter periodically, and any other worker can compare the 
> desired value and each chassis's value and see which ones are stale, e.g. if 
> the gap is bigger than 3, meaning the chassis missed 3 round of heartbeat.
> But it is true that it is more costly because there are 2 directions of 
> communications. It may or may not be a concern, depending on the scale and 
> the frequency of heartbeat.
>

That's true, it can work as you suggested.

In the OVN neutron driver we relaxed the checks to account for some
miss synchronization [0] and added some code to guard against the
frequency of the checks as well [1].

[0] https://review.opendev.org/#/c/703612/
[1] https://review.opendev.org/#/c/704530/

> > >
> > > I guess the only comment I have about this approach is that it doesn't
> > > feel completely OK to store a timestamp representation as string in
> > > "alive_at". I'm afraid this might be too inflexible and force CMSes to
> > > compare their local time with the value in this column.
> > >
> > > I know we discussed offline that using a counter that is periodically
> > > incremented by ovn-controller instead of a timestamp would complicate
> > > the implementation in Openstack but maybe other people on this mailing
> > > list have ideas on how to deal with this in a more generic way.
> > >
> >
> > Agreed, let's see if people have more ideas here.
> >
> > Also, let me expand my explanation a little. The reason why I think
> > having a counter is not ideal is because services would need to track
> > this number to know what the value was before and then compare with
> > the current value to figure out whether it's being incremented or not.
> >
> > In OpenStack, we have multiple API workers spread across the
> > controllers nodes and the API request to check for the services'
> > health status will land on any of those workers. Which means that I
> > can't keep the track of the counter in-memory. Mostly likely, I would
> > need t

Re: [ovs-dev] [PATCH ovn] [RFC] Add chassis liveness monitoring mechanism

[Lucas Alvares Gomes]

Thanks for the review Dumitry!

On Thu, Feb 20, 2020 at 9:19 AM Dumitru Ceara  wrote:
>
> On 2/19/20 4:37 PM, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > NOTE: SENDING THIS PATCH AS A RFC TO SEE WHAT OTHERS MIGHT THINK ABOUT
> > THE IDEA PRIOR TO WRITTING TESTS TO IT.
> >
> > CMSes integrating with OVN often uses the nb_cfg mechanism as a way to
> > check the health status of the ovn-controller process but, the current
> > implementation isn't ideal for that purpose because it floods the control
> > plane with update notifications every time the nb_cfg value is
> > incremented.
> >
> > This patch is merging two ideas:
> >
> > 1) Han Zhou proposed a patch [0] creating a table called Chassis_Private
> >where each hypervisor *only* writes and monitors its own record to
> >avoid this flooding problem.
> >
> > 2) Having ovn-controller to periodically log that it's alive instead of
> >relying on the nb_cfg mechanism.
> >
> > By using this mechanism, a CMS can more easily just read this new
> > Chassis_Private table and figure out the status of each Chassis
> > (ovn-controller) in the cluster.
> >
>
> Hi Lucas,
>
> Thanks a lot for working on this!
>
> > Here's some reasons why I believe this approach is better than having
> > to bump the  nb_cfg value:
> >
> > 1) Simple integration. Before, the CMS had to increment the nb_cfg
> >value in the NB_Global table and wait for it to be propagated to the
> >Southbound database (ovn-northd and then ovn-controllers) Chassis
> >table in order to know the status of the Chassis.
> >
> >Now, it can just read the Chassis_Private table and compare the
> >alive_at column against the current time.
>
> I guess the only comment I have about this approach is that it doesn't
> feel completely OK to store a timestamp representation as string in
> "alive_at". I'm afraid this might be too inflexible and force CMSes to
> compare their local time with the value in this column.
>
> I know we discussed offline that using a counter that is periodically
> incremented by ovn-controller instead of a timestamp would complicate
> the implementation in Openstack but maybe other people on this mailing
> list have ideas on how to deal with this in a more generic way.
>

Agreed, let's see if people have more ideas here.

Also, let me expand my explanation a little. The reason why I think
having a counter is not ideal is because services would need to track
this number to know what the value was before and then compare with
the current value to figure out whether it's being incremented or not.

In OpenStack, we have multiple API workers spread across the
controllers nodes and the API request to check for the services'
health status will land on any of those workers. Which means that I
can't keep the track of the counter in-memory. Mostly likely, I would
need to set an IDL event on the field being incremented and store the
status of those chassis somewhere else accessible by all API workers.

The advantage with the nb_cfg mechanism compared with the incremenal
counter is that those values are already in the OVSDB, so we can
compare what's in the NB DB with the SB DB to figure whether the
services are alive or not. But, the price is that we need more code to
deal with waiting for the synchronization of the OVSDBs and, if we
move it to the Chasiss_Private table it won't be backwards compatible.

Therefore I think a timestamp would be better. It's easy to understand
by either a service or a even a person. When u look at the alive_at
field u know the last time the service signilized it was alive. For
CMSes, the service can just read the Chassis_Private table and compare
the alive_at field with the current time to figure the status of the
service (right now it's UTC, but, we can change it to include the TZ
info if people think it's easier). No writes, sync issues and also
it's backwards compatible with the nb_cfg approach.

> >
> > 2) Less costy. Just one read from the db is needed, no writing. Code
> >using the nb_cfg mechanism had to implement a few safe-guard code to
> >make less error prone. See [1] and [2] for example.
> >
> > 3) Backwards compatibility. The patch [0] was moving the nb_cfg value
> >to new table so, systems relying on it would need to update their
> >code upon updating OVN.
>
> Nice!
>
> One more minor comment on the code, inline.
>
> Thanks,
> Dumitru
>
> >
> > To enable this new mechanism set an option called
> > chassis_liveness_interval=N to the "options" column in the NB_Glob

Re: [ovs-dev] [PATCH ovn] Build OVN using external OVS directory

[Lucas Alvares Gomes]

ON@LT_REVISION@
> +#define OVN_LIB_AGE @LT_AGE@
> +
> +#endif /* ovn/version.h */
> diff --git a/lib/ovsdb_automake.mk b/lib/ovsdb_automake.mk
> index f6fd5e8b3..73f236d1e 100644
> --- a/lib/ovsdb_automake.mk
> +++ b/lib/ovsdb_automake.mk
> @@ -1,10 +1,10 @@
>  # ovsdb-idlc
> -noinst_SCRIPTS += ovs/ovsdb/ovsdb-idlc
> -EXTRA_DIST += ovs/ovsdb/ovsdb-idlc.in
> -MAN_ROOTS += ovs/ovsdb/ovsdb-idlc.1
> -CLEANFILES += ovs/ovsdb/ovsdb-idlc
> +noinst_SCRIPTS += ${OVSDIR}/ovsdb/ovsdb-idlc
> +EXTRA_DIST += ${OVSDIR}/ovsdb/ovsdb-idlc.in
> +MAN_ROOTS += ${OVSDIR}/ovsdb/ovsdb-idlc.1
> +CLEANFILES += ${OVSDIR}/ovsdb/ovsdb-idlc
>  SUFFIXES += .ovsidl .ovsschema
> -OVSDB_IDLC = $(run_python) $(srcdir)/ovs/ovsdb/ovsdb-idlc.in
> +OVSDB_IDLC = $(run_python) ${OVSDIR}/ovsdb/ovsdb-idlc.in
>  .ovsidl.c:
> $(AM_V_GEN)$(OVSDB_IDLC) c-idl-source $< > $@.tmp && mv $@.tmp $@
>  .ovsidl.h:
> @@ -21,5 +21,5 @@ CLEANFILES += $(OVSIDL_BUILT)
>  # However, current versions of Automake seem to output all variable
>  # assignments before any targets, so it doesn't seem to be a problem,
>  # at least for now.
> -$(OVSIDL_BUILT): ovs/ovsdb/ovsdb-idlc.in
> +$(OVSIDL_BUILT): ${OVSDIR}/ovsdb/ovsdb-idlc.in
>
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 5411772a4..255964fb7 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -51,7 +51,7 @@ SYSTEM_KMOD_TESTSUITE = 
> $(srcdir)/tests/system-kmod-testsuite
>  SYSTEM_USERSPACE_TESTSUITE = $(srcdir)/tests/system-userspace-testsuite
>  DISTCLEANFILES += tests/atconfig tests/atlocal
>
> -AUTOTEST_PATH = 
> ovs/utilities:ovs/vswitchd:ovs/ovsdb:ovs/vtep:tests:$(PTHREAD_WIN32_DIR_DLL):$(SSL_DIR):controller-vtep:northd:utilities:controller
> +AUTOTEST_PATH = 
> $(ovs_builddir)/utilities:$(ovs_builddir)/vswitchd:$(ovs_builddir)/ovsdb:$(ovs_builddir)/vtep:tests:$(PTHREAD_WIN32_DIR_DLL):$(SSL_DIR):controller-vtep:northd:utilities:controller
>
>  check-local:
> set $(SHELL) '$(TESTSUITE)' -C tests AUTOTEST_PATH=$(AUTOTEST_PATH); \
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [[ovn]] OVN split: Fix modules_install

[Lucas Alvares Gomes]

Hi,

On Mon, Jul 29, 2019 at 10:25 AM Numan Siddique  wrote:
>
> On Mon, Jul 29, 2019 at 2:30 PM  wrote:
>
> > From: Lucas Alvares Gomes 
> >
> > The Makefile for modules_install should point to the ovs subrepository
> > to be able to compile the ovs kernel modules (otherwise the compilation
> > fails).
> >
> > Signed-off-by: Lucas Alvares Gomes 
> >
>
> I think it is better to delete this target rather than fixing it.
> Ideally OVS repo should be used to build the kernel module .
>

Got it, thanks for the advice.

I was updating networking-ovn (the OpenStack driver for OVN) to use
the new repository and found that problem. I will remove the target as
you suggested and use the OVS repository to compile the kernel
modules.

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] [ovn-northd] Don't emit ICMP need to frag on LRP with no IPv4 address

[Lucas Alvares Gomes]

Hi,

Thanks for the quick patch Daniel. Without this patch I was getting a
segmentation fault in ovn-northd:
http://paste.openstack.org/show/754850/. Daniel debugged it and found
the problem, I applied this patch and now it works :D

On Thu, Jul 25, 2019 at 4:38 PM Daniel Alvarez  wrote:
>
> Prior to this patch, when a LRP has only IPv6 addresses, ovn-northd
> will crash (SIGSEV) because the current code injects a flow to
> emit the ICMP need-to-frag from its IPv4 address which does not
> exist.
>
> This patch is adding a check to skip the flow installation in case
> the port does not have any IPv4 address.
>
> Signed-off-by: Daniel Alvarez 
> ---
>  ovn/northd/ovn-northd.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index eb6c47cad..3542ba72f 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -7705,7 +7705,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap 
> *ports,
>  for (size_t i = 0; i < od->nbr->n_ports; i++) {
>  struct ovn_port *rp = ovn_port_find(ports,
>  od->nbr->ports[i]->name);
> -if (!rp || rp == od->l3dgw_port) {
> +if (!rp || rp == od->l3dgw_port ||
> +!rp->lrp_networks.ipv4_addrs) {
>  continue;
>  }
>  ds_clear(&match);
> --
> 2.21.0 (Apple Git-120)
>
> ___________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovsdb-server: drop all connections on read/write status change

[Lucas Alvares Gomes]

Glad to see this being fixed, thanks Daniel.

On Tue, Jul 9, 2019 at 11:22 AM Daniel Alvarez  wrote:
>
> Prior to this patch, only db change aware connections were dropped
> on a read/write status change. However, current schema in OVN does
> not allow clients to monitor whether a particular DB changes this
> status. In order to accomplish this, we'd need to change the schema
> and adapting ovsdb-server and existing clients.
>
> Before tackling that, this patch is changing ovsdb-server to drop
> *all* the existing connections upon a read/write status change. This
> will force clients to reconnect and honor the change.
>
> Reported-at: 
> https://mail.openvswitch.org/pipermail/ovs-discuss/2019-July/048981.html
> Signed-off-by: Daniel Alvarez 
> ---
>  ovsdb/jsonrpc-server.c |  2 +-
>  tests/ovn.at   | 21 +
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
> index 4dda63a9d..ddbbc2e94 100644
> --- a/ovsdb/jsonrpc-server.c
> +++ b/ovsdb/jsonrpc-server.c
> @@ -365,7 +365,7 @@ ovsdb_jsonrpc_server_set_read_only(struct 
> ovsdb_jsonrpc_server *svr,
>  {
>  if (svr->read_only != read_only) {
>  svr->read_only = read_only;
> -ovsdb_jsonrpc_server_reconnect(svr, false,
> +ovsdb_jsonrpc_server_reconnect(svr, true,
> xstrdup(read_only
> ? "making server read-only"
> : "making server 
> read/write"));
> diff --git a/tests/ovn.at b/tests/ovn.at
> index daace1128..4da7059b3 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -14313,3 +14313,24 @@ OVN_CHECK_PACKETS([hv2/vif22-tx.pcap], 
> [vif22.expected])
>  OVN_CLEANUP([hv1],[hv2])
>
>  AT_CLEANUP
> +
> +# Run ovn-nbctl in daemon mode, change to a backup database and verify that
> +# an insert operation is not allowed.
> +AT_SETUP([ovn -- can't write to a backup database server instance])
> +ovn_start
> +on_exit 'kill $(cat ovn-nbctl.pid)'
> +export OVN_NB_DAEMON=$(ovn-nbctl --pidfile --detach)
> +
> +AT_CHECK([ovn-nbctl ls-add sw0])
> +as ovn-nb
> +AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/sync-status | grep active 
> | wc -l], [0], [1
> +])
> +ovs-appctl -t ovsdb-server ovsdb-server/set-active-ovsdb-server 
> tcp:192.0.2.2:6641
> +ovs-appctl -t ovsdb-server ovsdb-server/connect-active-ovsdb-server
> +AT_CHECK([ovs-appctl -t ovsdb-server ovsdb-server/sync-status | grep -c 
> backup], [0], [1
> +])
> +AT_CHECK([ovn-nbctl ls-add sw1], [1], [ignore],
> +[ovn-nbctl: transaction error: {"details":"insert operation not allowed when 
> database server is in read only mode","error":"not allowed"}
> +])
> +
> +AT_CLEANUP
> --
> 2.21.0 (Apple Git-120)
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] OVN resource agent - make promotion synchronous

[Lucas Alvares Gomes]

Thanks for the patch Michele.

As @Daniel pointed out, this have been tested and works.

On Tue, Jul 9, 2019 at 8:18 AM Michele Baldessari  wrote:
>
> Currently inside the ovsdb_server_promote() function we call 'promote_ovnnb'
> and 'promote_ovnsb' and then just record the new master state in the
> CIB.
>
> This creates a race because those two promote commands are asynchronous
> so when we exit the ovsdb_server_promote() function the underlying DBs
> are not guaranteed to be in master state. That means that clients might
> connect to an instance that is in read-only mode.
>
> We add a simple sleep loop where we wait for the underlying DB state to
> confirm the master state. We do not need to add a timeout loop because
> in case of an issue the resource timeout set within pacemaker will kick
> in and the resource agent script will be killed by pacemaker.
>
> Tested this within an openstack environment using ovn with roughly ~20
> reboots and was unable to trigger the issue (before the patch we would
> trigger the issue after a couple of reboots tops).
>
> Signed-off-by: Michele Baldessari 
> ---
>  ovn/utilities/ovndb-servers.ocf | 12 +++-
>  1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/ovn/utilities/ovndb-servers.ocf b/ovn/utilities/ovndb-servers.ocf
> index 10313304cb7c..cd47426689ef 100755
> --- a/ovn/utilities/ovndb-servers.ocf
> +++ b/ovn/utilities/ovndb-servers.ocf
> @@ -516,6 +516,8 @@ ovsdb_server_stop() {
>  }
>
>  ovsdb_server_promote() {
> +local state
> +
>  ovsdb_server_check_status ignore_northd
>  rc=$?
>  case $rc in
> @@ -540,7 +542,15 @@ ovsdb_server_promote() {
>  ${OVN_CTL} --ovn-manage-ovsdb=no start_northd
>  fi
>
> -ocf_log debug "ovndb_servers: Promoting $host_name as the master"
> +ocf_log debug "ovndb_servers: Waiting for promotion $host_name as master 
> to complete"
> +ovsdb_server_check_status
> +state=$?
> +while [ "$state" != "$OCF_RUNNING_MASTER" ]; do
> +  sleep 1
> +  ovsdb_server_check_status
> +  state=$?
> +done
> +ocf_log debug "ovndb_servers: Promotion of $host_name as the master 
> completed"
>  # Record ourselves so that the agent has a better chance of doing
>  # the right thing at startup
>  ${CRM_ATTR_REPL_INFO} -v "$host_name"
> --
> 2.21.0
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] OVN: Enhance ovndb-servers.ocf to handle inactive_probe_interval updates

[Lucas Alvares Gomes]

On Fri, 28 Jun 2019, 19:17 Numan Siddique,  wrote:

> On Fri, Jun 28, 2019 at 11:46 PM Numan Siddique 
> wrote:
>
> >
> >
> > On Wed, Jun 26, 2019 at 2:42 PM  wrote:
> >
> >> From: Lucas Alvares Gomes 
> >>
> >> This patch is enhacing the ovndb-servers.ocf script to handle updates to
> >> the inactive_probe_interval via pacemaker. For example, one could run:
> >>
> >> $ sudo crm_resource --resource ovndb_servers --set-parameter
> >> inactive_probe_interval --parameter-value 
> >>
> >> To set a new inactive probe interval in OVSDB. The patch also handles
> >> the case were multiple connection exists.
> >>
> >> Signed-off-by: Lucas Alvares Gomes 
> >>
> >
> > Acked-by: Numan Siddique 
> >
> >
> Hi Lucas, there are few checkpatch warnings. You may want to fix those
> errors
> and submit v2.
>

Hi Numans,

Thanks for looking into this, I've pushed a V2 already at
https://patchwork.ozlabs.org/patch/1122681/


>
>
> >
> >
> >> ---
> >>  ovn/utilities/ovndb-servers.ocf | 26 ++
> >>  1 file changed, 22 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/ovn/utilities/ovndb-servers.ocf
> >> b/ovn/utilities/ovndb-servers.ocf
> >> index 10313304c..62ac53b7c 100755
> >> --- a/ovn/utilities/ovndb-servers.ocf
> >> +++ b/ovn/utilities/ovndb-servers.ocf
> >> @@ -240,20 +240,38 @@ ovsdb_server_notify() {
> >>  else
> >> LISTON_ON_IP=${MASTER_IP}
> >>  fi
> >> -conn=`ovn-nbctl get NB_global . connections`
> >> -if [ "$conn" == "[]" ]
> >> +conn=`ovn-nbctl get NB_global . connections | awk -F'[][]'
> >> '{print $2}'`
> >> +if [ -z "$conn" ]
> >>  then
> >>  ovn-nbctl -- --id=@conn_uuid create Connection \
> >>  target="p${NB_MASTER_PROTO}\:${NB_MASTER_PORT}\:${LISTON_ON_IP}" \
> >>  inactivity_probe=$INACTIVE_PROBE -- set NB_Global .
> >> connections=@conn_uuid
> >> +else
> >> +for c in ${conn/, // }
> >> +do
> >> +iprob=`ovn-nbctl get Connection $c inactivity_probe`
> >> +if [ $iprob != $INACTIVE_PROBE ]
> >> +then
> >> +ovn-nbctl set Connection $c
> >> inactivity_probe=$INACTIVE_PROBE
> >> +fi
> >> +done
> >>  fi
> >>
> >> -conn=`ovn-sbctl get SB_global . connections`
> >> -if [ "$conn" == "[]" ]
> >> +conn=`ovn-sbctl get SB_global . connections | awk -F'[][]'
> >> '{print $2}'`
> >> +if [ -z "$conn" ]
> >>  then
> >>  ovn-sbctl -- --id=@conn_uuid create Connection \
> >>  target="p${SB_MASTER_PROTO}\:${SB_MASTER_PORT}\:${LISTON_ON_IP}" \
> >>  inactivity_probe=$INACTIVE_PROBE -- set SB_Global .
> >> connections=@conn_uuid
> >> +else
> >> +for c in ${conn/, // }
> >> +do
> >> +iprob=`ovn-sbctl get Connection $c inactivity_probe`
> >> +if [ $iprob != $INACTIVE_PROBE ]
> >> +then
> >> +ovn-sbctl set Connection $c
> >> inactivity_probe=$INACTIVE_PROBE
> >> +fi
> >> +done
> >>  fi
> >>
> >>  else
> >> --
> >> 2.22.0
> >>
> >> ___
> >> dev mailing list
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> >>
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] Request to release a new version of the ovs python library

[Lucas Alvares Gomes]

Hi,

On Sat, Jun 22, 2019 at 5:39 AM Ben Pfaff  wrote:
>
> On Fri, Jun 21, 2019 at 09:36:36AM +0100, Lucas Alvares Gomes wrote:
> > Hi,
> >
> > Not sure if this is the right place to ask for it but, can we get a
> > new release of the ovs python library please [0] ? The latest release
> > was in Oct, 9, 2018.
> >
> > I keep seeing the following error [1] in the upstream OpenStack gate:
> >
> > Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
> > neutron-server[32296]:   File
> > "/usr/local/lib/python3.6/dist-packages/ovs/db/idl.py", line 858, in
> > __getattr__
> > Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
> > neutron-server[32296]: del dmap[key]
> > Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
> > neutron-server[32296]: KeyError: 'host-192-168-233-188'
>
> I don't know who is in charge of that.  Do you?

Thanks Ben. Yes, now I know. Terry Wilson and Russel Bryant are the maintainers.

Terry released a 2.11.0 version of the python ovs library now (thanks Terry!)

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Request to release a new version of the ovs python library

[Lucas Alvares Gomes]

Hi,

Not sure if this is the right place to ask for it but, can we get a
new release of the ovs python library please [0] ? The latest release
was in Oct, 9, 2018.

I keep seeing the following error [1] in the upstream OpenStack gate:

Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
neutron-server[32296]:   File
"/usr/local/lib/python3.6/dist-packages/ovs/db/idl.py", line 858, in
__getattr__
Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
neutron-server[32296]: del dmap[key]
Jun 20 20:22:00.178737 ubuntu-bionic-vexxhost-sjc1-0008154837
neutron-server[32296]: KeyError: 'host-192-168-233-188'

This have been fixed for awihle in OVS already [2].

[0] https://pypi.org/project/ovs/#history
[1]
http://logs.openstack.org/07/633707/8/check/networking-ovn-tempest-dsvm-ovs-master/cb9181b/controller/logs/screen-q-svc.txt.gz?level=ERROR#_Jun_20_20_22_00_178737
[2] 
https://github.com/openvswitch/ovs/commit/f192ba279e9c7d378f348017281b14f1585e0eaa

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v6] OVN: Add support for Transport Zones

[Lucas Alvares Gomes]

On Mon, Apr 22, 2019 at 9:38 PM Ben Pfaff  wrote:
>
> On Thu, Apr 18, 2019 at 02:39:09PM +0100, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > TZs) is way to enable users of OVN to separate Chassis into different
> > logical groups that will only form tunnels between members of the same
> > groups. Each Chassis can belong to one or more Transport Zones. If
> > not set, the Chassis will be considered part of a default group.
>
> Applied to master, thanks.
>

Thank you very much!

> It might be nice to add to the documentation the fact about the default
> group, above.  It doesn't currently say that and it could be valuable
> tor readers.

Absolutely, I will add that to the documentation then.

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] OVN: Add support for Transport Zones

[Lucas Alvares Gomes]

On Thu, Apr 18, 2019 at 11:34 AM Lucas Alvares Gomes
 wrote:
>
> Hi,
>
> On Wed, Apr 17, 2019 at 6:04 PM Ben Pfaff  wrote:
> >
> > On Wed, Apr 17, 2019 at 04:55:24PM +0100, lmart...@redhat.com wrote:
> > > From: Lucas Alvares Gomes 
> > >
> > > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > > TZs) is way to enable users of OVN to separate Chassis into different
> > > logical groups that will only form tunnels between members of the same
> > > groups. Each Chassis can belong to one or more Transport Zones. If
> > > not set, the Chassis will be considered part of a default group.
> >
> > [...]
> >
> > > v4 -> v5
> > >  * Bumped the version of the ovn-sb.ovsschema to 2.2.1.
> >
> > The version should become 2.3.0, see ovsdb.7:
> >
> > An OVSDB schema also has a version of the form ``x.y.z``
> > e.g. ``1.2.3``.  Schemas managed within the Open vSwitch project
> > manage version numbering in the following way (but OVSDB does not
> > mandate this approach).  Whenever we change the database schema in a
> > non-backward compatible way (e.g. when we delete a column or a
> > table), we increment  and set  and  to 0.  When we change
> > the database schema in a backward compatible way (e.g. when we add a
> > new column), we increment  and set  to 0.  When we change the
> > database schema cosmetically (e.g. we reindent its syntax), we
> > increment .  The ``ovsdb-tool`` commands ``schema-version`` and
> > ``db-version`` extract the schema version from a schema or database
> > file, respectively.
> >
>
> Ops my bad, I will bump it to 2.3.0. Thanks for the pointer to the
> documentation, it's very helpful.
>
> > But the main issue I see here is that I get a consistent test failure in
> > the new test:
> >
> > # -*- compilation -*-
> > 2693. ovn.at:13656: testing ovn -- test transport zones ...
> > creating ovn-sb database
> > creating ovn-nb database
> > starting ovn-northd
> > starting backup ovn-northd
> > adding simulator 'main'
> > adding simulator 'hv1'
> > adding simulator 'hv2'
> > adding simulator 'hv3'
> > adding simulator 'hv4'
> > adding simulator 'hv5'
> > ../../tests/ovn.at:13669: ovs-vsctl --bare --columns=name find interface 
> > type="geneve" | awk NF | sort
> > --- -   2019-04-17 10:02:31.402675315 -0700
> > +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/2693/stdout   
> > 2019-04-17 10:02:31.397247995 -0700
> > @@ -1,5 +1,4 @@
> >  ovn-hv2-0
> >  ovn-hv3-0
> >  ovn-hv4-0
> > -ovn-hv5-0
> >
> > 2693. ovn.at:13656: 2693. ovn -- test transport zones (ovn.at:13656): 
> > FAILED (ovn.at:13669)
>
> Thanks for trying it out.
>
> I'm currently investigating the problem but I could not reproduce it
> locally. I did run this test multiple times on different distros
> (CentOS 7 [0], Fedora 29 [1] and Ubuntu Bionic [2]) and it's passing
> for me. I'm doing the following:
>
> $ make check TESTSUITEFLAGS="--list --keywords=ovn" | grep "transport zones"
>
> Then I use the test number from the previous command and run the test as:
>
> $ make check TESTSUITEFLAGS="--verbose 2693"
>
> I will continue the investigation to see if can find the problem.
>
> Also, mind letting me know how you're executing the tests so I can try
> it as well ?
>

Update: I was able to reproduce the problem by stressing all CPUs in
the background and running that test.

I will work on a fix for the problem and upload a new version of the
patch with the version and test fixed.

Thanks Ben for finding the problem in the first place.

[0] https://pastebin.com/zPrPVGd7

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] OVN: Add support for Transport Zones

[Lucas Alvares Gomes]

Hi,

On Wed, Apr 17, 2019 at 6:04 PM Ben Pfaff  wrote:
>
> On Wed, Apr 17, 2019 at 04:55:24PM +0100, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > TZs) is way to enable users of OVN to separate Chassis into different
> > logical groups that will only form tunnels between members of the same
> > groups. Each Chassis can belong to one or more Transport Zones. If
> > not set, the Chassis will be considered part of a default group.
>
> [...]
>
> > v4 -> v5
> >  * Bumped the version of the ovn-sb.ovsschema to 2.2.1.
>
> The version should become 2.3.0, see ovsdb.7:
>
> An OVSDB schema also has a version of the form ``x.y.z``
> e.g. ``1.2.3``.  Schemas managed within the Open vSwitch project
> manage version numbering in the following way (but OVSDB does not
> mandate this approach).  Whenever we change the database schema in a
> non-backward compatible way (e.g. when we delete a column or a
> table), we increment  and set  and  to 0.  When we change
> the database schema in a backward compatible way (e.g. when we add a
> new column), we increment  and set  to 0.  When we change the
> database schema cosmetically (e.g. we reindent its syntax), we
> increment .  The ``ovsdb-tool`` commands ``schema-version`` and
> ``db-version`` extract the schema version from a schema or database
> file, respectively.
>

Ops my bad, I will bump it to 2.3.0. Thanks for the pointer to the
documentation, it's very helpful.

> But the main issue I see here is that I get a consistent test failure in
> the new test:
>
> # -*- compilation -*-
> 2693. ovn.at:13656: testing ovn -- test transport zones ...
> creating ovn-sb database
> creating ovn-nb database
> starting ovn-northd
> starting backup ovn-northd
> adding simulator 'main'
> adding simulator 'hv1'
> adding simulator 'hv2'
> adding simulator 'hv3'
> adding simulator 'hv4'
> adding simulator 'hv5'
> ../../tests/ovn.at:13669: ovs-vsctl --bare --columns=name find interface 
> type="geneve" | awk NF | sort
> --- -   2019-04-17 10:02:31.402675315 -0700
> +++ /home/blp/nicira/ovs/_build/tests/testsuite.dir/at-groups/2693/stdout 
>   2019-04-17 10:02:31.397247995 -0700
> @@ -1,5 +1,4 @@
>  ovn-hv2-0
>  ovn-hv3-0
>  ovn-hv4-0
> -ovn-hv5-0
>
> 2693. ovn.at:13656: 2693. ovn -- test transport zones (ovn.at:13656): FAILED 
> (ovn.at:13669)

Thanks for trying it out.

I'm currently investigating the problem but I could not reproduce it
locally. I did run this test multiple times on different distros
(CentOS 7 [0], Fedora 29 [1] and Ubuntu Bionic [2]) and it's passing
for me. I'm doing the following:

$ make check TESTSUITEFLAGS="--list --keywords=ovn" | grep "transport zones"

Then I use the test number from the previous command and run the test as:

$ make check TESTSUITEFLAGS="--verbose 2693"

I will continue the investigation to see if can find the problem.

Also, mind letting me know how you're executing the tests so I can try
it as well ?

[0] https://pastebin.com/3iSyfivL
[1] https://pastebin.com/hjrpB9eD
[2] https://pastebin.com/t3QG383x

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4] OVN: Add support for Transport Zones

[Lucas Alvares Gomes]

On Wed, Apr 17, 2019 at 4:46 PM Numan Siddique  wrote:
>
> On Wed, Apr 17, 2019 at 8:21 PM  wrote:
>
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > TZs) is way to enable users of OVN to separate Chassis into different
> > logical groups that will only form tunnels between members of the same
> > groups. Each Chassis can belong to one or more Transport Zones. If
> > not set, the Chassis will be considered part of a default group.
> >
> > Configuring Transport Zones is done by creating a key called
> > "ovn-transport-zones" in the external_ids column of the Open_vSwitch
> > table from the local OVS instance. The value is a string with the name
> > of the Transport Zone that this instance is part of. Multiple TZs can
> > be specified with a comma-separated list. For example:
> >
> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1
> >
> > or
> >
> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1,tz2,tz3
> >
> > This configuration is also exposed in the Chassis table of the OVN
> > Southbound Database in a new column called "transport_zones".
> >
> > The use for Transport Zones includes but are not limited to:
> >
> > * Edge computing: As a way to preventing edge sites from trying to create
> >   tunnels with every node on every other edge site while still allowing
> >   these sites to create tunnels with the central node.
> >
> > * Extra security layer: Where users wants to create "trust zones"
> >   and prevent computes in a more secure zone to communicate with a less
> >   secure zone.
> >
> > This patch is also backward compatible so the upgrade guide for OVN [0]
> > is still valid and the ovn-controller service can be upgraded before the
> > OVSDBs.
> >
> > [0] http://docs.openvswitch.org/en/latest/intro/install/ovn-upgrades/
> >
> > Reported-by: Daniel Alvarez Sanchez 
> > Reported-at:
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> > v3 -> v4
> >  * Stopped using the "external_ids" column of the Chassis table and
> >instead added a new column called "transport_zones" to hold the set of
> >transport zones that the Chassis is part of.
> >
> > v2 -> v3
> >  * Enhanced the test to include two more Chassis and assert the case
> >where Chassis with no TZs set will have tunnels formed between them.
> >  * Rebased the patch on top of the latest master branch.
> >
> > v1 -> v2
> >
> >  * Rename the function check_chassis_tzones to chassis_tzones_overlap.
> >  * Fix a memory leak in chassis_tzones_overlap.
> >  * Pass the transport_zones to encaps_run() as a "const char *"
> >instead of "struct sbrec_chassis". With this we can also avoid not
> >running the function in case the Chassis entry is not yet created.
> >
> >  NEWS|   3 +
> >  ovn/controller/chassis.c|  26 -
> >  ovn/controller/chassis.h|   4 +-
> >  ovn/controller/encaps.c |  35 ++-
> >  ovn/controller/encaps.h |   4 +-
> >  ovn/controller/ovn-controller.8.xml |   9 ++
> >  ovn/controller/ovn-controller.c |  19 +++-
> >  ovn/ovn-sb.ovsschema|   7 +-
> >  ovn/ovn-sb.xml  |   8 ++
> >  tests/ovn.at| 151 
> >  10 files changed, 257 insertions(+), 9 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index c7440b476..e09f59d64 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -33,6 +33,9 @@ Post-v2.11.0
> >   * Added Policy-based routing(PBR) support to create
> > permit/deny/reroute
> > policies on the logical router. New table(Logical_Router_Policy)
> > added in
> > OVN-NB schema. New "ovn-nbctl" commands to add/delete/list PBR
> > policies.
> > + * Support for Transport Zones, a way to separate chassis into
> > +   logical groups which results in tunnels only been formed between
> > +   members of the same transport zone(s).
> > - New QoS type "linux-netem" on Linux.
> > - Added support for TLS Server Name Indication (SNI).
> >
> > diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
> > index 58d5d49d5..0f537f1f7 100644
> > --- a/ovn/controller/chassis.c
> >

Re: [ovs-dev] [PATCH v3] OVN: Add support for Transport Zones

[Lucas Alvares Gomes]

On Sat, Apr 13, 2019 at 12:08 AM Ben Pfaff  wrote:
>
> On Thu, Mar 28, 2019 at 10:37:00AM +, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > TZs) is way to enable users of OVN to separate Chassis into different
> > logical groups that will form tunnels only between members of the same
> > group(s).
>
> Thanks for working on this.
>
> This adds a new property to a Chassis and implements that property
> through the Chassis record's external-ids.  This strikes me as a
> layering violation, because external-ids is usually reserved for higher
> layers in the system.  Its documentation says:
>
>   external_ids: map of string-string pairs
>  Key-value  pairs for use by the software that manages the
>  OVN  Southbound  database   rather   than   by   ovn-con‐
>  troller/ovn-controller-vtep.  In  particular,  ovn-northd
>  can use key-value pairs in this column to relate entities
>  in the southbound database to higher-level entities (such
>  as entities in the OVN Northbound  database).  Individual
>  key-value  pairs in this column may be documented in some
>  cases to aid in understanding  and  troubleshooting,  but
>  the  reader should not mistake such documentation as com‐
>  prehensive.
>
> It would be a more natural fit to add a transport_zones column to the
> Chassis table.  (Probably a "set of strings" instead of a
> comma-delimited string.)
>

Good point, yeah by the description of that column it sounds like my
patch is misusing it indeed. I will modify the patch with your
suggestion.

I will also make sure we are good with upgrades because the upgrade
guide [0] recommends upgrading the ovn-controller service before the
OVSDBs so, I should handle the case where this new column does not yet
exist in the database as well.

[0] http://docs.openvswitch.org/en/latest/intro/install/ovn-upgrades/

> The new property should be documented in ovn-sb.xml.
>

Will do!

Thanks,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] OVN: Add support for Transport Zones

[Lucas Alvares Gomes Martins]

On Thu, Mar 28, 2019 at 9:19 AM Numan Siddique  wrote:
>
>
> Thanks Lucas for the patch. The patch looks good to me.
> Just  a few comments inline.
>
> Thanks
> Numan
>
> On Tue, Mar 26, 2019 at 11:55 PM Mark Michelson  wrote:
>>
>> Thanks Lucas, looks good to me.
>>
>> Acked-by: Mark Michelson 
>>
>> On 3/25/19 2:24 PM, lmart...@redhat.com wrote:
>> > From: Lucas Alvares Gomes 
>> >
>> > This patch is adding support for Transport Zones. Transport zones (a.k.a
>> > TZs) is way to enable users of OVN to separate Chassis into different
>> > logical groups that will form tunnels only between members of the same
>> > group(s).
>> >
>> > Each Chassis can belong to one or more Transport Zones. If not set,
>> > the Chassis will be considered part of a default group; this feature
>> > is backward compatible and did not require any changes to the database
>> > schemas.
>> >
>> > Configuring Transport Zones is done by creating a key called
>> > "ovn-transport-zones" in the external_ids of the Open_vSwitch table of the
>> > local OVS instance. The value is a string with the name of the Transport
>> > Zone that this instance is part of. Multiple TZs may be specified with
>> > a comma-separated list. For example:
>> >
>> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1
>> >
>> > or
>> >
>> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1,tz2,tz3
>> >
>> > This configuration will be also exposed in the Chassis table of the OVN
>> > Southbound Database so that external systems can see what TZ(s) each
>> > Chassis are part of and make decisions based those values.
>> >
>> > The use for Transport Zones includes but are not limited to:
>> >
>> > * Edge computing: As a way to preventing edge sites from trying to create
>> >tunnels with every node on every other edge site while still allowing
>> >these sites to create tunnels with the central node.
>> >
>> > * Extra security layer: Where users wants to create "trust zones"
>> >and prevent computes in a more secure zone to communicate with a less
>> >secure zone.
>> >
>> > Reported-by: Daniel Alvarez Sanchez 
>> > Reported-at: 
>> > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html
>> > Signed-off-by: Lucas Alvares Gomes 
>> > ---
>> >
>> > v1 -> v2
>> >   * Rename the function check_chassis_tzones to chassis_tzones_overlap.
>> >   * Fix a memory leak in chassis_tzones_overlap.
>> >   * Pass the transport_zones to encaps_run() as a "const char *"
>> > instead of "struct sbrec_chassis". With this we can also avoid not
>> > running the function in case the Chassis entry is not yet created.
>> >
>> >   NEWS|  3 +
>> >   ovn/controller/chassis.c|  8 ++-
>> >   ovn/controller/encaps.c | 58 +-
>> >   ovn/controller/encaps.h |  3 +-
>> >   ovn/controller/ovn-controller.8.xml |  9 +++
>> >   ovn/controller/ovn-controller.c | 14 -
>> >   tests/ovn.at| 93 +
>> >   7 files changed, 183 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/NEWS b/NEWS
>> > index 1e4744dbd..4adf49f57 100644
>> > --- a/NEWS
>> > +++ b/NEWS
>> > @@ -24,6 +24,9 @@ Post-v2.11.0
>> >  protocol extension.
>> >  - OVN:
>> >* Select IPAM mac_prefix in a random manner if not provided by the 
>> > user
>> > + * Support for Transport Zones, a way to separate chassis into
>> > +   logical groups which results in tunnels only been formed between
>> > +   members of the same transport zone(s).
>> >  - New QoS type "linux-netem" on Linux.
>> >
>> >   v2.11.0 - 19 Feb 2019
>> > diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
>> > index 3ea908d18..34c260410 100644
>> > --- a/ovn/controller/chassis.c
>> > +++ b/ovn/controller/chassis.c
>> > @@ -139,6 +139,8 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
>> >   const char *datapath_type =
>> >   br_int && br_int->datapath_type ? br_int->datapath_type : "";
>> >   const char *cms_options = get_cms_op

Re: [ovs-dev] [PATCH] OVN: Add support for Transport Zones

[Lucas Alvares Gomes Martins]

Hi,

Thanks a lot Mark for the quick review.

On Mon, Mar 25, 2019 at 3:40 PM Mark Michelson  wrote:
>
> Hi Lucas,
>
> Thanks for the patch. It's mostly good but I have a few issues with it.
> See below.
>
> On 3/25/19 10:36 AM, lmart...@redhat.com wrote:
> > From: Lucas Alvares Gomes 
> >
> > This patch is adding support for Transport Zones. Transport zones (a.k.a
> > TZs) is way to enable users of OVN to separate Chassis into different
> > logical groups that will form tunnels only between members of the same
> > group(s).
> >
> > Each Chassis can belong to one or more Transport Zones. If not set,
> > the Chassis will be considered part of a default group; this feature
> > is backward compatible and did not require any changes to the database
> > schemas.
> >
> > Configuring Transport Zones is done by creating a key called
> > "ovn-transport-zones" in the external_ids of the Open_vSwitch table of the
> > local OVS instance. The value is a string with the name of the Transport
> > Zone that this instance is part of. Multiple TZs may be specified with
> > a comma-separated list. For example:
> >
> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1
> >
> > or
> >
> > $ sudo ovs-vsctl set open . external-ids:ovn-transport-zones=tz1,tz2,tz3
> >
> > This configuration will be also exposed in the Chassis table of the OVN
> > Southbound Database so that external systems can see what TZ(s) each
> > Chassis are part of and make decisions based those values.
> >
> > The use for Transport Zones includes but are not limited to:
> >
> > * Edge computing: As a way to preventing edge sites from trying to create
> >tunnels with every node on every other edge site while still allowing
> >these sites to create tunnels with the central node.
> >
> > * Extra security layer: Where users wants to create "trust zones"
> >    and prevent computes in a more secure zone to communicate with a less
> >secure zone.
> >
> > Reported-by: Daniel Alvarez Sanchez 
> > Reported-at: 
> > https://mail.openvswitch.org/pipermail/ovs-discuss/2019-February/048255.html
> > Signed-off-by: Lucas Alvares Gomes 
> > ---
> >   NEWS|  3 +
> >   ovn/controller/chassis.c|  8 ++-
> >   ovn/controller/encaps.c | 54 +++--
> >   ovn/controller/encaps.h |  3 +-
> >   ovn/controller/ovn-controller.8.xml |  9 +++
> >   ovn/controller/ovn-controller.c |  2 +-
> >   tests/ovn.at| 93 +
> >   7 files changed, 164 insertions(+), 8 deletions(-)
> >
> > diff --git a/NEWS b/NEWS
> > index 1e4744dbd..4adf49f57 100644
> > --- a/NEWS
> > +++ b/NEWS
> > @@ -24,6 +24,9 @@ Post-v2.11.0
> >  protocol extension.
> >  - OVN:
> >* Select IPAM mac_prefix in a random manner if not provided by the 
> > user
> > + * Support for Transport Zones, a way to separate chassis into
> > +   logical groups which results in tunnels only been formed between
> > +   members of the same transport zone(s).
> >  - New QoS type "linux-netem" on Linux.
> >
> >   v2.11.0 - 19 Feb 2019
> > diff --git a/ovn/controller/chassis.c b/ovn/controller/chassis.c
> > index 3ea908d18..34c260410 100644
> > --- a/ovn/controller/chassis.c
> > +++ b/ovn/controller/chassis.c
> > @@ -139,6 +139,8 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >   const char *datapath_type =
> >   br_int && br_int->datapath_type ? br_int->datapath_type : "";
> >   const char *cms_options = get_cms_options(&cfg->external_ids);
> > +const char *transport_zones = smap_get_def(&cfg->external_ids,
> > +   "ovn-transport-zones", "");
> >
> >   struct ds iface_types = DS_EMPTY_INITIALIZER;
> >   ds_put_cstr(&iface_types, "");
> > @@ -167,18 +169,22 @@ chassis_run(struct ovsdb_idl_txn *ovnsb_idl_txn,
> >   = smap_get_def(&chassis_rec->external_ids, "iface-types", "");
> >   const char *chassis_cms_options
> >   = get_cms_options(&chassis_rec->external_ids);
> > +const char *chassis_transport_zones = smap_get_def(
> > +&chassis_rec->external_ids, "ovn-transport-zones", "");
> >
> >   /*

Re: [ovs-dev] [PATCH] ovn-nbctl: Don't segfault when ovn-northd doesn't configure dynamic addresses.

[Lucas Alvares Gomes]

On Mon, Mar 4, 2019 at 10:52 PM Justin Pettit  wrote:
>
> When ovn-nbctl is used to configure a logical switch port's addresses, it
> does a sanity-check to make sure that a duplicate address isn't being
> used.  If a port is configured as "dynamic", ovn-northd is supposed to
> populate the "dynamic_addresses" column in the Logical_Switch_Port
> table.  If it isn't ovn-nbctl, would dereference a null pointer as part
> of the duplicate address check.  This patch checks that "dynamic_addresses"
> is actually set first.
>
> Signed-off-by: Justin Pettit 
> ---
>  ovn/utilities/ovn-nbctl.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
> index 8a72e95741e2..2727b410a154 100644
> --- a/ovn/utilities/ovn-nbctl.c
> +++ b/ovn/utilities/ovn-nbctl.c
> @@ -1489,7 +1489,7 @@ lsp_contains_duplicates(const struct 
> nbrec_logical_switch *ls,
>  for (size_t j = 0; j < lsp_test->n_addresses; j++) {
>  struct lport_addresses laddrs_test;
>  char *addr = lsp_test->addresses[j];
> -if (is_dynamic_lsp_address(addr)) {
> +if (is_dynamic_lsp_address(addr) && lsp_test->dynamic_addresses) 
> {
>  addr = lsp_test->dynamic_addresses;
>  }
>  if (extract_lsp_addresses(addr, &laddrs_test)) {
> --
> 2.17.1

Looks good to me.

Acked-by: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ovn: change load balancer references to weak in NB schema

[Lucas Alvares Gomes]

Thanks Daniel,I agree it makes things easier for the integration with OpenStack.
On Mon, Feb 11, 2019 at 4:07 PM Daniel Alvarez  wrote:
>
> When a load balancer is added to multiple logical switches
> and routers it has be to be removed from all of them before
> being able to delete due to the current 'strong' reference
> in the NB schema.
>
> By changing it to 'weak', users can simply remove the load
> balancer without having to remove all the references manually.
> In particular, this will make things easier for networking-ovn,
> the OpenStack integration project as it'll save some
> calculations upon load balancer deletion.
>
> The update path has been successfully from the previous version
> of the schema.
>
> Signed-off-by: Daniel Alvarez 
> ---
>  ovn/ovn-nb.ovsschema |  8 
>  tests/ovn-nbctl.at   | 10 +-
>  2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
> index f3683df14..10a59649a 100644
> --- a/ovn/ovn-nb.ovsschema
> +++ b/ovn/ovn-nb.ovsschema
> @@ -1,7 +1,7 @@
>  {
>  "name": "OVN_Northbound",
> -"version": "5.14.0",
> -"cksum": "3600467067 20513",
> +"version": "5.14.1",
> +"cksum": "3758097843 20509",
>  "tables": {
>  "NB_Global": {
>  "columns": {
> @@ -46,7 +46,7 @@
>"max": "unlimited"}},
>  "load_balancer": {"type": {"key": {"type": "uuid",
>"refTable": 
> "Load_Balancer",
> -  "refType": "strong"},
> +  "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
>  "dns_records": {"type": {"key": {"type": "uuid",
> @@ -250,7 +250,7 @@
>   "max": "unlimited"}},
>  "load_balancer": {"type": {"key": {"type": "uuid",
>"refTable": 
> "Load_Balancer",
> -  "refType": "strong"},
> +  "refType": "weak"},
> "min": 0,
> "max": "unlimited"}},
>  "options": {
> diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at
> index f55277cee..7a5903c3a 100644
> --- a/tests/ovn-nbctl.at
> +++ b/tests/ovn-nbctl.at
> @@ -752,7 +752,15 @@ AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
>  AT_CHECK([ovn-nbctl lr-lb-add lr0 lb1])
>  AT_CHECK([ovn-nbctl lr-lb-add lr0 lb3])
>  AT_CHECK([ovn-nbctl lr-lb-del lr0])
> -AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])])
> +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> +
> +dnl Remove load balancers after adding them to a logical router/switch.
> +AT_CHECK([ovn-nbctl lr-lb-add lr0 lb0])
> +AT_CHECK([ovn-nbctl ls-lb-add ls0 lb1])
> +AT_CHECK([ovn-nbctl lb-del lb0])
> +AT_CHECK([ovn-nbctl lb-del lb1])
> +AT_CHECK([ovn-nbctl lr-lb-list lr0 | uuidfilt], [0], [])
> +AT_CHECK([ovn-nbctl ls-lb-list ls0 | uuidfilt], [0], [])])
>
>  dnl -
>
> --
> 2.17.2 (Apple Git-113)
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Un-revert Work around Python/C JSON unicode differences

[Lucas Alvares Gomes]

On Mon, Jan 14, 2019 at 2:35 PM Terry Wilson  wrote:
>
> This fix was reverted because it depended on a small bit of code
> in a patch that was reverted that changed some python/ovs testing
> and build. The fix is still necessary.
>
> The OVS C-based JSON parser operates on bytes, so the parser_feed
> function returns the number of bytes that are processed. The pure
> Python JSON parser currently operates on unicode, so it expects
> that Parser.feed() returns a number of characters. This difference
> leads to parsing errors when unicode characters are passed to the
> C JSON parser from Python.
>
> Signed-off-by: Terry Wilson 
> ---
>  python/ovs/json.py| 10 ++
>  python/ovs/jsonrpc.py |  9 +++--
>  2 files changed, 13 insertions(+), 6 deletions(-)
>
> diff --git a/python/ovs/json.py b/python/ovs/json.py
> index e84063f..f06204e 100644
> --- a/python/ovs/json.py
> +++ b/python/ovs/json.py
> @@ -21,10 +21,13 @@ import sys
>
>  import six
>
> +PARSER_C = 'C'
> +PARSER_PY = 'PYTHON'
>  try:
>  import ovs._json
> +PARSER = PARSER_C
>  except ImportError:
> -pass
> +PARSER = PARSER_PY
>
>  __pychecker__ = 'no-stringiter'
>
> @@ -91,10 +94,9 @@ class Parser(object):
>  MAX_HEIGHT = 1000
>
>  def __new__(cls, *args, **kwargs):
> -try:
> +if PARSER == PARSER_C:
>  return ovs._json.Parser(*args, **kwargs)
> -except NameError:
> -return super(Parser, cls).__new__(cls)
> +return super(Parser, cls).__new__(cls)
>
>  def __init__(self, check_trailer=False):
>  self.check_trailer = check_trailer
> diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
> index cc7b2cd..4a3027e 100644
> --- a/python/ovs/jsonrpc.py
> +++ b/python/ovs/jsonrpc.py
> @@ -272,7 +272,8 @@ class Connection(object):
>  # data, so we convert it here as soon as possible.
>  if data and not error:
>  try:
> -data = decoder.decode(data)
> +if six.PY3 or ovs.json.PARSER == ovs.json.PARSER_PY:
> +data = decoder.decode(data)
>  except UnicodeError:
>  error = errno.EILSEQ
>  if error:
> @@ -298,7 +299,11 @@ class Connection(object):
>  else:
>  if self.parser is None:
>  self.parser = ovs.json.Parser()
> -self.input = self.input[self.parser.feed(self.input):]
> +if six.PY3 and ovs.json.PARSER == ovs.json.PARSER_C:
> +self.input = self.input.encode('utf-8')[
> +self.parser.feed(self.input):].decode()
> +else:
> +self.input = self.input[self.parser.feed(self.input):]
>  if self.parser.is_done():
>      msg = self.__process_msg()
>  if msg:
> --
> 1.8.3.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 2/2 v2] Work around Python/C JSON unicode differences

[Lucas Alvares Gomes]

On Tue, Oct 9, 2018 at 5:33 PM Terry Wilson  wrote:
>
> The OVS C-based JSON parser operates on bytes, so the parser_feed
> function returns the number of bytes that are processed. The pure
> Python JSON parser currently operates on unicode, so it expects
> that Parser.feed() returns a number of characters. This difference
> leads to parsing errors when unicode characters are passed to the
> C JSON parser from Python.
>
> Signed-off-by: Terry Wilson 
> ---
>  python/ovs/jsonrpc.py | 9 +++--
>  1 file changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
> index 4873cff..1323ba7 100644
> --- a/python/ovs/jsonrpc.py
> +++ b/python/ovs/jsonrpc.py
> @@ -272,7 +272,8 @@ class Connection(object):
>  # data, so we convert it here as soon as possible.
>  if data and not error:
>  try:
> -data = decoder.decode(data)
> +if six.PY3 or ovs.json.PARSER == ovs.json.PARSER_PY:
> +data = decoder.decode(data)
>  except UnicodeError:
>  error = errno.EILSEQ
>  if error:
> @@ -298,7 +299,11 @@ class Connection(object):
>  else:
>  if self.parser is None:
>  self.parser = ovs.json.Parser()
> -self.input = self.input[self.parser.feed(self.input):]
> +if six.PY3 and ovs.json.PARSER == ovs.json.PARSER_C:
> +self.input = self.input.encode('utf-8')[
> +self.parser.feed(self.input):].decode()
> +else:
> +self.input = self.input[self.parser.feed(self.input):]
>  if self.parser.is_done():
>  msg = self.__process_msg()
>  if msg:
> --
> 1.8.3.1
>
> ___________
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-By: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN/OVS split: OVN mailing list?

[Lucas Alvares Gomes]

Hi,

Is there any progress on this ?

Cheers,
Lucas

On Mon, Aug 13, 2018 at 1:43 PM Miguel Angel Ajo Pelayo
 wrote:
>
> +1 on my side too :) Thank you for bringing up the topic
>
> On Mon, Aug 13, 2018 at 2:03 PM Daniel Alvarez Sanchez  
> wrote:
>>
>> +1 for the split of the ML
>>
>> On Mon, Aug 13, 2018 at 11:17 AM Lucas Alvares Gomes 
>> wrote:
>>
>> > Hi,
>> >
>> > > Before starting in-depth technical discussions on this list or the
>> > > ovs-dev list, I'm curious if people would be interested in splitting off
>> > > a separate OVN list for this and future OVN-related discussions? I can
>> > > see merits of keeping discussions on this list and of starting a new
>> > > one, so I'm interested in what others think about the matter.
>> > >
>> >
>> > As someone that relies on filters to get the OVN related
>> > changes/discussions from the current ML I can definitely see the
>> > benefits of creating a separated list for OVN. Also, since the code
>> > split is on it's way and OVN will become a project own its own this
>> > sounds like a natural first step to be taken towards that goal.
>> >
>> > Cheers,
>> > Lucas
>> > ___
>> > dev mailing list
>> > d...@openvswitch.org
>> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>> >
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] OVN/OVS split: OVN mailing list?

[Lucas Alvares Gomes]

Hi,

> Before starting in-depth technical discussions on this list or the
> ovs-dev list, I'm curious if people would be interested in splitting off
> a separate OVN list for this and future OVN-related discussions? I can
> see merits of keeping discussions on this list and of starting a new
> one, so I'm interested in what others think about the matter.
>

As someone that relies on filters to get the OVN related
changes/discussions from the current ML I can definitely see the
benefits of creating a separated list for OVN. Also, since the code
split is on it's way and OVN will become a project own its own this
sounds like a natural first step to be taken towards that goal.

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [branch-2.9] Revert "flow: Fix buffer overread for crafted IPv6 packets."

[Lucas Alvares Gomes]

Thanks Justin,

In networking-ovn (the OVN driver for OpenStack Neutron) we are seem
an IPv6 related test failure right now [0] and I can confirm that
after I've applied this patch locally and re-ran the test it works
again.

[0] 
http://logs.openstack.org/59/570459/2/check/networking-ovn-tempest-dsvm-ovs-release/4b5bb1d/logs/testr_results.html.gz
(the link will eventually expire)

Acked-By: Lucas Alvares Gomes 

On Sat, Jul 14, 2018 at 9:33 PM Justin Pettit  wrote:
>
> This reverts commit 0760bd61a666e9fa866fcb5ed67f48f34895d2f6.
>
> This patch was a cherry-pick from a bug fix in the master branch that
> fixed an overread for IPv6 packets.  However, the backport introduced a
> problem in older branches, since the code path is different.  In the
> master branch, this check is done on the raw packet data, which starts
> at the beginning of the IPv6 packet.  In older branches, this check is
> done after a call to data_pull(), which subtracts the IPv6 header length
> from the 'size' variable.  This means that valid IPv6 packets aren't
> being processed since the check thinks they are too long.
>
> CC: Ben Pfaff 
> Fixes: 0760bd61a66 ("flow: Fix buffer overread for crafted IPv6 packets.")
> Signed-off-by: Justin Pettit 
>
> ---
> This patch should be backported to older branches starting with branch-2.9.
> ---
>  lib/flow.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/flow.c b/lib/flow.c
> index c78f46d6c15a..f9d7c2a74007 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -804,7 +804,7 @@ miniflow_extract(struct dp_packet *packet, struct 
> miniflow *dst)
>  nh = data_pull(&data, &size, sizeof *nh);
>
>  plen = ntohs(nh->ip6_plen);
> -if (OVS_UNLIKELY(plen + IPV6_HEADER_LEN > size)) {
> +if (OVS_UNLIKELY(plen > size)) {
>  goto out;
>  }
>  /* Jumbo Payload option not supported yet. */
> --
> 2.17.1
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 3/3] OVN: add protocol unreachable support to OVN router ports

[Lucas Alvares Gomes]

 Hi,

> this should be the same issue reported by Darrell ('ovn: Fix gateway
> load balancing')
> Regards,
>

Yeah that's right, I have applied Darrell's patch [0] and re-run the
same test that myself and Daniel were debugging [1] and I can confirm
that it works now.

Thanks for the pointers!

[0] http://patchwork.ozlabs.org/patch/935938/
[1] 
https://github.com/openstack/neutron-tempest-plugin/blob/8bc66e3205b834e17e9a9e6b72b6203a7a02cada/neutron_tempest_plugin/scenario/test_floatingip.py#L180-L200

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovn-northd: Apply pre ACLs when using Port Groups

[Lucas Alvares Gomes]

tatic void
> -ovn_port_group_ls_add(struct ovn_port_group *pg,
> -  const struct nbrec_logical_switch *nb_ls)
> -{
> -struct ovn_port_group_ls *pg_ls = xzalloc(sizeof *pg_ls);
> -pg_ls->key = nb_ls->header_.uuid;
> -pg_ls->nb_ls = nb_ls;
> -hmap_insert(&pg->nb_lswitches, &pg_ls->key_node, uuid_hash(&pg_ls->key));
> -}
> -
> -static struct ovn_port_group_ls *
> -ovn_port_group_ls_find(struct ovn_port_group *pg, const struct uuid *ls_uuid)
> -{
> -struct ovn_port_group_ls *pg_ls;
> -
> -HMAP_FOR_EACH_WITH_HASH (pg_ls, key_node, uuid_hash(ls_uuid),
> - &pg->nb_lswitches) {
> -if (uuid_equals(ls_uuid, &pg_ls->key)) {
> -return pg_ls;
> -}
> -}
> -return NULL;
> -}
> -
>  static void
>  ovn_port_group_destroy(struct hmap *pgs, struct ovn_port_group *pg)
>  {
> @@ -3416,7 +3428,7 @@ static void
>  build_acls(struct ovn_datapath *od, struct hmap *lflows,
> struct hmap *port_groups)
>  {
> -bool has_stateful = has_stateful_acl(od);
> +bool has_stateful = has_stateful_acl(od, port_groups);
>
>  /* Ingress and Egress ACL Table (Priority 0): Packets are allowed by
>   * default.  A related rule at priority 1 is added below if there
> @@ -3769,7 +3781,7 @@ build_lswitch_flows(struct hmap *datapaths, struct hmap 
> *ports,
>  continue;
>  }
>
> -build_pre_acls(od, lflows);
> +build_pre_acls(od, lflows, port_groups);
>  build_pre_lb(od, lflows);
>  build_pre_stateful(od, lflows);
>  build_acls(od, lflows, port_groups);
> diff --git a/tests/ovn.at b/tests/ovn.at
> index 6553d17c6..93644b023 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9981,7 +9981,7 @@ ovn-nbctl create Port_Group name=pg2 ports="$pg2_ports"
>  # create ACLs on pg1 to drop traffic from pg2 to pg1
>  ovn-nbctl acl-add pg1 to-lport 1001 'outport == @pg1' drop
>  ovn-nbctl --type=port-group acl-add pg1 to-lport 1002 \
> -'outport == @pg1 && ip4.src == $pg2_ip4' allow-related
> +'outport == @pg1 && ip4.src == $pg2_ip4' allow
>
>  # Physical network:
>  #
> --
> 2.15.1 (Apple Git-101)
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-by: Lucas Alvares Gomes 

Thanks for the patch, I just pulled it in a test environment along
with the port group support series for networking-ovn [0], gave it a
go and it works.

[stack@host11 devstack]$ sudo ip net e
ovnmeta-7b7909ae-b000-4196-9433-6a6dea0f352e ssh cirros@10.0.0.3
The authenticity of host '10.0.0.3 (10.0.0.3)' can't be established.
RSA key fingerprint is SHA256:hECvgpUHbfO+hrwfOFmh6DYZxTL0p8gjBPkl8BwSNxA.
RSA key fingerprint is MD5:5c:12:10:b4:ca:a6:33:cc:52:8b:9b:06:c1:28:4f:2c.
Are you sure you want to continue connecting (yes/no)? yes
Warning: Permanently added '10.0.0.3' (RSA) to the list of known hosts.
cirros@10.0.0.3's password:
$ curl 169.254.169.254
1.0
2007-01-19
2007-03-01
2007-08-29
2007-10-10
2007-12-15
2008-02-01
2008-09-01
2009-04-04
latest$

[0] https://review.openstack.org/#/c/570186/
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/3] datapath: Fix max MTU size on RHEL 7.5 kernel

[Lucas Alvares Gomes]

On Tue, Jun 12, 2018 at 1:50 AM, Yi-Hung Wei  wrote:
> Without the patch, in RHEL 7.5, the maximum configurable MTU of vport
> internal device is 1500, which shall be 65535.  This patch fixes this
> issue.
>
> Fixes: 39ca338374ab ("datapath: compat: Fix build on RHEL 7.5")
> Reported-by: Lucas Alvares Gomes 
> Signed-off-by: Yi-Hung Wei 
> ---
>  datapath/vport-internal_dev.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> index 3cb8d06b2475..3fa86815c7fa 100644
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -169,6 +169,8 @@ static void do_setup(struct net_device *netdev)
>
>  #ifdef HAVE_NET_DEVICE_WITH_MAX_MTU
> netdev->max_mtu = ETH_MAX_MTU;
> +#elif HAVE_RHEL7_MAX_MTU
> +   netdev->extended->max_mtu = ETH_MAX_MTU;
>  #endif
> netdev->netdev_ops = &internal_dev_netdev_ops;
>
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Acked-by: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

Hi Yi-Hung,

> I format the patch and post in here:
> https://patchwork.ozlabs.org/patch/927995/ Could you help to take a
> look?

Oh great! Sorry for the delay on this, I will take a look!

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

Hi Yi-Hung Wei,

> But as mentioned in ovs commit 6c0bf091 ("datapath: use core MTU range
> checking in core net infra"), it might be the case that my commit [0]
> does not set max_mtu correctly.  How about the fix in the following?
> From what I tested, without the fix, min_mtu: 64, max_mtu: 1500, with
> that fix, min_mtu:64 and max_mtu: 65535.
>
>> As pointed out by commit [0], the ndo_change_mtu function pointer has been
>> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
>> on RHEL 7.5.
>>
>> So this patch fixes the backport issue by setting the
>> .extended.ndo_change_mtu when necessary.
>>
>> [0] 39ca338374abe367e28a2247bac9159695f19710
>
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -169,6 +169,8 @@ static void do_setup(struct net_device *netdev)
>
>  #ifdef HAVE_NET_DEVICE_WITH_MAX_MTU
> netdev->max_mtu = ETH_MAX_MTU;
> +#elif HAVE_RHEL7_MAX_MTU
> +   netdev->extended->max_mtu = ETH_MAX_MTU;
>  #endif
> netdev->netdev_ops = &internal_dev_netdev_ops;

Cool! I will give this a go and see if it works.

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: RHEL 7.5 ndo_change_mtu backward compatibility

[Lucas Alvares Gomes]

On Thu, May 17, 2018 at 12:27 PM,   wrote:
> From: Lucas Alvares Gomes 
>
> The commit [0] partially fixed the problem but in RHEL 7.5 neither
> .{min, max}_mtu or 'ndo_change_mtu' were being set/implemented for
> vport-internal_dev.c.
>
> As pointed out by commit [0], the ndo_change_mtu function pointer has been
> moved from 'struct net_device_ops' to 'struct net_device_ops_extended'
> on RHEL 7.5.
>
> So this patch fixes the backport issue by setting the
> .extended.ndo_change_mtu when necessary.
>
> [0] 39ca338374abe367e28a2247bac9159695f19710

Signed-off-by: Lucas Alvares Gomes 

Sorry, forgot it to append it to the commit message before submitting the patch.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] datapath: compat: Fix build on RHEL 7.5

[Lucas Alvares Gomes]

Great, thanks for the v2

On Fri, May 11, 2018 at 6:32 PM, Yi-Hung Wei  wrote:
> 1) OVS datapath compat modules breaks on RHEL 7.5, because it moves
> ndo_change_mtu function pointer from 'struct net_device_ops' to
> 'struct net_device_ops_extended'.
>
> 2) RHEL 7.5 introduces the MTU range checking as mentioned in
> 6c0bf091 ("datapath: use core MTU range checking in core net infra").
> However, the max_mtu field is defined in 'struct net_device_extended'
> but not in 'struct net_device' as upstream kernel.
>
> This patch defines a new symbol HAVE_RHEL7_MAX_MTU that determines
> the previous 2 conditions, and fixes the backport issue.
>
> Signed-off-by: Yi-Hung Wei 

Acked-by: Lucas Alvares Gomes 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] datapath: compat: Fix build on RHEL 7.5

[Lucas Alvares Gomes]

> diff --git a/datapath/vport-internal_dev.c b/datapath/vport-internal_dev.c
> index f48684b2e70f..90e76bac2165 100644
> --- a/datapath/vport-internal_dev.c
> +++ b/datapath/vport-internal_dev.c
> @@ -153,7 +153,7 @@ static const struct net_device_ops 
> internal_dev_netdev_ops = {
> .ndo_stop = internal_dev_stop,
> .ndo_start_xmit = internal_dev_xmit,
> .ndo_set_mac_address = eth_mac_addr,
> -#ifndef HAVE_NET_DEVICE_WITH_MAX_MTU
> +#if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) && !defined(HAVE_RHEL7_MAX_MTU)
> .ndo_change_mtu = internal_dev_change_mtu,
>  #endif
> .ndo_get_stats64 = (void *)internal_get_stats,

On RHEL 7.5, internal_dev_change_mtu will not be used and the compiler
will throw a warning about it:

  CC [M]  /root/ovs/datapath/linux/vport-internal_dev.o
/root/ovs/datapath/linux/vport-internal_dev.c:92:12: warning:
‘internal_dev_change_mtu’ defined but not used [-Wunused-function]
 static int internal_dev_change_mtu(struct net_device *dev, int new_mtu)

Can we have the same conditional [0] around that function definition
to avoid such warning ?

Other than that the patch looks good to me, thank you!

[0]  #if!defined(HAVE_NET_DEVICE_WITH_MAX_MTU) &&
!defined(HAVE_RHEL7_MAX_MTU)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v1] ovn-ctl: Trivial, remove duplicated stop_controller case option

[Lucas Alvares Gomes]

Signed-off-by: Lucas Alvares Gomes 
---
 ovn/utilities/ovn-ctl | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/ovn/utilities/ovn-ctl b/ovn/utilities/ovn-ctl
index 0e56bf8c5..dc0c26159 100755
--- a/ovn/utilities/ovn-ctl
+++ b/ovn/utilities/ovn-ctl
@@ -581,9 +581,6 @@ case $command in
 stop_controller)
 stop_controller
 ;;
-stop_controller)
-stop_controller
-;;
 stop_controller_vtep)
 stop_controller_vtep
 ;;
-- 
2.16.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] OVN python IDL: avoid useless JSON conversion to enhance performance

[Lucas Alvares Gomes]

Hi,

Excellent investigative work here Daniel, thanks for that!

On Wed, Feb 28, 2018 at 9:37 AM, Miguel Angel Ajo Pelayo
 wrote:
> And if we can get backports down the lane, that'd be great :)
>

+1 for backporting it. The changes seems straight forward to do so.

> On Wed, Feb 28, 2018 at 9:37 AM Miguel Angel Ajo Pelayo 
> wrote:
>
>> Acked-by: Miguel Angel Ajo 
>>
>> On Wed, Feb 28, 2018 at 9:13 AM Daniel Alvarez Sanchez <
>> dalva...@redhat.com> wrote:
>>
>>> Thanks Terry and Han for the reviews!
>>> I removed the 'OVN' keyword from the title and submitted a v2:
>>> [PATCH v2] python: avoid useless JSON conversion to enhance performance
>>>
>>> Thanks again.
>>> Daniel
>>>
>>> On Wed, Feb 28, 2018 at 12:41 AM, Han Zhou  wrote:
>>>
>>> > Great catch!
>>> > This patch is generic and not only for OVN, so I suggest to remove the
>>> > "OVN" keyword in commit title.
>>> >
>>> > Acked-by: Han Zhou 
>>> >
>>> > On Tue, Feb 27, 2018 at 12:44 PM, Terry Wilson 
>>> wrote:
>>> >
>>> >> On Tue, Feb 27, 2018 at 9:25 AM, Daniel Alvarez 
>>> >> wrote:
>>> >> > This patch removes a useless conversion to/from JSON in the
>>> >> > processing of any 'modify' operations inside the process_update2
>>> >> > method in Python IDL implementation.
>>> >> >
>>> >> > Previous code will make resources creation take longer as the number
>>> >> > of elements in the row grows because of that JSON conversion. This
>>> >> > patch eliminates it and now the time remains consant regardless
>>> >> > of the database contents improving performance and scaling.
>>> >> >
>>> >> > Reported-by: Daniel Alvarez Sanchez 
>>> >> > Reported-at: https://mail.openvswitch.org/p
>>> >> ipermail/ovs-discuss/2018-February/046263.html
>>> >> > Signed-off-by: Daniel Alvarez 
>>> >> > ---
>>> >> >  python/ovs/db/idl.py | 12 +---
>>> >> >  1 file changed, 5 insertions(+), 7 deletions(-)
>>> >> >
>>> >> > diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
>>> >> > index 60548bcf5..5a4d129c0 100644
>>> >> > --- a/python/ovs/db/idl.py
>>> >> > +++ b/python/ovs/db/idl.py
>>> >> > @@ -518,10 +518,8 @@ class Idl(object):
>>> >> >  if not row:
>>> >> >  raise error.Error('Modify non-existing row')
>>> >> >
>>> >> > -old_row_diff_json = self.__apply_diff(table, row,
>>> >> > -
>>> row_update['modify'])
>>> >> > -self.notify(ROW_UPDATE, row,
>>> >> > -Row.from_json(self, table, uuid,
>>> >> old_row_diff_json))
>>> >> > +old_row = self.__apply_diff(table, row,
>>> >> row_update['modify'])
>>> >> > +self.notify(ROW_UPDATE, row, Row(self, table, uuid,
>>> >> old_row))
>>> >> >  changed = True
>>> >> >  else:
>>> >> >  raise error.Error(' unknown operation',
>>> >> > @@ -584,7 +582,7 @@ class Idl(object):
>>> >> >  row_update[column.name] =
>>> >> self.__column_name(column)
>>> >> >
>>> >> >  def __apply_diff(self, table, row, row_diff):
>>> >> > -old_row_diff_json = {}
>>> >> > +old_row = {}
>>> >> >  for column_name, datum_diff_json in six.iteritems(row_diff):
>>> >> >  column = table.columns.get(column_name)
>>> >> >  if not column:
>>> >> > @@ -601,12 +599,12 @@ class Idl(object):
>>> >> >% (column_name, table.name, e))
>>> >> >  continue
>>> >> >
>>> >> > -old_row_diff_json[column_name] =
>>> >> row._data[column_name].to_json()
>>> >> > +old_row[column_name] = row._data[column_name].copy()
>>> >> >  datum = row._data[column_name].diff(datum_diff)
>>> >> >  if datum != row._data[column_name]:
>>> >> >  row._data[column_name] = datum
>>> >> >
>>> >> > -return old_row_diff_json
>>> >> > +return old_row
>>> >> >
>>> >> >  def __row_update(self, table, row, row_json):
>>> >> >  changed = False
>>> >> > --
>>> >> > 2.13.5
>>> >> >
>>> >> > ___
>>> >> > dev mailing list
>>> >> > d...@openvswitch.org
>>> >> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> >>
>>> >> +1
>>> >>
>>> >> This passes all of the functional tests in ovsdbapp when applied. Nice
>>> >> find!
>>> >>
>>> >> Terry
>>> >> ___
>>> >> dev mailing list
>>> >> d...@openvswitch.org
>>> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>> >>
>>> >
>>> >
>>> ___
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v1] OVN: Add external_ids to NAT and Logical_Router_Static_Route tables.

[Lucas Alvares Gomes Martins]

Hi,

Thanks a lot Russell and Ben for this email. Having this change in the
2.8 will make certain things *a lot* easier in networking-ovn.

I will go ahead and propose a patch for the 2.8 branch then.

Cheers,
Lucas

On Tue, Dec 5, 2017 at 7:20 PM, Ben Pfaff  wrote:
> I don't object to #2.
>
> On Tue, Dec 05, 2017 at 01:44:25PM -0500, Russell Bryant wrote:
>> Lucas asked me about backporting this one, as OpenStack would start
>> making use of it with an OVS 2.8 update if available.
>>
>> The schema change seems pretty harmless.  The catch is that this also
>> updated the schema version number from "5.8.1" to "5.8.2", while
>> branch-2.8 has "5.8.0".  master includes a change that introduced a
>> new feature, along with the "5.8.1" update, that we would not
>> backport.
>>
>> The main choices seem to be ...
>>
>> 1) Don't backport.
>>
>> 2) Backport, but leave the schema version number unchanged in branch-2.8.
>>
>> Does anyone see a problem with option #2?  It's easy enough to
>> determine if the new columns are present, even without the version
>> number bump.
>>
>> On Mon, Dec 4, 2017 at 2:11 PM, Ben Pfaff  wrote:
>> > Applied, thanks.
>> >
>> > On Mon, Dec 04, 2017 at 03:06:39PM +0100, Daniel Alvarez Sanchez wrote:
>> >> Acked-by: Daniel Alvarez 
>> >>
>> >> From [0] one can expect this column to be present in all tables.
>> >> [0] https://github.com/openvswitch/ovs/blob/v2.8.1/ovn/ovn-nb.xml#L19
>> >>
>> >> On Mon, Dec 4, 2017 at 2:16 PM,  wrote:
>> >>
>> >> > From: Lucas Alvares Gomes 
>> >> >
>> >> > The external_ids column is missing from the NAT and
>> >> > Logical_Router_Static_Route tables.
>> >> >
>> >> > Signed-off-by: Lucas Alvares Gomes 
>> >> > ---
>> >> >  ovn/ovn-nb.ovsschema | 14 ++
>> >> >  ovn/ovn-nb.xml   | 14 ++
>> >> >  2 files changed, 24 insertions(+), 4 deletions(-)
>> >> >
>> >> > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
>> >> > index fcd878cf2..081ddb54c 100644
>> >> > --- a/ovn/ovn-nb.ovsschema
>> >> > +++ b/ovn/ovn-nb.ovsschema
>> >> > @@ -1,7 +1,7 @@
>> >> >  {
>> >> >  "name": "OVN_Northbound",
>> >> > -"version": "5.8.1",
>> >> > -"cksum": "607160660 16929",
>> >> > +"version": "5.9.0",
>> >> > +"cksum": "1120419033 17249",
>> >> >  "tables": {
>> >> >  "NB_Global": {
>> >> >  "columns": {
>> >> > @@ -238,7 +238,10 @@
>> >> >   
>> >> > "dst-ip"]]},
>> >> >  "min": 0, "max": 1}},
>> >> >  "nexthop": {"type": "string"},
>> >> > -"output_port": {"type": {"key": "string", "min": 0,
>> >> > "max": 1}}},
>> >> > +"output_port": {"type": {"key": "string", "min": 0,
>> >> > "max": 1}},
>> >> > +"external_ids": {
>> >> > +"type": {"key": "string", "value": "string",
>> >> > + "min": 0, "max": "unlimited"}}},
>> >> >  "isRoot": false},
>> >> >  "NAT": {
>> >> >  "columns": {
>> >> > @@ -252,7 +255,10 @@
>> >> > "enum": ["set", ["dnat",
>> >> >   "snat",
>> >> >
>> >> > "dnat_and_snat"
>> >> > -   ]],
>> >> > +   ]]}}},
>> >> > +"external_ids&q

[ovs-dev] [PATCH] Add NAT information to the logical routers in nbctl show output

[Lucas Alvares Gomes]

This patch is changing the print_lr() function in ovn-nbctl.c to include
logical router NAT information as part of the output (external ip,
logical ip and type).

Signed-off-by: Lucas Alvares Gomes 
---
 ovn/utilities/ovn-nbctl.c | 12 
 1 file changed, 12 insertions(+)

diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c
index 598f502af..e9dcde701 100644
--- a/ovn/utilities/ovn-nbctl.c
+++ b/ovn/utilities/ovn-nbctl.c
@@ -589,6 +589,18 @@ print_lr(const struct nbrec_logical_router *lr, struct ds 
*s)
 ds_put_cstr(s, "]\n");
 }
 }
+
+for (size_t i = 0; i < lr->n_nat; i++) {
+const struct nbrec_nat *nat = lr->nat[i];
+ds_put_format(s, "nat "UUID_FMT"\n",
+  UUID_ARGS(&nat->header_.uuid));
+ds_put_cstr(s, "external ip: ");
+ds_put_format(s, "\"%s\"\n", nat->external_ip);
+ds_put_cstr(s, "logical ip: ");
+ds_put_format(s, "\"%s\"\n", nat->logical_ip);
+ds_put_cstr(s, "type: ");
+ds_put_format(s, "\"%s\"\n", nat->type);
+}
 }
 
 static void
-- 
2.12.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v3] python: Allow tuning the session probe_interval from IDL

[Lucas Alvares Gomes]

This patch is adding a new parameter called "probe_interval" to the
constructor of the Idl class. This new parameter will be used to tune
the database connection probing for that IDL session, some users might
want to tune it to be less agressive than the current 5s default in OVS
or even disable it.

Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1680146
Signed-off-by: Lucas Alvares Gomes 
---
 python/ovs/db/idl.py  | 12 +---
 python/ovs/jsonrpc.py | 11 +--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 079a03ba1..60548bcf5 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -94,7 +94,7 @@ class Idl(object):
 IDL_S_MONITOR_REQUESTED = 1
 IDL_S_MONITOR_COND_REQUESTED = 2
 
-def __init__(self, remote, schema):
+def __init__(self, remote, schema, probe_interval=None):
 """Creates and returns a connection to the database named 'db_name' on
 'remote', which should be in a form acceptable to
 ovs.jsonrpc.session.open().  The connection will maintain an in-memory
@@ -112,7 +112,12 @@ class Idl(object):
 As a convenience to users, 'schema' may also be an instance of the
 SchemaHelper class.
 
-The IDL uses and modifies 'schema' directly."""
+The IDL uses and modifies 'schema' directly.
+
+If "probe_interval" is zero it disables the connection keepalive
+feature. If non-zero the value will be forced to at least 1000
+milliseconds. If None it will just use the default value in OVS.
+"""
 
 assert isinstance(schema, SchemaHelper)
 schema = schema.get_idl_schema()
@@ -120,7 +125,8 @@ class Idl(object):
 self.tables = schema.tables
 self.readonly = schema.readonly
 self._db = schema
-self._session = ovs.jsonrpc.Session.open(remote)
+self._session = ovs.jsonrpc.Session.open(remote,
+probe_interval=probe_interval)
 self._monitor_request_id = None
 self._last_seqno = None
 self.change_seqno = 0
diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
index 69f7abeb8..09e9c8b0a 100644
--- a/python/ovs/jsonrpc.py
+++ b/python/ovs/jsonrpc.py
@@ -376,7 +376,7 @@ class Session(object):
 self.seqno = 0
 
 @staticmethod
-def open(name):
+def open(name, probe_interval=None):
 """Creates and returns a Session that maintains a JSON-RPC session to
 'name', which should be a string acceptable to ovs.stream.Stream or
 ovs.stream.PassiveStream's initializer.
@@ -387,7 +387,12 @@ class Session(object):
 If 'name' is a passive connection method, e.g. "ptcp:", the new session
 listens for connections to 'name'.  It maintains at most one connection
 at any given time.  Any new connection causes the previous one (if any)
-to be dropped."""
+to be dropped.
+
+If "probe_interval" is zero it disables the connection keepalive
+feature. If non-zero the value will be forced to at least 1000
+milliseconds. If None it will just use the default value in OVS.
+"""
 reconnect = ovs.reconnect.Reconnect(ovs.timeval.msec())
 reconnect.set_name(name)
 reconnect.enable(ovs.timeval.msec())
@@ -397,6 +402,8 @@ class Session(object):
 
 if not ovs.stream.stream_or_pstream_needs_probes(name):
 reconnect.set_probe_interval(0)
+elif probe_interval is not None:
+reconnect.set_probe_interval(probe_interval)
 
 return Session(reconnect, None)
 
-- 
2.12.2

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH v2] python: Allow tuning the session probe_interval from IDL

[Lucas Alvares Gomes]

This patch is adding a new parameter called "probe_interval" to the
constructor of the Idl class. This new parameter will be used to tune
the database connection probing for that IDL session, some users might
want to tune it to be less agressive than the current 5s default in OVS
or even disable it.

Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1680146
Signed-off-by: Lucas Alvares Gomes 
---
v2: Change the probe interval before the connection is opened

 python/ovs/db/idl.py  | 12 +---
 python/ovs/jsonrpc.py | 11 +--
 2 files changed, 18 insertions(+), 5 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 079a03ba1..60548bcf5 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -94,7 +94,7 @@ class Idl(object):
 IDL_S_MONITOR_REQUESTED = 1
 IDL_S_MONITOR_COND_REQUESTED = 2

-def __init__(self, remote, schema):
+def __init__(self, remote, schema, probe_interval=None):
 """Creates and returns a connection to the database named
'db_name' on
 'remote', which should be in a form acceptable to
 ovs.jsonrpc.session.open().  The connection will maintain an
in-memory
@@ -112,7 +112,12 @@ class Idl(object):
 As a convenience to users, 'schema' may also be an instance of the
 SchemaHelper class.

-The IDL uses and modifies 'schema' directly."""
+The IDL uses and modifies 'schema' directly.
+
+If "probe_interval" is zero it disables the connection keepalive
+feature. If non-zero the value will be forced to at least 1000
+milliseconds. If None it will just use the default value in OVS.
+"""

 assert isinstance(schema, SchemaHelper)
 schema = schema.get_idl_schema()
@@ -120,7 +125,8 @@ class Idl(object):
 self.tables = schema.tables
 self.readonly = schema.readonly
 self._db = schema
-self._session = ovs.jsonrpc.Session.open(remote)
+self._session = ovs.jsonrpc.Session.open(remote,
+probe_interval=probe_interval)
 self._monitor_request_id = None
 self._last_seqno = None
 self.change_seqno = 0
diff --git a/python/ovs/jsonrpc.py b/python/ovs/jsonrpc.py
index 69f7abeb8..09e9c8b0a 100644
--- a/python/ovs/jsonrpc.py
+++ b/python/ovs/jsonrpc.py
@@ -376,7 +376,7 @@ class Session(object):
 self.seqno = 0

 @staticmethod
-def open(name):
+def open(name, probe_interval=None):
 """Creates and returns a Session that maintains a JSON-RPC session
to
 'name', which should be a string acceptable to ovs.stream.Stream or
 ovs.stream.PassiveStream's initializer.
@@ -387,7 +387,12 @@ class Session(object):
 If 'name' is a passive connection method, e.g. "ptcp:", the new
session
 listens for connections to 'name'.  It maintains at most one
connection
 at any given time.  Any new connection causes the previous one (if
any)
-to be dropped."""
+to be dropped.
+
+If "probe_interval" is zero it disables the connection keepalive
+feature. If non-zero the value will be forced to at least 1000
+milliseconds. If None it will just use the default value in OVS.
+"""
 reconnect = ovs.reconnect.Reconnect(ovs.timeval.msec())
 reconnect.set_name(name)
 reconnect.enable(ovs.timeval.msec())
@@ -397,6 +402,8 @@ class Session(object):

 if not ovs.stream.stream_or_pstream_needs_probes(name):
 reconnect.set_probe_interval(0)
+elif probe_interval is not None:
+reconnect.set_probe_interval(probe_interval)

 return Session(reconnect, None)

--
2.12.2
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] python: Allow tuning the session probe_interval from IDL

[Lucas Alvares Gomes]

Hi,

> Nothing else in the Idl class is touching the reconnect object
> directly, which makes me wonder if this should also be a method on the
> session, or perhaps an option to Session.open().
>
> I also wonder if it'd be better as an optional argument to the
> constructor so that we can pass in the desired value before the
> connection is opened and is used from the beginning.  The separate
> method is tempting as far as writing code that can deal with versions
> of this library that may or may not have support for the official way
> of setting this, but it still feels like the "right" way is to be able
> to set the value before the connection is even opened.  What do you
> think?
>
> I feel like I'm nit picking this, sorry for that.

Not at all! That's some really useful feedback. Sure, let's set the
probe_interval value before we connect, it makes total sense. Also, I noted
that there's some logic checking whether to enable/disable proving when the
connection is being opened [0] which we should consider.

I will push a new patch soon. Thanks for the inputs!

[0]
https://github.com/openvswitch/ovs/blob/1b996ac917392b66cff5b7bb660c94ceb897ba57/python/ovs/jsonrpc.py#L398-L399

Cheers,
Lucas
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] python: Allow tuning the session probe_interval from IDL

[Lucas Alvares Gomes]

This patch is adding a new method set_probe_interval() to the Idl class
which allows configuring the probe interval for the IDL session.
Instances of Idl might want to tune this interval to be less agressive
than the current 5s one.

Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1680146
Signed-off-by: Lucas Alvares Gomes 
---
 python/ovs/db/idl.py | 9 +
 1 file changed, 9 insertions(+)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 079a03ba1..d3c0086c6 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -147,6 +147,15 @@ class Idl(object):
 table.condition = [True]
 table.cond_changed = False

+def set_probe_interval(self, probe_interval):
+"""Sets the probe interval for the IDL session, in milliseconds.
+
+If "probe_interval" is zero it disables the connection keepalive
+feature. If non-zero the value will be forced to at least 1000
+milliseconds.
+"""
+self._session.reconnect.set_probe_interval(probe_interval)
+
 def close(self):
 """Closes the connection to the database.  The IDL will no longer
 update."""
-- 
2.12.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev