[ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
svc.eng.git-mail
svc.eng.git-mail at nutanix.com
Fri Apr 2 21:15:35 UTC 2021
Hi Ben,
Thank you for the detailed explanation. That was helpful. I have made the changes on v3.
Thank You,
Karthik C
From: Ben Pfaff <blp at ovn.org>
Date: Thursday, March 25, 2021 at 8:05 PM
To: svc.eng.git-mail <svc.eng.git-mail at nutanix.com>
Cc: ovs-dev at openvswitch.org <ovs-dev at openvswitch.org>, Karthik Chandrashekar <karthik.c at nutanix.com>
Subject: Re: [ovs-dev] [PATCH ovn v2] Static Routes: Add ability to specify "discard" nexthop
On Thu, Mar 25, 2021 at 11:40:59PM +0000, svc.eng.git-mail at nutanix.com wrote:
> diff --git a/northd/lrouter.dl b/northd/lrouter.dl
> index 1a7cb2d23..1d8722282 100644
> --- a/northd/lrouter.dl
> +++ b/northd/lrouter.dl
> @@ -770,6 +770,36 @@ RouterStaticRoute(router, key, dsts) :-
> },
> var dsts = set_singleton(RouteDst{nexthop, src_ip, port, ecmp_symmetric_reply}).
>
> +/* compute route-route pairs for nexthop = "discard" routes */
> +function is_discard_route(nexthop: string): bool {
> + match (nexthop) {
> + "discard" -> true,
> + _ -> false
> + }
> +}
> +relation &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
> + key: route_key,
> + nexthop: string)
> +&DiscardRoute(.lrsr = lrsr,
> + .key = RouteKey{policy, ip_prefix, plen},
> + .nexthop = lrsr.nexthop) :-
> + lrsr in nb::Logical_Router_Static_Route(),
> + var policy = route_policy_from_string(lrsr.policy),
> + Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
> + is_discard_route(lrsr.nexthop).
> +
> +relation RouterDiscardRoute_(
> + router : Ref<Router>,
> + key : route_key,
> + nexthop : string)
> +
> +RouterDiscardRoute_(.router = router,
> + .key = route.key,
> + .nexthop = route.nexthop) :-
> + router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
> + var route_id = FlatMap(routes),
> + route in &DiscardRoute(.lrsr = nb::Logical_Router_Static_Route{._uuid = route_id}).
Thanks so much for writing the ddlog support for this! I have a few
comments.
I first noticed that this is more complicated than necessary:
function is_discard_route(nexthop: string): bool {
match (nexthop) {
"discard" -> true,
_ -> false
}
}
It can be written as simply:
function is_discard_route(nexthop: string): bool {
nexthop == "discard"
}
Then, I noticed that is_discard_route() is only used in one place, so
that the last line in this:
&DiscardRoute(.lrsr = lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
is_discard_route(lrsr.nexthop).
can be written as:
lrsr.nexthop == "discard".
and be a little clearer.
However, there's an optimization possibility here. ddlog is pretty
literal-minded and does things in order. Most routes won't be
"discard", so by putting that test first we can usually bypass the other
work. Now we have:
&DiscardRoute(.lrsr = lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(),
lrsr.nexthop == "discard".
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
We can make things even better by noticing that we can put the "discard"
test right into the specification of Logical_Router_Static_Route. That
makes things even faster since ddlog can index it:
&DiscardRoute(.lrsr = lrsr,
.key = RouteKey{policy, ip_prefix, plen},
.nexthop = lrsr.nexthop) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop == "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
At this point I noticed that the nexthop column wasn't good for anything
in DiscardRoute or RouterDiscardRoute_, because it's always the constant
string "discard". So we can get rid of it:
/* compute route-route pairs for nexthop = "discard" routes */
relation &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
key: route_key)
&DiscardRoute(.lrsr = lrsr,
.key = RouteKey{policy, ip_prefix, plen}) :-
lrsr in nb::Logical_Router_Static_Route(.nexthop = "discard"),
var policy = route_policy_from_string(lrsr.policy),
Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
relation RouterDiscardRoute_(
router : Ref<Router>,
key : route_key)
RouterDiscardRoute_(.router = router,
.key = route.key) :-
router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
var route_id = FlatMap(routes),
route in &DiscardRoute(.lrsr = nb::Logical_Router_Static_Route{._uuid = route_id}).
Warning[message] :-
RouterStaticRoute_(.router = router, .key = key, .nexthop = nexthop),
not RouterStaticRoute(.router = router, .key = key),
var message = "No path for ${key.policy} static route ${key.ip_prefix}/${key.plen} with next hop ${nexthop}".
The overall incremental change is the following:
diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1d87222820eb..8c12b2303bda 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -771,31 +771,20 @@ RouterStaticRoute(router, key, dsts) :-
var dsts = set_singleton(RouteDst{nexthop, src_ip, port, ecmp_symmetric_reply}).
/* compute route-route pairs for nexthop = "discard" routes */
-function is_discard_route(nexthop: string): bool {
- match (nexthop) {
- "discard" -> true,
- _ -> false
- }
-}
relation &DiscardRoute(lrsr: nb::Logical_Router_Static_Route,
- key: route_key,
- nexthop: string)
+ key: route_key)
&DiscardRoute(.lrsr = lrsr,
- .key = RouteKey{policy, ip_prefix, plen},
- .nexthop = lrsr.nexthop) :-
- lrsr in nb::Logical_Router_Static_Route(),
+ .key = RouteKey{policy, ip_prefix, plen}) :-
+ lrsr in nb::Logical_Router_Static_Route(.nexthop = "discard"),
var policy = route_policy_from_string(lrsr.policy),
- Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix),
- is_discard_route(lrsr.nexthop).
+ Some{(var ip_prefix, var plen)} = ip46_parse_cidr(lrsr.ip_prefix).
relation RouterDiscardRoute_(
router : Ref<Router>,
- key : route_key,
- nexthop : string)
+ key : route_key)
RouterDiscardRoute_(.router = router,
- .key = route.key,
- .nexthop = route.nexthop) :-
+ .key = route.key) :-
router in &Router(.lr = nb::Logical_Router{.static_routes = routes}),
var route_id = FlatMap(routes),
route in &DiscardRoute(.lrsr = nb::Logical_Router_Static_Route{._uuid = route_id}).
diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
index 1d25c1e5c58e..76067cd4201d 100644
--- a/northd/ovn_northd.dl
+++ b/northd/ovn_northd.dl
@@ -6457,7 +6457,7 @@ Flow(.logical_datapath = router.lr._uuid,
.__match = ip_match,
.actions = "drop;",
.external_ids = map_empty()) :-
- r in RouterDiscardRoute_(.router = router, .key = key, .nexthop = nexthop),
+ r in RouterDiscardRoute_(.router = router, .key = key),
(var ip_match, var priority) = build_route_match(r.key).
/* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.
Since discard routes are a special case, I also spent some time fussing
around trying to figure out whether it was better to alternatively come
up with a new type for next hops, something like this:
diff --git a/northd/lrouter.dl b/northd/lrouter.dl
index 1d87222820eb..6963a2346bcc 100644
--- a/northd/lrouter.dl
+++ b/northd/lrouter.dl
@@ -628,9 +628,11 @@ StaticRouteDown(lrsr_uuid) :-
_ -> false
}.
+typedef next_hop = NextHop{ip: v46_ip} | Discard
+
relation &StaticRoute(lrsr: nb::Logical_Router_Static_Route,
key: route_key,
- nexthop: v46_ip,
+ nexthop: next_hop,
output_port: Option<string>,
ecmp_symmetric_reply: bool)
But this introduced other special cases, and I concluded that it wasn't
necessarily better in the end.
More information about the dev
mailing list