upstream: fix bug causing crashes during priority host moves#6396
Merged
snowp merged 3 commits intoenvoyproxy:masterfrom Mar 27, 2019
Merged
upstream: fix bug causing crashes during priority host moves#6396snowp merged 3 commits intoenvoyproxy:masterfrom
snowp merged 3 commits intoenvoyproxy:masterfrom
Conversation
This fixes a bug where hosts that were moved between priorities would not be included in the hosts_added vector, resulting in crashes if the same host was moved multiple times when used with active health checking: if a host was moved between priorities twice, it would first get removed from the health checker, then on the second move the health checker would crash as it would attempt to remove a host it didn't know about. We fix this by explicitly adding the existing host to the list of added hosts iff the host was previously in a different priority. Uncovering this bug lead to the discovery of a bug in the batch updating done during EDS: std::set_difference assumes that the provided ranges are both *sorted*, which is not generally true during this update flow. This meant that the filtering of hosts that were added/removed did not work correctly, and would produce inconsistent result dependent on the ordering of the host pointers in the unordered_map. We fix this by using a standard for loop instead of std::set_difference. Not only is this more correct, it should also be faster for large sets as it performs the filtering in O(n) instead of O(n^2). Signed-off-by: Snow Pettersen <snowp@squareup.com>
added 2 commits
March 27, 2019 06:04
Signed-off-by: Snow Pettersen <snowp@squareup.com>
mattklein123
approved these changes
Mar 27, 2019
Member
mattklein123
left a comment
There was a problem hiding this comment.
Nice, thanks for fixing this. Can you add "Fixes " for both issues that this PR fixes into the description so they get closed and cross linked? Thank you!
Contributor
Author
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This fixes a bug where hosts that were moved between priorities would
not be included in the hosts_added vector, resulting in crashes if the
same host was moved multiple times when used with active health
checking: if a host was moved between priorities twice, it would first
get removed from the health checker, then on the second move the health
checker would crash as it would attempt to remove a host it didn't know
about.
We fix this by explicitly adding the existing host to the list of added
hosts iff the host existed previously in a different priority.
Uncovering this bug lead to the discovery of a bug in the batch updating
done during EDS: std::set_difference assumes that the provided ranges
are both sorted, which is not generally true during this update flow.
This meant that the filtering of hosts that were added/removed did not
work correctly, and would produce inconsistent result dependent on the
ordering of the host pointers in the unordered_map.
We fix this by using a standard for loop instead of std::set_difference.
Not only is this more correct, it should also be faster for large sets
as it performs the filtering in O(n) instead of O(n^2).
Signed-off-by: Snow Pettersen snowp@squareup.com
Risk Level: Medium
Testing: Extended EDS UT to prove that hosts aren't added/removed
Docs Changes: n/a
Release Notes: n/a
Fixes #6395