From e46f4727012c2724ec8b6c448c97b32222adcab4 Mon Sep 17 00:00:00 2001
From: Giacomo Marciani <mgiacomo@amazon.com>
Date: Fri, 9 Oct 2026 13:50:05 -0400
Subject: [PATCH] [clustermgtd] Fix an issue where the Comment and Extra fields
 of Slurm nodes and the Comment field of Slurm reservations could alter the
 node and reservation information read by ParallelCluster.

---
 CHANGELOG.md                                  |   2 +
 src/common/schedulers/slurm_commands.py       |   9 +-
 .../schedulers/slurm_reservation_commands.py  |   2 +-
 .../common/schedulers/test_slurm_commands.py  | 161 +++++++++++++++++-
 .../test_slurm_reservation_commands.py        |   1 +
 5 files changed, 172 insertions(+), 3 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index e8eb2111..4b5c8031 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -9,6 +9,8 @@ This file is used to list changes made in each version of the aws-parallelcluste
 **BUG FIXES**
 - Fix an issue where EC2 throttling during compute node launch is not retried and is reported as insufficient capacity
   when using Multiple Instance Types or multiple subnets.
+- Fix an issue where the Comment and Extra fields of Slurm nodes and the Comment field of Slurm reservations
+  could alter the node and reservation information read by ParallelCluster.
 
 3.16.1
 ------
diff --git a/src/common/schedulers/slurm_commands.py b/src/common/schedulers/slurm_commands.py
index a46e4412..dbec3dd8 100644
--- a/src/common/schedulers/slurm_commands.py
+++ b/src/common/schedulers/slurm_commands.py
@@ -75,10 +75,15 @@
     r"|(LastBusyTime=\S+)"
     r"|(ReservationName=\S+)"
     r"|(InstanceId=\S+)"
-    r"|(Reason=.*)",
+    r"|(Reason=.*)"
+    r"|(Comment=.*)"
+    r"|(Extra=.*)",
     re.MULTILINE,
 )
 
+# Fields matched up to the end of the line and discarded, to prevent their free-text content from being parsed as fields
+SCONTROL_DISCARDED_FIELDS = ("Comment", "Extra")
+
 # Fields extracted from `scontrol show partitions` output. `(?<!Next)` ensures `State` is matched but
 # `NextState` is not.
 SCONTROL_PARTITION_INFO_FIELD_REGEX = re.compile(
@@ -426,6 +431,8 @@ def _extract_scontrol_records(raw_output: str, field_regex) -> List[Dict[str, st
         fields = {}
         for match in field_regex.finditer(record):
             key, _, value = match.group(0).partition("=")
+            if key in SCONTROL_DISCARDED_FIELDS:
+                continue
             fields[key] = value
         if fields:
             records.append(fields)
diff --git a/src/common/schedulers/slurm_reservation_commands.py b/src/common/schedulers/slurm_reservation_commands.py
index afd71681..645c9c56 100644
--- a/src/common/schedulers/slurm_reservation_commands.py
+++ b/src/common/schedulers/slurm_reservation_commands.py
@@ -39,7 +39,7 @@
 # Fields extracted from raw `scontrol show reservations` output. Only `ReservationName` is anchored at the
 # start of a line; `(?<!Next)` ensures `State` is matched but `NextState` is not.
 SCONTROL_RESERVATION_INFO_FIELD_REGEX = re.compile(
-    r"^(ReservationName=\S+)" r"|(?<!Next)(State=\S+)" r"|(Users=\S+)" r"|(Nodes=\S+)",
+    r"^(ReservationName=\S+)" r"|(?<!Next)(State=\S+)" r"|(Users=\S+)" r"|(Nodes=\S+)" r"|(Comment=.*)",
     re.MULTILINE,
 )
 
diff --git a/tests/common/schedulers/test_slurm_commands.py b/tests/common/schedulers/test_slurm_commands.py
index e07e1877..3343cc6b 100644
--- a/tests/common/schedulers/test_slurm_commands.py
+++ b/tests/common/schedulers/test_slurm_commands.py
@@ -1404,7 +1404,127 @@ def test_get_nodes_info_argument_validation(nodes, cmd_timeout, expected_scontro
                     },
                 ]
             ),
-        )
+        ),
+        pytest.param(
+            (
+                "NodeName=queue1-st-compute-resource-1-1 Arch=x86_64 CoresPerSocket=1\n"
+                "   NodeAddr=1.2.3.4 NodeHostName=queue1-st-compute-resource-1-1 Version=25.11.4\n"
+                "   State=IDLE+CLOUD ThreadsPerCore=1 TmpDisk=0 Weight=1 Owner=N/A MCS_label=N/A\n"
+                "   Partitions=queue1\n"
+                "   BootTime=2023-01-26T09:56:30 SlurmdStartTime=2023-01-26T09:57:15\n"
+                "   LastBusyTime=2023-01-26T09:57:15\n"
+                "   Reason=someReason InstanceId=someInstanceId\n"
+                "   Comment=someComment State=DOWN+DRAIN NodeAddr=someNodeAddr "
+                "InstanceId=someInstanceId Reason=someReason\n"
+                "   Extra=someExtra State=DOWN+DRAIN NodeAddr=someNodeAddr "
+                "InstanceId=someInstanceId Reason=someReason\n"
+                "   InstanceId=i-0abc123def456 InstanceType=t2.medium\n"
+            ),
+            [
+                {
+                    "NodeName": "queue1-st-compute-resource-1-1",
+                    "NodeAddr": "1.2.3.4",
+                    "NodeHostName": "queue1-st-compute-resource-1-1",
+                    "State": "IDLE+CLOUD",
+                    "Partitions": "queue1",
+                    "SlurmdStartTime": "2023-01-26T09:57:15",
+                    "LastBusyTime": "2023-01-26T09:57:15",
+                    "Reason": "someReason InstanceId=someInstanceId",
+                    "InstanceId": "i-0abc123def456",
+                }
+            ],
+            id="Free-text fields with key=value pairs are ignored",
+        ),
+        pytest.param(
+            (
+                "NodeName=queue1-st-compute-resource-1-1 Arch=x86_64 CoresPerSocket=1\n"
+                "   NodeAddr=1.2.3.4 NodeHostName=queue1-st-compute-resource-1-1 Version=25.11.4\n"
+                "   State=IDLE+CLOUD ThreadsPerCore=1 TmpDisk=0 Weight=1 Owner=N/A MCS_label=N/A\n"
+                "   Partitions=queue1 Comment=someComment State=DOWN+DRAIN InstanceId=someInstanceId\n"
+            ),
+            [
+                {
+                    "NodeName": "queue1-st-compute-resource-1-1",
+                    "NodeAddr": "1.2.3.4",
+                    "NodeHostName": "queue1-st-compute-resource-1-1",
+                    "State": "IDLE+CLOUD",
+                    "Partitions": "queue1",
+                }
+            ],
+            id="Free-text field sharing a line with other fields is ignored up to the end of the line",
+        ),
+        pytest.param(
+            (
+                "NodeName=q1-st-cr1-1 Arch=x86_64 CoresPerSocket=1 \n"
+                "   CPUAlloc=0 CPUEfctv=2 CPUTot=2 CPULoad=0.08\n"
+                "   AvailableFeatures=static,t3.medium,cr1\n"
+                "   ActiveFeatures=static,t3.medium,cr1\n"
+                "   Gres=(null)\n"
+                "   NodeAddr=1.2.3.4 NodeHostName=q1-st-cr1-1 Version=25.11.8\n"
+                "   OS=Linux 6.12.110-135.202.amzn2023.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Sep 25 01:45:58 UTC 2026 \n"
+                "   RealMemory=3891 AllocMem=0 FreeMem=2452 Sockets=2 Boards=1\n"
+                "   State=IDLE+CLOUD ThreadsPerCore=1 TmpDisk=0 Weight=1 Owner=N/A MCS_label=N/A\n"
+                "   Partitions=q1 \n"
+                "   BootTime=2026-10-08T19:57:35 SlurmdStartTime=2026-10-08T20:00:08\n"
+                "   LastBusyTime=2026-10-08T20:00:11 ResumeAfterTime=None\n"
+                "   CfgTRES=cpu=2,mem=3891M,billing=2\n"
+                "   AllocTRES=\n"
+                "   CurrentWatts=0 AveWatts=0\n"
+                "   Reason=Node start up [root@2026-10-08T20:00:11]\n"
+                "   Comment=someComment\n"
+                "   InstanceId=i-003d87dc15788298b InstanceType=t3.medium\n"
+            ),
+            [
+                {
+                    "NodeName": "q1-st-cr1-1",
+                    "NodeAddr": "1.2.3.4",
+                    "NodeHostName": "q1-st-cr1-1",
+                    "State": "IDLE+CLOUD",
+                    "Partitions": "q1",
+                    "SlurmdStartTime": "2026-10-08T20:00:08",
+                    "LastBusyTime": "2026-10-08T20:00:11",
+                    "Reason": "Node start up [root@2026-10-08T20:00:11]",
+                    "InstanceId": "i-003d87dc15788298b",
+                }
+            ],
+            id="Real Slurm 25.11.8 output with Comment",
+        ),
+        pytest.param(
+            (
+                "NodeName=q1-st-cr1-1 Arch=x86_64 CoresPerSocket=1 \n"
+                "   CPUAlloc=0 CPUEfctv=2 CPUTot=2 CPULoad=0.08\n"
+                "   AvailableFeatures=static,t3.medium,cr1\n"
+                "   ActiveFeatures=static,t3.medium,cr1\n"
+                "   Gres=(null)\n"
+                "   NodeAddr=1.2.3.4 NodeHostName=q1-st-cr1-1 Version=25.11.8\n"
+                "   OS=Linux 6.12.110-135.202.amzn2023.x86_64 #1 SMP PREEMPT_DYNAMIC Fri Sep 25 01:45:58 UTC 2026 \n"
+                "   RealMemory=3891 AllocMem=0 FreeMem=2452 Sockets=2 Boards=1\n"
+                "   State=IDLE+CLOUD ThreadsPerCore=1 TmpDisk=0 Weight=1 Owner=N/A MCS_label=N/A\n"
+                "   Partitions=q1 \n"
+                "   BootTime=2026-10-08T19:57:35 SlurmdStartTime=2026-10-08T20:00:08\n"
+                "   LastBusyTime=2026-10-08T20:00:11 ResumeAfterTime=None\n"
+                "   CfgTRES=cpu=2,mem=3891M,billing=2\n"
+                "   AllocTRES=\n"
+                "   CurrentWatts=0 AveWatts=0\n"
+                "   Reason=Node start up [root@2026-10-08T20:00:11]\n"
+                "   Comment=someComment State=DOWN+DRAIN InstanceId=someInstanceId NodeAddr=someNodeAddr\n"
+                "   InstanceId=i-003d87dc15788298b InstanceType=t3.medium\n"
+            ),
+            [
+                {
+                    "NodeName": "q1-st-cr1-1",
+                    "NodeAddr": "1.2.3.4",
+                    "NodeHostName": "q1-st-cr1-1",
+                    "State": "IDLE+CLOUD",
+                    "Partitions": "q1",
+                    "SlurmdStartTime": "2026-10-08T20:00:08",
+                    "LastBusyTime": "2026-10-08T20:00:11",
+                    "Reason": "Node start up [root@2026-10-08T20:00:11]",
+                    "InstanceId": "i-003d87dc15788298b",
+                }
+            ],
+            id="Real Slurm 25.11.8 output with Comment containing fields",
+        ),
     ],
 )
 def test_extract_scontrol_records(scontrol_output, expected_parsed_output):
@@ -1558,6 +1678,45 @@ def test_run_scontrol_command_oserror_raises_and_logs(mocker, caplog):
     assert_that(caplog.text).contains("Unable to execute scontrol command")
 
 
+@pytest.mark.parametrize(
+    "free_text_fields",
+    [
+        pytest.param(["Comment"], id="Comment"),
+        pytest.param(["Extra"], id="Extra"),
+        pytest.param(["Comment", "Extra"], id="Comment and Extra"),
+    ],
+)
+def test_get_nodes_info_ignores_fields_in_free_text_fields(free_text_fields, mocker):
+    # A Comment or Extra containing key=value pairs must not alter the node information, e.g. making a healthy node
+    # drained
+    mocker.patch(
+        "common.schedulers.slurm_commands._run_scontrol_command",
+        return_value=(
+            "NodeName=q1-st-cr1-1 Arch=x86_64 CoresPerSocket=1 \n"
+            "   NodeAddr=1.2.3.4 NodeHostName=q1-st-cr1-1 Version=1.2.3\n"
+            "   State=IDLE+CLOUD ThreadsPerCore=1 TmpDisk=0 Weight=1 Owner=N/A MCS_label=N/A\n"
+            "   Partitions=q1 \n"
+            "   BootTime=2026-10-08T19:57:35 SlurmdStartTime=2026-10-08T20:00:08\n"
+            "   LastBusyTime=2026-10-08T20:00:11 ResumeAfterTime=None\n"
+            + "".join(
+                f"   {field}=some{field} State=DOWN+DRAIN NodeAddr=someNodeAddr InstanceId=someInstanceId\n"
+                for field in free_text_fields
+            )
+            + "   InstanceId=i-0123456789abcdefg InstanceType=t3.medium\n\n"
+        ),
+        autospec=True,
+    )
+
+    nodes = get_nodes_info("q1-st-cr1-1")
+
+    assert_that(nodes).is_length(1)
+    assert_that(nodes[0]).is_instance_of(StaticNode)
+    assert_that(nodes[0].state_string).is_equal_to("IDLE+CLOUD")
+    assert_that(nodes[0].is_drained()).is_false()
+    assert_that(nodes[0].nodeaddr).is_equal_to("1.2.3.4")
+    assert_that(nodes[0].instance_id).is_equal_to("i-0123456789abcdefg")
+
+
 def test_get_nodes_info_parsing_failure_propagates(mocker):
     # If scontrol returns a field the parser cannot interpret (e.g. an unparseable date), the error surfaces
     # instead of being silently masked by the (former) shell pipeline.
diff --git a/tests/common/schedulers/test_slurm_reservation_commands.py b/tests/common/schedulers/test_slurm_reservation_commands.py
index e600056b..fc2291f1 100644
--- a/tests/common/schedulers/test_slurm_reservation_commands.py
+++ b/tests/common/schedulers/test_slurm_reservation_commands.py
@@ -480,6 +480,7 @@ def test_get_slurm_reservations_info(mocker):
         "   TRES=cpu=480\n"
         "   Users=root Groups=(null) Accounts=(null) Licenses=(null) State=ACTIVE BurstBuffer=(null) Watts=n/a\n"
         "   MaxStartDelay=(null)\n"
+        "   Comment=someComment Users=someUser State=someState Nodes=someNodes\n"
     )
     run_scontrol_command_mocked = mocker.patch(
         "common.schedulers.slurm_reservation_commands._run_scontrol_command",