router: Implement reviewers suggestions.

Namely: * Doc formating * Simplify end2endblast now that its function is separate from that of end2end. * Restore endend almost completely to its original form for the same reason.
scionproto · Nov 16, 2023 · eea565f · eea565f
1 parent 65a4fe5
commit eea565f
Show file tree

Hide file tree

Showing 6 changed files with 104 additions and 241 deletions.
diff --git a/acceptance/router_benchmark/test.py b/acceptance/router_benchmark/test.py
@@ -137,11 +137,11 @@ def _run(self):
         loadtest = self.get_executable("end2end_integration")
         retCode, stdOut, stdErr = loadtest[
             "-d",
-            "-traces=false",
             "-outDir", self.artifacts,
             "-name", "router_benchmark",
             "-cmd", "./bin/end2endblast",
             "-attempts", 1500000,
+            "-timeout", "90s",  # Timeout is for all attempts together
             "-parallelism", 100,
             "-subset", "noncore#core#remoteISD"
         ].run_tee()
@@ -207,11 +207,11 @@ def _run(self):
         loadtest = self.get_executable("end2end_integration")
         retCode, stdOut, stdErr = loadtest[
             "-d",
-            "-traces=false",
             "-outDir", self.artifacts,
             "-name", "router_benchmark",
             "-cmd", "./bin/end2endblast",
             "-attempts", 1500000,
+            "-timeout", "90s",  # Timeout is for all attempts together
             "-parallelism", 100,
             "-subset", "noncore#noncore#remoteAS"
         ].run_tee()

diff --git a/doc/manuals/router.rst b/doc/manuals/router.rst
@@ -120,8 +120,6 @@ Environment Variables
    :Type: :ref:`duration <common-conf-duration>`
    :Default: ``5m``
 
-.. _gomaxprocs:
-
 .. envvar:: GOMAXPROCS
 
    Specified by the GO runtime. The Go runtime starts a number kernel threads such that the number
@@ -211,7 +209,7 @@ considers the following options.
       experimentaly.
 
       The number of kernel threads that go creates depends on the number of usable cores, which is
-      controlled by the environment variable ``GOMAXPROCS``. See :ref:`GOMAXPROCS <gomaxprocs>`.
+      controlled by the environment variable ``GOMAXPROCS``. See :env:`GOMAXPROCS`.
 
    .. option:: router.num_slow_processors = <int> (Default: 1)
 

diff --git a/tools/end2end/main.go b/tools/end2end/main.go
@@ -1,5 +1,6 @@
 // Copyright 2018 ETH Zurich
 // Copyright 2019 ETH Zurich, Anapaya Systems
+// Copyright 2023 SCION Association
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -101,14 +102,12 @@ func realMain() int {
 	}
 	validateFlags()
 
-	if traces {
-		closeTracer, err := integration.InitTracer("end2end-" + integration.Mode)
-		if err != nil {
-			log.Error("Tracer initialization failed", "err", err)
-			return 1
-		}
-		defer closeTracer()
+	closeTracer, err := integration.InitTracer("end2end-" + integration.Mode)
+	if err != nil {
+		log.Error("Tracer initialization failed", "err", err)
+		return 1
 	}
+	defer closeTracer()
 
 	if integration.Mode == integration.ModeServer {
 		server{}.run()
@@ -137,8 +136,7 @@ func validateFlags() {
 			integration.LogFatal("Invalid timeout provided", "timeout", timeout)
 		}
 	}
-	log.Info("Flags", "traces", traces, "timeout", timeout, "epic", epic,
-		"remote", remote)
+	log.Info("Flags", "timeout", timeout, "epic", epic, "remote", remote)
 }
 
 type server struct {
@@ -202,28 +200,23 @@ func (s server) handlePing(conn snet.PacketConn) error {
 		)
 	}
 
+	spanCtx, err := opentracing.GlobalTracer().Extract(
+		opentracing.Binary,
+		bytes.NewReader(pld.Trace),
+	)
+	if err != nil {
+		return serrors.WrapStr("extracting trace information", err)
+	}
+	span, _ := opentracing.StartSpanFromContext(
+		context.Background(),
+		"handle_ping",
+		ext.RPCServerOption(spanCtx),
+	)
+	defer span.Finish()
 	withTag := func(err error) error {
+		tracing.Error(span, err)
 		return err
 	}
-	if traces {
-		spanCtx, err := opentracing.GlobalTracer().Extract(
-			opentracing.Binary,
-			bytes.NewReader(pld.Trace),
-		)
-		if err != nil {
-			return serrors.WrapStr("extracting trace information", err)
-		}
-		span, _ := opentracing.StartSpanFromContext(
-			context.Background(),
-			"handle_ping",
-			ext.RPCServerOption(spanCtx),
-		)
-		defer span.Finish()
-		withTag = func(err error) error {
-			tracing.Error(span, err)
-			return err
-		}
-	}
 
 	if pld.Message != ping || !pld.Server.Equal(integration.Local.IA) {
 		return withTag(serrors.New("unexpected data in payload",
@@ -309,16 +302,12 @@ func (c *client) run() int {
 func (c *client) attemptRequest(n int) bool {
 	timeoutCtx, cancel := context.WithTimeout(context.Background(), timeout.Duration)
 	defer cancel()
-	ctx := timeoutCtx
-	span, _ := tracing.NilCtx()
 
-	if traces {
-		span, ctx = tracing.CtxWith(timeoutCtx, "attempt")
-		span.SetTag("attempt", n)
-		span.SetTag("src", integration.Local.IA)
-		span.SetTag("dst", remote.IA)
-		defer span.Finish()
-	}
+	span, ctx := tracing.CtxWith(timeoutCtx, "attempt")
+	span.SetTag("attempt", n)
+	span.SetTag("src", integration.Local.IA)
+	span.SetTag("dst", remote.IA)
+	defer span.Finish()
 
 	logger := log.FromCtx(ctx)
 
@@ -328,25 +317,20 @@ func (c *client) attemptRequest(n int) bool {
 		return false
 	}
 
+	span, ctx = tracing.StartSpanFromCtx(ctx, "attempt.ping")
+	defer span.Finish()
 	withTag := func(err error) error {
+		tracing.Error(span, err)
 		return err
 	}
-	if traces {
-		span, ctx = tracing.StartSpanFromCtx(ctx, "attempt.ping")
-		defer span.Finish()
-		withTag = func(err error) error {
-			tracing.Error(span, err)
-			return err
-		}
-	}
 
 	// Send ping
-	if err := c.ping(ctx, n, path, true); err != nil {
+	if err := c.ping(ctx, n, path); err != nil {
 		logger.Error("Could not send packet", "err", withTag(err))
 		return false
 	}
 	// Receive pong
-	if err := c.pong(ctx, true); err != nil {
+	if err := c.pong(ctx); err != nil {
 		tracing.Error(span, err)
 		logger.Error("Error receiving pong", "err", withTag(err))
 		if path != nil {
@@ -357,7 +341,7 @@ func (c *client) attemptRequest(n int) bool {
 	return true
 }
 
-func (c *client) ping(ctx context.Context, n int, path snet.Path, logIfOk bool) error {
+func (c *client) ping(ctx context.Context, n int, path snet.Path) error {
 	rawPing, err := json.Marshal(Ping{
 		Server:  remote.IA,
 		Message: ping,
@@ -402,9 +386,9 @@ func (c *client) ping(ctx context.Context, n int, path snet.Path, logIfOk bool)
 			},
 		},
 	}
-	if logIfOk {
-		log.Info("sending ping", "attempt", n, "path", path)
-	}
+
+	log.Info("sending ping", "attempt", n, "path", path)
+
 	if err := c.conn.WriteTo(pkt, remote.NextHop); err != nil {
 		return err
 	}
@@ -466,7 +450,7 @@ func (c *client) getRemote(ctx context.Context, n int) (snet.Path, error) {
 	return path, nil
 }
 
-func (c *client) pong(ctx context.Context, logIfOk bool) error {
+func (c *client) pong(ctx context.Context) error {
 	if err := c.conn.SetReadDeadline(getDeadline(ctx)); err != nil {
 		return serrors.WrapStr("setting read deadline", err)
 	}
@@ -494,9 +478,9 @@ func (c *client) pong(ctx context.Context, logIfOk bool) error {
 	if pld.Client != expected.Client || pld.Server != expected.Server || pld.Message != pong {
 		return serrors.New("unexpected contents received", "data", pld, "expected", expected)
 	}
-	if logIfOk {
-		log.Info("Received pong", "server", p.Source)
-	}
+
+	log.Info("Received pong", "server", p.Source)
+
 	return nil
 }
 

diff --git a/tools/end2end_integration/main.go b/tools/end2end_integration/main.go
@@ -43,7 +43,6 @@ var (
 	cmd         string
 	features    string
 	epic        bool
-	traces      bool
 )
 
 func getCmd() (string, bool) {
@@ -76,12 +75,10 @@ func realMain() int {
 		"-local", integration.SrcAddrPattern + ":0",
 		"-remote", integration.DstAddrPattern + ":" + integration.ServerPortReplace,
 		fmt.Sprintf("-epic=%t", epic),
-		fmt.Sprintf("-traces=%t", traces),
 	}
 	serverArgs := []string{
 		"-mode", "server",
 		"-local", integration.DstAddrPattern + ":0",
-		fmt.Sprintf("-traces=%t", traces),
 	}
 	if len(features) != 0 {
 		clientArgs = append(clientArgs, "--features", features)
@@ -121,7 +118,6 @@ func addFlags() {
 	flag.StringVar(&features, "features", "",
 		fmt.Sprintf("enable development features (%v)", feature.String(&feature.Default{}, "|")))
 	flag.BoolVar(&epic, "epic", false, "Enable EPIC.")
-	flag.BoolVar(&traces, "traces", true, "Enable Jaeger traces.")
 }
 
 // runTests runs the end2end tests for all pairs. In case of an error the
@@ -298,7 +294,6 @@ func clientTemplate(progressSock string) integration.Cmd {
 			"-timeout", timeout.String(),
 			"-local", integration.SrcAddrPattern + ":0",
 			"-remote", integration.DstAddrPattern + ":" + integration.ServerPortReplace,
-			fmt.Sprintf("-traces=%t", traces),
 			fmt.Sprintf("-epic=%t", epic),
 		},
 	}

diff --git a/tools/end2endblast/BUILD.bazel b/tools/end2endblast/BUILD.bazel
@@ -18,11 +18,8 @@ go_library(
         "//pkg/snet/path:go_default_library",
         "//pkg/sock/reliable:go_default_library",
         "//private/topology:go_default_library",
-        "//private/tracing:go_default_library",
         "//tools/integration:go_default_library",
         "//tools/integration/integrationlib:go_default_library",
-        "@com_github_opentracing_opentracing_go//:go_default_library",
-        "@com_github_opentracing_opentracing_go//ext:go_default_library",
     ],
 )