fix: do not stop running instance when restart is skipped - #151
Conversation
cdc9905 to
2438d9e
Compare
There was a problem hiding this comment.
PR Review
Summary
This PR fixes a regression where saving instance configuration with restart disabled (restart_service=False) would stop a running QLDS service. The fix adds a new keep_service_stopped boolean flag so the "stop" task in the playbook only fires when explicitly requested, while Re-run Host Setup still correctly preserves intentionally stopped instances.
Strengths
- Minimal, targeted fix — the change is a single
andclause in the Ansiblewhencondition plus the variable plumbed through from the one caller that should set it (instance_reconciliation.py:65). - Safe default —
keep_service_stopped | default(false)means every existing caller that doesn't pass the new variable gets the safe no-op behavior for free. This prevents the regression on bothrestart_instance_logic(line 364, norestart_serviceorkeep_service_stoppedpassed) andapply_instance_config_logic(line ~590, passesrestart_service=Falsebut notkeep_service_stopped). - Tests updated in both layers — the playbook unit test (
test_sync_configs_playbook_hooks.py) and the integration-style test (test_rerun_host_setup_instance_recovery.py) are both updated and the test name better describes the intended behavior. - No behaviour change for happy paths — when
restart_service=Truethe outernot (restart_service | bool)short-circuits, sokeep_service_stoppedis irrelevant.
Issues
Critical (Must Fix)
None.
Important (Should Fix)
tests/test_rerun_host_setup_instance_recovery.py:88 — missing regression test for the original bug
The PR adds an assertion that keep_service_stopped is forwarded correctly from the reconcile path. What's absent is a test that covers the other caller (apply_instance_config_logic, ansible_instance_mgmt.py:593) to prove it does not stop a running service when restart=False. That is the exact scenario described in the PR description ("Prevent saving instance configuration with restart disabled from stopping a running QLDS service"), and without a test for it, the original bug could silently re-appear.
Suggested addition in a test for apply_instance_config_logic: assert that when restart=False, keep_service_stopped is not present in extravars (or is False/absent), confirming the default-false path is relied upon deliberately rather than by accident.
ansible_instance_mgmt.py:625-626 — stale comment
# If restart_service is False, it doesn't touch service state.This was true before this fix but is now slightly misleading: for the reconcile path, restart_service=False now causes the playbook to actively stop a previously stopped service. The comment should be scoped to the apply_instance_config_logic context or removed.
Minor (Nice to Have)
docs/user/releases.md:7 — placeholder PR link
| `v1.14.10` | 2026-07-16 | — | ...
The — should be replaced with the actual PR link once this PR is merged, to keep the release table consistent with every other entry.
Assessment
Ready to merge? Yes, with the comment fix.
Reasoning: The core logic is sound and the default(false) approach is a solid defensive pattern that protects all call sites. The missing regression test for apply_instance_config_logic is a gap worth closing in a follow-up, but it doesn't block this fix since the playbook-level unit test already verifies the when condition is correct.
|
Addressed review findings:
Verification: 23 focused tests passed. |
2438d9e to
7b03a2f
Compare
Summary
restart: falseactively stopped a running QLDS serviceProduction evidence
x76-QL-LG-NA-EAST-02receivedrestart_service: falsesystemd state=stopped; systemd showed the service failed/stopped and RCON/stats ports refused connectionsqlds@27961restored both RCON and Redis live statusTest plan
pytest tests/test_sync_configs_playbook_hooks.py tests/test_rerun_host_setup_instance_recovery.py tests/test_task_apply_config.py -q(33 passed)test_validate_invalid_python(reproduces standalone on main-compatible code)statusreturned map/player outputserver:status:1:8repopulated with current server state