-
Notifications
You must be signed in to change notification settings - Fork 1.6k
feat: Add flag to limit the max number of targets added to an ELB #4361
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
Conversation
|
Welcome @mmiller-sh! |
|
Hi @mmiller-sh. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
| return "", "", false, ctrlerrors.NewErrorWithMetrics(controllerName, "update_tracked_ip_targets_error", err, m.metricsCollector) | ||
| } | ||
|
|
||
| eligibleTargetsCount := m.getMaxNewTargets(len(unmatchedEndpoints), len(targets), tgbScopedLogger) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of always doing the endpoint slicing, how about only doing it if m.maxTargetsPerInstance != 0
docs/deploy/configurations.md
Outdated
| | kube-ca-pem-filepath | string | | The file path to the CA to validate webhook callers, when unspecified all webhook callers are permitted. | | ||
| | alb-gateway-max-concurrent-reconciles | int | 3 | Maximum number of concurrently running reconcile loops for ALB gateways, if enabled | | ||
| | nlb-gateway-max-concurrent-reconciles | int | 3 | Maximum number of concurrently running reconcile loops for NLB gateways, if enabled | | ||
| | max-targets-per-instance | int | 0 | Maximum number of targets that will be added to a given ELB instance. The default value of zero will leave the number of targets unlimited | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
name is a little confusing, per your issue link you ran into the max number of targets in a target group limitation. how about max-targets-per-target-group?
to expand, instance is pretty general, and it's not clear if you're referring to targets per load balancer, targets per target group, how many pods can get packed into a node, etc. There is also a separate limits:
https://docs.aws.amazon.com/elasticloadbalancing/latest/application/load-balancer-limits.html Targets per Application Load Balancer
https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-limits.html Targets per Network Load Balancer
In this case, it looks like you're enforcing the targets per target group quota.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @zac-nixon. I agree max-targets-per-target-group makes more sense for this. I've updated the PR to reflect this, as well as addressed your other comment regarding limiting when we perform the endpoint slice.
|
/lgtm thanks for the contribution! |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mmiller-sh, wweiwei-li The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Issue
#4360
Description
This change adds an optional flag to limit the maximum number of targets that are attempted to be added to a particular load balancer instance. The primary use-case is to prevent "Max targets" quotas from being hit and blocking the reconciliation loop.
The existing behavior is preserved by default (no limits).
Checklist
README.md, or thedocsdirectory)BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯