[POC] [AAP-64471] Add dynamic DB/cache session manager#927
[POC] [AAP-64471] Add dynamic DB/cache session manager#927john-westcott-iv wants to merge 1 commit intoansible:develfrom
Conversation
Add FEATURE_GATEWAY_SIDECAR_CACHE_ENABLED feature flag to dynamically switch between cached+DB and DB-only session storage at runtime. When sidecar cache is disabled (default): sessions use cache + DB When sidecar cache is enabled: sessions use DB only to ensure logout/session invalidation is immediately consistent across all cluster nodes (no stale cached sessions in sidecar deployments). Co-authored-by: Claude <[email protected]>
|
DVCS PR Check Results: PR appears valid (JIRA key(s) found) |
There was a problem hiding this comment.
I prefer these to have a descriptive name, but I seem to be losing that battle.
| # Sidecar cache enabled = DB only (no centralized cache) | ||
| # Sidecar cache disabled = use centralized cache + DB | ||
| use_cache = not flag_state('FEATURE_GATEWAY_SIDECAR_CACHE_ENABLED') | ||
| parent = CachedDBSessionStore if use_cache else DBSessionStore |
There was a problem hiding this comment.
I thought that the cases were:
- flag true --> uses the local redis as a cache
- flag false --> uses the old clustered redis as the cache
What I see here is
- flag false --> uses the database as the cache
This behavior is actually what I wanted! It makes complete sense to add the sidecar redis, delete the redis cluster, and keep the DB cache (worse performance) as a fallback in case something goes wrong with the sidecar redis, which I find to be entirely reasonable.
But that only makes sense if we are getting rid of the clustered redis. And this is the best possible plan. We should do that, although the awkward part is that it isn't really a "feature flag" as opposed to a general setting or behavior-toggle. We would be abandoning the old behavior. But again, this is the best path forward. But not the plan you wrote up.
There was a problem hiding this comment.
For clarity, DBSessionStore is not a DB cache per say, its just sessions stored in the DB. CachedDBSessionStore is DB stored sessions but sessions are put into the cache to avoid the DB call. In the case of sidecar redis having sessions only in the DB helps prevent the user logout issue and sessions being left around in the cache on other machines (no need to call dispatcherd on a user logout to sync the sessions). In my research, this is what AWX is doing already.
The feature flag would eventually "go away" once side car was GA and the SessionStore class in this file would just inherit from DBSessionStore only.
There was a problem hiding this comment.
For clarity, DBSessionStore is not a DB cache per say, its just sessions stored in the DB.
Going cross-eyed here.
So if you're using DBSessionStore... is it accurate to say that you're not caching sessions?
Yes, sessions are in a table somewhere. That's what they are, and the table comes from some Django contrib app like contrib.auth. So I think it makes sense that doing a kv cache in the database makes absolutely no sense as opposed to just reading the values.
But it sounds like you're saying that the ultimate end-state with Redis sidecar would be that sessions aren't cached. That's not good, right? We need to cache sessions, right?
|



Description
FEATURE_GATEWAY_SIDECAR_CACHE_ENABLEDthat allows the session store to dynamically switch between cached+DB and DB-only storage modes at runtime.Type of Change
Self-Review Checklist
Testing Instructions
Prerequisites
Steps to Test
tox -e py312 -- test_app/tests/lib/sessions/stores/test_cached_dynamic_timeout.py -vExpected Results
FEATURE_GATEWAY_SIDECAR_CACHE_ENABLED=False(default): Sessions are stored in both cache and DBFEATURE_GATEWAY_SIDECAR_CACHE_ENABLED=True: Sessions are stored in DB onlyAdditional Context
This is a POC to validate the approach for supporting gateway deployments with distributed sidecar caches where session consistency is critical.
Key implementation detail
The core logic is just 4 lines:
Note: This PR was developed with assistance from Claude AI assistant.