refactor(flowcontrol): Adopt Composite FlowKey as Primary Identifier #1340
Conversation
This commit establishes the composite `types.FlowKey` (ID + Priority) as
the canonical identifier for a flow instance throughout the entire flow
control system. This architectural shift aligns the implementation with
the clarified domain model where a flow's priority is immutable.
This change is the result of a key architectural decision to simplify
the system's state management. The previous model, which treated
priority as a mutable attribute, required a complex state machine to
handle graceful updates. By making the `FlowKey` immutable, that
complexity is no longer necessary.
The key benefits of this new model are:
1. **Radical State Simplification:** The entire state machine for
handling priority updates has been removed. The concepts of "active"
vs. "draining" queues, generation counters, and queue reactivation
logic are now obsolete. This makes the `FlowRegistry` and
`ManagedQueue` implementations significantly simpler and more
robust.
2. **Unambiguous API Contracts:** Interfaces are now clearer. Methods
like `ManagedQueue(key types.FlowKey)` receive a single,
self-contained identifier, eliminating the need to pass `flowID` and
`priority` as separate parameters and removing any ambiguity about a
flow's identity.
3. **Architectural Alignment:** The implementation now correctly and
directly reflects the API's expectation that a flow instance is
uniquely defined by its ID *and* its priority.
**Architectural Consequence & Accepted Trade-off:**
This commit knowingly accepts a significant trade-off: inter-flow
fairness is now strictly siloed within each priority band.
Under this model, the system will never be able to reason about holistic
fairness for a single tenant across multiple priority levels (e.g.,
balancing `TenantA-High` against `TenantA-Medium`). Fairness policies
now operate on `(flowID, priority)` pairs as distinct, unrelated
entities.
This trade-off is considered acceptable because the requirement for
holistic, cross-priority fairness is currently undefined and out of
scope for the system's immediate goals.
✅ Deploy Preview for gateway-api-inference-extension ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
k8s-ci-robot
added
the
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: LukeAVanDrie The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @LukeAVanDrie. 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. |
k8s-ci-robot
added
needs-ok-to-test
This PR refactors the flow control system to establish the composite
types.FlowKey(ID + Priority) as the canonical, immutable identifier for a flow instance. This is a direct result of critical feedback and discussions on the open registry PR #1319 , which revealed a fundamental misalignment between the implementation's data model and the API's intended contract.The previous model treated a flow's priority as a mutable attribute, which required a complex state machine for handling "graceful updates" (managing
activevs.drainingstates, generation counters, etc.). This PR replaces that model with a simpler, more robust architecture where a flow instance is uniquely and immutably defined by itsFlowKey.This tracks #674
What this PR Does
types.FlowKey: A new struct combiningIDandPriorityis now the primary key for all flow instances.contracts.RegistryShardandframeworkinterfaces have been updated to useFlowKeyas the primary identifier, removing ambiguity and clarifying the API.ActiveManagedQueuehas been removed, and methods now consistently use theFlowKey.ManagedQueue.ShardProcessor: The controller's worker implementation has been updated to align with the new, simpler contracts.Key Benefits
FlowRegistryand its components are now significantly simpler, more robust, and easier to reason about.FlowKey, eliminating the need to passflowIDandpriorityas separate parameters.Architectural Consequence & Accepted Trade-off
This PR knowingly accepts a significant architectural trade-off: inter-flow fairness is now strictly siloed within each priority band.
Under this new model, the system will never be able to reason about holistic fairness for a single logical tenant across multiple priority levels (e.g., balancing
TenantA-HighagainstTenantA-Medium). Fairness policies operate on(flowID, priority)pairs as distinct, unrelated entities.This trade-off is considered acceptable as the requirement for holistic, cross-priority fairness is currently undefined and out of scope for the system's immediate goals. This change provides a simpler, more robust foundation to build upon.
Thank you to @kfswain and @ahg-g for the insightful feedback that led to this design compromise.