> Hi Lorenzo, Hi Karthik,
> > I’ve updated the patch and addressed a couple of comments. Let me know your > thoughts on the others. Replied in-line. ack, thx. I will review it as soon as you posted it. > > Thank You, > Karthik C > > On 3/3/22, 1:44 AM, "Lorenzo Bianconi" <[email protected]> wrote: > > > From: Karthik Chandrashekar > > > <[email protected]<mailto:[email protected]>> > > > > > > Add a new NB and SB table for managing Static MAC_Binding entries. > > > This table is currently supported for logical routers. OVN northd > > > is responsible for propagating the values from NB to SB. OVN controller > > > is responsible for installation MAC lookup flows. The priority of > > > the installed flows are based on override_dynamic_mac flag. This helps > > > control the precedence of statically programmed vs dynamically learnt > > > MAC Bindings. > > > > Hi Karthik, > > Hi Karthik, > > are you planning to continue working on this feature? > > Regards, > Lorenzo > > > > > thx for working on this :) > > I like the new approch, I added some comments inline. > > > > Regards, > > Lorenzo > > > > > > > > Signed-off-by: Karthik Chandrashekar > > > <[email protected]<mailto:[email protected]>> > > > --- > > > controller/lflow.c | 107 +++++++++++++----- > > > controller/lflow.h | 10 +- > > > controller/ovn-controller.c | 37 ++++++- > > > lib/automake.mk | 2 + > > > lib/static-mac-binding-index.c | 43 ++++++++ > > > lib/static-mac-binding-index.h | 27 +++++ > > > northd/en-northd.c | 8 ++ > > > northd/inc-proc-northd.c | 14 ++- > > > northd/northd.c | 75 +++++++++++++ > > > northd/northd.h | 3 + > > > ovn-nb.ovsschema | 15 ++- > > > ovn-nb.xml | 29 +++++ > > > ovn-sb.ovsschema | 14 ++- > > > ovn-sb.xml | 27 +++++ > > > tests/ovn-nbctl.at | 69 ++++++++++++ > > > tests/ovn-northd.at | 24 ++++ > > > tests/ovn.at | 90 +++++++++++++++ > > > utilities/ovn-nbctl.c | 193 ++++++++++++++++++++++++++++++++- > > > 18 files changed, 748 insertions(+), 39 deletions(-) > > > create mode 100644 lib/static-mac-binding-index.c > > > create mode 100644 lib/static-mac-binding-index.h > > > > > > diff --git a/controller/lflow.c b/controller/lflow.c > > > index 933e2f3cc..c86d35960 100644 > > > --- a/controller/lflow.c > > > +++ b/controller/lflow.c > > > @@ -1030,40 +1030,50 @@ static void > > > consider_neighbor_flow(struct ovsdb_idl_index > > > *sbrec_port_binding_by_name, > > > const struct hmap *local_datapaths, > > > const struct sbrec_mac_binding *b, > > > - struct ovn_desired_flow_table *flow_table) > > > + const struct sbrec_static_mac_binding *smb, > > > + struct ovn_desired_flow_table *flow_table, > > > + uint16_t priority) > > > > IIUC the code, doing so we will end-up having one entry for the dynamic > > learned mac > > (with lower priority) and one for the static mac binding (with higher > > priority). Is it correct? > > If so I guess we should just avoid adding the dynamic binding if we have > > configured > > a static one (and overwrite it if it occurs in the reverse order). > > > > That is correct, I’m using priority to determine what MAC should be used > based on the flag. > smb->override_dynamic_mac ? 150 : 50 > Does this affect correctness in any way? > > I think doing so has a clean separation of concerns while handling changes to > mac_binding and static_mac_binding tables based on the calls to > consider_neighbor_flow. I’ll have to do an extra lookup otherwise if I want > to overwrite using existing priority. ack, got your point, I was just wondering on the number of lflows here since for each entry we will have 2 flows, right? > > > [...] > > > +static const struct nbrec_static_mac_binding * > > > +static_mac_binding_by_port_ip(struct northd_input *input_data, > > > + const char *logical_port, const char *ip) > > > +{ > > > + const struct nbrec_static_mac_binding *nb_smb = NULL; > > > + > > > + NBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH ( > > > + nb_smb, input_data->nbrec_static_mac_binding_table) { > > > + if (!strcmp(nb_smb->logical_port, logical_port) && > > > + !strcmp(nb_smb->ip, ip)) { > > > + break; > > > + } > > > + } > > > > It seems we have 2 copies of a lookup-by-ip routine (one here) and one for > > ovn-nbctl. Can we please unify the code? > > Moreover can you please run checkpatch.py? > > > > The nbctl utility takes in IDL as the argument whereas northd has it’s own > struct. I don’t see a way to unify these using a common library. ack, let's drop the comment. > > I’ve fixed the errors thrown by checkpath.py except the Line lacks whitespace > around operator > > Looks like I already hit this error when I run checkpath on > utilities/ovn-nbctl.c for all the existing help strings in the file. Is > checkpath applicable to this file at all? yes, I guess you can ignore these errors. Regards, Lorenzo > > > + > > > + return nb_smb; > > > +} > > > + > > > +static void > > > +build_static_mac_binding_table(struct northd_input *input_data, > > > + struct ovsdb_idl_txn *ovnsb_txn, > > > + struct hmap *ports) > > > +{ > > > + /* Cleanup SB Static_MAC_Binding entries which do not have > > > corresponding > > > + * NB Static_MAC_Binding entries. */ > > > + const struct nbrec_static_mac_binding *nb_smb; > > > + const struct sbrec_static_mac_binding *sb_smb, *sb_smb_next; > > > + SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_SAFE (sb_smb, sb_smb_next, > > > + input_data->sbrec_static_mac_binding_table) { > > > + nb_smb = static_mac_binding_by_port_ip(input_data, > > > sb_smb->logical_port, > > > + sb_smb->ip); > > > + if (!nb_smb) { > > > + sbrec_static_mac_binding_delete(sb_smb); > > > + } > > > + } > > > + > > > + /* Create/Update SB Static_MAC_Binding entries with corresponding > > > values > > > + * from NB Static_MAC_Binding entries. */ > > > + NBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH ( > > > + nb_smb, input_data->nbrec_static_mac_binding_table) { > > > + struct ovn_port *op = ovn_port_find(ports, nb_smb->logical_port); > > > + if (op && op->nbrp) { > > > + struct ovn_datapath *od = op->od; > > > + if (od && od->sb) { > > > + const struct sbrec_static_mac_binding *mb = > > > + static_mac_binding_lookup( > > > + input_data->sbrec_static_mac_binding_by_lport_ip, > > > + nb_smb->logical_port, nb_smb->ip); > > > + if (!mb) { > > > + /* Create new entry */ > > > + mb = sbrec_static_mac_binding_insert(ovnsb_txn); > > > + sbrec_static_mac_binding_set_logical_port( > > > + mb, nb_smb->logical_port); > > > + sbrec_static_mac_binding_set_ip(mb, nb_smb->ip); > > > + sbrec_static_mac_binding_set_mac(mb, nb_smb->mac); > > > + sbrec_static_mac_binding_set_override_dynamic_mac(mb, > > > + nb_smb->override_dynamic_mac); > > > + sbrec_static_mac_binding_set_datapath(mb, od->sb); > > > + } else { > > > + /* Update existing entry if there is a change*/ > > > + if (strcmp(mb->mac, nb_smb->mac)) { > > > + sbrec_static_mac_binding_set_mac(mb, > > > nb_smb->mac); > > > + } > > > + if (mb->override_dynamic_mac != > > > + nb_smb->override_dynamic_mac) { > > > + > > > sbrec_static_mac_binding_set_override_dynamic_mac(mb, > > > + nb_smb->override_dynamic_mac); > > > + } > > > + } > > > + } > > > + } > > > + } > > > +} > > > + > > > void > > > > [...] > > > @@ -5697,6 +5704,175 @@ nbctl_lrp_get_redirect_type(struct ctl_context > > > *ctx) > > > !redirect_type ? "overlay": redirect_type); > > > } > > > > > > +static const struct nbrec_static_mac_binding * > > > +static_mac_binding_by_port_ip(struct ctl_context *ctx, > > > + const char *logical_port, const char *ip) > > > +{ > > > + const struct nbrec_static_mac_binding *nb_smb = NULL; > > > + > > > + NBREC_STATIC_MAC_BINDING_FOR_EACH(nb_smb, ctx->idl) { > > > + if (!strcmp(nb_smb->logical_port, logical_port) && > > > + !strcmp(nb_smb->ip, ip)) { > > > + break; > > > + } > > > > ditto. > > > > > + } > > > + > > > + return nb_smb; > > > +} > > > + > > > +static void > > > +nbctl_pre_static_mac_binding_add(struct ctl_context *ctx) > > > +{ > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name); > > > + > > > + ovsdb_idl_add_column(ctx->idl, > > > &nbrec_static_mac_binding_col_logical_port); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_ip); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_mac); > > > +} > > > + > > > +static void > > > +nbctl_static_mac_binding_add(struct ctl_context *ctx) > > > +{ > > > + const char *logical_port = ctx->argv[1]; > > > + const char *ip = ctx->argv[2]; > > > + const char *mac = ctx->argv[3]; > > > + char *new_ip = NULL; > > > + > > > + const struct nbrec_logical_router_port *lrp; > > > + char *error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp); > > > + if (error) { > > > + ctx->error = error; > > > + goto cleanup; > > > > I guess you can just return here. > > > > Done > > > > + } > > > + > > > + new_ip = normalize_addr_str(ip); > > > + if (!new_ip) { > > > + ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", ip); > > > + return; > > > + } > > > + > > > + struct eth_addr ea; > > > + if (!eth_addr_from_string(mac, &ea)) { > > > + ctl_error(ctx, "invalid mac address %s.", mac); > > > + goto cleanup; > > > + } > > > + > > > + bool may_exist = shash_find(&ctx->options, "--may-exist") != NULL; > > > + const struct nbrec_static_mac_binding *nb_smb = > > > + static_mac_binding_by_port_ip(ctx, logical_port, ip); > > > + if (nb_smb) { > > > + char *old_ip; > > > + bool should_return = false; > > > + old_ip = normalize_addr_str(nb_smb->ip); > > > + > > > + if (!strcmp(nb_smb->logical_port, logical_port)) { > > > + if (!strcmp(old_ip, new_ip)) { > > > > Have we already checked "logical_port" and "new_ip" during the lookup? > > Fixed this block. I want to compare the MACs here not the IPs. > > > > > > + if (may_exist) { > > > + nbrec_static_mac_binding_verify_mac(nb_smb); > > > + nbrec_static_mac_binding_set_mac(nb_smb, mac); > > > + should_return = true; > > > + } else { > > > + ctl_error(ctx, "%s, %s: a Static_MAC_Binding with > > > this " > > > + "logical_port and ip already exists", > > > + logical_port, new_ip); > > > + should_return = true; > > > + } > > > + } > > > + } > > > + free(old_ip); > > > + if (should_return) { > > > + goto cleanup; > > > + } > > > + } > > > + > > > + /* Create Static_MAC_Binding entry */ > > > + nb_smb = nbrec_static_mac_binding_insert(ctx->txn); > > > + nbrec_static_mac_binding_set_logical_port(nb_smb, logical_port); > > > + nbrec_static_mac_binding_set_ip(nb_smb, new_ip); > > > + nbrec_static_mac_binding_set_mac(nb_smb, mac); > > > + > > > +cleanup: > > > + free(new_ip); > > > +} > > > + > > > +static void > > > +nbctl_pre_static_mac_binding_del(struct ctl_context *ctx) > > > +{ > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_port_col_name); > > > + > > > + ovsdb_idl_add_column(ctx->idl, > > > &nbrec_static_mac_binding_col_logical_port); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_ip); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_mac); > > > +} > > > + > > > +static void > > > +nbctl_static_mac_binding_del(struct ctl_context *ctx) > > > +{ > > > + bool must_exist = !shash_find(&ctx->options, "--if-exists"); > > > + const char *logical_port = ctx->argv[1]; > > > + const struct nbrec_logical_router_port *lrp; > > > + char *error = lrp_by_name_or_uuid(ctx, logical_port, true, &lrp); > > > + if (error) { > > > + ctx->error = error; > > > + return; > > > + } > > > + > > > + char *ip = normalize_addr_str(ctx->argv[2]); > > > + if (!ip) { > > > + ctl_error(ctx, "%s: Not a valid IPv4 or IPv6 address.", > > > ctx->argv[2]); > > > + return; > > > + } > > > + > > > + const struct nbrec_static_mac_binding *nb_smb = > > > + static_mac_binding_by_port_ip(ctx, logical_port, ip); > > > + > > > + if (nb_smb) { > > > + /* Remove the matching Static_MAC_Binding. */ > > > + nbrec_static_mac_binding_delete(nb_smb); > > > + goto cleanup; > > > + } > > > + > > > + if (must_exist) { > > > + ctl_error(ctx, "no matching Static_MAC_Binding with port %s and > > > ip %s", > > > + logical_port, ip); > > > + } > > > + > > > +cleanup: > > > + free(ip); > > > +} > > > + > > > +static void > > > +nbctl_pre_static_mac_binding_list(struct ctl_context *ctx) > > > +{ > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_logical_router_col_name); > > > + > > > + ovsdb_idl_add_column(ctx->idl, > > > &nbrec_static_mac_binding_col_logical_port); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_ip); > > > + ovsdb_idl_add_column(ctx->idl, &nbrec_static_mac_binding_col_mac); > > > +} > > > + > > > +static void > > > +nbctl_static_mac_binding_list(struct ctl_context *ctx) > > > +{ > > > + struct smap lr_mac_bindings = SMAP_INITIALIZER(&lr_mac_bindings); > > > + const struct nbrec_static_mac_binding *nb_smb = NULL; > > > + NBREC_STATIC_MAC_BINDING_FOR_EACH(nb_smb, ctx->idl) { > > > + char *key = xasprintf("%-25s%-25s", nb_smb->logical_port, > > > nb_smb->ip); > > > + smap_add_format(&lr_mac_bindings, key, "%s", nb_smb->mac); > > > + free(key); > > > + } > > > + > > > + const struct smap_node **nodes = smap_sort(&lr_mac_bindings); > > > + if (nodes) { > > > + ds_put_format(&ctx->output, "%-25s%-25s%s\n", > > > + "LOGICAL_PORT", "IP", "MAC"); > > > + for (size_t i = 0; i < smap_count(&lr_mac_bindings); i++) { > > > + const struct smap_node *node = nodes[i]; > > > + ds_put_format(&ctx->output, "%-25s%s\n", node->key, > > > node->value); > > > + } > > > + } > > > +} > > > + > > > static const struct nbrec_forwarding_group * > > > fwd_group_by_name_or_uuid(struct ctl_context *ctx, const char *id) > > > { > > > @@ -7198,6 +7374,17 @@ static const struct ctl_command_syntax > > > nbctl_commands[] = { > > > pre_ha_ch_grp_set_chassis_prio, cmd_ha_ch_grp_set_chassis_prio, > > > NULL, > > > "", RW }, > > > > > > + /* Static_MAC_Binding commands */ > > > + { "static-mac-binding-add", 3, 3, "LOGICAL_PORT IP MAC", > > > + nbctl_pre_static_mac_binding_add, nbctl_static_mac_binding_add, > > > NULL, > > > + "--may-exist", RW }, > > > + { "static-mac-binding-del", 2, 2, "LOGICAL_PORT IP", > > > + nbctl_pre_static_mac_binding_del, nbctl_static_mac_binding_del, > > > NULL, > > > + "--if-exists", RW }, > > > + { "static-mac-binding-list", 0, 1, "[LOGICAL_PORT]", > > > + nbctl_pre_static_mac_binding_list, nbctl_static_mac_binding_list, > > > NULL, > > > + "", RO }, > > > + > > > {NULL, 0, 0, NULL, NULL, NULL, NULL, "", RO}, > > > }; > > > > > > -- > > > 2.22.3 > > > > > > _______________________________________________ > > > dev mailing list > > > [email protected]<mailto:[email protected]> > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
