Fix healthchecks that do not persist when priority is changed #35748

shulin-sq · 2024-08-20T01:46:00Z

Commit Message: Use previously calculated healthcheck when endpoints move priority levels repeatedly. This is done by changing how mutable_cross_priority_host_map_ tracks hosts. mutable_cross_priority_host_map_ is used to track if a host already exists in the previous configuration, and its healthcheck should not be recalculated. This worked only some of the time because it would remove and then add all hosts that have changed in a priority, to a map of string (ip addr:port) to Host instance. However this did not account for when an endpoint had two Host representatives in different priorities, as is in the case when an endpoint changes priorities and there is an edge case where a host can be removed from mutable_cross_priority_host_map_ before the "should we skip activate healthchecks" logic triggers. This PR fixes that by only removing an endpoint from mutable_cross_priority_host_map_ if that removal is executed from the lowest priority. This fix does assume that memberships in priorities are always calculated starting from the lowest number to the highest.

Additional Description:
Risk Level: Med?
Testing: see note at the bottom
Docs Changes: n/a
Release Notes: added a note to the changelog
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue] #35243
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

Explanation

This was a difficult bug to spot because it only happens when priority changes multiple times

For example let's consider this situation:

we have an endpoint A that swaps between priority 0, 1, and 2
we assume that priority is always processed starting from 0 and going up (eg, 0, 1, 2, 3, 4... etc)
mutable_cross_priority_host_map_ is the "list" in the situation that includes all endpoints from all priorities

When priority number goes up things are ok

0 -> 1
processing priority 0: remove A
processing priority 1: add A

1 -> 2
priority 1: remove A
priority 2: add A

but things get weird when numbers go down

2 -> 1, things are still peaceful here
priority 1: add A (but this gets ignored since A is already in the list)
priority 2: remove A (!!!)

1 -> 0, at this point the list does not include A, so any logic that checks if A exists in the cross priorty host map will fail and A will be considered as a new endpoint.

without the fix: https://gist.github.com/shulin-sq/adfb4268f5f199f054e908e3fd7afae8
with the fix:
https://gist.github.com/shulin-sq/7779a341e598d81cfaeca447b0f582d1

Testing

functional test described in the issue: Do not redo healthchecks for endpoints with different priorities #35243
unit test, confirmed that it fails before the fix is applied

repokitteh-read-only · 2024-08-20T01:46:05Z

Hi @shulin-sq, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #35748 was opened by shulin-sq.

see: more, trace.

adisuissa · 2024-08-20T13:38:43Z

Thanks for tackling this!
Can you please fix DCO and format?

shulin-sq · 2024-08-20T17:37:55Z

@adisuissa thanks for pointing it out!

I made some changes

ran the format script tools/local_fix_format.sh -all
added DCO
squashed my commits
fixed some of my comments on the new part of the unit test

mpuncel · 2024-08-21T16:18:17Z

test/extensions/clusters/eds/eds_test.cc

@@ -1403,6 +1403,133 @@ TEST_F(EdsTest, EndpointMovedToNewPriority) {
    EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC));
    EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL));
  }
+
+  // 1 -> 2
+  // Moves all the endpoints to priority 1.


priority 2 right?

yes you're right. I'll just delete this line since I don't mention it below.

shulin-sq · 2024-08-21T17:15:34Z

source/common/upstream/upstream_impl.cc

@@ -987,7 +988,18 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(const HostVector& hosts_add
  }

  for (const auto& host : hosts_removed) {
-    mutable_cross_priority_host_map_->erase(addressToString(host->address()));
+    auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address()));


@adisuissa is there a preference to use find() vs [] in the project? https://github.com/abseil/abseil-cpp/blob/5a01d0f77e37493570e35aedaa95c4bcf1673c7c/absl/container/internal/raw_hash_set.h#L3361 find might be O(n) as opposed to O(1) but I saw a few other places in the codebase using find with maps as well

Can you refer me to where you saw that the time complexity of find is O(n) and of the operator is O(1)?
I think both should be O(1) average complexity, and O(n) worst-case (due to hash conflicts), but I may be mistaken.

https://github.com/abseil/abseil-cpp/blob/master/absl/container/internal/raw_hash_set.h#L3545 I don't know this code well but also couldn't find any docs that explain this definitively. I will defer to your recommendation. I would be fine with keeping it since other places use find() to access this map this is an established pattern

adisuissa

Thanks for tracking and fixing this issue.
Overall LGTM, modulo minor comments.

adisuissa · 2024-08-22T14:31:32Z

changelogs/current.yaml

@@ -2,6 +2,9 @@ date: Pending

 behavior_changes:
 # *Changes that are expected to cause an incompatibility if applicable; deployment changes are likely required*
+- area: upstream


this should probably move down to the "bug fixes" section.

adisuissa · 2024-08-22T14:33:27Z

source/common/upstream/upstream_impl.cc

@@ -987,7 +988,18 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(const HostVector& hosts_add
  }

  for (const auto& host : hosts_removed) {
-    mutable_cross_priority_host_map_->erase(addressToString(host->address()));
+    auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address()));


nit: create a (const) temp var holding the addressToString(host->address()) result, and pass it here and below.

adisuissa · 2024-08-22T14:39:18Z

source/common/upstream/upstream_impl.cc

@@ -987,7 +988,18 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(const HostVector& hosts_add
  }

  for (const auto& host : hosts_removed) {
-    mutable_cross_priority_host_map_->erase(addressToString(host->address()));
+    auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address()));


Can you refer me to where you saw that the time complexity of find is O(n) and of the operator is O(1)?
I think both should be O(1) average complexity, and O(n) worst-case (due to hash conflicts), but I may be mistaken.

adisuissa · 2024-08-22T14:40:11Z

source/common/upstream/upstream_impl.cc

+    auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address()));
+
+    if (existing_host != mutable_cross_priority_host_map_->end()) {
+      // an earlier priority added this host, which indicates that the host moved from


style-nit: comments start with capital letter.

adisuissa · 2024-08-22T14:41:14Z

source/common/upstream/upstream_impl.cc

+      if (existing_host->second->priority() < priority) {
+        continue;
+      } else {
+        mutable_cross_priority_host_map_->erase(addressToString(host->address()));
+      }


Please convert to if (existing_host->second->priority() >= priority) ... and remove the continue part.

adisuissa · 2024-08-22T14:43:14Z

test/extensions/clusters/eds/eds_test.cc

@@ -1403,6 +1403,133 @@ TEST_F(EdsTest, EndpointMovedToNewPriority) {
    EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::FAILED_ACTIVE_HC));
    EXPECT_FALSE(hosts[1]->healthFlagGet(Host::HealthFlag::PENDING_DYNAMIC_REMOVAL));
  }
+
+  // 1 -> 2


I think this bug should have its specific test (and please add a comment for the regression issue at the top of the test).

adisuissa · 2024-08-22T14:44:06Z

source/common/upstream/upstream_impl.cc

@@ -987,7 +988,18 @@ void MainPrioritySetImpl::updateCrossPriorityHostMap(const HostVector& hosts_add
  }

  for (const auto& host : hosts_removed) {
-    mutable_cross_priority_host_map_->erase(addressToString(host->address()));
+    auto existing_host = mutable_cross_priority_host_map_->find(addressToString(host->address()));


adisuissa · 2024-08-22T14:46:36Z

Assigning @wbpcode for senior-maintainer review.
/assign @wbpcode

adisuissa · 2024-08-23T15:58:56Z

/wait

shulin-sq · 2024-08-26T17:06:50Z

/retest

shulin-sq · 2024-08-26T21:41:13Z

@adisuissa @wbpcode I have updated the PR to address the comments from the last round.

ggreenway · 2024-08-29T20:41:14Z

@wbpcode gentle nudge to review

wbpcode · 2024-08-30T01:48:02Z

I will get some free time to review this PR this weekend. Thanks. :)

wbpcode

LGTM overall with only one comment. Thanks for your contribution.

/wait-any

wbpcode · 2024-08-31T14:47:40Z

source/common/upstream/upstream_impl.cc

+      // An earlier priority added this host, which indicates that the host moved from
+      // a higher value priority to a lower value priority. We should not remove it from the
+      // cross priority map.
+      if (existing_host->second->priority() >= priority) {


when will the > happens? Or should we only do the erasing when the priority is equal?

PS: I think this is a design problem of corss_priority_map which I can improve in future.

changed, thanks for spotting. I think the thing I was thinking of is what I described in the comment

// An earlier priority added this host, which indicates that the host moved from // a higher value priority to a lower value priority. We should not remove it from the // cross priority map.

but when the operators flipped it's more obvious that we only need to handle deletions when the priorities are equal. I've also updated the comment accordingly.

Regarding the design of cross_priority_map, I did think that because it's keyed on just the endpoint address, this seems to not capture differences of metadata of the endpoint but given that I was only focused on the priority part I wasn't sure if there was any other bugs here.

Yeah, it initially is designed for lb to search a address quickly which could tolerate minor inaccuracy. But now, I need to re-think it.

wbpcode

LGTM. Thanks.

wbpcode · 2024-09-04T14:12:18Z

Could you merge the main to kick the CI?

…vels repeatedly. This is done by changing how mutable_cross_priority_host_map_ tracks hosts. mutable_cross_priority_host_map_ is used to track if a host already exists in the previous configuration, and its healthcheck should not be recalculated. This worked only some of the time because it would remove and then add all hosts that have changed in a priority, to a map of string (ip addr:port) to Host instance. However this did not account for when an endpoint had two Host representatives in different priorities, as is in the case when an endpoint changes priorities and there is an edge case where a host can be removed from mutable_cross_priority_host_map_ before the "should we skip activate healthchecks" logic triggers. This PR fixes that by only removing an endpoint from mutable_cross_priority_host_map_ if that removal is executed from the lowest priority. This fix does assume that memberships in priorities are always calculated starting from the lowest number to the highest. Signed-off-by: Shulin Jia <shulin@squareup.com>

shulin-sq · 2024-09-04T20:09:42Z

@wbpcode done. Can you help with merging?

shulin-sq · 2024-09-06T18:57:34Z

@wbpcode just checking in, I did a rebase from master. did that not kick off the CI?

adisuissa self-assigned this Aug 20, 2024

shulin-sq force-pushed the shulin/fixprioritymovement branch 2 times, most recently from 430b3f6 to 993b5e1 Compare August 20, 2024 17:36

shulin-sq force-pushed the shulin/fixprioritymovement branch from 993b5e1 to e2659f1 Compare August 21, 2024 03:37

mpuncel reviewed Aug 21, 2024

View reviewed changes

shulin-sq commented Aug 21, 2024

View reviewed changes

adisuissa reviewed Aug 22, 2024

View reviewed changes

repokitteh-read-only bot assigned wbpcode Aug 22, 2024

repokitteh-read-only bot added waiting and removed waiting labels Aug 23, 2024

shulin-sq force-pushed the shulin/fixprioritymovement branch from a871846 to 94cfeab Compare August 23, 2024 19:05

wbpcode reviewed Aug 31, 2024

View reviewed changes

repokitteh-read-only bot added the waiting:any label Aug 31, 2024

shulin-sq force-pushed the shulin/fixprioritymovement branch from 94cfeab to 0ec9935 Compare September 3, 2024 18:41

repokitteh-read-only bot removed the waiting:any label Sep 3, 2024

shulin-sq force-pushed the shulin/fixprioritymovement branch from 0ec9935 to fd7e2ba Compare September 3, 2024 18:46

wbpcode approved these changes Sep 4, 2024

View reviewed changes

shulin-sq force-pushed the shulin/fixprioritymovement branch from fd7e2ba to 51d6ff0 Compare September 4, 2024 17:08

wbpcode merged commit 0a00922 into envoyproxy:main Sep 7, 2024
39 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix healthchecks that do not persist when priority is changed #35748

Fix healthchecks that do not persist when priority is changed #35748

shulin-sq commented Aug 20, 2024 •

edited

Loading

repokitteh-read-only bot commented Aug 20, 2024

adisuissa commented Aug 20, 2024

shulin-sq commented Aug 20, 2024

mpuncel Aug 21, 2024

shulin-sq Aug 21, 2024

shulin-sq Aug 21, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

shulin-sq Aug 22, 2024

adisuissa left a comment

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa Aug 22, 2024

adisuissa commented Aug 22, 2024

adisuissa commented Aug 23, 2024

shulin-sq commented Aug 26, 2024

shulin-sq commented Aug 26, 2024

ggreenway commented Aug 29, 2024

wbpcode commented Aug 30, 2024

wbpcode left a comment

wbpcode Aug 31, 2024

shulin-sq Sep 3, 2024

wbpcode Sep 4, 2024

wbpcode left a comment

wbpcode commented Sep 4, 2024

shulin-sq commented Sep 4, 2024

shulin-sq commented Sep 6, 2024

Fix healthchecks that do not persist when priority is changed #35748

Fix healthchecks that do not persist when priority is changed #35748

Conversation

shulin-sq commented Aug 20, 2024 • edited Loading

Explanation

Testing

repokitteh-read-only bot commented Aug 20, 2024

adisuissa commented Aug 20, 2024

shulin-sq commented Aug 20, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adisuissa commented Aug 22, 2024

adisuissa commented Aug 23, 2024

shulin-sq commented Aug 26, 2024

shulin-sq commented Aug 26, 2024

ggreenway commented Aug 29, 2024

wbpcode commented Aug 30, 2024

wbpcode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wbpcode left a comment

Choose a reason for hiding this comment

wbpcode commented Sep 4, 2024

shulin-sq commented Sep 4, 2024

shulin-sq commented Sep 6, 2024

shulin-sq commented Aug 20, 2024 •

edited

Loading