> From: Karthik Chandrashekar <[email protected]>
>
> From: Karthik Chandrashekar <[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?
> + 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.
> }
> }
> }
> @@ -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?
> +{
> + 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]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
