Message ID | 20230710084614.37833-1-wangchuanlei@inspur.com |
---|---|
State | Rejected |
Headers | show |
Series | [ovs-dev] northd: Lookup route policy before ip routing | expand |
Context | Check | Description |
---|---|---|
ovsrobot/apply-robot | success | apply and check: success |
ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
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/10035411109 It also makes OVN PBR tests fail: https://github.com/ovsrobot/ovn/actions/runs/5506310686 > > Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> 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; > }
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; }
If there is no route in table ip_routing, the route policy item in table policy is useless. 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. Signed-off-by: wangchuanlei <wangchuanlei@inspur.com> --- northd/northd.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)