On 7/19/23 02:57, wangchuanlei wrote: > Hi, Dumitru, > > Thank you for reply and the suggestions! > Base on your suggestions, i have a thought below.
Sorry for the late reply. > 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. > I still think I prefer this one because it's more explicit. >> 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? > I'm not sure I understand what happens if you don't hit a policy, wasn't it your use case that we should fall back to default destination route lookup then? Regards, Dumitru > 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
