3.0 Cherry-pick:Not use cloud_reg_addrs option and manually reset nodeaddr on power_down

Signed-off-by: chenwany <[email protected]>

Author: chenwany

src/common/schedulers/slurm_commands.py

@@ -193,13 +193,8 @@ def set_nodes_drain(nodes, reason):
 
 
 def set_nodes_power_down(nodes, reason=None):
-    """
-    Place slurm node into power_down state.
-
-    Do not reset the nodeaddr/nodehostname manually.
-    Nodeaddr/nodehostname will be reset automatically after power_down with cloud_reg_addrs.
-    """
-    update_nodes(nodes=nodes, state="power_down", reason=reason, raise_on_error=True)
+    """Place slurm node into power_down state and reset nodeaddr/nodehostname."""
+    reset_nodes(nodes=nodes, state="power_down", reason=reason, raise_on_error=True)
 
 
 def reset_nodes(nodes, state=None, reason=None, raise_on_error=False):

src/slurm_plugin/clustermgtd.py

@@ -670,12 +670,12 @@ def _handle_powering_down_nodes(self, slurm_nodes):
         Handle nodes that are powering down.
 
         Terminate instances backing the powering down node if any.
-        Do not reset the nodeaddr/nodehostname manually.
-        Nodeaddr/nodehostname will be reset automatically after power_down with cloud_reg_addrs.
-        Node state is not changed.
+        Reset the nodeaddr for the powering down node. Node state is not changed.
         """
         powering_down_nodes = [node for node in slurm_nodes if node.is_powering_down_with_nodeaddr()]
         if powering_down_nodes:
+            log.info("Resetting powering down nodes: %s", print_with_count(powering_down_nodes))
+            reset_nodes(nodes=[node.name for node in powering_down_nodes])
             instances_to_terminate = [node.instance.id for node in powering_down_nodes if node.instance]
             log.info("Terminating instances that are backing powering down nodes")
             self._instance_manager.delete_instances(

src/slurm_plugin/slurm_resources.py

@@ -374,12 +374,8 @@ def is_state_healthy(self, terminate_drain_nodes, terminate_down_nodes, log_warn
 
     def is_healthy(self, terminate_drain_nodes, terminate_down_nodes, log_warn_if_unhealthy=True):
         """Check if a slurm node is considered healthy."""
-        return (
-            self.is_powering_down()
-            or self.is_backing_instance_valid(log_warn_if_unhealthy=log_warn_if_unhealthy)
-            and self.is_state_healthy(
-                terminate_drain_nodes, terminate_down_nodes, log_warn_if_unhealthy=log_warn_if_unhealthy
-            )
+        return self.is_backing_instance_valid(log_warn_if_unhealthy=log_warn_if_unhealthy) and self.is_state_healthy(
+            terminate_drain_nodes, terminate_down_nodes, log_warn_if_unhealthy=log_warn_if_unhealthy
         )
 
     def is_bootstrap_failure(self):
@@ -408,15 +404,7 @@ def is_bootstrap_failure(self):
         return False
 
     def is_powering_down_with_nodeaddr(self):
-        """
-        Check if a slurm node is a powering down node with instance backing.
-
-        Nodes in powering_down(i.e. down%) state will still have the nodeaddr of the backing instance.
-        Nodeaddr is reset to nodename automatically by slurm after the power_down process,
-        when node goes back into power_saving(i.e. idle~).
-        If instance is not terminated during powering_down for some reason,
-        it becomes orphaned once the nodeaddr is reset, and will be handled as an orphaned node.
-        """
+        """Check if a slurm node is a powering down node with instance backing."""
         return self.is_nodeaddr_set() and (self.is_power() or self.is_powering_down())
 
     def needs_reset_when_inactive(self):

tests/common/schedulers/test_slurm_commands.py

@@ -346,7 +346,7 @@ def test_set_nodes_down(nodes, reason, reset_addrs, update_call_kwargs, mocker):
     ],
 )
 def test_set_nodes_power_down(nodes, reason, reset_addrs, update_call_kwargs, mocker):
-    update_mock = mocker.patch("common.schedulers.slurm_commands.update_nodes", autospec=True)
+    update_mock = mocker.patch("common.schedulers.slurm_commands.reset_nodes", autospec=True)
     set_nodes_power_down(nodes, reason)
     update_mock.assert_called_with(**update_call_kwargs)
 

tests/slurm_plugin/slurm_resources/test_slurm_resources.py

@@ -439,12 +439,18 @@ def test_slurm_node_is_bootstrap_failure(
             None,
             False,
         ),
-        # Powering_down nodes are handled separately, always considered healthy by this workflow
+        # Powering_down nodes with backing instance is considered as healthy
         (
             DynamicNode("queue-dy-c5xlarge-1", "ip-2", "hostname", "DOWN+CLOUD+POWERING_DOWN", "queue"),
             EC2Instance("id-2", "ip-2", "hostname", datetime(2020, 1, 1, 0, 0, 0)),
             True,
         ),
+        # Powering_down nodes without backing instance is considered as unhealthy
+        (
+            DynamicNode("queue-dy-c5xlarge-1", "ip-2", "hostname", "DOWN+CLOUD+POWERING_DOWN", "queue"),
+            None,
+            False,
+        ),
         # Node in POWER_SAVE, but still has ip associated should be considered unhealthy
         (
             DynamicNode("queue-dy-c5xlarge-1", "ip-2", "hostname", "IDLE+CLOUD+POWER", "queue"),
@@ -471,7 +477,8 @@ def test_slurm_node_is_bootstrap_failure(
         "dynamic_nodeaddr_not_set",
         "dynamic_unhealthy",
         "static_unhealthy",
-        "powering_down",
+        "powering_down_healthy",
+        "powering_down_unhealthy",
         "power_unhealthy1",
         "power_unhealthy2",
         "power_healthy",

tests/slurm_plugin/test_clustermgtd.py

@@ -626,7 +626,7 @@ def test_handle_unhealthy_dynamic_nodes(
 
 
 @pytest.mark.parametrize(
-    "slurm_nodes, instances, instances_to_terminate",
+    "slurm_nodes, instances, instances_to_terminate, expected_powering_down_nodes",
     [
         (
             [
@@ -637,7 +637,7 @@ def test_handle_unhealthy_dynamic_nodes(
                 DynamicNode(
                     "queue1-dy-c5xlarge-5", "queue1-dy-c5xlarge-5", "queue1-dy-c5xlarge-5", "POWERING_DOWN", "queue1"
                 ),
-                DynamicNode("queue1-st-c5xlarge-6", "ip-6", "hostname", "POWERING_DOWN", "queue1"),
+                StaticNode("queue1-st-c5xlarge-6", "ip-6", "hostname", "POWERING_DOWN", "queue1"),
             ],
             [
                 EC2Instance("id-1", "ip-1", "hostname", "some_launch_time"),
@@ -648,12 +648,15 @@ def test_handle_unhealthy_dynamic_nodes(
                 None,
             ],
             ["id-3"],
+            ["queue1-dy-c5xlarge-2", "queue1-dy-c5xlarge-3"],
         )
     ],
     ids=["basic"],
 )
 @pytest.mark.usefixtures("initialize_instance_manager_mock", "initialize_compute_fleet_status_manager_mock")
-def test_handle_powering_down_nodes(slurm_nodes, instances, instances_to_terminate, mocker):
+def test_handle_powering_down_nodes(
+    slurm_nodes, instances, instances_to_terminate, expected_powering_down_nodes, mocker
+):
     for node, instance in zip(slurm_nodes, instances):
         node.instance = instance
     mock_sync_config = SimpleNamespace(terminate_max_batch_size=4)
@@ -662,8 +665,7 @@ def test_handle_powering_down_nodes(slurm_nodes, instances, instances_to_termina
     reset_nodes_mock = mocker.patch("slurm_plugin.clustermgtd.reset_nodes", auto_spec=True)
     cluster_manager._handle_powering_down_nodes(slurm_nodes)
     mock_instance_manager.delete_instances.assert_called_with(instances_to_terminate, terminate_batch_size=4)
-    # We don't need to reset nodes manually because cloud_reg_addrs option is specified
-    reset_nodes_mock.assert_not_called()
+    reset_nodes_mock.assert_called_with(nodes=expected_powering_down_nodes)
 
 
 @pytest.mark.parametrize(