I have rebased this and submitted a new patch. Thank You, Karthik C
On 3/18/22, 4:18 PM, "Lorenzo Bianconi" <[email protected]> wrote: > From: Karthik Chandrashekar > <[email protected]<mailto:[email protected]>> > > From: Karthik Chandrashekar > <[email protected]<mailto:[email protected]>> Hi Karthik, Thanks for the new version. You need rebase it since it does not apply cleanly. Some comments inline. Regards, Lorenzo > [...] > @@ -2346,7 +2368,7 @@ add_lb_hairpin_flows(const struct > sbrec_load_balancer_table *lb_table, > > /* Handles neighbor changes in mac_binding table. */ > void > -lflow_handle_changed_neighbors( > +lflow_handle_changed_mac_bindings( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > const struct sbrec_mac_binding_table *mac_binding_table, > const struct hmap *local_datapaths, > @@ -2373,7 +2395,40 @@ lflow_handle_changed_neighbors( > VLOG_DBG("handle new mac_binding "UUID_FMT, > UUID_ARGS(&mb->header_.uuid)); > consider_neighbor_flow(sbrec_port_binding_by_name, > local_datapaths, > - mb, flow_table); > + mb, NULL, flow_table, 100); > + } > + } > +} > + > +/* Handles changes to static_mac_binding table. */ > +void > +lflow_handle_changed_static_mac_bindings( > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct sbrec_static_mac_binding_table *smb_table, > + const struct hmap *local_datapaths, > + struct ovn_desired_flow_table *flow_table) > +{ > + const struct sbrec_static_mac_binding *smb; > + SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, smb_table) { > + /* Remove any flows that should be removed. */ > + if (sbrec_static_mac_binding_is_deleted(smb)) { > + VLOG_DBG("handle deleted static_mac_binding "UUID_FMT, > + UUID_ARGS(&smb->header_.uuid)); > + ofctrl_remove_flows(flow_table, &smb->header_.uuid); > + } > + } I guess we merge these two loops, right? Done. I have fixed this. > + SBREC_STATIC_MAC_BINDING_TABLE_FOR_EACH_TRACKED (smb, smb_table) { > + if (!sbrec_static_mac_binding_is_deleted(smb)) { > + if (!sbrec_static_mac_binding_is_new(smb)) { > + VLOG_DBG("handle updated static_mac_binding "UUID_FMT, > + UUID_ARGS(&smb->header_.uuid)); > + ofctrl_remove_flows(flow_table, &smb->header_.uuid); > + } > + VLOG_DBG("handle new static_mac_binding "UUID_FMT, > + UUID_ARGS(&smb->header_.uuid)); > + consider_neighbor_flow(sbrec_port_binding_by_name, > local_datapaths, > + NULL, smb, flow_table, > + smb->override_dynamic_mac ? 150 : 50); iiuc, if the user does not provide override_dynamic_mac and we have conflict, we will end up with 2 flows, one dynamically learned (with priority 100) and one statically added (with priority 50). Correct? I think we should have just the "static mac" flow when we have a conflict. That is correct. The total number of flows = number of learnt macs + number of statically configured macs. I think that is ok. Trying to skip one over the other will lead to extra lookups and added complexity. > } > } > } > @@ -2443,7 +2498,9 @@ lflow_run(struct lflow_ctx_in *l_ctx_in, struct > lflow_ctx_out *l_ctx_out) > > add_logical_flows(l_ctx_in, l_ctx_out); > add_neighbor_flows(l_ctx_in->sbrec_port_binding_by_name, > - l_ctx_in->mac_binding_table, > l_ctx_in->local_datapaths, > + l_ctx_in->mac_binding_table, > + l_ctx_in->static_mac_binding_table, > + l_ctx_in->local_datapaths, > l_ctx_out->flow_table); > add_lb_hairpin_flows(l_ctx_in->lb_table, l_ctx_in->local_datapaths, > l_ctx_out->flow_table, > diff --git a/controller/lflow.h b/controller/lflow.h > index d61733bc2..c2944378b 100644 > --- a/controller/lflow.h > +++ b/controller/lflow.h > @@ -147,6 +147,7 @@ struct lflow_ctx_in { > const struct sbrec_fdb_table *fdb_table; > const struct sbrec_chassis *chassis; > const struct sbrec_load_balancer_table *lb_table; > + const struct sbrec_static_mac_binding_table *static_mac_binding_table; > const struct hmap *local_datapaths; > const struct shash *addr_sets; > const struct shash *port_groups; > @@ -191,9 +192,14 @@ bool lflow_handle_addr_set_update(const char *as_name, > struct addr_set_diff *, > struct lflow_ctx_out *, > bool *changed); > > -void lflow_handle_changed_neighbors( > +void lflow_handle_changed_mac_bindings( > struct ovsdb_idl_index *sbrec_port_binding_by_name, > - const struct sbrec_mac_binding_table *, > + const struct sbrec_mac_binding_table *mac_binding_table, > + const struct hmap *local_datapaths, > + struct ovn_desired_flow_table *); > +void lflow_handle_changed_static_mac_bindings( > + struct ovsdb_idl_index *sbrec_port_binding_by_name, > + const struct sbrec_static_mac_binding_table *smb_table, > const struct hmap *local_datapaths, > struct ovn_desired_flow_table *); > bool lflow_handle_changed_lbs(struct lflow_ctx_in *, struct lflow_ctx_out *); > diff --git a/controller/ovn-controller.c b/controller/ovn-controller.c > index c09018243..bda06eb39 100644 > --- a/controller/ovn-controller.c > +++ b/controller/ovn-controller.c > @@ -968,7 +968,8 @@ ctrl_register_ovs_idl(struct ovsdb_idl *ovs_idl) > SB_NODE(dhcpv6_options, "dhcpv6_options") \ > SB_NODE(dns, "dns") \ > SB_NODE(load_balancer, "load_balancer") \ > - SB_NODE(fdb, "fdb") > + SB_NODE(fdb, "fdb") \ > + SB_NODE(static_mac_binding, "static_mac_binding") [...] > +MAC_Binding commands:\n\ > + static-mac-binding-add LOGICAL_PORT IP MAC\n\ > + Add a Static_MAC_Binding entry\n\ > + static-mac-binding-del LOGICAL_PORT IP\n\ > + Delete Static_MAC_Binding entry\n\ > + static-mac-binding-list List all Static_MAC_Binding entries\n\ > +\n\ > %s\ > %s\ > \n\ > @@ -5712,6 +5719,164 @@ 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; > + } > + } > + > + return nb_smb; > +} > + > +static void > +nbctl_pre_static_mac_binding_add(struct ctl_context *ctx) We can add just nbctl_pre_static_mac_binding() since it is always the same, right? Done. I have fixed this. > +{ > + 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; > + return; > + } > + > + 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) { > + if (may_exist) { > + if (strcmp(nb_smb->mac, mac)) { > + nbrec_static_mac_binding_verify_mac(nb_smb); > + nbrec_static_mac_binding_set_mac(nb_smb, mac); > + } > + } else { > + ctl_error(ctx, "%s, %s: a Static_MAC_Binding with this " > + "logical_port and ip already exists", > + logical_port, new_ip); > + } > + 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) > { > @@ -7187,6 +7352,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.25.1 > > _______________________________________________ > 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
