diff --git a/CHANGELOG.md b/CHANGELOG.md index 042d6ec7..c013b333 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,13 @@ aws-parallelcluster-node CHANGELOG This file is used to list changes made in each version of the aws-parallelcluster-node package. +3.17.0 +------ + +**BUG FIXES** +- Fix dynamic nodes getting stuck in `COMPLETING` state indefinitely, holding their jobs, when the backing instance + of a node with jobs is terminated and its private IP is reused by another compute node. + 3.16.2 ------ diff --git a/src/slurm_plugin/slurm_resources.py b/src/slurm_plugin/slurm_resources.py index 77d9b472..e4455c5f 100644 --- a/src/slurm_plugin/slurm_resources.py +++ b/src/slurm_plugin/slurm_resources.py @@ -452,19 +452,31 @@ def is_powering_down_with_nodeaddr(self): # for example because of a short SuspendTimeout return self.is_nodeaddr_set() and (self.is_power() or self.is_powering_down()) + def is_instance_expected(self): + """ + Check if a slurm node is expected to be backed by an EC2 instance. + + A node is expected to have an instance if its nodeaddr is set, or if its InstanceId is set and the node is + not powering down or powered down. The InstanceId still identifies the node's instance after the nodeaddr + has been reset to the node name, until Slurm clears it when the node reaches POWERED_DOWN. + """ + return self.is_nodeaddr_set() or ( + bool(self.instance_id) and not (self.is_powering_down() or self.is_powered_down()) + ) + def is_backing_instance_valid( self, ec2_instance_missing_max_count, nodes_without_backing_instance_count_map: dict[str, MissingInstance], log_warn_if_unhealthy=True, ): - """Check if a slurm node's addr is set, it points to a valid instance in EC2.""" + """Check if a slurm node expected to be backed by an instance points to a valid instance in EC2.""" # Perform this logic only once and return the result thereafter if self.ec2_backing_instance_valid is not None: return self.ec2_backing_instance_valid # Set ec2_backing_instance_valid to True since it will be the result most often self.ec2_backing_instance_valid = True - if self.is_nodeaddr_set() and not self.instance: + if self.is_instance_expected() and not self.instance: if log_warn_if_unhealthy: logger.warning( "Node state check: no corresponding instance in EC2 for node %s, node state: %s", diff --git a/tests/slurm_plugin/slurm_resources/test_slurm_resources.py b/tests/slurm_plugin/slurm_resources/test_slurm_resources.py index 73454fd1..232c840f 100644 --- a/tests/slurm_plugin/slurm_resources/test_slurm_resources.py +++ b/tests/slurm_plugin/slurm_resources/test_slurm_resources.py @@ -1252,6 +1252,37 @@ def test_slurm_node_is_powering_down_with_nodeaddr(node, expected_result): {"queue1-st-c5xlarge-1": MissingInstance("queue1-st-c5xlarge-1", "ip-1", 1)}, True, ), + ( + # Node of GitHub issue #7671: nodeaddr reset to the node name, InstanceId of a terminated instance + DynamicNode( + "node-dy-c5xlarge-1", + "node-dy-c5xlarge-1", + "hostname", + "IDLE+CLOUD+COMPLETING+POWER_DOWN", + "node", + instance_id="i-1", + ), + None, + 0, + {}, + {}, + False, + ), + ( + DynamicNode( + "node-dy-c5xlarge-1", + "node-dy-c5xlarge-1", + "hostname", + "IDLE+CLOUD+POWERING_DOWN", + "node", + instance_id="i-1", + ), + None, + 0, + {}, + {}, + True, + ), ], ids=[ "static_no_backing_zero_max_count", @@ -1262,6 +1293,8 @@ def test_slurm_node_is_powering_down_with_nodeaddr(node, expected_result): "static_no_backing_with_count_exceeded", "static_backed_with_count_exceeded", "static_no_backing_count_not_exceeded_with_wrong_ip", + "dynamic_nodeaddr_reset_with_instance_id_no_backing", + "dynamic_powering_down_with_instance_id_no_backing", ], ) def test_slurm_node_is_backing_instance_valid(node, instance, max_count, count_map, final_map, expected_result): @@ -1278,6 +1311,69 @@ def test_slurm_node_is_backing_instance_valid(node, instance, max_count, count_m assert_that(count_map[node.name].ip).is_equal_to(node.nodeaddr) +@pytest.mark.parametrize( + "node, expected_result", + [ + (DynamicNode("queue1-dy-c5xlarge-1", "ip-1", "hostname", "MIXED+CLOUD", "queue1"), True), + ( + DynamicNode( + "queue1-dy-c5xlarge-1", + "queue1-dy-c5xlarge-1", + "hostname", + "IDLE+CLOUD+COMPLETING+POWER_DOWN", + "queue1", + instance_id="i-1", + ), + True, + ), + ( + DynamicNode( + "queue1-dy-c5xlarge-1", + "queue1-dy-c5xlarge-1", + "hostname", + "IDLE+CLOUD+POWERING_DOWN", + "queue1", + instance_id="i-1", + ), + False, + ), + ( + DynamicNode( + "queue1-dy-c5xlarge-1", + "queue1-dy-c5xlarge-1", + "hostname", + "IDLE+CLOUD+POWERED_DOWN", + "queue1", + instance_id="i-1", + ), + False, + ), + ( + DynamicNode( + "queue1-dy-c5xlarge-1", "queue1-dy-c5xlarge-1", "hostname", "IDLE+CLOUD+POWERED_DOWN", "queue1" + ), + False, + ), + ( + DynamicNode( + "queue1-dy-c5xlarge-1", "queue1-dy-c5xlarge-1", "hostname", "MIXED+CLOUD+POWERING_UP", "queue1" + ), + False, + ), + ], + ids=[ + "nodeaddr_set", + "nodeaddr_reset_with_instance_id", + "powering_down_with_instance_id", + "powered_down_with_instance_id", + "powered_down", + "powering_up_before_resume", + ], +) +def test_slurm_node_is_instance_expected(node, expected_result): + assert_that(node.is_instance_expected()).is_equal_to(expected_result) + + @pytest.mark.parametrize( "node, expected_result", [