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! 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 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. 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 > +]) _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
