End-to-end tests #62

eapolinario · 2020-04-20T09:01:36Z

This PR adds support for end-to-end tests involving go-control-plane SnapshotCache as the backbone for a management server, xds-relay server and one instance of envoy.

The setup is similar to how go-control-plane does its e2e tests, although in our case we decided to adopt a more conventional test structure, the idea being that we're going to extend the number of tests as time goes by.

Speaking about the mechanics of the tests, we run them in a container to make it easy to bring a real instance of envoy. By choosing to do it all in golang we had to sacrifice portability since there is no cross-platform way of safely exiting processes spawned in go. I ran multiple tests with more than one test in the same suite binding to the same ports (basically duplicates of the tests, only with different names) and had to introduce a 1 second delay between tests to make sure the OS can free up the ports between tests.

I also left a few TODO, but I'd like to get feedback on the overall approach.

Signed-off-by: eapolinario <eapolinario@lyft.com>

jyotimahapatra

This is great! Few questions.

.github/workflows/end2end-tests.yml

Dockerfile

integration/testdata/bootstrap.yaml

jyotimahapatra · 2020-04-20T18:03:15Z

integration/e2e_test.go

+}
+
+func TestSnapshotCacheSingleEnvoyAndXdsRelayServer(t *testing.T) {
+	if runtime.GOOS != "linux" {


since there are many nuances for running this and envoy is the only out of process component, would it be better to make this a bash driven test?

I thought about it and decided against a bash-driver test because I was envisioning a scenario where we control multiple envoy instances from the test. Imagine being able to stand up and down envoy instances in the middle of a test.

Actually, I was looking around the solution used by gcp (namely trap the even if made this a bash-driven test we would still suffer from the same problem (i.e. no cross-platform way of killing child processes and its descendants). The solution adopted in gcp (i.e. trapping the exit of the bash process to kill the parent envoy process) only kills the parent process. What I'm getting at is that even if we moved to bash we would still need to rely on a linux-only solution to handle this.

Another alternative I considered was to try to make sure that ports were unique in each test, but that also brings its own set of complications and was quickly shot down by me.

Imagine being able to stand up and down envoy instances in the middle of a test this is definitely a compelling scenario for avoiding a bash driver.

@eapolinario Out of curiosity, did you test this locally on a linux system then?

I think we have to run the tests inside docker.
I run my g-c-p ingetration tests like this docker run -v ~/src/go-control-plane:/go-control-plane --cpus=10 -it gcr.io/istio-testing/go-control-plane-ci:04-02-2020 /bin/bash

It will be hard to run it on mac because we will need a mac build of envoy.

@jessicayuen , I added a script to help run these tests on a mac. Check the run_docker.sh file. Assuming you have the docker runtime installed on your mac (super easy to install via homebrew: brew cask install docker), you can run this locally by running ./scripts/run_docker.sh make e2e-tests.

Signed-off-by: eapolinario <eapolinario@lyft.com>

jyotimahapatra

I think this is a great first step in setting up the e2e tests. Few more questions to brainstorm.

integration/e2e_test.go

jyotimahapatra · 2020-04-22T22:54:56Z

integration/testdata/envoy_bootstrap.yaml

+      grpc_services:
+      - envoy_grpc:
+          cluster_name: xds_cluster
+      set_node_on_first_message_only: true


https://www.envoyproxy.io/docs/envoy/latest/api-v2/api/v2/core/config_source.proto In our design we always expect xds relay to have this set_node_on_first_message_only to be false?

@jessicayuen wdyt about the constraint in our system?

its not a concern since gcp handles it https://github.com/envoyproxy/go-control-plane/blob/master/pkg/server/v2/server.go#L301-L305

Signed-off-by: eapolinario <eapolinario@lyft.com>

jessicayuen

Very neat! I just have a few minor comments:

integration/e2e_test.go

jessicayuen · 2020-04-23T19:25:45Z

Makefile

@@ -20,6 +20,10 @@ unit: ## Run all unit tests with coverage report
 integration-tests:  ## Run integration tests
 	go test -tags integration -v ./integration/

+.PHONY: e2e-tests
+e2e-tests: ## Run e2e tests


should this have a dependency on build-docker-image to create an image if it doesn't exist?

how about we check if the image is built in the /run_docker.sh script instead? Otherwise that script will not be super useful to run the tests locally.

Signed-off-by: eapolinario <eapolinario@lyft.com>

The xdsimpl integration tests doesn't need a envoy sidecar and can be written as integration tests rather than [e2e tests](#62). The integration tests help in strengthening the grpc mock behaviors in the upstream client unit tests. The tests use g-c-p server and xdsclient interacting over real grpc. Signed-off-by: Jyoti Mahapatra <jmahapatra@lyft.com>

eapolinario added 19 commits April 17, 2020 12:43

Setup initial envoy

83fba94

Signed-off-by: eapolinario <eapolinario@lyft.com>

snapshot cache + 1 envoy instance

c9b8cee

Signed-off-by: eapolinario <eapolinario@lyft.com>

Add cancellation of resources via context

81fd8f2

Signed-off-by: eapolinario <eapolinario@lyft.com>

Comment use of srv3

076e2bd

Signed-off-by: eapolinario <eapolinario@lyft.com>

Use of gomega.Eventually

ff328d4

Signed-off-by: eapolinario <eapolinario@lyft.com>

Add docker build tag

96d125f

Signed-off-by: eapolinario <eapolinario@lyft.com>

Merge branch 'master' into e2e-tests

e8ea5a7

Remove debug flag.

e06bf87

Signed-off-by: eapolinario <eapolinario@lyft.com>

Add a 1s wait between tests to allow for servers to be closed

e5160ce

Signed-off-by: eapolinario <eapolinario@lyft.com>

Comment use of PDEATHSIG.

c1afb97

Signed-off-by: eapolinario <eapolinario@lyft.com>

remove srv3

fa37d0b

Signed-off-by: eapolinario <eapolinario@lyft.com>

RunWithContext

e8bdae5

Signed-off-by: eapolinario <eapolinario@lyft.com>

Rename variables and rearrange functions.

b4e4f9e

Signed-off-by: eapolinario <eapolinario@lyft.com>

Add bootstrap configuration e2e test yaml file.

c95a479

Signed-off-by: eapolinario <eapolinario@lyft.com>

Mention why envoy is not hooked up to xds-relay server yet.

40bbd0b

Signed-off-by: eapolinario <eapolinario@lyft.com>

Setup dockerfile, make rule, and github workflow

b37de67

Signed-off-by: eapolinario <eapolinario@lyft.com>

fix path to root?

ba9e3eb

Signed-off-by: eapolinario <eapolinario@lyft.com>

non-interactive docker command.

18da595

Signed-off-by: eapolinario <eapolinario@lyft.com>

fix image build.

9bb2999

Signed-off-by: eapolinario <eapolinario@lyft.com>

jyotimahapatra reviewed Apr 20, 2020

View reviewed changes

jyotimahapatra mentioned this pull request Apr 20, 2020

xdsclient integration tests #63

Merged

eapolinario added 3 commits April 20, 2020 17:27

Add run_docker.sh script and update envoy version in image

cc367ac

Signed-off-by: eapolinario <eapolinario@lyft.com>

Be more lenient in the auto-documented make rules.

911b76c

Signed-off-by: eapolinario <eapolinario@lyft.com>

Review feedback

56e050f

Signed-off-by: eapolinario <eapolinario@lyft.com>

jyotimahapatra reviewed Apr 22, 2020

View reviewed changes

integration/e2e_test.go Outdated Show resolved Hide resolved

integration/e2e_test.go Show resolved Hide resolved

jyotimahapatra previously approved these changes Apr 22, 2020

View reviewed changes

jyotimahapatra reviewed Apr 22, 2020

View reviewed changes

Parametrize.

f50ef39

Signed-off-by: eapolinario <eapolinario@lyft.com>

eapolinario dismissed jyotimahapatra’s stale review via f50ef39 April 23, 2020 00:29

jyotimahapatra previously approved these changes Apr 23, 2020

View reviewed changes

jessicayuen reviewed Apr 23, 2020

View reviewed changes

This was referenced Apr 23, 2020

Hook up envoy to xds-relay instance in e2e test #65

Closed

Output envoy logs only in the case of failures #66

Open

Review feedback.

26ff408

Signed-off-by: eapolinario <eapolinario@lyft.com>

eapolinario dismissed jyotimahapatra’s stale review via 26ff408 April 23, 2020 22:20

eapolinario added 3 commits April 23, 2020 15:23

use Fatalf.

616239c

Signed-off-by: eapolinario <eapolinario@lyft.com>

Merge branch 'master' into e2e-tests

f052fd9

Signed-off-by: eapolinario <eapolinario@lyft.com>

Pipe in context.CancelFunc

fa64491

Signed-off-by: eapolinario <eapolinario@lyft.com>

eapolinario requested a review from jessicayuen April 24, 2020 03:06

jessicayuen approved these changes Apr 24, 2020

View reviewed changes

jessicayuen merged commit 22fd938 into envoyproxy:master Apr 24, 2020

eapolinario mentioned this pull request Apr 24, 2020

Set up integration test bed #42

Closed

jessicayuen linked an issue Jun 10, 2020 that may be closed by this pull request

Set up integration test bed #42

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

End-to-end tests #62

End-to-end tests #62

eapolinario commented Apr 20, 2020

jyotimahapatra left a comment •

edited

Loading

jyotimahapatra Apr 20, 2020

eapolinario Apr 20, 2020

eapolinario Apr 20, 2020

jyotimahapatra Apr 22, 2020

jessicayuen Apr 23, 2020

jyotimahapatra Apr 23, 2020 •

edited

Loading

eapolinario Apr 23, 2020

jyotimahapatra left a comment

jyotimahapatra Apr 22, 2020

jyotimahapatra Apr 22, 2020

jyotimahapatra Apr 22, 2020

jessicayuen left a comment

jessicayuen Apr 23, 2020

eapolinario Apr 23, 2020

End-to-end tests #62

End-to-end tests #62

Conversation

eapolinario commented Apr 20, 2020

jyotimahapatra left a comment • 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

jyotimahapatra Apr 23, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyotimahapatra left a 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

jessicayuen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jyotimahapatra left a comment •

edited

Loading

jyotimahapatra Apr 23, 2020 •

edited

Loading