Use list of dict to remove duplicate nodes in _find_active_nodes()

This preserves the insertion order of nodes in the list of active nodes and
removed the need to sort nodelists manually to compare them in unit tests.

Signed-off-by: Jacopo De Amicis <jdamicis@amazon.it>

Author: jdeamicis

src/slurm_plugin/clustermgtd.py

@@ -1123,12 +1123,11 @@ def _find_bootstrap_failure_nodes(slurm_nodes):
 
     @staticmethod
     def _find_active_nodes(partitions_name_map):
-        active_nodes = set()
+        active_nodes = []
         for partition in partitions_name_map.values():
             if partition.state != "INACTIVE":
-                active_nodes |= set(partition.slurm_nodes)
-        # TODO: Returning the set breaks some unit tests (I believe the unit test should be revised though).
-        return sorted(list(active_nodes), key=str)
+                active_nodes += partition.slurm_nodes
+        return list(dict.fromkeys(active_nodes))
 
     def _is_node_in_replacement_valid(self, node, check_node_is_valid):
         """

tests/slurm_plugin/test_clustermgtd.py

@@ -1316,14 +1316,13 @@ def test_maintain_nodes(
     # Run test
     cluster_manager._maintain_nodes(partitions, {})
     # Check function calls
-    active_nodes_sorted = sorted(active_nodes, key=str)
-    mock_update_replacement.assert_called_with(active_nodes_sorted)
+    mock_update_replacement.assert_called_with(active_nodes)
     mock_handle_dynamic.assert_called_with(expected_unhealthy_dynamic_nodes)
     mock_handle_static.assert_called_with(expected_unhealthy_static_nodes)
-    mock_handle_powering_down_nodes.assert_called_with(active_nodes_sorted)
-    mock_handle_failed_health_check_nodes_in_replacement.assert_called_with(active_nodes_sorted)
+    mock_handle_powering_down_nodes.assert_called_with(active_nodes)
+    mock_handle_failed_health_check_nodes_in_replacement.assert_called_with(active_nodes)
     if _is_protected_mode_enabled:
-        mock_handle_protected_mode_process.assert_called_with(active_nodes_sorted, partitions)
+        mock_handle_protected_mode_process.assert_called_with(active_nodes, partitions)
     else:
         mock_handle_protected_mode_process.assert_not_called()
 
@@ -3797,21 +3796,6 @@ def test_find_unhealthy_slurm_nodes(
     ],
 )
 def test_find_active_nodes(partitions_name_map, expected_nodelist):
-    """
-    Unit test for the `ClusterManager._find_active_nodes()` method.
-
-    Some context about the way this test is implemented:
-    - `ClusterManager._find_active_nodes()` may be implemented to return different types of iterables.
-      This test was implemented together with a fix that changed the return type from a list to a set,
-      and it was desirable to have the test compatible with both return types.
-    - The implementation that returned a list caused a duplication of node entities in the returned iterable
-      in case the same node belonged to multiple Slurm partitions (via a customization of the Slurm configuration).
-    - Due to the way we implement the `__hash__()` dunder method for the SlurmNode class, two different
-      SlurmNode objects with the same node name are squashed into the same entity in a set. Therefore
-      we cannot use `set(expected_nodelist)` when trying to test the duplication of the node entities
-      in the iterable returned by `ClusterManager._find_active_nodes()`.
-    - Sets are unordered, so when transforming them into lists we have to sort them to make them comparable with
-      the `expected_nodelist`.
-    """
-    result_nodelist = sorted(list(ClusterManager._find_active_nodes(partitions_name_map)), key=str)
-    assert_that(result_nodelist).is_equal_to(sorted(expected_nodelist, key=str))
+    """Unit test for the `ClusterManager._find_active_nodes()` method."""
+    result_nodelist = ClusterManager._find_active_nodes(partitions_name_map)
+    assert_that(result_nodelist).is_equal_to(expected_nodelist)