Address feedback for recent ExtraComputedSelector changes#12505
Address feedback for recent ExtraComputedSelector changes#12505nelljerram wants to merge 3 commits intoprojectcalico:masterfrom
Conversation
Generics didn't really do much here, except add a bit of complexity, given that `any` is considered comparable anyway, for back-compatibility.
marvin-tigera
added
release-note-required
There was a problem hiding this comment.
Pull request overview
This PR updates Felix’s ActiveRulesCalculator “extra computed selector” plumbing to carry a caller identifier through computed-selector match callbacks, and updates the main in-tree consumers/tests accordingly.
Changes:
- Extend
PolicyMatchListenercomputed-selector callbacks to include acallerparameter and update implementations (policy resolver, Istio calculator, live migration). - Convert
AddExtraComputedSelector/RemoveExtraComputedSelectorusage from package-level functions toActiveRulesCalculatormethods. - Update ARC unit tests to account for the new callback signature and multi-caller scenarios.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| felix/calc/active_rules_calculator.go | Changes computed-selector listener API and reworks extra selector registration/removal logic. |
| felix/calc/active_rules_calculator_test.go | Updates tests to use the new ARC methods and record caller in events. |
| felix/calc/live_migration_calculator.go | Filters computed-selector callbacks by caller and switches to ARC method calls. |
| felix/calc/istio_calculator.go | Switches to ARC method call and updates callback signatures. |
| felix/calc/policy_resolver.go | Updates callback signatures (no-op implementations). |
| arc.labelIndex.UpdateSelector(computedSelectorKey{ | ||
| selector: cs, | ||
| caller: caller, | ||
| }, sel) | ||
| } |
There was a problem hiding this comment.
Using computedSelectorKey{selector: cs, caller: caller} as the selector ID means the same selector string will be evaluated separately for every caller (scanAllLabels/scanAllSelectors run per selector ID). If multiple components register the same selector, this will multiply selector evaluation work and increase memory usage. A shared selector ID plus a separate callers set would keep evaluation cost constant while still allowing per-caller callback attribution.
There was a problem hiding this comment.
Yes, but this is the same as the existing situation for policy selectors.
| // The underlying selector is added to the label index when the first caller registers it, and it | ||
| // is only removed after the last caller removes its registration. Callbacks for matches/stops | ||
| // matching continue to be delivered to all registered PolicyMatchListeners while the selector is | ||
| // present. | ||
| func AddExtraComputedSelector[T comparable](arc *ActiveRulesCalculator, cs string, caller T) { | ||
| callers := arc.computedSelectorCallers[cs] | ||
| if callers == nil { | ||
| callers = set.New[any]() | ||
| arc.computedSelectorCallers[cs] = callers | ||
| sel, err := selector.Parse(cs) | ||
| if err != nil { | ||
| log.WithError(err).Panicf("Failed to parse computed selector %#v", cs) | ||
| } | ||
| arc.labelIndex.UpdateSelector(computedSelector(cs), sel) | ||
| func (arc *ActiveRulesCalculator) AddExtraComputedSelector(cs string, caller any) { |
There was a problem hiding this comment.
The docstring claims the underlying selector is added once (first caller) and only removed after the last caller unregisters, but the current implementation adds/removes a distinct selector ID per (cs, caller). That changes semantics (e.g. removing one caller will trigger match-stopped for that caller even if other callers still want the selector) and defeats the stated goal of sharing the expensive selector index. Consider restoring per-selector ref-counting (track callers per cs) and only UpdateSelector/DeleteSelector for the shared selector when transitioning 0->1 and 1->0 callers, while still passing caller through the callbacks.
There was a problem hiding this comment.
Semantics and commenting here have now changed.
In other words, to the listener that called AddExtraComputedSelector. Then fix up all the UTs accordingly.
nelljerram
added
docs-not-required
Address post-merge feedback on #12433 :