Skip to content

fix(replace): own service startup suppression#538

Open
ManjotS wants to merge 2 commits into
devfrom
fix/replace-node-own-service-start
Open

fix(replace): own service startup suppression#538
ManjotS wants to merge 2 commits into
devfrom
fix/replace-node-own-service-start

Conversation

@ManjotS

@ManjotS ManjotS commented Jul 25, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • add a role-level skip_start_scylla_service safety gate
  • make replace_node.yml set the gate while installing/configuring the replacement node
  • remove the requirement for reused deployment params to set start_scylla_service: false

Context

Reimplements scylladb/field-engineering#2940 after scylladb/field-engineering#3218 was reverted.

The earlier change relied on example-playbooks/replace_node/vars/main.yml, but automation passes nodes_params.yml with -e @file. Those extra vars have higher precedence and commonly set start_scylla_service: true, causing Scylla to start before replacement metadata is configured.

This change gives the replace playbook an independent role control. It suppresses automatic Scylla startup, related replication adjustment, and Manager agent startup even when reused deployment parameters enable normal service startup. The playbook still starts the replacement explicitly after setting replace_node_first_boot or replace_address_first_boot.

Test plan

  • Parse all modified YAML files with PyYAML
  • Run ansible-playbook --syntax-check with repository async plugins and required new_node variable

Made with Cursor

Reused deployment parameters load as extra vars and may set
start_scylla_service=true, overriding replace defaults. Give operation
playbook independent role control so replacement metadata is configured
before Scylla starts.

Co-authored-by: Cursor <cursoragent@cursor.com>

@ManjotS ManjotS left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed the role/playbook changes for the skip_start_scylla_service gate. This is a clean, well-scoped fix: it introduces an independent role-level variable that suppresses automatic Scylla startup, replication adjustment, and Manager-agent startup, and threads it through every when: clause that previously gated on start_scylla_service alone (the big start_scylla_service dependent tasks block, the four Adjust replication for * includes, and the Manager agent start task in manager_agents.yml).

Why this works: skip_start_scylla_service is set via role vars: on the role invocation in replace_node.yml (roles: - role: ansible-scylla-node vars: {skip_start_scylla_service: true}). In Ansible's variable precedence, extra-vars (-e @nodes_params.yml) always win over role params — which is exactly the bug being fixed (reused start_scylla_service: true from nodes_params.yml was clobbering the playbook's own false). Since skip_start_scylla_service is a brand-new variable name that existing nodes_params.yml files won't define, it can't be shadowed the same way. Good fix for the root cause rather than a workaround.

Verification performed:

  • Confirmed ansible-playbook --syntax-check passes on replace_node.yml with the repo's async action plugins loaded.
  • Traced every start_scylla_service reference in the role to confirm the new gate is applied everywhere it needs to be, and that start_one_node.yml (used to explicitly start the new node later, and to start it during the final wait/repair play) is unaffected since it's invoked directly via tasks_from, not through the gated block.
  • Confirmed the second play in replace_node.yml (hosts: scylla) applying the gate to all hosts, not just new_node, matches the pre-existing behavior of the old vars/main.yml start_scylla_service: false (which also applied playbook-wide), so this isn't a behavior change in scope.
  • Confirmed skip_start_scylla_service: false as a new role default doesn't affect any other playbook that consumes this role.

Coverage: correctness/logic, security, performance, maintainability, and docs (README) all reviewed — no concerns. No new tests were added, but this is a variable-precedence/control-flow fix in Ansible YAML where the existing syntax-check + manual training-cluster run (noted as pending in the test plan) is the appropriate validation; I don't think unit tests are expected here. No blocking or important issues found.

Comment thread ansible-scylla-node/defaults/main.yml
Comment thread ansible-scylla-node/tasks/common.yml Outdated
Add the `is defined` guard to the replication-adjust when clauses so
they match the pattern used elsewhere in the role, hardening against a
future removal of the role default.

Co-authored-by: Cursor <cursoragent@cursor.com>
@ManjotS
ManjotS requested a review from vreniers July 25, 2026 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant