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.

Sign up for GitHub

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

Jump to bottom

Move EDS Clusters initialization after ADS #2760

Merged
merged 12 commits into from
Mar 11, 2018
Merged

Move EDS Clusters initialization after ADS #2760

merged 12 commits into from
Mar 11, 2018

Conversation

ramaraochavali
Copy link
Contributor

Fixes #2759

htuch
htuch suggested changes Mar 8, 2018
View reviewed changes
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 for starting work on the fix here. Should probably add a regression test.

source/common/upstream/cluster_manager_impl.cc Outdated
@@ -238,38 +263,13 @@ ClusterManagerImpl::ClusterManagerImpl(const envoy::config::bootstrap::v2::Boots
}
}

Optional<std::string> local_cluster_name;
Copy link
Member

Choose a reason for hiding this comment

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

Why did this need to move?

source/common/upstream/cluster_manager_impl.cc
// Once the initial set of static bootstrap clusters are created (including the local cluster),
// we can instantiate the thread local cluster manager.
tls_->set([this, local_cluster_name](
Event::Dispatcher& dispatcher) -> ThreadLocal::ThreadLocalObjectSharedPtr {
return std::make_shared<ThreadLocalClusterManagerImpl>(*this, dispatcher, local_cluster_name);
});

// Now setup ADS if needed, this might rely on a primary cluster and the
// thread local cluster manager.
Copy link
Member

@htuch htuch Mar 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This comment was there to point out that ADS init must happen after the above tls_->set. We need TLS to be able to initialize gRPC async client manager bits. I'm surprised the tests pass without this (specifically ads_integration_test with Envoy gRPC).

Copy link
Member

Choose a reason for hiding this comment

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

Actually, I think the need for this went away when I fixed the stats scope to be created on the main thread. Feel free to ignore this comment, if an issue arises I can go and fix it, since this is Grpc::AsyncClient implementation stuff.

@htuch htuch self-assigned this Mar 8, 2018
@ramaraochavali
Copy link
Contributor Author

@htuch addressed the feedback and added a ConfigIntegrationTest that validates this case. I verified that with old code that test fails and new code it passes. PTAL.

htuch
htuch suggested changes Mar 9, 2018
View reviewed changes
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 for the test, looks good, some minor feedback on some test improvements.

test/integration/ads_integration_test.cc Outdated
ads_cluster->MergeFrom(bootstrap.static_resources().clusters()[0]);
ads_cluster->set_name("ads_cluster");

// Add EDS Cluster that uses ADS as config Source.
Copy link
Member

Choose a reason for hiding this comment

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

Nit: s/Add EDS/Add EDS static/

test/integration/ads_integration_test.cc Outdated
ads_eds_cluster->set_type(envoy::api::v2::Cluster::EDS);
auto* eds_cluster_config = ads_eds_cluster->mutable_eds_cluster_config();
auto* eds_config = eds_cluster_config->mutable_eds_config();
eds_config->mutable_ads()->InitAsDefaultInstance();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: you can just write eds_config->mutable_ads();

test/integration/ads_integration_test.cc Outdated
@@ -231,6 +231,7 @@ class AdsIntegrationTest : public HttpIntegrationTest, public Grpc::GrpcClientIn
TestEnvironment::runfilesPath("test/config/integration/certs/upstreamcacert.pem"));
}
});

Copy link
Member

Choose a reason for hiding this comment

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

Nit: superfluous whitespace change.

test/integration/ads_integration_test.cc Outdated
INSTANTIATE_TEST_CASE_P(IpVersionsClientType, AdsConfigIntegrationTest,
GRPC_CLIENT_INTEGRATION_PARAMS);

// Validate that we don't crash on EDS Cluster that uses ADS as ConfigSource.
Copy link
Member

Choose a reason for hiding this comment

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

More nits: don't crash on EDS static cluster...

This is a regression test for .

test/integration/ads_integration_test.cc
ads_connection_ = fake_upstreams_[1]->waitForHttpConnection(*dispatcher_);
ads_stream_ = ads_connection_->waitForNewStream(*dispatcher_);
ads_stream_->startGrpcStream();
ads_stream_->finishGrpcStream(Grpc::Status::Ok);
Copy link
Member

Choose a reason for hiding this comment

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

Do you need all of this just to verify initialization?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

with out this, some ipv6 tests are failing.

Copy link
Member

Choose a reason for hiding this comment

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

This seems surprising. If you reduce it to initialize() only, it fails on IPv6 abut not IPv4?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried reducing it to waitForHttpConnection then ipv6 test failed. If I reduce to initialise the test is passes with old code also. That’s why added these additional steps

test/integration/ads_integration_test.cc
@@ -434,5 +435,58 @@ TEST_P(AdsFailIntegrationTest, ConnectDisconnect) {
ads_stream_->finishGrpcStream(Grpc::Status::Internal);
}

class AdsConfigIntegrationTest : public HttpIntegrationTest,
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if this could have been also done as a server unit test, i.e. just verifying the config loads. I think it's fine to do this here, but I'd also try and minify the amount of stuff we have; all we want to do is verify initialization with an ADS configuration. We don't care about setting up an ADS server or actually serving up ADS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i agree. I thought about it being a unit test. I added it here so that we could add ADS related config tests that ensure ADS connection works with config changes in future if needed.

htuch
htuch approved these changes Mar 9, 2018
View reviewed changes
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, ready to merge when OS X flake rerun completes.

@htuch htuch merged commit e6ff690 into envoyproxy:master Mar 11, 2018
@ramaraochavali ramaraochavali deleted the fix/ads_init branch March 12, 2018 04:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@htuch htuch htuch approved these changes

Assignees

@htuch htuch

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ramaraochavali @htuch