> On Oct 27, 2016, at 8:12 AM, itamaro <[email protected]> wrote: > > From: itamarofek <[email protected]>
Sorry for the delay. I had a bunch of suggested changes in my drafts folder and apparently never hit send. The previous patches had version numbers. I assume this is v5. Can you please add v6 to the next version? It's also helpful to provide a history of change highlights. The title mentions adding an explicit tunnel key, but the bigger change seem to me is the introduction of levels. Adding levels probably merits its own commit, but I won't hold you to that. However, at a minimum, I do think the section describing the commit should explain better what the levels are. > This patch adds support for handeling a per-tunnel tunnel key in the > ovs-vtep and vtep-ctl. s/handeling/handling/ > The aim of this patch is to support the usage of hardware vtep switch as > an inter-cloud gateway, which is used to connect two separated l2 broadcast > domains > > Requested-by: "Ofer Ben-Yacov" <[email protected]> > Signed-off-by: "Itamar Ofek" <[email protected]> > Co-authored-by: "Darrell Ball" <[email protected]> > --- > tests/vtep-ctl.at | 217 +++++++++++++++++++++++++++++++++++++++++++++++----- > vtep/ovs-vtep | 122 ++++++++++++++++++----------- > vtep/vtep-ctl.c | 203 +++++++++++++++++++++++++++++++++--------------- > vtep/vtep.ovsschema | 9 ++- > vtep/vtep.xml | 12 +++ > 5 files changed, 438 insertions(+), 125 deletions(-) > diff --git a/vtep/vtep-ctl.c b/vtep/vtep-ctl.c > index 3450deb..6a0a62e 100644 > --- a/vtep/vtep-ctl.c > +++ b/vtep/vtep-ctl.c > > @@ -344,18 +345,18 @@ Logical Router commands:\n\ > lr-exists LR exit 2 if LR does not exist\n\ > \n\ > MAC binding commands:\n\ > - add-ucast-local LS MAC [ENCAP] IP add ucast local entry in LS\n\ > - del-ucast-local LS MAC del ucast local entry from LS\n\ > - add-mcast-local LS MAC [ENCAP] IP add mcast local entry in LS\n\ > - del-mcast-local LS MAC [ENCAP] IP del mcast local entry from LS\n\ > - clear-local-macs LS clear local mac entries\n\ > - list-local-macs LS list local mac entries\n\ > - add-ucast-remote LS MAC [ENCAP] IP add ucast remote entry in LS\n\ > - del-ucast-remote LS MAC del ucast remote entry from LS\n\ > - add-mcast-remote LS MAC [ENCAP] IP add mcast remote entry in LS\n\ > - del-mcast-remote LS MAC [ENCAP] IP del mcast remote entry from LS\n\ > - clear-remote-macs LS clear remote mac entries\n\ > - list-remote-macs LS list remote mac entries\n\ > + add-ucast-local LS MAC [ENCAP] IP [KEY [LEVEL]] add ucast local entry in > LS\n\ > + del-ucast-local LS MAC del ucast local entry > from LS\n\ > + add-mcast-local LS MAC [ENCAP] IP [KEY [LEVEL]] add mcast local entry in > LS\n\ > + del-mcast-local LS MAC [ENCAP] IP del mcast local entry > from LS\n\ > + clear-local-macs LS clear local mac entries\n\ > + list-local-macs LS list local mac entries\n\ > + add-ucast-remote LS MAC [ENCAP] IP [KEY [LEVEL]] add ucast remote entry in > LS\n\ > + del-ucast-remote LS MAC del ucast remote entry > from LS\n\ > + add-mcast-remote LS MAC [ENCAP] IP [KEY [LEVEL]] add mcast remote entry in > LS\n\ > + del-mcast-remote LS MAC [ENCAP] IP del mcast remote entry > from LS\n\ > + clear-remote-macs LS clear remote mac > entries\n\ > + list-remote-macs LS list remote mac entries\n\ The man page should be updated with these changes. > @@ -450,6 +451,14 @@ struct vtep_ctl_context { > * struct vtep_ctl_lrouter. */ > }; > > +struct add_mac_args { > + const char *mac; > + const char *encap; > + const char *dst_ip; > + const char *tunnel_key; > + const char *tunnel_level; > +}; I'd put this definition right above parse_add_mac_args(), since the definition isn't need for more than a thousand lines. > @@ -1647,34 +1659,75 @@ cmd_lr_exists(struct ctl_context *ctx) > } > } > > +inline void > +static parse_add_mac_args(struct ctl_context *ctx, struct add_mac_args* args) This code is assuming a specific location for each "arg" argument, so I'd add a comment describing what that is, including which ones are optional. > +{ > + ovs_be32 ip = 0; > + if (ctx->argc < 4 ) { > + ctl_fatal("invalid argument set only %d args supplied",ctx->argc); > + > + } > + int index = 2; > + args->mac = ctx->argv[index]; > + /* check 3rd argument if its ip assume vxlan_over_ipv4 > + * else take the encapsulation from supplied argument > + */ > + if (!ip_parse(ctx->argv[++index], &ip)) { > + args->encap = ctx->argv[index++]; > + } else { > + args->encap = "vxlan_over_ipv4"; > + } > + args->dst_ip = ctx->argv[index]; > + /* if no more arguments supplied prsing is done > + */ s/prsing/parsing/ As mentioned in CodingStyle.md, please write full sentences that start with a capital letter and end with a period. My personal preference is to not have the end of the comment on a new line, but I think it looks especially odd with a one line comment. > static void > add_ucast_entry(struct ctl_context *ctx, bool local) > { > struct vtep_ctl_context *vtepctl_ctx = vtep_ctl_context_cast(ctx); > struct vtep_ctl_lswitch *ls; > - const char *mac; > - const char *encap; > - const char *dst_ip; > + struct add_mac_args args; > + memset(&args, 0, sizeof(args)); As described CodingStyle.md, "sizeof" shouldn't use parentheses. > + int64_t tunnel_scope_key = 0; It looks like this variable is only needed if "arg.tunnel_key" exists. I'd move it into that block. > + > struct vteprec_physical_locator *ploc_cfg; > > vtep_ctl_context_populate_cache(ctx); > > ls = find_lswitch(vtepctl_ctx, ctx->argv[1], true); > - mac = ctx->argv[2]; > - > - if (ctx->argc == 4) { > - encap = "vxlan_over_ipv4"; > - dst_ip = ctx->argv[3]; > - } else { > - encap = ctx->argv[3]; > - dst_ip = ctx->argv[4]; > - } > - > - ploc_cfg = find_ploc(vtepctl_ctx, encap, dst_ip); > + parse_add_mac_args(ctx, &args); > + ploc_cfg = find_ploc(vtepctl_ctx, args.encap, args.dst_ip); > if (!ploc_cfg) { > ploc_cfg = vteprec_physical_locator_insert(ctx->txn); > - vteprec_physical_locator_set_dst_ip(ploc_cfg, dst_ip); > - vteprec_physical_locator_set_encapsulation_type(ploc_cfg, encap); > + if (args.tunnel_key) { > + ovs_scan(args.tunnel_key, "%"SCNd64, &tunnel_scope_key); How about using str_to_llong() instead of ovs_scan()? > + vteprec_physical_locator_set_tunnel_key(ploc_cfg, > + &tunnel_scope_key, 1); > + if (args.tunnel_level){ Put a space between the parenthesis and curly bracket. > @@ -1789,7 +1842,8 @@ commit_mcast_entries(struct vtep_ctl_mcast_mac > *mcast_mac) > static void > add_mcast_entry(struct ctl_context *ctx, > struct vtep_ctl_lswitch *ls, const char *mac, > - const char *encap, const char *dst_ip, bool local) > + const char *encap, const char *dst_ip, const char* > tunnel_key, > + const char* tunnel_scope, bool local) Can you make add_mcast_entry() look more like add_ucast_entry()? For example, call parse_add_mac_args() inside add_mcast_entry() and del_mcast_entry(). It would reduce the number of arguments to those functions and make the code a bit cleaner and more consistent. > @@ -1838,6 +1892,16 @@ add_mcast_entry(struct ctl_context *ctx, > ploc_cfg = find_ploc(vtepctl_ctx, encap, dst_ip); > if (!ploc_cfg) { > ploc_cfg = vteprec_physical_locator_insert(ctx->txn); > + if (tunnel_key) { > + int64_t tunnel_scope_key = 0; > + ovs_scan(tunnel_key, "%"SCNd64, &tunnel_scope_key); Same questions about using str_to_llong() instead of ovs_scan(). > @@ -1907,27 +1971,18 @@ add_del_mcast_entry(struct ctl_context *ctx, bool > add, bool local) > { > ... > + parse_add_mac_args(ctx, &args); > if (add) { > - add_mcast_entry(ctx, ls, mac, encap, dst_ip, local); > + add_mcast_entry(ctx, ls, args.mac, args.encap, args.dst_ip, > args.tunnel_key,args.tunnel_level, local); This line is way over 79 characters. > --- a/vtep/vtep.xml > +++ b/vtep/vtep.xml > @@ -1174,6 +1174,18 @@ > </p> > </column> > > + <column name="tunnel_level"> > + <p> > + For <code>vxlan_over_ipv4</code> encapsulation, represents the > + hierarchy level of the <ref table="Physical_Locator"/>. > + Broadcast, unknown unicast and multicast (BUM) traffic received > + at one level is never sent to another <ref table="Physical_Locator"/> > + of the same level. The only valid values supported are level0 and > + level1; this column is optional and the default value if not > + configured is level0. Put <code> blocks around "level0" and "level1". I still need to review "ovs-vtep", but I wanted to at least get you some feedback, and I know we're waiting on feedback from some of the original authors of the schema on their opinion on this approach. --Justin _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
