Fix dynamic nodes stuck in COMPLETING after their instance is terminated and there is a new instance reusing the same IP

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
 ------
 

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",

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",
     [