Skip to content

load balancer: added least request/random/round robin load balancer extension#23472

Merged
wbpcode merged 23 commits intoenvoyproxy:mainfrom
wbpcode:dev-load-balancer-opt
Jan 8, 2023
Merged

load balancer: added least request/random/round robin load balancer extension#23472
wbpcode merged 23 commits intoenvoyproxy:mainfrom
wbpcode:dev-load-balancer-opt

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Oct 13, 2022

Commit Message: load balancer: added least request/random/round robin load balancer extension
Additional Description:

Part of work of #20634. This PR make the least request/random/round robin load balancer as an extension and could be configured by the load_balancing_policy.

One-page simple roadmap doc about the new extensions.
https://docs.google.com/document/d/1GtayHX8orSEChtdn_2stRB8BNY2LE0Y0FatDVGODNHA/edit?usp=sharing

Risk Level: Low. Not change current behavior.
Testing: wait.
Docs Changes: wait.
Release Notes: wait.
Platform Specific Features: wait.

wbpcode added 2 commits October 13, 2022 15:25
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@repokitteh-read-only
Copy link
Copy Markdown

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: #23472 was opened by wbpcode.

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 13, 2022

/wait

Waiting for #23474 #23453 to be merged first.

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
wbpcode added 2 commits October 29, 2022 09:04
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
wbpcode added 5 commits October 29, 2022 10:32
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode wbpcode assigned moderation and markdroth and unassigned moderation Oct 30, 2022
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 30, 2022

cc @markdroth This is the first implementation for the load balancer extension. And it would be very quick to convert the other policies to the extension implementation.

In this implementation, I treat the common_lb_config a shared context of target cluster. Would this be acceptable for you?

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Nov 2, 2022

Hi, @mattklein123 this is ready for a formal code review 😺

Copy link
Copy Markdown
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 for working on this. A few questions. Thank you.

/wait

Comment thread source/extensions/extensions_metadata.yaml Outdated
Comment thread source/common/upstream/load_balancer_impl.h
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 4, 2023

Question - as part of this (probably in a follow up) would you be willing to have the load balancer code be factory created, so the code for each load balancer can be removed to the respective extension directory and out of the main build, or is that out of scope for you?

It's in my TODO list.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 4, 2023

At a high level makes sense. Can we get integration tests for the new extensions?

Get it.

wbpcode added 3 commits January 4, 2023 06:58
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@alyssawilk
Copy link
Copy Markdown
Contributor

Awesome! Please cc me on those as I have an annual resolution to avoid more forceRegister problems for E-M and we'll probably need to add some forceRegister there :-P
cc @Augustyniak

wbpcode added 2 commits January 5, 2023 02:49
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Copy link
Copy Markdown
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.

LGTM, thanks!

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 6, 2023

Awesome! Please cc me on those as I have an annual resolution to avoid more forceRegister problems for E-M and we'll probably need to add some forceRegister there :-P cc @Augustyniak

cc @alyssawilk I think I have added some forceRegister in this PR. Could you do a quick check for it? Should I do any change?

@alyssawilk
Copy link
Copy Markdown
Contributor

Sorry I'm theoretically out today so I'm going to ask Rafal to take a look.
Generally when things Envoy uses are moved to extensions we need a PR like this
https://github.com/envoyproxy/envoy/pull/24750/files
to make sure they're not dead code stripped by iOS. I know the DFP uses its own custom host selection, but I don't recall offhand what Lyft uses for the stats cluster so I'll defer to @Augustyniak to make sure we're not losing anything Lyft uses

@Augustyniak
Copy link
Copy Markdown
Contributor

we use round robin so most probably we will need to register factory related to it (in a manner similar to the PR shared by Alyssa https://github.com/envoyproxy/envoy/pull/24750/files). Trying to verify it now in #24797 and will report back.

@Augustyniak
Copy link
Copy Markdown
Contributor

@wbpcode mobile ci jobs in #24797 are green so your changes seems to be working on mobile as-is just fine. I think that something imports/includes round robin in EM already.

In other words, no need to add any explicit factory registration for now.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 8, 2023

Sorry I'm theoretically out today so I'm going to ask Rafal to take a look. Generally when things Envoy uses are moved to extensions we need a PR like this https://github.com/envoyproxy/envoy/pull/24750/files to make sure they're not dead code stripped by iOS. I know the DFP uses its own custom host selection, but I don't recall offhand what Lyft uses for the stats cluster so I'll defer to @Augustyniak to make sure we're not losing anything Lyft uses

I see. This PR doesn't move the implementation of LBs to source/extensions but just make them could be created by the factory.
So, there should no any change to mobile for now. cc @Augustyniak

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Jan 8, 2023

/retest

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: a #23472 (comment) was created by @wbpcode.

see: more, trace.

@wbpcode wbpcode merged commit d036285 into envoyproxy:main Jan 8, 2023
VishalDamgude pushed a commit to freshworks-oss/envoy that referenced this pull request Feb 1, 2023
…xtension (envoyproxy#23472)

* load balancer: update related methodsfor load balancer extensions

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: VishalDamgude <vishal.damgude@freshworks.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants