[ovs-dev] [PATCH v4] ovn: Support for GARP for NAT IPs via localnet

[Guru Shetty]

On 12 August 2016 at 12:28, Chandra S Vejendla <csvejend at us.ibm.com> wrote:

> In cases where a DNAT IP is moved to a new router or the SNAT IP is reused
> with a new mac address, the NAT IPs become unreachable because the external
> switches/routers have stale ARP entries. This commit
> aims to fix the problem by sending GARPs for NAT IPs via locanet
>
> A new options key "nat-addresses" is added to the logical switch port of
> type router. The value for the key "nat-addresses" is the MAC address of
> the
> port followed by a list of SNAT & DNAT IPs.
>
> Signed-off-by: Chandra Sekhar Vejendla <csvejend at us.ibm.com>
> Acked-by: Ryan Moats <rmoats at us.ibm.com>
>

Thanks, it took me a while to wrap my head around the design, so thanks for
the help over the IRC.

This patch actually does 2 things.
1. Adds localnet support for l3gateway
2. Adds support for GARP for NAT addresses.

You should ideally have split this into 2 patches.

For the first part (localnet support), please expand on the commit message
to make it easy to understand. That way, when someone 'git annotate's a few
months down the line and finds this commit, they understand why whats being
done.

In the larger picture, why are we doing this on a switch's port? Can you
explain the rationale behind it (in the commit message)? We could have done
the same thing as you are doing on the router's port itself, isn't it?

.......snip......

-bool extract_lsp_addresses(char *address, struct lport_addresses *);
>
+bool extract_lsp_addresses(const char *address, struct lport_addresses *);
>  bool extract_lrp_networks(const struct nbrec_logical_router_port *,
>                            struct lport_addresses *);
>  void destroy_lport_addresses(struct lport_addresses *);
> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c
> index 861f872..61a9705 100644
> --- a/ovn/northd/ovn-northd.c
> +++ b/ovn/northd/ovn-northd.c
> @@ -1186,6 +1186,20 @@ ovn_port_update_sbrec(const struct ovn_port *op)
>              if (chassis) {
>                  smap_add(&new, "l3gateway-chassis", chassis);
>              }
> +
> +            const char *nat_addresses = smap_get(&op->nbsp->options,
> +                                           "nat-addresses");
> +            if (nat_addresses) {
> +                struct lport_addresses laddrs;
> +                if (!extract_lsp_addresses(nat_addresses, &laddrs)) {
>
The above creates a memory leak. I do not see a corresponding destroy_lport_
addresses()


> +                    static struct vlog_rate_limit rl =
> +                        VLOG_RATE_LIMIT_INIT(1, 1);
> +                    VLOG_WARN_RL(&rl, "Error extracting nat-addresses.");
> +                }
> +                else {
>

The else condition needs to move up one line (CodingStyle violation).


> +                    smap_add(&new, "nat-addresses", nat_addresses);
> +                }
> +            }
>              sbrec_port_binding_set_options(op->sb, &new);
>              smap_destroy(&new);
>          }
> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
> index 4ce295a..3fd8690 100644
> --- a/ovn/ovn-nb.xml
> +++ b/ovn/ovn-nb.xml
> @@ -225,6 +225,16 @@
>            table="Logical_Router_Port"/> to which this logical switch port
> is
>            connected.
>          </column>
> +
> +        <column name="options" key="nat-addresses">
> +          MAC address of the <code>router-port</code> followed by a list
> of
> +          SNAT and DNAT IP adddresses. This is used to send gratuitous
> ARPs for
>
s/adddresses/addresses



> +          SNAT and DNAT IP addresses via <code>localnet</code> and is
> valid for
> +          only L3 gateway ports.  Example: <code>80:fa:5b:06:72:b7
> 158.36.44.22
> +          158.36.44.24</code>. This would result in generation of
> gratuitous
> +          ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC
> +          address of 80:fa:5b:06:72:b7.
> +        </column>
>        </group>
>
>        <group title="Options for localnet ports">
> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml
> index 13c9526..a65abbe 100644
> --- a/ovn/ovn-sb.xml
> +++ b/ovn/ovn-sb.xml
> @@ -1650,6 +1650,16 @@ tcp.flags = RST;
>        <column name="options" key="l3gateway-chassis">
>          The <code>chassis</code> in which the port resides.
>        </column>
> +
> +        <column name="options" key="nat-addresses">
> +          MAC address of the <code>l3gateway</code> port followed by a
> list of
> +          SNAT and DNAT IP adddresses. This is used to send gratuitous
> ARPs for
>
s/adddresses/addresses


> +          SNAT and DNAT IP addresses via <code>localnet</code> and is
> valid for
> +          only L3 gateway ports.  Example: <code>80:fa:5b:06:72:b7
> 158.36.44.22
> +          158.36.44.24</code>. This would result in generation of
> gratuitous
> +          ARPs for IP addresses 158.36.44.22 and 158.36.44.24 with a MAC
> +          address of 80:fa:5b:06:72:b7.
> +        </column>
>      </group>
>
>