Hi Mark, thanks for the testing! It looks great! And thanks for pointing out that the feature enables the conjunctive match. I am glad this helps.
On Thu, Mar 1, 2018 at 3:49 PM, Mark Michelson <[email protected]> wrote: > Hi Han, > > Current criticisms notwithstanding, I decided to test your patch with a > scenario that I thought it would help with. > > The base scenario is that I create an address set with 1000 addresses in > it. I then create logical switch ports and create a new ACL for each port: > > ovn-nbctl acl-add ls0 to-lport 1000 "outport == \"lsp${COUNT}\" && ip4.dst > == \$set1" allow > > The idea is that I'm creating a bunch of ACLs with identical L3 > restrictions but with different ports. I ran this using the 'time' utility, > and here's the results: > > For 100 ports: 2m17.979s > For 200 ports: 9m29.771s > > The big reason for this is that the ACLs generate a lot of flows. For 100 > ports, it generates 102501 flows in table 44 (ACL egress table). For 200 > ports, it generates 205001 flows in that table. Dobuling the number of > ports doubles the number of flows. And doubling the number of flows > quadruples the amount of time it takes to run. > > I applied your patch and modified the scenario a bit. Instead of adding an > ACL for each new port, I instead created one ACL. It looks like: > > ovn-nbctl acl-add ls0 to-lport 1000 "outport == @pg1 && ip4.dst == \$set1" > allow > > For each added port, I add the port to group pg1. Re-running the scenario, > I got pretty much the same results. Honestly, I expected this. > > Now here's the exciting part. I then applied Numan Siddique's conjunctive > match patch on top of yours. I then re-ran the test and got these results: > > For 100 ports: 6.453s > For 200 ports: 16.008s > > Yeah, that's much better! I actually thought I had messed something up at > first, but after double-checking the generated flows, all was correct. With > 100 ports, table 44 has 1127 flows. Rather than requiring 100 ports * 1000 > addresses, it creates a conjunctive match of 100 ports + 1000 addresses. > For 200 ports, table 44 has 1227 flows. It added exactly 100 flows over the > 100 port scenario. > > From a performance perspective, this is great! I'll give the actual code a > review in the upcoming days, hopefully before the weekend. > > > On 02/28/2018 09:37 PM, Han Zhou wrote: > >> This patch enables using port group names in ACL match conditions. >> Users can create a port group in northbound DB Port_Group table, >> and then use the name of the port group in ACL match conditions >> for "inport" or "outport". It can help reduce the number of ACLs >> for CMS clients such as OpenStack Neutron, for the use cases >> where a group of logical ports share same ACL rules except the >> "inport"/"outport" part. Without this patch, the clients have to >> create N (N = number of lports) ACLs, and this patch helps achieve >> the same goal with only one ACL. E.g.: >> >> to-lport 1000 "outport == @port_group1 && ip4.src == {IP1, IP2, ...}" >> allow-related >> >> There was a similar attempt by Zong Kai Li in 2016 [1]. This patch >> takes a slightly different approach by using weak refs instead of >> strings, which requires a new table instead of reusing the address >> set table. This way it will also benefit for a follow up patch that >> enables generating address sets automatically from port groups to >> avoid a lot a trouble from client perspective [2]. >> >> [1] https://mail.openvswitch.org/pipermail/ovs-dev/2016-August/0 >> 77118.html >> [2] https://mail.openvswitch.org/pipermail/ovs-discuss/2018-Febr >> uary/046260.html >> >> Reported-by: Daniel Alvarez Sanchez <[email protected]> >> Reported-at: https://mail.openvswitch.org/pipermail/ovs-discuss/2018-Febr >> uary/046166.html >> Signed-off-by: Han Zhou <[email protected]> >> --- >> NEWS | 2 + >> include/ovn/expr.h | 24 +++++--- >> include/ovn/lex.h | 1 + >> ovn/controller/lflow.c | 16 +++-- >> ovn/controller/lflow.h | 1 + >> ovn/controller/ofctrl.c | 6 +- >> ovn/controller/ofctrl.h | 3 +- >> ovn/controller/ovn-controller.c | 31 +++++++--- >> ovn/lib/actions.c | 3 +- >> ovn/lib/expr.c | 127 ++++++++++++++++++++++++++++-- >> ---------- >> ovn/lib/lex.c | 20 +++++++ >> ovn/northd/ovn-northd.c | 47 +++++++++++++++ >> ovn/ovn-nb.ovsschema | 15 ++++- >> ovn/ovn-nb.xml | 30 ++++++++++ >> ovn/ovn-sb.ovsschema | 10 +++- >> ovn/ovn-sb.xml | 20 +++++++ >> ovn/utilities/ovn-trace.c | 27 +++++++-- >> tests/ovn.at | 34 +++++++++++ >> tests/test-ovn.c | 43 ++++++++++---- >> 19 files changed, 381 insertions(+), 79 deletions(-) >> >> diff --git a/NEWS b/NEWS >> index 4230189..da2c3ec 100644 >> --- a/NEWS >> +++ b/NEWS >> @@ -14,6 +14,8 @@ Post-v2.9.0 >> * OFPT_ROLE_STATUS is now available in OpenFlow 1.3. >> - Linux kernel 4.14 >> * Add support for compiling OVS with the latest Linux 4.14 kernel >> + - OVN: >> + * Port_Group is supported in ACL match conditions. >> v2.9.0 - 19 Feb 2018 >> -------------------- >> diff --git a/include/ovn/expr.h b/include/ovn/expr.h >> index 711713e..3995e62 100644 >> --- a/include/ovn/expr.h >> +++ b/include/ovn/expr.h >> @@ -382,9 +382,11 @@ expr_from_node(const struct ovs_list *node) >> void expr_format(const struct expr *, struct ds *); >> void expr_print(const struct expr *); >> struct expr *expr_parse(struct lexer *, const struct shash *symtab, >> - const struct shash *addr_sets); >> + const struct shash *addr_sets, >> + const struct shash *port_groups); >> struct expr *expr_parse_string(const char *, const struct shash *symtab, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> char **errorp); >> struct expr *expr_clone(struct expr *); >> @@ -404,6 +406,7 @@ bool expr_is_normalized(const struct expr *); >> char *expr_parse_microflow(const char *, const struct shash *symtab, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> bool (*lookup_port)(const void *aux, >> const char *port_name, >> unsigned int *portp), >> @@ -486,19 +489,22 @@ void expr_constant_set_format(const struct >> expr_constant_set *, struct ds *); >> void expr_constant_set_destroy(struct expr_constant_set *cs); >> >> -/* Address sets. >> +/* Constant sets. >> * >> - * Instead of referring to a set of value as: >> + * For example, instead of referring to a set of IP addresses as: >> * {addr1, addr2, ..., addrN} >> * You can register a set of values and refer to them as: >> * $name >> - * The address set entries should all have integer/masked-integer values. >> - * The values that don't qualify are ignored. >> + * >> + * If convert_to_integer is true, the set must contain >> + * integer/masked-integer values. The values that don't qualify >> + * are ignored. >> */ >> -void expr_addr_sets_add(struct shash *addr_sets, const char *name, >> - const char * const *values, size_t n_values); >> -void expr_addr_sets_remove(struct shash *addr_sets, const char *name); >> -void expr_addr_sets_destroy(struct shash *addr_sets); >> +void expr_const_sets_add(struct shash *const_sets, const char *name, >> + const char * const *values, size_t n_values, >> + bool convert_to_integer); >> +void expr_const_sets_remove(struct shash *const_sets, const char *name); >> +void expr_const_sets_destroy(struct shash *const_sets); >> #endif /* ovn/expr.h */ >> diff --git a/include/ovn/lex.h b/include/ovn/lex.h >> index afa4a23..8d55857 100644 >> --- a/include/ovn/lex.h >> +++ b/include/ovn/lex.h >> @@ -37,6 +37,7 @@ enum lex_type { >> LEX_T_INTEGER, /* 12345 or 1.2.3.4 or ::1 or >> 01:02:03:04:05 */ >> LEX_T_MASKED_INTEGER, /* 12345/10 or 1.2.0.0/16 or ::2/127 >> or... */ >> LEX_T_MACRO, /* $NAME */ >> + LEX_T_PORT_GROUP, /* @NAME */ >> LEX_T_ERROR, /* invalid input */ >> /* Bare tokens. */ >> diff --git a/ovn/controller/lflow.c b/ovn/controller/lflow.c >> index df125b1..cc2a020 100644 >> --- a/ovn/controller/lflow.c >> +++ b/ovn/controller/lflow.c >> @@ -70,6 +70,7 @@ static void consider_logical_flow(struct controller_ctx >> *ctx, >> struct hmap *nd_ra_opts, >> uint32_t *conj_id_ofs, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> struct hmap *flow_table, >> struct sset *active_tunnels, >> struct sset *local_lport_ids); >> @@ -149,6 +150,7 @@ add_logical_flows(struct controller_ctx *ctx, >> struct ovn_extend_table *meter_table, >> const struct sbrec_chassis *chassis, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> struct hmap *flow_table, >> struct sset *active_tunnels, >> struct sset *local_lport_ids) >> @@ -179,8 +181,8 @@ add_logical_flows(struct controller_ctx *ctx, >> lflow, local_datapaths, >> group_table, meter_table, chassis, >> &dhcp_opts, &dhcpv6_opts, &nd_ra_opts, >> - &conj_id_ofs, addr_sets, flow_table, >> - active_tunnels, local_lport_ids); >> + &conj_id_ofs, addr_sets, port_groups, >> + flow_table, active_tunnels, >> local_lport_ids); >> } >> dhcp_opts_destroy(&dhcp_opts); >> @@ -201,6 +203,7 @@ consider_logical_flow(struct controller_ctx *ctx, >> struct hmap *nd_ra_opts, >> uint32_t *conj_id_ofs, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> struct hmap *flow_table, >> struct sset *active_tunnels, >> struct sset *local_lport_ids) >> @@ -258,7 +261,8 @@ consider_logical_flow(struct controller_ctx *ctx, >> struct hmap matches; >> struct expr *expr; >> - expr = expr_parse_string(lflow->match, &symtab, addr_sets, >> &error); >> + expr = expr_parse_string(lflow->match, &symtab, addr_sets, >> port_groups, >> + &error); >> if (!error) { >> if (prereqs) { >> expr = expr_combine(EXPR_T_AND, expr, prereqs); >> @@ -450,13 +454,15 @@ lflow_run(struct controller_ctx *ctx, >> struct ovn_extend_table *group_table, >> struct ovn_extend_table *meter_table, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> struct hmap *flow_table, >> struct sset *active_tunnels, >> struct sset *local_lport_ids) >> { >> add_logical_flows(ctx, chassis_index, local_datapaths, >> - group_table, meter_table, chassis, addr_sets, >> flow_table, >> - active_tunnels, local_lport_ids); >> + group_table, meter_table, chassis, addr_sets, >> + port_groups, flow_table, active_tunnels, >> + local_lport_ids); >> add_neighbor_flows(ctx, flow_table); >> } >> diff --git a/ovn/controller/lflow.h b/ovn/controller/lflow.h >> index 22bf534..dcf2fe7 100644 >> --- a/ovn/controller/lflow.h >> +++ b/ovn/controller/lflow.h >> @@ -69,6 +69,7 @@ void lflow_run(struct controller_ctx *, >> struct ovn_extend_table *group_table, >> struct ovn_extend_table *meter_table, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> struct hmap *flow_table, >> struct sset *active_tunnels, >> struct sset *local_lport_ids); >> diff --git a/ovn/controller/ofctrl.c b/ovn/controller/ofctrl.c >> index 8d6d1b6..e3ee6b1 100644 >> --- a/ovn/controller/ofctrl.c >> +++ b/ovn/controller/ofctrl.c >> @@ -1145,7 +1145,8 @@ ofctrl_lookup_port(const void *br_int_, const char >> *port_name, >> * must free(). */ >> char * >> ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, const char >> *flow_s, >> - const struct shash *addr_sets) >> + const struct shash *addr_sets, >> + const struct shash *port_groups) >> { >> int version = rconn_get_version(swconn); >> if (version < 0) { >> @@ -1154,7 +1155,8 @@ ofctrl_inject_pkt(const struct ovsrec_bridge >> *br_int, const char *flow_s, >> struct flow uflow; >> char *error = expr_parse_microflow(flow_s, &symtab, addr_sets, >> - ofctrl_lookup_port, br_int, >> &uflow); >> + port_groups, ofctrl_lookup_port, >> + br_int, &uflow); >> if (error) { >> return error; >> } >> diff --git a/ovn/controller/ofctrl.h b/ovn/controller/ofctrl.h >> index d53bc68..a8b3afc 100644 >> --- a/ovn/controller/ofctrl.h >> +++ b/ovn/controller/ofctrl.h >> @@ -47,7 +47,8 @@ struct ovn_flow *ofctrl_dup_flow(struct ovn_flow >> *source); >> void ofctrl_ct_flush_zone(uint16_t zone_id); >> char *ofctrl_inject_pkt(const struct ovsrec_bridge *br_int, >> - const char *flow_s, const struct shash >> *addr_sets); >> + const char *flow_s, const struct shash >> *addr_sets, >> + const struct shash *port_groups); >> /* Flow table interfaces to the rest of ovn-controller. */ >> void ofctrl_add_flow(struct hmap *desired_flows, uint8_t table_id, >> diff --git a/ovn/controller/ovn-controller.c >> b/ovn/controller/ovn-controller.c >> index 7592bda..492e4fa 100644 >> --- a/ovn/controller/ovn-controller.c >> +++ b/ovn/controller/ovn-controller.c >> @@ -288,9 +288,22 @@ addr_sets_init(struct controller_ctx *ctx, struct >> shash *addr_sets) >> { >> const struct sbrec_address_set *as; >> SBREC_ADDRESS_SET_FOR_EACH (as, ctx->ovnsb_idl) { >> - expr_addr_sets_add(addr_sets, as->name, >> - (const char *const *) as->addresses, >> - as->n_addresses); >> + expr_const_sets_add(addr_sets, as->name, >> + (const char *const *) as->addresses, >> + as->n_addresses, true); >> + } >> +} >> + >> +/* Iterate port groups in the southbound database. Create and update the >> + * corresponding symtab entries as necessary. */ >> +static void >> +port_groups_init(struct controller_ctx *ctx, struct shash *port_groups) >> +{ >> + const struct sbrec_port_group *pg; >> + SBREC_PORT_GROUP_FOR_EACH (pg, ctx->ovnsb_idl) { >> + expr_const_sets_add(port_groups, pg->name, >> + (const char *const *) pg->ports, >> + pg->n_ports, false); >> } >> } >> @@ -696,6 +709,8 @@ main(int argc, char *argv[]) >> if (br_int && chassis) { >> struct shash addr_sets = SHASH_INITIALIZER(&addr_sets); >> addr_sets_init(&ctx, &addr_sets); >> + struct shash port_groups = SHASH_INITIALIZER(&port_groups); >> + port_groups_init(&ctx, &port_groups); >> patch_run(&ctx, br_int, chassis); >> @@ -713,8 +728,8 @@ main(int argc, char *argv[]) >> struct hmap flow_table = >> HMAP_INITIALIZER(&flow_table); >> lflow_run(&ctx, chassis, >> &chassis_index, &local_datapaths, >> &group_table, >> - &meter_table, &addr_sets, &flow_table, >> - &active_tunnels, &local_lport_ids); >> + &meter_table, &addr_sets, &port_groups, >> + &flow_table, &active_tunnels, >> &local_lport_ids); >> if (chassis_id) { >> bfd_run(&ctx, br_int, chassis, &local_datapaths, >> @@ -740,7 +755,7 @@ main(int argc, char *argv[]) >> if (pending_pkt.conn) { >> char *error = ofctrl_inject_pkt(br_int, >> pending_pkt.flow_s, >> - &addr_sets); >> + &port_groups, >> &addr_sets); >> if (error) { >> unixctl_command_reply_error(pending_pkt.conn, >> error); >> free(error); >> @@ -754,8 +769,10 @@ main(int argc, char *argv[]) >> update_sb_monitors(ctx.ovnsb_idl, chassis, >> &local_lports, &local_datapaths); >> - expr_addr_sets_destroy(&addr_sets); >> + expr_const_sets_destroy(&addr_sets); >> shash_destroy(&addr_sets); >> + expr_const_sets_destroy(&port_groups); >> + shash_destroy(&port_groups); >> } >> /* If we haven't handled the pending packet insertion >> diff --git a/ovn/lib/actions.c b/ovn/lib/actions.c >> index fde3bff..8845e77 100644 >> --- a/ovn/lib/actions.c >> +++ b/ovn/lib/actions.c >> @@ -234,7 +234,8 @@ add_prerequisite(struct action_context *ctx, const >> char *prerequisite) >> struct expr *expr; >> char *error; >> - expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, >> &error); >> + expr = expr_parse_string(prerequisite, ctx->pp->symtab, NULL, NULL, >> + &error); >> ovs_assert(!error); >> ctx->prereqs = expr_combine(EXPR_T_AND, ctx->prereqs, expr); >> } >> diff --git a/ovn/lib/expr.c b/ovn/lib/expr.c >> index b0aa672..1a33269 100644 >> --- a/ovn/lib/expr.c >> +++ b/ovn/lib/expr.c >> @@ -464,6 +464,7 @@ struct expr_context { >> struct lexer *lexer; /* Lexer for pulling more tokens. */ >> const struct shash *symtab; /* Symbol table. */ >> const struct shash *addr_sets; /* Address set table. */ >> + const struct shash *port_groups; /* Port groups table. */ >> bool not; /* True inside odd number of NOT >> operators. */ >> }; >> @@ -752,6 +753,36 @@ parse_addr_sets(struct expr_context *ctx, struct >> expr_constant_set *cs, >> } >> static bool >> +parse_port_group(struct expr_context *ctx, struct expr_constant_set *cs, >> + size_t *allocated_values) >> +{ >> + struct expr_constant_set *port_group >> + = (ctx->port_groups >> + ? shash_find_data(ctx->port_groups, ctx->lexer->token.s) >> + : NULL); >> + if (!port_group) { >> + lexer_syntax_error(ctx->lexer, "expecting port group name"); >> + return false; >> + } >> + >> + if (!assign_constant_set_type(ctx, cs, EXPR_C_STRING)) { >> + return false; >> + } >> + >> + size_t n_values = cs->n_values + port_group->n_values; >> + if (n_values >= *allocated_values) { >> + cs->values = xrealloc(cs->values, n_values * sizeof *cs->values); >> + *allocated_values = n_values; >> + } >> + for (size_t i = 0; i < port_group->n_values; i++) { >> + cs->values[cs->n_values++].string = >> + xstrdup(port_group->values[i].string); >> + } >> + >> + return true; >> +} >> + >> +static bool >> parse_constant(struct expr_context *ctx, struct expr_constant_set *cs, >> size_t *allocated_values) >> { >> @@ -788,6 +819,12 @@ parse_constant(struct expr_context *ctx, struct >> expr_constant_set *cs, >> } >> lexer_get(ctx->lexer); >> return true; >> + } else if (ctx->lexer->token.type == LEX_T_PORT_GROUP) { >> + if (!parse_port_group(ctx, cs, allocated_values)) { >> + return false; >> + } >> + lexer_get(ctx->lexer); >> + return true; >> } else { >> lexer_syntax_error(ctx->lexer, "expecting constant"); >> return false; >> @@ -939,65 +976,74 @@ expr_constant_set_destroy(struct expr_constant_set >> *cs) >> } >> } >> -/* Adds an address set named 'name' to 'addr_sets', replacing any >> existing >> - * address set entry with the given name. */ >> +/* Adds an constant set named 'name' to 'const_sets', replacing any >> existing >> + * constant set entry with the given name. */ >> void >> -expr_addr_sets_add(struct shash *addr_sets, const char *name, >> - const char *const *values, size_t n_values) >> +expr_const_sets_add(struct shash *const_sets, const char *name, >> + const char *const *values, size_t n_values, >> + bool convert_to_integer) >> { >> /* Replace any existing entry for this name. */ >> - expr_addr_sets_remove(addr_sets, name); >> + expr_const_sets_remove(const_sets, name); >> struct expr_constant_set *cs = xzalloc(sizeof *cs); >> - cs->type = EXPR_C_INTEGER; >> cs->in_curlies = true; >> cs->n_values = 0; >> cs->values = xmalloc(n_values * sizeof *cs->values); >> - for (size_t i = 0; i < n_values; i++) { >> - /* Use the lexer to convert each address set into the proper >> - * integer format. */ >> - struct lexer lex; >> - lexer_init(&lex, values[i]); >> - lexer_get(&lex); >> - if (lex.token.type != LEX_T_INTEGER >> - && lex.token.type != LEX_T_MASKED_INTEGER) { >> - VLOG_WARN("Invalid address set entry: '%s', token type: %d", >> - values[i], lex.token.type); >> - } else { >> - union expr_constant *c = &cs->values[cs->n_values++]; >> - c->value = lex.token.value; >> - c->format = lex.token.format; >> - c->masked = lex.token.type == LEX_T_MASKED_INTEGER; >> - if (c->masked) { >> - c->mask = lex.token.mask; >> + if (convert_to_integer) { >> + 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 >> + * integer format. */ >> + struct lexer lex; >> + lexer_init(&lex, values[i]); >> + lexer_get(&lex); >> + if (lex.token.type != LEX_T_INTEGER >> + && lex.token.type != LEX_T_MASKED_INTEGER) { >> + VLOG_WARN("Invalid constant set entry: '%s', token type: >> %d", >> + values[i], lex.token.type); >> + } else { >> + union expr_constant *c = &cs->values[cs->n_values++]; >> + c->value = lex.token.value; >> + c->format = lex.token.format; >> + c->masked = lex.token.type == LEX_T_MASKED_INTEGER; >> + if (c->masked) { >> + c->mask = lex.token.mask; >> + } >> } >> + lexer_destroy(&lex); >> + } >> + } else { >> + cs->type = EXPR_C_STRING; >> + for (size_t i = 0; i < n_values; i++) { >> + union expr_constant *c = &cs->values[cs->n_values++]; >> + c->string = xstrdup(values[i]); >> } >> - lexer_destroy(&lex); >> } >> - shash_add(addr_sets, name, cs); >> + shash_add(const_sets, name, cs); >> } >> void >> -expr_addr_sets_remove(struct shash *addr_sets, const char *name) >> +expr_const_sets_remove(struct shash *const_sets, const char *name) >> { >> - struct expr_constant_set *cs = shash_find_and_delete(addr_sets, >> name); >> + struct expr_constant_set *cs = shash_find_and_delete(const_sets, >> name); >> if (cs) { >> expr_constant_set_destroy(cs); >> free(cs); >> } >> } >> -/* Destroy all contents of 'addr_sets'. */ >> +/* Destroy all contents of 'const_sets'. */ >> void >> -expr_addr_sets_destroy(struct shash *addr_sets) >> +expr_const_sets_destroy(struct shash *const_sets) >> { >> struct shash_node *node, *next; >> - SHASH_FOR_EACH_SAFE (node, next, addr_sets) { >> + SHASH_FOR_EACH_SAFE (node, next, const_sets) { >> struct expr_constant_set *cs = node->data; >> - shash_delete(addr_sets, node); >> + shash_delete(const_sets, node); >> expr_constant_set_destroy(cs); >> free(cs); >> } >> @@ -1214,11 +1260,13 @@ expr_parse__(struct expr_context *ctx) >> * lexer->error is NULL. */ >> struct expr * >> expr_parse(struct lexer *lexer, const struct shash *symtab, >> - const struct shash *addr_sets) >> + const struct shash *addr_sets, >> + const struct shash *port_groups) >> { >> struct expr_context ctx = { .lexer = lexer, >> .symtab = symtab, >> - .addr_sets = addr_sets }; >> + .addr_sets = addr_sets, >> + .port_groups = port_groups }; >> return lexer->error ? NULL : expr_parse__(&ctx); >> } >> @@ -1230,13 +1278,15 @@ expr_parse(struct lexer *lexer, const struct >> shash *symtab, >> * error (with free()). */ >> struct expr * >> expr_parse_string(const char *s, const struct shash *symtab, >> - const struct shash *addr_sets, char **errorp) >> + const struct shash *addr_sets, >> + const struct shash *port_groups, >> + char **errorp) >> { >> struct lexer lexer; >> lexer_init(&lexer, s); >> lexer_get(&lexer); >> - struct expr *expr = expr_parse(&lexer, symtab, addr_sets); >> + struct expr *expr = expr_parse(&lexer, symtab, addr_sets, >> port_groups); >> lexer_force_end(&lexer); >> *errorp = lexer_steal_error(&lexer); >> if (*errorp) { >> @@ -1460,7 +1510,7 @@ expr_get_level(const struct expr *expr) >> static enum expr_level >> expr_parse_level(const char *s, const struct shash *symtab, char >> **errorp) >> { >> - struct expr *expr = expr_parse_string(s, symtab, NULL, errorp); >> + struct expr *expr = expr_parse_string(s, symtab, NULL, NULL, errorp); >> enum expr_level level = expr ? expr_get_level(expr) : >> EXPR_L_NOMINAL; >> expr_destroy(expr); >> return level; >> @@ -1618,7 +1668,7 @@ parse_and_annotate(const char *s, const struct >> shash *symtab, >> char *error; >> struct expr *expr; >> - expr = expr_parse_string(s, symtab, NULL, &error); >> + expr = expr_parse_string(s, symtab, NULL, NULL, &error); >> if (expr) { >> expr = expr_annotate__(expr, symtab, nesting, &error); >> } >> @@ -3277,6 +3327,7 @@ expr_parse_microflow__(struct lexer *lexer, >> char * OVS_WARN_UNUSED_RESULT >> expr_parse_microflow(const char *s, const struct shash *symtab, >> const struct shash *addr_sets, >> + const struct shash *port_groups, >> bool (*lookup_port)(const void *aux, >> const char *port_name, >> unsigned int *portp), >> @@ -3286,7 +3337,7 @@ expr_parse_microflow(const char *s, const struct >> shash *symtab, >> lexer_init(&lexer, s); >> lexer_get(&lexer); >> - struct expr *e = expr_parse(&lexer, symtab, addr_sets); >> + struct expr *e = expr_parse(&lexer, symtab, addr_sets, port_groups); >> lexer_force_end(&lexer); >> if (e) { >> diff --git a/ovn/lib/lex.c b/ovn/lib/lex.c >> index 2f49af0..9463640 100644 >> --- a/ovn/lib/lex.c >> +++ b/ovn/lib/lex.c >> @@ -231,6 +231,10 @@ lex_token_format(const struct lex_token *token, >> struct ds *s) >> ds_put_format(s, "$%s", token->s); >> break; >> + case LEX_T_PORT_GROUP: >> + ds_put_format(s, "@%s", token->s); >> + break; >> + >> case LEX_T_LPAREN: >> ds_put_cstr(s, "("); >> break; >> @@ -573,6 +577,18 @@ lex_parse_addr_set(const char *p, struct lex_token >> *token) >> return lex_parse_id(p, LEX_T_MACRO, token); >> } >> +static const char * >> +lex_parse_port_group(const char *p, struct lex_token *token) >> +{ >> + p++; >> + if (!lex_is_id1(*p)) { >> + lex_error(token, "`$' must be followed by a valid identifier."); >> + return p; >> + } >> + >> + return lex_parse_id(p, LEX_T_PORT_GROUP, token); >> +} >> + >> /* Initializes 'token' and parses the first token from the beginning of >> * null-terminated string 'p' into 'token'. Stores a pointer to the >> start of >> * the token (after skipping white space and comments, if any) into >> '*startp'. >> @@ -747,6 +763,10 @@ next: >> p = lex_parse_addr_set(p, token); >> break; >> + case '@': >> + p = lex_parse_port_group(p, token); >> + break; >> + >> case ':': >> if (p[1] != ':') { >> token->type = LEX_T_COLON; >> diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c >> index d83681c..2924d8f 100644 >> --- a/ovn/northd/ovn-northd.c >> +++ b/ovn/northd/ovn-northd.c >> @@ -6135,6 +6135,49 @@ sync_address_sets(struct northd_context *ctx) >> shash_destroy(&sb_address_sets); >> } >> +/* Each port group in Port_Group table in OVN_Northbound has a >> corresponding >> + * entry in Port_Group table in OVN_Southbound. In OVN_Northbound the >> entries >> + * contains lport uuids, while in OVN_Southbound we store the lport >> names. >> + */ >> +static void >> +sync_port_groups(struct northd_context *ctx) >> +{ >> + struct shash sb_port_groups = SHASH_INITIALIZER(&sb_port_groups); >> + >> + const struct sbrec_port_group *sb_port_group; >> + SBREC_PORT_GROUP_FOR_EACH (sb_port_group, ctx->ovnsb_idl) { >> + shash_add(&sb_port_groups, sb_port_group->name, sb_port_group); >> + } >> + >> + const struct nbrec_port_group *nb_port_group; >> + NBREC_PORT_GROUP_FOR_EACH (nb_port_group, ctx->ovnnb_idl) { >> + sb_port_group = shash_find_and_delete(&sb_port_groups, >> + nb_port_group->name); >> + if (!sb_port_group) { >> + sb_port_group = sbrec_port_group_insert(ctx->ovnsb_txn); >> + sbrec_port_group_set_name(sb_port_group, >> nb_port_group->name); >> + } >> + >> + const char **nb_port_names = xcalloc(nb_port_group->n_ports, >> + sizeof *nb_port_names); >> + int i; >> + for (i = 0; i < nb_port_group->n_ports; i++) { >> + nb_port_names[i] = nb_port_group->ports[i]->name; >> + } >> + sbrec_port_group_set_ports(sb_port_group, >> + nb_port_names, >> + nb_port_group->n_ports); >> + free(nb_port_names); >> + } >> + >> + struct shash_node *node, *next; >> + SHASH_FOR_EACH_SAFE (node, next, &sb_port_groups) { >> + sbrec_port_group_delete(node->data); >> + shash_delete(&sb_port_groups, node); >> + } >> + shash_destroy(&sb_port_groups); >> +} >> + >> /* >> * struct 'dns_info' is used to sync the DNS records between OVN >> Northbound db >> * and Southbound db. >> @@ -6251,6 +6294,7 @@ ovnnb_db_run(struct northd_context *ctx, struct >> chassis_index *chassis_index, >> build_lflows(ctx, &datapaths, &ports); >> sync_address_sets(ctx); >> + sync_port_groups(ctx); >> sync_dns_entries(ctx, &datapaths); >> struct ovn_datapath *dp, *next_dp; >> @@ -6848,6 +6892,9 @@ main(int argc, char *argv[]) >> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_address_set); >> add_column_noalert(ovnsb_idl_loop.idl, >> &sbrec_address_set_col_name); >> add_column_noalert(ovnsb_idl_loop.idl, >> &sbrec_address_set_col_addresses); >> + ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_port_group); >> + add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_group_col_name); >> + add_column_noalert(ovnsb_idl_loop.idl, &sbrec_port_group_col_ports); >> ovsdb_idl_add_table(ovnsb_idl_loop.idl, &sbrec_table_dns); >> add_column_noalert(ovnsb_idl_loop.idl, &sbrec_dns_col_datapaths); >> diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema >> index 32f9d5a..2d09282 100644 >> --- a/ovn/ovn-nb.ovsschema >> +++ b/ovn/ovn-nb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_Northbound", >> "version": "5.10.0", >> - "cksum": "626737541 17810", >> + "cksum": "222987318 18430", >> "tables": { >> "NB_Global": { >> "columns": { >> @@ -114,6 +114,19 @@ >> "min": 0, "max": "unlimited"}}}, >> "indexes": [["name"]], >> "isRoot": true}, >> + "Port_Group": { >> + "columns": { >> + "name": {"type": "string"}, >> + "ports": {"type": {"key": {"type": "uuid", >> + "refTable": >> "Logical_Switch_Port", >> + "refType": "weak"}, >> + "min": 0, >> + "max": "unlimited"}}, >> + "external_ids": { >> + "type": {"key": "string", "value": "string", >> + "min": 0, "max": "unlimited"}}}, >> + "indexes": [["name"]], >> + "isRoot": true}, >> "Load_Balancer": { >> "columns": { >> "name": {"type": "string"}, >> diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml >> index b7a5b6b..83727c5 100644 >> --- a/ovn/ovn-nb.xml >> +++ b/ovn/ovn-nb.xml >> @@ -922,6 +922,36 @@ >> </group> >> </table> >> + <table name="Port_Group" title="Port Groups"> >> + <p> >> + Each row in this table represents a named group of logical switch >> ports. >> + </p> >> + >> + <p> >> + Port groups may be used in the <ref column="match" table="ACL"/> >> column >> + of the <ref table="ACL"/> table. For syntax information, see the >> details >> + of the expression language used for the <ref column="match" >> + table="Logical_Flow" db="OVN_Southbound"/> column in the <ref >> + table="Logical_Flow" db="OVN_Southbound"/> table of the <ref >> + db="OVN_Southbound"/> database. >> + </p> >> + >> + <column name="name"> >> + A name for the port group. Names are ASCII and must match >> + <code>[a-zA-Z_.][a-zA-Z_.0-9]*</code>. >> + </column> >> + >> + <column name="ports"> >> + The logical switch ports belonging to the group in uuids. >> + </column> >> + >> + <group title="Common Columns"> >> + <column name="external_ids"> >> + See <em>External IDs</em> at the beginning of this document. >> + </column> >> + </group> >> + </table> >> + >> <table name="Load_Balancer" title="load balancer"> >> <p> >> Each row represents one load balancer. >> diff --git a/ovn/ovn-sb.ovsschema b/ovn/ovn-sb.ovsschema >> index 2abcc6b..9e271d4 100644 >> --- a/ovn/ovn-sb.ovsschema >> +++ b/ovn/ovn-sb.ovsschema >> @@ -1,7 +1,7 @@ >> { >> "name": "OVN_Southbound", >> "version": "1.15.0", >> - "cksum": "70426956 13327", >> + "cksum": "1839738004 13639", >> "tables": { >> "SB_Global": { >> "columns": { >> @@ -55,6 +55,14 @@ >> "max": "unlimited"}}}, >> "indexes": [["name"]], >> "isRoot": true}, >> + "Port_Group": { >> + "columns": { >> + "name": {"type": "string"}, >> + "ports": {"type": {"key": "string", >> + "min": 0, >> + "max": "unlimited"}}}, >> + "indexes": [["name"]], >> + "isRoot": true}, >> "Logical_Flow": { >> "columns": { >> "logical_datapath": {"type": {"key": {"type": "uuid", >> diff --git a/ovn/ovn-sb.xml b/ovn/ovn-sb.xml >> index f000b16..2eac943 100644 >> --- a/ovn/ovn-sb.xml >> +++ b/ovn/ovn-sb.xml >> @@ -377,6 +377,18 @@ >> <column name="addresses"/> >> </table> >> + <table name="Port_Group" title="Port Groups"> >> + <p> >> + This table contains names for the logical switch ports in the >> + <ref db="OVN_Northbound"/> database that belongs to the same group >> + that is defined in <ref table="Port_Group" db="OVN_Northbound"/> >> + in the <ref db="OVN_Northbound"/> database. >> + </p> >> + >> + <column name="name"/> >> + <column name="ports"/> >> + </table> >> + >> <table name="Logical_Flow" title="Logical Network Flows"> >> <p> >> Each row in this table represents one logical flow. >> @@ -770,6 +782,14 @@ >> <code>$set1</code>. >> </p> >> + <p> >> + You may refer to a group of logical switch ports stored in the >> + <ref table="Port_Group"/> table by its <ref column="name" >> + table="Port_Group"/>. An <ref table="Port_Group"/> with a name >> + of <code>port_group1</code> can be referred to as >> + <code>@port_group1</code>. >> + </p> >> + >> <p><em>Miscellaneous</em></p> >> <p> >> diff --git a/ovn/utilities/ovn-trace.c b/ovn/utilities/ovn-trace.c >> index db39c49..5a9e73d 100644 >> --- a/ovn/utilities/ovn-trace.c >> +++ b/ovn/utilities/ovn-trace.c >> @@ -418,6 +418,9 @@ static struct shash symtab; >> /* Address sets. */ >> static struct shash address_sets; >> +/* Port groups. */ >> +static struct shash port_groups; >> + >> /* DHCP options. */ >> static struct hmap dhcp_opts; /* Contains "struct gen_opts_map"s. */ >> static struct hmap dhcpv6_opts; /* Contains "struct gen_opts_map"s. */ >> @@ -697,9 +700,22 @@ read_address_sets(void) >> const struct sbrec_address_set *sbas; >> SBREC_ADDRESS_SET_FOR_EACH (sbas, ovnsb_idl) { >> - expr_addr_sets_add(&address_sets, sbas->name, >> + expr_const_sets_add(&address_sets, sbas->name, >> (const char *const *) sbas->addresses, >> - sbas->n_addresses); >> + sbas->n_addresses, true); >> + } >> +} >> + >> +static void >> +read_port_groups(void) >> +{ >> + shash_init(&port_groups); >> + >> + const struct sbrec_port_group *sbpg; >> + SBREC_PORT_GROUP_FOR_EACH (sbpg, ovnsb_idl) { >> + expr_const_sets_add(&port_groups, sbpg->name, >> + (const char *const *) sbpg->ports, >> + sbpg->n_ports, false); >> } >> } >> @@ -796,7 +812,8 @@ read_flows(void) >> char *error; >> struct expr *match; >> - match = expr_parse_string(sblf->match, &symtab, &address_sets, >> &error); >> + match = expr_parse_string(sblf->match, &symtab, &address_sets, >> + &port_groups, &error); >> if (error) { >> VLOG_WARN("%s: parsing expression failed (%s)", >> sblf->match, error); >> @@ -937,6 +954,7 @@ read_db(void) >> read_ports(); >> read_mcgroups(); >> read_address_sets(); >> + read_port_groups(); >> read_gen_opts(); >> read_flows(); >> read_mac_bindings(); >> @@ -2009,7 +2027,8 @@ trace(const char *dp_s, const char *flow_s) >> struct flow uflow; >> char *error = expr_parse_microflow(flow_s, &symtab, &address_sets, >> - ovntrace_lookup_port, dp, &uflow); >> + &port_groups, >> ovntrace_lookup_port, >> + dp, &uflow); >> if (error) { >> char *s = xasprintf("error parsing flow: %s\n", error); >> free(error); >> diff --git a/tests/ovn.at b/tests/ovn.at >> index 8ee3bf0..c8ab7b7 100644 >> --- a/tests/ovn.at >> +++ b/tests/ovn.at >> @@ -304,6 +304,7 @@ inport = "eth0" => Syntax error at `=' expecting >> relational operator. >> 123 == 123 => Syntax error at `123' expecting field name. >> $name => Syntax error at `$name' expecting address set name. >> +@name => Syntax error at `@name' expecting port group name. >> 123 == xyzzy => Syntax error at `xyzzy' expecting field name. >> xyzzy == 1 => Syntax error at `xyzzy' expecting field name. >> @@ -657,6 +658,24 @@ ip,nw_src=8.0.0.0/8.0.0.0 >> ]) >> AT_CLEANUP >> +AT_SETUP([ovn -- converting expressions to flows -- port groups]) >> +AT_KEYWORDS([expression]) >> +expr_to_flow () { >> + echo "$1" | ovstest test-ovn expr-to-flows | sort >> +} >> +AT_CHECK([expr_to_flow 'outport == @pg1'], [0], [dnl >> +reg15=0x11 >> +reg15=0x12 >> +reg15=0x13 >> +]) >> +AT_CHECK([expr_to_flow 'outport == {@pg_empty}'], [0], [dnl >> +(no flows) >> +]) >> +AT_CHECK([expr_to_flow 'outport == {"lsp1", @pg_empty}'], [0], [dnl >> +reg15=0x11 >> +]) >> +AT_CLEANUP >> + >> AT_SETUP([ovn -- action parsing]) >> dnl Unindented text is input (a set of OVN logical actions). >> dnl Indented text is expected output. >> @@ -1194,6 +1213,13 @@ ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == >> 0x1236 && outport == "lp33"' d >> ovn-nbctl create Address_Set name=set1 addresses=\"f0:00:00:00:00:11\ >> ",\"f0:00:00:00:00:21\",\"f0:00:00:00:00:31\" >> ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1237 && eth.src == >> $set1 && outport == "lp33"' drop >> +get_lsp_uuid () { >> + ovn-nbctl lsp-list lsw0 | grep $1 | awk '{ print $1 }' >> +} >> + >> +ovn-nbctl create Port_Group name=pg1 ports=`get_lsp_uuid >> lp22`,`get_lsp_uuid lp33` >> +ovn-nbctl acl-add lsw0 to-lport 1000 'eth.type == 0x1238 && outport == >> @pg1' drop >> + >> # Pre-populate the hypervisors' ARP tables so that we don't lose any >> # packets for ARP resolution (native tunneling doesn't queue packets >> # for ARP resolution). >> @@ -1334,10 +1360,18 @@ for is in 1 2 3; do >> else >> acl4=$d >> fi >> + if test $d = $s || test $d = 22 || test $d = 33; then >> + # dest of 22 and 33 should be dropped >> + # due to the 5th ACL that uses port_group(pg1). >> + acl5= >> + else >> + acl5=$d >> + fi >> test_packet $s f000000000$d f000000000$s 1234 >> #7, acl1 >> test_packet $s f000000000$d f000000000$s 1235 $acl2 >> #7, acl2 >> test_packet $s f000000000$d f000000000$s 1236 $acl3 >> #7, acl3 >> test_packet $s f000000000$d f000000000$s 1237 $acl4 >> #7, acl4 >> + test_packet $s f000000000$d f000000000$s 1238 $acl5 #7, >> acl5 >> test_packet $s f000000000$d f00000000055 >> 810000091234 #4 >> test_packet $s f000000000$d 0100000000$s $s$d >> #5 >> diff --git a/tests/test-ovn.c b/tests/test-ovn.c >> index 7452753..d4a5d59 100644 >> --- a/tests/test-ovn.c >> +++ b/tests/test-ovn.c >> @@ -211,10 +211,24 @@ create_addr_sets(struct shash *addr_sets) >> }; >> static const char *const addrs4[] = { NULL }; >> - expr_addr_sets_add(addr_sets, "set1", addrs1, 3); >> - expr_addr_sets_add(addr_sets, "set2", addrs2, 3); >> - expr_addr_sets_add(addr_sets, "set3", addrs3, 3); >> - expr_addr_sets_add(addr_sets, "set4", addrs4, 0); >> + expr_const_sets_add(addr_sets, "set1", addrs1, 3, true); >> + expr_const_sets_add(addr_sets, "set2", addrs2, 3, true); >> + expr_const_sets_add(addr_sets, "set3", addrs3, 3, true); >> + expr_const_sets_add(addr_sets, "set4", addrs4, 0, true); >> +} >> + >> +static void >> +create_port_groups(struct shash *port_groups) >> +{ >> + shash_init(port_groups); >> + >> + static const char *const pg1[] = { >> + "lsp1", "lsp2", "lsp3", >> + }; >> + static const char *const pg2[] = { NULL }; >> + >> + expr_const_sets_add(port_groups, "pg1", pg1, 3, false); >> + expr_const_sets_add(port_groups, "pg_empty", pg2, 0, false); >> } >> static bool >> @@ -245,23 +259,29 @@ test_parse_expr__(int steps) >> { >> struct shash symtab; >> struct shash addr_sets; >> + struct shash port_groups; >> struct simap ports; >> struct ds input; >> create_symtab(&symtab); >> create_addr_sets(&addr_sets); >> + create_port_groups(&port_groups); >> simap_init(&ports); >> simap_put(&ports, "eth0", 5); >> simap_put(&ports, "eth1", 6); >> simap_put(&ports, "LOCAL", ofp_to_u16(OFPP_LOCAL)); >> + simap_put(&ports, "lsp1", 0x11); >> + simap_put(&ports, "lsp2", 0x12); >> + simap_put(&ports, "lsp3", 0x13); >> ds_init(&input); >> while (!ds_get_test_line(&input, stdin)) { >> struct expr *expr; >> char *error; >> - expr = expr_parse_string(ds_cstr(&input), &symtab, >> &addr_sets, &error); >> + expr = expr_parse_string(ds_cstr(&input), &symtab, &addr_sets, >> + &port_groups, &error); >> if (!error && steps > 0) { >> expr = expr_annotate(expr, &symtab, &error); >> } >> @@ -298,8 +318,10 @@ test_parse_expr__(int steps) >> simap_destroy(&ports); >> expr_symtab_destroy(&symtab); >> shash_destroy(&symtab); >> - expr_addr_sets_destroy(&addr_sets); >> + expr_const_sets_destroy(&addr_sets); >> shash_destroy(&addr_sets); >> + expr_const_sets_destroy(&port_groups); >> + shash_destroy(&port_groups); >> } >> static void >> @@ -373,7 +395,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) >> ovn_init_symtab(&symtab); >> struct flow uflow; >> - char *error = expr_parse_microflow(ctx->argv[1], &symtab, NULL, >> + char *error = expr_parse_microflow(ctx->argv[1], &symtab, NULL, >> NULL, >> lookup_atoi_cb, NULL, &uflow); >> if (error) { >> ovs_fatal(0, "%s", error); >> @@ -383,7 +405,7 @@ test_evaluate_expr(struct ovs_cmdl_context *ctx) >> while (!ds_get_test_line(&input, stdin)) { >> struct expr *expr; >> - expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, >> &error); >> + expr = expr_parse_string(ds_cstr(&input), &symtab, NULL, NULL, >> &error); >> if (!error) { >> expr = expr_annotate(expr, &symtab, &error); >> } >> @@ -857,7 +879,8 @@ test_tree_shape_exhaustively(struct expr *expr, >> struct shash *symtab, >> expr_format(expr, &s); >> char *error; >> - modified = expr_parse_string(ds_cstr(&s), symtab, NULL, >> &error); >> + modified = expr_parse_string(ds_cstr(&s), symtab, NULL, >> + NULL, &error); >> if (error) { >> fprintf(stderr, "%s fails to parse (%s)\n", >> ds_cstr(&s), error); >> @@ -1163,7 +1186,7 @@ test_expr_to_packets(struct ovs_cmdl_context *ctx >> OVS_UNUSED) >> while (!ds_get_test_line(&input, stdin)) { >> struct flow uflow; >> char *error = expr_parse_microflow(ds_cstr(&input), &symtab, >> NULL, >> - lookup_atoi_cb, NULL, &uflow); >> + NULL, lookup_atoi_cb, NULL, >> &uflow); >> if (error) { >> puts(error); >> free(error); >> >> > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
