Function to filter NUMA node ids with allocated memory! #4299
Conversation
…d as nodeset! Added a function to identify a nodeset consisting of only those numa node ids which has memory allocated to it. This function is then used to retrieve the filtered NUMA nodeset, which is subsequently employed for hugepage memory backing. This ensures that hugepages are allocated only from nodes with actual memory resources. For example: Host has 2 numa nodes with id as 0 and 4, but only numa node 4 has memory so that will be used to back the memory being used as a nodeset. Signed-off-by: Anushree-Mathur <[email protected]>
WalkthroughThe change modifies the Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Anushree-Mathur
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @virttest/libvirt_vm.py:
- Around line 677-690: The get_guest_numa function should use
avocado.utils.process instead of subprocess and add error handling: replace
sp.check_output("numactl -H", shell=True) with a call using process.run or
process.system to execute ["numactl", "-H"] without shell, catch
CalledProcessError (or check return code) and return a safe default (e.g., empty
string) or log the error before failing; update parsing to handle empty output
robustly; also remove the unused import subprocess as sp to keep imports
consistent.
🧹 Nitpick comments (1)
virttest/libvirt_vm.py (1)
17-17: Prefer usingavocado.utils.processfor consistency.The codebase already imports and uses
avocado.utils.process(line 24) for command execution. Using rawsubprocessis inconsistent with the established patterns. Consider removing this import and usingprocess.run()orprocess.getoutput()instead in the new helper function.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
virttest/libvirt_vm.py
🧰 Additional context used
🧬 Code graph analysis (1)
virttest/libvirt_vm.py (2)
virttest/remote_commander/remote_runner.py (1)
shell(660-676)virttest/staging/utils_memory.py (1)
numa_nodes(94-107)
🪛 GitHub Actions: pre-commit
virttest/libvirt_vm.py
[error] 1206-1206: Trailing whitespace detected on line 1206; pre-commit hook trailing-whitespace fixed the line.
🪛 Ruff (0.14.10)
virttest/libvirt_vm.py
679-679: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
679-679: Starting a process with a partial executable path
(S607)
| def get_guest_numa(): | ||
| # Retrieve only NUMA nodes with allocated memory resources | ||
| out_numa = sp.check_output("numactl -H", shell=True).decode() | ||
| numa_nodes = [] | ||
| for line in out_numa.splitlines(): | ||
| line = line.strip() | ||
| if line.startswith("node") and "size:" in line: | ||
| parts = line.split() | ||
| node_id = parts[1] | ||
| size_mb = int(parts[3]) | ||
| if size_mb > 0: | ||
| numa_nodes.append(node_id) | ||
|
|
||
| return ",".join(numa_nodes) |
There was a problem hiding this comment.
Add error handling and use process module for consistency.
This function has several issues:
-
Missing error handling: If
numactlis not installed or fails,sp.check_outputraisesCalledProcessError, causing VM creation to fail with an unhelpful traceback. -
Inconsistent with codebase patterns: The codebase uses
avocado.utils.processfor command execution. Seevirttest/staging/utils_memory.pyfor a similar pattern. -
Unnecessary
shell=True: The command doesn't use shell features, soshell=Trueis not needed (flagged by static analysis S602/S607).
🔧 Suggested fix using process module with error handling
def get_guest_numa():
# Retrieve only NUMA nodes with allocated memory resources
- out_numa = sp.check_output("numactl -H", shell=True).decode()
+ try:
+ result = process.run("numactl -H", shell=False, verbose=False)
+ out_numa = result.stdout_text
+ except process.CmdError as e:
+ LOG.warning("Failed to get NUMA info via numactl: %s", e)
+ return ""
numa_nodes = []
for line in out_numa.splitlines():
line = line.strip()
if line.startswith("node") and "size:" in line:
parts = line.split()
node_id = parts[1]
size_mb = int(parts[3])
if size_mb > 0:
numa_nodes.append(node_id)
return ",".join(numa_nodes)With this change, you can also remove the import subprocess as sp at line 17.
Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.14.10)
679-679: subprocess call with shell=True seems safe, but may be changed in the future; consider rewriting without shell
(S602)
679-679: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
In @virttest/libvirt_vm.py around lines 677 - 690, The get_guest_numa function
should use avocado.utils.process instead of subprocess and add error handling:
replace sp.check_output("numactl -H", shell=True) with a call using process.run
or process.system to execute ["numactl", "-H"] without shell, catch
CalledProcessError (or check return code) and return a safe default (e.g., empty
string) or log the error before failing; update parsing to handle empty output
robustly; also remove the unused import subprocess as sp to keep imports
consistent.
| guest_numa = str(params.get("hugepage_pinned_numa")) | ||
| if guest_numa == "None": | ||
| # if user didn't mention hugepage_pinnned_numa use | ||
| # numa_nodes to back all the numa nodes. | ||
| guest_numa = int(params.get("numa_nodes", 2)) | ||
| guest_numa = ",".join(map(str, list(range(guest_numa)))) | ||
| # numa_nodes which has memory allocated, to back all | ||
| # the numa nodes. | ||
| guest_numa = get_guest_numa() | ||
| virt_install_cmd += pin_hugepage(help_text, hp_size, guest_numa) |
There was a problem hiding this comment.
Fix trailing whitespace and handle empty get_guest_numa() return.
-
Line 1209 has trailing whitespace (flagged by pre-commit CI).
-
If
get_guest_numa()returns an empty string (e.g.,numactlfails or no nodes have memory), passing an emptyguest_numatopin_hugepage()may produce invalid configuration.
🔧 Suggested fix
if guest_numa == "None":
# if user didn't mention hugepage_pinnned_numa use
- # numa_nodes which has memory allocated, to back all
+ # numa_nodes which has memory allocated, to back all
# the numa nodes.
guest_numa = get_guest_numa()
+ if not guest_numa:
+ LOG.error("No NUMA nodes with allocated memory found for hugepage pinning")
+ else:
- virt_install_cmd += pin_hugepage(help_text, hp_size, guest_numa)
+ virt_install_cmd += pin_hugepage(help_text, hp_size, guest_numa)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| guest_numa = str(params.get("hugepage_pinned_numa")) | |
| if guest_numa == "None": | |
| # if user didn't mention hugepage_pinnned_numa use | |
| # numa_nodes to back all the numa nodes. | |
| guest_numa = int(params.get("numa_nodes", 2)) | |
| guest_numa = ",".join(map(str, list(range(guest_numa)))) | |
| # numa_nodes which has memory allocated, to back all | |
| # the numa nodes. | |
| guest_numa = get_guest_numa() | |
| virt_install_cmd += pin_hugepage(help_text, hp_size, guest_numa) | |
| guest_numa = str(params.get("hugepage_pinned_numa")) | |
| if guest_numa == "None": | |
| # if user didn't mention hugepage_pinnned_numa use | |
| # numa_nodes which has memory allocated, to back all | |
| # the numa nodes. | |
| guest_numa = get_guest_numa() | |
| if not guest_numa: | |
| LOG.error("No NUMA nodes with allocated memory found for hugepage pinning") | |
| else: | |
| virt_install_cmd += pin_hugepage(help_text, hp_size, guest_numa) |
🧰 Tools
🪛 GitHub Actions: pre-commit
[error] 1206-1206: Trailing whitespace detected on line 1206; pre-commit hook trailing-whitespace fixed the line.
1 participant
Added a function to identify a nodeset consisting of only those numa node ids which has memory allocated to it.
This function is then used to retrieve the filtered NUMA nodeset, which is subsequently employed for hugepage memory backing. This ensures that hugepages are allocated only from nodes with actual memory resources. For example:
Host has 2 numa nodes with id as 0 and 4, but only numa node 4 has memory so that will be used to back the memory being used as a nodeset.
Signed-off-by: Anushree-Mathur [email protected]
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.