refactor(flowcontrol): Migrate Fairness Policies to EPP Plugin System#2031
Conversation
|
Hi @LukeAVanDrie. Thanks for your PR. I'm waiting for a github.com 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. DetailsInstructions 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. |
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Sorry for the large diff! I thought a lot about how to split this, but I ultimately opted for a single atomic PR for two architectural reasons:
Review Strategy:
I have also left inline reviewer comments to answer anticipated questions. Thanks! |
| FlowKeysFunc func() []types.FlowKey | ||
| QueueFunc func(flowID string) framework.FlowQueueAccessor | ||
| IterateQueuesFunc func(callback func(queue framework.FlowQueueAccessor) (keepIterating bool)) | ||
| IterateQueuesFunc func(callback func(flow framework.FlowQueueAccessor) (keepIterating bool)) |
There was a problem hiding this comment.
This function iterates on the queues of a flow, why not say that in the name?
I think the name should be FlowQueuesIterator, again without the func suffix.
There was a problem hiding this comment.
I see the point, but IterateQueuesFunc is deliberately named to map 1:1 with the
IterateQueues method in the PriorityBandAccessor interface it mocks. Changing the mock field name without changing the interface method name would reduce discoverability and consistency.
Renaming the interface method itself is something we can consider, but I'd prefer to handle that in a separate cleanup PR to keep this refactor focused.
|
Thank you for changing direction here and going with a style that is in-line with the other EPP plugins I have only reviewed a small part of this PR. I think it should have been broken up into at least two PRs. One that did the renames and second one that changed the flowcontrol extensions into proper EPP plugins. |
|
/ok-to-test |
Thanks! This is a fair point regarding |
| PriorityName: template.PriorityName, | ||
| IntraFlowDispatchPolicy: template.IntraFlowDispatchPolicy, | ||
| InterFlowDispatchPolicy: template.InterFlowDispatchPolicy, | ||
| FairnessPolicy: template.FairnessPolicy, |
There was a problem hiding this comment.
Why is the FairnessPolicy (the old InterFlowDispatchPolicy) per priority in a shard? Shouldn't it be at the shard level?
This is unless I misunderstand that the FairnessPolicy is between priorities within a shard? While the IntraFlowDispatchPolicy is the ordering of things in a Priority.
|
While you're renaming things here, I really don't understand the use of the word band throughout the flow control layer. Bands usually are ranges of things (to quote the AI part of a Google search "or a range of values (like a salary band)"). Here a band is for a single priority. In the following code snippet it is clear that a PriorityBand is for a single priority. type PriorityBandConfig struct {
// Priority is the unique numerical priority level for this band.
// Convention: Highest numeric value corresponds to highest priority (centered on 0).
// Required.
Priority int
... |
|
General comment: Please focus only on aligning on the user facing names for now, names of internal structs/types can be changed separately as a followup. |
|
I am ok with this, but for the future, this PR is massive, it could have been split in to two: the renaming and the architectural change for plugin creation |
Define the new `framework.FairnessPolicy` interface to replace the legacy `InterFlowDispatchPolicy`. This introduces a separation of concerns (Stateless Logic + Scoped State) to enable integration with the singleton EPP Plugin Registry. Includes: - `FairnessPolicy` interface with `NewState` and `Pick` methods. - `PriorityBandAccessor` interface updates for state retrieval. - Updated mocks in `framework/mocks`.
Port existing policies to the new `FairnessPolicy` interface and register them with the EPP Plugin Registry. - `RoundRobin`: Implements stateful cursor logic using `NewState`. - `GlobalStrict` (formerly BestHead): Implements stateless greedy logic. - Adds `interflow/doc.go` to document the 3-tier hierarchy.
Update config loader to validate and hydrate Fairness Policies using EPP Plugin Registry handles. - Updates `PriorityBandConfig` to hold `FairnessPolicy` instances. - Moves policy resolution from runtime (shard creation) to config load time to fail fast on missing plugins. - Hydrates default policies via the plugin handle. - Update `PriorityBandConfig` to hold a `FairnessPolicy` instance. - Move policy resolution from runtime (shard creation) to config load time. - Ensure default policies (`GlobalStrict`) are strictly validated.
Update `RegistryShard` and `FlowController` to use the new plugin-based `FairnessPolicy` instead of the legacy factory. - Initializes policy state via `NewState` during band creation. - Updates dispatch loop to pass context and state to `Pick()`. - Preserves granular, per-band locking semantics.
Remove the obsolete `interflow/factory.go` and `RegisteredPolicies` map now that all logic uses the standard EPP `plugins` package. - Deletes legacy `InterFlowDispatchPolicy` interface from `policies.go`. - Deletes outdated `interflow/README.md` in favor of `doc.go`. - Updates comments to reflect new terminology.
2783569 to
ce79bae
Compare
Thanks for the feedback. In our domain model, a "Priority Band" represents the logical grouping of all state and resources associated with a specific priority level. While "band" often implies a range of values, here it refers to a "horizontal slice" of traffic entitlement. To clarify this distinction and scope, I've updated the This terminology is used similarly in Linux Traffic Control if you want prior art here. |
Clarify architectural concepts and naming conventions based on review feedback. - Clarify "Priority Band" definition as a "range of flows". - Explicitly define FairnessPolicy scope within a band. - Document Mock naming conventions (V suffix for values) to explain divergence from interface method names. - Annotate WithFairnessPolicy for future config loader usage (Issue kubernetes-sigs#1794).
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ahg-g, LukeAVanDrie The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
…kubernetes-sigs#2031) * refactor: define FairnessPolicy interface Define the new `framework.FairnessPolicy` interface to replace the legacy `InterFlowDispatchPolicy`. This introduces a separation of concerns (Stateless Logic + Scoped State) to enable integration with the singleton EPP Plugin Registry. Includes: - `FairnessPolicy` interface with `NewState` and `Pick` methods. - `PriorityBandAccessor` interface updates for state retrieval. - Updated mocks in `framework/mocks`. * feat(flowcontrol): port policies to plugin system Port existing policies to the new `FairnessPolicy` interface and register them with the EPP Plugin Registry. - `RoundRobin`: Implements stateful cursor logic using `NewState`. - `GlobalStrict` (formerly BestHead): Implements stateless greedy logic. - Adds `interflow/doc.go` to document the 3-tier hierarchy. * feat: integrate policies with EPP config Update config loader to validate and hydrate Fairness Policies using EPP Plugin Registry handles. - Updates `PriorityBandConfig` to hold `FairnessPolicy` instances. - Moves policy resolution from runtime (shard creation) to config load time to fail fast on missing plugins. - Hydrates default policies via the plugin handle. - Update `PriorityBandConfig` to hold a `FairnessPolicy` instance. - Move policy resolution from runtime (shard creation) to config load time. - Ensure default policies (`GlobalStrict`) are strictly validated. * refactor(flowcontrol): use FairnessPolicy plugins Update `RegistryShard` and `FlowController` to use the new plugin-based `FairnessPolicy` instead of the legacy factory. - Initializes policy state via `NewState` during band creation. - Updates dispatch loop to pass context and state to `Pick()`. - Preserves granular, per-band locking semantics. * cleanup(flowcontrol): remove legacy policy factory Remove the obsolete `interflow/factory.go` and `RegisteredPolicies` map now that all logic uses the standard EPP `plugins` package. - Deletes legacy `InterFlowDispatchPolicy` interface from `policies.go`. - Deletes outdated `interflow/README.md` in favor of `doc.go`. - Updates comments to reflect new terminology. * refactor(flowcontrol): address reviewer feedback Clarify architectural concepts and naming conventions based on review feedback. - Clarify "Priority Band" definition as a "range of flows". - Explicitly define FairnessPolicy scope within a band. - Document Mock naming conventions (V suffix for values) to explain divergence from interface method names. - Annotate WithFairnessPolicy for future config loader usage (Issue kubernetes-sigs#1794).
…kubernetes-sigs/gateway-api-inference-extension#2031) * refactor: define FairnessPolicy interface Define the new `framework.FairnessPolicy` interface to replace the legacy `InterFlowDispatchPolicy`. This introduces a separation of concerns (Stateless Logic + Scoped State) to enable integration with the singleton EPP Plugin Registry. Includes: - `FairnessPolicy` interface with `NewState` and `Pick` methods. - `PriorityBandAccessor` interface updates for state retrieval. - Updated mocks in `framework/mocks`. * feat(flowcontrol): port policies to plugin system Port existing policies to the new `FairnessPolicy` interface and register them with the EPP Plugin Registry. - `RoundRobin`: Implements stateful cursor logic using `NewState`. - `GlobalStrict` (formerly BestHead): Implements stateless greedy logic. - Adds `interflow/doc.go` to document the 3-tier hierarchy. * feat: integrate policies with EPP config Update config loader to validate and hydrate Fairness Policies using EPP Plugin Registry handles. - Updates `PriorityBandConfig` to hold `FairnessPolicy` instances. - Moves policy resolution from runtime (shard creation) to config load time to fail fast on missing plugins. - Hydrates default policies via the plugin handle. - Update `PriorityBandConfig` to hold a `FairnessPolicy` instance. - Move policy resolution from runtime (shard creation) to config load time. - Ensure default policies (`GlobalStrict`) are strictly validated. * refactor(flowcontrol): use FairnessPolicy plugins Update `RegistryShard` and `FlowController` to use the new plugin-based `FairnessPolicy` instead of the legacy factory. - Initializes policy state via `NewState` during band creation. - Updates dispatch loop to pass context and state to `Pick()`. - Preserves granular, per-band locking semantics. * cleanup(flowcontrol): remove legacy policy factory Remove the obsolete `interflow/factory.go` and `RegisteredPolicies` map now that all logic uses the standard EPP `plugins` package. - Deletes legacy `InterFlowDispatchPolicy` interface from `policies.go`. - Deletes outdated `interflow/README.md` in favor of `doc.go`. - Updates comments to reflect new terminology. * refactor(flowcontrol): address reviewer feedback Clarify architectural concepts and naming conventions based on review feedback. - Clarify "Priority Band" definition as a "range of flows". - Explicitly define FairnessPolicy scope within a band. - Document Mock naming conventions (V suffix for values) to explain divergence from interface method names. - Annotate WithFairnessPolicy for future config loader usage (Issue kubernetes-sigs/gateway-api-inference-extension#1794).
What type of PR is this?
/kind feature
/kind cleanup
What this PR does / why we need it:
This PR refactors the experimental Flow Control layer to use the standard EPP Plugin Registry for Fairness Policies (formerly, Inter-Flow Dispatch Policies), replacing the previous hardcoded factory implementation.
Architectural Change:
Previously, policies were instantiated per-band using a custom factory. Now, policies are registered as Singletons in the global EPP registry to align with the wider project architecture.
To support stateful logic (like Round Robin cursors) for each Priority Band while using singleton plugins, we utilize the flyweight pattern:
NewState(ctx)and stored on thePriorityBand.Pick(ctx, band)method receives the specific state for that band at runtime.Renaming & Standardization:
InterFlowDispatchPolicy→FairnessPolicy: Aligns terminology with the "Fairness" tier in the dispatch hierarchy.BestHead→GlobalStrict: Renamed to better describe the behavior (strict global ordering ignoring flow boundaries) rather than the implementation.pkg/epp/flowcontrol/framework/plugins/interflow/doc.goto document the 3-Tier Dispatch Hierarchy (Priority → Fairness → Ordering).Reviewer Guide:
This PR includes a significant refactor (~1.5k lines changed), but the logic changes are localized. I recommend reviewing by commit:
FairnessPolicyandAccessorinterfaces.RoundRobinandGlobalStrictto the new interface.config/loaderto validate/hydrate policies at startup.RegistryandControllerto use the new system.Relationship to Previous Proposals:
This implementation supersedes the "Transient Plugin Lifecycle" proposal (#1977 ).
Based on feedback from @shmuelk and @kfswain I have abandoned the "Transient/Factory" framework changes in favor of the simpler flyweight pattern.
Which issue(s) this PR fixes:
Part of #1715
Does this PR introduce a user-facing change?: