Add Redis DB selection to the Add Instance modal - #166
Conversation
PR #166 Review — Redis DB Selection for Add InstanceReviewed Strengths
IssuesCritical (Must Fix)None found. Important (Should Fix)
Minor (Nice to Have)
AssessmentReady to merge? With fixes (Important #1 recommended before merge; #2 can be a fast follow if time-constrained). Reasoning: Backend design and test coverage are solid — the shared |
Substitutes for browser verification: checks the label/value, the disabled-until-host-selected state, the option count/order, the InfoTooltip "used by" trigger on occupied DBs, and -- the key design intent -- that an occupied DB stays selectable and fires onChange.
Documents the optional redis_db field (api_reference.md), points the architecture/technical notes at resolve_redis_db() as the single derivation source instead of the inline formula, and adds a short user-facing note on the Redis DB dropdown to the instance-creation page. Also corrects edit-configs.md, which still described qlx_redisDatabase as always port-derived. docs/qlds_args_reference.md was also corrected locally (it wrongly attributed qlx_redisDatabase to instance.id) but that file is gitignored (docs/* with an explicit allow-list) and isn't part of this commit.
f1110d8 to
11907c5
Compare
The auto-derive effect only watched `port`, so switching hosts left a stale Redis DB value on screen when the new host kept the same port number but should re-derive, or cleared the port entirely.
… singleton create_app() was called without RCON_ENABLED: False, so it hit the real SocketIO+Redis init path and mutated the module-level socketio object with a message queue -- breaking every later test that calls socketio.test_client() in the full-suite run.
The Redis DB picker no longer re-derives from the chosen port; it defaults to the lowest free DB for the host on initial load and then stays fixed until the operator changes it explicitly, independent of port/host changes. Also surfaces the Redis DB on the instance details drawer and adds a thin scrollbar style to the drawer bodies.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #166 — Redis DB Selection at Instance Creation (v1.24.0)
This PR lets operators explicitly choose a Redis logical DB (1–8) when deploying a new instance, instead of always deriving it from the port number. Pre-existing instances with no stored value continue to derive it from the port, preserving backward compatibility.
Strengths
-
Single source of truth for DB resolution.
ui/constants.py:resolve_redis_db()and its JS mirroreffectiveRedisDb()inredisDbOptions.jseliminate the prior duplicatedport - REDIS_DB_PORT_OFFSETarithmetic that existed in bothansible_instance_mgmt.pyandserver_status_poll.py. This was the actual bug-surface — now there is one place to change. -
Bulletproof input validation.
_validate_redis_db()atui/routes/instance_routes.py:1460–1472correctly handles Python'sbool-is-a-subclass-of-intgotcha by checkingisinstance(raw, bool)first. Zero (QLSM-reserved), >8, floats, and strings are all caught with descriptive 400 errors. -
Backward compatibility by design. The nullable column with
NULL = derive from portmeans every pre-existing instance gets the same DB it always had, with no migration data-fill required. The dedicated testtest_qlds_args_are_byte_identical_when_redis_db_is_nullattests/test_redis_db_resolution.py:50guards this regression point. -
Thorough test coverage at all layers. Python API integration tests (
test_instance_redis_db_api.py), unit tests for the resolution function (test_resolve_redis_db.py), Ansible arg-builder and status-poller integration (test_redis_db_resolution.py), JS unit tests for all three helper functions (redisDbOptions.test.js), and component tests including the headless-UI JSDOM workaround (InstanceBasicInfoForm.redisDb.test.jsx). -
UX is non-intrusive. The DB field auto-selects the next free DB on host selection, stays disabled until a host is picked, and shows a warning tooltip on occupied DBs without blocking selection — matching the documented intent that sharing is a deliberate, supported choice.
-
Documentation is complete. API reference, architecture doc, technical doc, user guide, and release notes are all updated coherently.
Issues
Critical (Must Fix)
None.
Important (Should Fix)
None.
Minor (Nice to Have)
1. Constants duplicated across language boundary
frontend-react/src/components/addInstance/redisDbOptions.js:1–2
export const REDIS_DB_PORT_OFFSET = 27959;
export const MAX_REDIS_DB = 8;These mirror ui/constants.py's REDIS_DB_PORT_OFFSET and MAX_INSTANCES_PER_HOST. There is no enforcement that they stay in sync — a change on the Python side silently leaves the frontend wrong. The existing pattern in this codebase for other constants (e.g. BASE_GAME_PORT) has the same shape, so this isn't unique to this PR. Worth a comment in both files pointing to the other, so whoever changes one is reminded to check the other.
2. Awkward field label in the details modal
frontend-react/src/components/instances/InstanceDetailsModal.jsx:933
<Field label="Redis DB Instance">"Redis DB Instance" reads oddly — "Redis DB" would match the deploy-form label and the API field name. Minor cosmetic inconsistency.
3. nextFreeRedisDb silently wraps to DB 1 when all 8 are occupied
frontend-react/src/components/addInstance/redisDbOptions.js:52–54
const free = options.find((option) => !option.inUse);
return free ? free.db : 1;When all DBs are taken, the function returns 1 (which is occupied) without any signal to the caller. The warning tooltip on the button will show, so the user isn't completely in the dark, but a caller that wants to distinguish "free DB found" from "everything full" cannot do so from the return value alone. Given that MAX_INSTANCES_PER_HOST = 8 and the host can only ever have 8 instances, this scenario is only reachable when the host is full — at which point the deploy form should presumably already be blocking submission for unrelated reasons (no available ports). Low real-world impact.
4. resolve_redis_db does not re-validate stored values
ui/constants.py:1388–1390
if instance.redis_db is not None:
return int(instance.redis_db)A DB value that bypassed the API (direct DB write, data migration, future code path) could be 0 or >8 and would pass through silently. The status poller does have a db < 1 guard that raises ValueError, so extreme bad values are caught there, but 0 would surface as a runtime error rather than a clean validation failure. Not a concern for normal operation; noted for completeness.
Assessment
Ready to merge? Yes
Reasoning: The core abstraction (resolve_redis_db) is clean, the migration is safe (nullable, no backfill needed), input validation is correct, and the test suite covers the failure modes that matter — including the backward-compat regression test for the byte-identical arg string. The minor notes above are cosmetic or theoretical; none block production use.
Matches the Add Instance form's label and the API field name.
Redis DB now defaults to the lowest free DB and stays fixed regardless of port changes, rather than tracking the port.
Highlights that admins can give each instance its own DB or share one across instances that need to share plugin state.
The field was described as merely "auto-selected," undersetting that it's a full dropdown offering every DB 1-8, freely pickable at any time.
1 participant
Summary
Adds a narrow Redis DB dropdown to the Add New QL Instance modal, letting the operator choose which Redis logical database a new instance stores its minqlx state in.
Until now the DB index was never stored anywhere — it was recomputed from the game port (
port - 27959) in two independent places, which meant the two could drift apart and the operator had no way to influence it.Design
QLInstance.redis_dbis nullable andNULLmeans "derive from the port", which is exactly what every pre-existing instance already did. A regression test asserts aNULLinstance produces a byte-identicalqlds_argsstring. No data migration, no instance restarts.1..MAX_INSTANCES_PER_HOST(8).1..upperwhereupper = min(8, max(instance_count + 1, highest_occupied, selected))— a host with no instances offers just1, a host with one offers1, 2. It never dumps all 8 up front. Thehighest_occupiedandselectedterms keep an occupied DB visible above the baseline and guarantee the current value is always present in its own list.net_portinserver.cfg) already funnel throughsetPort, so one effect covers them.Used by <instance name>on hover, but the option stays fully selectable — sharing a DB between two instances is a supported choice. There is no uniqueness check and no warning copy.Side effect worth noting: the two duplicated derivation formulas are now a single
ui.constants.resolve_redis_db()helper, so they can no longer drift.Test plan
b788ca5in a separate worktree and confirmed pre-existing (socketio / rcon-fleet).HooksTab.test.jsxdelete-confirmation tests in a file this branch never touches.cd frontend-react && pnpm buildsucceeds.New coverage:
resolve_redis_dbunit tests, a backward-compatqlds_argsregression guard, status-poller resolution tests, six API validation tests, 12buildRedisDbOptionsunit tests, and five dropdown render tests — including the key one asserting that clicking an occupied DB still firesonRedisDbChange.Known limitation
docs/qlds_args_reference.mdstill claimsqlx_redisDatabasecomes frominstance.id. That has never been true — it has always been port-derived. The file is gitignored (.gitignore:134docs/*), so the correction could not be committed here.docs/user/operations/edit-configs.mdcarried the same stale claim and was fixed.https://claude.ai/code/session_01Ab7PPM5Du7xVeAsq6aickq