On Thu, Mar 25, 2021 at 11:40:59PM +0000, svc.eng.git-m...@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. _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev