[chores] Separated session redis bucket from cache bucket#626
[chores] Separated session redis bucket from cache bucket#626nemesifier wants to merge 1 commit into
Conversation
📝 WalkthroughWalkthroughThis PR reorganizes Redis database allocation to separate cache and session storage. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@defaults/main.yml`:
- Around line 74-75: Update the role variable docs to reflect the new Redis DB
split: change references that currently say cache is on "/1" to indicate cache
uses DB "/0" and add documentation for the new variable
openwisp2_redis_sessions_url explaining it points to the sessions Redis DB (DB
"/1"); update examples and any sample values to show openwisp2_redis_cache_url =
"redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/0" and
openwisp2_redis_sessions_url = "redis://{{ openwisp2_redis_host }}:{{
openwisp2_redis_port }}/1" and include a short note about
backwards-compatibility/migration if applicable.
- Line 75: The sessions Redis URL should preserve backward compatibility by
preferring any explicit override before deriving from host/port: change the
default for openwisp2_redis_sessions_url so it uses an existing explicit
openwisp2_redis_sessions_url if set, then falls back to
openwisp2_redis_cache_url if that is set, and only then composes "redis://{{
openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1"; update the defaults entry
for openwisp2_redis_sessions_url accordingly so templates that consume it (e.g.,
settings.py usage) continue to honor previous custom cache-only overrides.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: ced82c70-c1f2-4e9d-b497-af4c62899265
📒 Files selected for processing (2)
defaults/main.ymltemplates/openwisp2/settings.py
📜 Review details
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{yml,yaml,j2}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside Jinja, YAML, and shell blocks.
Files:
defaults/main.yml
**/*.{yml,yaml,j2,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it.
Files:
defaults/main.yml
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
templates/openwisp2/settings.py
🔇 Additional comments (1)
templates/openwisp2/settings.py (1)
200-200: LGTM!Also applies to: 361-368, 372-372
| openwisp2_redis_port: 6379 | ||
| openwisp2_redis_cache_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" | ||
| openwisp2_redis_cache_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/0" | ||
| openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" |
There was a problem hiding this comment.
Preserve backward compatibility for custom Redis URL overrides.
Line 75 derives openwisp2_redis_sessions_url from openwisp2_redis_host/openwisp2_redis_port only. Deployments that previously customized only openwisp2_redis_cache_url (e.g., auth or non-default endpoint) will now have sessions routed to a different/default Redis target, causing session failures after upgrade (consumed at templates/openwisp2/settings.py Line 364).
Suggested fix
-openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1"
+openwisp2_redis_sessions_url: "{{ openwisp2_redis_cache_url | regex_replace('/[0-9]+$', '/1') }}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" | |
| openwisp2_redis_sessions_url: "{{ openwisp2_redis_cache_url | regex_replace('/[0-9]+$', '/1') }}" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@defaults/main.yml` at line 75, The sessions Redis URL should preserve
backward compatibility by preferring any explicit override before deriving from
host/port: change the default for openwisp2_redis_sessions_url so it uses an
existing explicit openwisp2_redis_sessions_url if set, then falls back to
openwisp2_redis_cache_url if that is set, and only then composes "redis://{{
openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1"; update the defaults entry
for openwisp2_redis_sessions_url accordingly so templates that consume it (e.g.,
settings.py usage) continue to honor previous custom cache-only overrides.
45e7808 to
9088648
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
defaults/main.yml (1)
75-75:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftPreserve backward compatibility for custom Redis URL overrides.
Deployments that previously customized
openwisp2_redis_cache_url(e.g., with authentication credentials, non-default host/port, or alternate Redis instance) will experience session failures after upgrade. The newopenwisp2_redis_sessions_urldefaults to deriving fromopenwisp2_redis_host/openwisp2_redis_portonly, ignoring any custom cache URL configuration. Sessions will route to the wrong Redis instance.🔧 Suggested fix to derive sessions URL from cache URL
-openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" +openwisp2_redis_sessions_url: "{{ openwisp2_redis_cache_url | regex_replace('/[0-9]+$', '/1') }}"This preserves the scheme, credentials, host, and port from
openwisp2_redis_cache_urlwhile switching only the database index to/1.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@defaults/main.yml` at line 75, The new openwisp2_redis_sessions_url currently builds from openwisp2_redis_host/openwisp2_redis_port and ignores any custom openwisp2_redis_cache_url, breaking deployments that override the cache URL; update defaults/main.yml so openwisp2_redis_sessions_url is derived from openwisp2_redis_cache_url when present (preserving its scheme, credentials, host and port) but replacing the DB index with "/1", and only fall back to "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" when openwisp2_redis_cache_url is unset.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@defaults/main.yml`:
- Around line 74-75: Add a brief comment above the openwisp2_redis_cache_url and
openwisp2_redis_sessions_url entries explaining the Redis DB separation
strategy: that DB 0 is used for cache and DB 1 for sessions to prevent user
sessions from being lost when the cache is flushed, and include any operational
notes (e.g., clearing cache should target DB 0 only) so operators understand why
two DBs are configured and how to manage them.
---
Duplicate comments:
In `@defaults/main.yml`:
- Line 75: The new openwisp2_redis_sessions_url currently builds from
openwisp2_redis_host/openwisp2_redis_port and ignores any custom
openwisp2_redis_cache_url, breaking deployments that override the cache URL;
update defaults/main.yml so openwisp2_redis_sessions_url is derived from
openwisp2_redis_cache_url when present (preserving its scheme, credentials, host
and port) but replacing the DB index with "/1", and only fall back to
"redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" when
openwisp2_redis_cache_url is unset.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 07dc9255-2b65-43cd-bb7f-4ed62d84a878
📒 Files selected for processing (2)
defaults/main.ymltemplates/openwisp2/settings.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build ubuntu2404
- GitHub Check: Build debian12
- GitHub Check: Build ubuntu2204
- GitHub Check: Build debian13
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{yml,yaml,j2}
📄 CodeRabbit inference engine (AGENTS.md)
Avoid unnecessary blank lines inside Jinja, YAML, and shell blocks.
Files:
defaults/main.yml
**/*.{yml,yaml,j2,sh}
📄 CodeRabbit inference engine (AGENTS.md)
Write comments only when they explain why code is shaped a certain way. Put comments before the relevant block instead of scattering them inside it.
Files:
defaults/main.yml
**/*.py
📄 CodeRabbit inference engine (Custom checks)
General: For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework
Files:
templates/openwisp2/settings.py
🔇 Additional comments (1)
templates/openwisp2/settings.py (1)
200-200: LGTM!Also applies to: 361-368, 372-372
| openwisp2_redis_cache_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/0" | ||
| openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1" |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Consider adding a comment explaining the Redis database separation strategy.
Per coding guidelines, comments should explain why code is shaped a certain way. The separation of cache (DB 0) and sessions (DB 1) serves an important purpose mentioned in the PR objectives: preventing session termination when cache is cleared. Documenting this rationale would help operators understand the architecture.
📝 Suggested comment
+# Redis database separation:
+# - DB 0: application cache (can be cleared without affecting sessions/channels/broker)
+# - DB 1: user sessions (isolated to prevent cache clears from terminating sessions)
+# - DB 2: Celery message broker (see openwisp2_celery_broker_url below)
openwisp2_redis_cache_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/0"
openwisp2_redis_sessions_url: "redis://{{ openwisp2_redis_host }}:{{ openwisp2_redis_port }}/1"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@defaults/main.yml` around lines 74 - 75, Add a brief comment above the
openwisp2_redis_cache_url and openwisp2_redis_sessions_url entries explaining
the Redis DB separation strategy: that DB 0 is used for cache and DB 1 for
sessions to prevent user sessions from being lost when the cache is flushed, and
include any operational notes (e.g., clearing cache should target DB 0 only) so
operators understand why two DBs are configured and how to manage them.
Source: Coding guidelines
Checklist
Reference to Existing Issue
Not needed, small improvement being done in all repositories.
Description of Changes
Small improvement to the test env which separates cache from session storage to avoid terminating sessions when clearing the cache.