On 8/8/25 11:54 PM, Rosemarie O'Riorden wrote: > Previously, if there was a logical switch with a virtual port that had a > virtual parent port configured, and then the parent port was deleted and > recreated, the parent port would not be reclaimed by the chassis and > thus would not behave as the parent port, breaking traffic. > > This is because virtual ports were not being updated if their parents > were recently created or deleted. > > So long as virtual ports have a parent port configured, there should be > no issue deleting and recreating the parent. It should continue working > as the parent as soon as it's up again. > > With this change, the parent port will function as the parent even after > deletion and recreation. The chassis will properly reclaim the virtual > port.
Hi, Rosemarie. Thanks for the update! This version seems correct to me, see a few small comments below. Best regards, Ilya Maximets. > > Reported-at: https://issues.redhat.com/browse/FDP-710 > Co-authored-by: Mohammad Heib <mh...@redhat.com> > Signed-off-by: Mohammad Heib <mh...@redhat.com> > Signed-off-by: Rosemarie O'Riorden <rosema...@redhat.com> IIUC, the following Fixes tag may be good to add here: Fixes: b337750e45be ("northd: Incremental processing of VIF changes in 'northd' node.") > --- > v3: > - Fixed algorithm to use an sset and loop through created ports, only > updating them if their virtual parent was recently created or > deleted. > - Added case for deleted parents, as just mentioned. > - Fixed send_garp() syntax in test and remove redefinition. > - Added wait for port binding to disappear in test. > - Added more virtual parents to test. > - Fixed various syntax issues/nits. > > v2: Fixed memory leak by freeing op_smap and tokstr. > --- > northd/northd.c | 49 +++++++++++++++++++++++++++ > tests/ovn.at | 89 +++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 138 insertions(+) > > diff --git a/northd/northd.c b/northd/northd.c > index 2cb69f9aa..daaa89a3c 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -4560,6 +4560,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > && (vector_len(&od->router_ports) == hmap_count(&od->ports))); > > struct ovn_port *op; > + struct ovs_list exist_virtual_ports; nit: I'd suggest to re-order the above two lines and add an empty one here. Also, 'exist ports' doesn't seem like a proper English, maybe 'existing'? > + ovs_list_init(&exist_virtual_ports); > HMAP_FOR_EACH (op, dp_node, &od->ports) { > op->visited = false; > } > @@ -4629,6 +4631,8 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > delete_fdb_entries(ni->sbrec_fdb_by_dp_and_port, > od->tunnel_key, old_tunnel_key); > } > + } else if (!strcmp(op->nbsp->type, "virtual")) { > + ovs_list_push_back(&exist_virtual_ports, &op->list); > } > op->visited = true; > } > @@ -4675,6 +4679,51 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn > *ovnsb_idl_txn, > } > } > > + /* Update old virtual ports that have newly created or newly deleted > + * VIF as parent port. This code handles cases where the virtual port was > + * created before the parent port or when the parent port was recreated. > + */ > + struct hmapx_node *hmapx_node; > + > + struct sset created_ports; > + sset_init(&created_ports); > + > + struct sset deleted_ports; > + sset_init(&deleted_ports); We don't really need two sets here, may just add all the names to the same set, e.g. 'created_or_deleted_ports'. > + > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->created) { > + op = hmapx_node->data; > + sset_add(&created_ports, op->nbsp->name); > + } > + > + HMAPX_FOR_EACH (hmapx_node, &trk_lsps->deleted) { > + op = hmapx_node->data; > + sset_add(&deleted_ports, op->nbsp->name); > + } > + > + LIST_FOR_EACH_POP (op, list, &exist_virtual_ports) { We can save a few cycles here by moving the loop under the if (!sset_is_empty(...)) condition. There is no need to iterate over all the existing ports again in this case. Note: there is no real need to cleanup the list in this case. > + const char *virtual_parents = > + smap_get_def(&op->nbsp->options, "virtual-parents", ""); > + char *tokstr = xstrdup(virtual_parents); > + char *save_ptr = NULL; > + char *vparent; nit: I'd add an empty line here. > + for (vparent = strtok_r(tokstr, ",", &save_ptr); vparent != NULL; > + vparent = strtok_r(NULL, ",", &save_ptr)) { > + if (sset_find(&created_ports, vparent)) { > + add_op_to_northd_tracked_ports(&trk_lsps->updated, op); > + break; > + } > + if (sset_find(&deleted_ports, vparent)) { > + add_op_to_northd_tracked_ports(&trk_lsps->updated, op); > + break; > + } > + } > + free(tokstr); > + } > + > + sset_destroy(&created_ports); > + sset_destroy(&deleted_ports); > + > return true; > > fail: > diff --git a/tests/ovn.at b/tests/ovn.at > index fadc62a76..44226a7ac 100644 > --- a/tests/ovn.at > +++ b/tests/ovn.at > @@ -43651,3 +43651,92 @@ AT_CHECK([grep -q "Postponing virtual lport sw0-vir" > hv3/ovn-controller.log], [1 > OVN_CLEANUP([hv1]) > AT_CLEANUP > ]) > + > +OVN_FOR_EACH_NORTHD([ > +AT_SETUP([virtual port - parent port re-create]) > +AT_KEYWORDS([virtual ports]) > +ovn_start > + > +net_add n1 > + > +sim_add hv1 > +as hv1 > +check ovs-vsctl add-br br-phys > +ovn_attach n1 br-phys 192.168.0.1 > +check ovn-appctl vlog/set dbg > +check ovs-vsctl -- add-port br-int hv1-vif1 -- \ > + set interface hv1-vif1 external-ids:iface-id=sw0-p1 \ > + options:tx_pcap=hv1/vif1-tx.pcap \ > + options:rxq_pcap=hv1/vif1-rx.pcap \ > + ofport-request=1 > + > +check ovs-appctl -t ovn-controller vlog/set dbg > + > +check ovn-nbctl ls-add sw0 > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 > 10.0.0.10 1000::3" > + > +check ovn-nbctl lsp-add sw0 sw0-vir > +check ovn-nbctl lsp-set-addresses sw0-vir "50:54:00:00:00:10 10.0.0.10" > +check ovn-nbctl lsp-set-port-security sw0-vir "50:54:00:00:00:10 10.0.0.10" > +check ovn-nbctl lsp-set-type sw0-vir virtual > +check ovn-nbctl set logical_switch_port sw0-vir options:virtual-ip=10.0.0.10 > +check ovn-nbctl set logical_switch_port sw0-vir > options:virtual-parents=sw0-p1,sw0-p2,sw0-p3,sw0-p4 > + > +wait_for_ports_up > +check ovn-nbctl --wait=hv sync > +hv1_ch_uuid=$(ovn-sbctl --bare --columns _uuid find chassis name="hv1") > + > +# From sw0-p1 send GARP for 10.0.0.10. hv1 should claim sw0-vir > +# and sw0-p1 should be its virtual_parent. > +eth_src=50:54:00:00:00:03 > +eth_dst=ff:ff:ff:ff:ff:ff > +spa=10.0.0.10 > +tpa=10.0.0.10 > +send_garp hv1 hv1-vif1 1 $eth_src $eth_dst $spa $tpa > + > +OVS_WAIT_UNTIL([test 1 = $(cat hv1/ovn-controller.log | \ > + grep "pinctrl received packet-in" | \ > + grep opcode=BIND_VPORT | \ > + grep OF_Table_ID=$(ovn-debug lflow-stage-to-oftable ls_in_arp_rsp) | \ > + wc -l)]) > + > +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 > + > +# Delete parent port. > +check ovn-nbctl lsp-del sw0-p1 > +check ovn-nbctl --wait=hv sync > +wait_row_count Port_Binding 0 logical_port=sw0-vir chassis=$hv1_ch_uuid > + > +# Re-add parent port. > +check ovn-nbctl lsp-add sw0 sw0-p1 > +check ovn-nbctl lsp-set-addresses sw0-p1 "50:54:00:00:00:03 10.0.0.3 1000::3" > +check ovn-nbctl lsp-set-port-security sw0-p1 "50:54:00:00:00:03 10.0.0.3 > 10.0.0.10 1000::3" > + > +wait_for_ports_up > + > +# Must sleep for vport-backoff duration, otherwise ovn-controller will > +# increase the backoff and not claim the port. > +sleep_time=$(ovn-sbctl get Port_Binding sw0-vir options:vport-backoff) > +sleep $(echo "$sleep_time" | tr -d '"' | awk '{print $1/1000}') > + > +# From sw0-p1 send GARP for 10.0.0.10. hv1 should claim sw0-vir > +# and sw0-p1 should be its virtual_parent. > +send_garp hv1 hv1-vif1 1 $eth_src $eth_dst $spa $tpa > + > +OVS_WAIT_UNTIL([test 2 = $(cat hv1/ovn-controller.log | \ > + grep "pinctrl received packet-in" | \ > + grep opcode=BIND_VPORT | \ > + grep OF_Table_ID=$(ovn-debug lflow-stage-to-oftable ls_in_arp_rsp) | \ > + wc -l)]) > + > +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 > + > +OVN_CLEANUP([hv1]) > +AT_CLEANUP > +]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
