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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 24680 bytes --]

On Wed, Jul 05, 2023 at 02:30:48AM +0200, Stefano Brivio wrote:
> 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.

Yeah, I guess not.  My point here is that given that we're generally
trying to avoid NAT, it seems to me we should primarily test the
no-NAT case, then have specific tests for the NAT cases.  I'll try to
find a better way to phrase that.

> > 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).

Right, I see/share your concern.  To some extent there's an
unavoidable trade-off here.  Avoiding lots of repetition in the test
cases, leads to "code" in some sense: we need something resembling
function calls and loops at least to re-invoke standard test pieces.
That's not to say we can't make it better than it is in this draft.

> > @@ -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.

Fair point.  I was considering making the idiom here:
	from avocado_classless.test import *
Which will import "all" (in fact a curated list) of things from the
module for use here.  I didn't go that way because pylint whinged
about it, but we could suppress that easily enough.

> Of course,
> assert_eq_unordered is not available in the current tests, and we need
> it, plus a syntax to represent that.

I'm not quite sure what you're getting at here.

> 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.

Right... so "LOOPBACK6" is longer than "::1", but not than
"ipaddress.ip_address('::1')".  That is, these helpers produce
something that's in an actual IP address type, rather than a bare
string.  I could try moving to just using strings for IP addresses
throughout the tests, but that might cause some different messes.

> > +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.

Right, I could introduce "from foo import *" conventions for some of
these too.

> > +
> > +
> > +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.

Yeah.  I agree that this is uncomfortably cryptic, however this one I
don't see an easy way to remove.  I heavily use this to conveniently
modify and extend context managers.  It allows two things which are
both very convenient:

1) You can do:

	@contextlib.contextmanager
	def foo(...):
	    setup()

	    yield whatever   # <== actual test runs here

	    cleanup()

this will do the cleanup() even if the test blows up.

2) You can do

	@contextlib.contextmanager
	def bar(...):
	    with other_context_1():
	        with other_context_2():
		    yield whatever    # <=== actual test runs here

This effectively makes this a combination of the two existing
contexts, again ensuring that both their cleanup happens if the test
blows up.

It's certainly possible to do this with direct implementations of the
contextmanager protocol but it's much more verbose, and not
significantly less esoteric.

> > +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.

Hmm.. I'm not sure what's throwing you on this specifically.  We need
both pieces of setup for our test, so we need to nest them within each
other.  They have to go in this order because the parameters to
unshare_site() need something from the simnet.

> Those parameters require a complete understanding of unshare_site(),

Hm, ok.  I could require that this takes keyword arguments, so it
would be something like:
    unshare_site(name='pastans', unshare_opts='-Ucnpf --mount-proc',
                 ..)

Would that help?	

> 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).

Yeah.  And it's arguably not entirely accurate either.  I could rework
the 'site' stuff so that I call this, say, 'capable' or 'keep_caps'
instead.  Would that help?

> So far, what this says is:
> 
> - use addressing from simple_net()

Hrm, it's more than that: simple_net() is actually *creating* the
simulated simple network (and cleaning it up once the with clause
ends).

> - detach user and network namespace, remounting proc in it, preserve (I
>   guess?) credentials

Well, if by "preserve credentials" you mean "keep the capabilities you
have because you own the namespace", yes (this is equivalent to
"nsenter --keep-caps", not "nsenter --preserve-credentials").

> 
> > +            with tempfile.TemporaryDirectory() as tmpdir:
> 
> ...even less intuitive why this statement (tmpdir="$(mktemp -d)") is
> nested.

So it's not quite equivalent to tmpdir=$(mktemp -d): it does that at
the start of the with, but it also removes the directory at the end of
the with.

We need the network constructed, the namespace created and the
temporary directory available all at the same time.  So the scope of
each 'with' context needs to overlap - in other words to be nested.
In this case the order of nesting doesn't matter, we could put the
temporary directory 'with' on the outside.

> 
> > +                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.

Yes, specifically, start pasta with it's "host" being simnet.simhost,
and its "guest" being guestns.  Again I could enforce keyword
parameters to make the meaning of the arguments a bit clearer.

[Aside, I plan to extent Pasta() so that if you don't pass a guest ns
it will default to having pasta create its own - that ns will still be
available as 'pasta.ns', so the rest of the stuff can remain the same]

Then the "yield" essentially means "now that we've done the setup,
here is where to run the actual specific test".

> So, from my expectation, pseudocode:
> 
> setup:
>   net: simple_net
>   pasta_options: -t 10002 -T 10003 -u 10002 -U 10003
>   start pasta

Hrm, that pseudocode assumes a pretty specific set of possible setups,
and how they relate to each other.  For example I can't see how it
would extend to running two different pasta instances on different
simulated hosts.  That's possible with the approach I'm using here.

> 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)"
> }

It's replacing not just that, but also the matching teardown function,
and ensures that the teardown runs if there's an exception in the test
itself (including running the teardowns for the outer contexts if the
setup of the inner contexts failed).

> 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.

I'm just not convinced that it would be, without losing a great deal
of the flexibility in this system.  Or at least, not clearer than a
more polished version of this proposal.

> > +
> > +
> > +@test
> > +def test_ifname():
> 
> I guess fine (even though it's missing a descriptive name _inline_).

We could put a description in a docstring easily enough.

> > +    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:

Yeah.  One of the bits I'm least happy with is what to do in cases
where the information the setup needs to pass to the tests proper is
quite complex: so here it has both the simulated network which is
hosting the pasta instance, and also the namespace within pasta.  This
specific test doesn't need the former, hence the _.  We could use a
suitably named dummy variable which might be a bit more readable,
though it would need a lint suppression (unused variable).

I've considered another approach, though I'm not sure it's an
improvement, which is to use more intermediate structs/classes.  So,
in that approach pasta_unconfigured() would give you a
'PastaTestScenario' instance with fields for the guest ns, host ns and
whatever else seems useful.

> 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

It's more than that though: we're also requesting a very specific
(simulated) environment for the test, which the pseudocode above just
kind of assumes.  'pasta_unconfigured()' (name certainly negotiable)
is handling the setup and teardown of everything we need for a certain
class of pasta tests.  That has 3 namespaces, a veth, and a pasta
instance, all configured in a particular way.

> ...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).

So the OO approach works ok for pretty simple setups - probably why it
became popular with jUnit etc: the class handles the setup, the
methods do the specific tests.  But it breaks down pretty quickly:
   - If you want to use multiple "setup" pieces at once you need to
     use multiple inheritence, which is a nightmare
   - If you want to parameterize parts of the setup, you need to do it
     at the whole class level which involves wierd non-local
     interactions
   - If you want to use multiple setup pieces which are actually the
     same piece repeated with different parameters, you're pretty much
     out of luck

I realize that context managers aren't the most obvious thing, but
they have the huge advantage that every test can directly and
reasonably tersely declare what setup they need to run:

    def test():
	with setup_i_need():
	    with other_setup_i_need():
	       with setup_i_need(param='non default'):
	           # actually do the test

This also handles the necessary teardown at each level, and it still
works if some of the inner setups need paramaters that are derived
fromt the outer setups.

> 
> > +        assert_eq_unordered(ns.ifs(), ('lo', IFNAME))
> 
> Clear (probably clearer with a "list includes" operator).

It's not the same as list includes (that's assert_in()).  This checks
that the lists (strictly, iterables) are identical ignoring order.  So
in this case ['lo', IFNAME] and [IFNAME, 'lo'] would be ok, but ['lo',
'eth99', IFNAME] would not be.

> > +
> > +
> > +@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.

Yeah, I'm not super happy with this one.  In particular I dislike the
inconsistency: in simple tests like the above we have, loosely the
setup, followed by the test.  In these composed examples we have the
tests then the setup.  So combining a few options I've thought of, I
could do something like this:

	@contextlib.contextmanager
	def pasta_ndp():
	    with pasta_unconfigured() as (simnet, guestns):
	        guestns.ifup(IFNAME)
	        yield NdpTestScenario(ifname=IFNAME, net=HOST_NET6,
		                      router=simnet.gw_ip6_ll.ip,
				      ndpclient=guestns)

	compose_matrix([pasta_ndp], NDP_TESTS)

Here NDP_TESTS would come from the ndp module and have the list of
individual ndp test cases, each of which takes an NdpTestScenario as
parameter.

> > +
> > +@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).

Right.  So would the treatment above help?  Enough?  It would look
something like:

	@contextlib.contextmanager
	def pasta_dhcp():
	    ....
	    yield DhcpTestScenario(ifname=IFNAME, client_ip4=IP4.ip,
	                           server_ip4=GW_IP4.ip, mtu=65520)

	compose_matrix([pasta_dhcp], DHCP_TESTS)

> > +@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.

It is needed, and yes we probably should, I haven't had a chance to
look into what, exactly.  We may hit that kernel bug I noticed where
permissions to the disable_dad sysctl seem to behave weirdly.

> > +        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.

I'm not entirely sure what you mean here.  Would the same treatment as
for the ndp and dhcp cases above help?

	@contextlib.contextmanager
	def outward_transfer():
	    with pasta_configured() as (simnet, ns):
	        yield TransferTestScenario(client=ns,
	                                   server=simnet.gw,
	                                   connect_ip4=REMOTE_IP4.ip,
	                                   connect_ip6=REMOTE_IP6.ip,
					   connect_port=10000)

	# variants for inward transfer, and spliced_transfer
	compose_matrix([outward_transfer, inward_transfer, spliced_transfer],
	               TRANSFER_TESTS)

> > +
> > +
> > +@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.

So, I was only thinking of pasta_configured() and pasta_unconfigured()
as useful for the pretty specific pattern of pasta invocations in this
batch of tests (which is why they're local to that file).  I've also
had at pasta_options (which I'm thinking of as the log-to-file tests,
since that's all they test so far).  I was planning a new helper to
set up a suitable scenario for that, which would be based on
simple_net() and Pasta() with some kind of parameterization for where
to place the logfile.

> All we need for that is:
> 
> - define options and network model/addressing

I think that short statement hides a lot of complexity.

> - 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).

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]