Skip to content

replace_node.yml: Remove unnecessary seeds validation #431

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
igorribeiroduarte wants to merge 1 commit into
base: master
Choose a base branch
from
Open

replace_node.yml: Remove unnecessary seeds validation #431

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions example-playbooks/replace_node/replace_node.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,12 +15,6 @@
set_fact:
broadcast_address: "{{ alive_nodes_broadcast_address }}"

- name: "Check if the replaced node is in the seeds list"
Copy link
Collaborator

@vladzcloudius vladzcloudius Feb 7, 2025

Choose a reason for hiding this comment

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

These lines were added by this commit:

commit d9ebf24558667875d9fd5567578a57fe1ec22af6
Author: Igor Duarte <igor.duarte@scylladb.com>
Date:   Thu Jan 4 18:28:25 2024 -0300

    replace_node.yml: Validate seeds and update nodes
    
    In e42ec43000e73054649b11bc5883e2821a74da63 we wrongly removed the task
    validating the seeds and also stopped calling the node role for the alive nodes.
    However, we need to do both in order to make sure that the replaced node is
    not part of the seeds anymore and also to make sure that after the replace
    all the nodes have the correct seeds.

The description above suggests that this is not the first attempt to remove these lines.
Please, double (triple) check if it's ok to do so this time.

Copy link
Collaborator

@vreniers vreniers Feb 10, 2025

Choose a reason for hiding this comment

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

I have a comment based on that commit. Regarding running the node role for the alive nodes to update seed_provider, I'm not sure if that is sufficient. It would assume a rolling restart as this isn't a live updatable parameter.

Copy link
Collaborator

@vladzcloudius vladzcloudius Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

seeds value is only important during the node startup. Seeder is not used by running nodes.
Hence you don't need to RR when you change seeders config, @vreniers. It will be picked up by nodes if/when they will be restarted in the future.

Copy link
Collaborator

@vladzcloudius vladzcloudius Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@igorribeiroduarte this change as-is is a no-go because we are restoring the original seeders configuration at the end of the playbook - hence the validation. So if you want to remove the seeders validation you need to take care of the case in question. One option is to leave the temporary seeder. This should be ok since current scylla versions require a single seeder anyway.

fail:
msg: "The replaced node can't be a seed!"
when: replaced_node_broadcast_address in scylla_seeds or replaced_node in scylla_seeds
run_once: true

- name: Stop scylla-server on the node being replaced
block:
- name: Disable Scylla service
Expand Down