From patchwork Wed Apr 27 17:05:31 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Gurucharan Shetty X-Patchwork-Id: 615749 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (archives.nicira.com [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3qw5vC1zwbz9t4Z for ; Thu, 28 Apr 2016 03:05:43 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id 5A6521058F; Wed, 27 Apr 2016 10:05:42 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e4.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id 0864C10566 for ; Wed, 27 Apr 2016 10:05:41 -0700 (PDT) Received: from bar5.cudamail.com (unknown [192.168.21.12]) by mx1e4.cudamail.com (Postfix) with ESMTPS id 7F2E31E01AD for ; Wed, 27 Apr 2016 11:05:40 -0600 (MDT) X-ASG-Debug-ID: 1461776739-09eadd6dea2b4a40001-byXFYA Received: from mx3-pf2.cudamail.com ([192.168.14.1]) by bar5.cudamail.com with ESMTP id VT8dsLtCxeSynW33 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Wed, 27 Apr 2016 11:05:39 -0600 (MDT) X-Barracuda-Envelope-From: guru@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.14.1 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx3-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 27 Apr 2016 17:05:38 -0000 Received-SPF: pass (mx3-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.196 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.196 X-Barracuda-RBL-IP: 217.70.183.196 Received: from mfilter42-d.gandi.net (mfilter42-d.gandi.net [217.70.178.172]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id 0906E1720BA for ; Wed, 27 Apr 2016 19:05:36 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter42-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter42-d.gandi.net (mfilter42-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id PbOmAPwDKL8A for ; Wed, 27 Apr 2016 19:05:32 +0200 (CEST) X-Originating-IP: 209.85.215.53 Received: from mail-lf0-f53.google.com (mail-lf0-f53.google.com [209.85.215.53]) (Authenticated sender: guru@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id 364331721CE for ; Wed, 27 Apr 2016 19:05:32 +0200 (CEST) Received: by mail-lf0-f53.google.com with SMTP id j11so64460669lfb.1 for ; Wed, 27 Apr 2016 10:05:32 -0700 (PDT) X-Gm-Message-State: AOPr4FWdIB5bJUhClsvly4aOFUwObubMCx4gJwtNJuueBf9th/GEHlXLjwE2c0YVwqEQtL/9VBBXgLP2KSR/zA== MIME-Version: 1.0 X-Received: by 10.25.28.140 with SMTP id c134mr3268703lfc.99.1461776731617; Wed, 27 Apr 2016 10:05:31 -0700 (PDT) Received: by 10.114.181.5 with HTTP; Wed, 27 Apr 2016 10:05:31 -0700 (PDT) In-Reply-To: <1461491344-16084-1-git-send-email-ruansx@cn.ibm.com> References: <1461491344-16084-1-git-send-email-ruansx@cn.ibm.com> Date: Wed, 27 Apr 2016 10:05:31 -0700 X-Gmail-Original-Message-ID: Message-ID: X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-V2-426034541 X-CudaMail-DTE: 042716 X-CudaMail-Originating-IP: 217.70.183.196 X-CudaMail-Envelope-Sender: guru@ovn.org X-ASG-Orig-Subj: [##CM-V2-426034541##]Re: [ovs-dev] [PATCH V5 1/1] ovn-northd: Add support for static_routes. From: Guru Shetty To: "steve.ruan" X-Barracuda-Connect: UNKNOWN[192.168.14.1] X-Barracuda-Start-Time: 1461776739 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 X-Content-Filtered-By: Mailman/MimeDel 2.1.16 Cc: ovs dev Subject: Re: [ovs-dev] [PATCH V5 1/1] ovn-northd: Add support for static_routes. X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" On 24 April 2016 at 02:49, steve.ruan wrote: Thank you for working through this. I have a few comments. With this patch, your author name becomes "steve.ruan". Did you intend it to be Steve Ruan instead? Your email address in the author name (gmail) is different than the email address in your signed-off-by (IBM). They need to be the same. static routes are useful when connecting multiple > routers with each other. > > Signed-off-by: steve.ruan > Co-authored-by: Gurucharan Shetty > > Reported-by: Na Zhu > Reported-by: Dustin Lundquist > > Reported-at: > https://bugs.launchpad.net/networking-ovn/+bug/1545140 > https://bugs.launchpad.net/networking-ovn/+bug/1539347 > --- > ovn/northd/ovn-northd.8.xml | 5 +- > ovn/northd/ovn-northd.c | 81 +++++++++++++++++ > ovn/ovn-nb.ovsschema | 15 +++- > ovn/ovn-nb.xml | 31 +++++++ > ovn/utilities/ovn-nbctl.8.xml | 5 ++ > ovn/utilities/ovn-nbctl.c | 5 ++ > tests/ovn.at | 203 > ++++++++++++++++++++++++++++++++++++++++++ > 7 files changed, 341 insertions(+), 4 deletions(-) > You will also need to add yourself to AUTHORS file. > > diff --git a/ovn/northd/ovn-northd.8.xml b/ovn/northd/ovn-northd.8.xml > index 1cd8072..970c352 100644 > --- a/ovn/northd/ovn-northd.8.xml > +++ b/ovn/northd/ovn-northd.8.xml > @@ -689,8 +689,9 @@ next; >

> >

> - If the route has a gateway, G is the gateway IP > address, > - otherwise it is ip4.dst. > + If the route has a gateway, G is the gateway IP > address. > + Instead, if the route is from a configured static route, > G > + is the next hop IP address. Else it is ip4.dst. >

> > > diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c > index e3436da..e2c5a78 100644 > --- a/ovn/northd/ovn-northd.c > +++ b/ovn/northd/ovn-northd.c > @@ -1828,6 +1828,79 @@ add_route(struct hmap *lflows, const struct > ovn_port *op, > } > > static void > +build_static_route_flow(struct hmap *lflows, struct ovn_datapath *od, > + struct hmap *ports, > + const struct nbrec_logical_router_static_route *route) > +{ > + struct ovn_port *out_port, *op; > + ovs_be32 prefix, next_hop, mask; > + int len; > + > + /* verify nexthop */ > From CodingStyle: Comments should be written as full sentences that start with a capital letter and end with a period. Put two spaces between sentences. I have the same comment for everywhere else. > + char *error = ip_parse_masked(route->nexthop, &next_hop, &mask); > + if (error || mask != OVS_BE32_MAX) { > + static struct vlog_rate_limit rl > + = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "bad next hop ip address %s", > + route->nexthop); > + free(error); > + return; > + } > + > + /* verify prefix */ > + error = ip_parse_masked(route->prefix, &prefix, &mask); > + if (error || !ip_is_cidr(mask)) { > + static struct vlog_rate_limit rl > + = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "bad 'network' in static routes %s", > + route->prefix); > + free(error); > + return; > + } > + > + /* find the outgoing port */ > + out_port = NULL; > + len = 0; > + if (route->output_port){ > You need a space before "{" (similar violations at other places) > + out_port = ovn_port_find(ports, route->output_port); > + if (!out_port){ > + static struct vlog_rate_limit rl > + = VLOG_RATE_LIMIT_INIT(5, 1); > + VLOG_WARN_RL(&rl, "bad 'out port' in static routes %s", > + route->output_port); > + return; > + } > + } > else below should be in the previous line. Have a look at CodingStyle document. > + else{ /* output_port is not specified, then find the > ... ... > + * router port with longest net mask match */diff --git > a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema > index e3e41e3..e67f903 100644 > --- a/ovn/ovn-nb.ovsschema > +++ b/ovn/ovn-nb.ovsschema > @@ -1,7 +1,7 @@ > { > "name": "OVN_Northbound", > - "version": "2.1.0", > - "cksum": "2201582413 4513", > + "version": "2.1.1", > + "cksum": "1773273858 5105", > "tables": { > "Logical_Switch": { > "columns": { > @@ -71,6 +71,11 @@ > "refType": "strong"}, > "min": 0, > "max": "unlimited"}}, > + "static_routes": {"type": {"key": {"type": "uuid", > + "refTable": > "Logical_Router_Static_Route", > + "refType": "strong"}, > + "min": 0, > + "max": "unlimited"}}, > "default_gw": {"type": {"key": "string", "min": 0, "max": > 1}}, > "enabled": {"type": {"key": "boolean", "min": 0, "max": > 1}}, > "external_ids": { > @@ -88,6 +93,12 @@ > "type": {"key": "string", "value": "string", > "min": 0, "max": "unlimited"}}}, > "indexes": [["name"]], > + "isRoot": false}, > + "Logical_Router_Static_Route": { > + "columns": { > + "prefix": {"type": "string"}, > I think something like "ip_prefix" is a better name than just "prefix". > + "nexthop": {"type": "string"}, > + "output_port": {"type": {"key": "string", "min": 0, > "max": 1}}}, > "isRoot": false} > } > } > diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml > index 843ae4c..b7091c2 100644 > --- a/ovn/ovn-nb.xml > +++ b/ovn/ovn-nb.xml > @@ -623,6 +623,10 @@ > The router's ports. > > > + > + One or more static routes, refer to Logical_Router_Static_Route > table. > + > + > > IP address to use as default gateway, if any. > > @@ -724,4 +728,31 @@ > > > > + > + > +

> + Each route represents a static route. > +

> + > + > +

> + Prefix of this route, example 192.168.100.0/24. > +

> + > + > + > +

> + Nexthop of this route, nexthop can be a IP address of neutron > port, or > + IP address which has been learnt by dynamic ARP. > Since this is OVN and we neutron is just one of the many upstream users, we should avoid naming them. Also, looking at the code, it looks like we currently only handle the case where the nexthop is a router port's IP address. The documentation can be updated if and when there is code to support something different. There were a few other stylistic and codingstyle violations. I also made changes to documentation to suit OVN's style. I am pasting the entire incremental difference. If you are happy with it, I will apply this patch. If not, please choose the ones you are happy with and respin. # Create logical port foo1 in foo diff --git a/AUTHORS b/AUTHORS index 3f05a02..5290ef5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -211,6 +211,7 @@ Steffen Gebert steffen.gebert@informatik.uni-wuerzburg.de Sten Spans sten@blinkenlights.nl Stephane A. Sezer sas@cd80.net Stephen Finucane stephen.finucane@intel.com +Steve Ruan ruansx@cn.ibm.com SUGYO Kazushi sugyo.org@gmail.com Tadaaki Nagao nagao@stratosphere.co.jp Terry Wilson twilson@redhat.com diff --git a/ovn/northd/ovn-northd.c b/ovn/northd/ovn-northd.c index ea1dd6b..9372d5a 100644 --- a/ovn/northd/ovn-northd.c +++ b/ovn/northd/ovn-northd.c @@ -1828,70 +1828,65 @@ add_route(struct hmap *lflows, const struct ovn_port *op, static void build_static_route_flow(struct hmap *lflows, struct ovn_datapath *od, - struct hmap *ports, - const struct nbrec_logical_router_static_route *route) + struct hmap *ports, + const struct nbrec_logical_router_static_route *route) { struct ovn_port *out_port, *op; ovs_be32 prefix, next_hop, mask; int len; - /* verify nexthop */ + /* Verify nexthop ip address. */ char *error = ip_parse_masked(route->nexthop, &next_hop, &mask); if (error || mask != OVS_BE32_MAX) { - static struct vlog_rate_limit rl - = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL(&rl, "bad next hop ip address %s", - route->nexthop); + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "bad next hop ip address %s", route->nexthop); free(error); return; } - /* verify prefix */ - error = ip_parse_masked(route->prefix, &prefix, &mask); + /* Verify IP prefix. */ + error = ip_parse_masked(route->ip_prefix, &prefix, &mask); if (error || !ip_is_cidr(mask)) { - static struct vlog_rate_limit rl - = VLOG_RATE_LIMIT_INIT(5, 1); + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "bad 'network' in static routes %s", - route->prefix); + route->ip_prefix); free(error); return; } - /* find the outgoing port */ + /* Find the outgoing port. */ out_port = NULL; len = 0; - if (route->output_port){ + if (route->output_port) { out_port = ovn_port_find(ports, route->output_port); - if (!out_port){ - static struct vlog_rate_limit rl - = VLOG_RATE_LIMIT_INIT(5, 1); + if (!out_port) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); VLOG_WARN_RL(&rl, "bad 'out port' in static routes %s", - route->output_port); + route->output_port); return; } - } - else{ /* output_port is not specified, then find the - * router port with longest net mask match */ - for (int i = 0; i < od->nbr->n_ports; i++){ - + } else { + /* Since output_port is not specified, find the + * router port with longest prefix match */ + for (int i = 0; i < od->nbr->n_ports; i++) { struct nbrec_logical_router_port *lrp = od->nbr->ports[i]; op = ovn_port_find(ports, lrp->name); - if (!op){ /* this should not happen */ + if (!op) { + /* This should not happen. */ continue; } - if (op->network && !((op->network ^ next_hop) & op->mask)){ - if (ip_count_cidr_bits(op->mask) > len){ + if (op->network && !((op->network ^ next_hop) & op->mask)) { + if (ip_count_cidr_bits(op->mask) > len) { len = ip_count_cidr_bits(op->mask); out_port = op; } } } - if (!out_port){ - static struct vlog_rate_limit rl - = VLOG_RATE_LIMIT_INIT(5, 1); - VLOG_WARN_RL(&rl, "no matched out port for next hop %s", - route->nexthop); + if (!out_port) { + static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 1); + VLOG_WARN_RL(&rl, "no matched outport for next hop %s", + route->nexthop); return; } } @@ -2068,8 +2063,8 @@ build_lrouter_flows(struct hmap *datapaths, struct hmap *ports, continue; } - /* convert the routing table to flow */ - for (int i = 0; i < od->nbr->n_static_routes; i++){ + /* Convert the static routes to flows. */ + for (int i = 0; i < od->nbr->n_static_routes; i++) { const struct nbrec_logical_router_static_route *route; route = od->nbr->static_routes[i]; diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema index e67f903..8163f6a 100644 --- a/ovn/ovn-nb.ovsschema +++ b/ovn/ovn-nb.ovsschema @@ -1,7 +1,7 @@ { "name": "OVN_Northbound", "version": "2.1.1", - "cksum": "1773273858 5105", + "cksum": "2615511875 5108", "tables": { "Logical_Switch": { "columns": { @@ -96,7 +96,7 @@ "isRoot": false}, "Logical_Router_Static_Route": { "columns": { - "prefix": {"type": "string"}, + "ip_prefix": {"type": "string"}, "nexthop": {"type": "string"}, "output_port": {"type": {"key": "string", "min": 0, "max": 1}}}, "isRoot": false} diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml index b7091c2..943b1a6 100644 --- a/ovn/ovn-nb.xml +++ b/ovn/ovn-nb.xml @@ -624,7 +624,7 @@ - One or more static routes, refer to Logical_Router_Static_Route table. + One or more static routes for the router. @@ -729,28 +729,29 @@

- +

- Each route represents a static route. + Each record represents a static route.

- +

- Prefix of this route, example 192.168.100.0/24. + IP prefix of this route (e.g. 192.168.100.0/24).

- +

- Nexthop of this route, nexthop can be a IP address of neutron port, or - IP address which has been learnt by dynamic ARP. + Nexthop IP address for this route. Nexthop IP address should be the IP + address of a connected router port.

- +

- Port name of the logical router port table. It may be configured or may - not be configured. + The name of the via which the packet + needs to be sent out. This is optional and when not specified, OVN will + automatically figure this out based on the .

diff --git a/tests/ovn.at b/tests/ovn.at index 39494ea..1993d8c 100644 --- a/tests/ovn.at +++ b/tests/ovn.at @@ -2398,15 +2398,15 @@ ovn-nbctl set logical_router_port $lrp2_uuid peer="R1_R2" #install static routes ovn-nbctl -- --id=@lrt create Logical_Router_Static_Route \ -prefix=172.16.1.0/24 nexthop=20.0.0.2 -- add Logical_Router \ +ip_prefix=172.16.1.0/24 nexthop=20.0.0.2 -- add Logical_Router \ R1 static_routes @lrt ovn-nbctl -- --id=@lrt create Logical_Router_Static_Route \ -prefix=172.16.2.0/24 nexthop=20.0.0.2 -- add Logical_Router \ +ip_prefix=172.16.2.0/24 nexthop=20.0.0.2 -- add Logical_Router \ R1 static_routes @lrt ovn-nbctl -- --id=@lrt create Logical_Router_Static_Route \ -prefix=192.168.1.0/24 nexthop=20.0.0.1 -- add Logical_Router \ +ip_prefix=192.168.1.0/24 nexthop=20.0.0.1 -- add Logical_Router \ R2 static_routes @lrt