Migrate sig_failover_acceptance to bazel #3550

scrye · 2019-12-20T09:24:36Z

Also:

add support for plugging a configurable fake SCIOND into the SIG; this
makes it possible to run SIG tests without a control-plane
change the entrypoint of docker debug images from su-exec; it is not needed
in debugging/development environments
change UDP proxy command line options; the new proxy uses
deterministic ports when forwarding data
add config option AllowRunAsRoot to allow running the SIG and Go
dispatcher as root
use ping instead of using go/acceptance/sig_ping_acceptance for
connectivity tests
use static docker-compose file instead of topology generator
to set up sig_failover_acceptance; the generator builds more
files than necessary for the test, and also does not have
support for fake sciond data; doing it directly from hand crafted
files is easier to inspect, modify and debug;
remove BS, CS, PS, SD, BRs from sig_failover_acceptance

This change is

lukedirtwalker

Reviewed 4 of 10 files at r2, 1 of 1 files at r3.
Reviewable status: 1 of 29 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker and @scrye)

.buildkite/pipeline_buildlint.yml, line 57 at r2 (raw file):

        - exit_status: 255 # Forced agent shutdown
  - label: "Acceptance: sig_failover"
    command: bazel test //acceptance/sig_failover:sig_failover_test

I think you need to create the accept_artifacts folder first:
- "mkdir -p $ACCEPTANCE_ARTIFACTS"

acceptance/sig_failover/docker-compose.yml, line 18 at r1 (raw file):

services:
  patha:
    command: ["-lx", "242.254.100.3:50000", "-rx", "242.254.100.2:30041", "-ly", "242.254.200.3:50000", "-ry", "242.254.200.2:30041"]

lx and rx are a bit short for my taste, what do they mean?

acceptance/sig_failover/docker-compose.yml, line 61 at r1 (raw file):

    image: bazel/acceptance/sig_failover_acceptance:sig1
    network_mode: service:dispatcher1
    privileged: true

why is that needed?

acceptance/sig_failover/test, line 60 at r1 (raw file):

run_test() {(set -e

    bash "$(rlocation __main__/go/tools/udpproxy/udpproxy)" --norun

add a comment what this block does. I guess it loads all the required docker images, but this is far from obvious ;)

lukedirtwalker

Reviewed 21 of 32 files at r1, 1 of 10 files at r2.
Reviewable status: 22 of 29 files reviewed, 7 unresolved discussions (waiting on @lukedirtwalker and @scrye)

go/lib/sciond/fake/fake.go, line 61 at r2 (raw file):

type Path struct {
	JSONFingerprint string   `json:"Fingerprint"`

Usually we use snake_case for JSON. IDC this much here.

go/lib/sciond/fake/testdata/sd.json, line 14 at r2 (raw file):

      }
    ]
  }

add newline

go/sig/egress/reader/reader.go, line 81 at r2 (raw file):

			buf = buf[:length]
			dstIP, err := r.getDestIP(buf)
			log.Debug("XXX", "read", buf[:length], "to", dstIP)

remove?

go/sig/egress/session/sessmon.go, line 137 at r2 (raw file):

			"newExpiration", currPath.Path().Expiry(),
			"oldMTU", mtu,
			"newMTU", currPath.Path().MTU)

💯

lukedirtwalker

Reviewed 1 of 10 files at r2, 3 of 4 files at r4.
Reviewable status: 25 of 29 files reviewed, 10 unresolved discussions (waiting on @scrye)

acceptance/sig_failover/BUILD.bazel, line 7 at r4 (raw file):

    size = "small",
    srcs = ["test"],
    args = ["setup"],

Is that still needed?

acceptance/sig_failover/BUILD.bazel, line 13 at r4 (raw file):

    data = [
        "//go/tools/udpproxy:udpproxy",
        "//acceptance/sig_failover:dispatcher1",

Nit: I think for deps in the same file usually the shorter form like ":dispacher1" is used

acceptance/sig_failover/BUILD.bazel, line 18 at r4 (raw file):

        "//acceptance/sig_failover:sig2",
        "docker-compose.yml",
    ] + glob(["testdata/**/*"]),

testdata is already included by the containers, so no longer needed here I think.

acceptance/sig_failover/test, line 101 at r4 (raw file):

mkdir -p $ACCEPTANCE_ARTIFACTS

echo "ls -la $ACCEPTANCE_ARTIFACTS"

Clean up?

acceptance/sig_failover/test, line 117 at r4 (raw file):

docker-compose -f acceptance/sig_failover/docker-compose.yml down

find $ACCEPTANCE_ARTIFACTS

Remove?

lukedirtwalker

Reviewed 1 of 32 files at r1, 1 of 4 files at r4.
Reviewable status: 27 of 29 files reviewed, 13 unresolved discussions (waiting on @scrye)

acceptance/sig_failover/docker-compose.yml, line 12 at r4 (raw file):

    driver: bridge
    driver_opts:
      com.docker.network.bridge.name: bridge2

for what is that needed?

go/lib/sciond/fake/fake.go, line 3 at r4 (raw file):

// Copyright 2019 Anapaya Systems

package fake

I wonder about the naming. This is also kind of a mock? But maybe mock is already overloaded, so not sure.

go/lib/sciond/fake/fake.go, line 204 at r4 (raw file):

func (c connector) Close(ctx context.Context) error {
	panic("not implemented")

this could also just be return nil

lukedirtwalker

Reviewed 2 of 10 files at r2.
Reviewable status: all files reviewed, 15 unresolved discussions (waiting on @scrye)

go/lib/sciond/fake/fake.go, line 149 at r4 (raw file):

	currentTime := time.Now()
	secondsElapsed := int(currentTime.Sub(c.creationTime).Seconds())

time.Since would be slightly nicer

go/lib/sciond/fake/fake_test.go, line 115 at r4 (raw file):

	assert.True(t, paths[0].Expiry().After(time.Now().Add(time.Hour)))

	time.Sleep(time.Second)

I don't like that. But except from adding a custom clock into the provider I don't see a way around it. So I guess we can leave it for now.

Also: - add support for plugging a configurable fake SCIOND into the SIG; this makes it possible to run SIG tests without a control-plane - change the entrypoint of docker debug images from su-exec; it is not needed in debugging/development environments - change UDP proxy command line options; the new proxy uses deterministic ports when forwarding data - add config option AllowRunAsRoot to allow running the SIG and Go dispatcher as root - use ping instead of using go/acceptance/sig_ping_acceptance for connectivity tests - use static docker-compose file instead of topology generator to set up sig_failover_acceptance; the generator builds more files than necessary for the test, and also does not have support for fake sciond data; doing it directly from hand crafted files is easier to inspect, modify and debug; - remove BS, CS, PS, SD, BRs from sig_failover_acceptance Fixes #3125.

scrye

Reviewable status: 17 of 28 files reviewed, 14 unresolved discussions (waiting on @lukedirtwalker and @scrye)

acceptance/sig_failover/BUILD.bazel, line 7 at r4 (raw file):