Skip to content

Commit

Permalink
priority_set_ bug fix, pass endpoints test
Browse files Browse the repository at this point in the history 
Signed-off-by: Drew S. Ortega <drewortega@google.com>
  • Loading branch information
@drewsortega
drewsortega committed Sep 11, 2020
1 parent 0f2fafc commit 37a9bdf
Show file tree
Hide file tree
Showing 3 changed files with 165 additions and 8 deletions.
4 changes: 2 additions & 2 deletions source/common/upstream/health_discovery_service.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -459,7 +459,7 @@ void HdsCluster::updateHealthchecks(
void HdsCluster::updateHosts(
const Protobuf::RepeatedPtrField<envoy::config::endpoint::v3::LocalityLbEndpoints>&
locality_endpoints) {
HostVectorSharedPtr hosts;
HostVectorSharedPtr hosts = std::make_shared<std::vector<HostSharedPtr>>();
std::vector<HostSharedPtr> hosts_added;
std::vector<HostSharedPtr> hosts_removed;
std::vector<HostVector> hosts_by_locality;
Expand Down Expand Up @@ -499,7 +499,7 @@ void HdsCluster::updateHosts(
hosts_ = std::move(hosts);
hosts_map_ = std::move(hosts_map);

ENVOY_LOG(debug, "HOSTS ADDED: {}, REMOVED: {}, REUSED: {}", hosts_added.size(),
ENVOY_LOG(debug, "Hosts Added: {}, Removed: {}, Reused: {}", hosts_added.size(),
hosts_removed.size(), hosts_->size() - hosts_added.size());

hosts_per_locality_ =
Expand Down
4 changes: 0 additions & 4 deletions source/common/upstream/upstream_impl.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -576,10 +576,6 @@ void PrioritySetImpl::updateHosts(uint32_t priority, UpdateHostsParams&& update_
static_cast<HostSetImpl*>(host_sets_[priority].get())
->updateHosts(std::move(update_hosts_params), std::move(locality_weights), hosts_added,
hosts_removed, overprovisioning_factor);

This comment has been minimized.

Sign in to view

Copy link
@xdzhai

xdzhai Sep 11, 2020

Contributor

Changing this shared function is probably not desired, unless it has a bug, which should be fixed separately.
I think you are seeking for suggestion if there is any better workaround, you may add some comment here.

This comment was marked as outdated.

Sign in to view

Copy link
@drewsortega

drewsortega Sep 11, 2020 •
edited
Loading

Author Contributor

From what I understand, runUpdateCallbacks(...) is always called in the previous function above, HostSetImpl:: updateHosts(...) right at the end of the function. If batch_update_ is not set, then runUpdateCallbacks(...) is called twice in a row without any code in between. This causes issues when running with hosts_removed, but from my searching through the code I cant find any tests around this or much use of hosts_removed here in general. I suppose this is more of a bug than I had realized, I can peel this off into a different PR if you think that is best.

This comment has been minimized.

Sign in to view

Copy link
@xdzhai

xdzhai Sep 11, 2020

Contributor

I think you can leave it as-is for now (or add some comment to explain), and get reviewers' suggestion.
if (!batch_update_) {
runUpdateCallbacks(hosts_added, hosts_removed);
}
}

void PrioritySetImpl::batchHostUpdate(BatchUpdateCb& callback) {
Expand Down
165 changes: 163 additions & 2 deletions test/common/upstream/hds_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -747,9 +747,10 @@ TEST_F(HdsTest, TestClusterChange) {
EXPECT_CALL(dispatcher_, deferredDelete_(_)).Times(AtLeast(1));
// Process message
hds_delegate_->onReceiveMessage(std::move(message));
auto response1 = hds_delegate_->sendResponse();
hds_delegate_->sendResponse();

// Get cluster raw pointers to make sure they are the same addresses, that we reused them.
// Get cluster shared pointers to make sure they are the same memory addresses, that we reused
// them.
auto original_clusters = hds_delegate_->hdsClusters();
ASSERT_EQ(original_clusters.size(), 2);

Expand Down Expand Up @@ -791,5 +792,165 @@ TEST_F(HdsTest, TestClusterChange) {
checkHdsCounters(3, 0, 0, 3);
}

// Edit one of two cluster's endpoints by adding and removing.
TEST_F(HdsTest, TestUpdateEndpoints) {
EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_));
EXPECT_CALL(async_stream_, sendMessageRaw_(_, _));
createHdsDelegate();

// Create Message, and later add/remove endpoints from the second cluster.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 1, 2));

// Create a new active connection on request, setting its status to connected
// to mock a found endpoint.
EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _))
.WillRepeatedly(Invoke(
[](Network::Address::InstanceConstSharedPtr, Network::Address::InstanceConstSharedPtr,
Network::TransportSocketPtr&, const Network::ConnectionSocket::OptionsSharedPtr&) {
Network::MockClientConnection* connection =
new NiceMock<Network::MockClientConnection>();

// pretend our endpoint was connected to.
connection->raiseEvent(Network::ConnectionEvent::Connected);

// return this new, connected endpoint.
return connection;
}));

EXPECT_CALL(*server_response_timer_, enableTimer(_, _)).Times(AtLeast(1));
EXPECT_CALL(async_stream_, sendMessageRaw_(_, false));
EXPECT_CALL(test_factory_, createClusterInfo(_)).WillRepeatedly(Return(cluster_info_));
EXPECT_CALL(dispatcher_, deferredDelete_(_)).Times(AtLeast(1));
// Process message
hds_delegate_->onReceiveMessage(std::move(message));
hds_delegate_->sendResponse();

//
auto original_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(original_hosts.size(), 2);

// Add 3 endpoints to the specifier's second cluster. The first in the list should reuse pointers.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 1, 5));
hds_delegate_->onReceiveMessage(std::move(message));

// Get the new clusters list from HDS.
auto new_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(new_hosts.size(), 5);

// Make sure our first two endpoints are at the same address in memory as before.
for (int i = 0; i < 2; i++) {
EXPECT_EQ(original_hosts[i], new_hosts[i]);
}
EXPECT_TRUE(original_hosts[0] != new_hosts[2]);

// This time, have 4 endpoints, 2 each under 2 localities.
// The first locality will be reused, so its 2 endpoints will be as well.
// The second locality is new so we should be getting 2 new endpoints.
// Since the first locality had 5 but now has 2, we are removing 3.
// 2 ADDED, 3 REMOVED, 2 REUSED.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 2, 2));
hds_delegate_->onReceiveMessage(std::move(message));

// Get this new list of hosts.
auto final_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(final_hosts.size(), 4);

// Ensure the first two elements in the new list are reused.
for (int i = 0; i < 2; i++) {
EXPECT_EQ(new_hosts[i], final_hosts[i]);
}

// Ensure the first last two elements in the new list are different then the previous list.
for (int i = 2; i < 4; i++) {
EXPECT_TRUE(new_hosts[i] != final_hosts[i]);
}

// Check to see that HDS got three requests, and updated three times with it.
checkHdsCounters(3, 0, 0, 3);
}

// Test adding, reusing, and removing health checks.
TEST_F(HdsTest, TestUpdateEndpoints) {
EXPECT_CALL(*async_client_, startRaw(_, _, _, _)).WillOnce(Return(&async_stream_));
EXPECT_CALL(async_stream_, sendMessageRaw_(_, _));
createHdsDelegate();

// Create Message, and later add/remove endpoints from the second cluster.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 1, 2));

// Create a new active connection on request, setting its status to connected
// to mock a found endpoint.
EXPECT_CALL(dispatcher_, createClientConnection_(_, _, _, _))
.WillRepeatedly(Invoke(
[](Network::Address::InstanceConstSharedPtr, Network::Address::InstanceConstSharedPtr,
Network::TransportSocketPtr&, const Network::ConnectionSocket::OptionsSharedPtr&) {
Network::MockClientConnection* connection =
new NiceMock<Network::MockClientConnection>();

// pretend our endpoint was connected to.
connection->raiseEvent(Network::ConnectionEvent::Connected);

// return this new, connected endpoint.
return connection;
}));

EXPECT_CALL(*server_response_timer_, enableTimer(_, _)).Times(AtLeast(1));
EXPECT_CALL(async_stream_, sendMessageRaw_(_, false));
EXPECT_CALL(test_factory_, createClusterInfo(_)).WillRepeatedly(Return(cluster_info_));
EXPECT_CALL(dispatcher_, deferredDelete_(_)).Times(AtLeast(1));
// Process message
hds_delegate_->onReceiveMessage(std::move(message));
hds_delegate_->sendResponse();

//
auto original_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(original_hosts.size(), 2);

// Add 3 endpoints to the specifier's second cluster. The first in the list should reuse pointers.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 1, 5));
hds_delegate_->onReceiveMessage(std::move(message));

// Get the new clusters list from HDS.
auto new_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(new_hosts.size(), 5);

// Make sure our first two endpoints are at the same address in memory as before.
for (int i = 0; i < 2; i++) {
EXPECT_EQ(original_hosts[i], new_hosts[i]);
}
EXPECT_TRUE(original_hosts[0] != new_hosts[2]);

// This time, have 4 endpoints, 2 each under 2 localities.
// The first locality will be reused, so its 2 endpoints will be as well.
// The second locality is new so we should be getting 2 new endpoints.
// Since the first locality had 5 but now has 2, we are removing 3.
// 2 ADDED, 3 REMOVED, 2 REUSED.
message.reset(createSimpleMessage());
message->MergeFrom(*createComplexSpecifier(1, 2, 2));
hds_delegate_->onReceiveMessage(std::move(message));

// Get this new list of hosts.
auto final_hosts = hds_delegate_->hdsClusters()[1]->hosts();
ASSERT_EQ(final_hosts.size(), 4);

// Ensure the first two elements in the new list are reused.
for (int i = 0; i < 2; i++) {
EXPECT_EQ(new_hosts[i], final_hosts[i]);
}

// Ensure the first last two elements in the new list are different then the previous list.
for (int i = 2; i < 4; i++) {
EXPECT_TRUE(new_hosts[i] != final_hosts[i]);
}

// Check to see that HDS got three requests, and updated three times with it.
checkHdsCounters(3, 0, 0, 3);
}

} // namespace Upstream
} // namespace Envoy

0 comments on commit 37a9bdf

Please sign in to comment.