On 7/15/25 11:22 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>
> ---
> v2: Fixed memory leak by freeing op_smap and tokstr.
> ---
> northd/northd.c | 32 ++++++++++++++++++
> tests/ovn.at | 90 +++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 122 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index b8ca445bc..21e51453d 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,34 @@ 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", ""));
Hmm, we're iterating over the list of existing virtual ports and just
putting them into smap. Looks strange.
> + 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) {
This is always true, because it the condition of the loop already.
We should be looking up this virtual parent in the map of all the
ports created on this iteration instead, and marking the port as
updated only if it is there.
> + add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
> + break;
> + }
> + }
> + free(tokstr);
> + }
> + }
In practice, it seems like this nested loop is just marking all the existing
virtual ports as updated a few times.
The original suggestion here was to create an smap or even an sset (because
we don't need anything, except for the names) of the names of all the
'created ports'. Then iterate over all the 'existing virtual ports' and
check if their virtual parents were created on this iteration by looking up
in the sset.
I'm not sure, but we may also want to check for parents deleted on this
iteration, otherwise the code seems a little incomplete.
> +
> + smap_destroy(&op_smap);
> +
> 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
> +}
We have this function now in the tests/ovn-macros.at, no need to re-define.
> +
> +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"`
nit: Maybe use $() instead of ``, for consistency throughout the test.
> +
> +# 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`])
nit: Might be better to re-write like this (not tested):
OVS_WAIT_FOR_OUTPUT([dnl
grep "pinctrl received packet-in" hv1/ovn-controller.log | \
grep opcode=BIND_VPORT | \
grep OF_Table_ID=$(ovn-debug lflow-stage-to-oftable ls_in_arp_rsp)], [0], [dnl
1
])
> +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
> +
Should probably wait here for the port binding to go away.
> +# 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`])
Same here.
> +
> +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
