SIG Acceptance Tests Setup

[worxli]

Fixes #1499

This adds the setup for SIG acceptance tests. Also adds a simple ping acceptance test.

This change is 

[lukedirtwalker]

Reviewed 24 of 32 files at r1.
Reviewable status: 24 of 32 files reviewed, 5 unresolved discussions (waiting on @worxli and @scrye)

---

go/integration/ping_sigintegration/ping.go, line 40 at r1 (raw file):

var sigAddr integration.HostAddr = func(ia addr.IA) snet.Addr {
	conf := "gen/sig-testing.conf"
	file, err := os.Open(conf)

Please parse the sig-testing.conf in a map and then use this map in the sigAddr function you pass to IAPairs. The way it is now you parse this file hundreds of times.

---

python/topology/common.py, line 129 at r1 (raw file):

def _sciond_name(topo_id):

Isn't the underline prefix for private methods usually? I think most methods here shouldn't have the underline prefix. But please only change the ones you add here.

---

python/topology/common.py, line 137 at r1 (raw file):

def _usr_vol():

I'd name this docker_usr_vol since it is in common now.

---

python/topology/common.py, line 137 at r1 (raw file):

def _usr_vol():

Also why is this a method and not a constant?

---

python/topology/docker_utils.py, line 51 at r1 (raw file):

        if self.args.sig:
            text = ''
            for topo_id in self.args.topo_dicts:

Why is that happening in here? Couldn't that be in the SIGGenerator

[lukedirtwalker]

Reviewed 3 of 32 files at r1, 1 of 1 files at r2.
Reviewable status: 28 of 32 files reviewed, 8 unresolved discussions (waiting on @worxli and @scrye)

---

docker/tester.sh, line 4 at r2 (raw file):

set -ex

if [ -n "$REMOTE_NETS" ] && [ -n "$SIG_IP" ]; then

That seems to be specific to the SIG, shouldn't we only have that in sig.sh

---

go/integration/ping_sigintegration/ping.go, line 73 at r2 (raw file):

1>&2

Maybe mention why you have to redirect Stdout to StdErr.

---

go/integration/ping_sigintegration/ping.go, line 91 at r2 (raw file):

			log.Info(fmt.Sprintf("Test %v: %v,%v -> %v,%v (%v/%v)", in.Name(), conn.Src.IA,
				conn.Src.Host, conn.Dst.IA, conn.Dst.Host, i+1, len(pairs)))
			// log.Debug(fmt.Sprintf("Ping %s from %s", conn.Dst.Host.L3, conn.Src.Host.L3))

Delete? Or make it log.Trace and do it unconditional?

[lukedirtwalker]

Reviewed 4 of 32 files at r1.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @worxli and @scrye)

[scrye]

Reviewed 31 of 32 files at r1, 1 of 1 files at r2.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @worxli)

a discussion (no related file):
Running this on my machine failed with:

Step 12/15 : RUN tar xf ~/go_vendor.tar.gz -C go/vendor/
 ---> Running in 60138457aca1
Removing intermediate container 60138457aca1
 ---> d583d7851d4a
Step 13/15 : RUN tar xf ~/python_local.tar.gz -C ~
 ---> Running in 5633a32e427b
tar: .local/lib: Cannot mkdir: Permission denied
tar: .local/lib/python2.7: Cannot mkdir: Permission denied
tar: .local/lib/python2.7/site-packages: Cannot mkdir: Permission denied
tar: .local/lib/python2.7/site-packages/supervisor_wildcards-0.1.3.dist-info: Cannot mkdir: Permission denied

during

[--->------] Building scion docker image

---

---

go/integration/ping_sigintegration/ping.go, line 1 at r1 (raw file):

// Copyright 2018 ETH Zurich

The folder name here doesn't seem to follow the pattern of the others. ping_sig_integration possibly fits better?

---

python/topology/generator.py, line 76 at r1 (raw file):

    parser.add_argument('--in-docker', action='store_true',
                        help='Set if running in a docker container')
    parser.add_argument('--sig', action='store_true', help='Generate topology for SIG testing')

Maybe using this argument should throw an error if used in a non-docker topology? (Or at least the comment should suggest it is only for docker).

---

python/topology/sig.py, line 148 at r1 (raw file):

            },
            'logging.file': {
                'Level': 'debug',

It would be useful if this respected the -t flag to go from debug to trace.

[scrye]

a discussion (no related file):

Previously, scrye (Sergiu Costea) wrote…

Running this on my machine failed with:

Step 12/15 : RUN tar xf ~/go_vendor.tar.gz -C go/vendor/
 ---> Running in 60138457aca1
Removing intermediate container 60138457aca1
 ---> d583d7851d4a
Step 13/15 : RUN tar xf ~/python_local.tar.gz -C ~
 ---> Running in 5633a32e427b
tar: .local/lib: Cannot mkdir: Permission denied
tar: .local/lib/python2.7: Cannot mkdir: Permission denied
tar: .local/lib/python2.7/site-packages: Cannot mkdir: Permission denied
tar: .local/lib/python2.7/site-packages/supervisor_wildcards-0.1.3.dist-info: Cannot mkdir: Permission denied

during

[--->------] Building scion docker image

Actually, this might be a local issue. Will investigate.

---

[lukedirtwalker]

Reviewable status: all files reviewed, 12 unresolved discussions (waiting on @worxli and @scrye)

---

python/topology/generator.py, line 76 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Maybe using this argument should throw an error if used in a non-docker topology? (Or at least the comment should suggest it is only for docker).

+1

[worxli]

Reviewable status: 23 of 33 files reviewed, 12 unresolved discussions (waiting on @lukedirtwalker, @scrye, and @worxli)

---

docker/tester.sh, line 4 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

That seems to be specific to the SIG, shouldn't we only have that in sig.sh

The sig.sh is only in the image where the SIG is run. This is in the tester image which is used for all kinds of testing, including SIG testing.

---

go/integration/ping_sigintegration/ping.go, line 1 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

The folder name here doesn't seem to follow the pattern of the others. ping_sig_integration possibly fits better?

Nah. Then it is run by go_integration because that looks for everything with *_integration but it should really not be run there. Maybe it would be better to add an acceptance folder instead?

---

go/integration/ping_sigintegration/ping.go, line 40 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Please parse the sig-testing.conf in a map and then use this map in the sigAddr function you pass to IAPairs. The way it is now you parse this file hundreds of times.

Done.

---

go/integration/ping_sigintegration/ping.go, line 73 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

1>&2

Maybe mention why you have to redirect Stdout to StdErr.

Done.

---

go/integration/ping_sigintegration/ping.go, line 91 at r2 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Delete? Or make it log.Trace and do it unconditional?

Leftover, will remove.

---

python/topology/common.py, line 129 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Isn't the underline prefix for private methods usually? I think most methods here shouldn't have the underline prefix. But please only change the ones you add here.

Done.

---

python/topology/common.py, line 137 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I'd name this docker_usr_vol since it is in common now.

Done.

---

python/topology/common.py, line 137 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Also why is this a method and not a constant?

Done.

---

python/topology/docker_utils.py, line 51 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Why is that happening in here? Couldn't that be in the SIGGenerator

This is specific to testing. Thus it should be done here.

---

python/topology/generator.py, line 76 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

+1

Done.

---

python/topology/sig.py, line 148 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

It would be useful if this respected the -t flag to go from debug to trace.

Done.

[scrye]

Reviewed 3 of 10 files at r3.
Reviewable status: 26 of 33 files reviewed, 10 unresolved discussions (waiting on @lukedirtwalker, @scrye, and @worxli)

---

go/integration/ping_sigintegration/ping.go, line 1 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Nah. Then it is run by go_integration because that looks for everything with *_integration but it should really not be run there. Maybe it would be better to add an acceptance folder instead?

Ah, I see. Then it probably makes more sense in acceptance, yes.

[scrye]

Reviewed 1 of 10 files at r3.
Reviewable status: 27 of 33 files reviewed, 10 unresolved discussions (waiting on @lukedirtwalker, @scrye, and @worxli)

[lukedirtwalker]

Reviewed 7 of 10 files at r3.
Reviewable status: 32 of 33 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

go/integration/ping_sigintegration/ping.go, line 53 at r3 (raw file):

	defer log.Flush()
	if !*integration.Docker {
		fmt.Fprintf(os.Stderr, "Can only run %s test with docker!\n", name)

You could also use log.Crit now that the log is initialized, below as well.

---

go/integration/ping_sigintegration/ping.go, line 58 at r3 (raw file):

	if err := readTestingConf(); err != nil {
		fmt.Fprintf(os.Stderr, "Error reading testing conf: %s\n", err)
		os.Exit(1)

return 1

[worxli]

Reviewable status: 32 of 33 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @scrye)

a discussion (no related file):

Previously, scrye (Sergiu Costea) wrote…

Actually, this might be a local issue. Will investigate.

Done.

---

---

go/integration/ping_sigintegration/ping.go, line 1 at r1 (raw file):

Previously, scrye (Sergiu Costea) wrote…

Ah, I see. Then it probably makes more sense in acceptance, yes.

Done.

---

go/integration/ping_sigintegration/ping.go, line 53 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

You could also use log.Crit now that the log is initialized, below as well.

Done.

---

go/integration/ping_sigintegration/ping.go, line 58 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

return 1

Done.

[lukedirtwalker]

Reviewed 1 of 1 files at r4, 9 of 9 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scrye and @worxli)

---

acceptance/sig_acceptance/test, line 15 at r5 (raw file):

test_run() {
    sleep 10

I'd probably move this to setup since we really wait until the setup is "done"

---

go/acceptance/sig_ping_acceptance/main.go, line 73 at r5 (raw file):

// RunTests runs the client for each IAPair.
// In case of an error the function is terminated immediately.
func runTests(in integration.Integration, pairs []integration.IAPair) error {

Once 2161 is merged this one should also use RunUnaryTests

[worxli]

Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @scrye, @worxli, and @lukedirtwalker)

---

acceptance/sig_acceptance/test, line 15 at r5 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I'd probably move this to setup since we really wait until the setup is "done"

Done.

[worxli]

Reviewable status: 31 of 33 files reviewed, 4 unresolved discussions (waiting on @lukedirtwalker, @scrye, and @worxli)

---

go/acceptance/sig_ping_acceptance/main.go, line 73 at r5 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Once 2161 is merged this one should also use RunUnaryTests

Will do.

[scrye]

Reviewed 2 of 2 files at r6.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @worxli and @lukedirtwalker)

a discussion (no related file):

Previously, worxli (Lukas Bischofberger) wrote…

Done.

For completeness, this was not related to this PR. It was due to the file system used by docker. Changing from aufs to overlay2 fixed the issue.

---

[lukedirtwalker]

Reviewed 9 of 10 files at r7.
Reviewable status: complete! all files reviewed, all discussions resolved

[worxli]

Reviewable status: all files reviewed, 1 unresolved discussion

---

acceptance/sig_acceptance/Dockerfile.sig, line 10 at r7 (raw file):

RUN ["setcap", "cap_net_admin+ei", "/app/sig"]
# Note: this process needs explicit CAP_NET_ADMIN from docker. e.g. with `cap_add: NET_ADMIN` from docker-compose
ENTRYPOINT ["./sig.sh"]

This doesn't propagate signals, so reloading the SIG doesn't work.

[kormat]

Reviewed 11 of 32 files at r1, 1 of 10 files at r7, 12 of 17 files at r8.
Reviewable status: 30 of 33 files reviewed, 2 unresolved discussions (waiting on @scrye, @lukedirtwalker, and @worxli)

---

docker.sh, line 35 at r8 (raw file):

}

cmd_tester() {

I think this can be replaced with a simple docker build - < docker/Dockerfile.tester. I don't think any of the params or source files are needed.

---

go/lib/integration/integration.go, line 169 at r8 (raw file):

}

type HostAddr func(ia addr.IA) snet.Addr

Cute :)

[kormat]

Reviewed 1 of 17 files at r8.
Reviewable status: 31 of 33 files reviewed, 3 unresolved discussions (waiting on @scrye, @lukedirtwalker, and @worxli)

---

python/topology/generator.py, line 77 at r8 (raw file):

    parser.add_argument('--image-tag', help='Docker image tag')
    parser.add_argument('--sig', action='store_true',
                        help='Generate a SIG per AS (only available with -d)')

This should really be very very clear that this flag is only useful for a sig acceptance test.

[kormat]

Reviewed 1 of 17 files at r8.
Reviewable status: 32 of 33 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)

[kormat]

Reviewed 1 of 32 files at r1, 1 of 10 files at r3, 1 of 9 files at r5, 1 of 10 files at r7, 1 of 17 files at r8.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @worxli)

[worxli]

Reviewable status: 25 of 33 files reviewed, 2 unresolved discussions (waiting on @kormat, @lukedirtwalker, and @scrye)

---

docker.sh, line 35 at r8 (raw file):

Previously, kormat (Stephen Shirley) wrote…

I think this can be replaced with a simple docker build - < docker/Dockerfile.tester. I don't think any of the params or source files are needed.

Done.

---

python/topology/generator.py, line 77 at r8 (raw file):

Previously, kormat (Stephen Shirley) wrote…

This should really be very very clear that this flag is only useful for a sig acceptance test.

Done.

[kormat]

Reviewed 7 of 11 files at r9.
Reviewable status: 30 of 33 files reviewed, all discussions resolved (waiting on @lukedirtwalker and @kormat)

[lukedirtwalker]

Reviewed 2 of 17 files at r8, 5 of 11 files at r9.
Reviewable status: 32 of 33 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

python/topology/common.py, line 145 at r9 (raw file):

def remote_nets(networks, topo_id):
    rem_nets = []

Please document networks parameter and what the function returns.

---

python/topology/docker_utils.py, line 94 at r8 (raw file):

        }
        if self.args.sig:
            net = self.args.networks[name][0]

Okay this block is not really clear to me. Can you add some explanation on what it does?

[worxli]

Reviewable status: 32 of 33 files reviewed, 2 unresolved discussions (waiting on @lukedirtwalker)

---

python/topology/common.py, line 145 at r9 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Please document networks parameter and what the function returns.

Done.

---

python/topology/docker_utils.py, line 94 at r8 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Okay this block is not really clear to me. Can you add some explanation on what it does?

Done.

[lukedirtwalker]

Reviewed 7 of 17 files at r8, 4 of 11 files at r9, 2 of 2 files at r10.
Reviewable status: complete! all files reviewed, all discussions resolved