router: newbenchmark improvements.

Moved test cases into home dir and implement a number of reviewer's suggestions.

acceptance/router_newbenchmark/BUILD.bazel

@@ -6,14 +6,11 @@ load("//:scion.bzl", "scion_go_binary")
 
 go_library(
     name = "go_default_library",
-    srcs = [
-        "cases.go",
-        "main.go",
-        "topo.go",
-    ],
+    srcs = ["main.go"],
     importpath = "github.com/scionproto/scion/acceptance/router_newbenchmark",
     visibility = ["//visibility:private"],
     deps = [
+        "//acceptance/router_newbenchmark/cases:go_default_library",
         "//pkg/addr:go_default_library",
         "//pkg/log:go_default_library",
         "//pkg/private/serrors:go_default_library",
@@ -39,14 +36,11 @@ scion_go_binary(
 exports_files([
     "conf",
     "test.py",
-    "pause.tar",
 ])
 
 args = [
     "--executable",
     "brload:$(location //acceptance/router_newbenchmark:brload)",
-    "--container-loader",
-    "posix-router:latest#$(location //acceptance/router_newbenchmark:router)",
 ]
 
 data = [
@@ -69,9 +63,3 @@ container_image(
     name = "router",
     base = "//docker:posix_router",
 )
-
-go_binary(
-    name = "router_newbenchmark",
-    embed = [":go_default_library"],
-    visibility = ["//visibility:public"],
-)

acceptance/router_newbenchmark/cases/BUILD.bazel

@@ -0,0 +1,22 @@
+load("@io_bazel_rules_go//go:def.bzl", "go_binary")
+load("//tools/lint:go.bzl", "go_library")
+
+go_library(
+    name = "go_default_library",
+    srcs = [
+        "br_transit.go",
+        "topo.go",
+    ],
+    visibility = ["//acceptance/router_newbenchmark:__pkg__"],
+    importpath = "github.com/scionproto/scion/acceptance/router_newbenchmark/cases",
+    deps = [
+        "//pkg/addr:go_default_library",
+        "//pkg/private/util:go_default_library",
+        "//pkg/slayers:go_default_library",
+        "//pkg/slayers/path:go_default_library",
+        "//pkg/slayers/path/scion:go_default_library",
+        "//pkg/private/xtest:go_default_library",
+        "@com_github_google_gopacket//:go_default_library",
+        "@com_github_google_gopacket//layers:go_default_library",
+    ],
+)

acceptance/router_newbenchmark/cases.go → acceptance/router_newbenchmark/cases/br_transit.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package main
+package cases
 
 import (
 	"hash"

acceptance/router_newbenchmark/topo.go → acceptance/router_newbenchmark/cases/topo.go

@@ -12,7 +12,7 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-package main
+package cases
 
 import (
 	"fmt"
@@ -107,10 +107,10 @@ var (
 	macAddrs map[netip.Addr]net.HardwareAddr = map[netip.Addr]net.HardwareAddr{}
 )
 
-// initInterfaces collects the names and mac addresses for the interfaces setup by the invoker
+// InitInterfaces collects the names and mac addresses for the interfaces setup by the invoker
 // according to instructions given via listInterfaces().
 // This information is indexed by our own interface labels.
-func initInterfaces(pairs []string) {
+func InitInterfaces(pairs []string) {
 	for _, pair := range pairs {
 		p := strings.Split(pair, "=")
 		label := p[0]
@@ -160,7 +160,7 @@ func hostAddr(ip netip.Addr) addr.Host {
 // to set up the network accordingly before executing the test without that option.
 // We do not choose interface names or mac addresses those will be provided by the invoker
 // via the --interfaces options.
-func listInterfaces() string {
+func ListInterfaces() string {
 	var sb strings.Builder
 	for l, i := range intfMap {
 		sb.WriteString(l)
@@ -172,6 +172,7 @@ func listInterfaces() string {
 		sb.WriteString(i.peerIP.String())
 		sb.WriteString("\n")
 	}
+	// "our_label,24,<ip on router side>,<ip on far side>\n"
 
 	return sb.String()
 }

acceptance/router_newbenchmark/main.go

@@ -29,6 +29,7 @@ import (
 	"github.com/google/gopacket/afpacket"
 	"github.com/google/gopacket/layers"
 
+	"github.com/scionproto/scion/acceptance/router_newbenchmark/cases"
 	"github.com/scionproto/scion/pkg/log"
 	"github.com/scionproto/scion/pkg/private/serrors"
 	"github.com/scionproto/scion/pkg/scrypto"
@@ -52,7 +53,7 @@ type Case func(payload string, mac hash.Hash, numDistinct int) (string, string,
 
 var (
 	allCases = map[string]Case{
-		"br_transit": BrTransit,
+		"br_transit": cases.BrTransit,
 	}
 	logConsole = flag.String("log.console", "debug", "Console logging level: debug|info|error")
 	dir        = flag.String("artifacts", "", "Artifacts directory")
@@ -63,29 +64,31 @@ var (
 			reflect.ValueOf(allCases).MapKeys()))
 	showIntf   = flag.Bool("show_interfaces", false, "Show interfaces needed by the test")
 	interfaces = arrayFlags{}
-	handles    = make(map[string]*afpacket.TPacket)
 )
 
 // initDevices inventories the available network interfaces, picks the ones that a case may inject
-// traffic into, and associates them with a AF Packet interface.
-func initDevices() error {
+// traffic into, and associates them with a AF Packet interface. It returns the packet interfaces
+// corresponding to each network interface.
+func openDevices() (map[string]*afpacket.TPacket, error) {
 	devs, err := net.Interfaces()
 	if err != nil {
-		return serrors.WrapStr("listing network interfaces", err)
+		return nil, serrors.WrapStr("listing network interfaces", err)
 	}
 
+	handles := make(map[string]*afpacket.TPacket)
+
 	for _, dev := range devs {
 		if !strings.HasPrefix(dev.Name, "veth_") || !strings.HasSuffix(dev.Name, "_host") {
 			continue
 		}
 		handle, err := afpacket.NewTPacket(afpacket.OptInterface(dev.Name))
 		if err != nil {
-			return serrors.WrapStr("creating TPacket", err)
+			return nil, serrors.WrapStr("creating TPacket", err)
 		}
 		handles[dev.Name] = handle
 	}
 
-	return nil
+	return handles, nil
 }
 
 func main() {
@@ -100,7 +103,7 @@ func realMain() int {
     peer_mac: the mac address of <host_interface>`)
 	flag.Parse()
 	if *showIntf {
-		fmt.Println(listInterfaces())
+		fmt.Println(cases.ListInterfaces())
 		return 0
 	}
 
@@ -119,25 +122,24 @@ func realMain() int {
 		return 1
 	}
 
-	artifactsDir, err := os.MkdirTemp("", "brload_")
-	if err != nil {
-		log.Error("Cannot create tmp dir", "err", err)
-		return 1
-	}
-	if *dir != "" {
-		artifactsDir = *dir
-	}
+	artifactsDir := *dir
 	if v := os.Getenv("TEST_ARTIFACTS_DIR"); v != "" {
 		artifactsDir = v
 	}
+
+	if artifactsDir == "" {
+		log.Error("Artifacts directory not configured")
+		return 1
+	}
+
 	hfMAC, err := loadKey(artifactsDir)
 	if err != nil {
 		log.Error("Loading keys failed", "err", err)
 		return 1
 	}
 
-	initInterfaces(interfaces)
-	err = initDevices()
+	cases.InitInterfaces(interfaces)
+	handles, err := openDevices()
 	if err != nil {
 		log.Error("Loading devices failed", "err", err)
 		return 1
@@ -170,34 +172,7 @@ func realMain() int {
 
 	go func() {
 		defer log.HandlePanic()
-		numRcv := 0
-
-		defer func() {
-			listenerChan <- numRcv
-			close(listenerChan)
-		}()
-
-		for {
-			got, ok := <-packetChan
-			if !ok {
-				// No more packets
-				log.Info("No more Packets")
-				return
-			}
-			if err := got.ErrorLayer(); err != nil {
-				log.Error("error decoding packet", "err", err)
-				continue
-			}
-			layer := got.Layer(gopacket.LayerTypePayload)
-			if layer == nil {
-				log.Error("error fetching packet payload: no PayLoad")
-				continue
-			}
-			if string(layer.LayerContents()) == payloadString {
-				numRcv++
-				return
-			}
-		}
+		receivePackets(packetChan, payloadString, listenerChan)
 	}()
 
 	// We started everything that could be started. So the best window for perf mertics
@@ -213,23 +188,65 @@ func realMain() int {
 	// The test harness looks for this output.
 	fmt.Printf("metricsBegin: %d metricsEnd: %d\n", metricsBegin, metricsEnd)
 
-	// In short tests (<1M packets), we finish sending before the first packets arrive.
-	time.Sleep(time.Second * time.Duration(1))
-
-	// If our listener is still stuck there, unstick it. Closing the device doesn't cause the
-	// packet channel to close (presumably a bug). Close the channel ourselves.
-	close(packetChan)
-
-	outcome := <-listenerChan
-	if outcome == 0 {
-		log.Error("Listener never saw a valid packet being forwarded")
-		return 1
+	// Get the results from the packet listener.
+	// Give it one second as in very short tests (<1M pkts) we get here before the first packet.
+	outcome := 0
+	timeout := time.After(1 * time.Second)
+	for outcome == 0 {
+		select {
+		case outcome = <-listenerChan:
+			if outcome == 0 {
+				log.Error("Listener never saw a valid packet being forwarded")
+				return 1
+			}
+		case <-timeout:
+			// If our listener is still stuck there, unstick it. Closing the device doesn't cause the
+			// packet channel to close (presumably a bug). Close the channel ourselves.
+			// After this, the next loop is guaranteed an outcome.
+			close(packetChan)
+		}
 	}
 
 	fmt.Printf("Listener results: %d\n", outcome)
 	return 0
 }
 
+// receivePkts consume some or all (at least one if it arrives) of the packets
+// arriving on the given handle and checks that they contain the given payload.
+// The number of consumed packets is returned via the given outcome channel.
+// Currently we are content with receiving a single correct packet and we terminate after
+// that.
+func receivePackets(packetChan chan gopacket.Packet, payload string, outcome chan int) {
+	numRcv := 0
+
+	defer func() {
+		outcome <- numRcv
+		close(outcome)
+	}()
+
+	for {
+		got, ok := <-packetChan
+		if !ok {
+			// No more packets
+			log.Info("No more Packets")
+			return
+		}
+		if err := got.ErrorLayer(); err != nil {
+			log.Error("error decoding packet", "err", err)
+			continue
+		}
+		layer := got.Layer(gopacket.LayerTypePayload)
+		if layer == nil {
+			log.Error("error fetching packet payload: no PayLoad")
+			continue
+		}
+		if string(layer.LayerContents()) == payload {
+			numRcv++
+			return
+		}
+	}
+}
+
 // loadKey loads the keys that the router under test uses to sign hop fields.
 func loadKey(artifactsDir string) (hash.Hash, error) {
 	keysDir := filepath.Join(artifactsDir, "conf", "keys")

acceptance/router_newbenchmark/test.py

@@ -103,7 +103,7 @@ class RouterBMTest(base.TestBase):
 
     # Accepts an IntfReq records names and mac addresses into an Intf and associates it with
     # the request label.
-    def create_interface(self, req: IntfReq, ns: str) -> Intf:
+    def create_interface(self, req: IntfReq, ns: str):
         hostIntf = f"veth_{req.label}_host"
         brIntf = f"veth_{req.label}"
         peerMac = mac_for_ip(req.peerIp)
@@ -161,8 +161,7 @@ def setup_prepare(self):
         # We supply the label->(host-side-name,mac,peermac) mapping to brload when we start it.
         self.intfMap = {}
         brload = self.get_executable("brload")
-        output = exec_sudo(f"{brload.executable} -artifacts {self.artifacts} "
-                           "-show_interfaces")
+        output = brload("-artifacts", f"{self.artifacts}", "-show_interfaces")
 
         for line in output.splitlines():
             elems = line.split(",")