Skip to content

xds: add config for random and maglev load balancer extensions#23453

Merged
wbpcode merged 2 commits intoenvoyproxy:mainfrom
wbpcode:dev-proto-for-lb
Oct 18, 2022
Merged

xds: add config for random and maglev load balancer extensions#23453
wbpcode merged 2 commits intoenvoyproxy:mainfrom
wbpcode:dev-proto-for-lb

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Oct 12, 2022 •
edited
Loading

Commit Message: xds: add config for random and maglev load balancer extensions
Additional Description:

Part of work to close #20634. And this PR mark all extensions as not implemented. We should implement these extensions one by one.

Risk Level: low. no code change.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

xds: add config for random and maglev load balancer extensions
7c0fed9 
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 12, 2022

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

🐱

Caused by: #23453 was opened by wbpcode.

see: more, trace.

@kyessenov
Copy link
Copy Markdown
Contributor

kyessenov commented Oct 12, 2022

Should we also add one for ORIGINAL_DST?

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 12, 2022

Should we also add one for ORIGINAL_DST?

The ORIGINAL_DST is only used by the original dst cluster. It's more like a cluster_provider lb config.

IMO, it would be better to make the original dst cluster as a custom cluster type in the future.

@markdroth
Copy link
Copy Markdown
Contributor

markdroth commented Oct 12, 2022

/lgtm api

Thanks for working on this!

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 12, 2022

/assign-from @envoyproxy/senior-maintainers

@repokitteh-read-only
Copy link
Copy Markdown

repokitteh-read-only bot commented Oct 12, 2022

@envoyproxy/senior-maintainers assignee is @snowp

🐱

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

see: more, trace.

update extension type to envoy.load_balancing_policies because envoy.…
c531b51 
…clusters is extension type for custom cluster

Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 13, 2022 •
edited
Loading

Hi, @markdroth , could you take a look again to the APIs? I did a little update.

soulxu
soulxu reviewed Oct 13, 2022
View reviewed changes
api/envoy/extensions/load_balancing_policies/wrr_locality/v3/wrr_locality.proto
// <arch_overview_load_balancing_types>` for more information.
// [#extension: envoy.clusters.lb_policy]
// [#extension: envoy.load_balancing_policies]
message WrrLocality {
Copy link
Copy Markdown
Member

@soulxu soulxu Oct 13, 2022

Choose a reason for hiding this comment

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

Should we hide this also?

wbpcode
wbpcode commented Oct 13, 2022
View reviewed changes
api/envoy/extensions/load_balancing_policies/least_request/v3/least_request.proto
option (udpa.annotations.file_status).package_version_status = ACTIVE;

// [#protodoc-title: Least Request Load Balancing Policy]
// [#not-implemented-hide:]
Copy link
Copy Markdown
Member Author

@wbpcode wbpcode Oct 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

file level hiding is enabled. cc @soulxu

@markdroth
Copy link
Copy Markdown
Contributor

markdroth commented Oct 13, 2022

/lgtm api

@wbpcode wbpcode mentioned this pull request Oct 13, 2022
Merged
snowp
snowp approved these changes Oct 14, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

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

Thanks!

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 14, 2022

wait 1.24.0 release, then we can merge this pr.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Oct 18, 2022

I was hoping to merge this PR after 1.24.0 was released, but it looks like 1.24.0 will take a little longer. So maybe we can merge this API only PR now?

cc @phlax

@phlax
Copy link
Copy Markdown
Member

phlax commented Oct 18, 2022

So maybe we can merge this API only PR now?

lets land it - it has approvals and shouldnt break anything i think

@wbpcode wbpcode merged commit e435270 into envoyproxy:main Oct 18, 2022
@wbpcode wbpcode deleted the dev-proto-for-lb branch October 18, 2022 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@soulxu soulxu soulxu left review comments

@snowp snowp snowp approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@snowp snowp

@markdroth markdroth

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

xds: Envoy should support its built-in LB policies as extensions

6 participants

@wbpcode @kyessenov @markdroth @phlax @soulxu @snowp