The patch fixes most of the failures from libvirt_mem - #6035
Open
TasmiyaNalatwad wants to merge 1 commit into
Open
TasmiyaNalatwad wants to merge 1 commit into
TasmiyaNalatwad wants to merge 1 commit into
Conversation
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
December 3, 2024 07:19
87ec01a to
9907b71
Compare
Contributor
Author
|
There are few failures in this set of tests which require some other fix, which will be taken up as other task and fixed. Previous results The results after patch applied are below |
chunfuwen
reviewed
Apr 2, 2025
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
April 10, 2025 13:18
9907b71 to
d5257a0
Compare
chunfuwen
reviewed
May 6, 2025
Contributor
Author
|
@chunfuwen @chloerh @dzhengfy would you please review the changes and help me in proceeding with the PR merge. Thank you |
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
September 10, 2025 10:26
d5257a0 to
c3c2d63
Compare
WalkthroughAdds PPC64 and PPC64LE-specific memory configuration overrides in Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant TR as Test Runner
participant LM as libvirt_mem.py
participant PM as platform.machine()
participant XML as Domain XML / NUMA Cells
TR->>LM: check_dom_xml()
LM->>PM: machine()
PM-->>LM: arch (e.g., ppc64le / other)
LM->>XML: read domain memory and NUMA cell values
alt arch contains "ppc64"
note right of LM#dff0d8: Sum NUMA-cell memory for cur/max checks
else
note right of LM#f0f0d8: Use standard memory fields for checks
end
TR->>LM: modify_domain_xml(align_mem_values=...)
LM->>PM: machine()
PM-->>LM: arch
LM->>XML: read per-NUMA-cell memory values
alt align_mem_values enabled
opt arch == "ppc64le"
note right of LM#d8eff0: Clamp each cell to ≥1 GiB, then align
end
note right of LM#f0f0d8: Apply alignment to per-cell memory values
end
LM-->>TR: return updated/validated XML
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
🔇 Additional comments (1)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
libvirt/tests/src/libvirt_mem.py (1)
206-221: Refactor arch detection and guard NUMA cells in libvirt/tests/src/libvirt_mem.py (lines ~206–221)
Normalizearchviacpu_util.get_arch().lower()with aplatform.machine().lower()fallback, and replace directcpuxml.numa_cellaccess with a safegetattrguard and non‐empty check to avoid None or empty list errors.- arch = platform.machine() + # Normalize arch with cpu_util, fallback to platform.machine() + try: + arch = cpu_util.get_arch().lower() + except Exception: + arch = platform.machine().lower() @@ - numa_cells = cpuxml.numa_cell - - if 'ppc64' in arch: - cur_mem = max_mem = sum(int(cell["memory"]) for cell in numa_cells) + numa_cells = getattr(cpuxml, "numa_cell", []) or [] + if arch.startswith("ppc64") and numa_cells: + cur_mem = max_mem = sum(int(cell["memory"]) for cell in numa_cells)
🧹 Nitpick comments (1)
libvirt/tests/cfg/libvirt_mem.cfg (1)
66-67: Drop redundant override (same value already set).tg_size is already 524288 at Line 62; repeating it under ppc64/ppc64le is unnecessary and increases maintenance.
Apply:
- ppc64,ppc64le: - tg_size = 524288
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/libvirt_mem.cfg(3 hunks)libvirt/tests/src/libvirt_mem.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Python 3.11
- GitHub Check: Python 3.8
- GitHub Check: Python 3.12
- GitHub Check: Python 3.9
🔇 Additional comments (4)
libvirt/tests/cfg/libvirt_mem.cfg (3)
34-36: Arch-specific overrides here look good.Placing ppc64/ppc64le overrides under the variant block (without “only”) correctly narrows the changed keys to those arches without filtering out the whole variant for others.
44-46: mem_basic PPC overrides are appropriate.Consistent with the top-level positive_test overrides; no issues spotted.
216-217: Reasonable PPC-specific bump for attach_many_times.max_mem_rt=3670016 aligns with larger DIMM needs on ppc; OK.
libvirt/tests/src/libvirt_mem.py (1)
370-382: Refactor: broaden ppc match and simplify loop- arch = platform.machine() - min_memory_value = 1048576 # 1 GiB in KiB + try: + arch = cpu_util.get_arch().lower() + except Exception: + arch = platform.machine().lower() + min_memory_value = 1048576 # 1 GiB in KiB @@ - for cell in range(cells.__len__()): - memory_value = int(cells[cell]["memory"]) - if arch == "ppc64le": - memory_value = max(memory_value, min_memory_value) - else: - memory_value = memory_value - cells[cell]["memory"] = str(utils_numeric.align_value( - memory_value, - align_to_value)) + for idx, cell in enumerate(cells): + memory_value = int(cell["memory"]) + if arch.startswith("ppc64"): + memory_value = max(memory_value, min_memory_value) + cells[idx]["memory"] = str( + utils_numeric.align_value(memory_value, align_to_value) + )Consider making
min_memory_valuea config knob (e.g.,ppc64_min_cell_mem) to avoid hardcoding.
Contributor
Author
|
@chunfuwen @chloerh @dzhengfy May I request you all to please review the changes and let me know the inputs. Thank you all |
|
@chunfuwen @chloerh @dzhengfy @luckyh Pleae review the patch. All review comments are been addressed. |
dzhengfy
reviewed
Oct 10, 2025
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
October 10, 2025 06:39
c3c2d63 to
f63999b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
libvirt/tests/src/libvirt_mem.py (3)
216-221: Move NUMA cell extraction inside the PPC64-specific conditional.Lines 216-218 extract
cpuxmlandnuma_cellsunconditionally but only use them when'ppc64' in arch. This creates unused variable assignments on non-PPC64 platforms.Apply this diff to improve efficiency:
- cpuxml = dom_xml.cpu - numa_cells = cpuxml.numa_cell - if 'ppc64' in arch: + cpuxml = dom_xml.cpu + numa_cells = cpuxml.numa_cell cur_mem = max_mem = sum(int(cell["memory"]) for cell in numa_cells)
371-371: Move constant inside conditional scope.
min_memory_valueis defined unconditionally on Line 371 but only used whenarch == "ppc64le"(Line 377). Moving it inside the conditional improves clarity and avoids unnecessary assignments on non-PPC64LE platforms.Apply this diff:
cells = [ast.literal_eval(x) for x in numa_cells] arch = platform.machine() - min_memory_value = 1048576 # 1 GiB in KiB # Rounding the numa memory values if align_mem_values: for cell in range(cells.__len__()): memory_value = int(cells[cell]["memory"]) if arch == "ppc64le": + min_memory_value = 1048576 # 1 GiB in KiB memory_value = max(memory_value, min_memory_value)
206-206: Consider caching platform detection result.
platform.machine()is called twice (Lines 206 and 370) in separate functions within the same test run. While this is not a performance concern, consider whether the architecture could be detected once at module level or passed as a parameter to improve consistency.Example approach at module level:
# Near the top of the file after imports HOST_ARCH = platform.machine().lower()Then use
HOST_ARCHin both functions. This ensures consistent detection across all test functions.Also applies to: 370-370
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
libvirt/tests/cfg/libvirt_mem.cfg(2 hunks)libvirt/tests/src/libvirt_mem.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- libvirt/tests/cfg/libvirt_mem.cfg
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Python 3.8
- GitHub Check: Python 3.9
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
October 10, 2025 07:32
f63999b to
8514b6d
Compare
Contributor
Author
|
@dzhengfy Would you please review the changes made. Thank you! |
Contributor
Author
|
@chunfuwen Thank you for the approval. Would you please help me in merging this PR |
Collaborator
|
As we have the approval can we merge this to tp-libvirt. |
Contributor
Author
|
@chunfuwen As we have the approval for this PR. Would you please help me in merging this PR. Thanks in Advance. |
Contributor
Author
|
@luckyh @chunfuwen @dzhengfy Requesting to please help me merge this PR as i already have approval. Apologies for addressing again. Thanks in Advance. |
Contributor
Author
|
@luckyh @chunfuwen @dzhengfy Requesting to please help me merge this PR as i already have approval. Thanks in Advance. |
crobinso
requested changes
Sep 15, 2026
Collaborator
|
@TasmiyaNalatwad I do not think any of those users are involved in the project any longer. I can help with review. |
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
September 16, 2026 07:18
8514b6d to
ba77849
Compare
Based on the test requirements adding/modifying some of the cfg values to make the guest run and flexibly test the scenarios. Signed-off-by: Tasmiya Nalatwad <tasmiya@linux.vnet.ibm.com>
TasmiyaNalatwad
force-pushed
the
libvirt_mem_fixes
branch
from
September 16, 2026 07:23
ba77849 to
6082ed7
Compare
Contributor
Author
Thank you for your time, i have addressed review comments from coderabbitai and marked addressed ones as Resolved. Please have a look at this PR. TIA |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Based on the test requirements adding/modifying some of the cfg values to make the guest run and flexibly test the scenarios.
Summary by CodeRabbit
Bug Fixes
Tests