It's similar to the processing we do for address sets. There's a bit more mechanics involved due to the fact that we need to split NB port groups per datapath.
We currently only partially implement incremental processing of port_group changes in the lflow node. That is, we deal with the case when the sets of "switches per port group" doesn't change. In that specific case ACL lflows don't need to be reprocessed. In a synthetic benchmark that created (in this order): - 500 switches - 2000 port groups - 4 ACLs per port group - 10000 ports distributed equally between the switches and port groups we measured the following ovn-northd CPU usage: +-------------------------+------------+--------------------+ | Incremental processing? | --wait=sb? | northd avg cpu (%) | +-------------------------+------------+--------------------+ | N | Y | 84.2 | +-------------------------+------------+--------------------+ | Y | Y | 41.5 | +-------------------------+------------+--------------------+ | N | N | 93.2 | +-------------------------+------------+--------------------+ | Y | N | 53.6 | +-------------------------+------------+--------------------+ where '--wait=sb' set to 'Y' means the benchmark was waiting for the port and port group operations to be propagated to the Southbound DB before continuing to the next operation. Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=2228162 Signed-off-by: Dumitru Ceara <[email protected]> --- V2: - First 4 patches of the series merged, only respun the 5th one. - Addressed Han's comments: - reversed semantics of ls_port_groups_sets_unchanged - changed "struct port_group_to_ls_table" to "struct port_group_ls_table" - changed "struct port_group_to_ls" to "struct port_group_ls_record" - fixed bug due to missing recompute whenever a per-LS port group (but not the last one) was deleted - handled SB port group creation in the I-P handler (normally that should not happen but if the SB is manually changed we now reconcile it without trigerring a recompute) - fixed up some comment typos - changed error checks in ls_port_group_record_clear() into assert() calls. - updated unit test to include missing scenarios - added CHECK_NO_CHANGE_AFTER_RECOMPUTE calls to unit test - added port group check to _DUMP_DB_TABLES - Addressed Ales' comments: - use xmalloc instead of xzalloc followed by full initialization of the structure - Other changes: - unified I-P handler processing for port_group_ls_record creation/deletion --- northd/en-lflow.c | 17 ++ northd/en-lflow.h | 1 + northd/en-port-group.c | 452 ++++++++++++++++++++++++++++++++++----- northd/en-port-group.h | 36 +++- northd/inc-proc-northd.c | 13 +- northd/ovn-northd.c | 4 + tests/ovn-northd.at | 371 ++++++++++++++++++++++++++++++++ 7 files changed, 832 insertions(+), 62 deletions(-) diff --git a/northd/en-lflow.c b/northd/en-lflow.c index c6c0d4ebbf..153bf0f57a 100644 --- a/northd/en-lflow.c +++ b/northd/en-lflow.c @@ -122,6 +122,23 @@ lflow_northd_handler(struct engine_node *node, return true; } +bool +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 false; + } + + engine_set_node_state(node, EN_UPDATED); + return true; +} + void *en_lflow_init(struct engine_node *node OVS_UNUSED, struct engine_arg *arg OVS_UNUSED) { diff --git a/northd/en-lflow.h b/northd/en-lflow.h index 5e3fbc25e3..5417b2faff 100644 --- a/northd/en-lflow.h +++ b/northd/en-lflow.h @@ -13,5 +13,6 @@ void en_lflow_run(struct engine_node *node, void *data); void *en_lflow_init(struct engine_node *node, struct engine_arg *arg); void en_lflow_cleanup(void *data); bool lflow_northd_handler(struct engine_node *, void *data); +bool lflow_port_group_handler(struct engine_node *, void *data); #endif /* EN_LFLOW_H */ diff --git a/northd/en-port-group.c b/northd/en-port-group.c index 2c36410246..0de9dc5f67 100644 --- a/northd/en-port-group.c +++ b/northd/en-port-group.c @@ -33,15 +33,43 @@ 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 struct ls_port_group_record *ls_port_group_record_add( - struct ls_port_group *, +static bool ls_port_group_process( + struct ls_port_group_table *, + struct port_group_ls_table *, + const struct hmap *ls_ports, const struct nbrec_port_group *, - const char *port_name); + struct hmapx *updated_ls_port_groups); + +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 struct ls_port_group_record *ls_port_group_record_create( + struct ls_port_group *, + const struct nbrec_port_group *); + +static struct ls_port_group_record *ls_port_group_record_find( + struct ls_port_group *, const struct nbrec_port_group *nb_pg); static void ls_port_group_record_destroy( struct ls_port_group *, struct ls_port_group_record *); +static struct port_group_ls_record *port_group_ls_record_create( + struct port_group_ls_table *, + const struct nbrec_port_group *); +static void port_group_ls_record_destroy(struct port_group_ls_table *, + struct port_group_ls_record *); + +static const struct sbrec_port_group *create_sb_port_group( + struct ovsdb_idl_txn *ovnsb_txn, const char *sb_pg_name); +static void update_sb_port_group(struct sorted_array *nb_ports, + const struct sbrec_port_group *sb_pg); +static const struct sbrec_port_group *sb_port_group_lookup_by_name( + struct ovsdb_idl_index *sbrec_port_group_by_name, const char *name); + void ls_port_group_table_init(struct ls_port_group_table *table) { @@ -82,39 +110,16 @@ ls_port_group_table_find(const struct ls_port_group_table *table, } void -ls_port_group_table_build(struct ls_port_group_table *ls_port_groups, - const struct nbrec_port_group_table *pg_table, - const struct hmap *ls_ports) +ls_port_group_table_build( + struct ls_port_group_table *ls_port_groups, + struct port_group_ls_table *port_group_lses, + const struct nbrec_port_group_table *pg_table, + const struct hmap *ls_ports) { const struct nbrec_port_group *nb_pg; NBREC_PORT_GROUP_TABLE_FOR_EACH (nb_pg, pg_table) { - for (size_t i = 0; i < nb_pg->n_ports; i++) { - const char *port_name = nb_pg->ports[i]->name; - const struct ovn_datapath *od = - northd_get_datapath_for_port(ls_ports, port_name); - - if (!od) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_ERR_RL(&rl, "lport %s in port group %s not found.", - port_name, nb_pg->name); - continue; - } - - if (!od->nbs) { - static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); - VLOG_WARN_RL(&rl, "lport %s in port group %s has no lswitch.", - nb_pg->ports[i]->name, - nb_pg->name); - continue; - } - - struct ls_port_group *ls_pg = - ls_port_group_table_find(ls_port_groups, od->nbs); - if (!ls_pg) { - ls_pg = ls_port_group_create(ls_port_groups, od->nbs, od->sb); - } - ls_port_group_record_add(ls_pg, nb_pg, port_name); - } + ls_port_group_process(ls_port_groups, port_group_lses, + ls_ports, nb_pg, NULL); } } @@ -145,18 +150,18 @@ ls_port_group_table_sync( get_sb_port_group_name(ls_pg_rec->nb_pg->name, ls_pg->sb_datapath_key, &sb_name); + const char *sb_pg_name_cstr = ds_cstr(&sb_name); sb_port_group = shash_find_and_delete(&sb_port_groups, - ds_cstr(&sb_name)); + sb_pg_name_cstr); if (!sb_port_group) { - sb_port_group = sbrec_port_group_insert(ovnsb_txn); - sbrec_port_group_set_name(sb_port_group, ds_cstr(&sb_name)); - } - - const char **nb_port_names = sset_array(&ls_pg_rec->ports); - sbrec_port_group_set_ports(sb_port_group, - nb_port_names, - sset_count(&ls_pg_rec->ports)); - free(nb_port_names); + sb_port_group = create_sb_port_group(ovnsb_txn, + sb_pg_name_cstr); + }; + + struct sorted_array ports = + sorted_array_from_sset(&ls_pg_rec->ports); + update_sb_port_group(&ports, sb_port_group); + sorted_array_destroy(&ports); } } ds_destroy(&sb_name); @@ -201,31 +206,164 @@ ls_port_group_destroy(struct ls_port_group_table *ls_port_groups, } } -static struct ls_port_group_record * -ls_port_group_record_add(struct ls_port_group *ls_pg, - const struct nbrec_port_group *nb_pg, - const char *port_name) +/* 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. + */ +static bool +ls_port_group_process(struct ls_port_group_table *ls_port_groups, + struct port_group_ls_table *port_group_lses, + const struct hmap *ls_ports, + const struct nbrec_port_group *nb_pg, + struct hmapx *updated_ls_port_groups) { - struct ls_port_group_record *ls_pg_rec = NULL; - size_t hash = uuid_hash(&nb_pg->header_.uuid); + 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); + if (!pg_ls) { + pg_ls = port_group_ls_record_create(port_group_lses, nb_pg); + } else { + /* Clear all old records corresponding to this port group; we'll + * reprocess it below. */ + ls_port_group_record_clear(ls_port_groups, pg_ls, + &cleared_ls_port_groups); + } - HMAP_FOR_EACH_WITH_HASH (ls_pg_rec, key_node, hash, &ls_pg->nb_pgs) { - if (ls_pg_rec->nb_pg == nb_pg) { - goto done; + for (size_t i = 0; i < nb_pg->n_ports; i++) { + const char *port_name = nb_pg->ports[i]->name; + const struct ovn_datapath *od = + northd_get_datapath_for_port(ls_ports, port_name); + + if (!od) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_ERR_RL(&rl, "lport %s in port group %s not found.", + port_name, nb_pg->name); + continue; + } + + if (!od->nbs) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 1); + VLOG_WARN_RL(&rl, "lport %s in port group %s has no lswitch.", + nb_pg->ports[i]->name, + nb_pg->name); + continue; + } + + struct ls_port_group *ls_pg = + ls_port_group_table_find(ls_port_groups, od->nbs); + if (!ls_pg) { + ls_pg = ls_port_group_create(ls_port_groups, od->nbs, od->sb); + } + + struct ls_port_group_record *ls_pg_rec = + 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; + } + sset_add(&ls_pg_rec->ports, port_name); + + hmapx_add(&pg_ls->switches, + CONST_CAST(struct nbrec_logical_switch *, od->nbs)); + if (updated_ls_port_groups) { + hmapx_add(updated_ls_port_groups, ls_pg); + } + } + + 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 (hmap_is_empty(&ls_pg->nb_pgs)) { + ls_port_group_destroy(ls_port_groups, ls_pg); } } + hmapx_destroy(&cleared_ls_port_groups); - ls_pg_rec = xzalloc(sizeof *ls_pg_rec); + return ls_pg_rec_created || ls_pg_rec_destroyed; +} + +/* Destroys all the struct ls_port_group_record that might be associated to + * northbound database logical switches. Stores ls_port_groups that + * were cleared in the 'cleared_ls_port_groups' map. + */ +static void +ls_port_group_record_clear(struct ls_port_group_table *ls_to_port_groups, + struct port_group_ls_record *pg_ls, + struct hmapx *cleared_ls_port_groups) +{ + struct hmapx_node *node; + + HMAPX_FOR_EACH (node, &pg_ls->switches) { + const struct nbrec_logical_switch *nbs = node->data; + + struct ls_port_group *ls_pg = + ls_port_group_table_find(ls_to_port_groups, nbs); + ovs_assert(ls_pg); + + /* Clear ports in the port group record. */ + struct ls_port_group_record *ls_pg_rec = + ls_port_group_record_find(ls_pg, pg_ls->nb_pg); + ovs_assert(ls_pg_rec); + + sset_clear(&ls_pg_rec->ports); + hmapx_add(cleared_ls_port_groups, ls_pg); + } +} + +static bool +ls_port_group_record_prune(struct ls_port_group *ls_pg) +{ + 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; + } + } + return records_pruned; +} + +static struct ls_port_group_record * +ls_port_group_record_create(struct ls_port_group *ls_pg, + const struct nbrec_port_group *nb_pg) +{ + struct ls_port_group_record *ls_pg_rec = xmalloc(sizeof *ls_pg_rec); *ls_pg_rec = (struct ls_port_group_record) { .nb_pg = nb_pg, .ports = SSET_INITIALIZER(&ls_pg_rec->ports), }; - hmap_insert(&ls_pg->nb_pgs, &ls_pg_rec->key_node, hash); -done: - sset_add(&ls_pg_rec->ports, port_name); + hmap_insert(&ls_pg->nb_pgs, &ls_pg_rec->key_node, + uuid_hash(&nb_pg->header_.uuid)); return ls_pg_rec; } +static struct ls_port_group_record * +ls_port_group_record_find(struct ls_port_group *ls_pg, + const struct nbrec_port_group *nb_pg) +{ + size_t hash = uuid_hash(&nb_pg->header_.uuid); + struct ls_port_group_record *ls_pg_rec; + + HMAP_FOR_EACH_WITH_HASH (ls_pg_rec, key_node, hash, &ls_pg->nb_pgs) { + if (ls_pg_rec->nb_pg == nb_pg) { + return ls_pg_rec; + } + } + return NULL; +} + + static void ls_port_group_record_destroy(struct ls_port_group *ls_pg, struct ls_port_group_record *ls_pg_rec) @@ -237,6 +375,71 @@ ls_port_group_record_destroy(struct ls_port_group *ls_pg, } } +void +port_group_ls_table_init(struct port_group_ls_table *table) +{ + *table = (struct port_group_ls_table) { + .entries = HMAP_INITIALIZER(&table->entries), + }; +} + +void +port_group_ls_table_clear(struct port_group_ls_table *table) +{ + struct port_group_ls_record *pg_ls; + HMAP_FOR_EACH_SAFE (pg_ls, key_node, &table->entries) { + port_group_ls_record_destroy(table, pg_ls); + } +} + +void +port_group_ls_table_destroy(struct port_group_ls_table *table) +{ + port_group_ls_table_clear(table); + hmap_destroy(&table->entries); +} + +struct port_group_ls_record * +port_group_ls_table_find(const struct port_group_ls_table *table, + const struct nbrec_port_group *nb_pg) +{ + struct port_group_ls_record *pg_ls; + + HMAP_FOR_EACH_WITH_HASH (pg_ls, key_node, uuid_hash(&nb_pg->header_.uuid), + &table->entries) { + if (nb_pg == pg_ls->nb_pg) { + return pg_ls; + } + } + return NULL; +} + +static struct port_group_ls_record * +port_group_ls_record_create(struct port_group_ls_table *table, + const struct nbrec_port_group *nb_pg) +{ + struct port_group_ls_record *pg_ls = xmalloc(sizeof *pg_ls); + + *pg_ls = (struct port_group_ls_record) { + .nb_pg = nb_pg, + .switches = HMAPX_INITIALIZER(&pg_ls->switches), + }; + hmap_insert(&table->entries, &pg_ls->key_node, + uuid_hash(&nb_pg->header_.uuid)); + return pg_ls; +} + +static void +port_group_ls_record_destroy(struct port_group_ls_table *table, + struct port_group_ls_record *pg_ls) +{ + if (pg_ls) { + hmapx_destroy(&pg_ls->switches); + hmap_remove(&table->entries, &pg_ls->key_node); + free(pg_ls); + } +} + /* Incremental processing implementation. */ static struct port_group_input port_group_get_input_data(struct engine_node *node) @@ -259,6 +462,7 @@ en_port_group_init(struct engine_node *node OVS_UNUSED, struct port_group_data *pg_data = xmalloc(sizeof *pg_data); ls_port_group_table_init(&pg_data->ls_port_groups); + port_group_ls_table_init(&pg_data->port_groups_lses); return pg_data; } @@ -268,6 +472,15 @@ en_port_group_cleanup(void *data_) struct port_group_data *data = data_; ls_port_group_table_destroy(&data->ls_port_groups); + port_group_ls_table_destroy(&data->port_groups_lses); +} + +void +en_port_group_clear_tracked_data(void *data_) +{ + struct port_group_data *data = data_; + + data->ls_port_groups_sets_changed = true; } void @@ -280,7 +493,10 @@ en_port_group_run(struct engine_node *node, void *data_) stopwatch_start(PORT_GROUP_RUN_STOPWATCH_NAME, time_msec()); ls_port_group_table_clear(&data->ls_port_groups); + port_group_ls_table_clear(&data->port_groups_lses); + ls_port_group_table_build(&data->ls_port_groups, + &data->port_groups_lses, input_data.nbrec_port_group_table, input_data.ls_ports); @@ -291,3 +507,127 @@ en_port_group_run(struct engine_node *node, void *data_) stopwatch_stop(PORT_GROUP_RUN_STOPWATCH_NAME, time_msec()); engine_set_node_state(node, EN_UPDATED); } + +bool +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 false; + } + } + + struct hmapx updated_ls_port_groups = + HMAPX_INITIALIZER(&updated_ls_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; + } + } + + /* If changes have been successfully processed incrementally then 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); + } + } + ds_destroy(&sb_pg_name); + } + + data->ls_port_groups_sets_changed = !success; + engine_set_node_state(node, EN_UPDATED); + hmapx_destroy(&updated_ls_port_groups); + return success; +} + +static void +sb_port_group_apply_diff(const void *arg, const char *item, bool add) +{ + const struct sbrec_port_group *pg = arg; + if (add) { + sbrec_port_group_update_ports_addvalue(pg, item); + } else { + sbrec_port_group_update_ports_delvalue(pg, item); + } +} + +static const struct sbrec_port_group * +create_sb_port_group(struct ovsdb_idl_txn *ovnsb_txn, const char *sb_pg_name) +{ + struct sbrec_port_group *sb_port_group = + sbrec_port_group_insert(ovnsb_txn); + + sbrec_port_group_set_name(sb_port_group, sb_pg_name); + return sb_port_group; +} + +static void +update_sb_port_group(struct sorted_array *nb_ports, + const struct sbrec_port_group *sb_pg) +{ + struct sorted_array sb_ports = sorted_array_from_dbrec(sb_pg, ports); + sorted_array_apply_diff(nb_ports, &sb_ports, + sb_port_group_apply_diff, sb_pg); + sorted_array_destroy(&sb_ports); +} + +/* Finds and returns the port group set with the given 'name', or NULL + * if no such port group exists. */ +static const struct sbrec_port_group * +sb_port_group_lookup_by_name(struct ovsdb_idl_index *sbrec_port_group_by_name, + const char *name) +{ + struct sbrec_port_group *target = sbrec_port_group_index_init_row( + sbrec_port_group_by_name); + sbrec_port_group_index_set_name(target, name); + + struct sbrec_port_group *retval = sbrec_port_group_index_find( + sbrec_port_group_by_name, target); + + sbrec_port_group_index_destroy_row(target); + return retval; +} diff --git a/northd/en-port-group.h b/northd/en-port-group.h index 5cbf6c6c4a..3b28a23694 100644 --- a/northd/en-port-group.h +++ b/northd/en-port-group.h @@ -18,6 +18,7 @@ #include <stdint.h> +#include "lib/hmapx.h" #include "lib/inc-proc-eng.h" #include "lib/ovn-nb-idl.h" #include "lib/ovn-sb-idl.h" @@ -54,9 +55,33 @@ struct ls_port_group *ls_port_group_table_find( const struct ls_port_group_table *, const struct nbrec_logical_switch *); -void ls_port_group_table_build(struct ls_port_group_table *ls_port_groups, - const struct nbrec_port_group_table *, - const struct hmap *ls_ports); +/* Per port group map of datapaths with ports in the group. */ +struct port_group_ls_table { + struct hmap entries; /* Stores struct port_group_ls_record. */ +}; + +struct port_group_ls_record { + struct hmap_node key_node; /* Index on 'pg->header_.uuid'. */ + + const struct nbrec_port_group *nb_pg; + + /* Map of 'struct nbrec_logical_switch *' with ports in the group. */ + struct hmapx switches; +}; + +void port_group_ls_table_init(struct port_group_ls_table *); +void port_group_ls_table_clear(struct port_group_ls_table *); +void port_group_ls_table_destroy(struct port_group_ls_table *); + +struct port_group_ls_record *port_group_ls_table_find( + const struct port_group_ls_table *, + const struct nbrec_port_group *); + +void ls_port_group_table_build( + struct ls_port_group_table *ls_port_groups, + struct port_group_ls_table *port_group_lses, + const struct nbrec_port_group_table *, + const struct hmap *ls_ports); void ls_port_group_table_sync(const struct ls_port_group_table *ls_port_groups, const struct sbrec_port_group_table *, struct ovsdb_idl_txn *ovnsb_txn); @@ -75,10 +100,15 @@ 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; }; void *en_port_group_init(struct engine_node *, struct engine_arg *); void en_port_group_cleanup(void *data); +void en_port_group_clear_tracked_data(void *data); void en_port_group_run(struct engine_node *, void *data); +bool port_group_nb_port_group_handler(struct engine_node *, void *data); + #endif /* EN_PORT_GROUP_H */ diff --git a/northd/inc-proc-northd.c b/northd/inc-proc-northd.c index f807e2bae6..02d6441139 100644 --- a/northd/inc-proc-northd.c +++ b/northd/inc-proc-northd.c @@ -139,7 +139,7 @@ static ENGINE_NODE(northd_output, "northd_output"); static ENGINE_NODE(sync_meters, "sync_meters"); static ENGINE_NODE(sync_to_sb, "sync_to_sb"); static ENGINE_NODE(sync_to_sb_addr_set, "sync_to_sb_addr_set"); -static ENGINE_NODE(port_group, "port_group"); +static ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_group, "port_group"); static ENGINE_NODE(fdb_aging, "fdb_aging"); static ENGINE_NODE(fdb_aging_waker, "fdb_aging_waker"); @@ -199,7 +199,7 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_lflow, &en_sb_multicast_group, NULL); engine_add_input(&en_lflow, &en_sb_igmp_group, NULL); engine_add_input(&en_lflow, &en_northd, lflow_northd_handler); - engine_add_input(&en_lflow, &en_port_group, NULL); + engine_add_input(&en_lflow, &en_port_group, lflow_port_group_handler); engine_add_input(&en_sync_to_sb_addr_set, &en_nb_address_set, sync_to_sb_addr_set_nb_address_set_handler); @@ -208,7 +208,8 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, engine_add_input(&en_sync_to_sb_addr_set, &en_northd, NULL); engine_add_input(&en_sync_to_sb_addr_set, &en_sb_address_set, NULL); - engine_add_input(&en_port_group, &en_nb_port_group, NULL); + engine_add_input(&en_port_group, &en_nb_port_group, + port_group_nb_port_group_handler); engine_add_input(&en_port_group, &en_sb_port_group, NULL); /* No need for an explicit handler for northd changes. Port changes * that affect port_groups trigger updates to the NB.Port_Group @@ -294,6 +295,12 @@ void inc_proc_northd_init(struct ovsdb_idl_loop *nb, "sbrec_address_set_by_name", sbrec_address_set_by_name); + struct ovsdb_idl_index *sbrec_port_group_by_name + = ovsdb_idl_index_create1(sb->idl, &sbrec_port_group_col_name); + engine_ovsdb_node_add_index(&en_sb_port_group, + "sbrec_port_group_by_name", + sbrec_port_group_by_name); + struct ovsdb_idl_index *sbrec_fdb_by_dp_and_port = ovsdb_idl_index_create2(sb->idl, &sbrec_fdb_col_dp_key, &sbrec_fdb_col_port_key); diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c index 060c6407bc..2050aee9a8 100644 --- a/northd/ovn-northd.c +++ b/northd/ovn-northd.c @@ -844,6 +844,10 @@ main(int argc, char *argv[]) for (size_t i = 0; i < SBREC_METER_N_COLUMNS; i++) { ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, &sbrec_meter_columns[i]); } + for (size_t i = 0; i < SBREC_PORT_GROUP_N_COLUMNS; i++) { + ovsdb_idl_omit_alert(ovnsb_idl_loop.idl, + &sbrec_port_group_columns[i]); + } unixctl_command_register("sb-connection-status", "", 0, 0, ovn_conn_show, ovnsb_idl_loop.idl); diff --git a/tests/ovn-northd.at b/tests/ovn-northd.at index 78141ab59e..3ef92bb3ff 100644 --- a/tests/ovn-northd.at +++ b/tests/ovn-northd.at @@ -11,6 +11,7 @@ m4_define([_DUMP_DB_TABLES], [ ovn-sbctl list address_set >> $1 ovn-sbctl list meter >> $1 ovn-sbctl list meter_band >> $1 + ovn-sbctl list port_group_set >> $1 ]) # CHECK_NO_CHANGE_AFTER_RECOMPUTE @@ -8962,6 +8963,376 @@ AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats sync_to_sb_a AT_CLEANUP ]) +OVN_FOR_EACH_NORTHD_NO_HV([ +AT_SETUP([Port group incremental processing]) +ovn_start + +check ovn-nbctl ls-add sw1 \ + -- lsp-add sw1 sw1.1 \ + -- lsp-add sw1 sw1.2 \ + -- lsp-add sw1 sw1.3 \ + -- ls-add sw2 \ + -- lsp-add sw2 sw2.1 \ + -- lsp-add sw2 sw2.2 \ + -- lsp-add sw2 sw2.3 + +check ovn-nbctl --wait=sb sync +sw1_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=sw1) +sw2_key=$(fetch_column Datapath_Binding tunnel_key external_ids:name=sw2) + +check_acl_lflows() { +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list sw1 | grep ls_in_acl_eval | grep eth.src==41:41:41:41:41:41 -c], [ignore], [dnl +$1 +]) +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list sw1 | grep ls_in_acl_eval | grep eth.src==42:42:42:42:42:42 -c], [ignore], [dnl +$2 +]) +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list sw2 | grep ls_in_acl_eval | grep eth.src==41:41:41:41:41:41 -c], [ignore], [dnl +$3 +]) +AT_CHECK_UNQUOTED([ovn-sbctl lflow-list sw2 | grep ls_in_acl_eval | grep eth.src==42:42:42:42:42:42 -c], [ignore], [dnl +$4 +]) +} + +AS_BOX([Create new PG1 and PG2]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb -- pg-add pg1 -- pg-add 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 NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 0 +]) +dnl The port_group node recomputes every time a NB port group is added/deleted. +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Add ACLs on PG1 and PG2]) +check ovn-nbctl --wait=sb \ + -- 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 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Add one port from the two switches to PG1]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb \ + -- pg-set-ports pg1 sw1.1 sw2.1 +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, it should handle nb_global update +dnl though, therefore "compute: 1". +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +dnl Expect ACL1 on sw1 and sw2 +check_acl_lflows 1 0 1 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Add one port from the two switches to PG2]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb \ + -- 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" + +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 NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +dnl Expect both ACLs on sw1 and sw2 +check_acl_lflows 1 1 1 1 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Add one more port from the two switches to PG1 and PG2]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb \ + -- pg-set-ports pg1 sw1.1 sw2.1 sw1.3 sw2.3 \ + -- pg-set-ports pg2 sw1.2 sw2.2 sw1.3 sw2.3 +check_column "sw1.1 sw1.3" sb:Port_Group ports name="${sw1_key}_pg1" +check_column "sw2.1 sw2.3" sb:Port_Group ports name="${sw2_key}_pg1" +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, it should handle nb_global update +dnl though, therefore "compute: 1". +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 0 +- compute: 1 +- abort: 0 +]) +dnl Expect both ACLs on sw1 and sw2 +check_acl_lflows 1 1 1 1 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Remove the last port from PG1 and PG2]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb \ + -- 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" + +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 NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 0 +- compute: 1 +- abort: 0 +]) +dnl Expect both ACLs on sw1 and sw2 +check_acl_lflows 1 1 1 1 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Remove the second port from PG2]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +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, it should handle nb_global update +dnl though, therefore "compute: 1". +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +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 + +AS_BOX([Remove the second port from PG1]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb pg-set-ports pg1 sw1.1 +check_column "sw1.1" sb:Port_Group ports name="${sw1_key}_pg1" +AT_CHECK([fetch_column sb:Port_Group ports name="${sw2_key}_pg1"], [0], [ +]) +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, it should handle nb_global update +dnl though, therefore "compute: 1". +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +dnl Expect both ACLs on sw1 and not on sw2. +check_acl_lflows 1 1 0 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AS_BOX([Add second port to both PGs]) +check as northd ovn-appctl -t NORTHD_TYPE inc-engine/clear-stats +check ovn-nbctl --wait=sb \ + -- 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" + +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 NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +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 NORTHD_TYPE inc-engine/clear-stats +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" +AT_CHECK([fetch_column sb:Port_Group ports name="${sw2_key}_pg1"], [0], [ +]) +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, it should handle nb_global update +dnl though, therefore "compute: 1". +AT_CHECK([as northd ovn-appctl -t NORTHD_TYPE inc-engine/show-stats northd], [0], [dnl +Node: northd +- recompute: 0 +- compute: 1 +- abort: 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 NORTHD_TYPE inc-engine/show-stats port_group], [0], [dnl +Node: port_group +- recompute: 1 +- compute: 0 +- abort: 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 NORTHD_TYPE inc-engine/show-stats lflow], [0], [dnl +Node: lflow +- recompute: 1 +- compute: 0 +- abort: 0 +]) +dnl Expect both ACLs on sw1 and no ACLs on sw2 +check_acl_lflows 1 1 0 0 +CHECK_NO_CHANGE_AFTER_RECOMPUTE + +AT_CLEANUP +]) + OVN_FOR_EACH_NORTHD([ AT_SETUP([Check default drop]) AT_KEYWORDS([drop]) -- 2.39.3 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
