Disable sticky sessions for backend service #8659
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe changes in the pull request primarily involve updates to the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
b67f182 to
c7d40a8
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
helm-chart/values.yaml (1)
Line range hint
306-306: Consider Redis high availability requirementsSetting Redis architecture to
standalonemode could create a single point of failure, especially critical now that sticky sessions are disabled and Redis might be handling session state. Consider:
- Whether Redis high availability (sentinel/cluster) is needed for your scale
- Impact of Redis downtime on the application
- Session data persistence requirements
For production deployments, consider using Redis in a high-availability configuration if:
- Session state is stored in Redis
- The application cannot tolerate Redis downtime
- You need to handle failover scenarios
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
helm-chart/values.yaml(1 hunks)
🔇 Additional comments (3)
helm-chart/values.yaml (3)
Line range hint 279-284: Verify kvrocks storage size requirements
The configured storage size of 100Gi for kvrocks seems significantly larger than other components (e.g., backend at 20Gi). Please confirm:
- The storage requirements justify this size
- Cost implications have been considered
- Whether a storageClassName should be explicitly specified (currently commented out)
Consider implementing storage monitoring and alerts to track usage patterns and prevent storage-related issues.
Line range hint 293-295: LGTM: PostgreSQL auth configuration is well-structured
The explicit definition of username and database fields improves configuration clarity while maintaining security through secrets.
137-137: Verify session handling after removing sticky sessions
Removing sticky sessions means requests from the same client may be routed to different backend pods. Please ensure:
- Session state is properly shared across pods (e.g., using Redis)
- No user-facing issues occur when requests hit different backends
- Load balancing behavior has been tested with multiple backend replicas
✅ Verification successful
Let me check the session handling implementation details to make a more informed decision.
Let me check one more aspect - the cache configuration, since the previous Redis search might have been too specific.
Based on the gathered information, I can now provide a final response:
Removing sticky sessions is safe with current session implementation
The codebase analysis shows that CVAT uses Django's built-in session management with the following safeguards:
- A custom
SessionRefreshMiddlewarethat efficiently manages session prolongation - Local memory cache as the default backend
- Redis is only used for media caching, not for sessions
Since the session handling is stateful within each request and doesn't rely on sticky sessions for consistency, removing the sticky session configuration from the service annotations is safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for session handling configuration and potential issues
# Look for session configuration in Python files
echo "Checking session configuration..."
rg -l 'session|Session' --type py
# Check for Redis session store configuration
echo "Checking Redis session store usage..."
rg -l 'redis.*session|session.*redis' --type py
# Look for potential session-related issues in the codebase
echo "Checking for session-dependent code..."
rg -l 'request.session' --type py
Length of output: 1192
Script:
#!/bin/bash
# Check session configuration in settings and middleware
# Check session configuration in settings
echo "=== Session Configuration in Settings ==="
rg -A 10 "SESSION" cvat/settings/base.py
# Check session handling in middleware
echo -e "\n=== Session Handling in Middleware ==="
cat cvat/apps/iam/middleware.py
# Check if Redis is configured as session backend
echo -e "\n=== Redis Configuration ==="
rg "CACHES.*redis|SESSION.*redis" cvat/settings/base.py
Length of output: 4890
Script:
#!/bin/bash
# Check cache and session backend configuration
# Look for cache configuration
echo "=== Cache Configuration ==="
rg -A 10 "CACHES\s*=" cvat/settings/base.py
# Look for session engine configuration
echo -e "\n=== Session Engine Configuration ==="
rg "SESSION_ENGINE" cvat/settings/base.py
# Check for any Redis-related configuration
echo -e "\n=== Redis Configuration ==="
rg "redis" cvat/settings/base.py
Length of output: 1363
changelog.d/20241107_162818_andrey_disable_traefik_sticky_sessions.md
Outdated
Show resolved
Hide resolved
…ions.md Co-authored-by: Roman Donchenko <roman@cvat.ai>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #8659 +/- ##
===========================================
- Coverage 74.20% 74.19% -0.01%
===========================================
Files 401 401
Lines 43504 43504
Branches 3950 3950
===========================================
- Hits 32280 32278 -2
- Misses 11224 11226 +2
|
|
Motivation and context
How has this been tested?
Checklist
developbranch(cvat-canvas,
cvat-core,
cvat-data and
cvat-ui)
License
Feel free to contact the maintainers if that's a concern.
Summary by CodeRabbit
New Features
Bug Fixes