router: clean up rate limiters when spec.rateLimit is cleared from ModelRoute - #1506
Conversation
|
cc @YaoZengzeng @LiZhenCheng9527 @hzxuzhonghu @git-malu @FAUST-BENCHOU Hi maintainers, when you get a chance please take a look at this PR. It ensures rate limiters are properly cleaned up when spec.rateLimit is removed from a ModelRoute custom resource. Unit tests have been added and all checks are passing. |
cf9d704 to
59659c6
Compare
|
[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 |
a019c43 to
d0248dd
Compare
LiZhenCheng9527
left a comment
LiZhenCheng9527
left a comment
There was a problem hiding this comment.
I think it would be possible to write a function that takes *uint32 (the type of inputTokensPerUnit), if it to nil, delete a recode in map. and abstracts the deletion strategy into a function. This would help avoid multiple if-else branches within a single function.
…delRoute When an existing ModelRoute resource is updated to remove its spec.rateLimit configuration, the router's event callback in pkg/kthena-router/router/router.go previously returned early without calling DeleteLimiter. Consequently, the router retained active input and output token rate limiters in memory and continued enforcing stale rate limits against requests for that model indefinitely. This change updates the router's ModelRoute event handler to: - Invoke loadRateLimiter.DeleteLimiter for the model name when spec.rateLimit is nil on EventAdd or EventUpdate events. - Update AddOrUpdateLimiter in TokenRateLimiter to clear directional (input or output) limiters when either InputTokensPerUnit or OutputTokensPerUnit is nil on update. Unit tests in ratelimit_test.go and router_test.go verify that rate limiters are deleted when spec.rateLimit is cleared or updated. Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
d0248dd to
4fdf333
Compare
3 participants
What type of PR is this?
/kind bug
What this PR does / why we need it:
When an existing
ModelRoutecustom resource is updated to remove itsspec.rateLimitconfiguration (clearingspec.rateLimitso traffic for that model is no longer rate-limited), the router's event callback inpkg/kthena-router/router/router.gopreviously returned early without callingloadRateLimiter.DeleteLimiter. Consequently, the router retained active input and output token rate limiters in memory and continued enforcing stale rate limits (TPS/TPM) against all requests for that model indefinitely.This PR addresses the issue by:
ModelRouteevent handler inpkg/kthena-router/router/router.goto invokeloadRateLimiter.DeleteLimiter(data.ModelName)whenspec.rateLimitisnilduringEventAddorEventUpdateevents.AddOrUpdateLimiterinpkg/kthena-router/filters/ratelimit/ratelimit.goto delete specific directional limiters (inputLimiteroroutputLimiter) whenInputTokensPerUnitorOutputTokensPerUnitis set tonilon update.ratelimit_test.goandrouter_test.goverifying that rate limiters are cleared whenspec.rateLimitis removed.Which issue(s) this PR fixes:
Fixes #1505
Bug evidence (required for bug-related PRs):
pkg/kthena-router/router/router.go: https://github.com/volcano-sh/kthena/blob/5d19b80c102a0a28f41ddaeec7b25cae37dfc879/pkg/kthena-router/router/router.go#L139-L156pkg/kthena-router/filters/ratelimit/ratelimit.go: https://github.com/volcano-sh/kthena/blob/5d19b80c102a0a28f41ddaeec7b25cae37dfc879/pkg/kthena-router/filters/ratelimit/ratelimit.go#L60-L98Special notes for your reviewer:
TestTokenRateLimiter_DeleteLimiter,TestTokenRateLimiter_UpdateClearsNilDirectionalLimits, andTestRouter_ModelRouteRateLimitClearedOnUpdate../pkg/kthena-router/...pass cleanly.Does this PR introduce a user-facing change?: