On 11 April 2016 at 16:52, Mickey Spiegel <emspi...@us.ibm.com> wrote:
> -----Gurucharan Shetty <g...@ovn.org> wrote: -----
>
> >To: dev@openvswitch.org
> >From: Gurucharan Shetty
> >Sent by: "dev"
> >Date: 04/11/2016 07:46AM
> >Cc: Gurucharan Shetty <g...@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 <g...@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:
>
I will wait for Steve's patch with your changes added and unit tests.
>
> 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@openvswitch.org
> >http://openvswitch.org/mailman/listinfo/dev
> >
>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
