Add go integration tests with supervisord

[karampok]

This change is 

[karampok]

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion

---

scion.sh, line 118 at r1 (raw file):

}

run_setup() {

@worxli @lukedirtwalker running integration test as above this was failing because the sudo was interactive, removing theme completely seems to works (at least for supervisord way). Any idea if it is required?

[worxli]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @karampok and @worxli)

---

.buildkite/pipeline.yml, line 15 at r1 (raw file):

      - bazel --bazelrc=.bazelrc_ci test //go/... --print_relative_test_log_paths
    key: go_tests
    artifact_paths:

why are you removing this?

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

    - ./bin/pp_integration  > /dev/null 2>&1
    - ./bin/end2end_integration  > /dev/null 2>&1
    - tar -czf "logs-${BUILDKITE_COMMIT:-local}.tar.gz" logs/*

there should be a pre-artifact hook that does that.

[worxli]

Reviewable status: 0 of 2 files reviewed, 3 unresolved discussions (waiting on @karampok and @lukedirtwalker)

---

scion.sh, line 118 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

@worxli @lukedirtwalker running integration test as above this was failing because the sudo was interactive, removing theme completely seems to works (at least for supervisord way). Any idea if it is required?

Does it still work locally? Does it work locally when the dirs do not exist? (maybe the services nowadays actually create the necessary socket dirs).

[lukedirtwalker]

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

---

.buildkite/pipeline.yml, line 15 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

why are you removing this?

Yeah please keep it.

---

.buildkite/pipeline.yml, line 61 at r1 (raw file):

    - ./bin/cert_req_integration  > /dev/null 2>&1
    - ./bin/pp_integration  > /dev/null 2>&1
    - ./bin/end2end_integration  > /dev/null 2>&1

I think there should also be scmp integration?

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

there should be a pre-artifact hook that does that.

You can just change the artifacts paths below to "artifacts.out/**/*"

---

.buildkite/pipeline.yml, line 63 at r1 (raw file):

    - ./bin/end2end_integration  > /dev/null 2>&1
    - tar -czf "logs-${BUILDKITE_COMMIT:-local}.tar.gz" logs/*
    - ./scion.sh stop

use topo_clean instead.

Also are you sure that this is always executed? even if stuff before fails?

[worxli]

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

---

.buildkite/pipeline.yml, line 63 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

use topo_clean instead.

Also are you sure that this is always executed? even if stuff before fails?

No, then it's not.

[worxli]

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

---

.buildkite/pipeline.yml, line 59 at r1 (raw file):

    - ./scion.sh topology nobuild
    - ./scion.sh run nobuild && sleep 10
    - ./bin/cert_req_integration  > /dev/null 2>&1

why are you silencing this? at least their output should be put in the logs. also, why are all of them in the same step?

[worxli]

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

---

.buildkite/pipeline.yml, line 59 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

why are you silencing this? at least their output should be put in the logs. also, why are all of them in the same step?

I mean, it's probably alright to put them all in the same step but you might consider not to, to reduce test interactions.

[karampok]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @worxli)

---

scion.sh, line 118 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

Does it still work locally? Does it work locally when the dirs do not exist? (maybe the services nowadays actually create the necessary socket dirs).

yes it does work after manually deleting the file. Otherwise the pieline fails because we have to set up the LOGNAME variable.

---

.buildkite/pipeline.yml, line 15 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Yeah please keep it.

Sure, how exactly is that being used?

---

.buildkite/pipeline.yml, line 59 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

I mean, it's probably alright to put them all in the same step but you might consider not to, to reduce test interactions.

You spin up a topology and you run tests (pings). If I put on different step, this will run in different agents, so I would have to spin up each time a topology. I do not see interactions between test, and to be honest given they operate on the same topology without introducing failure (to my knowledge), we should increase the interactions (even run in parallel).
I suggest to keep them as it for now.
Yes the test binaries log in log folder by default so it is okay ( i want eventually to make the test output compact and not to silence them)

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

You can just change the artifacts paths below to "artifacts.out/**/*"

why having artififacts.out/**/* is better? I on-purpose want to grab only a specific log tar file

@worxli why there should be? The idea of having the above format is simple.
And if the lines will get really complicated then they will be transferred the content inside a script.
And that script wil be able to run without buildkite. Hiding the the source of the env value in another script is not really required.
Also it is safer because copy paste would to rm -rf /${file:-"default}/* could just to that rm -rf /*

---

.buildkite/pipeline.yml, line 63 at r1 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

No, then it's not.

No it is not executed. Is there a way to make sure it is executed using some buildkite pipeline approach?
Otherwise, we have to create a such logic (aka https://concourse-ci.org/on-failure-step-hook.html) on our own.

@lukedirtwalker I on purpose do not quote everything, I quote only when var interpolation takes place as you would in a bash script
(according to https://www.shellcheck.net/)

[worxli]

Reviewable status: 1 of 2 files reviewed, 6 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @worxli)

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

why having artififacts.out/**/* is better? I on-purpose want to grab only a specific log tar file

@worxli why there should be? The idea of having the above format is simple.
And if the lines will get really complicated then they will be transferred the content inside a script.
And that script wil be able to run without buildkite. Hiding the the source of the env value in another script is not really required.
Also it is safer because copy paste would to rm -rf /${file:-"default}/* could just to that rm -rf /*

s/should/is. @lukedirtwalker can explain that to you. also to problem with doing the tar here is that it's only executed when all previous steps pass. usually you want the logs when they do not pass though.

[lukedirtwalker]

Reviewed 1 of 1 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @worxli)

---

.buildkite/pipeline.yml, line 15 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

Sure, how exactly is that being used?

It is used via the pre-artifacts hook

---

.buildkite/pipeline.yml, line 59 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

You spin up a topology and you run tests (pings). If I put on different step, this will run in different agents, so I would have to spin up each time a topology. I do not see interactions between test, and to be honest given they operate on the same topology without introducing failure (to my knowledge), we should increase the interactions (even run in parallel).
I suggest to keep them as it for now.
Yes the test binaries log in log folder by default so it is okay ( i want eventually to make the test output compact and not to silence them)

The output should not be silenced, the should go into some file that will go into the artifacts.

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

why having artififacts.out/**/* is better? I on-purpose want to grab only a specific log tar file

@worxli why there should be? The idea of having the above format is simple.
And if the lines will get really complicated then they will be transferred the content inside a script.
And that script wil be able to run without buildkite. Hiding the the source of the env value in another script is not really required.
Also it is safer because copy paste would to rm -rf /${file:-"default}/* could just to that rm -rf /*

We have a pre-artifacts hook that stuffs everything into a the artifacts.out folder. So I think we should definitely reuse this functionality.

---

.buildkite/pipeline.yml, line 63 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

No it is not executed. Is there a way to make sure it is executed using some buildkite pipeline approach?
Otherwise, we have to create a such logic (aka https://concourse-ci.org/on-failure-step-hook.html) on our own.

@lukedirtwalker I on purpose do not quote everything, I quote only when var interpolation takes place as you would in a bash script
(according to https://www.shellcheck.net/)

So we definitely need to be sure that this executes. Otherwise we can have an agent in an unclean state. I think we should do this in a post-command hook (https://buildkite.com/docs/agent/v3/hooks) Or we do the whole step in a script.

[karampok]

Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

.buildkite/pipeline.yml, line 15 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

It is used via the pre-artifacts hook

Done.

---

.buildkite/pipeline.yml, line 59 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

The output should not be silenced, the should go into some file that will go into the artifacts.

Done.

---

.buildkite/pipeline.yml, line 62 at r1 (raw file):

pre-artifacts
I was not aware of that hook, done

---

.buildkite/pipeline.yml, line 63 at r1 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

So we definitely need to be sure that this executes. Otherwise we can have an agent in an unclean state. I think we should do this in a post-command hook (https://buildkite.com/docs/agent/v3/hooks) Or we do the whole step in a script.

I suppose that hook https://github.com/scionproto/scion/blob/master/.buildkite/hooks/pre-exit takes care of the cleanup.
I have a bit of concern that long term a hook might be altered and introduce weird behavior. But I suppose this is the buildkite way.

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

  - label: "Check generated go_deps.bzl file is up to date with go.mod"
    command:
      - "rm -rf  /tmp/${BUILDKITE_STEP_ID:-local}/"

This change is added because otherwise every time I run locally the tests I clean up my tmp folder
(BUILDKID_STEP_ID is not implemented in bk cli)

[worxli]

Reviewed 1 of 3 files at r3.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @karampok, @lukedirtwalker, and @worxli)

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

  - label: "Check generated go_deps.bzl file is up to date with go.mod"
    command:
      - "rm -rf  /tmp/${BUILDKITE_STEP_ID:-local}/"

remove the extra space after rf. also let's do TMPDIR="/tmp/${BUILDKITE_STEP_ID:-local}/" and then use that for the rest of the commands.
or better set and ENV var per job.

---

.buildkite/pipeline_buildlint.yml, line 63 at r3 (raw file):

    - ./scion.sh topology nobuild
    - ./scion.sh run nobuild && sleep 10
    - ./bin/cert_req_integration -log.console warn -log.dir logs

-log.dir logs is the default, thus not needed.

[lukedirtwalker]

Reviewed 2 of 3 files at r3.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @karampok and @worxli)

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

Previously, karampok (Konstantinos) wrote…

This change is added because otherwise every time I run locally the tests I clean up my tmp folder
(BUILDKID_STEP_ID is not implemented in bk cli)

Is that a bug in the bk cli? I would assume the full env that is available in a normal BK should also be available here.

[lukedirtwalker]

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

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

Is that a bug in the bk cli? I would assume the full env that is available in a normal BK should also be available here.

I just merged #3563 so this should no longer be needed.

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

remove the extra space after rf. also let's do TMPDIR="/tmp/${BUILDKITE_STEP_ID:-local}/" and then use that for the rest of the commands.
or better set and ENV var per job.

I just merged #3563 so this should no longer be needed.

[karampok]

Reviewable status: 1 of 5 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

.buildkite/pipeline_buildlint.yml, line 24 at r3 (raw file):

Previously, lukedirtwalker (Lukas Vogel) wrote…

I just merged #3563 so this should no longer be needed.

Done.

---

.buildkite/pipeline_buildlint.yml, line 63 at r3 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

-log.dir logs is the default, thus not needed.

Done.

[karampok]

Reviewable status: 0 of 4 files reviewed, 3 unresolved discussions (waiting on @lukedirtwalker and @worxli)

---

scion.sh, line 118 at r1 (raw file):

Previously, karampok (Konstantinos) wrote…

yes it does work after manually deleting the file. Otherwise the pieline fails because we have to set up the LOGNAME variable.

I will not add this as part of this PR

[worxli]

Reviewed 2 of 5 files at r4.
Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @karampok)

[worxli]

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @karampok)

---

.buildkite/hooks/pre-artifact, line 30 at r4 (raw file):

ARTIFACTS="buildkite.${BUILDKITE_ORGANIZATION_SLUG}.${TARGET}.${BUILD}.${BUILDKITE_STEP_KEY:-unset}.${BUILDKITE_JOB_ID}"
mkdir -p "artifacts/$ARTIFACTS" artifacts.out

why the change here?

[worxli]

Reviewable status: 1 of 4 files reviewed, 1 unresolved discussion (waiting on @karampok)

---

.buildkite/hooks/pre-exit, line 13 at r4 (raw file):

docker volume prune -f

rm -rf bazel-testlogs logs/* traces gen gen-cache "$TEST_ARTIFACTS" artifacts

here it leaks from the pre-artifact context. I think pre-artifact should make sure it's removed.

[karampok]

Reviewable status: 1 of 3 files reviewed, 1 unresolved discussion (waiting on @worxli)

---

.buildkite/hooks/pre-exit, line 13 at r4 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

here it leaks from the pre-artifact context. I think pre-artifact should make sure it's removed.

done

[worxli]

Reviewable status: 1 of 3 files reviewed, 3 unresolved discussions (waiting on @karampok and @worxli)

---

.gitignore, line 18 at r4 (raw file):

gen-test

where does this come from?

---

.gitignore, line 79 at r4 (raw file):

# buildkite
artifacts.out

I don't see this locally being created.

[worxli]

Reviewed 2 of 2 files at r5.
Reviewable status: 2 of 3 files reviewed, 2 unresolved discussions (waiting on @karampok)

[karampok]

Reviewable status: 1 of 3 files reviewed, 2 unresolved discussions (waiting on @worxli)

---

.gitignore, line 18 at r4 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

gen-test

where does this come from?

good question, i have locally

14:48 $ ls gen-test/ISD1/
ASff00_0_110/ ASff00_0_111/ ASff00_0_112/ trcs/         trc-v1.toml

and I presumed it was a thing but probably it was something manually created. Reverted.

---

.gitignore, line 79 at r4 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

I don't see this locally being created.

I can see it that it is being created at https://github.com/scionproto/scion/blob/master/.buildkite/hooks/pre-artifact#L41
and is where locally we have the tar files.
Nevertheless there was a bug, bk if run locally does provide the BUILDKITE_PIPELINE_SLUG with value scion

Now after running locally, you should see the tar files

 ls artifacts.out/
buildkite.local.pub-integration-tests.build-1.unset.35d53592-c16d-41e9-9c2c-438fca36572a.tar.gz
buildkite.local.pub-integration-tests.build-1.unset.68aebe0c-9e6b-4c1b-8050-e44f2caf66f4.tar.gz
buildkite.local.pub-integration-tests.build-1.unset.84b1d570-2a66-4909-837e-44f7074af5fe.tar.gz

[worxli]

Reviewed 1 of 2 files at r6.
Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @karampok)

---

.gitignore, line 79 at r4 (raw file):

Previously, karampok (Konstantinos) wrote…

I can see it that it is being created at https://github.com/scionproto/scion/blob/master/.buildkite/hooks/pre-artifact#L41
and is where locally we have the tar files.
Nevertheless there was a bug, bk if run locally does provide the BUILDKITE_PIPELINE_SLUG with value scion

Now after running locally, you should see the tar files

 ls artifacts.out/
buildkite.local.pub-integration-tests.build-1.unset.35d53592-c16d-41e9-9c2c-438fca36572a.tar.gz
buildkite.local.pub-integration-tests.build-1.unset.68aebe0c-9e6b-4c1b-8050-e44f2caf66f4.tar.gz
buildkite.local.pub-integration-tests.build-1.unset.84b1d570-2a66-4909-837e-44f7074af5fe.tar.gz

👍 but make it /artifacts.out

[karampok]

Reviewable status: 2 of 3 files reviewed, 1 unresolved discussion (waiting on @worxli)

---

.gitignore, line 79 at r4 (raw file):

Previously, worxli (Lukas Bischofberger) wrote…

👍 but make it /artifacts.out

Done.

[worxli]

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