On Tue, Jun 4, 2024 at 2:55 PM Xavier Simonart <[email protected]> wrote:
> Hi Ales > > On Tue, Jun 4, 2024 at 2:36 PM Ales Musil <[email protected]> wrote: > >> >> >> On Tue, Jun 4, 2024 at 2:19 PM Xavier Simonart <[email protected]> >> wrote: >> >>> Hi Ales >>> >>> Thanks for the patch. >>> >> >> Hi Xavier, >> >> thank you for the review. >> >> >>> On Fri, May 31, 2024 at 2:52 PM Ales Musil <[email protected]> wrote: >>> >>>> Add missing sync calls to make sure that the flows are present and >>>> strip the statistics from the flows. >>>> >>>> Fixes: 3faadc76ad71 ("northd: Fix pmtud for non routed traffic.") >>>> Signed-off-by: Ales Musil <[email protected]> >>>> --- >>>> tests/ovn-controller.at | 42 +++++++++++++++++++++++------------------ >>>> 1 file changed, 24 insertions(+), 18 deletions(-) >>>> >>>> diff --git a/tests/ovn-controller.at b/tests/ovn-controller.at >>>> index fc50b6ff8..3d3825cb8 100644 >>>> --- a/tests/ovn-controller.at >>>> +++ b/tests/ovn-controller.at >>>> @@ -3036,13 +3036,16 @@ check ovs-vsctl \ >>>> -- add-port br-int vif2 \ >>>> -- set Interface vif2 external_ids:iface-id=lsp2 >>>> >>>> +wait_for_ports_up >>>> +check ovn-nbctl --wait=hv sync >>>> + >>>> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int >>>> table=OFTABLE_CT_ZONE_LOOKUP | \ >>>> - sed -e 's/cookie=0x.*, duration=.*, table/cookie=??, >>>> duration=??, table/' | \ >>>> sed -e >>>> 's/actions=load:0x.*->NXM_NX_REG13/actions=load:0x?->NXM_NX_REG13/' | \ >>>> - grep -v NXST_FLOW |sort], [0], [dnl >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=0 >>>> actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> + grep -v NXST_FLOW | \ >>>> + awk '{print $7, $8}' | sort], [0], [dnl >>>> +priority=0 actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> ]) >>>> >>>> In addition to your change, I have seen a different kind of failure for >>> this test: on some systems, the test always fails because ct_zones id are >>> different, and hence reg11 and reg12 are filled with different values. >>> I think that this happens depending on whether the system uses murmur or >>> crc for hashing. >>> Hence, I think we should also sed/replace the values loaded in reg11 and >>> reg12 as we do for reg13. >>> Same comment applies to the similar checks below. >>> I have such a system where tests are failing. So, if it's easier for >>> you, let me know if you'd like me to apply a further patch, or test your >>> changes, or provide access to the system. >>> >> >> It makes sense, I agree that we should replace all loads and the reg >> values, which makes me wonder if it isn't actually better to just count the >> flows instead WDYT? >> > In fact, I was wondering about almost the opposite (:-)) - we could query > the ct-zone-list to find out the zone_id and check whether the flows are > exactly what we expect. But then I found that to be really overkill. > So, I think that checking the flows (w/ zone-id hidden) seemed a correct > trade-off. Also, it makes the test easier to understand (we know what we > expect), and make debugging easier if the test fails. > WDYT? > I'll do that in v2. Thanks > >> >>> >>> check ovn-nbctl lsp-add ls1 lsp3 \ >>>> @@ -3051,24 +3054,27 @@ check ovs-vsctl \ >>>> -- add-port br-int vif3 \ >>>> -- set Interface vif3 external_ids:iface-id=lsp3 >>>> >>>> +wait_for_ports_up >>>> +check ovn-nbctl --wait=hv sync >>>> + >>>> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int >>>> table=OFTABLE_CT_ZONE_LOOKUP | \ >>>> - sed -e 's/cookie=0x.*, duration=.*, table/cookie=??, >>>> duration=??, table/' | \ >>>> sed -e >>>> 's/actions=load:0x.*->NXM_NX_REG13/actions=load:0x?->NXM_NX_REG13/' | \ >>>> - grep -v NXST_FLOW |sort], [0], [dnl >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=0 >>>> actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x3,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> + grep -v NXST_FLOW | \ >>>> + awk '{print $7, $8}' | sort], [0], [dnl >>>> +priority=0 actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x3,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> ]) >>>> >>>> -check ovn-nbctl lsp-del lsp3 >>>> +check ovn-nbctl --wait=hv lsp-del lsp3 >>>> AT_CHECK([as hv1 ovs-ofctl dump-flows br-int >>>> table=OFTABLE_CT_ZONE_LOOKUP | \ >>>> - sed -e 's/cookie=0x.*, duration=.*, table/cookie=??, >>>> duration=??, table/' | \ >>>> - sed -e >>>> 's/actions=load:0x.*->NXM_NX_REG13/actions=load:0x?->NXM_NX_REG13/' | >>>> - grep -v NXST_FLOW |sort], [0], [dnl >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=0 >>>> actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> - cookie=??, duration=??, table=OFTABLE_CT_ZONE_LOOKUP, n_packets=0, >>>> n_bytes=0, idle_age=0, priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> + sed -e >>>> 's/actions=load:0x.*->NXM_NX_REG13/actions=load:0x?->NXM_NX_REG13/' | \ >>>> + grep -v NXST_FLOW | \ >>>> + awk '{print $7, $8}' | sort], [0], [dnl >>>> +priority=0 actions=resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x1,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> +priority=100,reg14=0x2,metadata=0x1 >>>> actions=load:0x?->NXM_NX_REG13[[0..15]],load:0x2->NXM_NX_REG11[[]],load:0x1->NXM_NX_REG12[[]],resubmit(,OFTABLE_LOG_INGRESS_PIPELINE) >>>> ]) >>>> >>>> OVN_CLEANUP([hv1]) >>>> -- >>>> 2.45.0 >>>> >>>> Besides the comment above, it looks good to me. >>> Thanks >>> Xavier >>> >>>> _______________________________________________ >>>> dev mailing list >>>> [email protected] >>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev >>>> >>>> >> Thanks, >> Ales >> > Thanks > Xavier > >> -- >> >> Ales Musil >> >> Senior Software Engineer - OVN Core >> >> Red Hat EMEA <https://www.redhat.com> >> >> [email protected] >> <https://red.ht/sig> >> > -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
