Skip to content

Add max_sessions_per_event_loop to quic protocol options. #41371

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Add max_sessions_per_event_loop to quic protocol options. #41371

wants to merge 13 commits into from

Conversation

fishpan1209
Copy link
Contributor

Commit Message: Add a new quic configuration field, 'max_sessions_per_event_loop', to QuicProtocolOptions in Envoy listener.
Additional Description: This change allows tuning the maximum number of new QUIC sessions created within a single event loop. The default value is 16, preserving the previous hardcoded limit.
Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes: Add max_sessions_per_event_loop to quic protocol options.
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

@repokitteh-read-only RepoKitteh - Read Only
Copy link

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #41371 was opened by fishpan1209.

see: more, trace.

@repokitteh-read-only RepoKitteh - Read Only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @wbpcode
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #41371 was opened by fishpan1209.

see: more, trace.

@fishpan1209
Copy link
Contributor Author

/assign @RyanTheOptimist
Hi Ryan, please review this change. I make a few changes compared to the previous pr:

  1. move the new field from listener.proto to QuicProtocolOptions, as this field is specific to QUIC. The "quic" prefix has been removed from the field name since it's now within the QuicProtocolOptions.
  2. I added unit tests to validate that the value is extracted from the config correctly.
  3. The existing unit test ActiveQuicListenerTest.ProcessBufferedChlos already verifies that the maximun sessions created per loop is respected. Combined with the new tests, I believe we have sufficient coverage to ensure the max_sessions_per_event_loop limit set in the config is enforced. Therefore, I haven't added an integration test, as per-loop session limits are challenging to validate in an integration environment.
    pls lmk if you think otherwise, thank you.

@fishpan1209 fishpan1209 marked this pull request as ready for review October 6, 2025 16:05
RyanTheOptimist
RyanTheOptimist reviewed Oct 8, 2025
View reviewed changes
Copy link
Contributor

@RyanTheOptimist RyanTheOptimist 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

test/common/quic/active_quic_listener_test.cc Outdated
initialize();
const uint32_t max_sessions_per_event_loop_for_test =
ActiveQuicListener::kNumSessionsToCreatePerLoop;
EXPECT_EQ(max_sessions_per_event_loop_for_test,
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Just inline ActiveQuicListener::kNumSessionsToCreatePerLoop directly into the EXPECT_EQ.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

source/common/quic/active_quic_listener.h
// During hot restart, an optional handler for packets that weren't for existing connections.
OptRef<Network::NonDispatchedUdpPacketHandler> non_dispatched_udp_packet_handler_;
Network::IoHandle::UdpSaveCmsgConfig udp_save_cmsg_config_;
uint32_t max_sessions_per_event_loop_;
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: brief comment (particularly if there are any special semantics attached to particular values)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done, thank you.

wbpcode
wbpcode reviewed Oct 9, 2025
View reviewed changes
Copy link
Member

@wbpcode wbpcode left a comment

Choose a reason for hiding this comment

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

/lgtm api

@repokitteh-read-only repokitteh-read-only bot removed the api label Oct 9, 2025
@botengyao
Copy link
Member

Waiting for addressing Ryan's comments.

/wait

@fishpan1209
address comments
5b70265 
Signed-off-by: Ting Pan <[email protected]>
@fishpan1209
merge from upstream
503d6e4 
Signed-off-by: Ting Pan <[email protected]>
@fishpan1209
resolve merge conflicts
5d7d858 
Signed-off-by: Ting Pan <[email protected]>
@fishpan1209
resolve merge conflicts
2643c1e 
Signed-off-by: Ting Pan <[email protected]>
@fishpan1209
Copy link
Contributor Author

/retest

1 similar comment
@fishpan1209
Copy link
Contributor Author

/retest

@fishpan1209
Copy link
Contributor Author

@RyanTheOptimist all tests pass now, pls merge, thank you

@RyanTheOptimist RyanTheOptimist enabled auto-merge (squash) October 15, 2025 20:08
@RyanTheOptimist
Copy link
Contributor

Summarizing offline discussion, let's try to figure out of the test failure happens on main or if it's an issue with this PR first.

@fishpan1209
Copy link
Contributor Author

/retest

@fishpan1209
Copy link
Contributor Author

/retest Envoy/Checks

@fishpan1209
Copy link
Contributor Author

failed //test/extensions/filters/http/oauth2:oauth_integration_test
/retest

@fishpan1209
Copy link
Contributor Author

@RyanTheOptimist
I ran a dummy pr: #41552
It fails load_stats_integration_test consistently locally, but passed it with the CI.
I also ran this pr again, it didn't fail that test again.
In short words, I think it's my local environment failing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wbpcode wbpcode wbpcode left review comments

@RyanTheOptimist RyanTheOptimist RyanTheOptimist left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@wbpcode wbpcode

@RyanTheOptimist RyanTheOptimist

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@fishpan1209 @botengyao @RyanTheOptimist @wbpcode