systemtests now run completely in containers #46

dseevr · 2016-12-15T21:35:47Z

Copied from the top of systemtests.sh:

builds a systemtests container
creates a docker network (all containers are attached to it)
starts an etcd container
starts a consul container
starts a systemtests container (does nothing by default)
starts a ccn_proxy container on port 10000 linked to etcd
starts a ccn_proxy container on port 10001 linked to consul
executes ./scripts/systemtests_in_container.sh which runs all the systemtests against the etcd proxy and consul proxy
stops etcd proxy container
stops consul proxy container
stops systemtests container
stops etcd container
stops consul container
destroys the docker network

Fixes https://github.com/contiv/ccn/issues/84

dseevr · 2017-01-06T00:40:06Z

As mentioned on Slack, CI will not pass until the firewall issue is resolved and the unit tests are also containerized (in progress)

rhim

Some questions. Mostly looks good.

rhim · 2017-01-06T00:30:16Z

scripts/run.sh

+docker run --rm \
+	-p 10000:10000 \
+	-v $(pwd)/local_certs:/certs:ro \
+	ccn_proxy:devbuild \


why is devbuild version hardcoded here?

This script can actually be deleted I guess. This was a convenience script for quickly testing the proxy during the early stages of development. There's now too many external components (datastore, netmaster, AD, etc.) to realistically have a universal "quick run" target.

rhim · 2017-01-06T00:31:13Z

scripts/systemtests.sh

+	   -p 10000:10000 \
+	   -v $(pwd)/local_certs:/local_certs:ro \
+	   --network $NETWORK_NAME \
+	   ccn_proxy:devbuild \


same, why hardcode of version?

This is the default name when no BUILD_VERSION is given to the make build target.

Hmm... I was thinking about this in the context of a CI run (which invokes the make ci target which runs make build before tests run). I guess we can't make that assumption and need to always build the main proxy container before tests run (either systemtests or unit tests)

rhim · 2017-01-06T00:31:29Z

scripts/systemtests.sh

+	   -p 10001:10001 \
+	   -v $(pwd)/local_certs:/local_certs:ro \
+	   --network $NETWORK_NAME \
+	   ccn_proxy:devbuild \


This should really be made into a variable. I missed it because originally there was only one proxy being spawned and I just copy-pasted and tweaked the --data-store-address and port

rhim · 2017-01-06T00:36:14Z

systemtests/init_test.go

 func Test(t *testing.T) {
 	if len(os.Getenv("DEBUG")) > 0 {
 		log.SetLevel(log.DebugLevel)
 	}

-	datastore := test.GetDatastore()
+	proxyHost = os.Getenv("PROXY_ADDRESS")


Does this mean that systemtests assume that proxy is setup and running?

Yeah, this will always be set before this container is invoked. Can you think of a use case for running systemtests in any other way than the make systemtests target?

I'll add a comment here that this is being set by systemtests_in_container.sh when the test suite is spun up

rhim · 2017-01-06T00:41:51Z

scripts/systemtests.sh

+
+
+echo "Stopping etcd proxy container..."
+docker stop $ETCD_PROXY_CONTAINER_ID


docker rm -f -v to do both in one shot? Same for all below.

I'll test this tomorrow and make sure it does The Right Thing™ when the container doesn't shut down in a timely manner. docker stop lets you specify a timeout (which I use when shutting down the systemtests container)

dseevr · 2017-01-06T19:48:55Z

Removed scripts/run.sh, made the proxy image name a variable, replaced stop + rm -v with rm -f -v, and added a comment about where PROXY_ADDRESS is set

Also, cleaned up the systemtests.sh script a bit by moving some redundant IP stuff into a function and adding more comments

neelimamukiri

Looks good overall. Nice hack with the systemtest container start!

neelimamukiri · 2017-01-06T20:17:52Z

build/Dockerfile.base

+RUN apt-get update && apt-get -y upgrade && \
+    	    apt-get -y install curl build-essential docker.io
+
+RUN curl -O https://storage.googleapis.com/golang/go1.7.4.linux-amd64.tar.gz


Will having this local save us some CI headaches?

Are you thinking that we put it into the repo itself? This will permanently increase the size of the repo by ~70mb for each version we upgrade to, and Github will start yelling at us and telling us to use their Large File Storage service 🙃

Also because we COPY the whole repo into our build container, it'll slow down almost every Docker-related operation we do here... 🤔

Good points :) Ignore the suggestion. I wish we could build and push this to docker hub once and reuse it, though docker pull has its reliability issues too.

neelimamukiri · 2017-01-06T20:39:57Z

scripts/checks.sh

-  echo 1>&2 "${out}"
-  exit 1
+# only build the base if necessary (saves a lot of time)
+if [[ "$(docker images -q ccn_proxy_build_base 2>/dev/null)" == "" ]]; then


What happens when go version changes? Why is the image not being cached and how much additional time do you see this step taking?

Actually, I'm going to remove the caching on this stuff and just change us to a more lightweight base image instead of using ubuntu:16.04

neelimamukiri · 2017-01-06T20:48:11Z

scripts/checks_in_container.sh

+set -e
+if [ "`echo \"${out}\" | sed '/^$/d' | wc -l`" -gt 0 ]
+then
+  echo 1>&2 "gocycle errors in:"


gocycle -> gocyclo. Wouldn't it be simpler to move 10-60 to a function and call all the commands using it - something like run_check("gofmt", gofmt -s -l ${files}) ...

This script is originally from volplugin (https://github.com/contiv/volplugin/blob/0453e29b4d9916ae35ae492b9f3f302ff72bcc58/build/scripts/checks.sh) and we were using it verbatim. In this PR, I just removed the "fetch if not installed" lines because we're baking them in in the Dockerfile.checks

I'll fix this typo though :)

neelimamukiri · 2017-01-06T21:01:12Z

scripts/systemtests.sh

+# 12. stops etcd container
+# 13. stops consul container
+# 14. destroys the docker network
+#


How do you use the mock server vs a real netmaster for the system tests?

Take a peek at any of the systemtests/*_test.go files and look for the runTest() function. Each systemtest has the MockServer injected into it and it can be programmed in a test-specific way.

Oh, I misread the actual question being asked here. As mentioned on Slack, we don't support running the systemtests against a real netmaster for various logistical reasons. We use the mockserver to emulate the responses a real netmaster would send (@yuva29 did the heavy lifting on that) and validate that we process them properly.

neelimamukiri · 2017-01-06T21:06:50Z

scripts/systemtests.sh

+docker exec $SYSTEMTESTS_CONTAINER_ID bash ./scripts/systemtests_in_container.sh $ETCD_PROXY_ADDRESS $CONSUL_PROXY_ADDRESS || true
+
+# ---- CLEANUP ------------------------------------------------------------------
+


Are the logs stored somewhere, in case things fail and we want to debug?

All the test logs are written to stdout, and there's additional output if the DEBUG envvar is set. I actually need to pass -e DEBUG="$DEBUG" to the systemtests container docker run command...

I think having the tests clean up after themselves by default is a good idea, but I'm open to ideas around allowing state to hang around afterwards. Maybe a KEEP_CONTAINERS envvar you can set will result in the containers not being deleted? The downside there is that you would need to manually delete all the containers and then delete the Docker network before you could run the systemtests again.

As long as the CI gets them through stdout it is sufficient. For the rare cases where we need the containers themselves running (which is a pure proxy case should be very rare), we can change it then.

If you run DEBUG=1 make systemtests, you'll now get log.Debug*() output on stdout/stderr as well 😄

Signed-off-by: Bill Robinson <[email protected]>

neelimamukiri

LGTM

dseevr changed the title ~~systemtests now launch etcd, consul, and ccn_proxy containers before running~~ systemtests now run completely in containers Jan 6, 2017

dseevr requested review from rhim and neelimamukiri January 6, 2017 00:20

rhim suggested changes Jan 6, 2017

View reviewed changes

neelimamukiri reviewed Jan 6, 2017

View reviewed changes

dseevr added 2 commits January 6, 2017 14:58

systemtests now run completely in containers

5b638cb

Signed-off-by: Bill Robinson <[email protected]>

make checks now runs in a container

b71dbac

Signed-off-by: Bill Robinson <[email protected]>

neelimamukiri approved these changes Jan 6, 2017

View reviewed changes

rhim approved these changes Jan 6, 2017

View reviewed changes

dseevr merged commit 55648a8 into contiv:master Jan 6, 2017

dseevr deleted the make_testing_great_again branch January 6, 2017 23:34

dseevr pushed a commit to dseevr-dev/auth_proxy that referenced this pull request Dec 21, 2017

Creating one single bundle.js file (contiv#46)

710dc50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

systemtests now run completely in containers #46

systemtests now run completely in containers #46

dseevr commented Dec 15, 2016 •

edited

Loading

dseevr commented Jan 6, 2017

rhim left a comment

rhim Jan 6, 2017

dseevr Jan 6, 2017 •

edited

Loading

rhim Jan 6, 2017

dseevr Jan 6, 2017

rhim Jan 6, 2017

dseevr Jan 6, 2017 •

edited

Loading

rhim Jan 6, 2017

dseevr Jan 6, 2017

rhim Jan 6, 2017

dseevr Jan 6, 2017

dseevr commented Jan 6, 2017 •

edited

Loading

neelimamukiri left a comment

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017 •

edited

Loading

neelimamukiri Jan 6, 2017

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017

dseevr Jan 6, 2017 •

edited

Loading

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017

neelimamukiri Jan 6, 2017

dseevr Jan 6, 2017

neelimamukiri Jan 6, 2017

neelimamukiri left a comment



		echo "Stopping etcd proxy container..."
		docker stop $ETCD_PROXY_CONTAINER_ID

		docker exec $SYSTEMTESTS_CONTAINER_ID bash ./scripts/systemtests_in_container.sh $ETCD_PROXY_ADDRESS $CONSUL_PROXY_ADDRESS \|\| true

		# ---- CLEANUP ------------------------------------------------------------------

systemtests now run completely in containers #46

systemtests now run completely in containers #46

Conversation

dseevr commented Dec 15, 2016 • edited Loading

dseevr commented Jan 6, 2017

rhim left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr commented Jan 6, 2017 • edited Loading

neelimamukiri left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dseevr Jan 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

neelimamukiri left a comment

Choose a reason for hiding this comment

dseevr commented Dec 15, 2016 •

edited

Loading

dseevr Jan 6, 2017 •

edited

Loading

dseevr Jan 6, 2017 •

edited

Loading

dseevr commented Jan 6, 2017 •

edited

Loading

dseevr Jan 6, 2017 •

edited

Loading

dseevr Jan 6, 2017 •

edited

Loading