[ovs-dev] [PATCH v1 2/2] ovn: Send garp on localnet

[Ben Pfaff]

On Sun, Apr 03, 2016 at 09:49:04PM -0400, Ramu Ramamurthy wrote:
> In some usecases such as VM migration or when VMs reuse IP addresses,
> VMs become unreachable externally because
> external switches/routers on localnet
> have stale port-mac or arp caches. The problem resolves
> after some time when the caches ageout which could be
> minutes for port-mac bindings or hours for arp caches.
> 
> To fix this, send some gratuitous ARPs when a logical
> port on a localnet datapath gets added. Such gratuitous arps
> help on a best-effort basis to update the mac-port bindings and arp-caches
> of external switches and routers on the localnet.
> 
> Reported-at: https://bugs.launchpad.net/networking-ovn/+bug/1545897
> Reported-by: Kyle Mestery <mestery at mestery.com>
> Signed-off-by: Ramu Ramamurthy <ramu.ramamurthy at us.ibm.com>

Darrell (CCed) brought up some issues with this idea.  I don't know
whether his questions have been satisfactorily answered.

Is there an equivalent of GARP for IPv6?

The GARP code manages and acts based on timers, but I don't see anything
that calls, e.g., poll_timer_wait_until(), to ensure that ovn-controller
wakes up when it's time to send a GARP.  This call would ordinarily be
made by adding another call to pinctrl_wait().

Please be careful about coding style.  The first thing that jumps out at
me here is comment style.  CodingStyle.md says:

      Comments should be written as full sentences that start with a
    capital letter and end with a period.  Put two spaces between
    sentences.

(Comments that aren't on lines of their own don't need to be sentences
but still should be capitalized and end with a period.)

The following is unsafe if there is no such datapath, because
CONTAINER_OF does not treat a null pointer specially:

            struct local_datapath *ld =
                CONTAINER_OF(hmap_first_with_hash(local_datapaths,
                                                  pb->datapath->tunnel_key),
                             struct local_datapath, hmap_node);

Please write sizeof *garp here instead of sizeof(struct garp_data):

            /* the mac address must be valid */
            struct garp_data *garp = xmalloc(sizeof(struct garp_data));

as requested in CodingStyle:

      The "sizeof" operator is unique among C operators in that it accepts
    two very different kinds of operands: an expression or a type.  In
    general, prefer to specify an expression, e.g. "int *x =
    xmalloc(sizeof *x);".  When the operand of sizeof is an expression,
    there is no need to parenthesize that operand, and please don't.

Please put spaces around + and * as recommended by CodingStyle, e.g. here:
+            garp->announce_time = time_msec()+1000;
and here:
+    garp->announce_time = current_time + garp->backoff*1000;

Thanks,

Ben.