On Tue, Aug 3, 2021 at 6:58 PM Han Zhou <[email protected]> wrote: > > On Tue, Aug 3, 2021 at 2:49 PM Han Zhou <[email protected]> wrote: > > > > > > > > On Tue, Aug 3, 2021 at 1:42 PM Numan Siddique <[email protected]> wrote: > > > > > > On Tue, Aug 3, 2021 at 4:08 PM Numan Siddique <[email protected]> wrote: > > > > > > > > On Tue, Aug 3, 2021 at 3:29 PM Han Zhou <[email protected]> wrote: > > > > > > > > > > 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. > > > > > > > > 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?
I think REGBIT_EGRESS_LOOPBACK check is required so that the injected icmp4 packet generated from ovn-controller is skipped in this stage. Otherwise there will be recursion of the packet. @Lorenzo Bianconi Please correct me if I'm wrong. > > > > (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) I'm fine with it if you think it will not break the existing functionality. @Lorenzo Bianconi If you've any comments here please let us know. Thanks Numan > > > > 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 > > > > > [email protected] > > > > > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > > > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
