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

Mickey Spiegel emspiege at us.ibm.com
Tue Apr 12 00:25:21 UTC 2016


Gurucharan Shetty <guru at ovn.org> wrote:
>To: dev at openvswitch.org
>From: Gurucharan Shetty 
>Sent by: "dev" 
>Date: 04/11/2016 07:46AM
>Cc: Gurucharan Shetty <guru at ovn.org>
>Subject: [ovs-dev] [PATCH] ovn-northd: Add support for static_routes.
>
>static routes are useful when connecting multiple
>routers with each other.

Yes they are.
They are also useful for other cases such as directing traffic to a network appliance, for example OpenStack VPNaaS.
Static routes can also be used to direct traffic externally.

>
>Signed-off-by: Gurucharan Shetty <guru at ovn.org>
>---
>This patch does not add a outport to the schema. I think that
>enhancement should be easily addable as soon as there is an
>upstream user. We still have 2-3 months before the next release.

I see this claim, and yes it is possible to add outport or other fields to the schema proposed in this patch, but is it clean?
I see two areas where IMO it would be easier to add outport using the approach in Steve's patch:

1)  This patch already puts two values in the proposed static_routes column  of the Logical_Router table in OVN_Northbound, prefix and next_hop, for  example "172.16.1.0/24=20.0.0.2". IMO, putting three values, prefix,  next_hop, and outport in one static_routes column of OVN_Northbound is  going a bit far.
Steve's patch proposes a separate table  Logical_Router_Static_Route, with separate columns for prefix and  nexthop. With that approach, adding outport is as simple as adding a  column to the table.
Also note that I am now contemplating adding  another column for the EVPN case, to specify the new DMAC for the  packet, instead of going through the ARP_Resolve stage based on the next  hop. It is a little soon to say whether this will really be necessary,  but it does point out that we may need extensibility of any route  construct going forward.
IMO, routes are a basic and important enough construct to justify another table.
By  using the tables, we also let the IDL take care of building  ovn-northd.c data structures for static routes, without requiring  definition and population of specific data structures such as route_port  proposed in this patch.
Note: For both Steve's patch and this patch,  I think the abstraction should not be limited to static routes, so the  name should be something like prefix_routes, ovn_routes, or  router_routes, rather than static_routes. We would like to reuse the  same constructs for dynamic routes in the case of OpenStack BGP Dynamic  Routing and BGP EVPN.

2) In this patch, in ovn-northd.c, for each  logical router port, it walks through that logical router's static  routes to see if this logical router port is the current longest match  for the static route, in which case it overwrites  route_port->ovn_port. If the static route already specifies the  outport, this logic should not apply.
With Steve's patch, for each  datapath, it walks that datapath's static routes, calling  build_static_route_flow. In build_static_route_flow, to find the  outgoing port, it walks that router's ports looking for the longest  match. This nested inner loop can easily be skipped if an outport is  provided by the static route.
When the logical router ports are the  outer loop rather than the inner loop, it is not as easy and  straightforward to skip this logic.

The details mostly look good  at first glance. We should probably resolve what to do about the two  competing static route patches before dealing with nits.

Mickey

>---
> ovn/northd/ovn-northd.8.xml |    5 +-
> ovn/northd/ovn-northd.c     |   75 +++++++++++++++++
> ovn/ovn-nb.ovsschema        |    8 +-
> ovn/ovn-nb.xml              |    4 +
> tests/ovn.at                |  195
>+++++++++++++++++++++++++++++++++++++++++++
> 5 files changed, 283 insertions(+), 4 deletions(-)
>
>diff --git a/ovn/northd/ovn-northd.8.xml
>b/ovn/northd/ovn-northd.8.xml
>index 465b7c7..7dbefa9 100644
>--- a/ovn/northd/ovn-northd.8.xml
>+++ b/ovn/northd/ovn-northd.8.xml
>@@ -680,8 +680,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 4b1d611..2da17b2 100644
>--- a/ovn/northd/ovn-northd.c
>+++ b/ovn/northd/ovn-northd.c
>@@ -233,6 +233,17 @@ allocate_tnlid(struct hmap *set, const char
>*name, uint32_t max,
>     return 0;
> }
> 
>+/* Holds the next hop ip address and the logical router port via
>which
>+ * a static route is reachable. */
>+struct route_to_port {
>+    ovs_be32 ip;                /* network address of the route. */
>+    ovs_be32 mask;              /* network mask of the route. */
>+    ovs_be32 next_hop;          /* next_hop ip address for the above
>route. */
>+    struct ovn_port *ovn_port;  /* The logical router port via which
>the packet
>+                                 * needs to exit to reach the next
>hop. */
>+};
>+
>+
> /* The 'key' comes from nbs->header_.uuid or nbr->header_.uuid or
>  * sb->external_ids:logical-switch. */
> struct ovn_datapath {
>@@ -248,6 +259,9 @@ struct ovn_datapath {
>     /* Logical router data (digested from nbr). */
>     const struct ovn_port *gateway_port;
>     ovs_be32 gateway;
>+    /* Maps a static route to a ovn logical router port via which
>packet
>+     * needs to exit. */
>+    struct shash routes_map;
> 
>     /* Logical switch data. */
>     struct ovn_port **router_ports;
>@@ -271,6 +285,7 @@ ovn_datapath_create(struct hmap *datapaths, const
>struct uuid *key,
>     od->nbs = nbs;
>     od->nbr = nbr;
>     hmap_init(&od->port_tnlids);
>+    shash_init(&od->routes_map);
>     od->port_key_hint = 0;
>     hmap_insert(datapaths, &od->key_node, uuid_hash(&od->key));
>     return od;
>@@ -285,6 +300,7 @@ ovn_datapath_destroy(struct hmap *datapaths,
>struct ovn_datapath *od)
>          * use it. */
>         hmap_remove(datapaths, &od->key_node);
>         destroy_tnlids(&od->port_tnlids);
>+        shash_destroy_free_data(&od->routes_map);
>         free(od->router_ports);
>         free(od);
>     }
>@@ -408,6 +424,39 @@ join_datapaths(struct northd_context *ctx,
>struct hmap *datapaths,
>         /* Set the gateway port to NULL.  If there is a gateway, it
>will get
>          * filled in as we go through the ports later. */
>         od->gateway_port = NULL;
>+
>+        /* Handle static routes set in this logical router. */
>+        struct smap_node *node;
>+        SMAP_FOR_EACH(node, &nbr->static_routes) {
>+            ovs_be32 next_hop;
>+            if (!ip_parse(node->value, &next_hop) || !next_hop) {
>+                static struct vlog_rate_limit rl =
>VLOG_RATE_LIMIT_INIT(5, 1);
>+                VLOG_WARN_RL(&rl, "bad next hop ip address %s for
>router"
>+                             ""UUID_FMT"", node->value,
>+                             UUID_ARGS(&nbr->header_.uuid));
>+                continue;
>+            }
>+
>+            ovs_be32 ip, mask;
>+            char *error = ip_parse_masked(node->key, &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' in static routes
>%s",
>+                             node->key);
>+                free(error);
>+                continue;
>+            }
>+
>+            struct route_to_port *route_port = xmalloc(sizeof
>*route_port);
>+            route_port->ip = ip;
>+            route_port->mask = mask;
>+            route_port->next_hop = next_hop;
>+            /* The correct ovn_port will be filled while traversing
>+             * logical_router_ports. */
>+            route_port->ovn_port = NULL;
>+            shash_add(&od->routes_map, node->key, route_port);
>+        }
>     }
> }
> 
>@@ -643,6 +692,23 @@ join_logical_ports(struct northd_context *ctx,
>                         od->gateway_port = op;
>                     }
>                 }
>+
>+                /* Go through the static routes of the router to see
>which
>+                 * route is reachable via this logical router port.
>*/
>+                struct shash_node *node;
>+                SHASH_FOR_EACH(node, &od->routes_map) {
>+                    struct route_to_port *route_port = node->data;
>+                    /* If 'od' has a static route and 'op' routes to
>it... */
>+                    if (!((op->network ^ route_port->next_hop) &
>op->mask)) {
>+                        /* .....and if 'op' is a longer match than
>the current
>+                         * choice... */
>+                        const struct ovn_port *curr =
>route_port->ovn_port;
>+                        int len = curr ?
>ip_count_cidr_bits(curr->mask) : 0;
>+                        if (ip_count_cidr_bits(op->mask) > len) {
>+                            route_port->ovn_port = op;
>+                        }
>+                    }
>+                }
>             }
>         }
>     }
>@@ -1948,6 +2014,15 @@ build_lrouter_flows(struct hmap *datapaths,
>struct hmap *ports,
>             continue;
>         }
> 
>+        struct shash_node *node;
>+        SHASH_FOR_EACH(node, &od->routes_map) {
>+            struct route_to_port *route_port = node->data;
>+            if (route_port->ovn_port) {
>+                add_route(lflows, route_port->ovn_port,
>route_port->ip,
>+                          route_port->mask, route_port->next_hop);
>+            }
>+        }
>+
>         if (od->gateway && od->gateway_port) {
>             add_route(lflows, od->gateway_port, 0, 0, od->gateway);
>         }
>diff --git a/ovn/ovn-nb.ovsschema b/ovn/ovn-nb.ovsschema
>index 40a7a97..9f6b193 100644
>--- a/ovn/ovn-nb.ovsschema
>+++ b/ovn/ovn-nb.ovsschema
>@@ -1,7 +1,7 @@
> {
>     "name": "OVN_Northbound",
>-    "version": "2.0.2",
>-    "cksum": "4289495412 4436",
>+    "version": "2.0.3",
>+    "cksum": "4222091871 4648",
>     "tables": {
>         "Logical_Switch": {
>             "columns": {
>@@ -72,6 +72,10 @@
>                                    "min": 0,
>                                    "max": "unlimited"}},
>                 "default_gw": {"type": {"key": "string", "min": 0,
>"max": 1}},
>+                "static_routes" : {
>+                    "type": {"key": {"type": "string"},
>+                             "value": {"type" : "string"},
>+                             "min": 0, "max": "unlimited"}},
>                 "external_ids": {
>                     "type": {"key": "string", "value": "string",
>                              "min": 0, "max": "unlimited"}}},
>diff --git a/ovn/ovn-nb.xml b/ovn/ovn-nb.xml
>index e65bc3a..69d565c 100644
>--- a/ovn/ovn-nb.xml
>+++ b/ovn/ovn-nb.xml
>@@ -627,6 +627,10 @@
>       IP address to use as default gateway, if any.
>     </column>
> 
>+    <column name="static_routes">
>+      One or more static routes, mapping IP prefixes to next hop IP
>addresses.
>+    </column>
>+
>     <group title="Common Columns">
>       <column name="external_ids">
>         See <em>External IDs</em> at the beginning of this document.
>diff --git a/tests/ovn.at b/tests/ovn.at
>index 22121e1..3e726e7 100644
>--- a/tests/ovn.at
>+++ b/tests/ovn.at
>@@ -2141,3 +2141,198 @@ OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
> OVS_APP_EXIT_AND_WAIT([ovsdb-server])
> 
> AT_CLEANUP
>+
>+AT_SETUP([ovn -- 2 HVs, 3 LS, 1 lport/LS, 2 peer LRs, static
>routes])
>+AT_KEYWORDS([ovnstaticroutespeer])
>+AT_SKIP_IF([test $HAVE_PYTHON = no])
>+ovn_start
>+
>+# Logical network:
>+# Two LRs - R1 and R2 that are connected to each other as peers in
>20.0.0.0/24
>+# network. R1 has switchess foo (192.168.1.0/24)
>+# connected to it.
>+# R2 has alice (172.16.1.0/24) and bob (172.16.2.0/24) connected to
>it.
>+
>+ovn-nbctl create Logical_Router name=R1
>+ovn-nbctl create Logical_Router name=R2
>+
>+ovn-nbctl lswitch-add foo
>+ovn-nbctl lswitch-add alice
>+ovn-nbctl lswitch-add bob
>+
>+# Connect foo to R1
>+ovn-nbctl -- --id=@lrp create Logical_Router_port name=foo \
>+network=192.168.1.1/24 mac=\"00:00:00:01:02:03\" -- add
>Logical_Router R1 \
>+ports @lrp -- lport-add foo rp-foo
>+
>+ovn-nbctl set Logical_port rp-foo type=router
>options:router-port=foo \
>+addresses=\"00:00:00:01:02:03\"
>+
>+# Connect alice to R2
>+ovn-nbctl -- --id=@lrp create Logical_Router_port name=alice \
>+network=172.16.1.1/24 mac=\"00:00:00:01:02:04\" -- add
>Logical_Router R2 \
>+ports @lrp -- lport-add alice rp-alice
>+
>+ovn-nbctl set Logical_port rp-alice type=router
>options:router-port=alice \
>+addresses=\"00:00:00:01:02:04\"
>+
>+# Connect bob to R2
>+ovn-nbctl -- --id=@lrp create Logical_Router_port name=bob \
>+network=172.16.2.1/24 mac=\"00:00:00:01:02:05\" -- add
>Logical_Router R2 \
>+ports @lrp -- lport-add bob rp-bob
>+
>+ovn-nbctl set Logical_port rp-bob type=router
>options:router-port=bob \
>+addresses=\"00:00:00:01:02:05\"
>+
>+# Connect R1 to R2
>+lrp1_uuid=`ovn-nbctl -- --id=@lrp create Logical_Router_port
>name=R1_R2 \
>+network="20.0.0.1/24" mac=\"00:00:00:02:03:04\" \
>+-- add Logical_Router R1 ports @lrp`
>+
>+lrp2_uuid=`ovn-nbctl -- --id=@lrp create Logical_Router_port
>name=R2_R1 \
>+network="20.0.0.2/24" mac=\"00:00:00:02:03:05\" \
>+-- add Logical_Router R2 ports @lrp`
>+
>+ovn-nbctl set logical_router_port $lrp1_uuid peer="R2_R1"
>+ovn-nbctl set logical_router_port $lrp2_uuid peer="R1_R2"
>+
>+ovn-nbctl set Logical_Router R1 static_routes:172.16.1.0/24=20.0.0.2
>+ovn-nbctl set logical_router R1 static_routes:172.16.2.0/24=20.0.0.2
>+ovn-nbctl set logical_router R2
>static_routes:192.168.1.0/24=20.0.0.1
>+
>+# Create logical port foo1 in foo
>+ovn-nbctl lport-add foo foo1 \
>+-- lport-set-addresses foo1 "f0:00:00:01:02:03 192.168.1.2"
>+
>+# Create logical port alice1 in alice
>+ovn-nbctl lport-add alice alice1 \
>+-- lport-set-addresses alice1 "f0:00:00:01:02:04 172.16.1.2"
>+
>+# Create logical port bob1 in bob
>+ovn-nbctl lport-add bob bob1 \
>+-- lport-set-addresses bob1 "f0:00:00:01:02:05 172.16.2.2"
>+
>+# Create two hypervisor and create OVS ports corresponding to
>logical ports.
>+net_add n1
>+
>+sim_add hv1
>+as hv1
>+ovs-vsctl add-br br-phys
>+ovn_attach n1 br-phys 192.168.0.1
>+ovs-vsctl -- add-port br-int hv1-vif1 -- \
>+    set interface hv1-vif1 external-ids:iface-id=foo1 \
>+    options:tx_pcap=hv1/vif1-tx.pcap \
>+    options:rxq_pcap=hv1/vif1-rx.pcap \
>+    ofport-request=1
>+
>+ovs-vsctl -- add-port br-int hv1-vif2 -- \
>+    set interface hv1-vif2 external-ids:iface-id=alice1 \
>+    options:tx_pcap=hv1/vif2-tx.pcap \
>+    options:rxq_pcap=hv1/vif2-rx.pcap \
>+    ofport-request=2
>+
>+sim_add hv2
>+as hv2
>+ovs-vsctl add-br br-phys
>+ovn_attach n1 br-phys 192.168.0.2
>+ovs-vsctl -- add-port br-int hv2-vif1 -- \
>+    set interface hv2-vif1 external-ids:iface-id=bob1 \
>+    options:tx_pcap=hv2/vif1-tx.pcap \
>+    options:rxq_pcap=hv2/vif1-rx.pcap \
>+    ofport-request=1
>+
>+
>+# Pre-populate the hypervisors' ARP tables so that we don't lose any
>+# packets for ARP resolution (native tunneling doesn't queue packets
>+# for ARP resolution).
>+ovn_populate_arp
>+
>+# Allow some time for ovn-northd and ovn-controller to catch up.
>+# XXX This should be more systematic.
>+sleep 1
>+
>+ip_to_hex() {
>+    printf "%02x%02x%02x%02x" "$@"
>+}
>+trim_zeros() {
>+    sed 's/\(00\)\{1,\}$//'
>+}
>+
>+# Send ip packets between foo1 and alice1
>+src_mac="f00000010203"
>+dst_mac="000000010203"
>+src_ip=`ip_to_hex 192 168 1 2`
>+dst_ip=`ip_to_hex 172 16 1 2`
>+packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${ds
>t_ip}0035111100080000
>+as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
>+
>+# Send ip packets between foo1 and bob1
>+src_mac="f00000010203"
>+dst_mac="000000010203"
>+src_ip=`ip_to_hex 192 168 1 2`
>+dst_ip=`ip_to_hex 172 16 2 2`
>+packet=${dst_mac}${src_mac}08004500001c0000000040110000${src_ip}${ds
>t_ip}0035111100080000
>+as hv1 ovs-appctl netdev-dummy/receive hv1-vif1 $packet
>+
>+echo "---------NB dump-----"
>+ovn-nbctl show
>+echo "---------------------"
>+ovn-nbctl list logical_router
>+echo "---------------------"
>+ovn-nbctl list logical_router_port
>+echo "---------------------"
>+
>+echo "---------SB dump-----"
>+ovn-sbctl list datapath_binding
>+echo "---------------------"
>+ovn-sbctl list port_binding
>+echo "---------------------"
>+
>+echo "------ hv1 dump ----------"
>+as hv1 ovs-ofctl dump-flows br-int
>+echo "------ hv2 dump ----------"
>+as hv2 ovs-ofctl dump-flows br-int
>+
>+# Packet to Expect at bob1
>+src_mac="000000010205"
>+dst_mac="f00000010205"
>+src_ip=`ip_to_hex 192 168 1 2`
>+dst_ip=`ip_to_hex 172 16 2 2`
>+expected=${dst_mac}${src_mac}08004500001c000000003e110200${src_ip}${
>dst_ip}0035111100080000
>+
>+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv2/vif1-tx.pcap |
>trim_zeros > received.packets
>+echo $expected | trim_zeros > expout
>+AT_CHECK([cat received.packets], [0], [expout])
>+
>+# Packet to Expect at alice1
>+src_mac="000000010204"
>+dst_mac="f00000010204"
>+src_ip=`ip_to_hex 192 168 1 2`
>+dst_ip=`ip_to_hex 172 16 1 2`
>+expected=${dst_mac}${src_mac}08004500001c000000003e110200${src_ip}${
>dst_ip}0035111100080000
>+
>+$PYTHON "$top_srcdir/utilities/ovs-pcap.in" hv1/vif2-tx.pcap |
>trim_zeros > received1.packets
>+echo $expected | trim_zeros > expout
>+AT_CHECK([cat received1.packets], [0], [expout])
>+
>+for sim in hv1 hv2; do
>+    as $sim
>+    OVS_APP_EXIT_AND_WAIT([ovn-controller])
>+    OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>+    OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>+done
>+
>+as ovn-sb
>+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>+
>+as ovn-nb
>+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>+
>+as northd
>+OVS_APP_EXIT_AND_WAIT([ovn-northd])
>+
>+as main
>+OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
>+OVS_APP_EXIT_AND_WAIT([ovsdb-server])
>+
>+AT_CLEANUP
>-- 
>1.7.9.5
>
>_______________________________________________
>dev mailing list
>dev at openvswitch.org
>http://openvswitch.org/mailman/listinfo/dev
>




More information about the dev mailing list