| Message ID | 1466616508-2214-1-git-send-email-zealokii@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
> On Jun 22, 2016, at 10:28 AM, Zong Kai LI <zealokii@gmail.com> wrote: > > This patch renames Logical_Router_Port 'network' column to 'networks' > and aligns it to Logical_Switch_Port 'addresses' column for peer port > in Logical_Switch_Port which type is 'router'. > > Currently, a lsp port whos type is 'router', may have multiple addresses > for IPv6 address case, but it's peer lrp port has only 1 address in > 'network' column. It's inconsistent, this patch tries to fix that. > > Signed-off-by: Zong Kai LI <zealokii@gmail.com> > --- > ovn/northd/ovn-northd.c | 5 +++-- > ovn/ovn-nb.ovsschema | 6 ++++-- > ovn/ovn-nb.xml | 7 +++--- > ovn/utilities/ovn-nbctl.c | 54 +++++++++++++++++++++++++++++++++++------------ > tests/ovn-nbctl.at | 9 ++++++++ > 5 files changed, 61 insertions(+), 20 deletions(-) > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index 17713ec..5851fca 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -612,11 +612,12 @@ join_logical_ports(struct northd_context *ctx, > } > > ovs_be32 ip, mask; > - char *error = ip_parse_masked(nbr->network, &ip, &mask); > + /* It should have only one IPv4 address. */ > + char *error = ip_parse_masked(nbr->networks[0], &ip, &mask); You changed the schema to allow not specifying a network, but don't check if one is not defined. I think this will cause a crash in that instance. > > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > index 58f04b2..4a345c8 100644 > --- a/ovn/ovn-nb.ovsschema > +++ b/ovn/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > "version": "3.1.0", > - "cksum": "1426508118 6135", > + "cksum": "2770494524 6251", You need to change the version number when you modify the schema. > "tables": { > "Logical_Switch": { > "columns": { > @@ -95,7 +95,9 @@ > "Logical_Router_Port": { > "columns": { > "name": {"type": "string"}, > - "network": {"type": "string"}, > + "networks": {"type": {"key": "string", > + "min": 0, > + "max": "unlimited"}}, ... > --- a/ovn/ovn-nb.xml > +++ b/ovn/ovn-nb.xml > @@ -689,11 +689,12 @@ > </p> > </column> > > - <column name="network"> > - The IP address of the router and the netmask. For example, > + <column name="networks"> > + The IP addresses of the router and the netmask. For example, > <code>192.168.0.1/24</code> indicates that the router's IP address is > 192.168.0.1 and that packets destined to 192.168.0.<var>x</var> should be > - routed to this port. > + routed to this port. A router port should have only one IPv4 address, or > + multiple IPv6 addresses. Is there a reason to limit it to one IPv4 address, but allow multiple IPv6 addresses? Also, this gives the impression that IPv6 routing is implemented, but it's not at this point. I have a patch in my tree that adds support for multiple IPv4 addresses in addition to multiple IPv6 addresses. My preference would be to add multiple lrp network support to the ovn-nbctl command at the same time that ovn-northd can make use of them. I'm planning to send out my series within the next couple of weeks. Thanks, --Justin
On Fri, Jun 24, 2016 at 4:43 AM, Justin Pettit <jpettit@ovn.org> wrote: > > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > > index 17713ec..5851fca 100644 > > --- a/ovn/northd/ovn-northd.c > > +++ b/ovn/northd/ovn-northd.c > > @@ -612,11 +612,12 @@ join_logical_ports(struct northd_context *ctx, > > } > > > > ovs_be32 ip, mask; > > - char *error = ip_parse_masked(nbr->network, &ip, &mask); > > + /* It should have only one IPv4 address. */ > > + char *error = ip_parse_masked(nbr->networks[0], &ip, > &mask); > > You changed the schema to allow not specifying a network, but don't check > if one is not defined. I think this will cause a crash in that instance. > > Sure, I will try to fix this, and add a test for the case you mentioned. > > > > diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > > index 58f04b2..4a345c8 100644 > > --- a/ovn/ovn-nb.ovsschema > > +++ b/ovn/ovn-nb.ovsschema > > @@ -1,7 +1,7 @@ > > { > > "name": "OVN_Northbound", > > "version": "3.1.0", > > - "cksum": "1426508118 6135", > > + "cksum": "2770494524 6251", > > You need to change the version number when you modify the schema. > > Oops, thanks for mention that. > > - <column name="network"> > > - The IP address of the router and the netmask. For example, > > + <column name="networks"> > > + The IP addresses of the router and the netmask. For example, > > <code>192.168.0.1/24</code> indicates that the router's IP > address is > > 192.168.0.1 and that packets destined to 192.168.0.<var>x</var> > should be > > - routed to this port. > > + routed to this port. A router port should have only one IPv4 > address, or > > + multiple IPv6 addresses. > > Is there a reason to limit it to one IPv4 address, but allow multiple IPv6 > addresses? Also, this gives the impression that IPv6 routing is > implemented, but it's not at this point. > > I have a patch in my tree that adds support for multiple IPv4 addresses in > addition to multiple IPv6 addresses. My preference would be to add > multiple lrp network support to the ovn-nbctl command at the same time that > ovn-northd can make use of them. I'm planning to send out my series within > the next couple of weeks. > > Thanks, > > --Justin > > > To be honest, I start to think about it just from IPv6 side, not IPv4. So I may make some mistake, and afer I checked ovn-nb.xml for Logical_Switch_Port table addresses column, I'm sure I've made a mistake. Maybe multiple IPv4 addresses should be supported also. Good catch. For IPv6, multiple IPv6 addresses in Logical_Router_Port network column will benefit the implementation for RA/SLAAC, cause they're prefixes natively. For IPv4, I'm not sure yet.
> On Jun 23, 2016, at 10:14 PM, Zong Kai LI <zealokii@gmail.com> wrote: > > To be honest, I start to think about it just from IPv6 side, not IPv4. So I may make some mistake, and afer I checked ovn-nb.xml for Logical_Switch_Port table addresses column, I'm sure I've made a mistake. Maybe multiple IPv4 addresses should be supported also. Good catch. > For IPv6, multiple IPv6 addresses in Logical_Router_Port network column will benefit the implementation for RA/SLAAC, cause they're prefixes natively. For IPv4, I'm not sure yet. Just in case it wasn't clear, I already have a patch series that adds support for setting and using multiple addresses (IPv4 and IPv6) on logical router ports. I plan to send it out in the next week or two, so I don't think it would make sense to rework this patch. Thanks for submitting the patch, though. I hope you'll try out the series and let me know if you find any problems or have suggestions. --Justin
diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index 17713ec..5851fca 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -612,11 +612,12 @@ join_logical_ports(struct northd_context *ctx, } ovs_be32 ip, mask; - char *error = ip_parse_masked(nbr->network, &ip, &mask); + /* It should have only one IPv4 address. */ + char *error = ip_parse_masked(nbr->networks[0], &ip, &mask); if (error || mask == OVS_BE32_MAX || !ip_is_cidr(mask)) { static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL(&rl, "bad 'network' %s", nbr->network); + VLOG_WARN_RL(&rl, "bad 'network' %s", nbr->networks[0]); free(error); continue; } diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema index 58f04b2..4a345c8 100644 --- a/ovn/ovn-nb.ovsschema +++ b/ovn/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", "version": "3.1.0", - "cksum": "1426508118 6135", + "cksum": "2770494524 6251", "tables": { "Logical_Switch": { "columns": { @@ -95,7 +95,9 @@ "Logical_Router_Port": { "columns": { "name": {"type": "string"}, - "network": {"type": "string"}, + "networks": {"type": {"key": "string", + "min": 0, + "max": "unlimited"}}, "mac": {"type": "string"}, "peer": {"type": {"key": "string", "min": 0, "max": 1}}, "enabled": {"type": {"key": "boolean", "min": 0, "max": 1}}, diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml index 6355c44..56d82fb 100644 --- a/ovn/ovn-nb.xml +++ b/ovn/ovn-nb.xml @@ -689,11 +689,12 @@ </p> </column> - <column name="network"> - The IP address of the router and the netmask. For example, + <column name="networks"> + The IP addresses of the router and the netmask. For example, <code>192.168.0.1/24</code> indicates that the router's IP address is 192.168.0.1 and that packets destined to 192.168.0.<var>x</var> should be - routed to this port. + routed to this port. A router port should have only one IPv4 address, or + multiple IPv6 addresses. </column> <column name="mac"> diff --git a/ovn/utilities/ovn-nbctl.c b/ovn/utilities/ovn-nbctl.c index 7789cdd..7416100 100644 --- a/ovn/utilities/ovn-nbctl.c +++ b/ovn/utilities/ovn-nbctl.c @@ -1523,8 +1523,21 @@ nbctl_lrp_add(struct ctl_context *ctx) const char *lrp_name = ctx->argv[2]; const char *mac = ctx->argv[3]; - const char *network = ctx->argv[4]; - const char *peer = (ctx->argc == 6) ? ctx->argv[5] : NULL; + char **networks = ctx->argv + 4; + size_t n_networks = 1; + const char *peer = NULL; + if (ctx->argc >= 6) { + struct in6_addr ipv6; + unsigned int plen; + char *error = ipv6_parse_cidr(ctx->argv[ctx->argc - 1], &ipv6, &plen); + if (error) { + peer = ctx->argv[ctx->argc - 1]; + n_networks += ctx->argc - 6; + free(error); + } else { + n_networks += ctx->argc - 5; + } + } const struct nbrec_logical_router_port *lrp; lrp = lrp_by_name_or_uuid(ctx, lrp_name, false); @@ -1547,9 +1560,21 @@ nbctl_lrp_add(struct ctl_context *ctx) lrp->mac); } - if (strcmp(network, lrp->network)) { - ctl_fatal("%s: port already exists with network %s", lrp_name, - lrp->network); + for (size_t i = 0; i != lrp->n_networks; i++) { + for (size_t j = 0; j != n_networks; j++) { + if (strcmp(networks[j], lrp->networks[i])) { + struct ds msg = DS_EMPTY_INITIALIZER; + ds_put_format(&msg, "%s: port already exists with network", + lrp_name); + for (size_t k = 0; k != lrp->n_networks; k++) { + ds_put_format(&msg, " %s", lrp->networks[k]); + } + char fatal_msg[msg.length + 1]; + memcpy(fatal_msg, msg.string, msg.length + 1); + ds_destroy(&msg); + ctl_fatal(fatal_msg); + } + } } if ((!peer != !lrp->peer) || @@ -1568,14 +1593,16 @@ nbctl_lrp_add(struct ctl_context *ctx) ovs_be32 ipv4; unsigned int plen; - char *error = ip_parse_cidr(network, &ipv4, &plen); + char *error = ip_parse_cidr(networks[0], &ipv4, &plen); if (error) { free(error); struct in6_addr ipv6; - error = ipv6_parse_cidr(network, &ipv6, &plen); - if (error) { - free(error); - ctl_fatal("%s: invalid network address.", network); + for (size_t i = 0; i!= n_networks; i++) { + error = ipv6_parse_cidr(networks[i], &ipv6, &plen); + if (error) { + free(error); + ctl_fatal("%s: invalid network address.", networks[i]); + } } } @@ -1583,7 +1610,8 @@ nbctl_lrp_add(struct ctl_context *ctx) lrp = nbrec_logical_router_port_insert(ctx->txn); nbrec_logical_router_port_set_name(lrp, lrp_name); nbrec_logical_router_port_set_mac(lrp, mac); - nbrec_logical_router_port_set_network(lrp, network); + nbrec_logical_router_port_set_networks(lrp, + (const char**)networks, n_networks); if (peer) { nbrec_logical_router_port_set_peer(lrp, peer); } @@ -2120,8 +2148,8 @@ static const struct ctl_command_syntax nbctl_commands[] = { { "lr-list", 0, 0, "", NULL, nbctl_lr_list, NULL, "", RO }, /* logical router port commands. */ - { "lrp-add", 4, 5, "ROUTER PORT MAC NETWORK [PEER]", NULL, nbctl_lrp_add, - NULL, "--may-exist", RW }, + { "lrp-add", 4, INT_MAX, "ROUTER PORT MAC NETWORK [PEER]", NULL, + nbctl_lrp_add, NULL, "--may-exist", RW }, { "lrp-del", 1, 1, "LPORT", NULL, nbctl_lrp_del, NULL, "--if-exists", RW }, { "lrp-list", 1, 1, "ROUTER", NULL, nbctl_lrp_list, NULL, "", RO }, { "lrp-set-enabled", 2, 2, "PORT STATE", NULL, nbctl_lrp_set_enabled, diff --git a/tests/ovn-nbctl.at b/tests/ovn-nbctl.at index 91d482b..25ebe28 100644 --- a/tests/ovn-nbctl.at +++ b/tests/ovn-nbctl.at @@ -331,6 +331,15 @@ AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/2 AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp1 00:00:00:01:02:03 192.168.1.1/24 lrp1-peer]) AT_CHECK([ovn-nbctl lrp-del lrp1]) + +AT_CHECK([ovn-nbctl lrp-add lr0 lrp2 00:00:00:01:02:03 2001:db8::1 2001:db8::2 2001:db8::3]) + +AT_CHECK([ovn-nbctl --may-exist lrp-add lr0 lrp2 00:00:00:01:02:03 2001:db8::4 2001:db8::2], [1], [], + [ovn-nbctl: lrp2: port already exists with network 2001:db8::1 2001:db8::2 2001:db8::3 +]) + +AT_CHECK([ovn-nbctl lrp-del lrp2]) + AT_CHECK([ovn-nbctl lrp-list lr0 | ${PERL} $srcdir/uuidfilt.pl], [0], [dnl <0> (lrp0) ])
This patch renames Logical_Router_Port 'network' column to 'networks' and aligns it to Logical_Switch_Port 'addresses' column for peer port in Logical_Switch_Port which type is 'router'. Currently, a lsp port whos type is 'router', may have multiple addresses for IPv6 address case, but it's peer lrp port has only 1 address in 'network' column. It's inconsistent, this patch tries to fix that. Signed-off-by: Zong Kai LI <zealokii@gmail.com> --- ovn/northd/ovn-northd.c | 5 +++-- ovn/ovn-nb.ovsschema | 6 ++++-- ovn/ovn-nb.xml | 7 +++--- ovn/utilities/ovn-nbctl.c | 54 +++++++++++++++++++++++++++++++++++------------ tests/ovn-nbctl.at | 9 ++++++++ 5 files changed, 61 insertions(+), 20 deletions(-)