chore: Improve Flow Control docs and logging #1726
Conversation
This commit completes the refactoring of the request lifecycle by making context management explicit and adapting the FlowController to the new internal components. This is the final commit in a series: 1. Harden FlowItem for concurrent finalization. 2. Adapt ShardProcessor to new FlowItem lifecycle. 3. This commit: Introduce explicit context handling. Key changes: - The `FlowControlRequest` interface no longer includes `Context()`. - `FlowController.EnqueueAndWait` now accepts a `context.Context` as its first argument, making the request lifecycle explicit and caller-controlled. - The controller now actively monitors this context and will initiate an "asynchronous finalization" if the context expires after an item has been handed off to a processor. - Adds extensive integration and concurrency tests to validate the new end-to-end lifecycle management under contention.
- Overhaul package-level documentation in doc.go files for controller and internal packages to clarify architecture, concurrency, and ownership. - Refine GoDoc and inline comments across all files for clarity, consistency, and to better explain intent. - Remove low signal-to-noise inline comments. - Adjust log levels for better signal-to-noise ratio (e.g., TRACE for high-frequency events). - Remove unnecessary logger.WithValues calls in hot paths within the ShardProcessor to reduce allocation overhead. - Remove overly defensive nil check in EnqueueAndWait. - Remove config deep copy in NewFlowController (we already pass by value) - Replace a panic with logger.Error in distributeRequest for a ManagedQueue invariant violation.
k8s-ci-robot
added
the
kind/cleanup
✅ 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
|
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
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
k8s-ci-robot
added
the
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
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
3 participants
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
This PR introduces several cleanups and a critical performance improvement to the Flow Control layer:
ShardProcessor.dispatchCycleby avoidinglogger.WithValues. Structured log fields are now passed directly to the logging methods, minimizing temporary object creation.FlowController.selectDistributionCandidateswhen aManagedQueuecannot be retrieved from a shard. The controller now logs an error and safely skips the problematic shard.ShardProcessor.dispatchItemwhen an item is not found during removal. This is now treated as a non-fatal condition as it indicates a benign race with the expiry cleanup loop.FlowController.EnqueueAndWait, trusting the caller to provide a valid instance. The associated test case has also been removed.Does this PR introduce a user-facing change?: