✨feat: Global rate limiting with redis #660
Conversation
volcano-sh-bot
requested review from
LiZhenCheng9527,
YaoZengzeng and
hzxuzhonghu
Summary of ChangesHello @katara-Jayprakash, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request focuses on enhancing the testing infrastructure for the Kthena router by adding robust end-to-end tests for both local and global rate limiting features. The global rate limiting tests specifically integrate with Redis to simulate real-world distributed environments, validating critical aspects such as quota enforcement, window resets, persistence across multiple routes, and graceful fallback when Redis is unavailable. These additions significantly improve the reliability and confidence in the router's rate limiting capabilities. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces end-to-end tests for the new global rate limiting feature using Redis. The changes are well-structured and the tests cover various scenarios, including Redis connectivity, rate limit enforcement, fallback behavior, and multi-route configurations. My review focuses on improving test code maintainability and robustness. I've pointed out a typo, significant code duplication that could be refactored into helper functions, the use of time.Sleep which could lead to flaky tests, and a slightly confusing test case name. Overall, this is a solid addition for ensuring the quality of the new feature.
|
|
||
| // TestModelRouteWithGlobalRateLimit tests global rate limiting enforced by the Kthena Router. | ||
| // This test runs the shared test function with Gateway API enabled (with ParentRefs). | ||
| func TestModelRouteWitGlobalRateLimit(t *testing.T) { |
There was a problem hiding this comment.
There's a typo in the function name. It should be TestModelRouteWithGlobalRateLimit to be consistent with the other test function names.
| func TestModelRouteWitGlobalRateLimit(t *testing.T) { | |
| func TestModelRouteWithGlobalRateLimit(t *testing.T) { |
test/e2e/router/shared.go
Outdated
| modelRoute := utils.LoadYAMLFromFile[networkingv1alpha1.ModelRoute]("examples/kthena-router/ModelRouteWithRateLimit.yaml") | ||
| modelRoute.Namespace = testNamespace | ||
| setupModelRouteWithGatewayAPI(modelRoute, useGatewayApi, kthenaNamespace) | ||
|
|
||
| createdModelRoute, err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Create(ctx, modelRoute, metav1.CreateOptions{}) | ||
| require.NoError(t, err, "Failed to create ModelRoute") | ||
|
|
||
| t.Cleanup(func() { | ||
| cleanupCtx := context.Background() | ||
| if err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Delete(cleanupCtx, createdModelRoute.Name, metav1.DeleteOptions{}); err != nil { | ||
| t.Logf("Warning: Failed to delete ModelRoute: %v", err) | ||
| } | ||
| }) | ||
|
|
||
| require.Eventually(t, func() bool { | ||
| mr, err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Get(ctx, createdModelRoute.Name, metav1.GetOptions{}) | ||
| return err == nil && mr != nil | ||
| }, 2*time.Minute, 2*time.Second, "ModelRoute should be created") |
There was a problem hiding this comment.
There is significant code duplication for setting up and cleaning up ModelRoute resources across the sub-tests in TestModelRouteWithRateLimitShared and TestModelRouteWithGlobalRateLimitShared. To improve maintainability and reduce boilerplate, I recommend extracting this logic into a helper function.
For example, you could create a helper for TestModelRouteWithRateLimitShared like this:
func setupModelRouteForRateLimitTest(t *testing.T, testCtx *routercontext.RouterTestContext, testNamespace string, useGatewayApi bool, kthenaNamespace string) *networkingv1alpha1.ModelRoute {
t.Helper()
ctx := context.Background()
modelRoute := utils.LoadYAMLFromFile[networkingv1alpha1.ModelRoute]("examples/kthena-router/ModelRouteWithRateLimit.yaml")
modelRoute.Namespace = testNamespace
setupModelRouteWithGatewayAPI(modelRoute, useGatewayApi, kthenaNamespace)
createdModelRoute, err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Create(ctx, modelRoute, metav1.CreateOptions{})
require.NoError(t, err, "Failed to create ModelRoute")
t.Cleanup(func() {
cleanupCtx := context.Background()
if err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Delete(cleanupCtx, createdModelRoute.Name, metav1.DeleteOptions{}); err != nil {
t.Logf("Warning: Failed to delete ModelRoute: %v", err)
}
})
require.Eventually(t, func() bool {
mr, err := testCtx.KthenaClient.NetworkingV1alpha1().ModelRoutes(testNamespace).Get(ctx, createdModelRoute.Name, metav1.GetOptions{})
return err == nil && mr != nil
}, 2*time.Minute, 2*time.Second, "ModelRoute should be created")
return createdModelRoute
}A similar helper could be created for TestModelRouteWithGlobalRateLimitShared that accepts parameters for name, modelName, and redisAddress to handle the variations in those tests.
test/e2e/router/shared.go
Outdated
| require.NoError(t, err, "Failed to update ModelRoute") | ||
|
|
||
| // Wait for update to propagate | ||
| time.Sleep(2 * time.Second) |
There was a problem hiding this comment.
Using time.Sleep to wait for configuration propagation can lead to flaky tests, as the actual time required can vary. It's more robust to use require.Eventually to poll until the desired state is observed. For example, you could try sending requests in a loop until the rate-limiting behavior changes as expected or a timeout is reached.
| }) | ||
|
|
||
| // Test 4: Verify multiple ModelRoutes sharing global rate limit | ||
| t.Run("VerifyMultipleModelRoutesSharing", func(t *testing.T) { |
There was a problem hiding this comment.
The test name VerifyMultipleModelRoutesSharing could be misinterpreted. This test actually verifies that two different ModelRoutes (with different modelNames) have independent rate limits, rather than sharing a single limit bucket, despite using the same global Redis backend. A more descriptive name would improve clarity.
Consider renaming it to something like VerifyIndependentGlobalRateLimitsForMultipleModelRoutes.
| t.Run("VerifyMultipleModelRoutesSharing", func(t *testing.T) { | |
| t.Run("VerifyIndependentGlobalRateLimitsForMultipleModelRoutes", func(t *testing.T) { |
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
Signed-off-by: katara-Jayprakash <katarajayprakash@icloud.com>
katara-Jayprakash
force-pushed
the
Global-Rate-limiting-with-Redis
branch
from
d0c0709 to
2380de9
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 |
| // with ParentRefs to the default Gateway. | ||
|
|
||
| // deployRedisForTest deploys Redis server and service for global rate limiting tests. | ||
| func deployRedisForTest(t *testing.T, ctx context.Context, testCtx *routercontext.RouterTestContext, namespace string) { |
There was a problem hiding this comment.
@YaoZengzeng this test require using sendchatcompletation that i already included in modelratelimit pull-request.if we can approve/merge that pr then i think we can use this function here too? would love to know your approch/thoughts,
There was a problem hiding this comment.
I think #641 would be merged quickly, then you could rebase.
There was a problem hiding this comment.
Thanx for your review sir,
3 participants
What type of PR is this?
implementng globalModuleRouteRateLimit e2e test in kthena ecosystem.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes #556
Special notes for your reviewer:
i actually wanted to know how to fix these commits actually there are some commits actually in my localratelimiting pr too and here why?
Does this PR introduce a user-facing change?: