[ovs-dev] [PATCH] ovn: Cache mac bindings from broadcasted dynamic ARP responses

[Babu Shanmugam]

Thank you the review, Ben. I posted a v1 for this patch. I corrected the 
bcast2lr action with datapath uuid and port name instead of using their ids.

--
Babu

On Saturday 24 December 2016 02:50 AM, Ben Pfaff wrote:
> On Thu, Dec 22, 2016 at 05:06:08PM +0530, Babu Shanmugam wrote:
>> This patch attempts to avoid the usage of MAC_Binding table.
>>
>> Dynamic ARP response originates from the logical ports with "unknown"
>> address. When the ARP resolution is requested via a logical
>> router datapath, ARP response will be delivered to the logical router
>> port of the switch. Since the logical router is distributed, the
>> response will never reach the other chassis from where the ARP resolution
>> is requested.
>>
>> This is done using a OVN logical action bcast2lr() which adds an action
>> to send the packet to a specific multicast group for the logical router
>> datapath. For the chassis that have an "unknown" port connected to any
>> logical router, the bcast2lr() action will do the broadcast. The other
>> chassis will listen for such broadcast packets and executes put_arp()
>> action on them.
>>
>> When the ovn-controller processes put_arp() action, it caches the MAC
>> bindings and this cache will be used to update the openflow table used
>> for the dynamic MAC resolution
>>
>> Signed-off-by: Babu Shanmugam <bschanmu at redhat.com>
> Thanks for the patch.  We need a solution for this problem, and this is
> the first real attempt I've seen.
>
> Clang says:
>
>      ../ovn/lib/actions.c:1672:9: error: variable 'dp_id' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>      ../ovn/lib/actions.c:1699:23: note: uninitialized use occurs here
>      ../ovn/lib/actions.c:1672:5: note: remove the 'if' if its condition is always true
>      ../ovn/lib/actions.c:1667:15: note: initialize the variable 'dp_id' to silence this warning
>      ../ovn/lib/actions.c:1672:9: error: variable 'chassis' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>      ../ovn/lib/actions.c:1700:22: note: uninitialized use occurs here
>      ../ovn/lib/actions.c:1672:5: note: remove the 'if' if its condition is always true
>      ../ovn/lib/actions.c:1669:18: note: initialize the variable 'chassis' to silence this warning
>      ../ovn/lib/actions.c:1672:9: error: variable 'port_key' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>      ../ovn/lib/actions.c:1701:26: note: uninitialized use occurs here
>      ../ovn/lib/actions.c:1672:5: note: remove the 'if' if its condition is always true
>      ../ovn/lib/actions.c:1668:18: note: initialize the variable 'port_key' to silence this warning
>      ../ovn/lib/actions.c:1672:9: error: variable 'is_this_chassis' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>      ../ovn/lib/actions.c:1702:30: note: uninitialized use occurs here
>      ../ovn/lib/actions.c:1672:5: note: remove the 'if' if its condition is always true
>      ../ovn/lib/actions.c:1670:25: note: initialize the variable 'is_this_chassis' to silence this warning
>      ../ovn/lib/actions.c:1709:19: error: format specifies type 'long' but the argument has type 'uint64_t' (aka 'unsigned long long') [-Werror,-Wformat]
>      ../ovn/northd/ovn-northd.c:3144:25: error: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long') [-Werror,-Wformat]
>      ../ovn/northd/ovn-northd.c:3145:25: error: format specifies type 'long' but the argument has type 'int64_t' (aka 'long long') [-Werror,-Wformat]
>
> GCC says:
>
>      ../ovn/lib/actions.c: In function 'format_BCAST2LR':
>      ../ovn/lib/actions.c:1708:41: error: format '%ld' expects argument of type 'long int', but argument 3 has type 'uint64_t {aka const long long unsigned int}' [-Werror=format=]
>           ds_put_format(s, "bcast2lr(dst_dp=%ld, dst_pkey=%d, chassis=\"%s\");",
>                                               ^
>      ../ovn/lib/actions.c: In function 'parse_action':
>      ../ovn/lib/actions.c:1702:28: error: 'is_this_chassis' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>           bc2lr->is_this_chassis = is_this_chassis;
>           ~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
>      ../ovn/lib/actions.c:1670:10: note: 'is_this_chassis' was declared here
>           bool is_this_chassis;
>                ^~~~~~~~~~~~~~~
>      ../ovn/lib/actions.c:1700:20: error: 'chassis' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>           bc2lr->chassis = chassis;
>           ~~~~~~~~~~~~~~~^~~~~~~~~
>      ../ovn/lib/actions.c:1669:11: note: 'chassis' was declared here
>           char *chassis;
>                 ^~~~~~~
>      ../ovn/northd/ovn-northd.c: In function ‘build_lswitch_flows’:
>      ../ovn/northd/ovn-northd.c:3142:44: error: format ‘%ld’ expects argument of type ‘long int’, but argument 3 has type ‘int64_t {aka const long long int}’ [-Werror=format=]
>                               "bcast2lr(dst_dp=%ld, dst_pkey=%ld, chassis=\"%s\");"
>                                                  ^
>      ../ovn/northd/ovn-northd.c:3142:58: error: format ‘%ld’ expects argument of type ‘long int’, but argument 4 has type ‘int64_t {aka const long long int}’ [-Werror=format=]
>                               "bcast2lr(dst_dp=%ld, dst_pkey=%ld, chassis=\"%s\");"
>
> I don't think that OVN logical actions should specify datapath or port
> numbers; there are no precedents.  Please use their names instead and
> let ovn-controller translate.
>
> Unfortunately, my biggest problem with this patch is that I simply don't
> understand the description of how this whole thing is supposed to work.
> Can you explain, step-by-step, how ARP now gets resolved?
>
> Thanks,
>
> Ben.