Re: [PATCH net-next v1 1/1] bonding: Adding limmited support for ARP monitoring with ovs.

[Jay Vosburgh <jv@jvosburgh.net>]

David J Wilder <wilder@us.ibm.com> wrote:

>Adding limited support for the ARP Monitoring feature when ovs is
>configured above the bond. When no vlan tags are used in the configuration
>or when the tag is added between the bond interface and the ovs bridge arp
>monitoring will function correctly. The use of tags between the ovs bridge
>and the routed interface are not supported.

	Looking at the patch, it isn't really "adding support," but
rather is disabling the "is this IP address configured above the bond"
checks if the bond is a member of an OVS bridge.  It also seems like it
would permit any ARP IP target, as long as the address is configured
somewhere on the system.

	Stated another way, the route lookup in bond_arp_send_all() for
the target IP address must return a device, but the logic to match that
device to the interface stack above the bond will always succeed if the
bond is a member of any OVS bridge.

	For example, given:
	
[ eth0, eth1 ] -> bond0 -> ovs-br -> ovs-port IP=10.0.0.1
eth2 IP=20.0.0.2

	Configuring arp_ip_target=20.0.0.2 on bond0 would apparently
succeed after this patch is applied, and the bond would send ARPs for
20.0.0.2.

>For example:
>1) bond0 -> ovs-br -> ovs-port (x.x.x.x) is supported
>2) bond0 -> bond0.100 -> ovs-br -> ovs-port (x.x.x.x) is supported.
>3) bond0 -> ovs-br -> ovs-port -> ovs-port.100 (x.x.x.x) is not supported.
>
>Configurations #1 and #2 were tested and verified to function corectly.
>In the second configuration the correct vlan tags were seen in the arp.

	Assuming that I'm understanding the behavior correctly, I'm not
sure that "if OVS then do whatever" is the right way to go, particularly
since this would still exhibit mysterious failures if a VLAN is
configured within OVS itself (case 3, above).

	I understand that the architecture of OVS limits the ability to
do these sorts of checks, but I'm unconvinced that implementing this
support halfway is going to create more issues than it solves.

	Lastly, thinking out loud here, I'm generally loathe to add more
options to bonding, but I'm debating whether this would be worth an
"ovs-is-a-black-box" option somewhere, so that users would have to
opt-in to the OVS alternate realm.

	-J

>Signed-off-by: David J Wilder <wilder@us.ibm.com>
>Signed-off-by: Pradeep Satyanarayana <pradeeps@linux.vnet.ibm.com>
>---
> drivers/net/bonding/bond_main.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 950d8e4d86f8..6f71a567ba37 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3105,7 +3105,13 @@ struct bond_vlan_tag *bond_verify_device_path(struct net_device *start_dev,
> 	struct net_device *upper;
> 	struct list_head  *iter;
> 
>-	if (start_dev == end_dev) {
>+	/* If start_dev is an OVS port then we have encountered an openVswitch
>+	 * bridge and can't go any further. The programming of the switch table
>+	 * will determine what packets will be sent to the bond. We can make no
>+	 * further assumptions about the network above the bond.
>+	 */
>+
>+	if (start_dev == end_dev || netif_is_ovs_port(start_dev)) {
> 		tags = kcalloc(level + 1, sizeof(*tags), GFP_ATOMIC);
> 		if (!tags)
> 			return ERR_PTR(-ENOMEM);

---
	-Jay Vosburgh, jv@jvosburgh.net