TC: selftests: current timeout (45s) is too low

TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Jamal, Cong, Jiri,

When looking for something else [1] in LKFT reports [2], I noticed that
the TC selftest ended with a timeout error:

  not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds

The timeout has been introduced 3 years ago:

  852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
per test")

Recently, a new option has been introduced to override the value when
executing the code:

  f6a01213e3f8 ("selftests: allow runners to override the timeout")

But I guess it is still better to set a higher default value for TC
tests. This is easy to fix by simply adding "timeout=<seconds>" in a
"settings" file in 'tc-testing' directory, e.g.

  echo timeout=1200 > tools/testing/selftests/tc-testing/settings

I'm sending this email instead of a patch because I don't know which
value makes sense. I guess you know how long the tests can take in a
(very) slow environment and you might want to avoid this timeout error.

I also noticed most of the tests were skipped [2], probably because
something is missing in the test environment? Do not hesitate to contact
the lkft team [3], that's certainly easy to fix and it would increase
the TC test coverage when they are validating all the different kernel
versions :)

Cheers,
Matt

[1] The impact of https://github.com/Linaro/test-definitions/pull/446
[2]
https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log
[3] lkft@linaro.org
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Pedro Tammela]

Hi Matthieu,

I have been involved in tdc for a while now, here are my comments.

On 12/07/2023 06:47, Matthieu Baerts wrote:
> Hi Jamal, Cong, Jiri,
> 
> When looking for something else [1] in LKFT reports [2], I noticed that
> the TC selftest ended with a timeout error:
> 
>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
> 
> The timeout has been introduced 3 years ago:
> 
>    852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
> per test")
> 
> Recently, a new option has been introduced to override the value when
> executing the code:
> 
>    f6a01213e3f8 ("selftests: allow runners to override the timeout")
> 
> But I guess it is still better to set a higher default value for TC
> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
> "settings" file in 'tc-testing' directory, e.g.
> 
>    echo timeout=1200 > tools/testing/selftests/tc-testing/settings
> 
> I'm sending this email instead of a patch because I don't know which
> value makes sense. I guess you know how long the tests can take in a
> (very) slow environment and you might want to avoid this timeout error.

I believe a timeout between 5-10 to minutes should cover the entire suite
> 
> I also noticed most of the tests were skipped [2], probably because
> something is missing in the test environment? Do not hesitate to contact
> the lkft team [3], that's certainly easy to fix and it would increase
> the TC test coverage when they are validating all the different kernel
> versions :)

 From the logs it seems like the kernel image is missing the 'ct' 
action. Possibly also missing other actions/tc components, so it seems 
like a kernel config issue.

Interestingly enough the `tdc.sh` script doesn't test the filter and 
infrastructure categories. Perhaps it would be better to let it run the 
entire suite instead of just a few tests. It could probably break some 
other automated testing out there but it will be worth it, specially 
since we have been adding some regression tests lately.

I do know RedHat runs tdc, so let's ask their opinion.
+Davide WDYT?

> 
> Cheers,
> Matt
> 
> [1] The impact of https://github.com/Linaro/test-definitions/pull/446
> [2]
> https://qa-reports.linaro.org/lkft/linux-next-master/build/next-20230711/testrun/18267241/suite/kselftest-tc-testing/test/tc-testing_tdc_sh/log
> [3] lkft@linaro.org

Re: TC: selftests: current timeout (45s) is too low

[Davide Caratti]

hello!

On Wed, Jul 12, 2023 at 3:43 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>
> Hi Matthieu,
>
> I have been involved in tdc for a while now, here are my comments.
>
> On 12/07/2023 06:47, Matthieu Baerts wrote:
> > Hi Jamal, Cong, Jiri,
> >
> > When looking for something else [1] in LKFT reports [2], I noticed that
> > the TC selftest ended with a timeout error:
> >
> >    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
> >
> > The timeout has been introduced 3 years ago:
> >
> >    852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
> > per test")
> >
> > Recently, a new option has been introduced to override the value when
> > executing the code:
> >
> >    f6a01213e3f8 ("selftests: allow runners to override the timeout")
> >
> > But I guess it is still better to set a higher default value for TC
> > tests. This is easy to fix by simply adding "timeout=<seconds>" in a
> > "settings" file in 'tc-testing' directory, e.g.
> >
> >    echo timeout=1200 > tools/testing/selftests/tc-testing/settings

finding a good default is not easy, because some kernel (e.g. those
built with debug options) are very slow .
Maybe we can leverage also on the other value in tdc_config.py [1] -
or at least ensure that the setting in 'setting' is consistent.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d37e56df23f9e

[...]

> > I also noticed most of the tests were skipped [2], probably because
> > something is missing in the test environment? Do not hesitate to contact
> > the lkft team [3], that's certainly easy to fix and it would increase
> > the TC test coverage when they are validating all the different kernel
> > versions :)
>
>  From the logs it seems like the kernel image is missing the 'ct'
> action. Possibly also missing other actions/tc components, so it seems
> like a kernel config issue.

when I run tdc I use to do:

#  yes | make kselftest-merge

so that the kconfigs are not forgot :)

> Interestingly enough the `tdc.sh` script doesn't test the filter and
> infrastructure categories. Perhaps it would be better to let it run the
> entire suite instead of just a few tests.

I remember that the "filter" category was not stable enough when we
wrote tdc.sh. If it's in good shape now, we can surely add it.

thanks,
-- 
Davide

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Davide,

On 12/07/2023 16:02, Davide Caratti wrote:
> hello!
> 
> On Wed, Jul 12, 2023 at 3:43 PM Pedro Tammela <pctammela@mojatatu.com> wrote:
>>
>> Hi Matthieu,
>>
>> I have been involved in tdc for a while now, here are my comments.
>>
>> On 12/07/2023 06:47, Matthieu Baerts wrote:
>>> Hi Jamal, Cong, Jiri,
>>>
>>> When looking for something else [1] in LKFT reports [2], I noticed that
>>> the TC selftest ended with a timeout error:
>>>
>>>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>>
>>> The timeout has been introduced 3 years ago:
>>>
>>>    852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
>>> per test")
>>>
>>> Recently, a new option has been introduced to override the value when
>>> executing the code:
>>>
>>>    f6a01213e3f8 ("selftests: allow runners to override the timeout")
>>>
>>> But I guess it is still better to set a higher default value for TC
>>> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
>>> "settings" file in 'tc-testing' directory, e.g.
>>>
>>>    echo timeout=1200 > tools/testing/selftests/tc-testing/settings
> 
> finding a good default is not easy, because some kernel (e.g. those
> built with debug options) are very slow .
Thank you for your feedback!

I agree it is not be easy. From what I see, lkft doesn't run the
selftests with a debug kconfig. I guess we can assume these tests are
either ran in a slow environment or with a debug kconfig but not both,
otherwise the timeout would be too high -- at least that's what we did
with MPTCP :)

Is 15 minute a good value to start with?

> Maybe we can leverage also on the other value in tdc_config.py [1] -
> or at least ensure that the setting in 'setting' is consistent.
> 
> [1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=d37e56df23f9e
I don't know the TC test env but it looks like it is a timeout that is
used in case of issues. I guess this timeout should in theory never be
fired except in case of big problem, no?
If we use it and if there are 543 tests -- according to the logs -- the
global timeout would be just under 1h50 :)
This timeout in the worst case scenario when nothing is working, I guess
it makes sense to have this kselftest timeout below that.

> 
> [...]
> 
>>> I also noticed most of the tests were skipped [2], probably because
>>> something is missing in the test environment? Do not hesitate to contact
>>> the lkft team [3], that's certainly easy to fix and it would increase
>>> the TC test coverage when they are validating all the different kernel
>>> versions :)
>>
>>  From the logs it seems like the kernel image is missing the 'ct'
>> action. Possibly also missing other actions/tc components, so it seems
>> like a kernel config issue.
> 
> when I run tdc I use to do:
> 
> #  yes | make kselftest-merge
> 
> so that the kconfigs are not forgot :)

It looks like it is more than what lkft is using (see my reply to
Pedro). Maybe a conflict with just the ones lkft is using then?

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Pedro,

On 12/07/2023 15:43, Pedro Tammela wrote:
> I have been involved in tdc for a while now, here are my comments.

Thank you for your reply!

> On 12/07/2023 06:47, Matthieu Baerts wrote:
>> Hi Jamal, Cong, Jiri,
>>
>> When looking for something else [1] in LKFT reports [2], I noticed that
>> the TC selftest ended with a timeout error:
>>
>>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>
>> The timeout has been introduced 3 years ago:
>>
>>    852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
>> per test")
>>
>> Recently, a new option has been introduced to override the value when
>> executing the code:
>>
>>    f6a01213e3f8 ("selftests: allow runners to override the timeout")
>>
>> But I guess it is still better to set a higher default value for TC
>> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
>> "settings" file in 'tc-testing' directory, e.g.
>>
>>    echo timeout=1200 > tools/testing/selftests/tc-testing/settings
>>
>> I'm sending this email instead of a patch because I don't know which
>> value makes sense. I guess you know how long the tests can take in a
>> (very) slow environment and you might want to avoid this timeout error.
> 
> I believe a timeout between 5-10 to minutes should cover the entire suite

Thank you for your feedback.
If we want to be on the safe side, I guess it is better to put 10
minutes or even 15, no?

>> I also noticed most of the tests were skipped [2], probably because
>> something is missing in the test environment? Do not hesitate to contact
>> the lkft team [3], that's certainly easy to fix and it would increase
>> the TC test coverage when they are validating all the different kernel
>> versions :)
> 
> From the logs it seems like the kernel image is missing the 'ct' action.
> Possibly also missing other actions/tc components, so it seems like a
> kernel config issue.

According to [1], the kconfig is generated by merging these files:

  defconfig, systemd.config [2], tools/testing/selftests/kexec/config,
tools/testing/selftests/net/config,
tools/testing/selftests/net/mptcp/config,
tools/testing/selftests/net/hsr/config,
tools/testing/selftests/net/forwarding/config,
tools/testing/selftests/tc-testing/config

You can see the final .config file in [3].

I can see "CONFIG_NET_ACT_CTINFO(=m)" but not "CONFIG_NET_ACT_CT" while
they are both in tc-testing/config file. Maybe a conflict with another
selftest config?

I don't see any mention of "NET_ACT_CT" in the build logs [4].

Cheers,
Matt

[1]
https://tuxapi.tuxsuite.com/v1/groups/linaro/projects/lkft/builds/2SPJN70f1LBiWmZIxl0WNcOmjwN
[2]
https://raw.githubusercontent.com/Linaro/meta-lkft/kirkstone/meta/recipes-kernel/linux/files/systemd.config
[3]
https://storage.tuxsuite.com/public/linaro/lkft/builds/2SPJN70f1LBiWmZIxl0WNcOmjwN/config
[4]
https://storage.tuxsuite.com/public/linaro/lkft/builds/2SPJN70f1LBiWmZIxl0WNcOmjwN/build-debug.log
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Pedro Tammela]

On 12/07/2023 11:43, Matthieu Baerts wrote:
> Hi Pedro,
> 
> On 12/07/2023 15:43, Pedro Tammela wrote:
>> I have been involved in tdc for a while now, here are my comments.
> 
> Thank you for your reply!
> 
>> On 12/07/2023 06:47, Matthieu Baerts wrote:
>>> Hi Jamal, Cong, Jiri,
>>>
>>> When looking for something else [1] in LKFT reports [2], I noticed that
>>> the TC selftest ended with a timeout error:
>>>
>>>     not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>>
>>> The timeout has been introduced 3 years ago:
>>>
>>>     852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
>>> per test")
>>>
>>> Recently, a new option has been introduced to override the value when
>>> executing the code:
>>>
>>>     f6a01213e3f8 ("selftests: allow runners to override the timeout")
>>>
>>> But I guess it is still better to set a higher default value for TC
>>> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
>>> "settings" file in 'tc-testing' directory, e.g.
>>>
>>>     echo timeout=1200 > tools/testing/selftests/tc-testing/settings
>>>
>>> I'm sending this email instead of a patch because I don't know which
>>> value makes sense. I guess you know how long the tests can take in a
>>> (very) slow environment and you might want to avoid this timeout error.
>>
>> I believe a timeout between 5-10 to minutes should cover the entire suite
> 
> Thank you for your feedback.
> If we want to be on the safe side, I guess it is better to put 10
> minutes or even 15, no?

Sure, makes sense.
If someone complains we can lower it.

> 
>>> I also noticed most of the tests were skipped [2], probably because
>>> something is missing in the test environment? Do not hesitate to contact
>>> the lkft team [3], that's certainly easy to fix and it would increase
>>> the TC test coverage when they are validating all the different kernel
>>> versions :)
>>
>>  From the logs it seems like the kernel image is missing the 'ct' action.
>> Possibly also missing other actions/tc components, so it seems like a
>> kernel config issue.
> 
> According to [1], the kconfig is generated by merging these files:
> 
>    defconfig, systemd.config [2], tools/testing/selftests/kexec/config,
> tools/testing/selftests/net/config,
> tools/testing/selftests/net/mptcp/config,
> tools/testing/selftests/net/hsr/config,
> tools/testing/selftests/net/forwarding/config,
> tools/testing/selftests/tc-testing/config
> 
> You can see the final .config file in [3].
> 
> I can see "CONFIG_NET_ACT_CTINFO(=m)" but not "CONFIG_NET_ACT_CT" while
> they are both in tc-testing/config file. Maybe a conflict with another
> selftest config?
> 
> I don't see any mention of "NET_ACT_CT" in the build logs [4].

There's a requirement for NET_ACT_CT which is not set in the final 
config (CONFIG_NF_FLOW_TABLE).

Perhaps this could fix?
diff --git a/tools/testing/selftests/tc-testing/config 
b/tools/testing/selftests/tc-testing/config
index 6e73b09c20c8..d1ad29040c02 100644
--- a/tools/testing/selftests/tc-testing/config
+++ b/tools/testing/selftests/tc-testing/config
@@ -5,6 +5,7 @@ CONFIG_NF_CONNTRACK=m
  CONFIG_NF_CONNTRACK_MARK=y
  CONFIG_NF_CONNTRACK_ZONES=y
  CONFIG_NF_CONNTRACK_LABELS=y
+CONFIG_NF_FLOW_TABLE=m
  CONFIG_NF_NAT=m
  CONFIG_NETFILTER_XT_TARGET_LOG=m

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Pedro,

On 12/07/2023 19:12, Pedro Tammela wrote:
> On 12/07/2023 11:43, Matthieu Baerts wrote:
>> Hi Pedro,
>>
>> On 12/07/2023 15:43, Pedro Tammela wrote:
>>> I have been involved in tdc for a while now, here are my comments.
>>
>> Thank you for your reply!
>>
>>> On 12/07/2023 06:47, Matthieu Baerts wrote:
>>>> Hi Jamal, Cong, Jiri,
>>>>
>>>> When looking for something else [1] in LKFT reports [2], I noticed that
>>>> the TC selftest ended with a timeout error:
>>>>
>>>>     not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>>>
>>>> The timeout has been introduced 3 years ago:
>>>>
>>>>     852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
>>>> per test")
>>>>
>>>> Recently, a new option has been introduced to override the value when
>>>> executing the code:
>>>>
>>>>     f6a01213e3f8 ("selftests: allow runners to override the timeout")
>>>>
>>>> But I guess it is still better to set a higher default value for TC
>>>> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
>>>> "settings" file in 'tc-testing' directory, e.g.
>>>>
>>>>     echo timeout=1200 > tools/testing/selftests/tc-testing/settings
>>>>
>>>> I'm sending this email instead of a patch because I don't know which
>>>> value makes sense. I guess you know how long the tests can take in a
>>>> (very) slow environment and you might want to avoid this timeout error.
>>>
>>> I believe a timeout between 5-10 to minutes should cover the entire
>>> suite
>>
>> Thank you for your feedback.
>> If we want to be on the safe side, I guess it is better to put 10
>> minutes or even 15, no?
> 
> Sure, makes sense.
> If someone complains we can lower it.
> 
>>
>>>> I also noticed most of the tests were skipped [2], probably because
>>>> something is missing in the test environment? Do not hesitate to
>>>> contact
>>>> the lkft team [3], that's certainly easy to fix and it would increase
>>>> the TC test coverage when they are validating all the different kernel
>>>> versions :)
>>>
>>>  From the logs it seems like the kernel image is missing the 'ct'
>>> action.
>>> Possibly also missing other actions/tc components, so it seems like a
>>> kernel config issue.
>>
>> According to [1], the kconfig is generated by merging these files:
>>
>>    defconfig, systemd.config [2], tools/testing/selftests/kexec/config,
>> tools/testing/selftests/net/config,
>> tools/testing/selftests/net/mptcp/config,
>> tools/testing/selftests/net/hsr/config,
>> tools/testing/selftests/net/forwarding/config,
>> tools/testing/selftests/tc-testing/config
>>
>> You can see the final .config file in [3].
>>
>> I can see "CONFIG_NET_ACT_CTINFO(=m)" but not "CONFIG_NET_ACT_CT" while
>> they are both in tc-testing/config file. Maybe a conflict with another
>> selftest config?
>>
>> I don't see any mention of "NET_ACT_CT" in the build logs [4].
> 
> There's a requirement for NET_ACT_CT which is not set in the final
> config (CONFIG_NF_FLOW_TABLE).
> 
> Perhaps this could fix?
> diff --git a/tools/testing/selftests/tc-testing/config
> b/tools/testing/selftests/tc-testing/config
> index 6e73b09c20c8..d1ad29040c02 100644
> --- a/tools/testing/selftests/tc-testing/config
> +++ b/tools/testing/selftests/tc-testing/config
> @@ -5,6 +5,7 @@ CONFIG_NF_CONNTRACK=m
>  CONFIG_NF_CONNTRACK_MARK=y
>  CONFIG_NF_CONNTRACK_ZONES=y
>  CONFIG_NF_CONNTRACK_LABELS=y
> +CONFIG_NF_FLOW_TABLE=m
>  CONFIG_NF_NAT=m
>  CONFIG_NETFILTER_XT_TARGET_LOG=m

Yes it does!

I got access to the tuxsuite to reproduce the issues with the suggested
fixes. The i386 build job is visible in [1] (kconfig in [2]) and the
test job in [3] (logs in [4]).

[1]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/builds/2SW6Vk3VYTGyW90OBecA3knJFIz
[2]
https://storage.tuxsuite.com/public/community/matthieu.baerts/builds/2SW6Vk3VYTGyW90OBecA3knJFIz/config
[3]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWB6sYne9afpOxqp3CNE5BxAn8
[4]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWB6sYne9afpOxqp3CNE5BxAn8/logs?format=html


Note that the TC tests have been executed in less than 3 minutes. 15
minutes seem more than enough then! (I don't know how "fast" is this
environment).

We can see that all tests have been executed except one:

> # ok 495 6bda - Add tunnel_key action with nofrag option # skipped - probe command: test skipped.

Maybe something else missing?

Other than that, 6 tests have failed:

- Add skbedit action with valid mark and mask with invalid format

> # not ok 284 bc15 - Add skbedit action with valid mark and mask with invalid format
> # 	Command exited with 0, expected 255

- Add ct action triggering DNAT tuple conflict:

> # not ok 373 3992 - Add ct action triggering DNAT tuple conflict
> # 	Could not match regex pattern. Verify command output:
> # cat: /proc/net/nf_conntrack: No such file or directory

- Add xt action with log-prefix

> # not ok 408 2029 - Add xt action with log-prefix
> # 	Could not match regex pattern. Verify command output:
> # total acts 1
> # 
> # 	action order 0: tablename: mangle  hook: NF_IP_POST_ROUTING
> # 	target  LOG level warn prefix \"PONG\"
> # 	index 100 ref 1 bind 0
> # 	not_in_hw

- Replace xt action log-prefix

> # not ok 409 3562 - Replace xt action log-prefix
> # 	Could not match regex pattern. Verify command output:
> # total acts 0
> # 
> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
> # 	target  LOG level warn prefix \"WIN\"
> # 	index 1 ref 1 bind 0
> # 	not_in_hw

- Delete xt action with invalid index

> # not ok 411 5169 - Delete xt action with invalid index
> # 	Could not match regex pattern. Verify command output:
> # total acts 0
> # 
> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
> # 	target  LOG level warn prefix \"PONG\"
> # 	index 1000 ref 1 bind 0
> # 	not_in_hw

- Add xt action with duplicate index

> # not ok 414 8437 - Add xt action with duplicate index
> # 	Could not match regex pattern. Verify command output:
> # total acts 0
> # 
> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
> # 	target  LOG level warn prefix \"PONG\"
> # 	index 101 ref 1 bind 0
> # 	not_in_hw

I can see that at least "CONFIG_NF_CONNTRACK_PROCFS" kconfig is needed
as well for the 373rd test (adding it seems helping: [5]).

Not sure about the 5 others, I don't know what these tests are doing, I
came here by accident and I don't think I'm the most appropriated person
to fix that: do you know if someone can look at the 5 other errors? :)

I can send patches to fix the timeout + the two missing kconfig if you want.

Cheers,
Matt

[5]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWHb7PJfqkUX1m8rLu3GXbsHE0/logs?format=html
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Pedro Tammela]

On 13/07/2023 10:59, Matthieu Baerts wrote:
> Hi Pedro,
> 
> On 12/07/2023 19:12, Pedro Tammela wrote:
>> On 12/07/2023 11:43, Matthieu Baerts wrote:
>>> Hi Pedro,
>>>
>>> On 12/07/2023 15:43, Pedro Tammela wrote:
>>>> I have been involved in tdc for a while now, here are my comments.
>>>
>>> Thank you for your reply!
>>>
>>>> On 12/07/2023 06:47, Matthieu Baerts wrote:
>>>>> Hi Jamal, Cong, Jiri,
>>>>>
>>>>> When looking for something else [1] in LKFT reports [2], I noticed that
>>>>> the TC selftest ended with a timeout error:
>>>>>
>>>>>      not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
>>>>>
>>>>> The timeout has been introduced 3 years ago:
>>>>>
>>>>>      852c8cbf34d3 ("selftests/kselftest/runner.sh: Add 45 second timeout
>>>>> per test")
>>>>>
>>>>> Recently, a new option has been introduced to override the value when
>>>>> executing the code:
>>>>>
>>>>>      f6a01213e3f8 ("selftests: allow runners to override the timeout")
>>>>>
>>>>> But I guess it is still better to set a higher default value for TC
>>>>> tests. This is easy to fix by simply adding "timeout=<seconds>" in a
>>>>> "settings" file in 'tc-testing' directory, e.g.
>>>>>
>>>>>      echo timeout=1200 > tools/testing/selftests/tc-testing/settings
>>>>>
>>>>> I'm sending this email instead of a patch because I don't know which
>>>>> value makes sense. I guess you know how long the tests can take in a
>>>>> (very) slow environment and you might want to avoid this timeout error.
>>>>
>>>> I believe a timeout between 5-10 to minutes should cover the entire
>>>> suite
>>>
>>> Thank you for your feedback.
>>> If we want to be on the safe side, I guess it is better to put 10
>>> minutes or even 15, no?
>>
>> Sure, makes sense.
>> If someone complains we can lower it.
>>
>>>
>>>>> I also noticed most of the tests were skipped [2], probably because
>>>>> something is missing in the test environment? Do not hesitate to
>>>>> contact
>>>>> the lkft team [3], that's certainly easy to fix and it would increase
>>>>> the TC test coverage when they are validating all the different kernel
>>>>> versions :)
>>>>
>>>>   From the logs it seems like the kernel image is missing the 'ct'
>>>> action.
>>>> Possibly also missing other actions/tc components, so it seems like a
>>>> kernel config issue.
>>>
>>> According to [1], the kconfig is generated by merging these files:
>>>
>>>     defconfig, systemd.config [2], tools/testing/selftests/kexec/config,
>>> tools/testing/selftests/net/config,
>>> tools/testing/selftests/net/mptcp/config,
>>> tools/testing/selftests/net/hsr/config,
>>> tools/testing/selftests/net/forwarding/config,
>>> tools/testing/selftests/tc-testing/config
>>>
>>> You can see the final .config file in [3].
>>>
>>> I can see "CONFIG_NET_ACT_CTINFO(=m)" but not "CONFIG_NET_ACT_CT" while
>>> they are both in tc-testing/config file. Maybe a conflict with another
>>> selftest config?
>>>
>>> I don't see any mention of "NET_ACT_CT" in the build logs [4].
>>
>> There's a requirement for NET_ACT_CT which is not set in the final
>> config (CONFIG_NF_FLOW_TABLE).
>>
>> Perhaps this could fix?
>> diff --git a/tools/testing/selftests/tc-testing/config
>> b/tools/testing/selftests/tc-testing/config
>> index 6e73b09c20c8..d1ad29040c02 100644
>> --- a/tools/testing/selftests/tc-testing/config
>> +++ b/tools/testing/selftests/tc-testing/config
>> @@ -5,6 +5,7 @@ CONFIG_NF_CONNTRACK=m
>>   CONFIG_NF_CONNTRACK_MARK=y
>>   CONFIG_NF_CONNTRACK_ZONES=y
>>   CONFIG_NF_CONNTRACK_LABELS=y
>> +CONFIG_NF_FLOW_TABLE=m
>>   CONFIG_NF_NAT=m
>>   CONFIG_NETFILTER_XT_TARGET_LOG=m
> 
> Yes it does!
> 
> I got access to the tuxsuite to reproduce the issues with the suggested
> fixes. The i386 build job is visible in [1] (kconfig in [2]) and the
> test job in [3] (logs in [4]).
> 
> [1]
> https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/builds/2SW6Vk3VYTGyW90OBecA3knJFIz
> [2]
> https://storage.tuxsuite.com/public/community/matthieu.baerts/builds/2SW6Vk3VYTGyW90OBecA3knJFIz/config
> [3]
> https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWB6sYne9afpOxqp3CNE5BxAn8
> [4]
> https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWB6sYne9afpOxqp3CNE5BxAn8/logs?format=html
> 
> 
> Note that the TC tests have been executed in less than 3 minutes. 15
> minutes seem more than enough then! (I don't know how "fast" is this
> environment).
> 
> We can see that all tests have been executed except one:
> 
>> # ok 495 6bda - Add tunnel_key action with nofrag option # skipped - probe command: test skipped.
> 
> Maybe something else missing?
> 
> Other than that, 6 tests have failed:
> 
> - Add skbedit action with valid mark and mask with invalid format
> 
>> # not ok 284 bc15 - Add skbedit action with valid mark and mask with invalid format
>> # 	Command exited with 0, expected 255
> 
> - Add ct action triggering DNAT tuple conflict:
> 
>> # not ok 373 3992 - Add ct action triggering DNAT tuple conflict
>> # 	Could not match regex pattern. Verify command output:
>> # cat: /proc/net/nf_conntrack: No such file or directory
> 
> - Add xt action with log-prefix
> 
>> # not ok 408 2029 - Add xt action with log-prefix
>> # 	Could not match regex pattern. Verify command output:
>> # total acts 1
>> #
>> # 	action order 0: tablename: mangle  hook: NF_IP_POST_ROUTING
>> # 	target  LOG level warn prefix \"PONG\"
>> # 	index 100 ref 1 bind 0
>> # 	not_in_hw
> 
> - Replace xt action log-prefix
> 
>> # not ok 409 3562 - Replace xt action log-prefix
>> # 	Could not match regex pattern. Verify command output:
>> # total acts 0
>> #
>> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
>> # 	target  LOG level warn prefix \"WIN\"
>> # 	index 1 ref 1 bind 0
>> # 	not_in_hw
> 
> - Delete xt action with invalid index
> 
>> # not ok 411 5169 - Delete xt action with invalid index
>> # 	Could not match regex pattern. Verify command output:
>> # total acts 0
>> #
>> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
>> # 	target  LOG level warn prefix \"PONG\"
>> # 	index 1000 ref 1 bind 0
>> # 	not_in_hw
> 
> - Add xt action with duplicate index
> 
>> # not ok 414 8437 - Add xt action with duplicate index
>> # 	Could not match regex pattern. Verify command output:
>> # total acts 0
>> #
>> # 	action order 1: tablename: mangle  hook: NF_IP_POST_ROUTING
>> # 	target  LOG level warn prefix \"PONG\"
>> # 	index 101 ref 1 bind 0
>> # 	not_in_hw
> 

Cool! So it seems we have some tests that bit rotted...

> I can see that at least "CONFIG_NF_CONNTRACK_PROCFS" kconfig is needed
> as well for the 373rd test (adding it seems helping: [5]).
> 
> Not sure about the 5 others, I don't know what these tests are doing, I
> came here by accident and I don't think I'm the most appropriated person
> to fix that: do you know if someone can look at the 5 other errors? :)

We can take a look, thank you.

> 
> I can send patches to fix the timeout + the two missing kconfig if you want.

Yes, please. Could you also do one final test with the following?
It will increase the total testing wall time but it's ~time~ we let the 
bull loose.

diff --git a/tools/testing/selftests/tc-testing/tdc.sh 
b/tools/testing/selftests/tc-testing/tdc.sh
index eb357bd7923c..ae6e19f7658d 100755
--- a/tools/testing/selftests/tc-testing/tdc.sh
+++ b/tools/testing/selftests/tc-testing/tdc.sh
@@ -5,3 +5,5 @@ modprobe netdevsim
  modprobe sch_teql
  ./tdc.py -c actions --nobuildebpf
  ./tdc.py -c qdisc
+./tdc.py -c filter
+./tdc.py -c infra

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Pedro,

On 13/07/2023 19:30, Pedro Tammela wrote:
> On 13/07/2023 10:59, Matthieu Baerts wrote:

(...)

>> We can see that all tests have been executed except one:
>>
>>> # ok 495 6bda - Add tunnel_key action with nofrag option # skipped -
>>> probe command: test skipped.
>>
>> Maybe something else missing?

Do you think this one can be due to a missing kconfig? This command is
failing:

  $TC actions add action tunnel_key help 2>&1 | grep -q nofrag

Or maybe that's normal, e.g. a feature no longer there?

>> Other than that, 6 tests have failed:

(...)

> Cool! So it seems we have some tests that bit rotted...

Who doesn't? :)

>> I can see that at least "CONFIG_NF_CONNTRACK_PROCFS" kconfig is needed
>> as well for the 373rd test (adding it seems helping: [5]).
>>
>> Not sure about the 5 others, I don't know what these tests are doing, I
>> came here by accident and I don't think I'm the most appropriated person
>> to fix that: do you know if someone can look at the 5 other errors? :)
> 
> We can take a look, thank you.

Thank you!

>> I can send patches to fix the timeout + the two missing kconfig if you
>> want.
> 
> Yes, please.

Sure, will do!

> Could you also do one final test with the following?
> It will increase the total testing wall time but it's ~time~ we let the
> bull loose.

Just did, it took just over 3 minutes (~3:05), see the log file in [1]
(test job in [2] and build job in [3]).

Not much longer but 15 more tests failing :)
Also, 12 new tests have been skipped:

> Tests using the DEV2 variable must define the name of a physical NIC with the -d option when running tdc.
But I guess that's normal when executing tdc.sh.

Cheers,
Matt

[1]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWzPV0rAEkfm82nptjEpjN1syj/logs?format=html
[2]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/tests/2SWzPV0rAEkfm82nptjEpjN1syj
[3]
https://tuxapi.tuxsuite.com/v1/groups/community/projects/matthieu.baerts/builds/2SWwyRVr1ZCKUNJ7wUqESGnRnjq
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Pedro Tammela]

On 13/07/2023 16:59, Matthieu Baerts wrote:
> Hi Pedro,
> 
> On 13/07/2023 19:30, Pedro Tammela wrote:
>> On 13/07/2023 10:59, Matthieu Baerts wrote:
> 
> (...)
> 
>>> We can see that all tests have been executed except one:
>>>
>>>> # ok 495 6bda - Add tunnel_key action with nofrag option # skipped -
>>>> probe command: test skipped.
>>>
>>> Maybe something else missing?
> 
> Do you think this one can be due to a missing kconfig? This command is
> failing:
> 
>    $TC actions add action tunnel_key help 2>&1 | grep -q nofrag
> 
> Or maybe that's normal, e.g. a feature no longer there?

I know what's happening... more below

> 
>>> Other than that, 6 tests have failed:
> 
> (...)
> 
>> Cool! So it seems we have some tests that bit rotted...
> 
> Who doesn't? :)
> 
>>> I can see that at least "CONFIG_NF_CONNTRACK_PROCFS" kconfig is needed
>>> as well for the 373rd test (adding it seems helping: [5]).
>>>
>>> Not sure about the 5 others, I don't know what these tests are doing, I
>>> came here by accident and I don't think I'm the most appropriated person
>>> to fix that: do you know if someone can look at the 5 other errors? :)
>>
>> We can take a look, thank you.
> 
> Thank you!
> 
>>> I can send patches to fix the timeout + the two missing kconfig if you
>>> want.
>>
>> Yes, please.
> 
> Sure, will do!
> 
>> Could you also do one final test with the following?
>> It will increase the total testing wall time but it's ~time~ we let the
>> bull loose.
> 
> Just did, it took just over 3 minutes (~3:05), see the log file in [1]
> (test job in [2] and build job in [3]).
> 
> Not much longer but 15 more tests failing :)
> Also, 12 new tests have been skipped:
> 
>> Tests using the DEV2 variable must define the name of a physical NIC with the -d option when running tdc.
> But I guess that's normal when executing tdc.sh.
> 

Yep, some tests could require physical hardware, so it's ok to skip those.

As for some of the tests that failed / skipped, it might be because of 
an old iproute2.
I see that's using bookworm as the rootfs, which has the 6.1 iproute2.
Generally tdc should run with the matching iproute2 version although 
it's not really required but rather recommended.
We do have a 'dependsOn' directive to skip in case of mismatches, so 
perhaps it might be necessary to adjust some of these tests.

OTOH, is it possible to have a rootfs which runs the tests in tandem 
with iproute2-next?

Thanks for chasing this! I will let the guys know and we will try to fix 
the test failures.

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi Pedro, LKFT team,

@LKFT team: there is question for you below.

On 13/07/2023 22:32, Pedro Tammela wrote:
> On 13/07/2023 16:59, Matthieu Baerts wrote:
>> On 13/07/2023 19:30, Pedro Tammela wrote:

(...)

>>> Could you also do one final test with the following?
>>> It will increase the total testing wall time but it's ~time~ we let the
>>> bull loose.
>>
>> Just did, it took just over 3 minutes (~3:05), see the log file in [1]
>> (test job in [2] and build job in [3]).
>>
>> Not much longer but 15 more tests failing :)
>> Also, 12 new tests have been skipped:
>>
>>> Tests using the DEV2 variable must define the name of a physical NIC
>>> with the -d option when running tdc.
>> But I guess that's normal when executing tdc.sh.
>>
> 
> Yep, some tests could require physical hardware, so it's ok to skip those.

OK

> As for some of the tests that failed / skipped, it might be because of
> an old iproute2.
> I see that's using bookworm as the rootfs, which has the 6.1 iproute2.
> Generally tdc should run with the matching iproute2 version although
> it's not really required but rather recommended.

Ah yes, it makes sense!

> We do have a 'dependsOn' directive to skip in case of mismatches, so
> perhaps it might be necessary to adjust some of these tests.

Yes, better to skip. Especially because the selftests are supposed to
support old tools and kernel versions:

  https://lore.kernel.org/stable/ZAHLYvOPEYghRcJ1@kroah.com/

> OTOH, is it possible to have a rootfs which runs the tests in tandem
> with iproute2-next?

I don't know :)
But I hope the LKFT team can help answering this question!

@LKFT team: is it possible to run the latest iproute2 version, even the
one from iproute2-next when validating linux-next?

> Thanks for chasing this! I will let the guys know and we will try to fix
> the test failures.

Thank you for your support!

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net

Re: TC: selftests: current timeout (45s) is too low

[Naresh Kamboju]

Hi Matthieu and Pedro,

On Fri, 14 Jul 2023 at 02:41, 'Matthieu Baerts' via lkft
<lkft@linaro.org> wrote:
>
> Hi Pedro, LKFT team,
>
> @LKFT team: there is question for you below.
>
> On 13/07/2023 22:32, Pedro Tammela wrote:
> > On 13/07/2023 16:59, Matthieu Baerts wrote:
> >> On 13/07/2023 19:30, Pedro Tammela wrote:
>
> (...)
>
> >>> Could you also do one final test with the following?
> >>> It will increase the total testing wall time but it's ~time~ we let the
> >>> bull loose.
> >>
> >> Just did, it took just over 3 minutes (~3:05), see the log file in [1]
> >> (test job in [2] and build job in [3]).
> >>
> >> Not much longer but 15 more tests failing :)
> >> Also, 12 new tests have been skipped:
> >>
> >>> Tests using the DEV2 variable must define the name of a physical NIC
> >>> with the -d option when running tdc.
> >> But I guess that's normal when executing tdc.sh.
> >>
> >
> > Yep, some tests could require physical hardware, so it's ok to skip those.
>
> OK
>
> > As for some of the tests that failed / skipped, it might be because of
> > an old iproute2.
> > I see that's using bookworm as the rootfs, which has the 6.1 iproute2.
> > Generally tdc should run with the matching iproute2 version although
> > it's not really required but rather recommended.
>
> Ah yes, it makes sense!
>
> > We do have a 'dependsOn' directive to skip in case of mismatches, so
> > perhaps it might be necessary to adjust some of these tests.
>
> Yes, better to skip. Especially because the selftests are supposed to
> support old tools and kernel versions:
>
>   https://lore.kernel.org/stable/ZAHLYvOPEYghRcJ1@kroah.com/
>
> > OTOH, is it possible to have a rootfs which runs the tests in tandem
> > with iproute2-next?
>
> I don't know :)
> But I hope the LKFT team can help answering this question!
>
> @LKFT team: is it possible to run the latest iproute2 version, even the
> one from iproute2-next when validating linux-next?

As you know, LKFT is using debian bookworm as rootfs.
We always try to have minimal maintenance and user space tools dependency.
However, I will check with our team to upgrade iproute2 and
tryout testing iproute2-next when validating linux-next.

We need to check our LKFT budget for this additional build and test cost.

Links:
 - https://kernel.googlesource.com/pub/scm/network/iproute2/iproute2-next/

- Naresh

RE: TC: selftests: current timeout (45s) is too low

[David Laight]

From: Matthieu Baerts
> Sent: 12 July 2023 15:43
> 
> Hi Pedro,
> 
> On 12/07/2023 15:43, Pedro Tammela wrote:
> > I have been involved in tdc for a while now, here are my comments.
> 
> Thank you for your reply!
> 
> > On 12/07/2023 06:47, Matthieu Baerts wrote:
> >> Hi Jamal, Cong, Jiri,
> >>
> >> When looking for something else [1] in LKFT reports [2], I noticed that
> >> the TC selftest ended with a timeout error:
> >>
> >>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
...
> >> I'm sending this email instead of a patch because I don't know which
> >> value makes sense. I guess you know how long the tests can take in a
> >> (very) slow environment and you might want to avoid this timeout error.
> >
> > I believe a timeout between 5-10 to minutes should cover the entire suite
> 
> Thank you for your feedback.
> If we want to be on the safe side, I guess it is better to put 10
> minutes or even 15, no?

Is it possible to use the time taken for an initial test
to scale the timeout for all the tests?

Then you could have a 45second timeout on a fast system and
a much longer timeout on a slow one.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: TC: selftests: current timeout (45s) is too low

[Matthieu Baerts]

Hi David,

On 14/07/2023 17:15, David Laight wrote:
> From: Matthieu Baerts
>> Sent: 12 July 2023 15:43
>>
>> Hi Pedro,
>>
>> On 12/07/2023 15:43, Pedro Tammela wrote:
>>> I have been involved in tdc for a while now, here are my comments.
>>
>> Thank you for your reply!
>>
>>> On 12/07/2023 06:47, Matthieu Baerts wrote:
>>>> Hi Jamal, Cong, Jiri,
>>>>
>>>> When looking for something else [1] in LKFT reports [2], I noticed that
>>>> the TC selftest ended with a timeout error:
>>>>
>>>>    not ok 1 selftests: tc-testing: tdc.sh # TIMEOUT 45 seconds
> ...
>>>> I'm sending this email instead of a patch because I don't know which
>>>> value makes sense. I guess you know how long the tests can take in a
>>>> (very) slow environment and you might want to avoid this timeout error.
>>>
>>> I believe a timeout between 5-10 to minutes should cover the entire suite
>>
>> Thank you for your feedback.
>> If we want to be on the safe side, I guess it is better to put 10
>> minutes or even 15, no?
> 
> Is it possible to use the time taken for an initial test
> to scale the timeout for all the tests?
> 
> Then you could have a 45second timeout on a fast system and
> a much longer timeout on a slow one.

For the selftests global timeout, that would be great but with the
current architecture, it is not possible to do that because the value of
this global timeout is used when starting the different selftests, e.g.

  /usr/bin/timeout --foreground 45 ./tdc.sh

For the per-test timeout used in TC test environment -- currently at 24
seconds -- I guess it could be adapted like that but that's a different
topic.

Cheers,
Matt
-- 
Tessares | Belgium | Hybrid Access Solutions
www.tessares.net