From 41ff2d57320c2331f0dabe403f2104d698cabf28 Mon Sep 17 00:00:00 2001 From: Xuanqi He Date: Fri, 24 Jul 2026 17:05:33 -0400 Subject: [PATCH] [Node] Log that an unhealthy static node is not protected from replacement A static node being replaced is shielded from being replaced again while it bootstraps. When that protection did not apply, the logs did not say so, making it hard to tell why a node under replacement was terminated again. Report, when a DOWN or DRAINED static node is found unhealthy, that the node is not within the replacement protection window and that it will be replaced, and include the node states in the log listing the unhealthy static nodes. The node state carries the reason, so the logging does not assume a specific cause. Also fix the format string of the debug log covering the opposite case, which was missing a placeholder and raised a logging error whenever a node under replacement was found DOWN. --- src/slurm_plugin/clustermgtd.py | 6 +++- src/slurm_plugin/slurm_resources.py | 16 +++++++-- .../slurm_resources/test_slurm_resources.py | 34 +++++++++++++++++++ 3 files changed, 52 insertions(+), 4 deletions(-) diff --git a/src/slurm_plugin/clustermgtd.py b/src/slurm_plugin/clustermgtd.py index d2253452..5ee5a751 100644 --- a/src/slurm_plugin/clustermgtd.py +++ b/src/slurm_plugin/clustermgtd.py @@ -961,7 +961,11 @@ def _maintain_nodes(self, partitions_name_map, compute_resource_nodes_map): log.info("Found the following unhealthy dynamic nodes: %s", print_with_count(unhealthy_dynamic_nodes)) self._handle_unhealthy_dynamic_nodes(unhealthy_dynamic_nodes) if unhealthy_static_nodes: - log.info("Found the following unhealthy static nodes: %s", print_with_count(unhealthy_static_nodes)) + # Include the node states, which carry the reason the nodes are considered unhealthy. + log.info( + "Found the following unhealthy static nodes: %s", + print_with_count([f"{node} in state {node.state_string}" for node in unhealthy_static_nodes]), + ) self._handle_unhealthy_static_nodes(unhealthy_static_nodes) # evaluate partitions to put in protected mode and ICEs nodes to terminate diff --git a/src/slurm_plugin/slurm_resources.py b/src/slurm_plugin/slurm_resources.py index 842e6a53..19ed0c50 100644 --- a/src/slurm_plugin/slurm_resources.py +++ b/src/slurm_plugin/slurm_resources.py @@ -589,20 +589,30 @@ def is_state_healthy(self, consider_drain_as_unhealthy, consider_down_as_unhealt return True else: if log_warn_if_unhealthy: - logger.warning("Node state check: node %s in DRAINED, node state: %s", self, self.state_string) + logger.warning( + "Node state check: node %s in DRAINED and not within the replacement protection window, " + "it is considered unhealthy and will be replaced, node state: %s", + self, + self.state_string, + ) return False # Check to see if node is in DOWN, ignoring any node currently being replaced elif self.is_down() and consider_down_as_unhealthy: if self.is_being_replaced: logger.debug( - "Node state check: node %s in DOWN but is currently being replaced, ignoring. Node state: ", + "Node state check: node %s in DOWN but is currently being replaced, ignoring, node state: %s", self, self.state_string, ) return True else: if log_warn_if_unhealthy: - logger.warning("Node state check: node %s in DOWN, node state: %s", self, self.state_string) + logger.warning( + "Node state check: node %s in DOWN and not within the replacement protection window, " + "it is considered unhealthy and will be replaced, node state: %s", + self, + self.state_string, + ) return False return True diff --git a/tests/slurm_plugin/slurm_resources/test_slurm_resources.py b/tests/slurm_plugin/slurm_resources/test_slurm_resources.py index dd0b4762..13b1a41a 100644 --- a/tests/slurm_plugin/slurm_resources/test_slurm_resources.py +++ b/tests/slurm_plugin/slurm_resources/test_slurm_resources.py @@ -718,6 +718,40 @@ def test_slurm_node_is_state_healthy( ) +@pytest.mark.parametrize( + "node, is_being_replaced, expected_warning", + [ + pytest.param( + StaticNode("queue-st-c5xlarge-1", "some_ip", "hostname", "DOWN+CLOUD+MAINTENANCE+RESERVED", "queue"), + False, + "node queue-st-c5xlarge-1(some_ip) in DOWN and not within the replacement protection window", + id="down_without_protection_window", + ), + pytest.param( + StaticNode("queue-st-c5xlarge-1", "some_ip", "hostname", "IDLE+CLOUD+DRAIN", "queue"), + False, + "node queue-st-c5xlarge-1(some_ip) in DRAINED and not within the replacement protection window", + id="drained_without_protection_window", + ), + pytest.param( + StaticNode("queue-st-c5xlarge-1", "some_ip", "hostname", "DOWN+CLOUD+MAINTENANCE+RESERVED", "queue"), + True, + None, + id="down_within_protection_window_not_warned", + ), + ], +) +def test_slurm_node_state_unhealthy_reason_is_logged(node, is_being_replaced, expected_warning, caplog): + """An unhealthy state is reported together with the fact that the node is not protected from replacement.""" + caplog.set_level(logging.WARNING) + node.is_being_replaced = is_being_replaced + node.is_state_healthy(consider_drain_as_unhealthy=True, consider_down_as_unhealthy=True) + if expected_warning: + assert_that(caplog.text).contains(expected_warning, node.state_string) + else: + assert_that(caplog.text).is_empty() + + @pytest.mark.parametrize( "node, instance, max_count, count_map, is_static_nodes_in_replacement, is_replacement_timeout, " "bootstrap_failure_messages, is_failing_health_check, is_node_bootstrap_failure",