On Tue, Jul 27, 2021 at 10:06 AM Lorenzo Bianconi <
[email protected]> wrote:
>
> Introduce check_pkt_larger action for ingress traffic
> entering the cluster from a distributed gw router port
> or from a gw router. This patch enables pMTU discovery
> for ingress traffic.
>
> Signed-off-by: Lorenzo Bianconi <[email protected]>
Thanks for the patch. I hit a problem while rebasing on top of this patch.
It is committed as:
With the current master (already has this patch committed), the DDlog test
case fails (100% failure when I run it individually, but sometimes passes
when I run in parallel with other cases):
router - check packet length - icmp defrag -- ovn-northd-ddlog --
dp-groups=yes
Reverting the commit (1c9e46ab5 northd: add check_pkt_larger lflows for
ingress traffic) makes test pass. I haven't debugged why this happened.
And I also have a question on the patch. Please see my comment below.
> diff --git a/northd/ovn_northd.dl b/northd/ovn_northd.dl
> index d1f7dbbed..838f72824 100644
> --- a/northd/ovn_northd.dl
> +++ b/northd/ovn_northd.dl
> @@ -4728,7 +4728,12 @@ for (&RouterPort(.lrp = lrp,
> * This will save us from having to match on inport further down in
> * the pipeline.
> */
> - var actions = "${rEG_INPORT_ETH_ADDR()} = ${lrp_networks.ea}; next;"
in {
> + var gw_mtu = lrp.options.get_int_def("gateway_mtu", 0) in
> + var mtu = gw_mtu + vLAN_ETH_HEADER_LEN() in
> + var actions = "${rEG_INPORT_ETH_ADDR()} = ${lrp_networks.ea}; " ++
> + if (gw_mtu > 0) {
> + "${rEGBIT_PKT_LARGER()} = check_pkt_larger(${mtu}); next;"
> + } else { "next;" } in {
> Flow(.logical_datapath = router._uuid,
> .stage = s_ROUTER_IN_ADMISSION(),
> .priority = 50,
> @@ -7558,11 +7563,12 @@ Flow(.logical_datapath = lr_uuid,
> var mtu = gw_mtu + vLAN_ETH_HEADER_LEN().
> MeteredFlow(.logical_datapath = lr_uuid,
> .stage = s_ROUTER_IN_LARGER_PKTS(),
> - .priority = 50,
> - .__match = "inport == ${rp.json_name} && outport ==
${l3dgw_port_json_name} && "
> - "ip4 && ${rEGBIT_PKT_LARGER()}",
> + .priority = 150,
> + .__match = "inport == ${rp.json_name} && ip4 && "
> + "${rEGBIT_PKT_LARGER()} &&
${rEGBIT_EGRESS_LOOPBACK()} == 0",
> .actions = "icmp4_error {"
> "${rEGBIT_EGRESS_LOOPBACK()} = 1; "
> + "${rEGBIT_PKT_LARGER()} = 0; "
> "eth.dst = ${rp.networks.ea}; "
> "ip4.dst = ip4.src; "
> "ip4.src = ${first_ipv4.addr}; "
> @@ -7585,13 +7591,46 @@ MeteredFlow(.logical_datapath = lr_uuid,
> rp in &RouterPort(.router = r),
> rp.lrp != l3dgw_port,
> Some{var first_ipv4} = rp.networks.ipv4_addrs.nth(0).
> +
> +MeteredFlow(.logical_datapath = lr_uuid,
> + .stage = s_ROUTER_IN_IP_INPUT(),
> + .priority = 150,
> + .__match = "inport == ${rp.json_name} && ip4 && "
> + "${rEGBIT_PKT_LARGER()} &&
${rEGBIT_EGRESS_LOOPBACK()} == 0",
> + .actions = "icmp4_error {"
> + "${rEGBIT_EGRESS_LOOPBACK()} = 1; "
> + "${rEGBIT_PKT_LARGER()} = 0; "
> + "eth.dst = ${rp.networks.ea}; "
> + "ip4.dst = ip4.src; "
> + "ip4.src = ${first_ipv4.addr}; "
> + "ip.ttl = 255; "
> + "icmp4.type = 3; /* Destination
Unreachable. */ "
> + "icmp4.code = 4; /* Frag Needed and DF
was Set. */ "
> + /* Set icmp4.frag_mtu to gw_mtu */
> + "icmp4.frag_mtu = ${gw_mtu}; "
> + "next(pipeline=ingress, table=0); "
> + "};",
> + .tags = map_empty(),
> + .controller_meter = r.copp.get(cOPP_ICMP4_ERR()),
> + .external_ids = stage_hint(rp.lrp._uuid)) :-
> + r in &Router(._uuid = lr_uuid),
> + Some{var l3dgw_port} = r.l3dgw_port,
> + var l3dgw_port_json_name = json_string_escape(l3dgw_port.name),
> + r.redirect_port_name != "",
> + var gw_mtu = l3dgw_port.options.get_int_def("gateway_mtu", 0),
> + gw_mtu > 0,
> + rp in &RouterPort(.router = r),
> + rp.lrp == l3dgw_port,
> + Some{var first_ipv4} = rp.networks.ipv4_addrs.nth(0).
> +
This flow is exactly the same as the one immediately above it, except that
"rp.lrp == l3dgw_port" instead of "rp.lrp != l3dgw_port". Does it mean
adding the flow for all LRPs, whether it is l3dgw_port or not? If so, why
not combine the code just by removing the "rp.lrp == l3dgw_port" or "rp.lrp
!= l3dgw_port" check? Did I misunderstand something? Same for the ipv6 flow
below.
Thanks,
Han
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
