[ovs-dev] [PATCH V5 1/1] ovn-northd: Add support for static_routes.

Guru Shetty guru at ovn.org
Wed Apr 27 17:05:31 UTC 2016


On 24 April 2016 at 02:49, steve.ruan <rsxin25 at 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 <ruansx at cn.ibm.com>
> Co-authored-by: Gurucharan Shetty <guru at ovn.org>
>
> Reported-by: Na Zhu <nazhu at cn.ibm.com>
> Reported-by: Dustin Lundquist <dlundquist at 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.gebert at informatik.uni-wuerzburg.de
 Sten Spans              sten at blinkenlights.nl
 Stephane A. Sezer       sas at cd80.net
 Stephen Finucane        stephen.finucane at intel.com
+Steve Ruan              ruansx at cn.ibm.com
 SUGYO Kazushi           sugyo.org at gmail.com
 Tadaaki Nagao           nagao at stratosphere.co.jp
 Terry Wilson            twilson at 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



More information about the dev mailing list