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() 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. 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
