When a logical switch is created or updated, lflow_handle_northd_ls_changes() already builds and syncs that switch's ls_stateful flows together with its by_ls flows (so the shared datapath groups stay stable). The ls_stateful record for the same switch also shows up in the ls_stateful node's tracked "crupdated" set, so lflow_ls_stateful_handler() would rebuild and resync those exact flows a second time.
Pass northd's tracked switches down to lflow_handle_ls_stateful_changes() and skip any crupdated ls_stateful record whose switch datapath was already handled by lflow_handle_northd_ls_changes(). Records for pre-existing switches (e.g. an ACL or port-group change) are untouched and still processed here. Deleted records need no such skip. An ls_stateful record is deleted only along with its switch datapath, so lflow_handle_northd_ls_changes() has already unlinked and synced its lflow_ref, and syncing destroys the ref nodes that were left unlinked. The resync done here therefore walks an already empty ref. Add a test covering the switches on both sides of the skip: a switch created together with an ACL (skipped here), an ACL change on a pre-existing switch (still processed here), a single transaction doing both at once, and the deletion of a switch that has ACLs. The transaction doing both at once is what pins the skip down to the individual switch: skipping every crupdated record instead drops the pre-existing switch's ACL flows and fails the test. Assisted-by: Claude Opus 4.8, Claude Opus 5, Claude Code Signed-off-by: Lucas Vargas Dias <[email protected]> --- northd/en-lflow.c | 9 ++++++ northd/northd.c | 25 ++++++++++++++++ northd/northd.h | 1 + tests/ovn-northd.at | 73 +++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 108 insertions(+) diff --git a/northd/en-lflow.c b/northd/en-lflow.c index 342766713..a1caf8d79 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -236,6 +236,14 @@ lflow_ls_stateful_handler(struct engine_node *node, void *data) return EN_UNHANDLED; } + /* Switch datapaths created/updated in this run had their ls_stateful + * flows already handled by lflow_handle_northd_ls_changes(); pass northd's + * tracked switches so we don't reprocess them here. */ + struct northd_data *northd_data = engine_get_input_data("northd", node); + const struct tracked_dps *trk_switches = + northd_has_lswitches_in_tracked_data(&northd_data->trk_data) + ? &northd_data->trk_data.trk_switches : NULL; + const struct engine_context *eng_ctx = engine_get_context(); struct lflow_data *lflow_data = data; struct lflow_input lflow_input; @@ -243,6 +251,7 @@ lflow_ls_stateful_handler(struct engine_node *node, void *data) lflow_get_input_data(node, &lflow_input); if (!lflow_handle_ls_stateful_changes(eng_ctx->ovnsb_idl_txn, &ls_sful_data->trk_data, + trk_switches, &lflow_input, lflow_data->lflow_table)) { return EN_UNHANDLED; diff --git a/northd/northd.c b/northd/northd.c index d296621c3..919b6b48e 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -22583,6 +22583,7 @@ exit: bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, struct ls_stateful_tracked_data *trk_data, + const struct tracked_dps *trk_switches, struct lflow_input *lflow_input, struct lflow_table *lflows) { @@ -22596,6 +22597,15 @@ lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, ovs_assert(od->nbs && uuid_equals(&od->nbs->header_.uuid, &ls_stateful_rec->nbs_uuid)); + /* Newly created/updated switch datapaths already had their + * ls_stateful flows built and synced by + * lflow_handle_northd_ls_changes() (which processes both the by_ls + * and ls_stateful refs together to keep shared datapath groups + * stable). Skip them here to avoid rebuilding the same flows. */ + if (trk_switches && hmapx_contains(&trk_switches->crupdated, od)) { + continue; + } + lflow_ref_unlink_lflows(ls_stateful_rec->lflow_ref, lflows); /* Generate new lflows. */ @@ -22613,6 +22623,17 @@ lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, * those datapath groups within those flows over and over again. */ HMAPX_FOR_EACH (hmapx_node, &trk_data->crupdated) { struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; + + /* Already synced by lflow_handle_northd_ls_changes() (see above). */ + if (trk_switches) { + const struct ovn_datapath *od = + ovn_datapaths_find_by_index(lflow_input->ls_datapaths, + ls_stateful_rec->ls_index); + if (hmapx_contains(&trk_switches->crupdated, od)) { + continue; + } + } + /* Sync the new flows to SB. */ bool handled = lflow_ref_sync_lflows( ls_stateful_rec->lflow_ref, lflows, ovnsb_txn, @@ -22625,6 +22646,10 @@ lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *ovnsb_txn, } } + /* No skip is needed for deleted records: an ls_stateful record is deleted + * only along with its switch datapath, so lflow_handle_northd_ls_changes() + * has already unlinked and synced its lflow_ref, and syncing destroys the + * unlinked ref nodes. The resync below therefore walks an empty ref. */ HMAPX_FOR_EACH (hmapx_node, &trk_data->deleted) { struct ls_stateful_record *ls_stateful_rec = hmapx_node->data; diff --git a/northd/northd.h b/northd/northd.h index 7facf8d76..816f886b1 100644 --- a/northd/northd.h +++ b/northd/northd.h @@ -1026,6 +1026,7 @@ bool lflow_handle_lr_stateful_changes(struct ovsdb_idl_txn *, struct lflow_table *lflows); bool lflow_handle_ls_stateful_changes(struct ovsdb_idl_txn *, struct ls_stateful_tracked_data *, + const struct tracked_dps *trk_switches, struct lflow_input *, struct lflow_table *lflows); bool northd_handle_sb_port_binding_changes( diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index cc7249f05..8e919e3e9 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -16804,6 +16804,79 @@ OVN_CLEANUP_NORTHD AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([ls_stateful lflow handling for created switches]) +AT_KEYWORDS([incremental processing]) + +ovn_start + +# A pre-existing switch with an ACL. It is not part of northd's tracked +# switches, so its ls_stateful record is processed by +# lflow_handle_ls_stateful_changes(). +check ovn-nbctl ls-add sw0 +check ovn-nbctl --wait=sb acl-add sw0 to-lport 1002 'ip4.src == 10.0.0.1' \ + allow-related + +# Creating a switch together with an ACL: the switch is in northd's tracked +# switches, so lflow_handle_northd_ls_changes() builds and syncs its +# ls_stateful flows, and the ls_stateful handler skips the record instead of +# building the very same flows a second time. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-add sw1 \ + -- acl-add sw1 to-lport 1002 'ip4.src == 10.0.0.2' allow-related +check_engine_stats northd norecompute compute +check_engine_stats ls_stateful norecompute compute +check_engine_stats lflow norecompute compute +AT_CHECK([ovn-sbctl dump-flows sw1 | grep ls_out_acl_eval | \ + grep -c 'ip4.src == 10.0.0.2'], [0], [2 +]) +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# The skip is per switch. A single transaction that both creates a switch +# with an ACL and adds an ACL to the pre-existing switch must skip only the +# created one: sw0's record is still processed here. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-add sw2 \ + -- acl-add sw2 to-lport 1002 'ip4.src == 10.0.0.3' allow-related \ + -- acl-add sw0 to-lport 1003 'ip4.src == 10.0.0.4' drop +check_engine_stats ls_stateful norecompute compute +check_engine_stats lflow norecompute compute +AT_CHECK([ovn-sbctl dump-flows sw2 | grep ls_out_acl_eval | \ + grep -c 'ip4.src == 10.0.0.3'], [0], [2 +]) +AT_CHECK([ovn-sbctl dump-flows sw0 | grep ls_out_acl_eval | \ + grep -c 'ip4.src == 10.0.0.4'], [0], [2 +]) +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# An ACL change on a switch northd did not track goes through the ls_stateful +# handler unchanged. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb acl-del sw0 to-lport 1003 'ip4.src == 10.0.0.4' +check_engine_stats ls_stateful norecompute compute +check_engine_stats lflow norecompute compute +AT_CHECK([ovn-sbctl dump-flows sw0 | grep -c 'ip4.src == 10.0.0.4'], [1], [0 +]) +AT_CHECK([ovn-sbctl dump-flows sw0 | grep ls_out_acl_eval | \ + grep -c 'ip4.src == 10.0.0.1'], [0], [2 +]) +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +# Deleting a switch that has ACLs: its ls_stateful record is deleted along +# with the datapath, whose lflow_ref lflow_handle_northd_ls_changes() has +# already unlinked and synced, so the deleted-record path walks an empty ref. +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats +check ovn-nbctl --wait=sb ls-del sw2 +check_engine_stats ls_stateful norecompute compute +check_engine_stats lflow norecompute compute +AT_CHECK([ovn-sbctl lflow-list | grep -c 'ip4.src == 10.0.0.3'], [1], [0 +]) +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +OVN_CLEANUP_NORTHD +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD_NO_HV([ AT_SETUP([Datapath count change with shared dp-group flow]) AT_KEYWORDS([incremental processing]) -- 2.43.0 -- _'Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas'._ * **'Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos'.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
