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]>
---
-v2: Updated based on Dumitru and Ilya's feedback i.e.
-- Avoid change handlers changing input data.
-- Do not use date +...%03N in tests as not portable and requiring GNU
date.
---
northd/northd.c | 127 +++++++++++++++++++++++---------------------
tests/ovn-northd.at | 30 +++++++++++
2 files changed, 95 insertions(+), 62 deletions(-)
diff --git a/northd/northd.c b/northd/northd.c
index d99aec0b8..8048a5d44 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -4599,75 +4599,78 @@ ls_handle_lsp_changes(struct ovsdb_idl_txn
*ovnsb_idl_txn,
op->visited = false;
}
- /* Compare the individual ports in the old and new Logical Switches */
- for (size_t j = 0; j < changed_ls->n_ports; ++j) {
- struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
- op = ovn_port_find_in_datapath(od, new_nbsp);
+ /* If ls is deleted, no need to update/create ports */
+ if (!ls_deleted) {
+ /* Compare the individual ports in the old and new Logical Switches */
+ for (size_t j = 0; j < changed_ls->n_ports; ++j) {
+ struct nbrec_logical_switch_port *new_nbsp = changed_ls->ports[j];
+ op = ovn_port_find_in_datapath(od, new_nbsp);
- if (!op) {
- if (!lsp_can_be_inc_processed(new_nbsp)) {
- goto fail;
- }
- op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
- new_nbsp->name, new_nbsp, od,
- ni->sbrec_mirror_table,
- ni->sbrec_chassis_by_name,
- ni->sbrec_chassis_by_hostname);
if (!op) {
- goto fail;
- }
- add_op_to_northd_tracked_ports(&trk_lsps->created, op);
- } else if (ls_port_has_changed(new_nbsp)) {
- /* Existing port updated */
- bool temp = false;
- if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
- !op->lsp_can_be_inc_processed ||
- !lsp_can_be_inc_processed(new_nbsp)) {
- goto fail;
- }
- const struct sbrec_port_binding *sb = op->sb;
- if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
- /* This port is used for svc monitor, which may be impacted
- * by this change. Fallback to recompute. */
- goto fail;
- }
- if (!lsp_handle_mirror_rules_changes(op) ||
- is_lsp_mirror_target_port(ni->nbrec_mirror_by_type_and_sink,
- op)) {
- /* Fallback to recompute. */
- goto fail;
- }
- if (!check_lsp_is_up &&
- !check_lsp_changes_other_than_up(new_nbsp)) {
- /* If the only change is the "up" column while the
- * "ignore_lsp_down" is set to true, just ignore this
- * change. */
- op->visited = true;
- continue;
- }
+ if (!lsp_can_be_inc_processed(new_nbsp)) {
+ goto fail;
+ }
+ op = ls_port_create(ovnsb_idl_txn, &nd->ls_ports,
+ new_nbsp->name, new_nbsp, od,
+ ni->sbrec_mirror_table,
+ ni->sbrec_chassis_by_name,
+ ni->sbrec_chassis_by_hostname);
+ if (!op) {
+ goto fail;
+ }
+ add_op_to_northd_tracked_ports(&trk_lsps->created, op);
+ } else if (ls_port_has_changed(new_nbsp)) {
+ /* Existing port updated */
+ bool temp = false;
+ if (lsp_is_type_changed(op->sb, new_nbsp, &temp) ||
+ !op->lsp_can_be_inc_processed ||
+ !lsp_can_be_inc_processed(new_nbsp)) {
+ goto fail;
+ }
+ const struct sbrec_port_binding *sb = op->sb;
+ if (sset_contains(&nd->svc_monitor_lsps, new_nbsp->name)) {
+ /* This port is used for svc monitor, which may be impacted
+ * by this change. Fallback to recompute. */
+ goto fail;
+ }
+ if (!lsp_handle_mirror_rules_changes(op) ||
+ is_lsp_mirror_target_port(
+ ni->nbrec_mirror_by_type_and_sink, op)) {
+ /* Fallback to recompute. */
+ goto fail;
+ }
+ if (!check_lsp_is_up &&
+ !check_lsp_changes_other_than_up(new_nbsp)) {
+ /* If the only change is the "up" column while the
+ * "ignore_lsp_down" is set to true, just ignore this
+ * change. */
+ op->visited = true;
+ continue;
+ }
- uint32_t old_tunnel_key = op->tunnel_key;
- if (!ls_port_reinit(op, ovnsb_idl_txn,
- new_nbsp,
- od, sb, ni->sbrec_mirror_table,
- ni->sbrec_chassis_by_name,
- ni->sbrec_chassis_by_hostname)) {
- if (sb) {
- sbrec_port_binding_delete(sb);
+ uint32_t old_tunnel_key = op->tunnel_key;
+ if (!ls_port_reinit(op, ovnsb_idl_txn,
+ new_nbsp,
+ od, sb, ni->sbrec_mirror_table,
+ ni->sbrec_chassis_by_name,
+ ni->sbrec_chassis_by_hostname)) {
+ if (sb) {
+ sbrec_port_binding_delete(sb);
+ }
+ ovn_port_destroy(&nd->ls_ports, op);
+ goto fail;
}
- ovn_port_destroy(&nd->ls_ports, op);
- goto fail;
- }
- add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
+ add_op_to_northd_tracked_ports(&trk_lsps->updated, op);
- if (old_tunnel_key != op->tunnel_key) {
- delete_fdb_entries(ni->sbrec_fdb_by_dp_and_port,
- od->tunnel_key, old_tunnel_key);
+ if (old_tunnel_key != op->tunnel_key) {
+ 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(&existing_virtual_ports, &op->list);
}
- } else if (!strcmp(op->nbsp->type, "virtual")) {
- ovs_list_push_back(&existing_virtual_ports, &op->list);
+ op->visited = true;
}
- op->visited = !ls_deleted;
}
/* Check for deleted ports */
diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at
index b9bd291b7..09fbcebbc 100644
--- a/tests/ovn-northd.at
+++ b/tests/ovn-northd.at
@@ -18972,3 +18972,33 @@ 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
+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
+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
+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
+])
--
2.47.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
