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
