replace_node.yml: Remove unnecessary seeds validation#431
replace_node.yml: Remove unnecessary seeds validation#431igorribeiroduarte wants to merge 1 commit intoscylladb:masterfrom
Conversation
This validation could prevent a user from replacing a node with itself when this node happens to be a seed, and it's completely unnecessary, since later on we have a task for temporarily changing the seed to an already bootstrapped node before starting the replace.
igorribeiroduarte
force-pushed
the
remove_seeds_validation
branch
from
349eb8f to
2d63c12
Compare
| set_fact: | ||
| broadcast_address: "{{ alive_nodes_broadcast_address }}" | ||
|
|
||
| - name: "Check if the replaced node is in the seeds list" |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
| set_fact: | ||
| broadcast_address: "{{ alive_nodes_broadcast_address }}" | ||
|
|
||
| - name: "Check if the replaced node is in the seeds list" |
There was a problem hiding this comment.
@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.
3 participants
This validation could prevent a user from replacing a node with itself when this node happens to be a seed, and it's completely unnecessary, since later on we have a task for temporarily changing the seed to an already bootstrapped node before starting the replace.