Hi Lorenzo, I’ve updated the patch and addressed a couple of comments. Let me know your thoughts on the others. Replied in-line.
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. > [...] > > +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. 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? > > + > > + 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
