Hi, Dumitru, Thank you for reply and the suggestions! Base on your suggestions, i have a thought below. On 7/14/23 11:19, wangchuanlei wrote: > Hi, Dumitru >
> Hi, wangchuanlei, > Thank you for review, i don't have performance measurements yet. > In real physical switch, if flows hit dynimic/static route, or policy > route, the flows will skip other route entries, it's more reasonable on logic. > >I see Cisco and Juniper routers do that, yes. However... > For ovn, in table of ip route, there is no default route entry , but > route policy has, if i add one default route entry on route, for > unknown flows, it will through default route entry, then it won't hit > any route policy, but it will go to next table, so, we will not know where > the flows will go, it may makes the network confused. > > What about add a default route entry on router, and delete the default route > policy on router? > And if flows hit route entry except the default route entry, we set a > register, the if we detect the register is set on policy table, we > skip the reroute or drop action in policy table. In reverse, if we > don't hit any route entry, the it go to policy table, if it don't hit any > policy entry, we should drop it. > > This idea will make the ovn-kubernetes test fail, but it's more reasonable. > What do you think? waiting your reply! > It's more than breaking ovn-kubernetes, it also breaks the original use case > for this feature: > https://github.com/ovn-org/ovn/commit/df4f37ea7f824d410b5682a879d50ced8b0fa71c > It's clear that the original intention of the author was to override the > routing decision (after it was made): > To achieve this, a new table is introduced in the ingress pipeline of the > Logical-router. The new table is between the ???IP Routing??? and the > ???ARP/ND > resolution??? table. This way, PBR can override routing decisions and > provide a > different next-hop. > To change the order of tables breaks the current "contract" OVN currently > provides. > > Anyone else have ideas about this? >I see two options (maybe there are more): > a. we add a new table all together (maybe Logical_Router_Forward_Filter) and > insert rules before the routing stages. This way may makes the flow tables more complex, because there is already have route table and policy table. > b. add a new "Logical_Router_Policy.options:stage" option (or a better > name) and support "pre-routing"/"post-routing". If we default its value to > "post-routing" then we ensure backwards compatibility. This options still needs a default route entry if we want go to the policy table. Based your ideas, what about add the options to Logical_Router, like: Logical_Router:options:stage(or a better name) and support "pre-routing"/"post-routing". Set the default value to "post-routing". Thus, (1) stage:"post-routing" We change nothing, all flows no changed. (2) stage:"pre-routing" We would't change the table order any more. But we add a default route enty which macth is 1, priority is 0, and action is set a register ,then go to next table. In policy table if the register is set, which is hit none route entry, we need lookup policy table, otherwise go to the next table. This way is backwards compatible. How do you think? Thanks, wangchuanlei > > Best regards! > wangchuanlei > >> Hi wangchuanlei, > >> Thanks for the patch! > >> On 7/10/23 10:46, wangchuanlei wrote: >> If there is no route in table ip_routing, the route policy item >> in table policy is useless. > >> I'm sorry but I disagree with this. We can always add a default route that >> matches all traffic that's not matched by any other route. > >> Afterwards, in the policy stage we can re-route/drop accordingly. > >> Because route policy has a higher priority than ip routing, so >> moddify the table id of IP_ROUTING and POLICY is a little better.By >> this way, there is no need reroute any more, this should be more >> effienct. > >> I can see how this can be slightly more efficient in slow path but I'm quite >> confident the difference won't be significant. Do you have performance >> measurements to indicate the opposite? > > >> The real problem with just swapping the two pipelines is that we change >> behavior completely. CMS out there (ovn-kube, neutron, etc.) rely on the >> fact that policies are applied after routing. We can't just decide to >> change this and break compatibility. > >> I'm quite sure the ovn-kubernetes tests also fail because of this change in >> behavior: > >> https://github.com/ovsrobot/ovn/actions/runs/5506310703/jobs/10035411 >> 109 > >> It also makes OVN PBR tests fail: > >> https://github.com/ovsrobot/ovn/actions/runs/5506310686 > >> >> Signed-off-by: wangchuanlei <[email protected]> > >> Regards, >> Dumitru > >> --- >> northd/northd.c | 14 ++++++++++---- >> 1 file changed, 10 insertions(+), 4 deletions(-) >> >> diff --git a/northd/northd.c b/northd/northd.c index >> a45c8b53a..35187abf8 100644 >> --- a/northd/northd.c >> +++ b/northd/northd.c >> @@ -174,10 +174,10 @@ enum ovn_stage { >> PIPELINE_STAGE(ROUTER, IN, ND_RA_OPTIONS, 10, "lr_in_nd_ra_options") >> \ >> PIPELINE_STAGE(ROUTER, IN, ND_RA_RESPONSE, 11, >> "lr_in_nd_ra_response") \ >> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_PRE, 12, >> "lr_in_ip_routing_pre") \ >> - PIPELINE_STAGE(ROUTER, IN, IP_ROUTING, 13, "lr_in_ip_routing") >> \ >> - PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 14, >> "lr_in_ip_routing_ecmp") \ >> - PIPELINE_STAGE(ROUTER, IN, POLICY, 15, "lr_in_policy") >> \ >> - PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 16, "lr_in_policy_ecmp") >> \ >> + PIPELINE_STAGE(ROUTER, IN, POLICY, 13, "lr_in_policy") >> \ >> + PIPELINE_STAGE(ROUTER, IN, POLICY_ECMP, 14, "lr_in_policy_ecmp") >> \ >> + PIPELINE_STAGE(ROUTER, IN, IP_ROUTING, 15, "lr_in_ip_routing") >> \ >> + PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 16, >> + "lr_in_ip_routing_ecmp") \ >> PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 17, "lr_in_arp_resolve") >> \ >> PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN, 18, "lr_in_chk_pkt_len") >> \ >> PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 19, "lr_in_larger_pkts") >> \ >> @@ -278,6 +278,7 @@ enum ovn_stage { >> #define REG_SRC_IPV4 "reg1" >> #define REG_SRC_IPV6 "xxreg1" >> #define REG_ROUTE_TABLE_ID "reg7" >> +#define REG_ROUTE_POLICY "reg2" >> >> /* Register used to store backend ipv6 address >> * for load balancer affinity. */ >> @@ -10240,6 +10241,7 @@ build_routing_policy_flow(struct hmap *lflows, >> struct ovn_datapath *od, >> bool is_ipv4 = strchr(nexthop, '.') ? true : false; >> ds_put_format(&actions, "%s = %s; " >> "%s = %s; " >> + "%s = 1; " >> "eth.src = %s; " >> "outport = %s; " >> "flags.loopback = 1; " >> @@ -10249,6 +10251,7 @@ build_routing_policy_flow(struct hmap *lflows, >> struct ovn_datapath *od, >> nexthop, >> is_ipv4 ? REG_SRC_IPV4 : REG_SRC_IPV6, >> lrp_addr_s, >> + REG_ROUTE_POLICY, >> out_port->lrp_networks.ea_s, >> out_port->json_key); >> >> @@ -10340,6 +10343,7 @@ build_ecmp_routing_policy_flows(struct hmap *lflows, >> struct ovn_datapath *od, >> out_port->json_key); >> >> ds_clear(&match); >> + ds_put_cstr(&actions, REG_ROUTE_POLICY" = 1; "); >> ds_put_format(&match, REG_ECMP_GROUP_ID" == %"PRIu16" && " >> REG_ECMP_MEMBER_ID" == %"PRIuSIZE, >> ecmp_group_id, i + 1); @@ -12911,6 +12915,8 @@ >> build_mcast_lookup_flows_for_lrouter( >> */ >> ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10550, >> "nd_rs || nd_ra", debug_drop_action()); >> + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, 10000, >> + REG_ROUTE_POLICY" == 1", "reg8[0..15] = 0; >> + next;"); >> if (!od->mcast_info.rtr.relay) { >> return; >> } > >
_______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
