On 8/22/23 08:58, Ales Musil wrote:
> On Thu, Aug 10, 2023 at 2:45 PM Dumitru Ceara <dce...@redhat.com> wrote:
>
>> 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 <dce...@redhat.com>
>> ---
>>
>
> Hi Dumitru,
>
> I have a couple of comments down below.
>
Hi Ales,
Thanks for the review!
[..]
>> 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)
>> {
>> - struct ls_port_group_record *ls_pg_rec = NULL;
>> + struct ls_port_group_record *ls_pg_rec =
>> + ls_port_group_record_find(ls_pg, nb_pg);
>> size_t hash = uuid_hash(&nb_pg->header_.uuid);
>>
>> - 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;
>> - }
>> + if (!ls_pg_rec) {
>> + ls_pg_rec = xzalloc(sizeof *ls_pg_rec);
>>
>
> nit: No need for zeroed alloc as all the fields are immediately overwritten.
>
Fair, I'll change it to xmalloc().
> + *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);
>> }
>>
>> - ls_pg_rec = xzalloc(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);
>> 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 +372,71 @@ ls_port_group_record_destroy(struct ls_port_group
>> *ls_pg,
>> }
>> }
>>
>> +void
>> +port_group_to_ls_table_init(struct port_group_to_ls_table *table)
>> +{
>> + *table = (struct port_group_to_ls_table) {
>> + .entries = HMAP_INITIALIZER(&table->entries),
>> + };
>> +}
>> +
>> +void
>> +port_group_to_ls_table_clear(struct port_group_to_ls_table *table)
>> +{
>> + struct port_group_to_ls *pg_ls;
>> + HMAP_FOR_EACH_SAFE (pg_ls, key_node, &table->entries) {
>> + port_group_to_ls_destroy(table, pg_ls);
>> + }
>> +}
>> +
>> +void
>> +port_group_to_ls_table_destroy(struct port_group_to_ls_table *table)
>> +{
>> + port_group_to_ls_table_clear(table);
>> + hmap_destroy(&table->entries);
>> +}
>> +
>> +struct port_group_to_ls *
>> +port_group_to_ls_table_find(const struct port_group_to_ls_table *table,
>> + const struct nbrec_port_group *nb_pg)
>> +{
>> + struct port_group_to_ls *pg_ls;
>> +
>> + HMAP_FOR_EACH_WITH_HASH (pg_ls, key_node,
>> uuid_hash(&nb_pg->header_.uuid),
>> + &table->entries) {
>>
>
> We should move the uuid_hash call outside the loop.
>
>
As Han pointed out, that's not really needed. I'll leave it as is.
Thanks,
Dumitru
>> + if (nb_pg == pg_ls->nb_pg) {
>> + return pg_ls;
>> + }
>> + }
>> + return NULL;
>> +}
>> +
>> +static struct port_group_to_ls *
>> +port_group_to_ls_create(struct port_group_to_ls_table *table,
>> + const struct nbrec_port_group *nb_pg)
>> +{
>> + struct port_group_to_ls *pg_ls = xmalloc(sizeof *pg_ls);
>> +
>> + *pg_ls = (struct port_group_to_ls) {
>> + .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_to_ls_destroy(struct port_group_to_ls_table *table,
>> + struct port_group_to_ls *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 +459,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_to_ls_table_init(&pg_data->port_groups_to_ls);
>> return pg_data;
>> }
>>
>> @@ -268,6 +469,15 @@ en_port_group_cleanup(void *data_)
>> struct port_group_data *data = data_;
>>
>> ls_port_group_table_destroy(&data->ls_port_groups);
>> + port_group_to_ls_table_destroy(&data->port_groups_to_ls);
>> +}
>> +
>> +void
>> +en_port_group_clear_tracked_data(void *data_)
>> +{
>> + struct port_group_data *data = data_;
>> +
>> + data->ls_port_groups_sets_unchanged = false;
>> }
>>
>> void
>> @@ -280,7 +490,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_to_ls_table_clear(&data->port_groups_to_ls);
>> +
>> ls_port_group_table_build(&data->ls_port_groups,
>> + &data->port_groups_to_ls,
>> input_data.nbrec_port_group_table,
>> input_data.ls_ports);
>>
>> @@ -291,3 +504,133 @@ 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);
>> + 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) {
>> + /* Newly created port groups can't be incrementally processed;
>> + * the rest yes. */
>> + if (ls_port_group_process(&data->ls_port_groups,
>> + &data->port_groups_to_ls,
>> + 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 struct sbrec_port_group *sb_pg =
>> + sb_port_group_lookup_by_name(sbrec_port_group_by_name,
>> + ds_cstr(&sb_pg_name));
>> + if (!sb_pg) {
>> + success = false;
>> + break;
>> + }
>> + 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_unchanged = 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 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);
>> +}
>> +
>> +static void
>> +sync_port_group(struct ovsdb_idl_txn *ovnsb_txn, const char *sb_pg_name,
>> + struct sorted_array *ports,
>> + struct shash *sb_port_groups)
>> +{
>> + const struct sbrec_port_group *sb_port_group =
>> + shash_find_and_delete(sb_port_groups, sb_pg_name);
>> + if (!sb_port_group) {
>> + sb_port_group = sbrec_port_group_insert(ovnsb_txn);
>> + sbrec_port_group_set_name(sb_port_group, sb_pg_name);
>> + sbrec_port_group_set_ports(sb_port_group, ports->arr, ports->n);
>> + } else {
>> + update_sb_port_group(ports, sb_port_group);
>> + }
>> +}
>> +
>> +/* 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..c3975f64ee 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_to_ls_table {
>> + struct hmap entries; /* Stores struct port_group_to_ls. */
>> +};
>> +
>> +struct port_group_to_ls {
>> + 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_to_ls_table_init(struct port_group_to_ls_table *);
>> +void port_group_to_ls_table_clear(struct port_group_to_ls_table *);
>> +void port_group_to_ls_table_destroy(struct port_group_to_ls_table *);
>> +
>> +struct port_group_to_ls *port_group_to_ls_table_find(
>> + const struct port_group_to_ls_table *,
>> + const struct nbrec_port_group *);
>> +
>> +void ls_port_group_table_build(
>> + struct ls_port_group_table *ls_port_groups,
>> + struct port_group_to_ls_table *port_group_to_switches,
>> + 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_to_ls_table port_groups_to_ls;
>> + bool ls_port_groups_sets_unchanged;
>> };
>>
>> 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 6d5f9e8d16..bd598ba5e2 100644
>> --- a/northd/inc-proc-northd.c
>> +++ b/northd/inc-proc-northd.c
>> @@ -137,7 +137,7 @@ static ENGINE_NODE(mac_binding_aging_waker,
>> "mac_binding_aging_waker");
>> static ENGINE_NODE(northd_output, "northd_output");
>> 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");
>>
>> @@ -193,7 +193,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);
>> @@ -202,7 +202,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
>> @@ -287,6 +288,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 4fa1b039ea..44385d604c 100644
>> --- a/northd/ovn-northd.c
>> +++ b/northd/ovn-northd.c
>> @@ -836,6 +836,10 @@ main(int argc, char *argv[])
>> ovsdb_idl_omit_alert(ovnsb_idl_loop.idl,
>> &sbrec_multicast_group_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 1a12513d7a..a04ba2b23f 100644
>> --- a/tests/ovn-northd.at
>> +++ b/tests/ovn-northd.at
>> @@ -8936,6 +8936,252 @@ 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
>> +])
>> +
>> +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
>> +
>> +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
>> +
>> +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
>> +
>> +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
>> +
>> +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
>> +
>> +AS_BOX([Remove the second 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 \
>> + -- pg-set-ports pg2 sw1.2
>> +check_column "sw1.1" sb:Port_Group ports name="${sw1_key}_pg1"
>> +check_column "sw1.2" sb:Port_Group ports name="${sw1_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 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 did 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
>> +
>> +AT_CLEANUP
>> +])
>> +
>> OVN_FOR_EACH_NORTHD([
>> AT_SETUP([Check default drop])
>> AT_KEYWORDS([drop])
>>
>>
> With that addressed:
>
> Acked-by: Ales Musil <amu...@redhat.com>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
