On Thu, Jul 31, 2025 at 10:13 PM Jacob Tanenbaum <jtane...@redhat.com> wrote:
> > > On Thu, Jul 31, 2025 at 3:06 AM Ales Musil <amu...@redhat.com> wrote: > >> >> >> On Mon, Jul 28, 2025 at 5:42 PM Jacob Tanenbaum via dev < >> ovs-dev@openvswitch.org> wrote: >> >>> Several operations on Port Groups required a full database >>> recalculation. >>> >>> 1. Creating a new Port Group >>> 2. Deleting a Port Group >>> 3. adding a port from a switch with no other ports on a port group >>> 4. removing a port from a port group that is the last one from a >>> specific switch >>> >>> Those four operations required a full database recalculation to ensure >>> that that the lflows from the ACLs and the southbound databases were >>> correctly populated. Instead reprocess only the relevant switches and >>> ACLs to avoid the cost of a full recalculation. This is done by having >>> the port_group_handler update the southbound database and pass the names >>> of the relevant switches to the ls_stateful_acl node. >>> >>> Tested using the ovn sandbox 3000 switches and 15000 ports, Creating a >>> port group with one ACL and one port >>> Without I-P improvements: >>> >>> Time spent on processing nb_cfg 2: >>> ovn-northd delay before processing: 1ms >>> ovn-northd completion: 132ms >>> >>> With I-P improvements: >>> >>> Time spent on processing nb_cfg 5: >>> ovn-northd delay before processing: 0ms >>> ovn-northd completion: 26ms >>> >>> Reported-at: https://issues.redhat.com/browse/FDP-758 >>> Reported-by: Dumitru Ceara <dce...@redhat.com> >>> Signed-off-by: Jacob Tanenbaum <jtane...@redhat.com> >>> >> >> Hello Jacob, >> >> I have a couple of comments on top of what Lorenzo already wrote. >> >> >>> >>> diff --git a/northd/en-lflow.c b/northd/en-lflow.c >>> index 63565ef80..ac8ebba16 100644 >>> --- a/northd/en-lflow.c >>> +++ b/northd/en-lflow.c >>> @@ -157,22 +157,6 @@ lflow_northd_handler(struct engine_node *node, >>> return EN_HANDLED_UPDATED; >>> } >>> >>> -enum engine_input_handler_result >>> -lflow_port_group_handler(struct engine_node *node, void *data >>> OVS_UNUSED) >>> -{ >>> - struct port_group_data *pg_data = >>> - engine_get_input_data("port_group", node); >>> - >>> - /* If the set of switches per port group didn't change then there's >>> no >>> - * need to reprocess lflows. Otherwise, there might be a need to >>> - * add/delete port-group ACLs to/from switches. */ >>> - if (pg_data->ls_port_groups_sets_changed) { >>> - return EN_UNHANDLED; >>> - } >>> - >>> - return EN_HANDLED_UPDATED; >>> -} >>> - >>> enum engine_input_handler_result >>> lflow_lr_stateful_handler(struct engine_node *node, void *data) >>> { >>> diff --git a/northd/en-lflow.h b/northd/en-lflow.h >>> index d3c96c027..326be83f9 100644 >>> --- a/northd/en-lflow.h >>> +++ b/northd/en-lflow.h >>> @@ -20,8 +20,6 @@ void *en_lflow_init(struct engine_node *node, struct >>> engine_arg *arg); >>> void en_lflow_cleanup(void *data); >>> enum engine_input_handler_result lflow_northd_handler(struct >>> engine_node *, >>> void *data); >>> -enum engine_input_handler_result lflow_port_group_handler(struct >>> engine_node *, >>> - void *data); >>> enum engine_input_handler_result >>> lflow_lr_stateful_handler(struct engine_node *, void *data); >>> enum engine_input_handler_result >>> diff --git a/northd/en-ls-stateful.c b/northd/en-ls-stateful.c >>> index 6aec43415..7b2a3e065 100644 >>> --- a/northd/en-ls-stateful.c >>> +++ b/northd/en-ls-stateful.c >>> @@ -159,11 +159,15 @@ ls_stateful_northd_handler(struct engine_node >>> *node, void *data_) >>> struct ls_stateful_record *ls_stateful_rec = >>> ls_stateful_table_find_(&data->table, od->nbs); >>> ovs_assert(ls_stateful_rec); >>> - ls_stateful_record_set_acls(ls_stateful_rec, od->nbs, >>> - input_data.ls_port_groups); >>> - >>> - /* Add the ls_stateful_rec to the tracking data. */ >>> - hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); >>> + /* Ensure that only one handler per engine run calls >>> + * ls_stateful_record_set_acls on the same ls_stateful rec. */ >>> + if (!hmapx_contains(&data->trk_data.crupdated, >>> ls_stateful_rec)) { >>> >> >> >> You can replace this with 'if (hmapx_add(..))', as hmapx_add will return >> a pointer >> if it's a new item, NULL otherwise. >> >> >>> + ls_stateful_record_set_acls(ls_stateful_rec, od->nbs, >>> + input_data.ls_port_groups); >>> + >>> + /* Add the ls_stateful_rec to the tracking data. */ >>> + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); >>> + } >>> } >>> >>> if (ls_stateful_has_tracked_data(&data->trk_data)) { >>> @@ -179,11 +183,24 @@ ls_stateful_port_group_handler(struct engine_node >>> *node, void *data_) >>> struct port_group_data *pg_data = >>> engine_get_input_data("port_group", node); >>> >>> - if (pg_data->ls_port_groups_sets_changed) { >>> - return EN_UNHANDLED; >>> + struct ed_type_ls_stateful *data = data_; >>> + struct hmapx_node *hmap_node; >>> + HMAPX_FOR_EACH (hmap_node, &pg_data->ls_port_groups_sets_changed) { >>> + const struct nbrec_logical_switch *nbs = hmap_node->data; >>> + struct ls_stateful_record *ls_stateful_rec = >>> + ls_stateful_table_find_(&data->table, nbs); >>> + ovs_assert(ls_stateful_rec); >>> + /* Ensure only one handler per engine run calls >>> + * ls_stateful_record_set_acls on the same ls_stateful rec. */ >>> + if (!hmapx_contains(&data->trk_data.crupdated, >>> ls_stateful_rec)) { >>> >> >> Same here with the hmapx_add. >> >> >>> + ls_stateful_record_set_acls(ls_stateful_rec, >>> + nbs, >>> + &pg_data->ls_port_groups); >>> + /* Add the ls_stateful_rec to the tracking data. */ >>> + hmapx_add(&data->trk_data.crupdated, ls_stateful_rec); >>> + } >>> } >>> >>> - struct ed_type_ls_stateful *data = data_; >>> if (ls_stateful_has_tracked_data(&data->trk_data)) { >>> return EN_HANDLED_UPDATED; >>> } >>> diff --git a/northd/en-port-group.c b/northd/en-port-group.c >>> index 4fc1a4f24..fd573d9df 100644 >>> --- a/northd/en-port-group.c >>> +++ b/northd/en-port-group.c >>> @@ -33,18 +33,24 @@ static struct ls_port_group *ls_port_group_create( >>> static void ls_port_group_destroy(struct ls_port_group_table *, >>> struct ls_port_group *); >>> >>> -static bool ls_port_group_process( >>> +static void ls_port_group_process( >>> struct ls_port_group_table *, >>> struct port_group_ls_table *, >>> + struct hmapx *, >>> const struct hmap *ls_ports, >>> const struct nbrec_port_group *, >>> - struct hmapx *updated_ls_port_groups); >>> + struct hmapx *updated_ls_port_groups, >>> + struct sset *pruned_ls_port_group_recs); >>> >>> static void ls_port_group_record_clear( >>> struct ls_port_group_table *, >>> struct port_group_ls_record *, >>> struct hmapx *cleared_ls_port_groups); >>> -static bool ls_port_group_record_prune(struct ls_port_group *); >>> + >>> +static bool ls_port_group_record_prune( >>> + struct ls_port_group *, >>> + const struct nbrec_port_group *, >>> + struct sset *); >>> >>> static struct ls_port_group_record *ls_port_group_record_create( >>> struct ls_port_group *, >>> @@ -118,8 +124,8 @@ ls_port_group_table_build( >>> { >>> const struct nbrec_port_group *nb_pg; >>> NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_pg, pg_table) { >>> - ls_port_group_process(ls_port_groups, port_group_lses, >>> - ls_ports, nb_pg, NULL); >>> + ls_port_group_process(ls_port_groups, port_group_lses, NULL, >>> + ls_ports, nb_pg, NULL, NULL); >>> } >>> } >>> >>> @@ -206,20 +212,27 @@ ls_port_group_destroy(struct ls_port_group_table >>> *ls_port_groups, >>> } >>> } >>> >>> -/* Process a NB.Port_Group record and stores any updated ls_port_groups >>> - * in updated_ls_port_groups. Returns true if a new ls_port_group had >>> - * to be created or destroyed. >>> +/* Process a NB.Port_Group record updated the ls_port_group_table and >>> the >>> + * port_group_ls_table. Stores a few different updates >>> + * 1. Updated ls_port_groups are stored in updated_ls_port_groups so >>> the I-P >>> + * can update the Southbound database >>> + * 2. If a port_groups switch set changes the switch is stored in >>> + * ls_port_groups_sets_changed so that later I-P nodes can >>> recalculate >>> + * lflows. >>> + * 3. If a port_group has a switch removed from it's switch set it is >>> stored >>> + * in pruned_ls_port_group_recs so that the SB entry can be deleted. >>> */ >>> -static bool >>> +static void >>> ls_port_group_process(struct ls_port_group_table *ls_port_groups, >>> struct port_group_ls_table *port_group_lses, >>> + struct hmapx *ls_port_groups_sets_changed, >>> const struct hmap *ls_ports, >>> const struct nbrec_port_group *nb_pg, >>> - struct hmapx *updated_ls_port_groups) >>> + struct hmapx *updated_ls_port_groups, >>> + struct sset *pruned_ls_port_group_recs) >>> { >>> struct hmapx cleared_ls_port_groups = >>> HMAPX_INITIALIZER(&cleared_ls_port_groups); >>> - bool ls_pg_rec_created = false; >>> >>> struct port_group_ls_record *pg_ls = >>> port_group_ls_table_find(port_group_lses, nb_pg); >>> @@ -233,6 +246,12 @@ ls_port_group_process(struct ls_port_group_table >>> *ls_port_groups, >>> } >>> >>> for (size_t i = 0; i < nb_pg->n_ports; i++) { >>> + if (nbrec_port_group_is_deleted(nb_pg)) { >>> + /* When a port group is deleted we don't need to >>> + * updated the port group as the entry will be pruned >>> + */ >>> + break; >>> + } >>> const char *port_name = nb_pg->ports[i]->name; >>> const struct ovn_datapath *od = >>> northd_get_datapath_for_port(ls_ports, port_name); >>> @@ -262,7 +281,10 @@ ls_port_group_process(struct ls_port_group_table >>> *ls_port_groups, >>> ls_port_group_record_find(ls_pg, nb_pg); >>> if (!ls_pg_rec) { >>> ls_pg_rec = ls_port_group_record_create(ls_pg, nb_pg); >>> - ls_pg_rec_created = true; >>> + if (ls_port_groups_sets_changed) { >>> + hmapx_add(ls_port_groups_sets_changed, >>> + CONST_CAST(struct nbrec_logical_switch *, >>> od->nbs)); >>> + } >>> } >>> sset_add(&ls_pg_rec->ports, port_name); >>> >>> @@ -273,13 +295,18 @@ ls_port_group_process(struct ls_port_group_table >>> *ls_port_groups, >>> } >>> } >>> >>> - bool ls_pg_rec_destroyed = false; >>> struct hmapx_node *node; >>> HMAPX_FOR_EACH (node, &cleared_ls_port_groups) { >>> struct ls_port_group *ls_pg = node->data; >>> >>> - if (ls_port_group_record_prune(ls_pg)) { >>> - ls_pg_rec_destroyed = true; >>> + if (ls_port_group_record_prune(ls_pg, >>> + nb_pg, >>> + pruned_ls_port_group_recs)) { >>> + if (ls_port_groups_sets_changed) { >>> + hmapx_add(ls_port_groups_sets_changed, >>> + CONST_CAST(struct nbrec_logical_switch >>> *,ls_pg->nbs)); >>> + hmapx_find_and_delete(&pg_ls->switches, ls_pg->nbs); >> >> >> >> Shouldn't this be outside the second if? It feels like it should be part >> of the 'ls_port_group_record_prune()' TBH. >> > > I can move the hmapx_find_and_delete() outside of the second if, but I am > not sure if it should be inside ls_port_group_record_prune(). There are two > different structs that are cleaned up `struct ls_port_group_record` and > `struct port_group_ls_record`. They hold the same information but in a > different way. So the ls_port_group_record_prune() removes a > ls_port_group_record from the ls_port_group_table and the > hmapx_find_and_delete() removes a switch from a port_group_ls_record. If > you feel strongly that the two operations should be combined should the > function name change? something like reconcile_port_group_tables()? > I don't have a strong preference so it can remain outside 'ls_port_group_record_prune()' but keeping it inside the if isn't the right thing. It gets called even when 'ls_port_groups_sets_changed' is NULL. > > >> >> + } >>> } >>> >>> if (hmap_is_empty(&ls_pg->nb_pgs)) { >>> @@ -287,8 +314,6 @@ ls_port_group_process(struct ls_port_group_table >>> *ls_port_groups, >>> } >>> } >>> hmapx_destroy(&cleared_ls_port_groups); >>> - >>> - return ls_pg_rec_created || ls_pg_rec_destroyed; >>> } >>> >>> /* Destroys all the struct ls_port_group_record that might be >>> associated to >>> @@ -320,17 +345,27 @@ ls_port_group_record_clear(struct >>> ls_port_group_table *ls_to_port_groups, >>> } >>> >>> static bool >>> -ls_port_group_record_prune(struct ls_port_group *ls_pg) >>> +ls_port_group_record_prune(struct ls_port_group *ls_pg, >>> + const struct nbrec_port_group *nb_pg, >>> + struct sset *pruned_ls_pg_rec) >>> { >>> struct ls_port_group_record *ls_pg_rec; >>> bool records_pruned = false; >>> >>> - HMAP_FOR_EACH_SAFE (ls_pg_rec, key_node, &ls_pg->nb_pgs) { >>> - if (sset_is_empty(&ls_pg_rec->ports)) { >>> - ls_port_group_record_destroy(ls_pg, ls_pg_rec); >>> - records_pruned = true; >>> + struct ds sb_pg_name = DS_EMPTY_INITIALIZER; >>> + ls_pg_rec = ls_port_group_record_find(ls_pg, nb_pg); >>> + if (sset_is_empty(&ls_pg_rec->ports) || >>> + nbrec_port_group_is_deleted(ls_pg_rec->nb_pg)) { >>> + if (pruned_ls_pg_rec) { >>> >> + get_sb_port_group_name(ls_pg_rec->nb_pg->name, >>> + ls_pg->sb_datapath_key, >>> + &sb_pg_name); >>> + sset_add(pruned_ls_pg_rec, ds_cstr(&sb_pg_name)); >>> } >>> + ls_port_group_record_destroy(ls_pg, ls_pg_rec); >>> + records_pruned = true; >>> } >>> + ds_destroy(&sb_pg_name); >>> return records_pruned; >>> } >>> >>> @@ -460,6 +495,10 @@ en_port_group_init(struct engine_node *node >>> OVS_UNUSED, >>> struct engine_arg *arg OVS_UNUSED) >>> { >>> struct port_group_data *pg_data = xmalloc(sizeof *pg_data); >>> + *pg_data = (struct port_group_data) { >>> + .ls_port_groups_sets_changed = >>> + HMAPX_INITIALIZER(&pg_data->ls_port_groups_sets_changed), >>> + }; >>> >>> ls_port_group_table_init(&pg_data->ls_port_groups); >>> port_group_ls_table_init(&pg_data->port_groups_lses); >>> @@ -473,6 +512,7 @@ en_port_group_cleanup(void *data_) >>> >>> ls_port_group_table_destroy(&data->ls_port_groups); >>> port_group_ls_table_destroy(&data->port_groups_lses); >>> + hmapx_destroy(&data->ls_port_groups_sets_changed); >>> } >>> >>> void >>> @@ -480,7 +520,7 @@ en_port_group_clear_tracked_data(void *data_) >>> { >>> struct port_group_data *data = data_; >>> >>> - data->ls_port_groups_sets_changed = true; >>> + hmapx_clear(&data->ls_port_groups_sets_changed); >>> } >>> >>> enum engine_node_state >>> @@ -514,73 +554,71 @@ port_group_nb_port_group_handler(struct >>> engine_node *node, void *data_) >>> struct port_group_input input_data = >>> port_group_get_input_data(node); >>> const struct engine_context *eng_ctx = engine_get_context(); >>> struct port_group_data *data = data_; >>> - bool success = true; >>> >>> const struct nbrec_port_group_table *nb_pg_table = >>> EN_OVSDB_GET(engine_get_input("NB_port_group", node)); >>> const struct nbrec_port_group *nb_pg; >>> >>> - /* Return false if a port group is created or deleted. >>> - * Handle I-P for only updated port groups. */ >>> - NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_pg_table) { >>> - if (nbrec_port_group_is_new(nb_pg) || >>> - nbrec_port_group_is_deleted(nb_pg)) { >>> - return EN_UNHANDLED; >>> - } >>> - } >>> - >>> struct hmapx updated_ls_port_groups = >>> HMAPX_INITIALIZER(&updated_ls_port_groups); >>> >>> + struct sset stale_sb_port_groups = >>> SSET_INITIALIZER(&stale_sb_port_groups); >>> NBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (nb_pg, nb_pg_table) { >>> - if (ls_port_group_process(&data->ls_port_groups, >>> - &data->port_groups_lses, >>> - input_data.ls_ports, >>> - nb_pg, &updated_ls_port_groups)) { >>> - success = false; >>> - break; >>> + ls_port_group_process(&data->ls_port_groups, >>> + &data->port_groups_lses, >>> + &data->ls_port_groups_sets_changed, >>> + input_data.ls_ports, >>> + nb_pg, &updated_ls_port_groups, >>> + &stale_sb_port_groups); >>> + >>> } >>> - } >>> >>> - /* If changes have been successfully processed incrementally then >>> update >>> + /* Changes have been successfully processed incrementally now update >>> * the SB too. */ >>> - if (success) { >>> - struct ovsdb_idl_index *sbrec_port_group_by_name = >>> - engine_ovsdb_node_get_index( >>> - engine_get_input("SB_port_group", node), >>> - "sbrec_port_group_by_name"); >>> - struct ds sb_pg_name = DS_EMPTY_INITIALIZER; >>> - >>> - struct hmapx_node *updated_node; >>> - HMAPX_FOR_EACH (updated_node, &updated_ls_port_groups) { >>> - const struct ls_port_group *ls_pg = updated_node->data; >>> - struct ls_port_group_record *ls_pg_rec; >>> - >>> - HMAP_FOR_EACH (ls_pg_rec, key_node, &ls_pg->nb_pgs) { >>> - get_sb_port_group_name(ls_pg_rec->nb_pg->name, >>> - ls_pg->sb_datapath_key, >>> - &sb_pg_name); >>> - >>> - const char *sb_pg_name_cstr = ds_cstr(&sb_pg_name); >>> - const struct sbrec_port_group *sb_pg = >>> - >>> sb_port_group_lookup_by_name(sbrec_port_group_by_name, >>> - sb_pg_name_cstr); >>> - if (!sb_pg) { >>> - sb_pg = create_sb_port_group(eng_ctx->ovnsb_idl_txn, >>> - sb_pg_name_cstr); >>> - } >>> - struct sorted_array nb_ports = >>> - sorted_array_from_sset(&ls_pg_rec->ports); >>> - update_sb_port_group(&nb_ports, sb_pg); >>> - sorted_array_destroy(&nb_ports); >>> + struct ovsdb_idl_index *sbrec_port_group_by_name = >>> + engine_ovsdb_node_get_index( >>> + engine_get_input("SB_port_group", node), >>> + "sbrec_port_group_by_name"); >>> + struct ds sb_pg_name = DS_EMPTY_INITIALIZER; >>> + >>> + struct hmapx_node *updated_node; >>> + HMAPX_FOR_EACH (updated_node, &updated_ls_port_groups) { >>> + const struct ls_port_group *ls_pg = updated_node->data; >>> + struct ls_port_group_record *ls_pg_rec; >>> + >>> + HMAP_FOR_EACH (ls_pg_rec, key_node, &ls_pg->nb_pgs) { >>> + get_sb_port_group_name(ls_pg_rec->nb_pg->name, >>> + ls_pg->sb_datapath_key, >>> + &sb_pg_name); >>> + >>> + const char *sb_pg_name_cstr = ds_cstr(&sb_pg_name); >>> + const struct sbrec_port_group *sb_pg = >>> + sb_port_group_lookup_by_name(sbrec_port_group_by_name, >>> + sb_pg_name_cstr); >>> + if (!sb_pg) { >>> + sb_pg = create_sb_port_group(eng_ctx->ovnsb_idl_txn, >>> + sb_pg_name_cstr); >>> } >>> + struct sorted_array nb_ports = >>> + sorted_array_from_sset(&ls_pg_rec->ports); >>> + update_sb_port_group(&nb_ports, sb_pg); >>> + sorted_array_destroy(&nb_ports); >>> } >>> - ds_destroy(&sb_pg_name); >>> } >>> + ds_destroy(&sb_pg_name); >>> + >>> + const char *stale_sb_port_group_name; >>> + SSET_FOR_EACH (stale_sb_port_group_name, &stale_sb_port_groups) { >>> + const struct sbrec_port_group *sb_pg = >>> + sb_port_group_lookup_by_name(sbrec_port_group_by_name, >>> + stale_sb_port_group_name); >>> + sbrec_port_group_delete(sb_pg); >>> + } >>> + sset_destroy(&stale_sb_port_groups); >>> + >>> >>> - data->ls_port_groups_sets_changed = !success; >>> hmapx_destroy(&updated_ls_port_groups); >>> - return success ? EN_HANDLED_UPDATED : EN_UNHANDLED; >>> + return EN_HANDLED_UPDATED; >>> } >>> >>> static void >>> diff --git a/northd/en-port-group.h b/northd/en-port-group.h >>> index 8dcc950da..f07f118aa 100644 >>> --- a/northd/en-port-group.h >>> +++ b/northd/en-port-group.h >>> @@ -104,7 +104,7 @@ struct port_group_input { >>> struct port_group_data { >>> struct ls_port_group_table ls_port_groups; >>> struct port_group_ls_table port_groups_lses; >>> - bool ls_port_groups_sets_changed; >>> + struct hmapx ls_port_groups_sets_changed; >>> }; >>> >>> void *en_port_group_init(struct engine_node *, struct engine_arg *); >>> diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c >>> index d43bfc16c..fc4ae58f8 100644 >>> --- a/northd/inc-proc-northd.c >>> +++ b/northd/inc-proc-northd.c >>> @@ -348,7 +348,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, >>> engine_add_input(&en_lflow, &en_sampling_app, NULL); >>> >>> engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); >>> - engine_add_input(&en_lflow, &en_port_group, >>> lflow_port_group_handler); >>> + engine_add_input(&en_lflow, &en_port_group, engine_noop_handler); >>> >> >> Please add a comment explaining why lflow doesn't >> need to handle PG changes anymore. >> >> >>> engine_add_input(&en_lflow, &en_lr_stateful, >>> lflow_lr_stateful_handler); >>> engine_add_input(&en_lflow, &en_ls_stateful, >>> lflow_ls_stateful_handler); >>> engine_add_input(&en_lflow, &en_multicast_igmp, >>> diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at >>> index 5e0747fcc..61d00e576 100644 >>> --- a/tests/ovn-northd.at >>> +++ b/tests/ovn-northd.at >>> @@ -10100,27 +10100,11 @@ AS_BOX([Create new PG1 and PG2]) >>> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>> check ovn-nbctl --wait=sb -- pg-add pg1 -- pg-add pg2 >>> dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node recomputes every time a NB port group is >>> added/deleted. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node is an input for the lflow node. Port_group >>> -dnl recompute/compute triggers lflow recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats northd norecompute compute >>> +dnl The port_group node does not recompute when a NB port group is >>> added/deleted. >>> +check_engine_stats port_group norecompute compute >>> +dnl The Port group recompute/compute should not trigger an lflow >>> recompute. >>> +check_engine_stats lflow norecompute compute >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> >>> AS_BOX([Add ACLs on PG1 and PG2]) >>> @@ -10137,28 +10121,15 @@ check_column "sw1.1" sb:Port_Group ports >>> name="${sw1_key}_pg1" >>> check_column "sw2.1" sb:Port_Group ports name="${sw2_key}_pg1" >>> >>> dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node recomputes also every time a port from a new >>> switch >>> -dnl is added to the group. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node is an input for the lflow node. Port_group >>> -dnl recompute/compute triggers lflow recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats northd norecompute compute >>> +dnl The port_group node does not recompute when a new switch is added >>> to the >>> +dnl group >>> +check_engine_stats port_group norecompute compute >>> +dnl The port_group node is an input for the ls_statful node. Port_group >>> +dnl should not trigger a recompute. >>> +check_engine_stats ls_stateful norecompute compute >>> +dnl Port_Group compute/recompute should not trigger an lflow recompute. >>> +check_engine_stats lflow norecompute compute >>> dnl Expect ACL1 on sw1 and sw2 >>> check_acl_lflows 1 0 1 0 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> @@ -10173,29 +10144,16 @@ check_column "sw1.2" sb:Port_Group ports >>> name="${sw1_key}_pg2" >>> check_column "sw2.2" sb:Port_Group ports name="${sw2_key}_pg2" >>> >>> dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node recomputes also every time a port from a new >>> switch >>> +check_engine_stats northd norecompute compute >>> +dnl The port_group node should not recompute when a port from a new >>> switch >>> >> >> nit: Extra space. >> >> >>> dnl is added to the group. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl The port_group node is an input for the lflow node. Port_group >>> -dnl recompute/compute triggers lflow recompute (for ACLs). >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl Expect both ACLs on sw1 and sw2 >>> +check_engine_stats port_group norecompute compute >>> +dnl The port_group node is an input for the ls_statful node. Port_group >>> +dnl should not trigger a recompute >>> +check_engine_stats ls_stateful norecompute compute >>> +dnl Port_Group compute/recompute should not trigger an lflow recompute. >>> +check_engine_stats lflow norecompute compute >>> +dnl Expect both ACLs on sw1 and sw2. >>> check_acl_lflows 1 1 1 1 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> >>> @@ -10210,29 +10168,11 @@ check_column "sw1.2 sw1.3" sb:Port_Group ports >>> name="${sw1_key}_pg2" >>> check_column "sw2.2 sw2.3" sb:Port_Group ports name="${sw2_key}_pg2" >>> >>> dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We did not change the set of switches a pg is applied to, there >>> should be >>> -dnl no recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We did not change the set of switches a pg is applied to, there >>> should be >>> -dnl no recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl Expect both ACLs on sw1 and sw2 >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> +dnl Expect both ACLs on sw1 and sw2. >>> check_acl_lflows 1 1 1 1 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> >>> @@ -10248,28 +10188,10 @@ check_column "sw2.2" sb:Port_Group ports >>> name="${sw2_key}_pg2" >>> >>> dnl The northd node should not recompute, it should handle nb_global >>> update >>> dnl though, therefore "compute: 1". >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We did not change the set of switches a pg is applied to, there >>> should be >>> -dnl no recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We did not change the set of switches a pg is applied to, there >>> should be >>> -dnl no recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> dnl Expect both ACLs on sw1 and sw2 >>> check_acl_lflows 1 1 1 1 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> @@ -10280,32 +10202,19 @@ check ovn-nbctl --wait=sb pg-set-ports pg2 >>> sw1.2 >>> check_column "sw1.1" sb:Port_Group ports name="${sw1_key}_pg1" >>> check_column "sw2.1" sb:Port_Group ports name="${sw2_key}_pg1" >>> check_column "sw1.2" sb:Port_Group ports name="${sw1_key}_pg2" >>> + >>> AT_CHECK([fetch_column sb:Port_Group ports name="${sw2_key}_pg2"], [0], >>> [ >>> ]) >>> >>> dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be >>> +check_engine_stats northd norecompute compute >>> +dnl We changed the set of switches a pg is applied to, this should no >>> longer require >>> dnl a recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be >>> -dnl a recompute (for ACLs). >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats port_group norecompute compute >>> +dnl There should be no recompute for ls_stateful >>> +check_engine_stats ls_stateful norecompute compute >>> +dnl No recompute should occur. >>> +check_engine_stats lflow norecompute compute >>> dnl Expect both ACLs on sw1 and only the first one on sw2. >>> check_acl_lflows 1 1 1 0 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> @@ -10320,29 +10229,11 @@ check_column "sw1.2" sb:Port_Group ports >>> name="${sw1_key}_pg2" >>> AT_CHECK([fetch_column sb:Port_Group ports name="${sw2_key}_pg2"], [0], >>> [ >>> ]) >>> >>> -dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be >>> -dnl a recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be >>> -dnl a recompute (for ACLs). >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> + >>> dnl Expect both ACLs on sw1 and not on sw2. >>> check_acl_lflows 1 1 0 0 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> @@ -10357,36 +10248,18 @@ check_column "sw2.1" sb:Port_Group ports >>> name="${sw2_key}_pg1" >>> check_column "sw1.2" sb:Port_Group ports name="${sw1_key}_pg2" >>> check_column "sw2.2" sb:Port_Group ports name="${sw2_key}_pg2" >>> >>> -dnl The northd node should not recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be a >>> -dnl recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be a >>> -dnl recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> + >>> dnl Expect both ACLs on sw1 and sw2 >>> check_acl_lflows 1 1 1 1 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> >>> AS_BOX([Remove second port from both PGs]) >>> check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>> -check ovn-nbctl --wait=sb \ >>> +check ovn-nbctl --wait=sb \ >>> -- pg-set-ports pg1 sw1.1 \ >>> -- pg-set-ports pg2 sw1.2 >>> check_column "sw1.1" sb:Port_Group ports name="${sw1_key}_pg1" >>> @@ -10396,33 +10269,53 @@ check_column "sw1.2" sb:Port_Group ports >>> name="${sw1_key}_pg2" >>> AT_CHECK([fetch_column sb:Port_Group ports name="${sw2_key}_pg2"], [0], >>> [ >>> ]) >>> >>> -dnl The northd node should not recompute,. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> northd], [0], [dnl >>> -Node: northd >>> -- recompute: 0 >>> -- compute: 1 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be a >>> -dnl recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> port_group], [0], [dnl >>> -Node: port_group >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl We changed the set of switches a pg is applied to, there should be a >>> -dnl recompute. >>> -AT_CHECK([as northd ovn-appctl -t ovn-northd inc-engine/show-stats >>> lflow], [0], [dnl >>> -Node: lflow >>> -- recompute: 1 >>> -- compute: 0 >>> -- cancel: 0 >>> -]) >>> -dnl Expect both ACLs on sw1 and no ACLs on sw2 >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> + >>> +dnl Expect both ACLs sw1 >>> check_acl_lflows 1 1 0 0 >>> CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> >>> +AS_BOX([Delete port groups PG1 and PG2]) >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>> +check ovn-nbctl --wait=sb \ >>> + -- pg-del pg1 \ >>> + -- pg-del pg2 >>> + >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> + >>> +dnl Expect no ACLs on sw1 and no ACLs on sw2 >>> +check_acl_lflows 0 0 0 0 >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> + >>> +AS_BOX([Add PG1 and PG2 again setting ports and ACLs in one >>> transaction]) >>> +check as northd ovn-appctl -t ovn-northd inc-engine/clear-stats >>> +check ovn-nbctl --wait=sb \ >>> + -- pg-add pg1 \ >>> + -- pg-add pg2 \ >>> + -- acl-add pg1 from-lport 1 eth.src==41:41:41:41:41:41 allow \ >>> + -- acl-add pg2 from-lport 1 eth.src==42:42:42:42:42:42 allow \ >>> + -- pg-set-ports pg1 sw1.1 sw2.1 \ >>> + -- pg-set-ports pg2 sw1.2 sw2.2 >>> +check_column "sw1.1" sb:Port_Group ports name="${sw1_key}_pg1" >>> +check_column "sw2.1" sb:Port_Group ports name="${sw2_key}_pg1" >>> +check_column "sw1.2" sb:Port_Group ports name="${sw1_key}_pg2" >>> +check_column "sw2.2" sb:Port_Group ports name="${sw2_key}_pg2" >>> + >>> +check_engine_stats northd norecompute compute >>> +check_engine_stats port_group norecompute compute >>> +check_engine_stats ls_stateful norecompute compute >>> +check_engine_stats lflow norecompute compute >>> + >>> +dnl Expect both ACLs on sw1 and sw2 >>> +check_acl_lflows 1 1 1 1 >>> +CHECK_NO_CHANGE_AFTER_RECOMPUTE >>> + >>> AT_CLEANUP >>> ]) >>> >>> -- >>> 2.50.1 >>> >>> _______________________________________________ >>> dev mailing list >>> d...@openvswitch.org >>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>> >>> >> Thanks, >> Ales >> > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
