On 24 April 2016 at 02:49, steve.ruan <rsxi...@gmail.com> 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 <rua...@cn.ibm.com> > Co-authored-by: Gurucharan Shetty <g...@ovn.org> > > Reported-by: Na Zhu <na...@cn.ibm.com> > Reported-by: Dustin Lundquist <dlundqu...@linux.vnet.ibm.com> > > 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; > </p> > > <p> > - If the route has a gateway, <var>G</var> is the gateway IP > address, > - otherwise it is <code>ip4.dst</code>. > + If the route has a gateway, <var>G</var> is the gateway IP > address. > + Instead, if the route is from a configured static route, > <var>G</var> > + is the next hop IP address. Else it is <code>ip4.dst</code>. > </p> > </li> > > 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. > </column> > > + <column name="static_routes"> > + One or more static routes, refer to Logical_Router_Static_Route > table. > + </column> > + > <column name="default_gw"> > IP address to use as default gateway, if any. > </column> > @@ -724,4 +728,31 @@ > </column> > </group> > </table> > + > + <table name="Logical_Router_Static_Route" title="logical router static > routes"> > + <p> > + Each route represents a static route. > + </p> > + > + <column name="prefix"> > + <p> > + Prefix of this route, example 192.168.100.0/24. > + </p> > + </column> > + > + <column name="nexthop"> > + <p> > + 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. diff --git a/AUTHORS b/AUTHORS index 3f05a02..5290ef5 100644 --- a/AUTHORS +++ b/AUTHORS @@ -211,6 +211,7 @@ Steffen Gebert steffen.geb...@informatik.uni-wuerzburg.de Sten Spans s...@blinkenlights.nl Stephane A. Sezer s...@cd80.net Stephen Finucane stephen.finuc...@intel.com +Steve Ruan rua...@cn.ibm.com SUGYO Kazushi sugyo....@gmail.com Tadaaki Nagao na...@stratosphere.co.jp Terry Wilson twil...@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 @@ </column> <column name="static_routes"> - One or more static routes, refer to Logical_Router_Static_Route table. + One or more static routes for the router. </column> <column name="default_gw"> @@ -729,28 +729,29 @@ </group> </table> - <table name="Logical_Router_Static_Route" title="logical router static routes"> + <table name="Logical_Router_Static_Route" title="Logical router static routes"> <p> - Each route represents a static route. + Each record represents a static route. </p> - <column name="prefix"> + <column name="ip_prefix"> <p> - Prefix of this route, example 192.168.100.0/24. + IP prefix of this route (e.g. 192.168.100.0/24). </p> </column> - + <column name="nexthop"> <p> - 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. </p> </column> - + <column name="output_port"> <p> - Port name of the logical router port table. It may be configured or may - not be configured. + The name of the <ref table="Logical_router_port"/> 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 <ref column="nexthop"/>. </p> </column> </table> 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 # Create logical port foo1 in foo _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
