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

upstream: remove thread local cluster after triggering call backs #8004

Merged
merged 5 commits into from
Aug 23, 2019
Merged

upstream: remove thread local cluster after triggering call backs #8004

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
5834950
remove threadlocal cluster after callbacks
ramaraochavali Aug 21, 2019
1731889
Merge branch 'master' into fix/cluster_remove_crash
ramaraochavali Aug 22, 2019
bc02ce1
add an integration test
ramaraochavali Aug 22, 2019
554cafa
format
ramaraochavali Aug 22, 2019
16295bb
remove unnecessary line
ramaraochavali Aug 22, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion source/common/upstream/cluster_manager_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -553,10 +553,10 @@ bool ClusterManagerImpl::removeCluster(const std::string& cluster_name) {

ASSERT(cluster_manager.thread_local_clusters_.count(cluster_name) == 1);
ENVOY_LOG(debug, "removing TLS cluster {}", cluster_name);
cluster_manager.thread_local_clusters_.erase(cluster_name);
for (auto& cb : cluster_manager.update_callbacks_) {
cb->onClusterRemoval(cluster_name);
}
cluster_manager.thread_local_clusters_.erase(cluster_name);
});
}

Expand Down
2 changes: 1 addition & 1 deletion test/common/upstream/cluster_manager_impl_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1330,9 +1330,9 @@ TEST_F(ClusterManagerImplTest, DynamicAddRemove) {
// tcp connections.
Http::ConnectionPool::Instance::DrainedCb drained_cb;
Tcp::ConnectionPool::Instance::DrainedCb drained_cb2;
EXPECT_CALL(*callbacks, onClusterRemoval(_)).Times(1);
EXPECT_CALL(*cp, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb));
EXPECT_CALL(*cp2, addDrainedCallback(_)).WillOnce(SaveArg<0>(&drained_cb2));
EXPECT_CALL(*callbacks, onClusterRemoval(_)).Times(1);
EXPECT_TRUE(cluster_manager_->removeCluster("fake_cluster"));
EXPECT_EQ(nullptr, cluster_manager_->get("fake_cluster"));
EXPECT_EQ(0UL, cluster_manager_->clusters().size());
Expand Down
1 change: 1 addition & 0 deletions test/integration/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ envoy_cc_test_library(
"//source/common/config:protobuf_link_hacks",
"//source/common/config:resources_lib",
"//source/common/protobuf:utility_lib",
"//source/extensions/filters/network/redis_proxy:config",
"//test/common/grpc:grpc_client_integration_lib",
"//test/test_common:network_utility_lib",
"//test/test_common:utility_lib",
Expand Down
34 changes: 34 additions & 0 deletions test/integration/ads_integration.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,17 @@ envoy::api::v2::Cluster AdsIntegrationTest::buildCluster(const std::string& name
name));
}

envoy::api::v2::Cluster AdsIntegrationTest::buildRedisCluster(const std::string& name) {
return TestUtility::parseYaml<envoy::api::v2::Cluster>(fmt::format(R"EOF(
name: {}
connect_timeout: 5s
type: EDS
eds_cluster_config: {{ eds_config: {{ ads: {{}} }} }}
lb_policy: MAGLEV
)EOF",
name));
Copy link
Contributor

Choose a reason for hiding this comment

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

formatting seems off here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It is same as buildCluster method, I thought it is some kept it like for some reason. Let me try formatting both.

Copy link
Contributor Author

@ramaraochavali ramaraochavali Aug 23, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

If I change this and reformat, it is coming like this only. So may be formatter is like this. This is the line I am referring to https://github.com/envoyproxy/envoy/blob/master/test/integration/ads_integration.cc#L46. I think I will leave it as is unless you have better ideas on this,

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, that's fine

}

envoy::api::v2::ClusterLoadAssignment
AdsIntegrationTest::buildClusterLoadAssignment(const std::string& name) {
return TestUtility::parseYaml<envoy::api::v2::ClusterLoadAssignment>(
Expand Down Expand Up @@ -87,6 +98,29 @@ envoy::api::v2::Listener AdsIntegrationTest::buildListener(const std::string& na
name, Network::Test::getLoopbackAddressString(ipVersion()), stat_prefix, route_config));
}

envoy::api::v2::Listener AdsIntegrationTest::buildRedisListener(const std::string& name,
const std::string& cluster) {
return TestUtility::parseYaml<envoy::api::v2::Listener>(fmt::format(
R"EOF(
name: {}
address:
socket_address:
address: {}
port_value: 0
filter_chains:
filters:
- name: envoy.redis_proxy
config:
settings:
op_timeout: 1s
stat_prefix: {}
prefix_routes:
catch_all_route:
cluster: {}
)EOF",
name, Network::Test::getLoopbackAddressString(ipVersion()), name, cluster));
}

envoy::api::v2::RouteConfiguration
AdsIntegrationTest::buildRouteConfig(const std::string& name, const std::string& cluster) {
return TestUtility::parseYaml<envoy::api::v2::RouteConfiguration>(fmt::format(R"EOF(
Expand Down
4 changes: 4 additions & 0 deletions test/integration/ads_integration.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,15 @@ class AdsIntegrationTest : public Grpc::GrpcClientIntegrationParamTest, public H

envoy::api::v2::Cluster buildCluster(const std::string& name);

envoy::api::v2::Cluster buildRedisCluster(const std::string& name);

envoy::api::v2::ClusterLoadAssignment buildClusterLoadAssignment(const std::string& name);

envoy::api::v2::Listener buildListener(const std::string& name, const std::string& route_config,
const std::string& stat_prefix = "ads_test");

envoy::api::v2::Listener buildRedisListener(const std::string& name, const std::string& cluster);

envoy::api::v2::RouteConfiguration buildRouteConfig(const std::string& name,
const std::string& cluster);

Expand Down
40 changes: 40 additions & 0 deletions test/integration/ads_integration_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,46 @@ TEST_P(AdsIntegrationTest, DuplicateInitialClusters) {
test_server_->waitForCounterGe("cluster_manager.cds.update_rejected", 1);
}

// Validates that removing a redis cluster does not crash Envoy.
// Regression test for issue https://github.com/envoyproxy/envoy/issues/7990.
TEST_P(AdsIntegrationTest, RedisClusterRemoval) {
initialize();

// Send initial configuration with a redis cluster and a redis proxy listener.
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "", {}, {}, {}, true));
sendDiscoveryResponse<envoy::api::v2::Cluster>(Config::TypeUrl::get().Cluster,
{buildRedisCluster("redis_cluster")},
{buildRedisCluster("redis_cluster")}, {}, "1");

EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "",
{"redis_cluster"}, {}, {}));
sendDiscoveryResponse<envoy::api::v2::ClusterLoadAssignment>(
Config::TypeUrl::get().ClusterLoadAssignment, {buildClusterLoadAssignment("redis_cluster")},
{buildClusterLoadAssignment("redis_cluster")}, {}, "1");

EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Cluster, "1", {}, {}, {}));
EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "", {}, {}, {}));
sendDiscoveryResponse<envoy::api::v2::Listener>(
Config::TypeUrl::get().Listener, {buildRedisListener("listener_0", "redis_cluster")},
{buildRedisListener("listener_0", "redis_cluster")}, {}, "1");

EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().ClusterLoadAssignment, "1",
{"redis_cluster"}, {}, {}));

EXPECT_TRUE(compareDiscoveryRequest(Config::TypeUrl::get().Listener, "1", {}, {}, {}));

// Validate that redis listener is successfully created.
test_server_->waitForCounterGe("listener_manager.listener_create_success", 1);

// Now send a CDS update, removing redis cluster added above.
sendDiscoveryResponse<envoy::api::v2::Cluster>(
Config::TypeUrl::get().Cluster, {buildCluster("cluster_2")}, {buildCluster("cluster_2")},
{"redis_cluster"}, "2");

// Validate that the cluster is removed successfully.
test_server_->waitForCounterGe("cluster_manager.cluster_removed", 1);
}

// Validate that the request with duplicate clusters in the subsequent requests (warming clusters)
// is rejected.
TEST_P(AdsIntegrationTest, DuplicateWarmingClusters) {
Expand Down