Skip to content

Infima Shuffle Shard Loadbalancer#15375

Closed
cgetzen wants to merge 13 commits intoenvoyproxy:mainfrom
cgetzen-forks:infima-shuffle-shard
Closed

Infima Shuffle Shard Loadbalancer#15375
cgetzen wants to merge 13 commits intoenvoyproxy:mainfrom
cgetzen-forks:infima-shuffle-shard

Conversation

@cgetzen
Copy link
Copy Markdown

@cgetzen cgetzen commented Mar 9, 2021

Commit Message: upstream: add shuffle-sharding load balancer
Additional Description:
Risk Level: Medium
Testing: TBD
Docs Changes: TBD
Release Notes: TBD
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
#14663
[Optional Deprecated:]
[Optional API Considerations:]

This implements what is closest to option 1 from the google doc referenced in the issue #14663.

If I get a tentative thumbs-up, I will begin working on:

  • tests
  • integrate with SubsetLB
  • importing infima from https://github.com/cgetzen/cpp-infima. This is a port of AWS' Route53 Infima, and by importing it from a repo that contains the NOTICE I believe we will be more in-line with licensing. I would be happy to transfer ownership of cpp-infima.
  • logging and metrics
  • docs

Description

Please learn more about shuffle sharding from the linked issue.
Using a Lattice to shuffle shard, we can ensure that a request isn't isolated to a single fault-boundary.

use_zone_as_dimensions: this allows one to use envoy's zone as a Lattice dimension.
dimensions: A list of strings that serve as Lattice dimensions. Similar to the SubsetLb, this pulls from ENVOY_LB metadata.
endpoints_per_cell: The amount of endpoints returned for a given cell. To calculate the number of endpoints available to a given unique request: endpoints_per_cell x cells. The number of cells is the multiplication of number of dimension values for each dimension (eg. "AZ" could have 3 values, "Region" could have 2 values = 6 cells)

@repokitteh-read-only
Copy link
Copy Markdown

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

see: more, trace.

@repokitteh-read-only
Copy link
Copy Markdown

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

🐱

Caused by: #15375 was opened by cgetzen.

see: more, trace.

Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@cgetzen cgetzen force-pushed the infima-shuffle-shard branch from 311958c to 8c1bdfb Compare March 9, 2021 01:01
@mattklein123
Copy link
Copy Markdown
Member

cc @wgallagher could you take a first pass?

@cgetzen cgetzen force-pushed the infima-shuffle-shard branch 3 times, most recently from b822391 to 93f3a85 Compare March 9, 2021 02:59
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@cgetzen cgetzen force-pushed the infima-shuffle-shard branch from 93f3a85 to 940bf0a Compare March 9, 2021 04:50
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@repokitteh-read-only
Copy link
Copy Markdown

CC @envoyproxy/dependency-shepherds: Your approval is needed for changes made to (bazel/.*repos.*\.bzl)|(bazel/dependency_imports\.bzl)|(api/bazel/.*\.bzl)|(.*/requirements\.txt)|(.*\.patch).

🐱

Caused by: #15375 was synchronize by cgetzen.

see: more, trace.

@repokitteh-read-only repokitteh-read-only bot added the deps Approval required for changes to Envoy's external dependencies label Mar 10, 2021
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.

Just a quick drive by until Bill gets a chance to take a look.

/wait

patches = ["@envoy//bazel/external:boringssl_fips.patch"],
)

def _com_github_cgetzen_cpp_infima():
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

will this move to a public repo before commit?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It is currently public here: https://github.com/cgetzen/cpp-infima
I'd like to see ownership transferred to github.com/envoyproxy, but there may be other options I haven't considered.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't reviewed the overall PR yet since it's still drafting, but just a heads up but we are unlikely to take a dependency on this external repo. I would just inline whatever code you actually need to make the PR function vs. using the library.

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Gotcha. This is a good opportunity for me to learn how licensing works.

Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@cgetzen
Copy link
Copy Markdown
Author

cgetzen commented Mar 31, 2021

  • Move to extension 🎉 (may need more discussion on layout)
  • Removed unnecessary comments around ENVOY_LOG (may need more discussion - using ENVOY_LOG_MISC)
  • More idiomatic empty optionals
  • Moved magic number into constant (may need more discussion)
  • Removed naked pointer

cgetzen added 2 commits March 30, 2021 22:30
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
cgetzen added 3 commits March 31, 2021 15:03
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
Comment on lines +173 to +174
# load balancers
/*/extensions/load_balancers @snowp @mattklein123
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I haven't reviewed the change, but if you did go ahead and make an LB extension point to fix that issue, please PR that first and we can review independently. Would be great to get that fixed! (This PR is too large as it is). Thanks!

/wait

Signed-off-by: Charlie Getzen <charliegetzenlc@gmail.com>
@github-actions
Copy link
Copy Markdown

github-actions bot commented May 4, 2021

This pull request has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in 7 days if no further activity occurs. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot added the stale stalebot believes this issue/PR has not been touched recently label May 4, 2021
@github-actions
Copy link
Copy Markdown

This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions!

@github-actions github-actions bot closed this May 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api deps Approval required for changes to Envoy's external dependencies stale stalebot believes this issue/PR has not been touched recently waiting

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants