On 5/15/20 5:49 PM, Lorenzo Bianconi wrote:
> In order to fix the issues introduced by commit
> c0bf32d72f8b ("Manage ARP process locally in a DVR scenario "), restore
> previous configuration of table 9 in ingress router pipeline and
> introduce a new stage called 'ip_src_policy' used to set the src address
> info in order to not distribute FIP traffic if DVR is enabled
>
> Fixes: c0bf32d72f8b ("Manage ARP process locally in a DVR scenario ")
> Signed-off-by: Lorenzo Bianconi <[email protected]>
Hi Lorenzo,
Thanks for working on fixing this issue!
As discussed offline, I'm a bit concerned about adding a new stage to
the OVN pipeline and I'd prefer if we could find a different way to
address this.
This being said, if there's no other easy way of fixing this, I think
your change looks ok and will handle all the bugs reported, including [0].
Thanks,
Dumitru
[0] https://bugzilla.redhat.com/show_bug.cgi?id=1834433
> ---
> northd/ovn-northd.8.xml | 65 ++++++++++++++++++++---------------------
> northd/ovn-northd.c | 38 ++++++++++--------------
> tests/ovn.at | 28 +++++-------------
> 3 files changed, 54 insertions(+), 77 deletions(-)
>
> diff --git a/northd/ovn-northd.8.xml b/northd/ovn-northd.8.xml
> index 8f224b07f..09dbb52b4 100644
> --- a/northd/ovn-northd.8.xml
> +++ b/northd/ovn-northd.8.xml
> @@ -2484,37 +2484,6 @@ output;
> </p>
> </li>
>
> - <li>
> - <p>
> - For distributed logical routers where one of the logical router
> ports
> - specifies a <code>redirect-chassis</code>, a priority-400 logical
> - flow for each <code>dnat_and_snat</code> NAT rules configured.
> - These flows will allow to properly forward traffic to the external
> - connections if available and avoid sending it through the tunnel.
> - Assuming the following NAT rule has been configured:
> - </p>
> -
> - <pre>
> -external_ip = <var>A</var>;
> -external_mac = <var>B</var>;
> -logical_ip = <var>C</var>;
> - </pre>
> -
> - <p>
> - the following action will be applied:
> - </p>
> -
> - <pre>
> -ip.ttl--;
> -reg0 = <var>ip.dst</var>;
> -reg1 = <var>A</var>;
> -eth.src = <var>B</var>;
> -outport = <var>router-port</var>;
> -next;
> - </pre>
> -
> - </li>
> -
> <li>
> <p>
> IPv4 routing table. For each route to IPv4 network <var>N</var>
> with
> @@ -2660,7 +2629,35 @@ outport = <var>P</var>;
> </li>
> </ul>
>
> - <h3>Ingress Table 12: ARP/ND Resolution</h3>
> + <h3>Ingress Table 12: IP Source Policy</h3>
> +
> + <p>
> + This table contains for distributed logical routers where one of
> + the logical router ports specifies a <code>redirect-chassis</code>,
> + a priority-100 logical flow for each <code>dnat_and_snat</code>
> + NAT rules configured.
> + These flows will allow to properly forward traffic to the external
> + connections if available and avoid sending it through the tunnel.
> + Assuming the following NAT rule has been configured:
> + </p>
> +
> + <pre>
> +external_ip = <var>A</var>;
> +external_mac = <var>B</var>;
> +logical_ip = <var>C</var>;
> + </pre>
> +
> + <p>
> + the following action will be applied:
> + </p>
> +
> + <pre>
> +reg1 = <var>A</var>;
> +eth.src = <var>B</var>;
> +next;
> + </pre>
> +
> + <h3>Ingress Table 13: ARP/ND Resolution</h3>
>
> <p>
> Any packet that reaches this table is an IP packet whose next-hop
> @@ -2819,7 +2816,7 @@ outport = <var>P</var>;
>
> </ul>
>
> - <h3>Ingress Table 13: Check packet length</h3>
> + <h3>Ingress Table 14: Check packet length</h3>
>
> <p>
> For distributed logical routers with distributed gateway port
> configured
> @@ -2849,7 +2846,7 @@ REGBIT_PKT_LARGER = check_pkt_larger(<var>L</var>);
> next;
> and advances to the next table.
> </p>
>
> - <h3>Ingress Table 14: Handle larger packets</h3>
> + <h3>Ingress Table 15: Handle larger packets</h3>
>
> <p>
> For distributed logical routers with distributed gateway port
> configured
> diff --git a/northd/ovn-northd.c b/northd/ovn-northd.c
> index 3c0070ea7..d5f3997a9 100644
> --- a/northd/ovn-northd.c
> +++ b/northd/ovn-northd.c
> @@ -175,11 +175,12 @@ enum ovn_stage {
> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING, 9, "lr_in_ip_routing") \
> PIPELINE_STAGE(ROUTER, IN, IP_ROUTING_ECMP, 10,
> "lr_in_ip_routing_ecmp") \
> PIPELINE_STAGE(ROUTER, IN, POLICY, 11, "lr_in_policy") \
> - PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 12, "lr_in_arp_resolve") \
> - PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN , 13, "lr_in_chk_pkt_len") \
> - PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 14,"lr_in_larger_pkts") \
> - PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 15, "lr_in_gw_redirect") \
> - PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 16, "lr_in_arp_request") \
> + PIPELINE_STAGE(ROUTER, IN, IP_SRC_POLICY, 12, "lr_in_ip_src_policy") \
> + PIPELINE_STAGE(ROUTER, IN, ARP_RESOLVE, 13, "lr_in_arp_resolve") \
> + PIPELINE_STAGE(ROUTER, IN, CHK_PKT_LEN , 14, "lr_in_chk_pkt_len") \
> + PIPELINE_STAGE(ROUTER, IN, LARGER_PKTS, 15,"lr_in_larger_pkts") \
> + PIPELINE_STAGE(ROUTER, IN, GW_REDIRECT, 16, "lr_in_gw_redirect") \
> + PIPELINE_STAGE(ROUTER, IN, ARP_REQUEST, 17, "lr_in_arp_request") \
> \
> /* Logical router egress stages. */ \
> PIPELINE_STAGE(ROUTER, OUT, UNDNAT, 0, "lr_out_undnat") \
> @@ -7103,8 +7104,6 @@ build_routing_policy_flow(struct hmap *lflows, struct
> ovn_datapath *od,
> ds_destroy(&actions);
> }
>
> -/* default logical flow prioriry for distributed routes */
> -#define DROUTE_PRIO 400
> struct parsed_route {
> struct ovs_list list_node;
> struct v46_ip prefix;
> @@ -7493,7 +7492,7 @@ build_ecmp_route_flow(struct hmap *lflows, struct
> ovn_datapath *od,
> }
>
> static void
> -add_distributed_routes(struct hmap *lflows, struct ovn_datapath *od)
> +add_ip_src_policy_flows(struct hmap *lflows, struct ovn_datapath *od)
> {
> struct ds actions = DS_EMPTY_INITIALIZER;
> struct ds match = DS_EMPTY_INITIALIZER;
> @@ -7511,12 +7510,9 @@ add_distributed_routes(struct hmap *lflows, struct
> ovn_datapath *od)
> is_ipv4 ? "4" : "6", nat->logical_ip,
> nat->logical_port);
> char *prefix = is_ipv4 ? "" : "xx";
> - ds_put_format(&actions, "outport = %s; eth.src = %s; "
> - "%sreg0 = ip%s.dst; %sreg1 = %s; next;",
> - od->l3dgw_port->json_key, nat->external_mac,
> - prefix, is_ipv4 ? "4" : "6",
> - prefix, nat->external_ip);
> - ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_ROUTING, DROUTE_PRIO,
> + ds_put_format(&actions, "eth.src = %s; %sreg1 = %s; next;",
> + nat->external_mac, prefix, nat->external_ip);
> + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_SRC_POLICY, 100,
> ds_cstr(&match), ds_cstr(&actions));
> ds_clear(&match);
> ds_clear(&actions);
> @@ -7547,12 +7543,6 @@ add_route(struct hmap *lflows, const struct ovn_port
> *op,
> }
> build_route_match(op_inport, network_s, plen, is_src_route, is_ipv4,
> &match, &priority);
> - /* traffic for internal IPs of logical switch ports must be sent to
> - * the gw controller through the overlay tunnels
> - */
> - if (op->nbrp && !op->nbrp->n_gateway_chassis) {
> - priority += DROUTE_PRIO;
> - }
>
> struct ds actions = DS_EMPTY_INITIALIZER;
> ds_put_format(&actions, "ip.ttl--; "REG_ECMP_GROUP_ID" = 0; %sreg0 = ",
> @@ -9519,9 +9509,13 @@ build_lrouter_flows(struct hmap *datapaths, struct
> hmap *ports,
> * logical router
> */
> HMAP_FOR_EACH (od, key_node, datapaths) {
> - if (od->nbr && od->l3dgw_port) {
> - add_distributed_routes(lflows, od);
> + if (!od->nbr) {
> + continue;
> + }
> + if (od->l3dgw_port) {
> + add_ip_src_policy_flows(lflows, od);
> }
> + ovn_lflow_add(lflows, od, S_ROUTER_IN_IP_SRC_POLICY, 0, "1",
> "next;");
> }
>
> /* Logical router ingress table IP_ROUTING & IP_ROUTING_ECMP: IP Routing.
> diff --git a/tests/ovn.at b/tests/ovn.at
> index f39fda2e4..fcc34fd5d 100644
> --- a/tests/ovn.at
> +++ b/tests/ovn.at
> @@ -9637,20 +9637,6 @@ AT_CHECK([as hv3 ovs-vsctl set Open_vSwitch .
> external-ids:ovn-bridge-mappings=p
> OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-vsctl show | \
> grep "Port patch-br-int-to-ln_port" | wc -l`])
>
> -AT_CHECK([test 1 = `ovn-sbctl dump-flows lr0 | grep lr_in_ip_routing | \
> -grep "ip4.src == 10.0.0.3 && is_chassis_resident(\"foo1\")" -c`])
> -AT_CHECK([test 1 = `ovn-sbctl dump-flows lr0 | grep lr_in_ip_routing | \
> -grep "ip4.src == 10.0.0.4 && is_chassis_resident(\"foo2\")" -c`])
> -
> -key=`ovn-sbctl --bare --columns tunnel_key list datapath_Binding lr0`
> -# Check that the OVS flows appear for the dnat_and_snat entries in
> -# lr_in_ip_routing table.
> -OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-ofctl dump-flows br-int table=17 | \
> -grep "priority=400,ip,metadata=0x$key,nw_src=10.0.0.3" -c`])
> -
> -OVS_WAIT_UNTIL([test 1 = `as hv3 ovs-ofctl dump-flows br-int table=17 | \
> -grep "priority=400,ip,metadata=0x$key,nw_src=10.0.0.4" -c`])
> -
> # Re-add nat-addresses option
> ovn-nbctl lsp-set-options lrp0-rp router-port=lrp0 nat-addresses="router"
>
> @@ -15141,7 +15127,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> # Since the sw0-vir is not claimed by any chassis, eth.dst should be set to
> # zero if the ip4.dst is the virtual ip in the router pipeline.
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00; next;)
> ])
>
> ip_to_hex() {
> @@ -15192,7 +15178,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> # There should be an arp resolve flow to resolve the virtual_ip with the
> # sw0-p1's MAC.
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
> ])
>
> # Forcibly clear virtual_parent. ovn-controller should release the binding
> @@ -15233,7 +15219,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> # There should be an arp resolve flow to resolve the virtual_ip with the
> # sw0-p2's MAC.
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:05; next;)
> ])
>
> # send the garp from sw0-p2 (in hv2). hv2 should claim sw0-vir
> @@ -15256,7 +15242,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> # There should be an arp resolve flow to resolve the virtual_ip with the
> # sw0-p3's MAC.
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
> ])
>
> # Now send arp reply from sw0-p1. hv1 should claim sw0-vir
> @@ -15277,7 +15263,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> > lflows.txt
>
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:03; next;)
> ])
>
> # Delete hv1-vif1 port. hv1 should release sw0-vir
> @@ -15295,7 +15281,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> > lflows.txt
>
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 00:00:00:00:00:00; next;)
> ])
>
> # Now send arp reply from sw0-p2. hv2 should claim sw0-vir
> @@ -15316,7 +15302,7 @@ ovn-sbctl dump-flows lr0 | grep lr_in_arp_resolve |
> grep "reg0 == 10.0.0.10" \
> > lflows.txt
>
> AT_CHECK([cat lflows.txt], [0], [dnl
> - table=12(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
> + table=13(lr_in_arp_resolve ), priority=100 , match=(outport == "lr0-sw0"
> && reg0 == 10.0.0.10), action=(eth.dst = 50:54:00:00:00:04; next;)
> ])
>
> # Delete sw0-p2 logical port
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
