From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by passt.top (Postfix) with ESMTP id 87CF65A026F for ; Wed, 5 Jul 2023 02:31:01 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1688517060; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=AWJckcvjrX+o88rMsagvWy5vGambc8W7kW68xVEPFWc=; b=Ua6QcZqxU+NtEudlw/OwmNbS3fEKcIScLK7Id8maTKJ2mnwqY6PaYqxiBIuvLesTCSUstU vfpPIxeV1NRQPIw+MqsDCo8waEBsmH8uapUo2/80cUVa+aVv8su+fLomiGjb4ZNE/bfUyB ZN9wz3a34JDjEZY3Oxrm8KlbJ0KQ2FM= Received: from mimecast-mx02.redhat.com (mimecast-mx02.redhat.com [66.187.233.88]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-572-Tv76e4LfMQWuQaUWfZxhww-1; Tue, 04 Jul 2023 20:30:56 -0400 X-MC-Unique: Tv76e4LfMQWuQaUWfZxhww-1 Received: from smtp.corp.redhat.com (int-mx05.intmail.prod.int.rdu2.redhat.com [10.11.54.5]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 081418007CE; Wed, 5 Jul 2023 00:30:56 +0000 (UTC) Received: from elisabeth (unknown [10.39.208.37]) by smtp.corp.redhat.com (Postfix) with ESMTPS id CAE7CF6433; Wed, 5 Jul 2023 00:30:54 +0000 (UTC) Date: Wed, 5 Jul 2023 02:30:48 +0200 From: Stefano Brivio To: David Gibson Subject: Re: [PATCH 27/27] avocado: Convert basic pasta tests Message-ID: <20230705023048.108f3c46@elisabeth> In-Reply-To: <20230627025429.2209702-28-david@gibson.dropbear.id.au> References: <20230627025429.2209702-1-david@gibson.dropbear.id.au> <20230627025429.2209702-28-david@gibson.dropbear.id.au> Organization: Red Hat MIME-Version: 1.0 X-Scanned-By: MIMEDefang 3.1 on 10.11.54.5 X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Message-ID-Hash: PVH5ZKF3SH3UG2NI7UOXWNQA5UQLMDYR X-Message-ID-Hash: PVH5ZKF3SH3UG2NI7UOXWNQA5UQLMDYR X-MailFrom: sbrivio@redhat.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; emergency; loop; banned-address; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: passt-dev@passt.top, crosa@redhat.com, jarichte@redhat.com X-Mailman-Version: 3.3.8 Precedence: list List-Id: Development discussion and patches for passt Archived-At: Archived-At: List-Archive: List-Archive: List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: Sorry for the delay, some (partial) feedback and a few ideas: On Tue, 27 Jun 2023 12:54:28 +1000 David Gibson 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: >=20 > * 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 tha= t > 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 simulat= ed > 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 > --- > 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 >=20 > diff --git a/oldtest/run b/oldtest/run > index a16bc49b..f1157f90 100755 > --- a/oldtest/run > +++ b/oldtest/run > @@ -70,13 +70,13 @@ run() { > =09test build/clang_tidy > =09teardown build > =20 > -#=09setup pasta > -#=09test pasta/ndp > -#=09test pasta/dhcp > -#=09test pasta/tcp > -#=09test pasta/udp > -#=09test passt/shutdown > -#=09teardown pasta > +=09setup pasta > +=09test pasta/ndp > +=09test pasta/dhcp > +=09test pasta/tcp > +=09test pasta/udp > +=09test passt/shutdown > +=09teardown pasta > =20 > #=09setup pasta_options > #=09test 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): > =20 > .PHONY: avocado-assets > avocado-assets: nstool small.bin medium.bin big.bin > +=09$(MAKE) -C .. pasta > =20 > .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 > + > +""" > +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 =3D 10002 > +SPLICE_FWD_PORT =3D 10003 > +FWD_OPTS =3D 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=3DFWD_OPTS): Clear. > + with simple_net() as simnet: Also clear. > + with unshare_site('pastans', '-Ucnpf --mount-proc', > + parent=3Dsimnet.simhost, sudo=3DTrue) as guest= ns: 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=3DTrue" 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=3D"$(mktemp -d)") is nested. > + pidfile =3D os.path.join(tmpdir, 'pasta.pid') > + > + with Pasta(simnet.simhost, pidfile, opts, ns=3Dguestns) = 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() { =09context_setup_host unshare =09context_run_bg unshare "unshare -rUnpf ${NSTOOL} hold ${STATESETUP}/ns.h= old" =09context_setup_nstool ns ${STATESETUP}/ns.hold =09# Ports: =09# =09# ns | host =09# ------------------|--------------------- =09# 10002 as server | spliced to ns =09# 10003 spliced to init | as server =09context_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=3D'inet6', scope=3D'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 =3D ns.addrs(IFNAME, scope=3D'global') > + assert_eq_unordered(addrs, [IP4, IP6]) > + > + > +@test > +def test_config_net_route4(): > + with pasta_configured() as (_, ns): > + (defroute,) =3D ns.routes4(dst=3D'default') > + gateway =3D ipaddress.ip_address(defroute['gateway']) > + assert_eq(gateway, GW_IP4.ip) > + > + > +@test > +def test_config_net_route6(): > + with pasta_configured() as (simnet, ns): > + (defroute,) =3D ns.routes6(dst=3D'default') > + gateway =3D 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 =3D ns.mtu(IFNAME) > + assert_eq(mtu, 65520) these all make intuitive sense to me. > + > + > +@test_transfers(ip4=3DREMOTE_IP4.ip, ip6=3DREMOTE_IP6.ip, port=3D10000) > +@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=3DIP4.ip, ip6=3DIP6.ip, port=3DIN_FWD_PORT, > + fromip4=3DREMOTE_IP4.ip, fromip6=3DREMOTE_IP6.ip) > +@contextlib.contextmanager > +def inward_transfer(): > + with pasta_configured() as (simnet, ns): > + yield simnet.gw, ns > + > + > +@test_transfers(ip4=3DLOOPBACK4, ip6=3DLOOPBACK6, port=3DSPLICE_FWD_PORT= , > + listenip4=3DLOOPBACK4, listenip6=3DLOOPBACK6) > +@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 (=C3=A0 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). --=20 Stefano