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

Infima Shuffle Shard Loadbalancer #15375

Closed
wants to merge 13 commits into from

Conversation

cgetzen
Copy link

@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

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

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 <[email protected]>
@mattklein123
Copy link
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 <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
@repokitteh-read-only
Copy link

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

api/envoy/config/cluster/v3/cluster.proto Outdated Show resolved Hide resolved
@@ -218,6 +219,16 @@ def _boringssl_fips():
patches = ["@envoy//bazel/external:boringssl_fips.patch"],
)

def _com_github_cgetzen_cpp_infima():

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
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
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
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 <[email protected]>
@cgetzen
Copy link
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

Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Signed-off-by: Charlie Getzen <[email protected]>
Comment on lines +173 to +174
# load balancers
/*/extensions/load_balancers @snowp @mattklein123
Copy link
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 <[email protected]>
@github-actions
Copy link

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

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