fix: bound policy ancestors on the fly to avoid O(N^2) translation latency - #9558
Conversation
✅ Deploy Preview for cerulean-figolla-1f9435 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
885ec06 to
dae129f
Compare
|
Thanks @zhaohuabing! LGTM. |
de4e8bf to
25fd6de
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de4e8bfc4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9558 +/- ##
==========================================
+ Coverage 75.59% 75.61% +0.01%
==========================================
Files 252 252
Lines 41747 41758 +11
==========================================
+ Hits 31559 31575 +16
+ Misses 8061 8058 -3
+ Partials 2127 2125 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
061f805 to
63cf84f
Compare
|
@codex review |
e2e20e3 to
d888858
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2e20e370c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e73e9ee to
c53ece6
Compare
…tency When many resources reference the same target, a policy's status.Ancestors list could grow without bound during translation. SetConditionForPolicyAncestor scans the existing ancestors on every call, so building a status with N ancestors was O(N^2); for a BackendTLSPolicy shared by e.g. 10k EnvoyExtensionPolicies this pushed translation time to ~1h. Cap status.Ancestors at maxPolicyAncestors+1 (17) as entries are added: SetConditionForPolicyAncestor evicts the lowest-priority ancestor once the list exceeds the soft cap, keeping every insert O(1) and the total O(N). The single slot above the CRD limit lets the post-processing TruncatePolicyAncestors still detect overflow, cut to 16, and stamp the Aggregated condition. Note: because eviction happens as ancestors are added, before an ancestor's rank is necessarily final, a policy with more than 16 ancestors that later marks a truncated ancestor Overridden shows only the Overridden condition on it - the redundant Accepted condition is dropped. This affects only truncated statuses. Fixes envoyproxy#9539 Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
c53ece6 to
6b01f5f
Compare
|
@codex review |
| message: Policy has been accepted. | ||
| reason: Accepted | ||
| status: "True" | ||
| type: Accepted |
There was a problem hiding this comment.
This change is a trade-off, as described in the "behavior change" section of the PR description.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Signed-off-by: Huabing (Robin) Zhao <zhaohuabing@gmail.com>
|
LGTM, thanks! |
|
/retest |
3 participants
What this PR does
Fixes #9539.
When many resources reference the same target, a policy's
status.Ancestorslist could grow without bound during translation.SetConditionForPolicyAncestorscans the existing ancestors on every call, so building a status withNancestors was O(N²). For aBackendTLSPolicyshared by ~10kEnvoyExtensionPolicies(one TLS-enabled ext-proc backend), this pushed translation time to ~1 hour.Fix
Cap
status.AncestorsatmaxPolicyAncestors + 1(17) as entries are added.SetConditionForPolicyAncestornow callsboundPolicyAncestors, which evicts the lowest-priority ancestor (same ordering asTruncatePolicyAncestors) once the list exceeds the soft cap. Each insert becomes O(1) and the total O(N).The single slot above the CRD limit of 16 is intentional: it lets the existing post-processing
TruncatePolicyAncestorsstill observelen > 16, sort, cut to 16, and stamp theAggregatedcondition exactly as before.Behavior change
For policy types that set the
Overriddencondition in a later pass (SecurityPolicy, BackendTrafficPolicy, EnvoyExtensionPolicy), a policy with more than 16 ancestors may show onlyOverridden(and lose the redundantAcceptedcondition) on a truncated ancestor. This happens because eviction runs as ancestors are added — before an ancestor's rank is final — so an ancestor evicted while onlyAcceptedis re-added later carrying justOverridden.Overriddensignal are preserved.*-status-conditions-truncated.out.yamlgoldens are updated accordingly.