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.

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

udp_proxy: added per packet load balancing possibility #18605

Merged
merged 10 commits into from
Nov 19, 2021
Merged

udp_proxy: added per packet load balancing possibility #18605

merged 10 commits into from
Nov 19, 2021

Conversation

michalmaka
Copy link
Contributor

Signed-off-by: Michał Mąka [email protected]

Commit Message: udp_proxy: added per packet load balancing possibility which can be enabled via specifying 'use_per_packet_load_balancing' field
Additional Description:
This feature adds possibility to have per packet load balancing in UDP proxy. Currently, there is kind of sticky session, which means that once data was forwarded to specific upstream host, the host is also used for subsequent messages if timeout does not occur.
By default, current behavior is used, but if use_per_packet_load_balancing configuration flag is set to true, on each data receival, new upstream host is selected and receives that data.
Risk Level: Medium
Testing: unit test, manual testing
Docs Changes: Introduced description of the feature and a way it can be enabled
Release Notes: Added to current.rst
Platform Specific Features: N/A

@repokitteh-read-only
Copy link

Hi @michalmaka, 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: #18605 was opened by michalmaka.

see: more, trace.

@repokitteh-read-only
Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @markdroth
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18605 was opened by michalmaka.

see: more, trace.

mattklein123
mattklein123 requested changes Oct 13, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks in general this makes sense to me. Some high level comments to get started. Thank you!

/wait

source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc Show resolved Hide resolved
source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc Outdated Show resolved Hide resolved
mattklein123
mattklein123 requested changes Oct 21, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Looks like it's heading on the right track, thanks. Please merge main and address the template issue and then I can look again.

/wait

source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc Outdated Show resolved Hide resolved
@michalmaka michalmaka force-pushed the per_packet_udp_load_balancing branch from c58a593 to 0f2efb2 Compare November 12, 2021 09:42
@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 @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #18605 was synchronize by michalmaka.

see: more, trace.

Michal Maka added 2 commits November 12, 2021 10:49
Removed template classes for per packet loadbalancing. Instead of thi…
2344634 
…s base class was factored out

Signed-off-by: Michal Maka <[email protected]>
Fixed clang-tidy issues
c625d5d 
Signed-off-by: Michal Maka <[email protected]>
mattklein123
mattklein123 reviewed Nov 15, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks looking good. A couple of other high level questions.

/wait

docs/root/configuration/listeners/udp_filters/udp_proxy.rst Outdated Show resolved Hide resolved
source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h Outdated Show resolved Hide resolved
Fixed documentation error
b3959e2 
Signed-off-by: Michal Maka <[email protected]>
mattklein123
mattklein123 requested changes Nov 17, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks generally LGTM. Just a few small comments and please revert any change that isn't needed anymore due to the simpler implementation.

/wait

source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h Outdated Show resolved Hide resolved
source/extensions/filters/udp/udp_proxy/udp_proxy_filter.cc Outdated Show resolved Hide resolved
source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h Show resolved Hide resolved
mattklein123
mattklein123 reviewed Nov 18, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks LGTM with one remaining small nit/question.

/wait-any

source/extensions/filters/udp/udp_proxy/udp_proxy_filter.h Outdated Show resolved Hide resolved
@mattklein123
Copy link
Member

Can you fix DCO?

/wait

@michalmaka michalmaka force-pushed the per_packet_udp_load_balancing branch from 170fde8 to 2047fcf Compare November 18, 2021 22:09
Michal Maka added 3 commits November 18, 2021 23:20
@michalmaka michalmaka force-pushed the per_packet_udp_load_balancing branch from 2047fcf to a0fd8b1 Compare November 18, 2021 22:28
@michalmaka
Copy link
Contributor Author

Can you fix DCO?

/wait

Done

@mattklein123
Copy link
Member

/retest

@repokitteh-read-only
Copy link

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #18605 (comment) was created by @mattklein123.

see: more, trace.

mattklein123
mattklein123 approved these changes Nov 19, 2021
View reviewed changes
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

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

Thanks, nice work!

@mattklein123 mattklein123 merged commit 23e5fc2 into envoyproxy:main Nov 19, 2021
@michalmaka
Copy link
Contributor Author

thanks!

@michalmaka michalmaka deleted the per_packet_udp_load_balancing branch November 22, 2021 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattklein123 mattklein123 mattklein123 approved these changes

Assignees

@mattklein123 mattklein123

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@michalmaka @mattklein123 @markdroth