From 36a57e7a388277d1e45f0cadd8e2601490a76b2d Mon Sep 17 00:00:00 2001 From: Dumitru Ceara Date: Wed, 3 Feb 2021 20:36:30 +0100 Subject: [PATCH 1/4] binding: Correctly set Port_Binding.up for container/virtual ports. For port bindings that are children of other port bindings (container and virtual ports) set Port_Binding.up directly, when claimed, if their parent bindings are already up. For non-VIF port bindings maintain compatibility with older versions and set Port_Binding.up as soon as they are claimed. Reported-by: Ben Pfaff Fixes: 4d3cb42b076b ("binding: Set Logical_Switch_Port.up when all OVS flows are installed.") Signed-off-by: Dumitru Ceara Signed-off-by: Numan Siddique (cherry picked from upstream commit aae25e67b1ba86e34d670eb808de86f7ba66c3c0) Conflicts: tests/ovn.at Change-Id: I17c5a4d0f89868190c81c7221caadf29959064b1 --- controller-vtep/binding.c | 3 +++ controller/binding.c | 47 +++++++++++++++++++++++++++++++++++++++++------ tests/ovn.at | 32 ++++++++++++++++++++++++++------ 3 files changed, 70 insertions(+), 12 deletions(-) diff --git a/controller-vtep/binding.c b/controller-vtep/binding.c index 8337715..d28a598 100644 --- a/controller-vtep/binding.c +++ b/controller-vtep/binding.c @@ -109,7 +109,10 @@ update_pb_chassis(const struct sbrec_port_binding *port_binding_rec, port_binding_rec->chassis->name, chassis_rec->name); } + + bool up = true; sbrec_port_binding_set_chassis(port_binding_rec, chassis_rec); + sbrec_port_binding_set_up(port_binding_rec, &up, 1); } } diff --git a/controller/binding.c b/controller/binding.c index d44f635..353debe 100644 --- a/controller/binding.c +++ b/controller/binding.c @@ -864,21 +864,52 @@ get_lport_type(const struct sbrec_port_binding *pb) return LP_UNKNOWN; } +/* For newly claimed ports, if 'notify_up' is 'false': + * - set the 'pb.up' field to true if 'pb' has no 'parent_pb'. + * - set the 'pb.up' field to true if 'parent_pb.up' is 'true' (e.g., for + * container and virtual ports). + * Otherwise request a notification to be sent when the OVS flows + * corresponding to 'pb' have been installed. + */ +static void +claimed_lport_set_up(const struct sbrec_port_binding *pb, + const struct sbrec_port_binding *parent_pb, + const struct sbrec_chassis *chassis_rec, + bool notify_up) +{ + if (!notify_up) { + bool up = true; + if (!parent_pb || (parent_pb->n_up && parent_pb->up[0])) { + sbrec_port_binding_set_up(pb, &up, 1); + } + return; + } + + if (pb->chassis != chassis_rec) { + binding_iface_bound_add(pb->logical_port); + } +} + /* Returns false if lport is not claimed due to 'sb_readonly'. * Returns true otherwise. */ static bool claim_lport(const struct sbrec_port_binding *pb, + const struct sbrec_port_binding *parent_pb, const struct sbrec_chassis *chassis_rec, const struct ovsrec_interface *iface_rec, - bool sb_readonly, struct hmap *tracked_datapaths) + bool sb_readonly, bool notify_up, + struct hmap *tracked_datapaths) { + if (!sb_readonly) { + claimed_lport_set_up(pb, parent_pb, chassis_rec, notify_up); + } + if (pb->chassis != chassis_rec) { if (sb_readonly) { return false; } - binding_iface_bound_add(pb->logical_port); if (pb->chassis) { VLOG_INFO("Changing chassis for lport %s from %s to %s.", pb->logical_port, pb->chassis->name, @@ -1041,8 +1072,12 @@ consider_vif_lport_(const struct sbrec_port_binding *pb, if (lbinding_set) { if (can_bind) { /* We can claim the lport. */ - if (!claim_lport(pb, b_ctx_in->chassis_rec, lbinding->iface, - !b_ctx_in->ovnsb_idl_txn, + const struct sbrec_port_binding *parent_pb = + lbinding->parent ? lbinding->parent->pb : NULL; + + if (!claim_lport(pb, parent_pb, b_ctx_in->chassis_rec, + lbinding->iface, !b_ctx_in->ovnsb_idl_txn, + !lbinding->parent, b_ctx_out->tracked_dp_bindings)){ return false; } @@ -1246,8 +1281,8 @@ consider_nonvif_lport_(const struct sbrec_port_binding *pb, b_ctx_out->tracked_dp_bindings); update_local_lport_ids(pb, b_ctx_out); - return claim_lport(pb, b_ctx_in->chassis_rec, NULL, - !b_ctx_in->ovnsb_idl_txn, + return claim_lport(pb, NULL, b_ctx_in->chassis_rec, NULL, + !b_ctx_in->ovnsb_idl_txn, false, b_ctx_out->tracked_dp_bindings); } else if (pb->chassis == b_ctx_in->chassis_rec) { return release_lport(pb, !b_ctx_in->ovnsb_idl_txn, diff --git a/tests/ovn.at b/tests/ovn.at index dfb94d2..1956f5c 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -8992,6 +8992,7 @@ AT_CHECK([test -z $bar2_zoneid]) # Add back bar2 ovn-nbctl lsp-add bar bar2 vm2 1 \ -- lsp-set-addresses bar2 "f0:00:00:01:02:08 192.168.2.3" +wait_for_ports_up ovn-nbctl --wait=hv sync bar2_zoneid=$(as hv2 ovs-vsctl get bridge br-int external_ids:ct-zone-bar2) @@ -14671,6 +14672,8 @@ OVS_WAIT_UNTIL( logical_port=ls1-lp_ext1` test "$chassis" = "$hv1_uuid"]) +wait_for_ports_up ls1-lp_ext1 + # There should be DHCPv4/v6 OF flows for the ls1-lp_ext1 port in hv1 (ovn-sbctl dump-flows lr0; ovn-sbctl dump-flows ls1) > sbflows as hv1 ovs-ofctl dump-flows br-int > brintflows @@ -14951,6 +14954,7 @@ OVS_WAIT_UNTIL( [chassis=`ovn-sbctl --bare --columns chassis find port_binding \ logical_port=ls1-lp_ext1` test "$chassis" = "$hv2_uuid"]) +wait_for_ports_up ls1-lp_ext1 # There should be OF flows for DHCP4/v6 for the ls1-lp_ext1 port in hv2 AT_CHECK([as hv2 ovs-ofctl dump-flows br-int | \ @@ -15065,6 +15069,7 @@ OVS_WAIT_UNTIL( [chassis=`ovn-sbctl --bare --columns chassis find port_binding \ logical_port=ls1-lp_ext1` test "$chassis" = "$hv1_uuid"]) +wait_for_ports_up ls1-lp_ext1 as hv1 ovs-vsctl show @@ -15145,6 +15150,7 @@ OVS_WAIT_UNTIL( [chassis=`ovn-sbctl --bare --columns chassis find port_binding \ logical_port=ls1-lp_ext1` test "$chassis" = "$hv3_uuid"]) +wait_for_ports_up ls1-lp_ext1 as hv1 ovs-vsctl show @@ -15229,6 +15235,7 @@ OVS_WAIT_UNTIL( [chassis=`ovn-sbctl --bare --columns chassis find port_binding \ logical_port=ls1-lp_ext1` test "$chassis" = "$hv1_uuid"]) +wait_for_ports_up ls1-lp_ext1 # There should be a flow in hv2 to drop traffic from ls1-lp_ext1 destined # to router mac. @@ -15246,6 +15253,7 @@ OVS_WAIT_UNTIL( [chassis=`ovn-sbctl --bare --columns chassis find port_binding \ logical_port=ls1-lp_ext1` test "$chassis" = "$hv2_uuid"]) +wait_for_ports_up ls1-lp_ext1 as hv1 OVS_APP_EXIT_AND_WAIT([ovs-vswitchd]) @@ -16604,12 +16612,10 @@ spa=$(ip_to_hex 10 0 0 10) tpa=$(ip_to_hex 10 0 0 10) send_garp 1 1 $eth_src $eth_dst $spa $tpa -OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \ -logical_port=sw0-vir) = x$hv1_ch_uuid], [0], []) - -AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ -logical_port=sw0-vir) = xsw0-p1]) - +wait_row_count Port_Binding 1 logical_port=sw0-vir chassis=$hv1_ch_uuid +check_row_count Port_Binding 1 logical_port=sw0-vir virtual_parent=sw0-p1 +wait_for_ports_up sw0-vir +check ovn-nbctl --wait=hv sync # There should be an arp resolve flow to resolve the virtual_ip with the # sw0-p1's MAC. @@ -16627,6 +16633,8 @@ ovn-sbctl clear port_binding $pb_uuid virtual_parent OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns chassis find port_binding \ logical_port=sw0-vir) = x]) +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-vir + # From sw0-p0 resend GARP for 10.0.0.10. hv1 should reclaim sw0-vir # and sw0-p1 should be its virtual_parent. send_garp 1 1 $eth_src $eth_dst $spa $tpa @@ -16637,6 +16645,8 @@ logical_port=sw0-vir) = x$hv1_ch_uuid], [0], []) AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = xsw0-p1]) +wait_for_ports_up sw0-vir + # From sw0-p3 send GARP for 10.0.0.10. hv1 should claim sw0-vir # and sw0-p3 should be its virtual_parent. eth_src=505400000005 @@ -16651,6 +16661,7 @@ logical_port=sw0-vir) = x$hv1_ch_uuid], [0], []) OVS_WAIT_UNTIL([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = xsw0-p3]) +wait_for_ports_up sw0-vir # There should be an arp resolve flow to resolve the virtual_ip with the # sw0-p2's MAC. @@ -16676,6 +16687,7 @@ logical_port=sw0-vir) = x$hv2_ch_uuid], [0], []) AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = xsw0-p2]) +wait_for_ports_up sw0-vir # There should be an arp resolve flow to resolve the virtual_ip with the # sw0-p3's MAC. @@ -16701,6 +16713,8 @@ sleep 1 AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = xsw0-p1]) +wait_for_ports_up sw0-vir + ovn-sbctl dump-flows lr0 > lr0-flows5 AT_CAPTURE_FILE([lr0-flows5]) AT_CHECK([grep lr_in_arp_resolve lr0-flows5 | grep "reg0 == 10.0.0.10" | sed 's/table=../table=??/'], [0], [dnl @@ -16717,6 +16731,8 @@ sleep 1 AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = x]) +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-vir + # Since the sw0-vir is not claimed by any chassis, eth.dst should be set to # zero if the ip4.dst is the virtual ip. ovn-sbctl dump-flows lr0 > lr0-flows6 @@ -16740,6 +16756,8 @@ sleep 1 AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = xsw0-p2]) +wait_for_ports_up sw0-vir + ovn-sbctl dump-flows lr0 > lr0-flows7 AT_CAPTURE_FILE([lr0-flows7]) AT_CHECK([grep lr_in_arp_resolve lr0-flows7 | grep "reg0 == 10.0.0.10" | sed 's/table=../table=??/'], [0], [dnl @@ -16754,6 +16772,8 @@ logical_port=sw0-vir) = x], [0], []) AT_CHECK([test x$(ovn-sbctl --bare --columns virtual_parent find port_binding \ logical_port=sw0-vir) = x]) +wait_row_count nb:Logical_Switch_Port 1 up=false name=sw0-vir + # Clear virtual_ip column of sw0-vir. There should be no bind_vport flows. ovn-nbctl --wait=hv remove logical_switch_port sw0-vir options virtual-ip -- 1.8.3.1