fix(kthena-router): remove gomonkey to fix test crashes on Go 1.24/ARM64#875
fix(kthena-router): remove gomonkey to fix test crashes on Go 1.24/ARM64#875xrwang8 wants to merge 5 commits intovolcano-sh:mainfrom
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes gomonkey runtime patching from pkg/kthena-router tests, which was causing test instability on Go 1.24.1 and darwin/arm64 environments. The fix introduces explicit dependency injection instead of runtime binary patching.
Changes:
- Added
PodRuntimeInspectorinterface indatastore/store.gofor pod metrics and model retrieval with a default implementation delegating to backend functions - Introduced functional dependency fields to
Routerstruct for request building, streaming detection, and proxy operations, with arouterDepsstruct for parameter passing - Refactored tests to use dependency injection with fake implementations instead of gomonkey patches
- Updated
datastore.New()to accept variadicOptionparameters for dependency injection while maintaining backward compatibility
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/kthena-router/router/router.go | Added buildDecodeRequest, isStreaming, and proxyRequest functional fields to Router, split NewRouter into NewRouter (public) and newRouterWithDeps (internal), updated methods to use injected dependencies |
| pkg/kthena-router/datastore/store.go | Added PodRuntimeInspector interface, realPodRuntimeInspector implementation, Option pattern support, and updated pod metrics/models calls to use injected inspector |
| pkg/kthena-router/router/router_test.go | Removed gomonkey imports and patches, added fakeScheduler, created test helpers (mustLoadTestRouterConfig, newTestRouterWithDeps), refactored TestProxyModelEndpoint to use dependency injection, updated setupTestRouter to accept *testing.T |
| pkg/kthena-router/datastore/store_test.go | Removed gomonkey imports and patches, added fakePodRuntimeInspector struct with call tracking, created newStore helper supporting optional inspector parameter, replaced gomonkey patches with inspector callbacks |
| pkg/kthena-router/datastore/ordering_test.go | Removed gomonkey imports and patches, created newStoreWithMockBackend helper using WithPodRuntimeInspector option, refactored all tests to use the new helper |
| pkg/kthena-router/controller/modelserver_controller_test.go | Removed gomonkey imports and patches, added local fakePodRuntimeInspector, created newStoreWithMockBackend helper, updated all test functions to use dependency injection |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the codebase to improve testability by replacing gomonkey-based mocking with dependency injection and interfaces. It introduces the PodRuntimeInspector interface in the datastore and a routerDeps structure for the router, allowing for cleaner unit tests. Feedback was provided regarding a potential issue in updatePodMetrics where a nil return from the metrics inspector could inadvertently reset pod metrics to zero, potentially impacting scheduling decisions.
xrwang8
force-pushed
the
fix/kthena-router-test-stability
branch
from
494ba42 to
afb55cc
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
xrwang8
force-pushed
the
fix/kthena-router-test-stability
branch
from
65f09ae to
99cf773
Compare
Add explicit datastore and router seams so kthena-router tests can use fakes instead of runtime monkey patching. Rewrite datastore, router, and controller tests to inject dependencies directly while keeping default production behavior unchanged. Signed-off-by: xrwang8 <[email protected]>
Run gofmt to fix field alignment in fakePodRuntimeInspector struct. Signed-off-by: xrwang8 <[email protected]>
Address code review feedback: if GetPodMetrics returns nil (unsupported inference engine or transient error), skip updating metrics instead of incorrectly resetting them to zero. Signed-off-by: xrwang8 <[email protected]>
This commit addresses test failures on macOS and Go 1.24/ARM64 by removing gomonkey and hacky dependency injection. Changes to model-serving-controller: - Add PodGroupManager interface for mock implementation - Add test hooks (function fields) to ModelServingController: - enqueueModelServingFunc - enqueueModelServingAfterFunc - getModelServingAndResourceDetailsFunc - shouldSkipHandlingFunc - handleDeletionInProgressFunc - deleteRoleFunc - Create fakePodGroupManager for tests - Update tests to use dependency injection instead of gomonkey - Remove gomonkey from go.mod Changes to kthena-router: - Revert router.go to match upstream community version - Remove dependency injection fields (buildDecodeRequest, isStreaming, proxyRequest) - Use mock HTTP server (httptest.NewServer) for testing Signed-off-by: xrwang8 <[email protected]>
Remove license files for gomonkey and procfs dependencies that were removed from the project. Signed-off-by: xrwang8 <[email protected]>
xrwang8
force-pushed
the
fix/kthena-router-test-stability
branch
from
99cf773 to
23f97ef
Compare
|
@hzxuzhonghu PTAL |
What type of PR is this?
/kind bug
What this PR does / why we need it:
Removes
gomonkeyruntime patching frompkg/kthena-routertests whichcaused instability on Go 1.24.1 + darwin/arm64.
The fix adds explicit dependency injection seams:
PodRuntimeInspectorinterface indatastorepackage forbackend.GetPodMetrics/GetPodModelsRouterfor request building, streamingdetection, and proxy
Tests now use fakes injected via
WithPodRuntimeInspector()andnewRouterWithDeps()instead of runtime binary patching.Which issue(s) this PR fixes:
Fixes #872
Special notes for your reviewer:
New()now accepts variadic...Optionbut remains backward compatible