Skip to content

upstream subset lb: add redundant keys support#28874

Merged
wbpcode merged 26 commits intoenvoyproxy:mainfrom
wbpcode:dev-subset-lb
Aug 29, 2023
Merged

upstream subset lb: add redundant keys support#28874
wbpcode merged 26 commits intoenvoyproxy:mainfrom
wbpcode:dev-subset-lb

Conversation

@wbpcode
Copy link
Copy Markdown
Member

@wbpcode wbpcode commented Aug 7, 2023

Commit Message: upstream subset lb: add redundant keys support
Additional Description:

Description is same with the comment:

    // If true, redundant key/value pairs are allowed in the request (route, weighted cluster or dynamic)
    // metadata. By default, only when the request metadata has **same** keys with one of the subset
    // selectors and the values of related keys are matched, we will get valid subset. For example:
    //
    // .. code-block:: json
    //
    //   { "subset_selectors": [
    //       { "keys": [ "version" ] },
    //       { "keys": [ "stage", "version" ] }
    //   ]}
    //
    // A request with metadata ``{"redundant-key": "any", "stage": "prod", "version": "v1"}`` or
    // ``{"redundant-key": "any", "version": "v1"}`` can never get valid subset even if the values of
    // ``stage`` and ``version`` are matched because of the redundant key/value pair in the request
    // metadata.
    //
    // By set this to true, only the necessary key/value pairs will be evaluated and redundant key/value
    // pairs will be ignored. Take the above example, if the request metadata is ``{"redundant-key": "any",
    // "stage": "prod", "version": "v1"}``, the subset lb will keep the ``{"stage": "prod", "version": "v1"}``
    // as the request metadata for the evaluation. If the request metadata is ``{"redundant-key": "any",
    // "version": "v1"}``, the subset lb will keep the ``{"version": "v1"}`` as the request metadata for
    // the evaluation.
    //
    // .. note::
    //   To avoid the performance impact and potential complexity, subset lb will keep at much as possible
    //   key/value pairs in the request metadata. Take the above example, if the request metadata is
    //   ``{"redundant-key": "any", "stage": "prod", "version": "v1"}``, the subset lb will keep the
    //   ``{"stage": "prod", "version": "v1"}`` as the request metadata for the evaluation and will not
    //   try the ``{"version": "v1"}`` as the request metadata.

Risk Level: low. minor change.
Testing: unit.
Docs Changes: n/a.
Release Notes: added.
Platform Specific Features: n/a.

wbpcode added 2 commits August 7, 2023 08:48
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 @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #28874 was opened by wbpcode.

see: more, trace.

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

@adisuissa adisuissa 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.
Left a high-level comment about this new feature.

Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread source/extensions/load_balancing_policies/subset/subset_lb.h Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
code and others added 3 commits August 9, 2023 09:54
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 9, 2023

/docs

@repokitteh-read-only
Copy link
Copy Markdown

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/28874/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

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

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 9, 2023

This is more readable.

image

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

wbpcode commented Aug 15, 2023

friendly ping @adisuissa

@wbpcode wbpcode assigned zuercher and unassigned snowp Aug 18, 2023
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 18, 2023

Re-assigned this to @zuercher because @zuercher is code owner of subset lb.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 18, 2023

/docs

@repokitteh-read-only
Copy link
Copy Markdown

Docs for this Pull Request will be rendered here:

https://storage.googleapis.com/envoy-pr/28874/docs/index.html

The docs are (re-)rendered each time the CI envoy-presubmit (precheck docs) job completes.

🐱

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

see: more, trace.

Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto
wbpcode added 4 commits August 19, 2023 03:06
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 21, 2023

cc @zuercher test was updated. 😸

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 21, 2023

cc @adisuissa any other concern to the API? 🤔

zuercher
zuercher previously approved these changes Aug 21, 2023
Copy link
Copy Markdown
Contributor

@adisuissa adisuissa left a comment

Choose a reason for hiding this comment

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

Left a few nits, but otherwise api lgtm.
Consider emphasizing that this is a 2step algorithm, choosing the keys subset first, and only then matching their values (so it won’t be mistaken with choosing the subset of keys+values that match w/o the redundant keys)

Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
Comment thread api/envoy/extensions/load_balancing_policies/subset/v3/subset.proto Outdated
code and others added 7 commits August 22, 2023 15:22
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
@repokitteh-read-only
Copy link
Copy Markdown

🙀 Error while processing event:

evaluation error
error: context deadline exceeded
🐱

Caused by: #28874 was synchronize by wbpcode.

see: more, trace.

code and others added 4 commits August 22, 2023 15:23
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
…proto

Co-authored-by: Adi (Suissa) Peleg <adip@google.com>
Signed-off-by: code <wangbaiping@corp.netease.com>
Signed-off-by: wbpcode <wangbaiping@corp.netease.com>
@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 22, 2023

cc @adisuissa Thanks for your detailed review. Related comments are addressed and comment of API is updated.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 22, 2023

Hi, @zuercher, could you give another LGTM. We updated API's comment after your code review. Thanks.

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

wbpcode commented Aug 29, 2023

/assign-from @envoyproxy/envoy-maintainers

@repokitteh-read-only
Copy link
Copy Markdown

@envoyproxy/envoy-maintainers assignee is @zuercher

🐱

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

see: more, trace.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 29, 2023

cc @RyanTheOptimist (on-call maintainer) Hi, could you give this PR another LGTM?

@zuercher has approved this PR before. And after his approve, we just update the API's comments. But seems @zuercher is busy recently and has no github activities.

@wbpcode
Copy link
Copy Markdown
Member Author

wbpcode commented Aug 29, 2023

ho, @zuercher thanks 🙏

@wbpcode wbpcode merged commit 74bfcb0 into envoyproxy:main Aug 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants