On Tue, Jul 27, 2021 at 10:06 AM Lorenzo Bianconi < lorenzo.bianc...@redhat.com> 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 <lorenzo.bianc...@redhat.com>
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 d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev