Skip to content

fix node removal #810

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

Closed
pranavcracker wants to merge 1 commit into from
+19 −1
Closed

fix node removal #810

Show file tree
Hide file tree
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
2 changes: 1 addition & 1 deletion kubemarine/core/cluster.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ def check_nodes_accessibility(self, skip_check_iaas: bool = True) -> None:
return

if procedure == 'remove_node':
group = self.make_group_from_roles(['control-plane', 'balancer'])
group = self.get_unchanged_nodes().having_roles(['control-plane', 'balancer'])
else:
group = self.make_group_from_roles(['control-plane', 'balancer']).include_group(self.get_new_nodes_or_self())

Expand Down
18 changes: 18 additions & 0 deletions test/unit/core/test_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -414,6 +414,24 @@ def test_any_removed_node_can_be_offline(self):
# no exception should occur
flow.run_tasks(res, tasks)

def test_remove_control_plane_if_offline(self):
Copy link
Copy Markdown
Collaborator

@theboringstuff theboringstuff May 15, 2026

Choose a reason for hiding this comment

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

I am not sure what this test does. I tried to revert above changes and test still passes, so it does not check new changes. If this test is not relevant, I suggest to remove it so that we have less tests to maintain

Copy link
Copy Markdown
Collaborator Author

@pranavcracker pranavcracker May 19, 2026

Choose a reason for hiding this comment

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

I think you are right. After deep investigation I confirmed both make_group_from_roles and get_unchanged_nodes().having_roles() always produce the same group at LIGHT stage — calculate_nodegroups excludes the removed node from nodes['all'] before this check runs.

I had tested this issue on miniha cluster earlier. I will investigate it more deeper on FullHA cluster.

inventory = demo.generate_inventory(**demo.FULLHA_KEEPALIVED)
online_hosts = [node["address"] for node in inventory["nodes"]]
control_planes = [i for i, node in enumerate(inventory["nodes"]) if 'control-plane' in node['roles']]

i = control_planes[0]
online_hosts.remove(inventory["nodes"][i]["address"])
procedure_inventory = demo.generate_procedure_inventory('remove_node')
procedure_inventory["nodes"] = [{"name": inventory["nodes"][i]["name"]}]

self._stub_detect_nodes_context(inventory, online_hosts, [])
context = demo.create_silent_context(['fake_path.yaml'], procedure='remove_node')
res = demo.FakeResources(context, inventory, procedure_inventory=procedure_inventory,
fake_shell=self.light_fake_shell)

# no exception should occur — offline control-plane being removed must not block the procedure
flow.run_tasks(res, tasks)

def test_remove_node_if_worker_offline(self):
inventory = demo.generate_inventory(**demo.FULLHA_KEEPALIVED)
online_hosts = [node["address"] for node in inventory["nodes"]]
Expand Down
Loading