> From: Karthik Chandrashekar <[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, thx for working on this :) I like the new approch, I added some comments inline. Regards, Lorenzo > > Signed-off-by: Karthik Chandrashekar <[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). [...] > +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? > + > + 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. > + } > + > + 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? > + 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] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
