On 12/4/25 6:16 PM, Xavier Simonart via dev wrote:
> If a Logical Switch Port was added/updated and its Logical Switch deleted
> within the same northd iteration, ovn-northd was asserting [0].
>
> [0] lib/ovsdb-idl.c:3668: assertion row->new_datum != NULL failed in
> ovsdb_idl_txn_write__()
>
> Stack trace:
> 6 ovs_assert_failure lib/util.c:90
> 7 ovsdb_idl_txn_write__ lib/ovsdb-idl.c:3668
> 8 ovsdb_idl_txn_write__ lib/ovsdb-idl.c:3668
> 9 sbrec_port_binding_set_nat_addresses lib/ovn-sb-idl.c:40588
> 10 sync_pbs_for_northd_changed_ovn_ports northd/northd.c:3959
> 11 sync_to_sb_pb_northd_handler northd/en-sync-sb.c:420
> 12 engine_compute lib/inc-proc-eng.c:478
> 13 engine_run_node lib/inc-proc-eng.c:550
> 14 engine_run lib/inc-proc-eng.c:579
> 15 inc_proc_northd_run northd/inc-proc-northd.c:608
> 16 main northd/ovn-northd.c:1134
>
> Fixes: 06e2c1bf0ce7 ("northd: I-P for logical switch creation/deletion in
> en_northd.")
> Signed-off-by: Xavier Simonart <[email protected]>
> ---
Hi Xavier,
Thanks for the patch!
> northd/northd.c | 5 +++++
> tests/ovn-northd.at | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 38 insertions(+)
>
> diff --git a/northd/northd.c b/northd/northd.c
> index a8d435be8..1f5cb7830 100644
> --- a/northd/northd.c
> +++ b/northd/northd.c
> @@ -3955,6 +3955,11 @@ sync_pbs_for_northd_changed_ovn_ports(
> {
> struct hmapx_node *hmapx_node;
>
> + HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->deleted) {
> + struct ovn_port *op = (struct ovn_port *) hmapx_node->data;
> + hmapx_find_and_delete(&trk_ovn_ports->created, op);
> + hmapx_find_and_delete(&trk_ovn_ports->updated, op);
> + }
This breaks assumptions of the I-P engine.
This is a handler for the en_northd -> en_sync_to_sb_pb data dependency,
running for the en_sync_to_sb_pb node. And trk_ovn_ports is actually
data populated by the en_northd node, so "input data" for en_sync_to_sb_pb.
Change handlers should not change input data. Maybe we should also
change engine_get_input_data() to return const void * in the future.
It does sound though that the actual problem is that we have another
case of record (Logical_Switch_Port) being created/updated+deleted in
multiple transactions that happen to be processed in the same iteration
of ovn-northd.
Discussing with Ales, the actual root cause seems to be in
ls_handle_lsp_changes().
There, if "ls_deleted == true" we shouldn't be adding any of the switch
ports to the tracked updated/created maps (the calls to
add_op_to_northd_tracked_ports()).
If I skip this whole for loop when ls_deleted == true:
https://github.com/ovn-org/ovn/blob/53057d35a5d1f50f9a96ae14e70ea1d80cff5a72/northd/northd.c#L4602-L4671
Then your test passes as well (with the change handler modification
reverted).
Isn't that the correct way of fixing the problem?
Side rant: ls_handle_lsp_changes() is horrible, we should improve it in
the near future.
> HMAPX_FOR_EACH (hmapx_node, &trk_ovn_ports->created) {
> sync_pb_for_lsp(hmapx_node->data, lr_stateful_table);
> }
> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
> index b9bd291b7..ffbf704c0 100644
> --- a/tests/ovn-northd.at
> +++ b/tests/ovn-northd.at
> @@ -18972,3 +18972,36 @@ check ovn-nbctl --wait=sb sync
> OVN_CLEANUP_NORTHD
> AT_CLEANUP
> ])
> +
> +OVN_FOR_EACH_NORTHD_NO_HV([
> +AT_SETUP([LS/LSP add and delete])
> +
> +ovn_start
> +
> +check ovn-nbctl --wait=sb ls-add ls1
> +
> +sleep_northd
> +AS_BOX([$(date +%H:%M:%S.%03N) Add/delete LSP while northd is sleeping])
Do we really need the date in the output? Is this a left over from your
debugging?
> +check ovn-nbctl lsp-add ls1 lsp1
> +check ovn-nbctl lsp-del lsp1
> +check ovn-nbctl lsp-add ls1 lsp1
> +wake_up_northd
> +check ovn-nbctl --wait=sb sync
> +
> +sleep_northd
> +AS_BOX([$(date +%H:%M:%S.%03N) Update/delete LS while northd is sleeping])
> +check ovn-nbctl lsp-set-addresses lsp1 "50:54:00:00:00:03 11.0.0.2"
> +check ovn-nbctl ls-del ls1
> +wake_up_northd
> +
> +check ovn-nbctl --wait=sb ls-add ls1
> +sleep_northd
> +AS_BOX([$(date +%H:%M:%S.%03N) Add/delete LS while northd is sleeping])
> +check ovn-nbctl lsp-add ls1 lsp1
> +check ovn-nbctl ls-del ls1
> +wake_up_northd
> +check ovn-nbctl --wait=sb sync
> +
> +OVN_CLEANUP_NORTHD
> +AT_CLEANUP
> +])
Regards,
Dumitru
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
