-
Notifications
You must be signed in to change notification settings - Fork 29
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
End-to-end tests #62
End-to-end tests #62
Conversation
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Few questions.
} | ||
|
||
func TestSnapshotCacheSingleEnvoyAndXdsRelayServer(t *testing.T) { | ||
if runtime.GOOS != "linux" { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@eapolinario Out of curiosity, did you test this locally on a linux system then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a great first step in setting up the e2e tests. Few more questions to brainstorm.
grpc_services: | ||
- envoy_grpc: | ||
cluster_name: xds_cluster | ||
set_node_on_first_message_only: true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jessicayuen wdyt about the constraint in our system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat! I just have a few minor comments:
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this have a dependency on build-docker-image
to create an image if it doesn't exist?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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>
Signed-off-by: eapolinario <eapolinario@lyft.com>
Signed-off-by: eapolinario <eapolinario@lyft.com>
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>
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.