Repository navigation
docs(infra): record redis role tools as binary names, not units (#16060, #16071) - #16511
Conversation
#16071) node-roles.ts:95 and its backend mirror, api/deployments.py:83, list redis-server/redis-cli/redis-sentinel for the redis role's "tools" display metadata. Both are correct as-is -- Redis Stack genuinely ships a binary named redis-server -- but neither said so, leaving the last open criterion on #16060 ("corrected or recorded as display-only") and #16071 ("the seven correct sites") unaddressed for this pair. Adds a comment to each recording the distinction and pointing at services/role_units.py, which owns the actual systemd unit name (redis-stack-server) separately. Resolves #16060 in full. #16071's remaining criterion (fix-architecture-issues.sh's stop discarding its result) is not a code change here: read redis_unit_is_the_stack_unit_16071_test.py's own _LEGACY_CLEANUP docstring first, and that discard is a deliberate, already-documented exemption for this exact site -- stopping an absent unit during architecture cleanup is the correct outcome, not a failure to detect, unlike the backup.py restore path #16060 fixed. Treating that recorded decision as satisfying the literal AC wording, not reversing it. Resolves #16071 in full on that basis; flagged explicitly in the PR body for review since it's a judgement call, not a fresh code change.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 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 |
AutoBot Phase Validation ResultsSystem Maturity: 96.5% Phase Status:PASS Phase 1: Core Infrastructure: 100.0% Recommendations:
|
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
|
Code review: REQUEST CHANGES, on the closing link, not the code. Code: fine. The change is comment-only: CI: no live failures. The one red in the rollup, "Validate PR template sections", is a stale run; the same check re-ran on the same head and passed. It's blocked only on review. Closing links: plain lists #16060 and #16071;
To land this, pick one:
Not a finding: targeting |
…ture-issues.sh (#16071) Review on #16511: AC4 asks that this script's Redis-stop step no longer discard its result against an absent unit, and the earlier PR body mistakenly treated redis_unit_is_the_stack_unit_16071_test.py's _LEGACY_CLEANUP exemption as covering it -- that exemption is about not flagging this file's deliberate dual-stop of both unit names, not about excusing `|| true` on the stop's own exit code. `systemctl stop redis-server 2>/dev/null || true` (and the same for redis-stack-server) could not tell "unit not installed on this host" (expected -- a node provisioned before #16060/#16071 may carry only one of the two) from "unit exists but the stop genuinely failed" (a real problem): both produced identical, silent success. Now: `systemctl list-unit-files "${unit}.service"` gates each unit -- absent means skip, nothing to do. A present unit whose stop fails is reported via error() and folds into a non-zero exit status for the block, instead of disappearing. New repo_tests/fix_architecture_issues_redis_stop_result_16071_test.py: static/structural checks on the script text (no execution -- this needs a live systemd host, which is CI/host evidence, not something to fake locally) that the blanket-swallow pattern is gone, the existence check gates both unit names, and a failed stop is still reported.
|
Fixed AC4 as requested (option 1): the earlier PR body's reasoning was wrong — the `_LEGACY_CLEANUP` exemption in `redis_unit_is_the_stack_unit_16071_test.py` covers not-flagging the dual-stop-by-name, not excusing the `|| true` on the stop's own result. That's now genuinely fixed in `fix-architecture-issues.sh` at `9ee396708`: each unit's existence is checked via `systemctl list-unit-files` before attempting a stop (absent → skip, nothing to report), and a stop that fails on a unit confirmed present is now surfaced via `error()` instead of `|| true`. New `repo_tests/fix_architecture_issues_redis_stop_result_16071_test.py` pins this statically (no execution — a real systemd host is needed for that). Pre-push hook picked it up in the changeset and reported all relevant tests passing. `Closes #16071` should now hold. |
|
Delta Scope of the delta (a diff of the net PR diffs):
The branch is 0 behind main. At posting, CI has 23 checks pending and none red. #16071 AC4 ("the
My earlier review already traced the other #16071 ACs and all of #16060 to code. Two Lows. Neither blocks.
|
1 participant
Thinking Path
A stale-issue sweep found #16060 and #16071 both had one remaining open criterion each, and both criteria were about the SAME pair of files (
node-roles.tsand its backend mirrorapi/deployments.py) and the same underlying question — is the redis role'stoolslist wrong, or just undocumented? ReadinggetRoleTools()'s only consumer (none — zero callers in the frontend today) and comparing againstapi/deployments.py:83's decision (already accepted as correct: Redis Stack genuinely ships aredis-serverbinary), the answer for both was "undocumented, not wrong," so the fix is a comment on each, not a value change. Batched into one PR because both issues share this exact fix.Review then found this PR's original reasoning about #16071's remaining AC4 was wrong: it treated
redis_unit_is_the_stack_unit_16071_test.py's_LEGACY_CLEANUPexemption forfix-architecture-issues.shas covering "the stop no longer discards its result" — that exemption is about not flagging the file's deliberate dual-stop of both unit names as an offense, not about excusing|| trueon the stop's own exit code. AC4 was genuinely unmet. Fixed here rather than downgrading toRefs, per the review's own preferred option: the script now checks each unit's existence before attempting to stop it (systemctl list-unit-files), so an absent unit is skipped cleanly, and a present unit whose stop genuinely fails is reported viaerror()instead of disappearing into the same|| truethat swallowed both cases identically before.What Changed
autobot-slm-frontend/src/constants/node-roles.tsandautobot-slm-backend/api/deployments.py: added a comment on the redis role'stoolsentry explaining these are binary names (Redis Stack shipsredis-server/redis-cliat/opt/redis-stack/bin/), not the systemd unit (redis-stack-server, owned separately byservices/role_units.py).autobot-infrastructure/shared/scripts/vm-management/fix-architecture-issues.sh(redis naming: five more sites name a unit the fleet does not have (#16060 follow-up) #16071 AC4): the Redis-stop step's blanketsudo systemctl stop <unit> 2>/dev/null || true(once per unit name) is replaced with a loop that checkssystemctl list-unit-files "${unit}.service"first — an absent unit is skipped, nothing attempted — and reports a genuine stop failure viaerror(), folding into a non-zero status for the block instead of vanishing.repo_tests/fix_architecture_issues_redis_stop_result_16071_test.py: static/structural checks on the script text (no execution — a realsystemctl stopneeds a live systemd host) that the blanket-swallow pattern is gone, the existence check gates both unit names, and a failed stop is still reported.Verification
Static reasoning only, per the owner's no-local-execution rule — no test suite was run locally; CI and the pre-push hook are the authority.
toolsarrays are byte-identical, and thatgetRoleTools()(the only consumer of the frontend list) has zero callers anywhere in the frontend today — comment-only, no behavioural risk on that half.redis_unit_is_the_stack_unit_16071_test.py's_LEGACY_CLEANUPdocstring and confirmed it stays satisfied: the exemption only requires this file to still nameredis-stack-serversomewhere, which it does (both units are still named in the existence-check loop).flake8 --select=F401,F821,E501on the touched Python files — clean.bash -n(syntax-only, no execution) on the touched shell script — clean.9ee396708); the pre-push hook's own pytest run picked uprepo_tests/fix_architecture_issues_redis_stop_result_16071_test.pyin the changeset and reported[pre-push OK] pytest: all relevant tests pass.Risks
Low. The existence-check idiom (
systemctl list-unit-files "<unit>.service") matches one already used elsewhere in this tree forautobot-redis/redis-stack-server(documented in the sibling guard test's docstring), so this isn't a new pattern. Not verified from a diff: actual behavior on a real host still needs to be observed once this deploys — the script only runs during manual VM architecture remediation, not on every deploy, so host evidence would come from the next time an operator runs it against a node carrying only one of the two units.Model Used
Claude Sonnet 5 (claude-sonnet-5)
Issue Link
Closes #16060
Closes #16071
Changelog fragment
changelog/unreleased/16060-16071-redis-naming-completion.md(added the redis naming: five more sites name a unit the fleet does not have (#16060 follow-up) #16071 AC4 fix, type docs → fix)Checklist
CLAUDE.mdmain