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()? > > + } >> } >> >> 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
