On Tue, Aug 3, 2021 at 2:49 PM Han Zhou <hz...@ovn.org> wrote: > > > > On Tue, Aug 3, 2021 at 1:42 PM Numan Siddique <num...@ovn.org> wrote: > > > > On Tue, Aug 3, 2021 at 4:08 PM Numan Siddique <num...@ovn.org> wrote: > > > > > > On Tue, Aug 3, 2021 at 3:29 PM Han Zhou <hz...@ovn.org> wrote: > > > > > > > > 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. > > > > > > I added the ddlog part of the code. Let me check and come back. > > > > I didn't add the code for patch 3. But I think it is not exactly the same. > > The pipeline stage is different right ? > > > > For the condition - rp.lrp != l3dgw_port, the flow is added in the > > stage - s_ROUTER_IN_LARGER_PKTS() > > and for rp.lrp == l3dgw_port, it is added in the stage - s_ROUTER_IN_IP_INPUT() > > > Thanks Numan. Sorry that I missed that difference in stage. > Now another question is, regarding the change: > > 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", > > Before the change, it matches the outport, so that only for the traffic sent to DGP from other (regular) LRPs are matched, but this patch changed it to apply the rule even for packets between regular LRPs. Is this intentional? Do we need MTU checking for packets between distributed LRPs? And why adding the check for the LOOPBACK bit? > > (It would be good if this is intentional because it would make my next rebasing easier, but I just want to make sure I understand it correctly) >
I checked further on this change, and it seems the outport is not needed for this flow because in the previous stage ( s_ROUTER_IN_CHK_PKT_LEN) it is already matched. (although I am still not sure why adding the LOOPBACK check) However, for the ingress part, check_pkt_larger() is added to the stage s_ROUTER_IN_ADMISSION regardless of whether it is a DGP, but based on whether the gw_mtu option exists. I think that is reasonable. It didn't need to distinguish DGP and ports on gateway routers. So I wonder if we could do the same for the egress, i.e. combining the logic of DGP and ports on gateway routers. This would simply the code and also the flows. I can make that change if it sounds good. (It can also simplify my rebasing for the multiple DGP support) > > Regarding the test case, it seems to me it's flaky. When I run the > > whole test suite with -j5, almost 100% of the > > time the test fails, but it passes when run individually for me. > > > Thanks for confirming. This is interesting, completely reverse behavior on my machine :) > > > The test case added in this patch series, does try to make sure that > > both northd-c and northd-ddlog generate > > the exact same flows. > > > > Thanks > > Numan > > > > > > > > Thanks > > > Numan > > > > > > > > > > > Thanks, > > > > Han > > > > _______________________________________________ > > > > dev mailing list > > > > d...@openvswitch.org > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev