Re: [PATCH net-next 1/1] tc-testing: initial version of tunnel_key unit tests

[Davide Caratti <dcaratti@redhat.com>]

hello Lucas,

On Wed, 2018-06-27 at 14:50 -0400, Lucas Bates wrote:
> On Tue, Jun 26, 2018 at 10:51 AM, Davide Caratti <dcaratti@redhat.com> wrote:
> > On Tue, 2018-06-26 at 09:17 -0400, Keara Leibovitz wrote:
> > > Create unittests for the tc tunnel_key action.
> > > 
> > > 
> > > Signed-off-by: Keara Leibovitz <kleib@mojatatu.com>
> > > ---
> > >  .../tc-testing/tc-tests/actions/tunnel_key.json    | 676 +++++++++++++++++++++
> > >  1 file changed, 676 insertions(+)
> > >  create mode 100644 tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
> > > 
> > > diff --git a/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
> > > new file mode 100644
> > > index 000000000000..bfe522ac8177
> > 
> > hello Keara!
> > 
> > I think the 'teardown' stage in some of these tests should be reviewed.
> > Those that are meant to test invalid configurations (like dc6b) should
> > allow non-zero exit codes in the teardown stage, if the wrong
> > configuration is catched by the userspace TC tool, before talking to the
> > kernel.
> > 
> > Otherwise, those tests will fail when they are invoked one by one with the
> > act_tunnel_key module unloaded.
> > 
> 
> Hi Davide, I thought I'd weigh in here.

glad to hear your feedback!

> In the short term, I think this is reasonable, but it's not a feasible
> long-term solution.  Here's why:
> 
> Allowing non-zero exit codes on setup and teardown was a precaution
> that needed to be implemented as flushing actions in a freshly-booted
> kernel returned errors - certain actions would only allow you to flush
> after that action had been added.

I guess this is a desired behavior, and it's common to all TC actions:

# grep bpf /proc/modules
# tc actions flush action bpf
RTNETLINK answers: Invalid argument
We have an error flushing
# modprobe act_bpf
 tc actions flush action bpf
# echo $?
0

> But, doing this on so many test cases means that we can lose control
> of the test environment, especially since a lot of commands get copied
> between test cases.  One test's command under test becomes the next
> test case's setup command, etc.  This can cause false results and
> potentially waste a lot of time for someone trying to track down a
> bug... Or cause bugs to be missed.

I understand, you want to ensure that 'teardown' leaves the scenario in a
status which is the same as before the 'setup' phase. Whether or not this
happened successfully, it's sane not to ignore the error code: otherwise,
test X will perturbate test X+1.

> So, how to fix: we've had some discussions about it already.  Jiri had
> requested the addition of a config file (like the one at
> tools/testing/selftests/net/forwarding/config, and maybe an addition
> to the README for tdc for explanation.  People would then possibly be
> restricted to running one test case file at a time based on what
> options they had loaded...  This is still not ideal.

All this depends on where the error condition is catched. Some parameters
(like the invalid 'index' in act_bpf) are rejected within userspace TC,
some others (like the invalid bytecode for test f84a) in the kernel.

> I think the best possible fix is to add a new plugin for tdc to
> exclude tests based on the kernel config.  This would require the
> addition of a new optional field to the test case format, where any
> and all included modules required for the test to work would be
> listed.  The plugin would look at this information, do its best to
> determine if the currently running kernel supports it, and allows the
> test to run or be skipped as a result.
> 
> Let me show an example of the new field:
> > > --- /dev/null
> > > +++ b/tools/testing/selftests/tc-testing/tc-tests/actions/tunnel_key.json
> > > @@ -0,0 +1,676 @@
> > > 
> > 
> > ...
> > 
> > > +    {
> > > +        "id": "dc6b",
> > > +        "name": "Add tunnel_key set action with missing mandatory src_ip parameter",
> > > +        "category": [
> > > +            "actions",
> > > +            "tunnel_key"
> > > +        ],
> 
>                "reqModules": [
>                    "CONFIG_NET_ACT_TUNNEL_KEY"
>                ],
> > > +        "setup": [
> > > +            [
> > > +                "$TC actions flush action tunnel_key",
> > > +                0,
> > > +                1,
> > > +                255
> > > +            ]
> > > +        ],
> > > +        "cmdUnderTest": "$TC actions add action tunnel_key set dst_ip 20.20.20.2 id 100",
> > > +        "expExitCode": "255",
> > > +        "verifyCmd": "$TC actions list action tunnel_key",
> > > +        "matchPattern": "action order [0-9]+: tunnel_key set.*dst_ip 20.20.20.2.*key_id 100",
> > > +        "matchCount": "0",
> > > +        "teardown": [
> > > +            "$TC actions flush action tunnel_key"
> > > +        ]
> > > +    },
> 
> As we venture into more and more complicated tests, where different
> modules would start getting mixed together, this might be the most
> effective route.
> 
> This plugin will require some changes I've made to our local version
> of tdc that I've been testing out - they change the way tdc handles
> its test results, and also give it the ability to skip tests without
> affecting the rest of the test run.

LGTM.  To maintain the possibility to test automatic module loading based
on the action, we only need to add another test per module (tipically the
first one) where the 'reqModules' line is not present.

> Until I'm able to submit everything, I'd be OK with having Keara add
> the non-zero exit codes to the teardown on her tests.  In the meantime
> we'll get the README updated and config file added as well.
> 
> How does this sound?

it sounds good to me, but at this point we can also leave the code of
tunnel_key as-is. there are many other items failing in this script:

for act in $ACT; do
while IFS=':' read -r id _ ; do modprobe -r act_${act} ; sleep 1 ; [ -n "$id" ] && ./tdc.py -p /home/davide/iproute2/tc/tc -e $id ; done <<EOF
`./tdc.py -l | grep ${act}`
EOF
done

So, it's ok for me if they are fixed all together in a series, and I
volunteer for testing it when they land on netdev list.

regards,
-- 
davide