Skip to content
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

test: Add non-aggregated CDS-over-gRPC integration test #5228

Merged
merged 54 commits into from
Jan 11, 2019

Conversation

fredlas
Copy link
Contributor

@fredlas fredlas commented Dec 5, 2018

Description: While trying to implement incremental CDS support in Envoy, I found it extremely difficult to work with the integration tests, which I hoped to adapt/copy for incremental. The only test using gRPC was ADS. I decided that a more manageable first step was to add a test for just (non-incremental) CDS over gRPC.
Risk Level: none, just a new test

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, this should be super useful to have on hand, much easier to understand than the ADS integration test.

test/integration/BUILD Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

Caused by: #5228 was synchronize by fredlas.

see: more, trace.

test/integration/BUILD Outdated Show resolved Hide resolved
test/integration/ads_integration_test.cc Outdated Show resolved Hide resolved
test/integration/ads_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/xds_integration_test_base.h Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@repokitteh-read-only
Copy link

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

Caused by: a #5228 (comment) was created by @htuch.

see: more, trace.

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas
Copy link
Contributor Author

fredlas commented Dec 20, 2018

Finally got it working, this time I'm pretty sure without any hacky workarounds or misunderstandings! It looks like the big issue was needing a way to defer the framework's check that listeners are listening. PTAL.

Also, a minor thing, while I'm making changes in IntegrationTestServer: would people be ok with renaming IntegrationTestServer::create() to createAndStart()? The first couple of times tracing through the code, I found myself surprised by how much create() was actually doing - i.e., calling start(), which does a lot.

Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas fredlas changed the title Add non-aggregated CDS-over-gRPC integration test test: Add non-aggregated CDS-over-gRPC integration test Dec 20, 2018
@fredlas
Copy link
Contributor Author

fredlas commented Dec 20, 2018

Wait, sorry, it's still sending the test request to the CDS server, because the static listener's route points to my_cds_cluster (in order to get the listener through initialization). I think I'm going to need to manually add a listener outside/after the normal initialization flow.

…t to

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
docs/root/configuration/cluster_manager/cluster_stats.rst Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/cds_integration_test.cc Outdated Show resolved Hide resolved
test/integration/http_integration.cc Outdated Show resolved Hide resolved
test/integration/integration.cc Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
…nstream

Signed-off-by: Fred Douglas <fredlas@google.com>
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo one final nit. Thanks for all the great cleanup work you've done in this PR.

test/integration/http_integration.cc Outdated Show resolved Hide resolved
Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
htuch
htuch previously approved these changes Jan 11, 2019
Copy link
Member

@htuch htuch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

Signed-off-by: Fred Douglas <fredlas@google.com>
@fredlas
Copy link
Contributor Author

fredlas commented Jan 11, 2019

Sorry, noticed some last-minute cleanup to be done!

@htuch htuch merged commit c84604c into envoyproxy:master Jan 11, 2019
@fredlas fredlas deleted the CDS_just_CDS branch January 24, 2019 19:45
@fredlas
Copy link
Contributor Author

fredlas commented Jan 28, 2019

Forgot to tag this as related to #4991.

fredlas added a commit to fredlas/envoy that referenced this pull request Mar 5, 2019
)

While trying to implement incremental CDS support in Envoy, I found it extremely difficult to work with the integration tests, which I hoped to adapt/copy for incremental. The only test using gRPC was ADS. I decided that a more manageable first step was to add a test for just (non-incremental) CDS over gRPC.

Risk Level: none, just a new test

Signed-off-by: Fred Douglas <fredlas@google.com>
Signed-off-by: Fred Douglas <fredlas@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants