Skip to content

Conversation

zac-nixon
Copy link
Collaborator

@zac-nixon zac-nixon commented Oct 16, 2025

Issue

#4373

Description

Fixes situation described in #4373. When the listener rule synthesizer moves rules into the last priorities (50,000 - 49900) and the listener is at all the rule limit, then the synthesizer gets stuck. This is because we always attempt to create rules prior to deleting, in order to prevent requests from not getting 404 errors due to missing listener rules. At first, I thought that the modify rule path should handle this, but closer inspection told me that we only call modify rule if the priority of the new rule matched the priority of a rule that we were deleting. This means that the only time you can successfully replace a rule is when you're at the maximum rules for a listener is to modify the rule at the exact priority (and perform no other modifications!).

The solution I came up with is to create a greedy algorithm that attempts to create all desired rules, up until we detect the listener is it's max rule limit. At this point of time, we start flip-flopping between delete and create in order to minimize time between rules missing on the listener. Finally, once all missing rules are created, we will delete all remaining undesired rules.

Some other solutions that I played around with..

1/ Always call modify rule. This meant determining which unmatched SDK and result LRs to map together to modify. The downside to this approach is that we can't guarantee that the customer provided priority ordering would match on the listener. This is because the Modify api doesn't allow priority modification. I was afraid of misrouting scenarios because of corrupted priority ordering.
2/ Something along the lines of again trying to map unmatched sdk and result LRs and perform the create / delete in that order. It was pretty hard to come up with a correct algorithm to do this matching..

Checklist

  • Added tests that cover your change (if possible)
  • Added/modified documentation as required (such as the README.md, or the docs directory)
  • Manually tested
  • Made sure the title of the PR is a good description that can go into the release notes

BONUS POINTS checklist: complete for good vibes and maybe prizes?! 🤯

  • Backfilled missing tests for code in same general area 🎉
  • Refactored something and made the world a better place 🌟

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 16, 2025
@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Oct 16, 2025

s.logger.Info("unmatchedResLRs size", "size", len(unmatchedResLRs))
for _, resLR := range unmatchedResLRs {
s.logger.Info("Unmatched", "res lr", *resLR)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lets add these to debug logs.

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 17, 2025
@k8s-ci-robot
Copy link
Contributor

k8s-ci-robot commented Oct 17, 2025

@zac-nixon: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-aws-load-balancer-controller-e2e-test e787054 link true /test pull-aws-load-balancer-controller-e2e-test

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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. I understand the commands that are listed here.

Copy link
Collaborator

@shraddhabang shraddhabang left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2025
@k8s-ci-robot k8s-ci-robot removed lgtm "Looks good to me", indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Oct 17, 2025
@shraddhabang
Copy link
Collaborator

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 17, 2025
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shraddhabang, zac-nixon

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:
  • OWNERS [shraddhabang,zac-nixon]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@shuqz
Copy link
Collaborator

shuqz commented Oct 17, 2025

/lgtm

@zac-nixon zac-nixon merged commit be01a12 into kubernetes-sigs:main Oct 17, 2025
7 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants