Skip to content

[clustermgtd] Fix an issue where the Slurm node Comment and Extra fields could alter the node information read by ParallelCluster. #741

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
gmarciani merged 1 commit into from
Oct 9, 2026
Merged

[clustermgtd] Fix an issue where the Slurm node Comment and Extra fields could alter the node information read by ParallelCluster. #741

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
------
Expand Down
9 changes: 8 additions & 1 deletion src/common/schedulers/slurm_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Comment on lines +84 to +85

@hanwen-cluster hanwen-cluster Oct 9, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor: what's the issue if we do not discard them? It would be cleaner if we don't add another global variable.
This is a minor style comment

@gmarciani gmarciani Oct 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do not discard them we would have two problems:

  1. we would have data around that we do not need, bad practice in general
  2. we need further code changes to handle them

Since the issue with having a new constant is code style, I suggest to keep the change as is.


# Fields extracted from `scontrol show partitions` output. `(?<!Next)` ensures `State` is matched but
# `NextState` is not.
SCONTROL_PARTITION_INFO_FIELD_REGEX = re.compile(
Expand Down Expand Up @@ -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)
Expand Down
2 changes: 1 addition & 1 deletion src/common/schedulers/slurm_reservation_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
)

Expand Down
161 changes: 160 additions & 1 deletion tests/common/schedulers/test_slurm_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Expand Down Expand Up @@ -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.
Expand Down
1 change: 1 addition & 0 deletions tests/common/schedulers/test_slurm_reservation_commands.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down
Loading