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

xds: add config for pick_first LB policy extension #26952

Merged
merged 3 commits into from
May 2, 2023

Conversation

easwars
Copy link
Contributor

@easwars easwars commented Apr 25, 2023

This PR adds an LB policy extension configuration, to be used in gRPC via the new load_balancing_policy field:

  • pick_first: This allows configuring the built-in PICK_FIRST LB policy via the new extension-based API.

Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

CC @markdroth

gRPC via the new load_balancing_policy field:

* pick_first: This allows configuring the built-in PICK_FIRST LB policy
  via the new extension-based API.

Risk Level: Low
Testing: N/A
Docs Changes: Included in PR
Release Notes: N/A
Platform Specific Features: N/A

Signed-off-by: Easwar Swaminathan <[email protected]>
@repokitteh-read-only
Copy link

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

see: more, trace.

@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: #26952 was opened by easwars.

see: more, trace.

Comment on lines 17 to 18
// extension point. See the :ref:`load balancing architecture overview
// <arch_overview_load_balancing_types>` for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to get implemented in Envoy? Do you have these docs ready? Are you going to submit the code in the final PR? I don't have any context on this, thank you.

/wait-any

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, I don't expect this to be implemented in Envoy. We do have a pick_first LB policy in gRPC and we want the ability to configure this policy as the leaf policy via the custom LB policy mechanism in xDS. I do have a document out for the changes we are planning to make to the pick_first LB policy which covers this.

@markdroth: Do you want to add anything?

Copy link
Member

Choose a reason for hiding this comment

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

OK then please remove all the docs and just add the not-implemented-hide annotation. Then you can remove most of the other changes. Thank you.

/wait

Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, this is a policy we have in gRPC, but this particular policy is unlikely to be implemented in Envoy.

Adding the not-implemented-hide annotation and removing the Envoy documentation sounds good.

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. Thanks.

Copy link
Member

Choose a reason for hiding this comment

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

Can you please remove the arch_voerview stuff also? It's not real. Just have the not-implemented-hide annotation and perhaps a short note that this is currently used in gRPC only. Thanks.

/wait

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. Thanks.

Signed-off-by: Easwar Swaminathan <[email protected]>
@repokitteh-read-only repokitteh-read-only bot removed the api label May 2, 2023
@mattklein123 mattklein123 merged commit 7897f75 into envoyproxy:main May 2, 2023
michaelfinch added a commit to michaelfinch/envoy that referenced this pull request May 2, 2023
* main: (175 commits)
  xds: add config for pick_first LB policy extension (envoyproxy#26952)
  ci: run Kotlin tests with signal_trace disabled (envoyproxy#27090)
  ssl: upgrade FIPS boringssl version (envoyproxy#27087)
  Add createPath to Filesystem abstraction. (envoyproxy#27052)
  mobile/ci: Increase test_timeout for ios tests (envoyproxy#27044)
  [mobile]remove Java and GMScore impl from Cronvoy (envoyproxy#27039)
  Fix compliance issues for iOS builds (envoyproxy#27027)
  docs: fix the license URL of the dependency "dd-trace-cpp" (envoyproxy#27054)
  ci/mobile: Hide CI progress in .bazelrc (envoyproxy#27045)
  thrift_proxy: add access log support for local reply (envoyproxy#27057)
  ci: Consolidate artifact targets (envoyproxy#27079)
  lb: moving maglev to extensions (envoyproxy#27037)
  Overload Manager: LoadShedPoint for HCM decode headers (envoyproxy#26769)
  Plumb ServerFactoryContext into header validator factory (envoyproxy#27008)
  access_log: use AccessLogType::NotSet instead of default value (envoyproxy#27058)
  access_log: pass access log type parameter to evaluate function (envoyproxy#27063)
  Remove unused member from GrpcStream (envoyproxy#27055)
  tools: setup build in local_fix_format (envoyproxy#27060)
  generic proxy: virtual host support for the generic proxy routing (envoyproxy#26932)
  deps: Bump pytooling publishing deps (envoyproxy#27059)
  ...
reskin89 pushed a commit to reskin89/envoy that referenced this pull request Jul 11, 2023
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.

3 participants