Date: Wed, 12 Jul 2023 14:12:23 -0300
From: Pedro Tammela <pctammela@...atatu.com>
To: Matthieu Baerts <matthieu.baerts@...sares.net>,
 Jamal Hadi Salim <jhs@...atatu.com>, Cong Wang <xiyou.wangcong@...il.com>,
 Jiri Pirko <jiri@...nulli.us>
Cc: netdev <netdev@...r.kernel.org>, Anders Roxell
 <anders.roxell@...aro.org>, Davide Caratti <dcaratti@...hat.com>
Subject: Re: TC: selftests: current timeout (45s) is too low

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

Powered by blists - more mailing lists