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

Add gRPC MaxConnectionAge & MaxConnectionAgeGrace Options #512

Merged
merged 2 commits into from
Apr 16, 2023

Conversation

krapie
Copy link
Member

@krapie krapie commented Apr 12, 2023

What this PR does / why we need it:

Add gRPC MaxConnectionAge & MaxConnectionAgeGrace options.

This is to avoid "split-brain" of long-lived gRPC stream connection when performing hash-based load balancing in Yorkie cluster.

For more information, follow: envoyproxy/envoy#26459

gRPC suggest to use MAX_CONNECTION_AGE to avoid long-lived RPC issue on load balancing(https://youtu.be/Naonb2XD_2Q?t=136), but stream is not actually closed even MAX_CONNECTION_AGE is configured.

This is because after MAX_CONNECTION_AGE, server sends GoAway to client, but GoAway is not a signal to close connection instantly. It's just a signal to tell client not to send additional request to server(grpc/grpc-java#8770)

Therefore, MAX_CONNECTION_AGE_GRACE is also introduced to forcibly close stream.

So this is how it works:

  1. After stream is established, stream is available for maxConnectionAge.
  2. After maxConnectionAge, GoAway frame is sent to client to gracefully close the stream. But stream is not closed at this point.
  3. After MaxConnectionAgeGrace, stream is forcibly closed.

Reference: https://pkg.go.dev/google.golang.org/grpc/keepalive

Hence, maxConnectionAge and maxConnectionAgeGrace options are added as gRPC server's keepalive option parameters.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

As I researched some information about long-lived connection, graceful close, etc, I now have good explanation about why close_connections_on_host_set_change(gracefully draining connections) are not working for long-lived gRPC connections.

It is because envoy sends HTTP2 GoAway frame on draining sequence, but GoAway is not for instant connection close. Therefore, no connection close are made after draining, and close_connections_on_host_set_change option will not work as well.

References: https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/operations/draining

Also, I found some interesting issue related to our issue on long-lived connection close: grpc/grpc#26703.

Maybe it will be useful to introduce force_close_connections_on_host_set_change on envoy:: envoyproxy/envoy#26459

Does this PR introduce a user-facing change?:


Additional documentation:


Checklist:

  • Added relevant tests or not required
  • Didn't break anything

@krapie krapie added the enhancement 🌟 New feature or request label Apr 12, 2023
@krapie krapie requested a review from hackerwins April 12, 2023 15:30
@krapie krapie self-assigned this Apr 12, 2023
@codecov
Copy link

codecov bot commented Apr 12, 2023

Codecov Report

Merging #512 (20b929b) into main (efe3e87) will increase coverage by 0.84%.
The diff coverage is 23.07%.

@@            Coverage Diff             @@
##             main     #512      +/-   ##
==========================================
+ Coverage   50.22%   51.06%   +0.84%     
==========================================
  Files          64       64              
  Lines        5671     5697      +26     
==========================================
+ Hits         2848     2909      +61     
+ Misses       2481     2429      -52     
- Partials      342      359      +17     
Impacted Files Coverage Δ
server/config.go 43.33% <0.00%> (-2.02%) ⬇️
server/rpc/config.go 42.85% <0.00%> (-57.15%) ⬇️
server/rpc/server.go 67.79% <60.00%> (-1.60%) ⬇️

... and 1 file with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@krapie krapie force-pushed the add-grpc-stream-max-connection-age branch from 653649d to 20b929b Compare April 13, 2023 07:35
@krapie krapie marked this pull request as ready for review April 13, 2023 07:35
Copy link
Member

@hackerwins hackerwins left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution.

@hackerwins hackerwins merged commit 4539bfe into main Apr 16, 2023
@hackerwins hackerwins deleted the add-grpc-stream-max-connection-age branch April 16, 2023 04:42
@krapie krapie changed the title Add gRPC MaxConnectionAge & MaxConnectionAgeGrace options Add gRPC MaxConnectionAge & MaxConnectionAgeGrace Options Apr 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement 🌟 New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make remaining connection age duration available to handlers
2 participants