diff mbox series

[v1,01/11] selftests: forwarding: custom_multipath_hash.sh: add cleanup for SIGTERM sent by timeout

Message ID 20230722003609.380549-1-mirsad.todorovac@alu.unizg.hr (mailing list archive)
State New
Headers show
Series [v1,01/11] selftests: forwarding: custom_multipath_hash.sh: add cleanup for SIGTERM sent by timeout | expand

Commit Message

Mirsad Todorovac July 22, 2023, 12:36 a.m. UTC
Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
keyboard, for the test times out and leaves incoherent network stack.

Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
Cc: Ido Schimmel <idosch@nvidia.com>
Cc: netdev@vger.kernel.org
---
 tools/testing/selftests/net/forwarding/custom_multipath_hash.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Ido Schimmel July 23, 2023, 8:25 a.m. UTC | #1
On Sat, Jul 22, 2023 at 02:36:00AM +0200, Mirsad Todorovac wrote:
> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
> keyboard, for the test times out and leaves incoherent network stack.
> 
> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
> Cc: Ido Schimmel <idosch@nvidia.com>
> Cc: netdev@vger.kernel.org
> ---

The patches are missing your Signed-off-by and a cover letter. Anyway,
please don't send a new version just yet. I'm not sure this is the
correct approach and I'm looking into it.

Thanks
Ido Schimmel July 23, 2023, 5:27 p.m. UTC | #2
On Sun, Jul 23, 2023 at 11:25:16AM +0300, Ido Schimmel wrote:
> On Sat, Jul 22, 2023 at 02:36:00AM +0200, Mirsad Todorovac wrote:
> > Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
> > keyboard, for the test times out and leaves incoherent network stack.
> > 
> > Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
> > Cc: Ido Schimmel <idosch@nvidia.com>
> > Cc: netdev@vger.kernel.org
> > ---
> 
> The patches are missing your Signed-off-by and a cover letter. Anyway,
> please don't send a new version just yet. I'm not sure this is the
> correct approach and I'm looking into it.

Please test with the following four patches on top of net.git:

https://github.com/idosch/linux/commits/submit/sefltests_fix_v1
Mirsad Todorovac July 23, 2023, 6:50 p.m. UTC | #3
On 7/23/23 19:27, Ido Schimmel wrote:
> On Sun, Jul 23, 2023 at 11:25:16AM +0300, Ido Schimmel wrote:
>> On Sat, Jul 22, 2023 at 02:36:00AM +0200, Mirsad Todorovac wrote:
>>> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
>>> keyboard, for the test times out and leaves incoherent network stack.
>>>
>>> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
>>> Cc: Ido Schimmel <idosch@nvidia.com>
>>> Cc: netdev@vger.kernel.org
>>> ---
>>
>> The patches are missing your Signed-off-by and a cover letter. Anyway,
>> please don't send a new version just yet. I'm not sure this is the
>> correct approach and I'm looking into it.
> 
> Please test with the following four patches on top of net.git:
> 
> https://github.com/idosch/linux/commits/submit/sefltests_fix_v1

Will do. Just applying.

However, I have upgraded to iproute2-next, maybe the error message should
give a hint on that, too ... That might relieve the pressure on developers
answering always the same questions ...

Kind regards,
Mirsad
Mirsad Todorovac July 23, 2023, 6:54 p.m. UTC | #4
On 7/23/23 10:25, Ido Schimmel wrote:
> On Sat, Jul 22, 2023 at 02:36:00AM +0200, Mirsad Todorovac wrote:
>> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
>> keyboard, for the test times out and leaves incoherent network stack.
>>
>> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
>> Cc: Ido Schimmel <idosch@nvidia.com>
>> Cc: netdev@vger.kernel.org
>> ---
> 
> The patches are missing your Signed-off-by and a cover letter. Anyway,
> please don't send a new version just yet. I'm not sure this is the
> correct approach and I'm looking into it.
> 
> Thanks

Sure thing. What a blunder, just when I thought I had the perfect patches.

Still, I think it is the way da go for all the test programs, to catch the
SIGINT and SIGTERM ...

This way, I need to reboot the system before running the tests on a clean
slate ...

Kind regards,
Mirsad
Mirsad Todorovac July 23, 2023, 9:37 p.m. UTC | #5
On 7/23/23 20:54, Mirsad Todorovac wrote:
> 
> 
> On 7/23/23 10:25, Ido Schimmel wrote:
>> On Sat, Jul 22, 2023 at 02:36:00AM +0200, Mirsad Todorovac wrote:
>>> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
>>> keyboard, for the test times out and leaves incoherent network stack.
>>>
>>> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
>>> Cc: Ido Schimmel <idosch@nvidia.com>
>>> Cc: netdev@vger.kernel.org
>>> ---
>>
>> The patches are missing your Signed-off-by and a cover letter. Anyway,
>> please don't send a new version just yet. I'm not sure this is the
>> correct approach and I'm looking into it.
>>
>> Thanks
> 
> Sure thing. What a blunder, just when I thought I had the perfect patches.
> 
> Still, I think it is the way da go for all the test programs, to catch the
> SIGINT and SIGTERM ...
> 
> This way, I need to reboot the system before running the tests on a clean
> slate ...

The tests now did not hang (applied all four patches of yours).

Thank you for your time and work patching.

Some tests however exited with error:

marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc2-net-forwarding-11.log
not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
not ok 15 selftests: net/forwarding: ethtool_extended_state.sh # exit=1
not ok 17 selftests: net/forwarding: ethtool.sh # exit=1
not ok 25 selftests: net/forwarding: hw_stats_l3_gre.sh # exit=1
not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
not ok 80 selftests: net/forwarding: tc_actions.sh # exit=1
not ok 83 selftests: net/forwarding: tc_flower.sh # exit=1
not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
not ok 89 selftests: net/forwarding: tc_tunnel_key.sh # exit=1
marvin@defiant:~/linux/kernel/linux_torvalds$

For your convenience, please find the complete test run output attached.

Kind regards,
Mirsad
Ido Schimmel July 24, 2023, 2:45 p.m. UTC | #6
On Sun, Jul 23, 2023 at 11:37:46PM +0200, Mirsad Todorovac wrote:
> Some tests however exited with error:
> 
> marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc2-net-forwarding-11.log
> not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
> not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
> not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1

I can't reproduce these three.

Do you have systemd-networkd running? If so, by default it tries to
take over interfaces unless you tell it not to. For example, on my
system I have:

$ cat /etc/systemd/network/10-ignore.link 
[Match]
OriginalName=*

[Link]
MACAddressPolicy=none

This tells systemd not to assign a persistent MAC address on virtual
interfaces.

And (redacted):

$ cat /etc/systemd/network/80-dhcp.network 
[Match]
...

This tells systemd to only manage the interfaces matching the match
criteria and ignore the rest.

> not ok 15 selftests: net/forwarding: ethtool_extended_state.sh # exit=1
> not ok 17 selftests: net/forwarding: ethtool.sh # exit=1
> not ok 25 selftests: net/forwarding: hw_stats_l3_gre.sh # exit=1

Fixed these three.

> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1

Fixed.

> not ok 80 selftests: net/forwarding: tc_actions.sh # exit=1
> not ok 83 selftests: net/forwarding: tc_flower.sh # exit=1
> not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
> not ok 89 selftests: net/forwarding: tc_tunnel_key.sh # exit=1

Can't reproduce these.

Pushed the fixes on top of the fixes from yesterday:

https://github.com/idosch/linux/commits/submit/sefltests_fix_v1
Mirsad Todorovac July 24, 2023, 8:46 p.m. UTC | #7
On 7/24/23 16:45, Ido Schimmel wrote:
> On Sun, Jul 23, 2023 at 11:37:46PM +0200, Mirsad Todorovac wrote:
>> Some tests however exited with error:

Hi,

>> marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc2-net-forwarding-11.log
>> not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
>> not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
>> not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
> 
> I can't reproduce these three.

I have now enabled 'set -x' and here is the link to the output.

NOTE as there are side-effects to running the test scripts, I have ran the
whole suite, just in case:

https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-12.log.xz

> Do you have systemd-networkd running?

No:

● defiant
     State: running
      Jobs: 0 queued
    Failed: 0 units
     Since: Mon 2023-07-24 18:52:51 CEST; 3h 45min ago
    CGroup: /
            ├─user.slice
            │ └─user-1000.slice
            │   ├─user@1000.service
            │   │ ├─session.slice
            │   │ │ ├─org.gnome.SettingsDaemon.MediaKeys.service
            │   │ │ │ └─3137 /usr/libexec/gsd-media-keys
            │   │ │ ├─org.gnome.SettingsDaemon.Smartcard.service
            │   │ │ │ └─3157 /usr/libexec/gsd-smartcard
            │   │ │ ├─org.gnome.SettingsDaemon.Datetime.service
            │   │ │ │ └─3131 /usr/libexec/gsd-datetime
            │   │ │ ├─xdg-document-portal.service
            │   │ │ │ ├─2799 /usr/libexec/xdg-document-portal
            │   │ │ │ └─2809 fusermount3 -o rw,nosuid,nodev,fsname=portal,auto_unmount,subtype=portal -- /run/user/1000/doc
            │   │ │ ├─org.gnome.SettingsDaemon.Housekeeping.service
            │   │ │ │ └─3132 /usr/libexec/gsd-housekeeping
            │   │ │ ├─xdg-desktop-portal.service
            │   │ │ │ └─3277 /usr/libexec/xdg-desktop-portal
            │   │ │ ├─org.freedesktop.IBus.session.GNOME.service
            │   │ │ │ ├─3126 sh -c /usr/bin/ibus-daemon --panel disable $([ "$XDG_SESSION_TYPE" = "x11" ] && echo "--xim")
            │   │ │ │ ├─3130 /usr/bin/ibus-daemon --panel disable
            │   │ │ │ ├─3229 /usr/libexec/ibus-dconf
            │   │ │ │ ├─3237 /usr/libexec/ibus-extension-gtk3
            │   │ │ │ └─3295 /usr/libexec/ibus-engine-simple
            │   │ │ ├─org.gnome.SettingsDaemon.Keyboard.service
            │   │ │ │ └─3136 /usr/libexec/gsd-keyboard
            │   │ │ ├─pipewire-media-session.service
            │   │ │ │ └─2751 /usr/bin/pipewire-media-session
            │   │ │ ├─org.gnome.SettingsDaemon.A11ySettings.service
            │   │ │ │ └─3127 /usr/libexec/gsd-a11y-settings
            │   │ │ ├─pulseaudio.service
            │   │ │ │ └─2752 /usr/bin/pulseaudio --daemonize=no --log-target=journal
            │   │ │ ├─org.gnome.SettingsDaemon.Wacom.service
            │   │ │ │ └─3161 /usr/libexec/gsd-wacom
            │   │ │ ├─org.gnome.SettingsDaemon.Sharing.service
            │   │ │ │ └─3156 /usr/libexec/gsd-sharing
            │   │ │ ├─org.gnome.SettingsDaemon.Color.service
            │   │ │ │ └─3128 /usr/libexec/gsd-color
            │   │ │ ├─org.gnome.SettingsDaemon.ScreensaverProxy.service
            │   │ │ │ └─3155 /usr/libexec/gsd-screensaver-proxy
            │   │ │ ├─org.gnome.SettingsDaemon.PrintNotifications.service
            │   │ │ │ ├─3143 /usr/libexec/gsd-print-notifications
            │   │ │ │ └─3290 /usr/libexec/gsd-printer
            │   │ │ ├─org.gnome.SettingsDaemon.Power.service
            │   │ │ │ └─3141 /usr/libexec/gsd-power
            │   │ │ ├─org.gnome.Shell@wayland.service
            │   │ │ │ ├─  2988 /usr/bin/gnome-shell
            │   │ │ │ └─414067 gjs /usr/share/gnome-shell/extensions/ding@rastersoft.com/ding.js -E -P /usr/share/gnome-shell/extensions/ding@rastersoft.com -M 0 -D 0:>
            │   │ │ ├─org.gnome.SettingsDaemon.Sound.service
            │   │ │ │ └─3159 /usr/libexec/gsd-sound
            │   │ │ ├─pipewire.service
            │   │ │ │ └─2750 /usr/bin/pipewire
            │   │ │ └─org.gnome.SettingsDaemon.Rfkill.service
            │   │ │   └─3148 /usr/libexec/gsd-rfkill
            │   │ ├─background.slice
            │   │ │ └─tracker-miner-fs-3.service
            │   │ │   └─2830 /usr/libexec/tracker-miner-fs-3
            │   │ ├─app.slice
            │   │ │ ├─speech-dispatcher.service
            │   │ │ │ ├─210924 /usr/bin/speech-dispatcher -s -t 0
            │   │ │ │ ├─210940 /usr/lib/speech-dispatcher-modules/sd_espeak-ng /etc/speech-dispatcher/modules/espeak-ng.conf
            │   │ │ │ └─210943 /usr/lib/speech-dispatcher-modules/sd_dummy /etc/speech-dispatcher/modules/dummy.conf
            │   │ │ ├─app-gnome-firefox-209967.scope
            │   │ │ │ ├─209967 /usr/lib/firefox/firefox
            │   │ │ │ ├─210044 /usr/lib/firefox/firefox -contentproc -parentBuildID 20230710165010 -prefsLen 30997 -prefMapSize 236126 -appDir /usr/lib/firefox/browser>
            │   │ │ │ ├─210071 /usr/lib/firefox/firefox -contentproc -childID 1 -isForBrowser -prefsLen 31138 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210161 /usr/lib/firefox/firefox -contentproc -childID 2 -isForBrowser -prefsLen 36726 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210248 /usr/lib/firefox/firefox -contentproc -parentBuildID 20230710165010 -prefsLen 36767 -prefMapSize 236126 -appDir /usr/lib/firefox/browser>
            │   │ │ │ ├─210249 /usr/lib/firefox/firefox -contentproc -parentBuildID 20230710165010 -sandboxingKind 0 -prefsLen 36767 -prefMapSize 236126 -appDir /usr/l>
            │   │ │ │ ├─210274 /usr/lib/firefox/firefox -contentproc -childID 4 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210278 /usr/lib/firefox/firefox -contentproc -childID 5 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210281 /usr/lib/firefox/firefox -contentproc -childID 6 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210291 /usr/lib/firefox/firefox -contentproc -childID 7 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210418 /usr/lib/firefox/firefox -contentproc -childID 8 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210468 /usr/lib/firefox/firefox -contentproc -childID 9 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 2023>
            │   │ │ │ ├─210471 /usr/lib/firefox/firefox -contentproc -childID 10 -isForBrowser -prefsLen 31117 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─210612 /usr/lib/firefox/firefox -contentproc -childID 12 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─210652 /usr/lib/firefox/firefox -contentproc -childID 13 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─210728 /usr/lib/firefox/firefox -contentproc -childID 15 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─210767 /usr/lib/firefox/firefox -contentproc -childID 16 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─210887 /usr/lib/firefox/firefox -contentproc -childID 18 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─211762 /usr/lib/firefox/firefox -contentproc -childID 37 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─211942 /usr/lib/firefox/firefox -contentproc -childID 39 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─212135 /usr/lib/firefox/firefox -contentproc -childID 43 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─242515 /usr/lib/firefox/firefox -contentproc -childID 70 -isForBrowser -prefsLen 31221 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 202>
            │   │ │ │ ├─311393 /usr/lib/firefox/firefox -contentproc -childID 108 -isForBrowser -prefsLen 31270 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ ├─414504 /usr/lib/firefox/firefox -contentproc -childID 205 -isForBrowser -prefsLen 31270 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ ├─414929 /usr/lib/firefox/firefox -contentproc -childID 214 -isForBrowser -prefsLen 31270 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ ├─415065 /usr/lib/firefox/firefox -contentproc -childID 217 -isForBrowser -prefsLen 31270 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ ├─415153 /usr/lib/firefox/firefox -contentproc -childID 218 -isForBrowser -prefsLen 31327 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ ├─415194 /usr/lib/firefox/firefox -contentproc -childID 219 -isForBrowser -prefsLen 31327 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ │ └─415241 /usr/lib/firefox/firefox -contentproc -childID 220 -isForBrowser -prefsLen 31327 -prefMapSize 236126 -jsInitLen 240908 -parentBuildID 20>
            │   │ │ ├─app-gnome-update\x2dnotifier-4388.scope
            │   │ │ │ └─4388 update-notifier
            │   │ │ ├─gvfs-goa-volume-monitor.service
            │   │ │ │ └─2848 /usr/libexec/gvfs-goa-volume-monitor
            │   │ │ ├─xdg-permission-store.service
            │   │ │ │ └─2803 /usr/libexec/xdg-permission-store
            │   │ │ ├─app-gnome-org.gnome.Evolution\x2dalarm\x2dnotify-3203.scope
            │   │ │ │ └─3203 /usr/libexec/evolution-data-server/evolution-alarm-notify
            │   │ │ ├─evolution-calendar-factory.service
            │   │ │ │ └─3054 /usr/libexec/evolution-calendar-factory
            │   │ │ ├─xdg-desktop-portal-gnome.service
            │   │ │ │ └─3282 /usr/libexec/xdg-desktop-portal-gnome
            │   │ │ ├─dconf.service
            │   │ │ │ └─3045 /usr/libexec/dconf-service
            │   │ │ ├─app-gnome\x2dsession\x2dmanager.slice
            │   │ │ │ └─gnome-session-manager@ubuntu.service
            │   │ │ │   └─2967 /usr/libexec/gnome-session-binary --systemd-service --session=ubuntu
            │   │ │ ├─gvfs-daemon.service
            │   │ │ │ ├─  2773 /usr/libexec/gvfsd
            │   │ │ │ ├─  2778 /usr/libexec/gvfsd-fuse /run/user/1000/gvfs -f
            │   │ │ │ ├─  3075 /usr/libexec/gvfsd-trash --spawner :1.2 /org/gtk/gvfs/exec_spaw/0
            │   │ │ │ ├─415094 /usr/libexec/gvfsd-network --spawner :1.2 /org/gtk/gvfs/exec_spaw/1
            │   │ │ │ └─415114 /usr/libexec/gvfsd-dnssd --spawner :1.2 /org/gtk/gvfs/exec_spaw/3
            │   │ │ ├─evolution-source-registry.service
            │   │ │ │ └─3043 /usr/libexec/evolution-source-registry
            │   │ │ ├─gvfs-udisks2-volume-monitor.service
            │   │ │ │ └─2835 /usr/libexec/gvfs-udisks2-volume-monitor
            │   │ │ ├─snap.snapd-desktop-integration.snapd-desktop-integration.service
            │   │ │ │ ├─3408 /snap/snapd-desktop-integration/83/usr/bin/snapd-desktop-integration
            │   │ │ │ └─3486 /snap/snapd-desktop-integration/83/usr/bin/snapd-desktop-integration
            │   │ │ ├─app-gnome-thunderbird-247930.scope
            │   │ │ │ ├─247930 /usr/lib/thunderbird/thunderbird
            │   │ │ │ └─248039 /usr/lib/thunderbird/thunderbird -contentproc -childID 1 -isForBrowser -prefsLen 11247 -prefMapSize 234029 -jsInitLen 277276 -parentBuil>
            │   │ │ ├─app-gnome-at\x2dspi\x2ddbus\x2dbus-2987.scope
            │   │ │ │ ├─2987 /usr/libexec/at-spi-bus-launcher --launch-immediately
            │   │ │ │ ├─2999 /usr/bin/dbus-daemon --config-file=/usr/share/defaults/at-spi2/accessibility.conf --nofork --print-address 11 --address=unix:path=/run/use>
            │   │ │ │ └─3091 /usr/libexec/at-spi2-registryd --use-gnome-session
            │   │ │ ├─app-org.gnome.Terminal.slice
            │   │ │ │ ├─vte-spawn-206cdf5a-0844-40f4-bde4-2795906ecfbf.scope
            │   │ │ │ │ ├─415133 bash
            │   │ │ │ │ ├─415265 systemctl status
            │   │ │ │ │ └─415266 pager
            │   │ │ │ ├─vte-spawn-1cee6c96-97c6-49f7-87b8-56316324bc9d.scope
            │   │ │ │ │ ├─259808 bash
            │   │ │ │ │ └─259849 ssh mtodorov@orion.grf.unizg.hr
            │   │ │ │ ├─vte-spawn-227851cc-18ba-495a-8614-bd7b6988cd46.scope
            │   │ │ │ │ └─4233 bash
            │   │ │ │ ├─vte-spawn-8e1d0389-31ab-4a66-ba0f-19f3b652e825.scope
            │   │ │ │ │ ├─251437 bash
            │   │ │ │ │ └─251459 ssh mtodorov@magrf.grf.unizg.hr
            │   │ │ │ ├─gnome-terminal-server.service
            │   │ │ │ │ └─3527 /usr/libexec/gnome-terminal-server
            │   │ │ │ └─vte-spawn-fe998971-59c0-419a-ae1c-4d74c2fe9e24.scope
            │   │ │ │   └─3553 bash
            │   │ │ ├─gvfs-gphoto2-volume-monitor.service
            │   │ │ │ └─2844 /usr/libexec/gvfs-gphoto2-volume-monitor
            │   │ │ ├─gnome-session-monitor.service
            │   │ │ │ └─2946 /usr/libexec/gnome-session-ctl --monitor
            │   │ │ ├─xdg-desktop-portal-gtk.service
            │   │ │ │ └─3340 /usr/libexec/xdg-desktop-portal-gtk
            │   │ │ ├─gvfs-metadata.service
            │   │ │ │ └─3396 /usr/libexec/gvfsd-metadata
            │   │ │ ├─dbus.service
            │   │ │ │ ├─  2766 /usr/bin/dbus-daemon --session --address=systemd: --nofork --nopidfile --systemd-activation --syslog-only
            │   │ │ │ ├─  2852 /usr/libexec/goa-daemon
            │   │ │ │ ├─  2859 /usr/libexec/goa-identity-service
            │   │ │ │ ├─  3037 /usr/libexec/gnome-shell-calendar-server
            │   │ │ │ ├─  3089 /usr/bin/gjs /usr/share/gnome-shell/org.gnome.Shell.Notifications
            │   │ │ │ ├─  3244 /usr/libexec/ibus-portal
            │   │ │ │ ├─  3306 /usr/bin/gjs /usr/share/gnome-shell/org.gnome.ScreenSaver
            │   │ │ │ └─233902 /usr/bin/gedit --gapplication-service
            │   │ │ ├─evolution-addressbook-factory.service
            │   │ │ │ └─3065 /usr/libexec/evolution-addressbook-factory
            │   │ │ ├─gvfs-mtp-volume-monitor.service
            │   │ │ │ └─2840 /usr/libexec/gvfs-mtp-volume-monitor
            │   │ │ ├─app-gnome-org.gnome.SettingsDaemon.DiskUtilityNotify-3218.scope
            │   │ │ │ └─3218 /usr/libexec/gsd-disk-utility-notify
            │   │ │ └─gvfs-afc-volume-monitor.service
            │   │ │   └─2861 /usr/libexec/gvfs-afc-volume-monitor
            │   │ └─init.scope
            │   │   ├─2743 /lib/systemd/systemd --user
            │   │   └─2744 (sd-pam)
            │   └─session-2.scope
            │     ├─  2731 gdm-session-worker [pam/gdm-password]
            │     ├─  2762 /usr/bin/gnome-keyring-daemon --daemonize --login
            │     ├─  2890 /usr/libexec/gdm-wayland-session env GNOME_SHELL_SESSION_MODE=ubuntu /usr/bin/gnome-session --session=ubuntu
            │     ├─  2893 /usr/libexec/gnome-session-binary --session=ubuntu
            │     └─251933 /usr/bin/ssh-agent -D -a /run/user/1000/keyring/.ssh
            ├─init.scope
            │ └─1 /sbin/init splash
            └─system.slice
              ├─irqbalance.service
              │ └─1282 /usr/sbin/irqbalance --foreground
              ├─haveged.service
              │ └─1110 /usr/sbin/haveged --Foreground --verbose=1
              ├─packagekit.service
              │ └─2503 /usr/libexec/packagekitd
              ├─systemd-udevd.service
              │ └─609 /lib/systemd/systemd-udevd
              ├─cron.service
              │ └─1270 /usr/sbin/cron -f -P
              ├─polkit.service
              │ └─1289 /usr/libexec/polkitd --no-debug
              ├─networkd-dispatcher.service
              │ └─1285 /usr/bin/python3 /usr/bin/networkd-dispatcher --run-startup-triggers
              ├─rtkit-daemon.service
              │ └─2306 /usr/libexec/rtkit-daemon
              ├─accounts-daemon.service
              │ └─1263 /usr/libexec/accounts-daemon
              ├─mcollective.service
              │ └─1402 ruby /usr/sbin/mcollectived --config=/etc/mcollective/server.cfg --pidfile=/var/run/mcollective.pid --no-daemonize
              ├─wpa_supplicant.service
              │ └─1309 /sbin/wpa_supplicant -u -s -O /run/wpa_supplicant
              ├─ModemManager.service
              │ └─1389 /usr/sbin/ModemManager
              ├─systemd-journald.service
              │ └─579 /lib/systemd/systemd-journald
              ├─power-profiles-daemon.service
              │ └─1293 /usr/libexec/power-profiles-daemon
              ├─unattended-upgrades.service
              │ └─1415 /usr/bin/python3 /usr/share/unattended-upgrades/unattended-upgrade-shutdown --wait-for-signal
              ├─ssh.service
              │ └─1431 sshd: /usr/sbin/sshd -D [listener] 0 of 10-100 startups
              ├─colord.service
              │ └─2665 /usr/libexec/colord
              ├─NetworkManager.service
              │ └─1276 /usr/sbin/NetworkManager --no-daemon
              ├─snapd.service
              │ └─1300 /usr/lib/snapd/snapd
              ├─gdm.service
              │ └─2245 /usr/sbin/gdm3
              ├─switcheroo-control.service
              │ └─1301 /usr/libexec/switcheroo-control
              ├─rsyslog.service
              │ └─1294 /usr/sbin/rsyslogd -n -iNONE
              ├─snap.canonical-livepatch.canonical-livepatchd.service
              │ └─1406 /snap/canonical-livepatch/235/canonical-livepatchd
              ├─kerneloops.service
              │ ├─2191 /usr/sbin/kerneloops --test
              │ └─2202 /usr/sbin/kerneloops
              ├─cups-browsed.service
              │ └─2163 /usr/sbin/cups-browsed
              ├─smartmontools.service
              │ └─1297 /usr/sbin/smartd -n
              ├─cups.service
              │ └─1401 /usr/sbin/cupsd -l
              ├─pcscd.service
              │ └─2600 /usr/sbin/pcscd --foreground --debug
              ├─upower.service
              │ └─2461 /usr/libexec/upowerd
              ├─systemd-oomd.service
              │ └─1122 /lib/systemd/systemd-oomd
              ├─systemd-resolved.service
              │ └─1124 /lib/systemd/systemd-resolved
              ├─udisks2.service
              │ └─1308 /usr/libexec/udisks2/udisksd
              ├─acpid.service
              │ └─1264 /usr/sbin/acpid
              ├─ntp.service
              │ └─1424 /usr/sbin/ntpd -p /var/run/ntpd.pid -g -u 130:138
              ├─dbus.service
              │ └─1273 @dbus-daemon --system --address=systemd: --nofork --nopidfile --systemd-activation --syslog-only
              ├─avahi-daemon.service
              │ ├─1268 avahi-daemon: running [defiant.local]
              │ └─1314 avahi-daemon: chroot helper
              └─systemd-logind.service
                └─1305 /lib/systemd/systemd-logind


> If so, by default it tries to
> take over interfaces unless you tell it not to. For example, on my
> system I have:
> 
> $ cat /etc/systemd/network/10-ignore.link
> [Match]
> OriginalName=*
> 
> [Link]
> MACAddressPolicy=none
> 
> This tells systemd not to assign a persistent MAC address on virtual
> interfaces.
> 
> And (redacted):
> 
> $ cat /etc/systemd/network/80-dhcp.network
> [Match]
> ...
> 
> This tells systemd to only manage the interfaces matching the match
> criteria and ignore the rest.

I didn't see how could I adjust this to my configuration, but I had some dayjob
tasks, too ...

>> not ok 15 selftests: net/forwarding: ethtool_extended_state.sh # exit=1
>> not ok 17 selftests: net/forwarding: ethtool.sh # exit=1
>> not ok 25 selftests: net/forwarding: hw_stats_l3_gre.sh # exit=1
> 
> Fixed these three.
> 
>> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
> 
> Fixed.

Great job, that's almost 50% of them!

>> not ok 80 selftests: net/forwarding: tc_actions.sh # exit=1
>> not ok 83 selftests: net/forwarding: tc_flower.sh # exit=1
>> not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
>> not ok 89 selftests: net/forwarding: tc_tunnel_key.sh # exit=1
> 
> Can't reproduce these.

Hope the above will help.

> Pushed the fixes on top of the fixes from yesterday:
> 
> https://github.com/idosch/linux/commits/submit/sefltests_fix_v1

I have applied them.

BTW, after running the full net/forwarding test suite, "ip link show"
looks like this:

marvin@defiant:~/linux/kernel/linux_torvalds$ ip link show
256: veth7@veth6: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 16:74:e0:e6:f0:92 brd ff:ff:ff:ff:ff:ff
257: veth6@veth7: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 22:f3:40:50:fb:73 brd ff:ff:ff:ff:ff:ff
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: enp16s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
     link/ether 9c:6b:00:01:fb:80 brd ff:ff:ff:ff:ff:ff
3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether b6:46:e6:4c:e4:00 brd ff:ff:ff:ff:ff:ff
4: veth0@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 2e:ff:7f:8a:6b:d4 brd ff:ff:ff:ff:ff:ff
5: veth3@veth2: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether ba:33:37:81:dc:5b brd ff:ff:ff:ff:ff:ff
6: veth2@veth3: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether f2:fd:0a:9b:94:17 brd ff:ff:ff:ff:ff:ff
278: veth9@veth8: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 0a:f1:22:04:0f:55 brd ff:ff:ff:ff:ff:ff
279: veth8@veth9: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 92:be:71:00:59:0f brd ff:ff:ff:ff:ff:ff
282: gre0@NONE: <NOARP> mtu 1476 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/gre 0.0.0.0 brd 0.0.0.0
283: gretap0@NONE: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
284: erspan0@NONE: <BROADCAST,MULTICAST> mtu 1450 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
366: ip6tnl0@NONE: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/tunnel6 :: brd :: permaddr ce1e:75f3:f565::
367: ip6gre0@NONE: <NOARP> mtu 1448 qdisc noop state DOWN mode DEFAULT group default qlen 1000
     link/gre6 :: brd :: permaddr 1e91:da47:154d::
237: veth5@veth4: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether 6a:e3:dc:ad:8c:a0 brd ff:ff:ff:ff:ff:ff
238: veth4@veth5: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
     link/ether ce:a7:61:90:c8:2d brd ff:ff:ff:ff:ff:ff
marvin@defiant:~/linux/kernel/linux_torvalds$

This is kinda awkward, because I have to reboot the machine for the next run, each time.

I am in no condition to try to figure out which tests leaked links.

I will leave the machine on in case you need additional diagnostics.

Thanks.

Kind regards,
Mirsad
Petr Machata July 25, 2023, 8:44 a.m. UTC | #8
Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> writes:

> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
> keyboard, for the test times out and leaves incoherent network stack.
>
> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
> Cc: Ido Schimmel <idosch@nvidia.com>
> Cc: netdev@vger.kernel.org
> ---
>  tools/testing/selftests/net/forwarding/custom_multipath_hash.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
> index 56eb83d1a3bd..c7ab883d2515 100755
> --- a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
> +++ b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
> @@ -363,7 +363,7 @@ custom_hash()
>  	custom_hash_v6
>  }
>  
> -trap cleanup EXIT
> +trap cleanup INT TERM EXIT
>  
>  setup_prepare
>  setup_wait

I believe the EXIT trap covers whatever the cause of the exit was, i.e.
INT and TERM are implicitly covered:

    $ vim tmp/x.sh
    $ cat tmp/x.sh
    foo() { date; }
    trap foo EXIT
    read -p Ready.
    $ bash tmp/x.sh
    Ready.^CTue Jul 25 10:44:20 AM CEST 2023

Also, the interrupt trap seems to prevent the exit actually:

    $ cat tmp/x.sh
    foo() { date; }
    trap foo INT TERM EXIT
    read -p Ready.
    [petr@yaviefel ~]$ bash tmp/x.sh 
    Ready.^CTue Jul 25 10:43:35 AM CEST 2023
    ^CTue Jul 25 10:43:35 AM CEST 2023
    ^CTue Jul 25 10:43:36 AM CEST 2023
    ^CTue Jul 25 10:43:36 AM CEST 2023

(I see the same when I kill -TERM the script.)

This would call cleanup, which would dismantle the configuration, but
then would happilly proceed in the script. I might be missing something,
but I don't see how this can work.
Mirsad Todorovac July 25, 2023, 4:23 p.m. UTC | #9
On 7/25/23 10:44, Petr Machata wrote:
> 
> Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> writes:
> 
>> Add trap and cleanup for SIGTERM sent by timeout and SIGINT from
>> keyboard, for the test times out and leaves incoherent network stack.
>>
>> Fixes: 511e8db54036c ("selftests: forwarding: Add test for custom multipath hash")
>> Cc: Ido Schimmel <idosch@nvidia.com>
>> Cc: netdev@vger.kernel.org
>> ---
>>   tools/testing/selftests/net/forwarding/custom_multipath_hash.sh | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> index 56eb83d1a3bd..c7ab883d2515 100755
>> --- a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> +++ b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
>> @@ -363,7 +363,7 @@ custom_hash()
>>   	custom_hash_v6
>>   }
>>   
>> -trap cleanup EXIT
>> +trap cleanup INT TERM EXIT
>>   
>>   setup_prepare
>>   setup_wait
> 
> I believe the EXIT trap covers whatever the cause of the exit was, i.e.
> INT and TERM are implicitly covered:
> 
>      $ vim tmp/x.sh
>      $ cat tmp/x.sh
>      foo() { date; }
>      trap foo EXIT
>      read -p Ready.
>      $ bash tmp/x.sh
>      Ready.^CTue Jul 25 10:44:20 AM CEST 2023

Thank you Petr for going to the bottom of this.

This is probably specific to bash. I tried to figure out from the manual,
but it wasn't so precise as direct experiment.

> Also, the interrupt trap seems to prevent the exit actually:
> 
>      $ cat tmp/x.sh
>      foo() { date; }
>      trap foo INT TERM EXIT
>      read -p Ready.
>      [petr@yaviefel ~]$ bash tmp/x.sh
>      Ready.^CTue Jul 25 10:43:35 AM CEST 2023
>      ^CTue Jul 25 10:43:35 AM CEST 2023
>      ^CTue Jul 25 10:43:36 AM CEST 2023
>      ^CTue Jul 25 10:43:36 AM CEST 2023
> 
> (I see the same when I kill -TERM the script.)
> 
> This would call cleanup, which would dismantle the configuration, but
> then would happilly proceed in the script. I might be missing something,
> but I don't see how this can work.

Certainly, an explicit 'exit' from the 'cleanup' function would do.

It is bound to exit in any case, so explicit exit can't hurt. But if 'trap cleanup EXIT'
catches all cases, then my patch set is clearly obsoleted.

I didn't see the logic in EXIT catching SIGINT and SIGTERM when there are explicit
traps, but that's bash issue, not  selftest/net/forwarding issue :-)

I should apologise, but my understanding of the manuals went after the 'ash' semanthics
of the trap.

The manual does say:

       trap [-lp] [[arg] sigspec ...]
              The command arg is to be read and executed when the shell receives signal(s) sigspec.  If arg is absent (and there is a  single  sigspec)  or  -,  each
              specified signal is reset to its original disposition (the value it had upon entrance to the shell).  If arg is the null string the signal specified by
              each sigspec is ignored by the shell and by the commands it invokes.  If arg is not present and -p has been supplied, then the trap commands associated
              with  each  sigspec  are displayed.  If no arguments are supplied or if only -p is given, trap prints the list of commands associated with each signal.
              The -l option causes the shell to print a list of signal names and their corresponding numbers.  Each sigspec is either a signal name defined in  <sig‐
              nal.h>, or a signal number.  Signal names are case insensitive and the SIG prefix is optional.

              If  a  sigspec  is EXIT (0) the command arg is executed on exit from the shell.  If a sigspec is DEBUG, the command arg is executed before every simple
              command, for command, case command, select command, every arithmetic for command, and before the first command executes in a shell function (see  SHELL
              GRAMMAR above).  Refer to the description of the extdebug option to the shopt builtin for details of its effect on the DEBUG trap.  If a sigspec is RE‐
              TURN, the command arg is executed each time a shell function or a script executed with the . or source builtins finishes executing.

              If a sigspec is ERR, the command arg is executed whenever a pipeline (which may consist of a single simple command), a list, or a compound command  re‐
              turns  a non-zero exit status, subject to the following conditions.  The ERR trap is not executed if the failed command is part of the command list im‐
              mediately following a while or until keyword, part of the test in an if statement, part of a command executed in a && or ||  list  except  the  command
              following  the  final && or ||, any command in a pipeline but the last, or if the command's return value is being inverted using !.  These are the same
              conditions obeyed by the errexit (-e) option.

              Signals ignored upon entry to the shell cannot be trapped or reset.  Trapped signals that are not being ignored are reset to their original values in a
              subshell or subshell environment when one is created.  The return status is false if any sigspec is invalid; otherwise trap returns true.

Maybe "If  a  sigspec  is EXIT (0) the command arg is executed on exit from the shell." should
have had less assumptions on what is obvious to the reader?

But from this it wasn't obvious to me that EXIT will catch exit by signals SIGINT and SIGTERM.

Perhaps mostly because of the leftovers after cleanup()?

Still it seems impossible to run two consecutive test runs without an intermediate reboot.
('systemctl restart networking' didn't help on ubuntu 22.04).

It doesn't seem impossible to fix these, but I think you as authors and knowers of the Linux
networking stack will do a better job at it.

Though I seem to benefit from these brainstorming exercises as well :-)

Thanks
Mirsad
Ido Schimmel July 26, 2023, 12:22 p.m. UTC | #10
On Mon, Jul 24, 2023 at 10:46:09PM +0200, Mirsad Todorovac wrote:
> On 7/24/23 16:45, Ido Schimmel wrote:
> > On Sun, Jul 23, 2023 at 11:37:46PM +0200, Mirsad Todorovac wrote:
> > > Some tests however exited with error:
> 
> Hi,
> 
> > > marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc2-net-forwarding-11.log
> > > not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
> > > not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
> > > not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
> > 
> > I can't reproduce these three.
> 
> I have now enabled 'set -x' and here is the link to the output.
> 
> NOTE as there are side-effects to running the test scripts, I have ran the

I don't believe this is correct after "selftests: forwarding: Switch off
timeout".

> whole suite, just in case:
> 
> https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-12.log.xz
> 
> > Do you have systemd-networkd running?
> 
> No:

[...]

> > > not ok 15 selftests: net/forwarding: ethtool_extended_state.sh # exit=1
> > > not ok 17 selftests: net/forwarding: ethtool.sh # exit=1
> > > not ok 25 selftests: net/forwarding: hw_stats_l3_gre.sh # exit=1
> > 
> > Fixed these three.
> > 
> > > not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
> > 
> > Fixed.
> 
> Great job, that's almost 50% of them!
> 
> > > not ok 80 selftests: net/forwarding: tc_actions.sh # exit=1
> > > not ok 83 selftests: net/forwarding: tc_flower.sh # exit=1
> > > not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
> > > not ok 89 selftests: net/forwarding: tc_tunnel_key.sh # exit=1
> > 
> > Can't reproduce these.
> 
> Hope the above will help.

Pushed fixes for tc_actions.sh, tc_flower.sh and tc_tunnel_key.sh to the
same branch. Please test them.

Regarding the MDB tests and tc_flower_l2_miss.sh, I suspect you might
have some daemon in user space that sends IGMP queries and therefore
messes with the tests. Please run the following commands in a separate
terminal before running tc_flower_l2_miss.sh:

# perf probe --add 'br_ip4_multicast_query'
# perf record -a -g -e 'probe:br_ip4_multicast_query'

After the test finishes, terminate the second command and run:

# perf report --stdio

It should show us if queries were received and which process sent them.

> 
> > Pushed the fixes on top of the fixes from yesterday:
> > 
> > https://github.com/idosch/linux/commits/submit/sefltests_fix_v1
> 
> I have applied them.
> 
> BTW, after running the full net/forwarding test suite, "ip link show"
> looks like this:
> 
> marvin@defiant:~/linux/kernel/linux_torvalds$ ip link show
> 256: veth7@veth6: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 16:74:e0:e6:f0:92 brd ff:ff:ff:ff:ff:ff
> 257: veth6@veth7: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 22:f3:40:50:fb:73 brd ff:ff:ff:ff:ff:ff
> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
>     link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
> 2: enp16s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
>     link/ether 9c:6b:00:01:fb:80 brd ff:ff:ff:ff:ff:ff
> 3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether b6:46:e6:4c:e4:00 brd ff:ff:ff:ff:ff:ff
> 4: veth0@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 2e:ff:7f:8a:6b:d4 brd ff:ff:ff:ff:ff:ff
> 5: veth3@veth2: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether ba:33:37:81:dc:5b brd ff:ff:ff:ff:ff:ff
> 6: veth2@veth3: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether f2:fd:0a:9b:94:17 brd ff:ff:ff:ff:ff:ff
> 278: veth9@veth8: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 0a:f1:22:04:0f:55 brd ff:ff:ff:ff:ff:ff
> 279: veth8@veth9: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 92:be:71:00:59:0f brd ff:ff:ff:ff:ff:ff
> 282: gre0@NONE: <NOARP> mtu 1476 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/gre 0.0.0.0 brd 0.0.0.0
> 283: gretap0@NONE: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> 284: erspan0@NONE: <BROADCAST,MULTICAST> mtu 1450 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
> 366: ip6tnl0@NONE: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/tunnel6 :: brd :: permaddr ce1e:75f3:f565::
> 367: ip6gre0@NONE: <NOARP> mtu 1448 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>     link/gre6 :: brd :: permaddr 1e91:da47:154d::
> 237: veth5@veth4: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether 6a:e3:dc:ad:8c:a0 brd ff:ff:ff:ff:ff:ff
> 238: veth4@veth5: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>     link/ether ce:a7:61:90:c8:2d brd ff:ff:ff:ff:ff:ff
> marvin@defiant:~/linux/kernel/linux_torvalds$
> 
> This is kinda awkward, because I have to reboot the machine for the next run, each time.

Why? The fact that the veth pairs are already present doesn't impact the
selftests.

> 
> I am in no condition to try to figure out which tests leaked links.

The veth pairs were created by the first invocation of the selftests and
are not deleted at the end. We already discussed it. But the fact that
they are already present does not mean you can't re-run the tests.

Regarding gre0, gretap0, erspan0, ip6tnl0 and ip6gre0, they are
automatically created by the kernel when the relevant kernel modules are
loaded. They are not used by the selftests.
Ido Schimmel July 26, 2023, 4:57 p.m. UTC | #11
On Wed, Jul 26, 2023 at 03:22:54PM +0300, Ido Schimmel wrote:
> Regarding the MDB tests and tc_flower_l2_miss.sh, I suspect you might
> have some daemon in user space that sends IGMP queries and therefore
> messes with the tests. Please run the following commands in a separate
> terminal before running tc_flower_l2_miss.sh:

Ignore that. I think it's a problem I already fixed in the past:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8bcfb4ae4d970b9a9724ddfbac26c387934e0e94

Ubuntu still uses an old version of libnet:
https://launchpad.net/ubuntu/+source/libnet

Pushed the fixes for tc_flower_l2_miss.sh, bridge_mdb.sh and
bridge_mdb_max.sh to the same branch.
Mirsad Todorovac July 27, 2023, 3:48 a.m. UTC | #12
On 7/26/23 18:57, Ido Schimmel wrote:
> On Wed, Jul 26, 2023 at 03:22:54PM +0300, Ido Schimmel wrote:
>> Regarding the MDB tests and tc_flower_l2_miss.sh, I suspect you might
>> have some daemon in user space that sends IGMP queries and therefore
>> messes with the tests. Please run the following commands in a separate
>> terminal before running tc_flower_l2_miss.sh:
> 
> Ignore that. I think it's a problem I already fixed in the past:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=8bcfb4ae4d970b9a9724ddfbac26c387934e0e94
> 
> Ubuntu still uses an old version of libnet:
> https://launchpad.net/ubuntu/+source/libnet
> 
> Pushed the fixes for tc_flower_l2_miss.sh, bridge_mdb.sh and
> bridge_mdb_max.sh to the same branch.

OK, just to give you some feedback, I will fix these in the afternoon (Lord willing) after my day job
for the situation appeared on my home box.

I wish I could thank you properly for your work but I do not own the Linux nor am I a maintainer :-)

Kind regards,
Mirsad
Mirsad Todorovac July 27, 2023, 7:26 p.m. UTC | #13
On 7/26/23 14:22, Ido Schimmel wrote:
> On Mon, Jul 24, 2023 at 10:46:09PM +0200, Mirsad Todorovac wrote:
>> On 7/24/23 16:45, Ido Schimmel wrote:
>>> On Sun, Jul 23, 2023 at 11:37:46PM +0200, Mirsad Todorovac wrote:
>>>> Some tests however exited with error:
>>
>> Hi,
>>
>>>> marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc2-net-forwarding-11.log
>>>> not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
>>>> not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
>>>> not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
>>>
>>> I can't reproduce these three.
>>
>> I have now enabled 'set -x' and here is the link to the output.
>>
>> NOTE as there are side-effects to running the test scripts, I have ran the
> 
> I don't believe this is correct after "selftests: forwarding: Switch off
> timeout".
> 
>> whole suite, just in case:
>>
>> https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-12.log.xz
>>
>>> Do you have systemd-networkd running?
>>
>> No:
> 
> [...]
> 
>>>> not ok 15 selftests: net/forwarding: ethtool_extended_state.sh # exit=1
>>>> not ok 17 selftests: net/forwarding: ethtool.sh # exit=1
>>>> not ok 25 selftests: net/forwarding: hw_stats_l3_gre.sh # exit=1
>>>
>>> Fixed these three.
>>>
>>>> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
>>>
>>> Fixed.
>>
>> Great job, that's almost 50% of them!
>>
>>>> not ok 80 selftests: net/forwarding: tc_actions.sh # exit=1
>>>> not ok 83 selftests: net/forwarding: tc_flower.sh # exit=1
>>>> not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
>>>> not ok 89 selftests: net/forwarding: tc_tunnel_key.sh # exit=1
>>>
>>> Can't reproduce these.
>>
>> Hope the above will help.
> 
> Pushed fixes for tc_actions.sh, tc_flower.sh and tc_tunnel_key.sh to the
> same branch. Please test them.
> 
> Regarding the MDB tests and tc_flower_l2_miss.sh, I suspect you might
> have some daemon in user space that sends IGMP queries and therefore
> messes with the tests. Please run the following commands in a separate
> terminal before running tc_flower_l2_miss.sh:
> 
> # perf probe --add 'br_ip4_multicast_query'
> # perf record -a -g -e 'probe:br_ip4_multicast_query'
> 
> After the test finishes, terminate the second command and run:
> 
> # perf report --stdio
> 
> It should show us if queries were received and which process sent them.
> 
>>
>>> Pushed the fixes on top of the fixes from yesterday:
>>>
>>> https://github.com/idosch/linux/commits/submit/sefltests_fix_v1
>>
>> I have applied them.
>>
>> BTW, after running the full net/forwarding test suite, "ip link show"
>> looks like this:
>>
>> marvin@defiant:~/linux/kernel/linux_torvalds$ ip link show
>> 256: veth7@veth6: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 16:74:e0:e6:f0:92 brd ff:ff:ff:ff:ff:ff
>> 257: veth6@veth7: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 22:f3:40:50:fb:73 brd ff:ff:ff:ff:ff:ff
>> 1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
>>      link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
>> 2: enp16s0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc fq_codel state UP mode DEFAULT group default qlen 1000
>>      link/ether 9c:6b:00:01:fb:80 brd ff:ff:ff:ff:ff:ff
>> 3: veth1@veth0: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether b6:46:e6:4c:e4:00 brd ff:ff:ff:ff:ff:ff
>> 4: veth0@veth1: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 2e:ff:7f:8a:6b:d4 brd ff:ff:ff:ff:ff:ff
>> 5: veth3@veth2: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether ba:33:37:81:dc:5b brd ff:ff:ff:ff:ff:ff
>> 6: veth2@veth3: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether f2:fd:0a:9b:94:17 brd ff:ff:ff:ff:ff:ff
>> 278: veth9@veth8: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 0a:f1:22:04:0f:55 brd ff:ff:ff:ff:ff:ff
>> 279: veth8@veth9: <BROADCAST,MULTICAST,M-DOWN> mtu 1500 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 92:be:71:00:59:0f brd ff:ff:ff:ff:ff:ff
>> 282: gre0@NONE: <NOARP> mtu 1476 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>      link/gre 0.0.0.0 brd 0.0.0.0
>> 283: gretap0@NONE: <BROADCAST,MULTICAST> mtu 1462 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
>> 284: erspan0@NONE: <BROADCAST,MULTICAST> mtu 1450 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 00:00:00:00:00:00 brd ff:ff:ff:ff:ff:ff
>> 366: ip6tnl0@NONE: <NOARP> mtu 1452 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>      link/tunnel6 :: brd :: permaddr ce1e:75f3:f565::
>> 367: ip6gre0@NONE: <NOARP> mtu 1448 qdisc noop state DOWN mode DEFAULT group default qlen 1000
>>      link/gre6 :: brd :: permaddr 1e91:da47:154d::
>> 237: veth5@veth4: <BROADCAST,MULTICAST,M-DOWN> mtu 2000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether 6a:e3:dc:ad:8c:a0 brd ff:ff:ff:ff:ff:ff
>> 238: veth4@veth5: <BROADCAST,MULTICAST,M-DOWN> mtu 10000 qdisc noqueue state DOWN mode DEFAULT group default qlen 1000
>>      link/ether ce:a7:61:90:c8:2d brd ff:ff:ff:ff:ff:ff
>> marvin@defiant:~/linux/kernel/linux_torvalds$
>>
>> This is kinda awkward, because I have to reboot the machine for the next run, each time.
> 
> Why? The fact that the veth pairs are already present doesn't impact the
> selftests.
> 
>>
>> I am in no condition to try to figure out which tests leaked links.
> 
> The veth pairs were created by the first invocation of the selftests and
> are not deleted at the end. We already discussed it. But the fact that
> they are already present does not mean you can't re-run the tests.
> 
> Regarding gre0, gretap0, erspan0, ip6tnl0 and ip6gre0, they are
> automatically created by the kernel when the relevant kernel modules are
> loaded. They are not used by the selftests.

If you're in dilemma, my experiment had shown that it is sufficient to delete one
side of the veth link, for another side automagically vanishes.

BTW, the patches successfully applied, safe for the following:

error: patch failed: tools/testing/selftests/net/forwarding/hw_stats_l3_gre.sh:99
error: tools/testing/selftests/net/forwarding/hw_stats_l3_gre.sh: patch does not apply

error: patch failed: tools/testing/selftests/net/forwarding/ethtool_extended_state.sh:108
error: tools/testing/selftests/net/forwarding/ethtool_extended_state.sh: patch does not apply

error: patch failed: tools/testing/selftests/net/forwarding/ethtool_mm.sh:278
error: tools/testing/selftests/net/forwarding/ethtool_mm.sh: patch does not apply

(Manual inspection revealed that all of those are adding of skip_on_veth which was already
present in the script, but I recall you added skip_on_veth recently, so I guess this is something
in our patch communication.)

The test results are very good:

marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc3-net-forwarding-16.log
not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1
not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
marvin@defiant:~/linux/kernel/linux_torvalds$ grep -v "^# +" ../kselftest-6.5-rc3-net-forwarding-16.log | grep -A1 FAIL | grep -v -e -- | grep -v OK
# TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
# 	"temp" entry has an unpending group timer
# TEST: IPv4 host entries forwarding tests                            [FAIL]
# 	Packet not locally received after adding a host entry
# TEST: IPv4 port group "exclude" entries forwarding tests            [FAIL]
# 	Packet from valid source not received on H2 after adding entry
# TEST: IPv4 port group "include" entries forwarding tests            [FAIL]
# 	Packet from valid source not received on H2 after adding entry
# TEST: IGMPv3 MODE_IS_INCLUDE tests                                  [FAIL]
# 	Source not add to source list
# TEST: ctl4: port: ngroups reporting                                 [FAIL]
# 	Couldn't add MDB entries
# TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
# 	Adding 5 MDB entries failed but should have passed
# TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
# 	dev veth1: Couldn't add MDB entries
# TEST: ctl4: port: ngroups reporting                                 [FAIL]
# 	Couldn't add MDB entries
# TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
# 	Adding 5 MDB entries failed but should have passed
# TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
# 	dev veth1 vid 10: Couldn't add MDB entries
# TEST: ctl4: port_vlan: ngroups reporting                            [FAIL]
# 	Couldn't add MDB entries
# TEST: ctl4: port_vlan: isolation of port and per-VLAN ngroups       [FAIL]
# 	Couldn't add MDB entries to VLAN 10
# TEST: ctl4: port_vlan maxgroups: reporting and treatment of 0       [FAIL]
# 	Adding 5 MDB entries failed but should have passed
# TEST: ctl4: port_vlan maxgroups: configure below ngroups            [FAIL]
# 	dev veth1 vid 10: Couldn't add MDB entries
# TEST: ctl4: port_vlan maxgroups: isolation of port and per-VLAN ngroups   [FAIL]
# 	Couldn't add 5 entries
# TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
# 	Wrong default mcast_startup_query_interval global vlan option value
# TEST: Ip6InHdrErrors                                                [FAIL]
# TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
# 	Expected to capture 10 packets, got 14.
# TEST: L2 miss - Multicast (IPv4)                                    [FAIL]
# 	Unregistered multicast filter was not hit before adding MDB entry
marvin@defiant:~/linux/kernel/linux_torvalds$

In case you want to pursue these failures, there is the complete test output log
here:

https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-16.log.xz

Thanks again, great work!

Kind regards,
Mirsad
Ido Schimmel July 30, 2023, 7:53 a.m. UTC | #14
On Thu, Jul 27, 2023 at 09:26:03PM +0200, Mirsad Todorovac wrote:
> marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc3-net-forwarding-16.log
> not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1

Other than one test case (see below), I believe this should be fixed by
the patches I just pushed to the existing branch. My earlier fix was
incomplete which is why it didn't solve the problem.

> not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1

Should be fixed with the patches.

> not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1

Nik, the relevant failure is this one:

# TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
# 	Wrong default mcast_startup_query_interval global vlan option value

Any idea why the kernel will report "mcast_startup_query_interval" as
3124 instead of 3125?

# + jq -e '.[].vlans[] | select(.vlan == 10 and                                             .mcast_startup_query_interval == 3125) '
# + echo -n '[{"ifname":"br0","vlans":[{"vlan":1,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000},{"vlan":10,"vlanEnd":11,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000}]}]'
# + check_err 4 'Wrong default mcast_startup_query_interval global vlan option value'

> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1

Please run this one with +x so that we will get more info.

> not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1

Petr, please take a look. Probably need to make the filters more
specific. The failure is:

# TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
# 	Expected to capture 10 packets, got 14.

> not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1

Should be fixed with the patches.

> marvin@defiant:~/linux/kernel/linux_torvalds$ grep -v "^# +" ../kselftest-6.5-rc3-net-forwarding-16.log | grep -A1 FAIL | grep -v -e -- | grep -v OK
> # TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
> # 	"temp" entry has an unpending group timer

Not sure about this one. What is the output with the following diff?

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 8493c3dfc01e..2b2a3b150861 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -628,6 +628,7 @@ __cfg_test_port_ip_sg()
        bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
                grep -q "0.00"
        check_fail $? "\"temp\" entry has an unpending group timer"
+       bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key"
        bridge mdb del dev br0 port $swp1 $grp_key vid 10
 
        # Check error cases.

> # TEST: IPv4 host entries forwarding tests                            [FAIL]
> # 	Packet not locally received after adding a host entry
> # TEST: IPv4 port group "exclude" entries forwarding tests            [FAIL]
> # 	Packet from valid source not received on H2 after adding entry
> # TEST: IPv4 port group "include" entries forwarding tests            [FAIL]
> # 	Packet from valid source not received on H2 after adding entry
> # TEST: IGMPv3 MODE_IS_INCLUDE tests                                  [FAIL]
> # 	Source not add to source list
> # TEST: ctl4: port: ngroups reporting                                 [FAIL]
> # 	Couldn't add MDB entries
> # TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
> # 	Adding 5 MDB entries failed but should have passed
> # TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
> # 	dev veth1: Couldn't add MDB entries
> # TEST: ctl4: port: ngroups reporting                                 [FAIL]
> # 	Couldn't add MDB entries
> # TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
> # 	Adding 5 MDB entries failed but should have passed
> # TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
> # 	dev veth1 vid 10: Couldn't add MDB entries
> # TEST: ctl4: port_vlan: ngroups reporting                            [FAIL]
> # 	Couldn't add MDB entries
> # TEST: ctl4: port_vlan: isolation of port and per-VLAN ngroups       [FAIL]
> # 	Couldn't add MDB entries to VLAN 10
> # TEST: ctl4: port_vlan maxgroups: reporting and treatment of 0       [FAIL]
> # 	Adding 5 MDB entries failed but should have passed
> # TEST: ctl4: port_vlan maxgroups: configure below ngroups            [FAIL]
> # 	dev veth1 vid 10: Couldn't add MDB entries
> # TEST: ctl4: port_vlan maxgroups: isolation of port and per-VLAN ngroups   [FAIL]
> # 	Couldn't add 5 entries
> # TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
> # 	Wrong default mcast_startup_query_interval global vlan option value
> # TEST: Ip6InHdrErrors                                                [FAIL]
> # TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
> # 	Expected to capture 10 packets, got 14.
> # TEST: L2 miss - Multicast (IPv4)                                    [FAIL]
> # 	Unregistered multicast filter was not hit before adding MDB entry
> marvin@defiant:~/linux/kernel/linux_torvalds$
> 
> In case you want to pursue these failures, there is the complete test output log
> here:
> 
> https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-16.log.xz
> 
> Thanks again, great work!
> 
> Kind regards,
> Mirsad
Mirsad Todorovac July 30, 2023, 4:48 p.m. UTC | #15
On 7/30/23 09:53, Ido Schimmel wrote:
> On Thu, Jul 27, 2023 at 09:26:03PM +0200, Mirsad Todorovac wrote:
>> marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc3-net-forwarding-16.log
>> not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
> 
> Other than one test case (see below), I believe this should be fixed by
> the patches I just pushed to the existing branch. My earlier fix was
> incomplete which is why it didn't solve the problem.
> 
>> not ok 5 selftests: net/forwarding: bridge_mdb_max.sh # exit=1
> 
> Should be fixed with the patches.

Congratulations! Indeed, it looks a lot better:

marvin@defiant:~/linux/kernel/linux_torvalds$ grep "not ok" ../kselftest-6.5-rc3-net-forwarding-18.log
not ok 3 selftests: net/forwarding: bridge_mdb.sh # exit=1
not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1
marvin@defiant:~/linux/kernel/linux_torvalds$ grep -v '^# +' ../kselftest-6.5-rc3-net-forwarding-18.log | grep -A1 -e '\[FAIL\]' | grep -v -e -- | grep -v OK
# TEST: IPv4 (S, G) port group entries configuration tests            [FAIL]
# 	Entry has an unpending group timer after replace
# TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
# 	Entry has an unpending group timer after replace
# TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
# 	Wrong default mcast_startup_query_interval global vlan option value
# TEST: Ip6InHdrErrors                                                [FAIL]
# TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
# 	Expected to capture 10 packets, got 15.
# TEST: mirror to ip6gretap: TTL change (skip_hw)                     [FAIL]
# 	Expected to capture 10 packets, got 13.
marvin@defiant:~/linux/kernel/linux_torvalds$

>> not ok 11 selftests: net/forwarding: bridge_vlan_mcast.sh # exit=1
> 
> Nik, the relevant failure is this one:
> 
> # TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
> # 	Wrong default mcast_startup_query_interval global vlan option value
> 
> Any idea why the kernel will report "mcast_startup_query_interval" as
> 3124 instead of 3125?
> 
> # + jq -e '.[].vlans[] | select(.vlan == 10 and                                             .mcast_startup_query_interval == 3125) '
> # + echo -n '[{"ifname":"br0","vlans":[{"vlan":1,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000},{"vlan":10,"vlanEnd":11,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000}]}]'
> # + check_err 4 'Wrong default mcast_startup_query_interval global vlan option value'
> 
>> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
> 
> Please run this one with +x so that we will get more info.

In fact, I have turned it on on all the remaining failing tests.

In case you want to investigate further, please find the debug output log
at the usual place:

https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-18.log.xz

https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/bridge_mdb.sh.out.xz
https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/bridge_vlan_mcast.sh.out.xz
https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/ip6_forward_instats_vrf.sh.out.xz
https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/mirror_gre_changes.sh.out.xz

I hope this helps, because you drastically reduced the number of [FAIL] results.

If it matters being heard from me, I think it's a great job!

Kind regards,
Mirsad

>> not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1
> 
> Petr, please take a look. Probably need to make the filters more
> specific. The failure is:
> 
> # TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
> # 	Expected to capture 10 packets, got 14.
> 
>> not ok 84 selftests: net/forwarding: tc_flower_l2_miss.sh # exit=1
> 
> Should be fixed with the patches.
> 
>> marvin@defiant:~/linux/kernel/linux_torvalds$ grep -v "^# +" ../kselftest-6.5-rc3-net-forwarding-16.log | grep -A1 FAIL | grep -v -e -- | grep -v OK
>> # TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
>> # 	"temp" entry has an unpending group timer
> 
> Not sure about this one. What is the output with the following diff?
> 
> diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> index 8493c3dfc01e..2b2a3b150861 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> @@ -628,6 +628,7 @@ __cfg_test_port_ip_sg()
>          bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
>                  grep -q "0.00"
>          check_fail $? "\"temp\" entry has an unpending group timer"
> +       bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key"
>          bridge mdb del dev br0 port $swp1 $grp_key vid 10
>   
>          # Check error cases.
> 
>> # TEST: IPv4 host entries forwarding tests                            [FAIL]
>> # 	Packet not locally received after adding a host entry
>> # TEST: IPv4 port group "exclude" entries forwarding tests            [FAIL]
>> # 	Packet from valid source not received on H2 after adding entry
>> # TEST: IPv4 port group "include" entries forwarding tests            [FAIL]
>> # 	Packet from valid source not received on H2 after adding entry
>> # TEST: IGMPv3 MODE_IS_INCLUDE tests                                  [FAIL]
>> # 	Source not add to source list
>> # TEST: ctl4: port: ngroups reporting                                 [FAIL]
>> # 	Couldn't add MDB entries
>> # TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
>> # 	Adding 5 MDB entries failed but should have passed
>> # TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
>> # 	dev veth1: Couldn't add MDB entries
>> # TEST: ctl4: port: ngroups reporting                                 [FAIL]
>> # 	Couldn't add MDB entries
>> # TEST: ctl4: port maxgroups: reporting and treatment of 0            [FAIL]
>> # 	Adding 5 MDB entries failed but should have passed
>> # TEST: ctl4: port maxgroups: configure below ngroups                 [FAIL]
>> # 	dev veth1 vid 10: Couldn't add MDB entries
>> # TEST: ctl4: port_vlan: ngroups reporting                            [FAIL]
>> # 	Couldn't add MDB entries
>> # TEST: ctl4: port_vlan: isolation of port and per-VLAN ngroups       [FAIL]
>> # 	Couldn't add MDB entries to VLAN 10
>> # TEST: ctl4: port_vlan maxgroups: reporting and treatment of 0       [FAIL]
>> # 	Adding 5 MDB entries failed but should have passed
>> # TEST: ctl4: port_vlan maxgroups: configure below ngroups            [FAIL]
>> # 	dev veth1 vid 10: Couldn't add MDB entries
>> # TEST: ctl4: port_vlan maxgroups: isolation of port and per-VLAN ngroups   [FAIL]
>> # 	Couldn't add 5 entries
>> # TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
>> # 	Wrong default mcast_startup_query_interval global vlan option value
>> # TEST: Ip6InHdrErrors                                                [FAIL]
>> # TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
>> # 	Expected to capture 10 packets, got 14.
>> # TEST: L2 miss - Multicast (IPv4)                                    [FAIL]
>> # 	Unregistered multicast filter was not hit before adding MDB entry
>> marvin@defiant:~/linux/kernel/linux_torvalds$
>>
>> In case you want to pursue these failures, there is the complete test output log
>> here:
>>
>> https://domac.alu.unizg.hr/~mtodorov/linux/selftests/net-forwarding/kselftest-6.5-rc3-net-forwarding-16.log.xz
>>
>> Thanks again, great work!
>>
>> Kind regards,
>> Mirsad
Ido Schimmel July 31, 2023, 7:54 a.m. UTC | #16
Thanks for testing.

On Sun, Jul 30, 2023 at 06:48:04PM +0200, Mirsad Todorovac wrote:
> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1

Regarding this one, in the log I don't see the require_command() that I
added in "selftests: forwarding: Set default IPv6 traceroute utility".
Also, at line 470 I see "ip vrf exec vveth0 2001:1:2::2" which is
another indication that you don't have the patch.
Mirsad Todorovac July 31, 2023, 9:24 a.m. UTC | #17
On 7/31/23 09:54, Ido Schimmel wrote:
> Thanks for testing.

Not at all.

> On Sun, Jul 30, 2023 at 06:48:04PM +0200, Mirsad Todorovac wrote:
>> not ok 26 selftests: net/forwarding: ip6_forward_instats_vrf.sh # exit=1
> 
> Regarding this one, in the log I don't see the require_command() that I
> added in "selftests: forwarding: Set default IPv6 traceroute utility".
> Also, at line 470 I see "ip vrf exec vveth0 2001:1:2::2" which is
> another indication that you don't have the patch.

This is correct.

Now I have:

root@defiant:tools/testing/selftests/net/forwarding# ./ip6_forward_instats_vrf.sh
SKIP: traceroute6 not installed

Mystery solved. This is much more useful output :-)

Installed traceroute6 and now the test is OK:

root@defiant:tools/testing/selftests/net/forwarding# ./ip6_forward_instats_vrf.sh
TEST: ping6                                                         [ OK ]
TEST: Ip6InTooBigErrors                                             [ OK ]
TEST: Ip6InHdrErrors                                                [ OK ]
TEST: Ip6InAddrErrors                                               [ OK ]
TEST: Ip6InDiscards                                                 [ OK ]
root@defiant:tools/testing/selftests/net/forwarding#

I guess that means only three are left.

# ./bridge_mdb.sh
dev br0 port veth1 grp 239.1.1.1 src 192.0.2.1 temp filter_mode include proto static vid 10  259.99
TEST: IPv4 (S, G) port group entries configuration tests            [FAIL]
	Entry has an unpending group timer after replace
dev br0 port veth1 grp ff0e::1 src 2001:db8:1::1 temp filter_mode include proto static vid 10  259.99
TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
	Entry has an unpending group timer after replace
# ./bridge_vlan_mcast.sh
TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
	Wrong default mcast_startup_query_interval global vlan option value
# ./mirror_gre_changes.sh
TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
	Expected to capture 10 packets, got 15.
TEST: mirror to ip6gretap: TTL change (skip_hw)                     [FAIL]
	Expected to capture 10 packets, got 13.
WARN: Could not test offloaded functionality
#

NOTE: The error happened because two patches collided. This patch

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 975fc5168c6334..40a8c1541b7f81 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -30,6 +30,7 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
  REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
  STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
  TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
+TROUTE6=${TROUTE6:=traceroute6}
  
  relative_path="${BASH_SOURCE%/*}"
  if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then

and this patch

diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
index 71f7c0c49677..5b0183013017 100755
--- a/tools/testing/selftests/net/forwarding/lib.sh
+++ b/tools/testing/selftests/net/forwarding/lib.sh
@@ -16,8 +16,6 @@ TEAMD=${TEAMD:=teamd}
  WAIT_TIME=${WAIT_TIME:=5}
  PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
  PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
-NETIF_TYPE=${NETIF_TYPE:=veth}
-NETIF_CREATE=${NETIF_CREATE:=yes}
  MCD=${MCD:=smcrouted}
  MC_CLI=${MC_CLI:=smcroutectl}
  PING_COUNT=${PING_COUNT:=10}
@@ -30,6 +28,20 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
  REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
  STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
  TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
+NETIF_TYPE=${NETIF_TYPE:=veth}
+NETIF_CREATE=${NETIF_CREATE:=yes}
+declare -A NETIFS=(
+       [p1]=veth0
+       [p2]=veth1
+       [p3]=veth2
+       [p4]=veth3
+       [p5]=veth4
+       [p6]=veth5
+       [p7]=veth6
+       [p8]=veth7
+       [p9]=veth8
+       [p10]=veth9
+)

  relative_path="${BASH_SOURCE%/*}"
  if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then

are not compatible.

I have applied the 'require_command $TROUTE6' patch manually.

I suppose this is what you intended to have:

# Can be overridden by the configuration file.
PING=${PING:=ping}
PING6=${PING6:=ping6}
MZ=${MZ:=mausezahn}
ARPING=${ARPING:=arping}
TEAMD=${TEAMD:=teamd}
WAIT_TIME=${WAIT_TIME:=5}
PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
MCD=${MCD:=smcrouted}
MC_CLI=${MC_CLI:=smcroutectl}
PING_COUNT=${PING_COUNT:=10}
PING_TIMEOUT=${PING_TIMEOUT:=5}
WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
REQUIRE_JQ=${REQUIRE_JQ:=yes}
REQUIRE_MZ=${REQUIRE_MZ:=yes}
REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
TROUTE6=${TROUTE6:=traceroute6}
NETIF_TYPE=${NETIF_TYPE:=veth}
NETIF_CREATE=${NETIF_CREATE:=yes}
declare -A NETIFS=(
        [p1]=veth0
        [p2]=veth1
        [p3]=veth2
        [p4]=veth3
        [p5]=veth4
        [p6]=veth5
        [p7]=veth6
        [p8]=veth7
        [p9]=veth8
        [p10]=veth9
)

relative_path="${BASH_SOURCE%/*}"
if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
         relative_path="."
fi
------------------------------------------------

Probably for the production patch you would like to have this fixed.

Have a nice day.

Kind regards,
Mirsad
Ido Schimmel July 31, 2023, 12:02 p.m. UTC | #18
On Mon, Jul 31, 2023 at 11:24:27AM +0200, Mirsad Todorovac wrote:
> I guess that means only three are left.
> 
> # ./bridge_mdb.sh
> dev br0 port veth1 grp 239.1.1.1 src 192.0.2.1 temp filter_mode include proto static vid 10  259.99
> TEST: IPv4 (S, G) port group entries configuration tests            [FAIL]
> 	Entry has an unpending group timer after replace
> dev br0 port veth1 grp ff0e::1 src 2001:db8:1::1 temp filter_mode include proto static vid 10  259.99
> TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
> 	Entry has an unpending group timer after replace

I suspect that what happens here is that you have a faster system
than me or a different HZ value (check CONFIG_HZ, mine is 1000). The
group membership time is probably 260.00 which is why grepping for
"0.00" works when it shouldn't. Can you try the patch below? No need to
run all the other tests.

diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
index 8493c3dfc01e..41c33a2de0a6 100755
--- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
+++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
@@ -617,7 +617,7 @@ __cfg_test_port_ip_sg()
                grep -q "permanent"
        check_err $? "Entry not added as \"permanent\" when should"
        bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
-               grep -q "0.00"
+               grep -q " 0.00"
        check_err $? "\"permanent\" entry has a pending group timer"
        bridge mdb del dev br0 port $swp1 $grp_key vid 10
 
@@ -626,7 +626,7 @@ __cfg_test_port_ip_sg()
                grep -q "temp"
        check_err $? "Entry not added as \"temp\" when should"
        bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
-               grep -q "0.00"
+               grep -q " 0.00"
        check_fail $? "\"temp\" entry has an unpending group timer"
        bridge mdb del dev br0 port $swp1 $grp_key vid 10
 
@@ -659,7 +659,7 @@ __cfg_test_port_ip_sg()
                grep -q "permanent"
        check_err $? "Entry not marked as \"permanent\" after replace"
        bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
-               grep -q "0.00"
+               grep -q " 0.00"
        check_err $? "Entry has a pending group timer after replace"
 
        bridge mdb replace dev br0 port $swp1 $grp_key vid 10 temp
@@ -667,7 +667,7 @@ __cfg_test_port_ip_sg()
                grep -q "temp"
        check_err $? "Entry not marked as \"temp\" after replace"
        bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
-               grep -q "0.00"
+               grep -q " 0.00"
        check_fail $? "Entry has an unpending group timer after replace"
        bridge mdb del dev br0 port $swp1 $grp_key vid 10

> # ./bridge_vlan_mcast.sh
> TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
> 	Wrong default mcast_startup_query_interval global vlan option value
> # ./mirror_gre_changes.sh
> TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
> 	Expected to capture 10 packets, got 15.
> TEST: mirror to ip6gretap: TTL change (skip_hw)                     [FAIL]
> 	Expected to capture 10 packets, got 13.
> WARN: Could not test offloaded functionality

I hope Nik and Petr will find the time to look into those. If not, I
will check when I can.

> 
> NOTE: The error happened because two patches collided. This patch
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 975fc5168c6334..40a8c1541b7f81 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -30,6 +30,7 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>  REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>  STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>  TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
> +TROUTE6=${TROUTE6:=traceroute6}
>  relative_path="${BASH_SOURCE%/*}"
>  if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
> 
> and this patch
> 
> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
> index 71f7c0c49677..5b0183013017 100755
> --- a/tools/testing/selftests/net/forwarding/lib.sh
> +++ b/tools/testing/selftests/net/forwarding/lib.sh
> @@ -16,8 +16,6 @@ TEAMD=${TEAMD:=teamd}
>  WAIT_TIME=${WAIT_TIME:=5}
>  PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
>  PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
> -NETIF_TYPE=${NETIF_TYPE:=veth}
> -NETIF_CREATE=${NETIF_CREATE:=yes}
>  MCD=${MCD:=smcrouted}
>  MC_CLI=${MC_CLI:=smcroutectl}
>  PING_COUNT=${PING_COUNT:=10}
> @@ -30,6 +28,20 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>  REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>  STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>  TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
> +NETIF_TYPE=${NETIF_TYPE:=veth}
> +NETIF_CREATE=${NETIF_CREATE:=yes}
> +declare -A NETIFS=(
> +       [p1]=veth0
> +       [p2]=veth1
> +       [p3]=veth2
> +       [p4]=veth3
> +       [p5]=veth4
> +       [p6]=veth5
> +       [p7]=veth6
> +       [p8]=veth7
> +       [p9]=veth8
> +       [p10]=veth9
> +)
> 
>  relative_path="${BASH_SOURCE%/*}"
>  if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
> 
> are not compatible.
> 
> I have applied the 'require_command $TROUTE6' patch manually.
> 
> I suppose this is what you intended to have:
> 
> # Can be overridden by the configuration file.
> PING=${PING:=ping}
> PING6=${PING6:=ping6}
> MZ=${MZ:=mausezahn}
> ARPING=${ARPING:=arping}
> TEAMD=${TEAMD:=teamd}
> WAIT_TIME=${WAIT_TIME:=5}
> PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
> PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
> MCD=${MCD:=smcrouted}
> MC_CLI=${MC_CLI:=smcroutectl}
> PING_COUNT=${PING_COUNT:=10}
> PING_TIMEOUT=${PING_TIMEOUT:=5}
> WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
> INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
> LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
> REQUIRE_JQ=${REQUIRE_JQ:=yes}
> REQUIRE_MZ=${REQUIRE_MZ:=yes}
> REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
> STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
> TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
> TROUTE6=${TROUTE6:=traceroute6}
> NETIF_TYPE=${NETIF_TYPE:=veth}
> NETIF_CREATE=${NETIF_CREATE:=yes}
> declare -A NETIFS=(
>        [p1]=veth0
>        [p2]=veth1
>        [p3]=veth2
>        [p4]=veth3
>        [p5]=veth4
>        [p6]=veth5
>        [p7]=veth6
>        [p8]=veth7
>        [p9]=veth8
>        [p10]=veth9
> )
> 
> relative_path="${BASH_SOURCE%/*}"
> if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>         relative_path="."
> fi
> ------------------------------------------------
> 
> Probably for the production patch you would like to have this fixed.

No, I don't intend to submit the patch that automatically creates the
veth pairs. It is superseded by "selftests: forwarding: Skip test when
no interfaces are specified".
Mirsad Todorovac July 31, 2023, 3:13 p.m. UTC | #19
On 7/31/23 14:02, Ido Schimmel wrote:
> On Mon, Jul 31, 2023 at 11:24:27AM +0200, Mirsad Todorovac wrote:
>> I guess that means only three are left.
>>
>> # ./bridge_mdb.sh
>> dev br0 port veth1 grp 239.1.1.1 src 192.0.2.1 temp filter_mode include proto static vid 10  259.99
>> TEST: IPv4 (S, G) port group entries configuration tests            [FAIL]
>> 	Entry has an unpending group timer after replace
>> dev br0 port veth1 grp ff0e::1 src 2001:db8:1::1 temp filter_mode include proto static vid 10  259.99
>> TEST: IPv6 (S, G) port group entries configuration tests            [FAIL]
>> 	Entry has an unpending group timer after replace
> 
> I suspect that what happens here is that you have a faster system
> than me or a different HZ value (check CONFIG_HZ, mine is 1000). The
> group membership time is probably 260.00 which is why grepping for
> "0.00" works when it shouldn't. Can you try the patch below? No need to
> run all the other tests.
> 
> diff --git a/tools/testing/selftests/net/forwarding/bridge_mdb.sh b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> index 8493c3dfc01e..41c33a2de0a6 100755
> --- a/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> +++ b/tools/testing/selftests/net/forwarding/bridge_mdb.sh
> @@ -617,7 +617,7 @@ __cfg_test_port_ip_sg()
>                  grep -q "permanent"
>          check_err $? "Entry not added as \"permanent\" when should"
>          bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
> -               grep -q "0.00"
> +               grep -q " 0.00"
>          check_err $? "\"permanent\" entry has a pending group timer"
>          bridge mdb del dev br0 port $swp1 $grp_key vid 10
>   
> @@ -626,7 +626,7 @@ __cfg_test_port_ip_sg()
>                  grep -q "temp"
>          check_err $? "Entry not added as \"temp\" when should"
>          bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
> -               grep -q "0.00"
> +               grep -q " 0.00"
>          check_fail $? "\"temp\" entry has an unpending group timer"
>          bridge mdb del dev br0 port $swp1 $grp_key vid 10
>   
> @@ -659,7 +659,7 @@ __cfg_test_port_ip_sg()
>                  grep -q "permanent"
>          check_err $? "Entry not marked as \"permanent\" after replace"
>          bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
> -               grep -q "0.00"
> +               grep -q " 0.00"
>          check_err $? "Entry has a pending group timer after replace"
>   
>          bridge mdb replace dev br0 port $swp1 $grp_key vid 10 temp
> @@ -667,7 +667,7 @@ __cfg_test_port_ip_sg()
>                  grep -q "temp"
>          check_err $? "Entry not marked as \"temp\" after replace"
>          bridge -d -s mdb show dev br0 vid 10 | grep "$grp_key" | \
> -               grep -q "0.00"
> +               grep -q " 0.00"
>          check_fail $? "Entry has an unpending group timer after replace"
>          bridge mdb del dev br0 port $swp1 $grp_key vid 10

Congrats! This worked:

root@defiant:tools/testing/selftests/net/forwarding# ./bridge_mdb.sh

INFO: # Host entries configuration tests
TEST: Common host entries configuration tests (IPv4)                [ OK ]
TEST: Common host entries configuration tests (IPv6)                [ OK ]
TEST: Common host entries configuration tests (L2)                  [ OK ]

INFO: # Port group entries configuration tests - (*, G)
TEST: Common port group entries configuration tests (IPv4 (*, G))   [ OK ]
TEST: Common port group entries configuration tests (IPv6 (*, G))   [ OK ]
TEST: IPv4 (*, G) port group entries configuration tests            [ OK ]
TEST: IPv6 (*, G) port group entries configuration tests            [ OK ]

INFO: # Port group entries configuration tests - (S, G)
TEST: Common port group entries configuration tests (IPv4 (S, G))   [ OK ]
TEST: Common port group entries configuration tests (IPv6 (S, G))   [ OK ]
dev br0 port veth1 grp 239.1.1.1 src 192.0.2.1 temp filter_mode include proto static vid 10  259.99
TEST: IPv4 (S, G) port group entries configuration tests            [ OK ]
dev br0 port veth1 grp ff0e::1 src 2001:db8:1::1 temp filter_mode include proto static vid 10  259.99
TEST: IPv6 (S, G) port group entries configuration tests            [ OK ]

INFO: # Port group entries configuration tests - L2
TEST: Common port group entries configuration tests (L2 (*, G))     [ OK ]
TEST: L2 (*, G) port group entries configuration tests              [ OK ]

INFO: # Large scale dump tests
TEST: IPv4 large scale dump tests                                   [ OK ]
TEST: IPv6 large scale dump tests                                   [ OK ]
TEST: L2 large scale dump tests                                     [ OK ]

INFO: # Forwarding tests
TEST: IPv4 host entries forwarding tests                            [ OK ]
TEST: IPv6 host entries forwarding tests                            [ OK ]
TEST: L2 host entries forwarding tests                              [ OK ]
TEST: IPv4 port group "exclude" entries forwarding tests            [ OK ]
TEST: IPv6 port group "exclude" entries forwarding tests            [ OK ]
TEST: IPv4 port group "include" entries forwarding tests            [ OK ]
TEST: IPv6 port group "include" entries forwarding tests            [ OK ]
TEST: L2 port entries forwarding tests                              [ OK ]

INFO: # Control packets tests
TEST: IGMPv3 MODE_IS_INCLUDE tests                                  [ OK ]
TEST: MLDv2 MODE_IS_INCLUDE tests                                   [ OK ]
root@defiant:tools/testing/selftests/net/forwarding#

>> # ./bridge_vlan_mcast.sh
>> TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
>> 	Wrong default mcast_startup_query_interval global vlan option value
>> # ./mirror_gre_changes.sh
>> TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
>> 	Expected to capture 10 packets, got 15.
>> TEST: mirror to ip6gretap: TTL change (skip_hw)                     [FAIL]
>> 	Expected to capture 10 packets, got 13.
>> WARN: Could not test offloaded functionality
> 
> I hope Nik and Petr will find the time to look into those. If not, I
> will check when I can.

Great. I will wait for the follow-up then ...

You can add:

Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>

at your convenience. I think that it appropriate with the Code of Conduct.

Kind regards,
Mirsad


>> NOTE: The error happened because two patches collided. This patch
>>
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 975fc5168c6334..40a8c1541b7f81 100755
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -30,6 +30,7 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>>   REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>>   STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>>   TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> +TROUTE6=${TROUTE6:=traceroute6}
>>   relative_path="${BASH_SOURCE%/*}"
>>   if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>
>> and this patch
>>
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 71f7c0c49677..5b0183013017 100755
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -16,8 +16,6 @@ TEAMD=${TEAMD:=teamd}
>>   WAIT_TIME=${WAIT_TIME:=5}
>>   PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
>>   PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
>> -NETIF_TYPE=${NETIF_TYPE:=veth}
>> -NETIF_CREATE=${NETIF_CREATE:=yes}
>>   MCD=${MCD:=smcrouted}
>>   MC_CLI=${MC_CLI:=smcroutectl}
>>   PING_COUNT=${PING_COUNT:=10}
>> @@ -30,6 +28,20 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>>   REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>>   STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>>   TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> +NETIF_TYPE=${NETIF_TYPE:=veth}
>> +NETIF_CREATE=${NETIF_CREATE:=yes}
>> +declare -A NETIFS=(
>> +       [p1]=veth0
>> +       [p2]=veth1
>> +       [p3]=veth2
>> +       [p4]=veth3
>> +       [p5]=veth4
>> +       [p6]=veth5
>> +       [p7]=veth6
>> +       [p8]=veth7
>> +       [p9]=veth8
>> +       [p10]=veth9
>> +)
>>
>>   relative_path="${BASH_SOURCE%/*}"
>>   if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>
>> are not compatible.
>>
>> I have applied the 'require_command $TROUTE6' patch manually.
>>
>> I suppose this is what you intended to have:
>>
>> # Can be overridden by the configuration file.
>> PING=${PING:=ping}
>> PING6=${PING6:=ping6}
>> MZ=${MZ:=mausezahn}
>> ARPING=${ARPING:=arping}
>> TEAMD=${TEAMD:=teamd}
>> WAIT_TIME=${WAIT_TIME:=5}
>> PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
>> PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
>> MCD=${MCD:=smcrouted}
>> MC_CLI=${MC_CLI:=smcroutectl}
>> PING_COUNT=${PING_COUNT:=10}
>> PING_TIMEOUT=${PING_TIMEOUT:=5}
>> WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
>> INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
>> LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
>> REQUIRE_JQ=${REQUIRE_JQ:=yes}
>> REQUIRE_MZ=${REQUIRE_MZ:=yes}
>> REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>> STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>> TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> TROUTE6=${TROUTE6:=traceroute6}
>> NETIF_TYPE=${NETIF_TYPE:=veth}
>> NETIF_CREATE=${NETIF_CREATE:=yes}
>> declare -A NETIFS=(
>>         [p1]=veth0
>>         [p2]=veth1
>>         [p3]=veth2
>>         [p4]=veth3
>>         [p5]=veth4
>>         [p6]=veth5
>>         [p7]=veth6
>>         [p8]=veth7
>>         [p9]=veth8
>>         [p10]=veth9
>> )
>>
>> relative_path="${BASH_SOURCE%/*}"
>> if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>          relative_path="."
>> fi
>> ------------------------------------------------
>>
>> Probably for the production patch you would like to have this fixed.
> 
> No, I don't intend to submit the patch that automatically creates the
> veth pairs. It is superseded by "selftests: forwarding: Skip test when
> no interfaces are specified".
Mirsad Todorovac July 31, 2023, 3:23 p.m. UTC | #20
On 7/31/23 14:02, Ido Schimmel wrote:

>> NOTE: The error happened because two patches collided. This patch
>>
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 975fc5168c6334..40a8c1541b7f81 100755
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -30,6 +30,7 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>>   REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>>   STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>>   TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> +TROUTE6=${TROUTE6:=traceroute6}
>>   relative_path="${BASH_SOURCE%/*}"
>>   if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>
>> and this patch
>>
>> diff --git a/tools/testing/selftests/net/forwarding/lib.sh b/tools/testing/selftests/net/forwarding/lib.sh
>> index 71f7c0c49677..5b0183013017 100755
>> --- a/tools/testing/selftests/net/forwarding/lib.sh
>> +++ b/tools/testing/selftests/net/forwarding/lib.sh
>> @@ -16,8 +16,6 @@ TEAMD=${TEAMD:=teamd}
>>   WAIT_TIME=${WAIT_TIME:=5}
>>   PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
>>   PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
>> -NETIF_TYPE=${NETIF_TYPE:=veth}
>> -NETIF_CREATE=${NETIF_CREATE:=yes}
>>   MCD=${MCD:=smcrouted}
>>   MC_CLI=${MC_CLI:=smcroutectl}
>>   PING_COUNT=${PING_COUNT:=10}
>> @@ -30,6 +28,20 @@ REQUIRE_MZ=${REQUIRE_MZ:=yes}
>>   REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>>   STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>>   TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> +NETIF_TYPE=${NETIF_TYPE:=veth}
>> +NETIF_CREATE=${NETIF_CREATE:=yes}
>> +declare -A NETIFS=(
>> +       [p1]=veth0
>> +       [p2]=veth1
>> +       [p3]=veth2
>> +       [p4]=veth3
>> +       [p5]=veth4
>> +       [p6]=veth5
>> +       [p7]=veth6
>> +       [p8]=veth7
>> +       [p9]=veth8
>> +       [p10]=veth9
>> +)
>>
>>   relative_path="${BASH_SOURCE%/*}"
>>   if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>
>> are not compatible.
>>
>> I have applied the 'require_command $TROUTE6' patch manually.
>>
>> I suppose this is what you intended to have:
>>
>> # Can be overridden by the configuration file.
>> PING=${PING:=ping}
>> PING6=${PING6:=ping6}
>> MZ=${MZ:=mausezahn}
>> ARPING=${ARPING:=arping}
>> TEAMD=${TEAMD:=teamd}
>> WAIT_TIME=${WAIT_TIME:=5}
>> PAUSE_ON_FAIL=${PAUSE_ON_FAIL:=no}
>> PAUSE_ON_CLEANUP=${PAUSE_ON_CLEANUP:=no}
>> MCD=${MCD:=smcrouted}
>> MC_CLI=${MC_CLI:=smcroutectl}
>> PING_COUNT=${PING_COUNT:=10}
>> PING_TIMEOUT=${PING_TIMEOUT:=5}
>> WAIT_TIMEOUT=${WAIT_TIMEOUT:=20}
>> INTERFACE_TIMEOUT=${INTERFACE_TIMEOUT:=600}
>> LOW_AGEING_TIME=${LOW_AGEING_TIME:=1000}
>> REQUIRE_JQ=${REQUIRE_JQ:=yes}
>> REQUIRE_MZ=${REQUIRE_MZ:=yes}
>> REQUIRE_MTOOLS=${REQUIRE_MTOOLS:=no}
>> STABLE_MAC_ADDRS=${STABLE_MAC_ADDRS:=no}
>> TCPDUMP_EXTRA_FLAGS=${TCPDUMP_EXTRA_FLAGS:=}
>> TROUTE6=${TROUTE6:=traceroute6}
>> NETIF_TYPE=${NETIF_TYPE:=veth}
>> NETIF_CREATE=${NETIF_CREATE:=yes}
>> declare -A NETIFS=(
>>         [p1]=veth0
>>         [p2]=veth1
>>         [p3]=veth2
>>         [p4]=veth3
>>         [p5]=veth4
>>         [p6]=veth5
>>         [p7]=veth6
>>         [p8]=veth7
>>         [p9]=veth8
>>         [p10]=veth9
>> )
>>
>> relative_path="${BASH_SOURCE%/*}"
>> if [[ "$relative_path" == "${BASH_SOURCE}" ]]; then
>>          relative_path="."
>> fi
>> ------------------------------------------------
>>
>> Probably for the production patch you would like to have this fixed.
> 
> No, I don't intend to submit the patch that automatically creates the
> veth pairs. It is superseded by "selftests: forwarding: Skip test when
> no interfaces are specified".

It is your call, but consider that the majority of testers will use the default setup
and maybe grep "not ok" messages in the log, because the amount of logs is overwhelming.

Knowing that there is "forwarding.config.sample" probably requires in-depth knowledge
of the selftest net/forwarding bundle and maybe direct hint from the developers?

Kind regards,
Mirsad
Ido Schimmel July 31, 2023, 3:48 p.m. UTC | #21
On Mon, Jul 31, 2023 at 05:13:37PM +0200, Mirsad Todorovac wrote:
> You can add:
> 
> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>

Added your tags to all 17 patches. Available here:
https://github.com/idosch/linux/tree/submit/selftests_fix_v2

Will submit later this week (most likely on Wednesday) after I verify
they don't cause other regressions.

Thanks for testing and reporting.
Petr Machata Aug. 1, 2023, 11:08 a.m. UTC | #22
Ido Schimmel <idosch@idosch.org> writes:

> On Thu, Jul 27, 2023 at 09:26:03PM +0200, Mirsad Todorovac wrote:
>> not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1
>
> Petr, please take a look. Probably need to make the filters more
> specific. The failure is:
>
> # TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
> # 	Expected to capture 10 packets, got 14.

Yeah, this reproduces easily on my laptop. The switches are somehow much
more quiet and do not really hit the issue.

Mirsad, can you please try this patch? It fixes the issue for me.

From 77461c209eb0067dca7fdf4431a907b2a1ce8c6a Mon Sep 17 00:00:00 2001
Message-ID: <77461c209eb0067dca7fdf4431a907b2a1ce8c6a.1690887929.git.petrm@nvidia.com>
From: Petr Machata <petrm@nvidia.com>
Date: Tue, 1 Aug 2023 12:57:53 +0200
Subject: [PATCH net-next] selftests: mirror_gre_changes: Tighten up the TTL
 test match
To: <netdev@vger.kernel.org>

This test verifies whether the encapsulated packets have the correct
configured TTL. It does so by sending ICMP packets through the test
topology and mirroring them to a gretap netdevice. On a busy host
however, more than just the test ICMP packets may end up flowing
through the topology, get mirrored, and counted. This leads to
potential spurious failures as the test observes much more mirrored
packets than the sent test packets, and assumes a bug.

Fix this by tightening up the mirror action match. Change it from
matchall to a flower classifier matching on ICMP packets specifically.

Signed-off-by: Petr Machata <petrm@nvidia.com>
---
 tools/testing/selftests/net/forwarding/mirror_gre_changes.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
index aff88f78e339..5ea9d63915f7 100755
--- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
+++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
@@ -72,7 +72,8 @@ test_span_gre_ttl()
 
 	RET=0
 
-	mirror_install $swp1 ingress $tundev "matchall $tcflags"
+	mirror_install $swp1 ingress $tundev \
+		"prot ip flower $tcflags ip_prot icmp"
 	tc filter add dev $h3 ingress pref 77 prot $prot \
 		flower skip_hw ip_ttl 50 action pass
Mirsad Todorovac Aug. 1, 2023, 4:54 p.m. UTC | #23
On 8/1/23 13:08, Petr Machata wrote:
> 
> Ido Schimmel <idosch@idosch.org> writes:
> 
>> On Thu, Jul 27, 2023 at 09:26:03PM +0200, Mirsad Todorovac wrote:
>>> not ok 49 selftests: net/forwarding: mirror_gre_changes.sh # exit=1
>>
>> Petr, please take a look. Probably need to make the filters more
>> specific. The failure is:
>>
>> # TEST: mirror to gretap: TTL change (skip_hw)                        [FAIL]
>> # 	Expected to capture 10 packets, got 14.
> 
> Yeah, this reproduces easily on my laptop. The switches are somehow much
> more quiet and do not really hit the issue.
> 
> Mirsad, can you please try this patch? It fixes the issue for me.
> 
>>From 77461c209eb0067dca7fdf4431a907b2a1ce8c6a Mon Sep 17 00:00:00 2001
> Message-ID: <77461c209eb0067dca7fdf4431a907b2a1ce8c6a.1690887929.git.petrm@nvidia.com>
> From: Petr Machata <petrm@nvidia.com>
> Date: Tue, 1 Aug 2023 12:57:53 +0200
> Subject: [PATCH net-next] selftests: mirror_gre_changes: Tighten up the TTL
>   test match
> To: <netdev@vger.kernel.org>
> 
> This test verifies whether the encapsulated packets have the correct
> configured TTL. It does so by sending ICMP packets through the test
> topology and mirroring them to a gretap netdevice. On a busy host
> however, more than just the test ICMP packets may end up flowing
> through the topology, get mirrored, and counted. This leads to
> potential spurious failures as the test observes much more mirrored
> packets than the sent test packets, and assumes a bug.
> 
> Fix this by tightening up the mirror action match. Change it from
> matchall to a flower classifier matching on ICMP packets specifically.
> 
> Signed-off-by: Petr Machata <petrm@nvidia.com>
> ---
>   tools/testing/selftests/net/forwarding/mirror_gre_changes.sh | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> index aff88f78e339..5ea9d63915f7 100755
> --- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
> @@ -72,7 +72,8 @@ test_span_gre_ttl()
>   
>   	RET=0
>   
> -	mirror_install $swp1 ingress $tundev "matchall $tcflags"
> +	mirror_install $swp1 ingress $tundev \
> +		"prot ip flower $tcflags ip_prot icmp"
>   	tc filter add dev $h3 ingress pref 77 prot $prot \
>   		flower skip_hw ip_ttl 50 action pass
>   

The problem seems to be fixed in the test run:

root@defiant:tools/testing/selftests/net/forwarding# ./mirror_gre_changes.sh
TEST: mirror to gretap: TTL change (skip_hw)                        [ OK ]
TEST: mirror to ip6gretap: TTL change (skip_hw)                     [ OK ]
TEST: mirror to gretap: tunnel down/up (skip_hw)                    [ OK ]
TEST: mirror to ip6gretap: tunnel down/up (skip_hw)                 [ OK ]
TEST: mirror to gretap: egress down/up (skip_hw)                    [ OK ]
TEST: mirror to ip6gretap: egress down/up (skip_hw)                 [ OK ]
TEST: mirror to gretap: remote address change (skip_hw)             [ OK ]
TEST: mirror to ip6gretap: remote address change (skip_hw)          [ OK ]
TEST: mirror to gretap: tunnel deleted (skip_hw)                    [ OK ]
TEST: mirror to ip6gretap: tunnel deleted (skip_hw)                 [ OK ]
TEST: mirror to gretap: underlay route removal (skip_hw)            [ OK ]
TEST: mirror to ip6gretap: underlay route removal (skip_hw)         [ OK ]
WARN: Could not test offloaded functionality
root@defiant:tools/testing/selftests/net/forwarding#

That leaves us with one more tests to fix:./bridge_vlan_mcast.sh

# ./bridge_vlan_mcast.sh

TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
         Wrong default mcast_startup_query_interval global vlan option value


[...]

+ bridge vlan global set vid 10 dev br0 mcast_snooping 1 mcast_last_member_interval 100
+ for current_test in ${TESTS:-$ALL_TESTS}
+ vlmc_startup_query_test
+ RET=0
++ bridge -j vlan global show
+ local 'goutput=[{"ifname":"br0","vlans":[{"vlan":1,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_
interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000},{"vlan":10,"vlanEnd":11,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_versi
on":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_respons
e_interval":1000}]}]'
+ jq -e '.[].vlans[] | select(.vlan == 10)'
+ echo -n '[{"ifname":"br0","vlans":[{"vlan":1,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interv
al":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000},{"vlan":10,"vlanEnd":11,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,
"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_inte
rval":1000}]}]'
+ check_err 0 'Could not find vlan 10'\''s global options'
+ local err=0
+ local 'msg=Could not find vlan 10'\''s global options'
+ [[ 0 -eq 0 ]]
+ [[ 0 -ne 0 ]]
+ jq -e '.[].vlans[] | select(.vlan == 10 and                                       .mcast_startup_query_interval == 3125) '
+ echo -n '[{"ifname":"br0","vlans":[{"vlan":1,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interv
al":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_interval":1000},{"vlan":10,"vlanEnd":11,"mcast_snooping":1,"mcast_querier":0,"mcast_igmp_version":2,"mcast_mld_version":1,
"mcast_last_member_count":2,"mcast_last_member_interval":100,"mcast_startup_query_count":2,"mcast_startup_query_interval":3124,"mcast_membership_interval":26000,"mcast_querier_interval":25500,"mcast_query_interval":12500,"mcast_query_response_inte
rval":1000}]}]'
+ check_err 4 'Wrong default mcast_startup_query_interval global vlan option value'
+ local err=4
+ local 'msg=Wrong default mcast_startup_query_interval global vlan option value'
+ [[ 0 -eq 0 ]]
+ [[ 4 -ne 0 ]]
+ RET=4
+ retmsg='Wrong default mcast_startup_query_interval global vlan option value'
+ log_test 'Vlan mcast_startup_query_interval global option default value'
+ local 'test_name=Vlan mcast_startup_query_interval global option default value'
+ local opt_str=
+ [[ 1 -eq 2 ]]
+ [[ 4 -ne 0 ]]
+ EXIT_STATUS=1
+ printf 'TEST: %-60s  [FAIL]\n' 'Vlan mcast_startup_query_interval global option default value '
TEST: Vlan mcast_startup_query_interval global option default value   [FAIL]
+ [[ ! -z Wrong default mcast_startup_query_interval global vlan option value ]]
+ printf '\t%s\n' 'Wrong default mcast_startup_query_interval global vlan option value'
         Wrong default mcast_startup_query_interval global vlan option value

[...]

Hope this helps.

Kind regards,
Mirsad
Mirsad Todorovac Aug. 1, 2023, 8:41 p.m. UTC | #24
On 7/31/23 17:48, Ido Schimmel wrote:
> On Mon, Jul 31, 2023 at 05:13:37PM +0200, Mirsad Todorovac wrote:
>> You can add:
>>
>> Tested-by: Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr>
> 
> Added your tags to all 17 patches. Available here:
> https://github.com/idosch/linux/tree/submit/selftests_fix_v2

Yes, thanks!

It is good to be known that I am available for testing patches on the available
equipment.

> Will submit later this week (most likely on Wednesday) after I verify
> they don't cause other regressions.
> 
> Thanks for testing and reporting.

Not at all.

I am most pleased that we nailed the errors and fails.

It was a great exercise for my little grey cells in a great environment of quality
professionals.

Much obliged.

Still, maybe you should add to README about the forwarding.config.sample opportunity?

I think what we all want as the community is greater test coverage, to discover the most
bugs on the most platforms ...

Kind regards,
Mirsad Todorovac
Petr Machata Aug. 2, 2023, 10:33 a.m. UTC | #25
Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> writes:

> On 8/1/23 13:08, Petr Machata wrote:
>> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>> b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>> index aff88f78e339..5ea9d63915f7 100755
>> --- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>> @@ -72,7 +72,8 @@ test_span_gre_ttl()
>>     	RET=0
>>   -	mirror_install $swp1 ingress $tundev "matchall $tcflags"
>> +	mirror_install $swp1 ingress $tundev \
>> +		"prot ip flower $tcflags ip_prot icmp"
>>   	tc filter add dev $h3 ingress pref 77 prot $prot \
>>   		flower skip_hw ip_ttl 50 action pass
>>   
>
> The problem seems to be fixed in the test run:
>
> root@defiant:tools/testing/selftests/net/forwarding# ./mirror_gre_changes.sh
> TEST: mirror to gretap: TTL change (skip_hw)                        [ OK ]
> TEST: mirror to ip6gretap: TTL change (skip_hw)                     [ OK ]
> TEST: mirror to gretap: tunnel down/up (skip_hw)                    [ OK ]
> TEST: mirror to ip6gretap: tunnel down/up (skip_hw)                 [ OK ]
> TEST: mirror to gretap: egress down/up (skip_hw)                    [ OK ]
> TEST: mirror to ip6gretap: egress down/up (skip_hw)                 [ OK ]
> TEST: mirror to gretap: remote address change (skip_hw)             [ OK ]
> TEST: mirror to ip6gretap: remote address change (skip_hw)          [ OK ]
> TEST: mirror to gretap: tunnel deleted (skip_hw)                    [ OK ]
> TEST: mirror to ip6gretap: tunnel deleted (skip_hw)                 [ OK ]
> TEST: mirror to gretap: underlay route removal (skip_hw)            [ OK ]
> TEST: mirror to ip6gretap: underlay route removal (skip_hw)         [ OK ]
> WARN: Could not test offloaded functionality
> root@defiant:tools/testing/selftests/net/forwarding#

OK, I'll add your Tested-by, let it pass through our regression and send
upstream.
Mirsad Todorovac Aug. 2, 2023, 11:06 a.m. UTC | #26
On 2.8.2023. 12:33, Petr Machata wrote:
> 
> Mirsad Todorovac <mirsad.todorovac@alu.unizg.hr> writes:
> 
>> On 8/1/23 13:08, Petr Machata wrote:
>>> diff --git a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>>> b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>>> index aff88f78e339..5ea9d63915f7 100755
>>> --- a/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>>> +++ b/tools/testing/selftests/net/forwarding/mirror_gre_changes.sh
>>> @@ -72,7 +72,8 @@ test_span_gre_ttl()
>>>      	RET=0
>>>    -	mirror_install $swp1 ingress $tundev "matchall $tcflags"
>>> +	mirror_install $swp1 ingress $tundev \
>>> +		"prot ip flower $tcflags ip_prot icmp"
>>>    	tc filter add dev $h3 ingress pref 77 prot $prot \
>>>    		flower skip_hw ip_ttl 50 action pass
>>>    
>>
>> The problem seems to be fixed in the test run:
>>
>> root@defiant:tools/testing/selftests/net/forwarding# ./mirror_gre_changes.sh
>> TEST: mirror to gretap: TTL change (skip_hw)                        [ OK ]
>> TEST: mirror to ip6gretap: TTL change (skip_hw)                     [ OK ]
>> TEST: mirror to gretap: tunnel down/up (skip_hw)                    [ OK ]
>> TEST: mirror to ip6gretap: tunnel down/up (skip_hw)                 [ OK ]
>> TEST: mirror to gretap: egress down/up (skip_hw)                    [ OK ]
>> TEST: mirror to ip6gretap: egress down/up (skip_hw)                 [ OK ]
>> TEST: mirror to gretap: remote address change (skip_hw)             [ OK ]
>> TEST: mirror to ip6gretap: remote address change (skip_hw)          [ OK ]
>> TEST: mirror to gretap: tunnel deleted (skip_hw)                    [ OK ]
>> TEST: mirror to ip6gretap: tunnel deleted (skip_hw)                 [ OK ]
>> TEST: mirror to gretap: underlay route removal (skip_hw)            [ OK ]
>> TEST: mirror to ip6gretap: underlay route removal (skip_hw)         [ OK ]
>> WARN: Could not test offloaded functionality
>> root@defiant:tools/testing/selftests/net/forwarding#
> 
> OK, I'll add your Tested-by, let it pass through our regression and send
> upstream.

Great stuff.

Though, all these were incompatibilities in the testing scripts, not really a bug
in the kernel proper ...

Have a nice day!

Mirsad
diff mbox series

Patch

diff --git a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
index 56eb83d1a3bd..c7ab883d2515 100755
--- a/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
+++ b/tools/testing/selftests/net/forwarding/custom_multipath_hash.sh
@@ -363,7 +363,7 @@  custom_hash()
 	custom_hash_v6
 }
 
-trap cleanup EXIT
+trap cleanup INT TERM EXIT
 
 setup_prepare
 setup_wait