Skip to content

Commit e718500

Browse files
rgs1htuch
rgs1
authored and
htuch
committed
Ensure subsets are updated when metadata changes (#3840)
This is a follow-up for #3810. Now that updates can be fired without hosts being added or removed, we need to handle this case and check if subsets need to be added or removed when only metadata of existing endpoints has changed. Fixes #3839 Signed-off-by: Raul Gutierrez Segales <[email protected]>
1 parent 218d112 commit e718500

File tree

3 files changed

+218
-4
lines changed

3 files changed

+218
-4
lines changed

‎source/common/upstream/subset_lb.cc‎

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -31,19 +31,42 @@ SubsetLoadBalancer::SubsetLoadBalancer(
3131
ASSERT(subsets.isEnabled());
3232

3333
// Create filtered default subset (if necessary) and other subsets based on current hosts.
34-
for (auto& host_set : priority_set.hostSetsPerPriority()) {
35-
update(host_set->priority(), host_set->hosts(), {});
36-
}
34+
refreshSubsets();
3735

3836
// Configure future updates.
3937
original_priority_set_callback_handle_ = priority_set.addMemberUpdateCb(
4038
[this](uint32_t priority, const HostVector& hosts_added, const HostVector& hosts_removed) {
41-
update(priority, hosts_added, hosts_removed);
39+
if (!hosts_added.size() && !hosts_removed.size()) {
40+
// It's possible that metadata changed, without hosts being added nor removed.
41+
// If so we need to add any new subsets, remove unused ones, and regroup hosts into
42+
// the right subsets.
43+
//
44+
// Note, note, note: if metadata for existing endpoints changed _and_ hosts were also
45+
// added or removed, we don't need to hit this path. That's fine, given that
46+
// findOrCreateSubset() will be called from processSubsets because it'll be triggered by
47+
// either hosts_added or hosts_removed. That's where the new subsets will be created.
48+
refreshSubsets(priority);
49+
} else {
50+
// This is a regular update with deltas.
51+
update(priority, hosts_added, hosts_removed);
52+
}
4253
});
4354
}
4455

4556
SubsetLoadBalancer::~SubsetLoadBalancer() { original_priority_set_callback_handle_->remove(); }
4657

58+
void SubsetLoadBalancer::refreshSubsets() {
59+
for (auto& host_set : original_priority_set_.hostSetsPerPriority()) {
60+
update(host_set->priority(), host_set->hosts(), {});
61+
}
62+
}
63+
64+
void SubsetLoadBalancer::refreshSubsets(uint32_t priority) {
65+
const auto& host_sets = original_priority_set_.hostSetsPerPriority();
66+
ASSERT(priority < host_sets.size());
67+
update(priority, host_sets[priority]->hosts(), {});
68+
}
69+
4770
HostConstSharedPtr SubsetLoadBalancer::chooseHost(LoadBalancerContext* context) {
4871
if (context) {
4972
bool host_chosen;

‎source/common/upstream/subset_lb.h‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -108,6 +108,10 @@ class SubsetLoadBalancer : public LoadBalancer, Logger::Loggable<Logger::Id::ups
108108
PrioritySubsetImplPtr priority_subset_;
109109
};
110110

111+
// Create filtered default subset (if necessary) and other subsets based on current hosts.
112+
void refreshSubsets();
113+
void refreshSubsets(uint32_t priority);
114+
111115
// Called by HostSet::MemberUpdateCb
112116
void update(uint32_t priority, const HostVector& hosts_added, const HostVector& hosts_removed);
113117

‎test/common/upstream/subset_lb_test.cc‎

Lines changed: 187 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -323,6 +323,25 @@ class SubsetLoadBalancerTest : public testing::TestWithParam<UpdateOrder> {
323323
EXPECT_EQ(added_host, lb_->chooseHost(nullptr));
324324
}
325325

326+
envoy::api::v2::core::Metadata buildMetadata(const std::string& version,
327+
bool is_default = false) const {
328+
envoy::api::v2::core::Metadata metadata;
329+
330+
if (version != "") {
331+
Envoy::Config::Metadata::mutableMetadataValue(
332+
metadata, Config::MetadataFilters::get().ENVOY_LB, "version")
333+
.set_string_value(version);
334+
}
335+
336+
if (is_default) {
337+
Envoy::Config::Metadata::mutableMetadataValue(
338+
metadata, Config::MetadataFilters::get().ENVOY_LB, "default")
339+
.set_string_value("true");
340+
}
341+
342+
return metadata;
343+
}
344+
326345
LoadBalancerType lb_type_{LoadBalancerType::RoundRobin};
327346
NiceMock<MockPrioritySet> priority_set_;
328347
MockHostSet& host_set_ = *priority_set_.getMockHostSet(0);
@@ -554,6 +573,174 @@ TEST_P(SubsetLoadBalancerTest, UpdateFailover) {
554573
EXPECT_FALSE(nullptr == lb_->chooseHost(&context_10).get());
555574
}
556575

576+
TEST_P(SubsetLoadBalancerTest, OnlyMetadataChanged) {
577+
TestLoadBalancerContext context_10({{"version", "1.0"}});
578+
TestLoadBalancerContext context_12({{"version", "1.2"}});
579+
TestLoadBalancerContext context_13({{"version", "1.3"}});
580+
TestLoadBalancerContext context_default({{"default", "true"}});
581+
const ProtobufWkt::Struct default_subset = makeDefaultSubset({{"default", "true"}});
582+
583+
EXPECT_CALL(subset_info_, defaultSubset()).WillRepeatedly(ReturnRef(default_subset));
584+
EXPECT_CALL(subset_info_, fallbackPolicy())
585+
.WillRepeatedly(Return(envoy::api::v2::Cluster::LbSubsetConfig::DEFAULT_SUBSET));
586+
587+
std::vector<std::set<std::string>> subset_keys = {{"version"}, {"default"}};
588+
EXPECT_CALL(subset_info_, subsetKeys()).WillRepeatedly(ReturnRef(subset_keys));
589+
590+
// Add hosts initial hosts.
591+
init({{"tcp://127.0.0.1:8000", {{"version", "1.2"}}},
592+
{"tcp://127.0.0.1:8001", {{"version", "1.0"}, {"default", "true"}}}});
593+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
594+
EXPECT_EQ(3U, stats_.lb_subsets_created_.value());
595+
EXPECT_EQ(0U, stats_.lb_subsets_removed_.value());
596+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
597+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
598+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_default));
599+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
600+
601+
// Swap the default version.
602+
host_set_.hosts_[0]->metadata(buildMetadata("1.2", true));
603+
host_set_.hosts_[1]->metadata(buildMetadata("1.0"));
604+
605+
host_set_.runCallbacks({}, {});
606+
607+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
608+
EXPECT_EQ(3U, stats_.lb_subsets_created_.value());
609+
EXPECT_EQ(0U, stats_.lb_subsets_removed_.value());
610+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
611+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
612+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_default));
613+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_13));
614+
615+
// Bump 1.0 to 1.3, one subset should be removed.
616+
host_set_.hosts_[1]->metadata(buildMetadata("1.3"));
617+
618+
// No hosts added nor removed, so we bypass modifyHosts().
619+
host_set_.runCallbacks({}, {});
620+
621+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
622+
EXPECT_EQ(4U, stats_.lb_subsets_created_.value());
623+
EXPECT_EQ(1U, stats_.lb_subsets_removed_.value());
624+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
625+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
626+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_default));
627+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_10));
628+
629+
// Rollback from 1.3 to 1.0.
630+
host_set_.hosts_[1]->metadata(buildMetadata("1.0"));
631+
632+
host_set_.runCallbacks({}, {});
633+
634+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
635+
EXPECT_EQ(5U, stats_.lb_subsets_created_.value());
636+
EXPECT_EQ(2U, stats_.lb_subsets_removed_.value());
637+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
638+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
639+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_default));
640+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_13));
641+
642+
// Make 1.0 default again.
643+
host_set_.hosts_[1]->metadata(buildMetadata("1.0", true));
644+
host_set_.hosts_[0]->metadata(buildMetadata("1.2"));
645+
646+
host_set_.runCallbacks({}, {});
647+
648+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
649+
EXPECT_EQ(5U, stats_.lb_subsets_created_.value());
650+
EXPECT_EQ(2U, stats_.lb_subsets_removed_.value());
651+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
652+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
653+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_default));
654+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
655+
}
656+
657+
TEST_P(SubsetLoadBalancerTest, MetadataChangedHostsAddedRemoved) {
658+
TestLoadBalancerContext context_10({{"version", "1.0"}});
659+
TestLoadBalancerContext context_12({{"version", "1.2"}});
660+
TestLoadBalancerContext context_13({{"version", "1.3"}});
661+
TestLoadBalancerContext context_14({{"version", "1.4"}});
662+
TestLoadBalancerContext context_default({{"default", "true"}});
663+
const ProtobufWkt::Struct default_subset = makeDefaultSubset({{"default", "true"}});
664+
665+
EXPECT_CALL(subset_info_, defaultSubset()).WillRepeatedly(ReturnRef(default_subset));
666+
EXPECT_CALL(subset_info_, fallbackPolicy())
667+
.WillRepeatedly(Return(envoy::api::v2::Cluster::LbSubsetConfig::DEFAULT_SUBSET));
668+
669+
std::vector<std::set<std::string>> subset_keys = {{"version"}, {"default"}};
670+
EXPECT_CALL(subset_info_, subsetKeys()).WillRepeatedly(ReturnRef(subset_keys));
671+
672+
// Add hosts initial hosts.
673+
init({{"tcp://127.0.0.1:8000", {{"version", "1.2"}}},
674+
{"tcp://127.0.0.1:8001", {{"version", "1.0"}, {"default", "true"}}}});
675+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
676+
EXPECT_EQ(3U, stats_.lb_subsets_created_.value());
677+
EXPECT_EQ(0U, stats_.lb_subsets_removed_.value());
678+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
679+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
680+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_default));
681+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
682+
683+
// Swap the default version.
684+
host_set_.hosts_[0]->metadata(buildMetadata("1.2", true));
685+
host_set_.hosts_[1]->metadata(buildMetadata("1.0"));
686+
687+
// Add a new host.
688+
modifyHosts({makeHost("tcp://127.0.0.1:8002", {{"version", "1.3"}})}, {});
689+
690+
EXPECT_EQ(4U, stats_.lb_subsets_active_.value());
691+
EXPECT_EQ(4U, stats_.lb_subsets_created_.value());
692+
EXPECT_EQ(0U, stats_.lb_subsets_removed_.value());
693+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
694+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
695+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_default));
696+
EXPECT_EQ(host_set_.hosts_[2], lb_->chooseHost(&context_13));
697+
698+
// Swap default again and remove the previous one.
699+
host_set_.hosts_[0]->metadata(buildMetadata("1.2"));
700+
host_set_.hosts_[1]->metadata(buildMetadata("1.0", true));
701+
702+
modifyHosts({}, {host_set_.hosts_[2]});
703+
704+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
705+
EXPECT_EQ(4U, stats_.lb_subsets_created_.value());
706+
EXPECT_EQ(1U, stats_.lb_subsets_removed_.value());
707+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
708+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
709+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_default));
710+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
711+
712+
// Swap the default version once more, this time adding a new host and removing
713+
// the current default version.
714+
host_set_.hosts_[0]->metadata(buildMetadata("1.2", true));
715+
host_set_.hosts_[1]->metadata(buildMetadata("1.0"));
716+
717+
modifyHosts({makeHost("tcp://127.0.0.1:8003", {{"version", "1.4"}})}, {host_set_.hosts_[1]});
718+
719+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
720+
EXPECT_EQ(5U, stats_.lb_subsets_created_.value());
721+
EXPECT_EQ(2U, stats_.lb_subsets_removed_.value());
722+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
723+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_10));
724+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_default));
725+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_13));
726+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_14));
727+
728+
// Make 1.4 default, without hosts being added/removed.
729+
host_set_.hosts_[0]->metadata(buildMetadata("1.2"));
730+
host_set_.hosts_[1]->metadata(buildMetadata("1.4", true));
731+
732+
host_set_.runCallbacks({}, {});
733+
734+
EXPECT_EQ(3U, stats_.lb_subsets_active_.value());
735+
EXPECT_EQ(5U, stats_.lb_subsets_created_.value());
736+
EXPECT_EQ(2U, stats_.lb_subsets_removed_.value());
737+
EXPECT_EQ(host_set_.hosts_[0], lb_->chooseHost(&context_12));
738+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_10));
739+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_default));
740+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_13));
741+
EXPECT_EQ(host_set_.hosts_[1], lb_->chooseHost(&context_14));
742+
}
743+
557744
TEST_P(SubsetLoadBalancerTest, UpdateRemovingLastSubsetHost) {
558745
EXPECT_CALL(subset_info_, fallbackPolicy())
559746
.WillRepeatedly(Return(envoy::api::v2::Cluster::LbSubsetConfig::ANY_ENDPOINT));

0 commit comments

Comments
 (0)