On 7/11/25 4:56 AM, Dumitru Ceara wrote: > On 7/10/25 7:46 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. >> >> 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. >> >> 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> >> --- > > Hi Rosemarie, > > Thanks for the patch!
Thank you for the review! > > I didn't get a chance to review it yet but it seems it introduces a > memory leak. Please see the CI output here: > > https://github.com/ovsrobot/ovn/actions/runs/16202471658/job/45754556600#step:12:5607 Thank you for making me aware of this. I saw that some tests were failing but I thought it was a fluke because of the final error message: + echo '*** ERROR: 1 ***' + podman rm -f 445906a9d9d9d8265c03ea9fdfb7032fc409857522e061ac6d957184a75032ff 445906a9d9d9d8265c03ea9fdfb7032fc409857522e061ac6d957184a75032ff Error: Process completed with exit code 1. I think this sometimes indicates a random CI failure, that will fix itself upon rerun. So I missed the very valid reason for failure this time :) > > I'll move the series to changes requested in patchwork for now, it might > be better to have green CI before we fully review this. Ack! > > Regards, > Dumitru > >> northd/northd.c | 29 ++++++++++++++++ >> tests/ovn.at | 90 +++++++++++++++++++++++++++++++++++++++++++++++++ >> 2 files changed, 119 insertions(+) >> >> diff --git a/northd/northd.c b/northd/northd.c >> index 052e15c2a..6cb1b5c24 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -4990,6 +4990,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; >> + ovs_list_init(&exist_virtual_ports); >> HMAP_FOR_EACH (op, dp_node, &od->ports) { >> op->visited = false; >> } >> @@ -5059,6 +5061,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; >> } >> @@ -5105,6 +5109,31 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn >> *ovnsb_idl_txn, >> } >> } >> >> + /* >> + * Update old virtual ports that have newly created 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 smap op_smap = SMAP_INITIALIZER(&op_smap); >> + >> + LIST_FOR_EACH_POP (op, list, &exist_virtual_ports) { >> + smap_add(&op_smap, op->nbsp->name, >> + smap_get_def(&op->nbsp->options, "virtual-parents", "")); >> + struct smap_node *node; >> + SMAP_FOR_EACH (node, &op_smap) { >> + char *tokstr = xstrdup(node->value); >> + char *save_ptr = NULL; >> + char *vparent; >> + for (vparent = strtok_r(tokstr, ",", &save_ptr); vparent != >> NULL; >> + vparent = strtok_r(NULL, ",", &save_ptr)) { >> + if (vparent) { >> + add_op_to_northd_tracked_ports(&trk_lsps->updated, op); >> + break; >> + } >> + } >> + } >> + } >> + >> return true; >> >> fail: >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 108de260e..28fc53cdc 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -43472,3 +43472,93 @@ 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 - parents port re-create]) >> +AT_KEYWORDS([virtual ports]) >> +ovn_start >> +send_garp() { >> + local hv=$1 inport=$2 eth_src=$3 eth_dst=$4 spa=$5 tpa=$6 >> + local >> request=${eth_dst}${eth_src}08060001080006040001${eth_src}${spa}${eth_dst}${tpa} >> + as hv$hv ovs-appctl netdev-dummy/receive hv${hv}-vif$inport $request >> +} >> + >> +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 >> + >> +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=505400000003 >> +eth_dst=ffffffffffff >> +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 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 >> + >> +# 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. >> +eth_src=505400000003 >> +eth_dst=ffffffffffff >> +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 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 >> +]) > -- Rosemarie O'Riorden rosema...@redhat.com _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
