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.

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

A workaround for ring hash on channel id to randomize the channel id #28986

Merged
merged 3 commits into from
Mar 2, 2022

Conversation

donnadionne
Copy link
Contributor

@donnadionne donnadionne commented Mar 1, 2022

(ref to xds_resolver) used.

We are using a ref to xds_resolver as the channel id for ring hash on channel id; this creates a problem that there is not enough spreading in the references to distribute to multiple backends for each channel.

A work around is to do a hash on the address and use it as the hash. This ensures for traffic on the same channel, using the same xds_resolver, will go to the same backend. This will also ensure some spreading.

Before workaround:

I0228 21:42:52.633404885 25 ring_hash.cc:437] donna h is 135652247085056 and first index is 0
I0228 21:42:52.636130779 25 ring_hash.cc:437] donna h is 135652247086080 and first index is 0
I0228 21:42:52.638996658 25 ring_hash.cc:437] donna h is 135652247088128 and first index is 0
I0228 21:42:52.641462784 25 ring_hash.cc:437] donna h is 135652247081984 and first index is 0
I0228 21:42:52.644984828 25 ring_hash.cc:437] donna h is 135652247085056 and first index is 0
I0228 21:42:52.647703412 25 ring_hash.cc:437] donna h is 135652247086080 and first index is 0
I0228 21:42:52.650517577 25 ring_hash.cc:437] donna h is 135652247088128 and first index is 0
I0228 21:42:52.653391423 25 ring_hash.cc:437] donna h is 135652247081984 and first index is 0
I0228 21:42:52.656102427 25 ring_hash.cc:437] donna h is 135652247085056 and first index is 0
I0228 21:42:52.659303063 25 ring_hash.cc:437] donna h is 135652247086080 and first index is 0
I0228 21:42:52.661962661 25 ring_hash.cc:437] donna h is 135652247088128 and first index is 0
I0228 21:42:52.664343137 25 ring_hash.cc:437] donna h is 135652247081984 and first index is 0
I0228 21:42:52.667011629 25 ring_hash.cc:437] donna h is 135652247085056 and first index is 0
I0228 21:42:52.669439107 25 ring_hash.cc:437] donna h is 135652247086080 and first index is 0
I0228 21:42:52.672174492 25 ring_hash.cc:437] donna h is 135652247088128 and first index is 0
I0228 21:42:52.674472235 25 xds_end2end_test.cc:6764] donna backend 0 getting 0
I0228 21:42:52.674577585 25 xds_end2end_test.cc:6764] donna backend 1 getting 100
I0228 21:42:52.674776205 25 xds_end2end_test.cc:6764] donna backend 2 getting 0
I0228 21:42:52.674929128 25 xds_end2end_test.cc:6764] donna backend 3 getting 0

After workaround:

I0301 02:49:49.624318937 23 ring_hash.cc:437] donna h is 3403279667131824387 and first index is 165
I0301 02:49:49.627107514 23 ring_hash.cc:437] donna h is 12945578205449051951 and first index is 692
I0301 02:49:49.629585388 23 ring_hash.cc:437] donna h is 6269367244575526413 and first index is 311
I0301 02:49:49.632590894 23 ring_hash.cc:437] donna h is 8096581829639078299 and first index is 430
I0301 02:49:49.635211980 23 ring_hash.cc:437] donna h is 3403279667131824387 and first index is 165
I0301 02:49:49.638068052 23 ring_hash.cc:437] donna h is 12945578205449051951 and first index is 692
I0301 02:49:49.640925917 23 ring_hash.cc:437] donna h is 6269367244575526413 and first index is 311
I0301 02:49:49.643497287 23 ring_hash.cc:437] donna h is 8096581829639078299 and first index is 430
I0301 02:49:49.646521646 23 ring_hash.cc:437] donna h is 3403279667131824387 and first index is 165
I0301 02:49:49.651473849 23 ring_hash.cc:437] donna h is 12945578205449051951 and first index is 692
I0301 02:49:49.654546284 23 ring_hash.cc:437] donna h is 6269367244575526413 and first index is 311
I0301 02:49:49.657680230 23 ring_hash.cc:437] donna h is 8096581829639078299 and first index is 430
I0301 02:49:49.659955430 23 xds_end2end_test.cc:6764] donna backend 0 getting 25
I0301 02:49:49.660060786 23 xds_end2end_test.cc:6764] donna backend 1 getting 25
I0301 02:49:49.660195373 23 xds_end2end_test.cc:6764] donna backend 2 getting 25
I0301 02:49:49.660294648 23 xds_end2end_test.cc:6764] donna backend 3 getting 25

With only 4 channels, a perfectly even spread is not the normal, but certainly we would see 25,25,50, or 25.75, as oppose to the 100, 0, 0, 0


This change is Reviewable

@donnadionne donnadionne added the release notes: no Indicates if PR should not be in release notes label Mar 1, 2022
Copy link
Contributor Author

@donnadionne donnadionne left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @markdroth)


src/core/ext/filters/client_channel/resolver/xds/xds_resolver.cc, line 651 at r1 (raw file):

Previously, markdroth (Mark D. Roth) wrote…

This can go under the case statement where it is used. Just put braces around the body of the case statement. In other words:

case XdsRouteConfigResource::Route::RouteAction::HashPolicy::CHANNEL_ID: {
  std::string new_hash_string;
  // ...code...
  break;
}

But that having been said, I'm not sure that taking the hash of the address is the best approach to begin with. Instead, how about just computing a random value at XdsResolver instantiation time and then using that value here instead of the address of the XdsResolver? It seems like that would give better distribution.

fixed according to discussions

Copy link
Member

@markdroth markdroth left a comment

Choose a reason for hiding this comment

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

Looks great!

@donnadionne donnadionne enabled auto-merge (squash) March 2, 2022 18:57
@donnadionne donnadionne merged commit 315dfb1 into grpc:master Mar 2, 2022
@copybara-service copybara-service bot added the imported Specifies if the PR has been imported to the internal repository label Mar 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bloat/none imported Specifies if the PR has been imported to the internal repository lang/core perf-change/none release notes: no Indicates if PR should not be in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants