On Tue, Apr 27, 2021 at 10:29 AM Han Zhou <[email protected]> wrote: > > > > On Tue, Apr 27, 2021 at 8:43 AM Mark Gray <[email protected]> wrote: > > > > On 22/04/2021 21:14, Han Zhou wrote: > > > For an ACL with match: outport == @PG && ip4.src == $PG_AS, given below > > > scale: > > > > > > P: PG size > > > LP: number of local lports > > > D: number of all datapaths (logical switches) > > > LD: number of datapaths that contain local lports > > > > > > With current OVN implementation, the total number of OF flows is: > > > LP + (P * D) + D > > > > > > The reason is, firstly, datapath is not part of the conjunction, so for > > > each datapath the lflow is reparsed. > > > > > > Secondly, although ovn-controller tries to filter out the flows that are > > > for non-local lports, with the conjunction match, the logic that filters > > > out non-local flows doesn't work for the conjunction part that doesn't > > > have the lport in the match (the P * D part). When there is only one > > > port on each LS it is fine, because no conjunction will be used because > > > SB port groups are splited per datapath, so each port group would have > > suggest "split per datapath" > > Ack > > > > only one port. However, when more than one ports are on each LS the flow > > > explosion happens. > > > > > > This patch deal with the second reason above, by refining the SB port > > > groups to store only locally bound lports: empty const sets will not > > > generate any flows. This reduces the related flow number from > > > LP + (P * D) + D to LP + (P * LD) + LD. > > > > > > Since LD is expected to be small, so even if it is a multiplier, the > > > total number is larged reduced. In particular, in ovn-k8s use cases the > > suggest "reduced significantly" > > Ack > > > > LD is always 1, so the formular above becomes LP + P + LD. > > > > > s/formular/formula > > Ack > > > > With a scale of 1k k8s nodes, each has 4 ports for the same PG: P = 4k, > > > LP = 4, D = 1k, LD = 1. The current implementation generates ~4m flows. > > > With this patch it becomes only ~4k. > > Cool! > > > > > > Reported-by: Girish Moodalbail <[email protected]> > > > Reported-at: https://mail.openvswitch.org/pipermail/ovs-dev/2021-March/381082.html > > > Reported-by: Dumitru Ceara <[email protected]> > > > Reported-at: https://bugzilla.redhat.com/show_bug.cgi?id=1944098 > > > Signed-off-by: Han Zhou <[email protected]> > > > > I tested this as well and it seemed to work as expected. > > Thanks for the test! > > > > --- > > > v1->v2: fix memory leaks found by address sanitizer > > > > > > controller/binding.c | 20 ++++ > > > controller/binding.h | 9 ++ > > > controller/ovn-controller.c | 217 ++++++++++++++++++++++++++++++------ > > > include/ovn/expr.h | 2 +- > > > lib/expr.c | 8 +- > > > tests/ovn.at | 53 +++++++++ > > > tests/test-ovn.c | 12 +- > > > utilities/ovn-trace.c | 4 +- > > > 8 files changed, 283 insertions(+), 42 deletions(-) > > > > > > diff --git a/controller/binding.c b/controller/binding.c > > > index 514f5f33f..5aca964cc 100644 > > > --- a/controller/binding.c > > > +++ b/controller/binding.c > > > @@ -2987,3 +2987,23 @@ cleanup: > > > > > > return b_lport; > > > } > > > + > > > +struct sset * > > > +binding_collect_local_binding_lports(struct local_binding_data *lbinding_data) > > > +{ > > > + struct sset *lports = xzalloc(sizeof *lports); > > > + sset_init(lports); > > > + struct shash_node *shash_node; > > > + SHASH_FOR_EACH (shash_node, &lbinding_data->lports) { > > > + struct binding_lport *b_lport = shash_node->data; > > > + sset_add(lports, b_lport->name); > > > + } > > > + return lports; > > > +} > > > + > > > +void > > > +binding_destroy_local_binding_lports(struct sset *lports) > > > +{ > > > + sset_destroy(lports); > > > + free(lports); > > > +} > > > diff --git a/controller/binding.h b/controller/binding.h > > > index 4fc9ef207..31f0352a0 100644 > > > --- a/controller/binding.h > > > +++ b/controller/binding.h > > > @@ -128,4 +128,13 @@ void binding_seqno_run(struct local_binding_data *lbinding_data); > > > void binding_seqno_install(struct local_binding_data *lbinding_data); > > > void binding_seqno_flush(void); > > > void binding_dump_local_bindings(struct local_binding_data *, struct ds *); > > > + > > > +/* Generates a sset of lport names from local_binding_data. > > > + * Note: the caller is responsible for destroying and freeing the returned > > > + * sset, by calling binding_collect_local_binding_lports(). */ > > I think this^ should say binding_destroy_local_binding_lports()? > > Oops. My bad. > > > > +struct sset *binding_collect_local_binding_lports(struct local_binding_data *); > > > + > > > +/* Destroy and free the lports sset returned by > > > + * binding_collect_local_binding_lports(). */ > > > +void binding_destroy_local_binding_lports(struct sset *lports); > > > #endif /* controller/binding.h */ > > > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > > > index 7320bd56c..c6ba9ff88 100644 > > > --- a/controller/ovn-controller.c > > > +++ b/controller/ovn-controller.c > > > @@ -1341,7 +1341,7 @@ addr_sets_init(const struct sbrec_address_set_table *address_set_table, > > > SBREC_ADDRESS_SET_TABLE_FOR_EACH (as, address_set_table) { > > > expr_const_sets_add(addr_sets, as->name, > > > (const char *const *) as->addresses, > > > - as->n_addresses, true); > > > + as->n_addresses, true, NULL); > > > } > > > } > > > > > > @@ -1358,7 +1358,7 @@ addr_sets_update(const struct sbrec_address_set_table *address_set_table, > > > } else { > > > expr_const_sets_add(addr_sets, as->name, > > > (const char *const *) as->addresses, > > > - as->n_addresses, true); > > > + as->n_addresses, true, NULL); > > > if (sbrec_address_set_is_new(as)) { > > > sset_add(new, as->name); > > > } else { > > > @@ -1367,6 +1367,7 @@ addr_sets_update(const struct sbrec_address_set_table *address_set_table, > > > } > > > } > > > } > > > + > > > static void > > > en_addr_sets_run(struct engine_node *node, void *data) > > > { > > > @@ -1415,20 +1416,72 @@ addr_sets_sb_address_set_handler(struct engine_node *node, void *data) > > > } > > > > > > struct ed_type_port_groups{ > > > - struct shash port_groups; > > > + /* A copy of SB port_groups, each converted as a sset for efficient lport > > > + * lookup. */ > > > + struct shash port_group_ssets; > > > + > > > + /* Const sets containing local lports, used for expr parsing. */ > > > + struct shash port_groups_cs_local; > > > + > > > bool change_tracked; > > > struct sset new; > > > struct sset deleted; > > > struct sset updated; > > > }; > > > > > > +static void > > > +port_group_ssets_add_or_update(struct shash *port_group_ssets, > > > + const struct sbrec_port_group *pg) > > > +{ > > > + struct sset *lports = shash_find_data(port_group_ssets, pg->name); > > > + if (lports) { > > > + sset_clear(lports); > > > + } else { > > > + lports = xzalloc(sizeof *lports); > > > + sset_init(lports); > > > + shash_add(port_group_ssets, pg->name, lports); > > > + } > > > + > > > + for (size_t i = 0; i < pg->n_ports; i++) { > > > + sset_add(lports, pg->ports[i]); > > > + } > > > +} > > > + > > > +static void > > > +port_group_ssets_delete(struct shash *port_group_ssets, > > > + const char *pg_name) > > > +{ > > > + struct shash_node *node = shash_find(port_group_ssets, pg_name); > > > + if (node) { > > > + struct sset *lports = node->data; > > > + shash_delete(port_group_ssets, node); > > > + sset_destroy(lports); > > > + free(lports); > > > + } > > > +} > > > + > > > +/* Delete and free all ssets in port_group_ssets, but not > > > + * destroying the shash itself. */ > > > +static void > > > +port_group_ssets_clear(struct shash *port_group_ssets) > > > +{ > > > + struct shash_node *node, *next; > > > + SHASH_FOR_EACH_SAFE (node, next, port_group_ssets) { > > > + struct sset *lports = node->data; > > > + shash_delete(port_group_ssets, node); > > > + sset_destroy(lports); > > > + free(lports); > > > + } > > > +} > > > + > > > static void * > > > en_port_groups_init(struct engine_node *node OVS_UNUSED, > > > struct engine_arg *arg OVS_UNUSED) > > > { > > > struct ed_type_port_groups *pg = xzalloc(sizeof *pg); > > > > > > - shash_init(&pg->port_groups); > > > + shash_init(&pg->port_group_ssets); > > > + shash_init(&pg->port_groups_cs_local); > > > pg->change_tracked = false; > > > sset_init(&pg->new); > > > sset_init(&pg->deleted); > > > @@ -1440,41 +1493,52 @@ static void > > > en_port_groups_cleanup(void *data) > > > { > > > struct ed_type_port_groups *pg = data; > > > - expr_const_sets_destroy(&pg->port_groups); > > > - shash_destroy(&pg->port_groups); > > > + > > > + expr_const_sets_destroy(&pg->port_groups_cs_local); > > > + shash_destroy(&pg->port_groups_cs_local); > > > + > > > + port_group_ssets_clear(&pg->port_group_ssets); > > > + shash_destroy(&pg->port_group_ssets); > > > + > > > sset_destroy(&pg->new); > > > sset_destroy(&pg->deleted); > > > sset_destroy(&pg->updated); > > > } > > > > > > -/* Iterate port groups in the southbound database. Create and update the > > > - * corresponding symtab entries as necessary. */ > > > - static void > > > +static void > > > port_groups_init(const struct sbrec_port_group_table *port_group_table, > > > - struct shash *port_groups) > > > + const struct sset *local_lports, > > > + struct shash *port_group_ssets, > > > + struct shash *port_groups_cs_local) > > > { > > > const struct sbrec_port_group *pg; > > > SBREC_PORT_GROUP_TABLE_FOR_EACH (pg, port_group_table) { > > > - expr_const_sets_add(port_groups, pg->name, > > > + port_group_ssets_add_or_update(port_group_ssets, pg); > > > + expr_const_sets_add(port_groups_cs_local, pg->name, > > > (const char *const *) pg->ports, > > > - pg->n_ports, false); > > > + pg->n_ports, false, local_lports); > > > } > > > } > > > > > > static void > > > port_groups_update(const struct sbrec_port_group_table *port_group_table, > > > - struct shash *port_groups, struct sset *new, > > > - struct sset *deleted, struct sset *updated) > > > + const struct sset *local_lports, > > > + struct shash *port_group_ssets, > > > + struct shash *port_groups_cs_local, > > > + struct sset *new, struct sset *deleted, > > > + struct sset *updated) > > > { > > > const struct sbrec_port_group *pg; > > > SBREC_PORT_GROUP_TABLE_FOR_EACH_TRACKED (pg, port_group_table) { > > > if (sbrec_port_group_is_deleted(pg)) { > > > - expr_const_sets_remove(port_groups, pg->name); > > > + expr_const_sets_remove(port_groups_cs_local, pg->name); > > > + port_group_ssets_delete(port_group_ssets, pg->name); > > > sset_add(deleted, pg->name); > > > } else { > > > - expr_const_sets_add(port_groups, pg->name, > > > + expr_const_sets_add(port_groups_cs_local, pg->name, > > > (const char *const *) pg->ports, > > > - pg->n_ports, false); > > > + pg->n_ports, false, local_lports); > > > + port_group_ssets_add_or_update(port_group_ssets, pg); > > > > Bit of a nit: maybe the port_group_ssets_() suite of functions could > > follow the same pattern as expr_const_sets_() by s/ssets/sets and > > changing the verbs. It keeps it consistent and is basically working on > > the same type of data sructure. > > > > e.g. > > > > port_group_sets_add() > > port_group_sets_remove() > > port_group_sets_destroy() > > > > port_group_ssets is different from expr_const_sets. port_groups_ssets is a struct shash with each value item being a struct sset. expr_const_sets is also a struct shash but each item is an array of strings. I think different naming patterns would help remind that they are different. > > > > if (sbrec_port_group_is_new(pg)) { > > > sset_add(new, pg->name); > > > } else { > > > @@ -1485,22 +1549,36 @@ port_groups_update(const struct sbrec_port_group_table *port_group_table, > > > } > > > > > > static void > > > -en_port_groups_run(struct engine_node *node, void *data) > > > +en_port_groups_clear_tracked_data(void *data_) > > > { > > > - struct ed_type_port_groups *pg = data; > > > - > > > + struct ed_type_port_groups *pg = data_; > > > sset_clear(&pg->new); > > > sset_clear(&pg->deleted); > > > sset_clear(&pg->updated); > > > - expr_const_sets_destroy(&pg->port_groups); > > > + pg->change_tracked = false; > > > +} > > > + > > > +static void > > > +en_port_groups_run(struct engine_node *node, void *data) > > > +{ > > > + struct ed_type_port_groups *pg = data; > > > + > > > + expr_const_sets_destroy(&pg->port_groups_cs_local); > > > + port_group_ssets_clear(&pg->port_group_ssets); > > > > > > struct sbrec_port_group_table *pg_table = > > > (struct sbrec_port_group_table *)EN_OVSDB_GET( > > > engine_get_input("SB_port_group", node)); > > > > > > - port_groups_init(pg_table, &pg->port_groups); > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + > > > + struct sset *local_b_lports = binding_collect_local_binding_lports( > > > + &rt_data->lbinding_data); > > > + port_groups_init(pg_table, local_b_lports, &pg->port_group_ssets, > > > + &pg->port_groups_cs_local); > > > + binding_destroy_local_binding_lports(local_b_lports); > > > > > > - pg->change_tracked = false; > > > engine_set_node_state(node, EN_UPDATED); > > > } > > > > > > @@ -1509,16 +1587,19 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > > { > > > struct ed_type_port_groups *pg = data; > > > > > > - sset_clear(&pg->new); > > > - sset_clear(&pg->deleted); > > > - sset_clear(&pg->updated); > > > - > > > struct sbrec_port_group_table *pg_table = > > > (struct sbrec_port_group_table *)EN_OVSDB_GET( > > > engine_get_input("SB_port_group", node)); > > > > > > - port_groups_update(pg_table, &pg->port_groups, &pg->new, > > > - &pg->deleted, &pg->updated); > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + > > > + struct sset *local_b_lports = binding_collect_local_binding_lports( > > > + &rt_data->lbinding_data); > > > + port_groups_update(pg_table, local_b_lports, &pg->port_group_ssets, > > > + &pg->port_groups_cs_local, &pg->new, &pg->deleted, > > > + &pg->updated); > > > + binding_destroy_local_binding_lports(local_b_lports); > > > > > > if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > > !sset_is_empty(&pg->updated)) { > > > @@ -1531,6 +1612,74 @@ port_groups_sb_port_group_handler(struct engine_node *node, void *data) > > > return true; > > > } > > > > > > +static bool > > > +port_groups_runtime_data_handler(struct engine_node *node, void *data) > > > +{ > > > + struct ed_type_port_groups *pg = data; > > > + > > > + struct sbrec_port_group_table *pg_table = > > > + (struct sbrec_port_group_table *)EN_OVSDB_GET( > > > + engine_get_input("SB_port_group", node)); > > > + > > > + struct ed_type_runtime_data *rt_data = > > > + engine_get_input_data("runtime_data", node); > > > + > > > + if (!rt_data->tracked) { > > > + return false; > > > + } > > > + > > > + if (hmap_is_empty(&rt_data->tracked_dp_bindings)) { > > > + goto out; > > > + } > > > + > > > + struct sset *local_b_lports = binding_collect_local_binding_lports( > > > + &rt_data->lbinding_data); > > > + > > > + const struct sbrec_port_group *pg_sb; > > > + SBREC_PORT_GROUP_TABLE_FOR_EACH (pg_sb, pg_table) { > > > + > > Remove blank line^ > > Ack > > > > + struct sset *pg_lports = shash_find_data(&pg->port_group_ssets, > > > + pg_sb->name); > > > > Correct me if I am wrong but I think this^ is the only reason you need > > the 'port_group_ssets' data structure? Perhaps you could remove the need > > for it by implementing expr_const_sets_find() function? It would > > simplify the code a lot as you would only need 'port_groups_cs_local'. > > If you could do this, you could also rename that to 'port_groups_local' > > In fact, what's more important of the usage of "port_group_ssets" is the check in the below loop: > > if (sset_contains(pg_lports, lport->pb->logical_port)) { ... > > It is to quickly check if a local port binding is affecting that PG. Without the "port_group_ssets" we will have to do a O(n) iteration of each lport of a PG. This is also briefly mentioned in the definition: > > /* A copy of SB port_groups, each converted as a sset for efficient lport > * lookup. */ > struct shash port_group_ssets; > > It can't be replaced by 'expr_const_sets_find' because they contain different information (local lports v.s. all lports) and also different data structure (array v.s. sset). > > > > + ovs_assert(pg_lports); > > > + > > > + struct tracked_binding_datapath *tdp; > > > + bool need_update = false; > > > + HMAP_FOR_EACH (tdp, node, &rt_data->tracked_dp_bindings) { > > > + struct shash_node *shash_node; > > > + SHASH_FOR_EACH (shash_node, &tdp->lports) { > > > + struct tracked_binding_lport *lport = shash_node->data; > > > + if (sset_contains(pg_lports, lport->pb->logical_port)) { > > > + /* At least one local port-binding change is related to the > > > + * port_group, so the port_group_cs_local needs update. */ > > > + need_update = true; > > > + break; > > > + } > > > + } > > > + if (need_update) { > > > + break; > > > + } > > > + } > > > + if (need_update) { > > > + expr_const_sets_add(&pg->port_groups_cs_local, pg_sb->name, > > > + (const char *const *) pg_sb->ports, > > > + pg_sb->n_ports, false, local_b_lports); > > > + sset_add(&pg->updated, pg_sb->name); > > > + } > > > + } > > > + > > > + binding_destroy_local_binding_lports(local_b_lports); > > > + > > > +out: > > > + if (!sset_is_empty(&pg->new) || !sset_is_empty(&pg->deleted) || > > > + !sset_is_empty(&pg->updated)) { > > > + engine_set_node_state(node, EN_UPDATED); > > > + } else { > > > + engine_set_node_state(node, EN_UNCHANGED); > > > + } > > > + pg->change_tracked = true; > > > + return true; > > > +} > > > + > > > /* Connection tracking zones. */ > > > struct ed_type_ct_zones { > > > unsigned long bitmap[BITMAP_N_LONGS(MAX_CT_ZONES)]; > > > @@ -1880,7 +2029,7 @@ static void init_lflow_ctx(struct engine_node *node, > > > > > > struct ed_type_port_groups *pg_data = > > > engine_get_input_data("port_groups", node); > > > - struct shash *port_groups = &pg_data->port_groups; > > > + struct shash *port_groups = &pg_data->port_groups_cs_local; > > > > > > l_ctx_in->sbrec_multicast_group_by_name_datapath = > > > sbrec_mc_group_by_name_dp; > > > @@ -2540,7 +2689,7 @@ main(int argc, char *argv[]) > > > "physical_flow_changes"); > > > ENGINE_NODE(flow_output, "flow_output"); > > > ENGINE_NODE(addr_sets, "addr_sets"); > > > - ENGINE_NODE(port_groups, "port_groups"); > > > + ENGINE_NODE_WITH_CLEAR_TRACK_DATA(port_groups, "port_groups"); > > > > > > #define SB_NODE(NAME, NAME_STR) ENGINE_NODE_SB(NAME, NAME_STR); > > > SB_NODES > > > @@ -2556,6 +2705,10 @@ main(int argc, char *argv[]) > > > addr_sets_sb_address_set_handler); > > > engine_add_input(&en_port_groups, &en_sb_port_group, > > > port_groups_sb_port_group_handler); > > > + /* port_groups computation requires runtime_data's lbinding_data for the > > > + * locally bound ports. */ > > > + engine_add_input(&en_port_groups, &en_runtime_data, > > > + port_groups_runtime_data_handler); > > > > > > /* Engine node physical_flow_changes indicates whether > > > * we can recompute only physical flows or we can > > > @@ -2986,7 +3139,7 @@ main(int argc, char *argv[]) > > > engine_get_data(&en_port_groups); > > > if (br_int && chassis && as_data && pg_data) { > > > char *error = ofctrl_inject_pkt(br_int, pending_pkt.flow_s, > > > - &as_data->addr_sets, &pg_data->port_groups); > > > + &as_data->addr_sets, &pg_data->port_groups_cs_local); > > > if (error) { > > > unixctl_command_reply_error(pending_pkt.conn, error); > > > free(error); > > > diff --git a/include/ovn/expr.h b/include/ovn/expr.h > > > index 032370058..2f7ecbc9c 100644 > > > --- a/include/ovn/expr.h > > > +++ b/include/ovn/expr.h > > > @@ -547,7 +547,7 @@ void expr_constant_set_destroy(struct expr_constant_set *cs); > > > > > > void expr_const_sets_add(struct shash *const_sets, const char *name, > > > const char * const *values, size_t n_values, > > > - bool convert_to_integer); > > > + bool convert_to_integer, const struct sset *filter); > > > void expr_const_sets_remove(struct shash *const_sets, const char *name); > > > void expr_const_sets_destroy(struct shash *const_sets); > > > > > > diff --git a/lib/expr.c b/lib/expr.c > > > index f061a8fbe..b42d78d3e 100644 > > > --- a/lib/expr.c > > > +++ b/lib/expr.c > > > @@ -1065,7 +1065,7 @@ expr_constant_set_destroy(struct expr_constant_set *cs) > > > void > > > > > expr_const_sets_add(struct shash *const_sets, const char *name, > > > const char *const *values, size_t n_values, > > > - bool convert_to_integer) > > > + bool convert_to_integer, const struct sset *filter) > > > { > > > /* Replace any existing entry for this name. */ > > > expr_const_sets_remove(const_sets, name); > > > @@ -1075,6 +1075,7 @@ expr_const_sets_add(struct shash *const_sets, const char *name, > > > cs->n_values = 0; > > > cs->values = xmalloc(n_values * sizeof *cs->values); > > > if (convert_to_integer) { > > > + ovs_assert(!filter); > > Because the parameters of this function do not apply to both the integer > > and the string case, this implies all the callers must know if they are > > dealing with integers or strings. I'm wondering should this function be > > split into something like: > > > > expr_const_sets_add_ints() > > expr_const_sets_add_strings() > > > > and maybe even: > > > > expr_const_sets_add_filtered_strings() > > > > I just looked through the code and this seems to be the case - in fact > > 'convert_to_integer' basically means that this is an integer. What do > > you think? > > > > Agree. I thought about the same thing but was lazy :). I can change it in the next version of the patch. > > > > cs->type = EXPR_C_INTEGER; > > > for (size_t i = 0; i < n_values; i++) { > > > /* Use the lexer to convert each constant set into the proper > > > @@ -1100,6 +1101,11 @@ expr_const_sets_add(struct shash *const_sets, const char *name, > > > } else { > > > cs->type = EXPR_C_STRING; > > > for (size_t i = 0; i < n_values; i++) { > > > + if (filter && !sset_find(filter, values[i])) { > > > + VLOG_DBG("Skip constant set entry '%s' for '%s'", > > > + values[i], name); > > > + continue; > > > + } > > > union expr_constant *c = &cs->values[cs->n_values++]; > > > c->string = xstrdup(values[i]); > > > } > > > diff --git a/tests/ovn.at b/tests/ovn.at > > > index 55444bbd7..aa1d1cf17 100644 > > > --- a/tests/ovn.at > > > +++ b/tests/ovn.at > > > @@ -26265,3 +26265,56 @@ primary lport : [[sw0p2]] > > > OVN_CLEANUP([hv1], [hv2]) > > > AT_CLEANUP > > > ]) > > > + > > > +# Tests the efficiency of conjunction flow generation when port groups are used > > > +# by ACLs. Make sure there is no open flow explosion in table 45 for an ACL > > > +# with self-referencing match condition of a port group: > > > +# > > > +# "outport == @pg1 && ip4.src == $pg1_ip4" > > > +# > > > +# 10 LSes (ls[0-9]), each has 2 LSPs (lsp[0-9][01]). All LSPs belong to port > > > +# group pg1, but only LSPs of LS0 are bound on HV1. > > > +# > > > +# The expected number of conjunction flows is 2 + 20 = 22. > > > > A brief explanation of why that number is expected should help our > > future selves. > > Ack > > > > +OVN_FOR_EACH_NORTHD([ > > > +AT_SETUP([ovn -- ACL with Port Group conjunction flow efficiency]) > > > +ovn_start > > > + > > > +net_add n1 > > > + > > > +sim_add hv1 > > > +as hv1 > > > > Maybe this test should be run with 2 hypervisors as it will catch any > > corner cases that may come up due to the locality of ports and > > datapaths? What do you think? > > > > In fact we have at least another 2 test cases, "Port Group" and "ACLs on Port Groups", both have 3 HVs, so I think the coverage is good. I could have added the flow count check in existing cases but it seems more clear to add a new test case focusing on the flow count efficiency only, so I added this new test case and I think one HV is enough for this purpose. What do you think? > > Thanks again for all your comments. I will update with v3.
Hi Mark, I addressed your comments with v3 here: https://patchwork.ozlabs.org/project/ovn/list/?series=241037 Please take a look. Thanks, Han _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
