Re: [PATCH 27/27] avocado: Convert basic pasta tests

[Stefano Brivio <sbrivio@redhat.com>]

Sorry for the delay, some (partial) feedback and a few ideas:

On Tue, 27 Jun 2023 12:54:28 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> Convert the old-style tests for pasta (DHCP, NDP, TCP and UDP transfers)
> to using avocado.  There are a few differences in what we test, but this
> should generally improve coverage:
> 
>  * We run in a constructed network environment, so we no longer depend on
>    the real host's networking configuration
>  * We do independent setup for each individual test
>  * We add explicit tests for --config-net, which we use to accelerate that
>    setup for the TCP and UDP tests
>  * The TCP and UDP tests now test transfers between the guest and a
>    (simulated) remote site that's on a different network from the simulated
>    pasta host.  This better matches the typical passt/pasta usecase

...this is not necessarily true -- it really depends, but sure, it's
important to have this too.

> Signed-off-by: David Gibson <david@gibson.dropbear.id.au>
> ---
>  oldtest/run                   |  14 ++--
>  test/Makefile                 |   1 +
>  test/avocado/pasta.py         | 129 ++++++++++++++++++++++++++++++++++
>  test/lib/layout               |  31 --------
>  test/lib/setup                |  40 -----------
>  test/pasta/dhcp               |  46 ------------
>  test/pasta/ndp                |  33 ---------
>  test/pasta/tcp                |  96 -------------------------
>  test/pasta/udp                |  59 ----------------
>  test/run                      |   8 ---
>  test/tasst/dhcpv6.py          |   4 +-
>  test/tasst/pasta.py           |  42 +++++++++++
>  test/tasst/scenario/simple.py |  44 ++++++------
>  13 files changed, 203 insertions(+), 344 deletions(-)
>  create mode 100644 test/avocado/pasta.py
>  delete mode 100644 test/pasta/dhcp
>  delete mode 100644 test/pasta/ndp
>  delete mode 100644 test/pasta/tcp
>  delete mode 100644 test/pasta/udp
>  create mode 100644 test/tasst/pasta.py
> 
> diff --git a/oldtest/run b/oldtest/run
> index a16bc49b..f1157f90 100755
> --- a/oldtest/run
> +++ b/oldtest/run
> @@ -70,13 +70,13 @@ run() {
>  	test build/clang_tidy
>  	teardown build
>  
> -#	setup pasta
> -#	test pasta/ndp
> -#	test pasta/dhcp
> -#	test pasta/tcp
> -#	test pasta/udp
> -#	test passt/shutdown
> -#	teardown pasta
> +	setup pasta
> +	test pasta/ndp
> +	test pasta/dhcp
> +	test pasta/tcp
> +	test pasta/udp
> +	test passt/shutdown
> +	teardown pasta
>  
>  #	setup pasta_options
>  #	test pasta_options/log_to_file
> diff --git a/test/Makefile b/test/Makefile
> index 953eacf2..9c3114c2 100644
> --- a/test/Makefile
> +++ b/test/Makefile
> @@ -227,6 +227,7 @@ $(VENV):
>  
>  .PHONY: avocado-assets
>  avocado-assets: nstool small.bin medium.bin big.bin
> +	$(MAKE) -C .. pasta
>  
>  .PHONY: avocado
>  avocado: avocado-assets $(VENV)
> diff --git a/test/avocado/pasta.py b/test/avocado/pasta.py
> new file mode 100644
> index 00000000..891313f5
> --- /dev/null
> +++ b/test/avocado/pasta.py

I'm just focusing on this for the moment, as this is the part I'm mostly
concerned about (writing tests), with a particular accent on what makes
this (still) read-only _code_ for me, and the gaps with the kind of
things I would have naturally expected to write.

That is, I'm mostly isolating negative aspects here.

I'm stressing "code" because I also would have the natural expectation
that it should/must be simpler to _write_ (not necessarily design) tests
rather than the code that end users use, because we can use whatever
language with no strict (only some) constraints on resources and speed.
So in my opinion it doesn't necessarily need to be "code" (and a
general feeling I have from this is that it really is code).

> @@ -0,0 +1,129 @@
> +#! /usr/bin/env avocado-runner-avocado-classless
> +
> +# SPDX-License-Identifier: GPL-2.0-or-later
> +#
> +# Copyright Red Hat
> +# Author: David Gibson <david@gibson.dropbear.id.au>
> +
> +"""
> +avocado/pasta.py - Basic tests for pasta mode
> +"""
> +
> +import contextlib
> +import ipaddress
> +import os
> +import tempfile

So far so good, I probably don't need to read up about every single
import.

> +from avocado_classless.test import assert_eq, assert_eq_unordered, test

Probably a future source of (needless) copy-and-paste. Of course,
assert_eq_unordered is not available in the current tests, and we need
it, plus a syntax to represent that.

Still, conceptually speaking, we shouldn't need this kind of import if
we are writing _a test for passt in the test suite for passt itself_,
and we want to check that the list of interfaces matches "lo" -- it's
something we'll need basically everywhere anyway.

> +from tasst.address import LOOPBACK4, LOOPBACK6

On one hand, probably more readable than a 127.0.0.1 here and there
(even though longer than "::1"), on the other hand there are 256
loopback addresses in IPv4. And there is only one way of writing "::1",
but many ways of remembering/misspelling LOOPBACK6.

> +from tasst.dhcp import test_dhcp
> +from tasst.dhcpv6 import test_dhcpv6
> +from tasst.ndp import test_ndp
> +from tasst.nstool import unshare_site
> +from tasst.pasta import Pasta
> +from tasst.scenario.simple import (
> +    simple_net, IFNAME, HOST_NET6, IP4, IP6, GW_IP4, REMOTE_IP4, REMOTE_IP6
> +)
> +from tasst.transfer import test_transfers

Similar for this -- it would be great to have an infrastructure where
we can just use what we need without looking it up. Rather minor though.

> +
> +
> +IN_FWD_PORT = 10002
> +SPLICE_FWD_PORT = 10003
> +FWD_OPTS = f'-t {IN_FWD_PORT} -u {IN_FWD_PORT} \
> +             -T {SPLICE_FWD_PORT} -U {SPLICE_FWD_PORT}'

I think clear enough.

> +
> +@contextlib.contextmanager

About this decorator: I now have a vague idea what it means after
reading the full series and some Avocado documentation... but I don't
find that very descriptive.

> +def pasta_unconfigured(opts=FWD_OPTS):

Clear.

> +    with simple_net() as simnet:

Also clear.

> +        with unshare_site('pastans', '-Ucnpf --mount-proc',
> +                          parent=simnet.simhost, sudo=True) as guestns:

If I ignore for a moment the implementation detail, it's not very
intuitive why this is nested in the "with" statement before.

Those parameters require a complete understanding of unshare_site(),
and... I now know what "sudo=True" means in Avocado, but I don't
even have sudo on my machine, so at a distracted look I would think I
need to install it and configure sudoers (which would make me as a
developer skip the tests).

So far, what this says is:

- use addressing from simple_net()
- detach user and network namespace, remounting proc in it, preserve (I
  guess?) credentials

> +            with tempfile.TemporaryDirectory() as tmpdir:

...even less intuitive why this statement (tmpdir="$(mktemp -d)") is
nested.

> +                pidfile = os.path.join(tmpdir, 'pasta.pid')
> +
> +                with Pasta(simnet.simhost, pidfile, opts, ns=guestns) as pasta:
> +                    yield simnet, pasta.ns

...and this finally says: start pasta in that (user? network?)
namespace.

So, from my expectation, pseudocode:

setup:
  net: simple_net
  pasta_options: -t 10002 -T 10003 -u 10002 -U 10003
  start pasta

and from my understanding of what this is replacing:

setup_pasta() {
	context_setup_host unshare

	context_run_bg unshare "unshare -rUnpf ${NSTOOL} hold ${STATESETUP}/ns.hold"

	context_setup_nstool ns ${STATESETUP}/ns.hold

	# Ports:
	#
	#                 ns        |         host
	#         ------------------|---------------------
	#  10002      as server     |    spliced to ns
	#  10003   spliced to init  |      as server

	context_run_bg passt "./pasta -f -t 10002 -T 10003 -u 10002 -U 10003 -P ${STATESETUP}/passt.pid $(${NSTOOL} info -pw ${STATESETUP}/ns.hold)"
}

which I actually find less elegant but much clearer, especially before
reading the whole series, I wonder if we could have either a
configuration format, or directives... but, disappointingly, I couldn't
decide on a more detailed proposal yet.

Still, I would aim at something resembling that
pseudocode/configuration above. Of course, with no details it looks
neat, so the comparison is unfair, but I'm under the impression that
even after adding details (that's what I'm trying to figure
out/articulate) it should still be much clearer.

> +
> +
> +@test
> +def test_ifname():

I guess fine (even though it's missing a descriptive name _inline_).

> +    with pasta_unconfigured() as (_, ns):

Ouch... a bit hard to grasp for people not familiar with Python or
Go... plus, conceptually, we _just_ want to say either:

test: "pasta namespace has the interface name we configured"
  enter namespace
  list of interfaces includes $NAME

or:

test: "pasta namespace has a loopback interface"
  list of interfaces in namespace includes $NAME

...and yes, that requires that we start pasta first, but I wonder if it
makes sense to mix the two notions (i.e. whether an object-oriented
approach makes sense at all, actually).

> +        assert_eq_unordered(ns.ifs(), ('lo', IFNAME))

Clear (probably clearer with a "list includes" operator).

> +
> +
> +@test_ndp(IFNAME, HOST_NET6)
> +@contextlib.contextmanager
> +def pasta_ndp():
> +    with pasta_unconfigured() as (simnet, guestns):
> +        guestns.ifup(IFNAME)
> +        yield guestns, simnet.gw_ip6_ll.ip

This really hides what we are checking. Other than that (but it's a big
issue in my opinion) I find it terse and elegant.

> +
> +@test_dhcp(IFNAME, IP4.ip, GW_IP4.ip, 65520)

The test_ndp decorator is probably usable, this one not so much. As we
have more parameters, it would probably be better to have something
more descriptive (even if necessarily more verbose).

> +@test_dhcpv6(IFNAME, IP6.ip)
> +@contextlib.contextmanager
> +def pasta_dhcp():
> +    with pasta_unconfigured() as (_, guestns):
> +        yield guestns
> +
> +
> +@contextlib.contextmanager
> +def pasta_configured():
> +    with pasta_unconfigured(FWD_OPTS + ' --config-net') as (simnet, ns):
> +        # Wait for DAD to complete on the --config-net address

Side note: this shouldn't be needed -- if it is, we should probably fix
something in pasta.

> +        ns.addr_wait(IFNAME, family='inet6', scope='global')
> +        yield simnet, ns

Except for the common points with my observation above, this looks
usable, and:

> +
> +
> +@test
> +def test_config_net_addr():
> +    with pasta_configured() as (_, ns):
> +        addrs = ns.addrs(IFNAME, scope='global')
> +        assert_eq_unordered(addrs, [IP4, IP6])
> +
> +
> +@test
> +def test_config_net_route4():
> +    with pasta_configured() as (_, ns):
> +        (defroute,) = ns.routes4(dst='default')
> +        gateway = ipaddress.ip_address(defroute['gateway'])
> +        assert_eq(gateway, GW_IP4.ip)
> +
> +
> +@test
> +def test_config_net_route6():
> +    with pasta_configured() as (simnet, ns):
> +        (defroute,) = ns.routes6(dst='default')
> +        gateway = ipaddress.ip_address(defroute['gateway'])
> +        assert_eq(gateway, simnet.gw_ip6_ll.ip)
> +
> +
> +@test
> +def test_config_net_mtu():
> +    with pasta_configured() as (_, ns):
> +        mtu = ns.mtu(IFNAME)
> +        assert_eq(mtu, 65520)

these all make intuitive sense to me.

> +
> +
> +@test_transfers(ip4=REMOTE_IP4.ip, ip6=REMOTE_IP6.ip, port=10000)
> +@contextlib.contextmanager
> +def outward_transfer():
> +    with pasta_configured() as (simnet, ns):
> +        yield ns, simnet.gw

This, however, not. I would naturally expect the function implementing
a template of data transfer test to do something with data.

> +
> +
> +@test_transfers(ip4=IP4.ip, ip6=IP6.ip, port=IN_FWD_PORT,
> +                fromip4=REMOTE_IP4.ip, fromip6=REMOTE_IP6.ip)
> +@contextlib.contextmanager
> +def inward_transfer():
> +    with pasta_configured() as (simnet, ns):
> +        yield simnet.gw, ns
> +
> +
> +@test_transfers(ip4=LOOPBACK4, ip6=LOOPBACK6, port=SPLICE_FWD_PORT,
> +                listenip4=LOOPBACK4, listenip6=LOOPBACK6)
> +@contextlib.contextmanager
> +def spliced_transfer():
> +    with pasta_configured() as (simnet, ns):
> +        yield ns, simnet.simhost

I still have to decode these before I can reasonably comment on them.

I'll follow up with a more detailed proposal of a possible
configuration format or perhaps something between a domain-specific
language and an annotated Python script (à la bats), but I'm trying to
consider a few more tests on top of these.

I started looking at "options" tests next, and realised my proposal was
a bit too simplistic. But also that the current version of those tests,
while somewhat verbose and surely clunky, shows in a very clear way
what we're checking and what we expect... and I'm not sure that the
outcome from extending pasta_configured() would be very usable.

All we need for that is:

- define options and network model/addressing

- have a clear syntax of identifying where pasta is starting

- a list of commands (which, themselves, are absolutely obvious and
  natural in a shell script, but I see the value of using Python
  features to check assertions, at least).

-- 
Stefano