Fixing maxvcpu mismatch with topology issue, making it dynamic! #6674
Conversation
Maxvcpu is hardcoded for the specific testcase in virsh_emulatorpin.py. When smp value, vcpu_cores, vcpu_threads, vcpu_sockets is specified at the time of running the testcase then the hardcoded value will fail. Adding the code to get this value dynamically based on the smp value provided in the test suite config file. 1 example is that hardcoded value is 7 in virsh_emulatorpin.cfg but in the testsuite config smp value given is 32 not topology will be affected in this case my check will help. Signed-off-by: Anushree-Mathur <[email protected]>
WalkthroughThe change modifies the Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 |
![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
🧹 Nitpick comments (1)
libvirt/tests/src/virsh_cmd/domain/virsh_emulatorpin.py (1)
271-281: Dynamicmax_vcpucalculation looks correct; consider caching topology and tightening error handlingThe new logic to derive
max_vcpufromsockets * cores * threadswhenvmxml.get_cpu_topology()is present is consistent with the intent to matchmaxvcputo the runtimesmp/topology and should fix the mismatch you described for cases likesmp=32.Two small, non-blocking improvements you might consider:
- Avoid calling
vmxml.get_cpu_topology()twice and cache the result, both for clarity and to ensure you’re working with a single, well-defined snapshot:topology = vmxml.get_cpu_topology() if topology: max_vcpu = int(topology['sockets']) * int(topology['cores']) * int(topology['threads']) vcpu_attrs['vcpu'] = max_vcpu
- For robustness, think about how you want to behave if
topologyis present but missing any ofsockets/cores/threadsor contains unexpected values. Right now that would raise a genericKeyError/ValueErrorand fail the test without a clear message; you might prefer either a more explicittest.fail()or to log and skip updatingvcpu_attrs['vcpu']in such a case.Also, this dynamic update only runs when
vcpu_attrs['cpuset'] == "x,y". Please double‑check that all scenarios wheresmp/ topology can vary and previously hit the hard‑codedmaxvcpu=7do go through this samecpusetplaceholder path; otherwise, some configs might still rely on static values in the.cfg.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
libvirt/tests/src/virsh_cmd/domain/virsh_emulatorpin.py(1 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
|
1 scenario example: Before applying the patch: After applying the patch: |
1 participant
Maxvcpu is hardcoded for the specific testcase in virsh_emulatorpin.py. When smp value, vcpu_cores, vcpu_threads, vcpu_sockets is specified at the time of running the testcase then the hardcoded value will fail. Adding the code to get this value dynamically based on the smp value provided in the test suite config file.
1 example is that hardcoded value is 7 in virsh_emulatorpin.cfg but in the testsuite config smp value given is 32 not topology will be affected in this case my check will help.
Signed-off-by: Anushree-Mathur [email protected]
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.