Re: [PATCH net-next v1 13/15] selftests: tc-testing: run dependsOn probes outside the test namespace

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The patch fixes a real regression but has no Fixes: tag.
- [Low] run_one_test() now passes a new stage string, 'probe', to every
  plugin's adjust_command(stage, command) hook.

--

selftests: tc-testing: run dependsOn probes outside the test namespace

This changes run_one_test() in tdc.py so the dependsOn probe is passed to
exec_cmd() with a new 'probe' stage instead of 'execute'. With that stage,
nsPlugin does not put "ip netns exec" in front of the probe, so the probe
runs in the base namespace and the probe-gated tests are no longer always
skipped.

> diff --git a/tools/testing/selftests/tc-testing/tdc.py b/tools/testing/selftests/tc-testing/tdc.py
> index 511d66c36a2a0..541caf984519f 100755
> --- a/tools/testing/selftests/tc-testing/tdc.py
> +++ b/tools/testing/selftests/tc-testing/tdc.py
> @@ -399,7 +399,7 @@ def run_one_test(pm, args, index, tidx):
>      if 'dependsOn' in tidx:
>          if (args.verbose > 0):
>              print('probe command for test skip')
> -        (p, procout) = exec_cmd(tidx, args, pm, 'execute', tidx['dependsOn'])
> +        (p, procout) = exec_cmd(tidx, args, pm, 'probe', tidx['dependsOn'])

[Severity: Low]

Should this patch have a Fixes: tag?

The probe block came from commit 7f3f86402609 ("selftests: tc-testing: add
"depends_on" property to skip tests"). At that time nsPlugin used one
shared netns that was created ahead of time, so the probe worked.

Commit 98cfbe4234a4 ("selftests/tc-testing: localize test resources")
moved tests to per-test tcut-<random> namespaces. It also wrote the
'execute' stage call shown above. In run_one_test(), the probe still runs
before the per-test suffix is added:

    NAMES['NS'] = '{}-{}'.format(NAMES['NS'], tidx['random'])

It also runs before pm.call_pre_case() creates the namespace. As a result,
nsPlugin's adjust_command() wraps the probe in a netns exec for the bare
"tcut" name, and no namespace with that name is ever created.

Because of this, the mirred dependsOn probes added later in dcfaf1f758ee
("selftests/tc-testing: require an up to date iproute2 for blockcast
tests") could never pass.

Would this tag fit?

Fixes: 98cfbe4234a4 ("selftests/tc-testing: localize test resources")

[Severity: Low]

This passes a new stage string, 'probe', to the adjust_command() hook of
every plugin. Should the in-tree docs list it?

The stage list in tools/testing/selftests/tc-testing/README reads:

  The stages are identified by the following strings:

    - pre  (pre-suite)
    - setup
    - command
    - verify
    - teardown
    - post (post-suite)

creating-plugins/AddingPlugins.txt also leaves 'probe' out of both its
phase list and its "The stages are represented by the following strings"
list. The commented stage-dispatch template in TdcPlugin.adjust_command()
has no 'probe' branch either.

The dependsOn description in creating-testcases/AddingTestCases.txt only
says:

  dependsOn:    Same as 'skip', but the value is executed as a command. The test
                is skipped when the command returns non-zero.

Probes now run in the base namespace, outside the test netns. A future
dependsOn command that uses per-test devices such as $DEV0, $DEV1 or
$DUMMY would fail, and its test would just be skipped. Could this be noted
in the dependsOn description?

The fix also depends on plugins passing unknown stages through unchanged.
nsPlugin and valgrindPlugin both do that today.

The existing lists were already out of date ('command' rather than
'execute'). The missing 'probe' entry and the base-namespace behaviour of
dependsOn are new with this patch.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261111005533.6436-1-toddfreed%40meta.com