WIP: Use PowerShell ProcessorCount for Windows CPU hotplug verification #4753
WIP: Use PowerShell ProcessorCount for Windows CPU hotplug verification #4753SamAlber wants to merge 1 commit intoRedHatQE:mainfrom
Conversation
%NUMBER_OF_PROCESSORS% is an inherited environment variable from the parent process (sshd). After CPU hotplug, sshd retains the stale boot-time value, causing SSH commands to report fewer CPUs than actually available. Replace with [Environment]::ProcessorCount which queries the kernel directly via GetSystemInfo, reliably reflecting hotplugged CPUs. This aligns with how kubevirt's own tests use nproc (kernel query) on Linux rather than environment variables. Signed-off-by: Samuel Albershtein <salbersh@redhat.com>
|
Report bugs in Issues Welcome! 🎉This pull request will be automatically processed with the following features: 🔄 Automatic Actions
📋 Available CommandsPR Status Management
Review & Approval
Testing & Validation
Container Operations
Cherry-pick Operations
Label Management
✅ Merge RequirementsThis PR will be automatically approved when the following conditions are met:
📊 Review ProcessApprovers and ReviewersApprovers:
Reviewers:
Available Labels
AI Features
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
|
/build-and-push-container |
📝 WalkthroughWalkthroughThe PR updates Windows CPU count detection in test utilities from batch-style environment variable echoing to a PowerShell command for improved reliability on modern Windows systems. ChangesWindows CPU Detection Method
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Review notesWhy PowerShell over batch environment variables? The original approach (
No risks identified: Single-line substitution with clear intent; no new dependencies or side effects. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/utils.py (1)
1-1:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: PR template required sections appear missing in the PR description.
The provided PR metadata does not include the required template headers (
What this PR does / why we need it,Which issue(s) this PR fixes,Special notes for reviewer,jira-ticket). Please restore the template sections (with meaningful content for the first one) to satisfy repo policy and keep review/automation consistent.As per coding guidelines, required PR template sections must be present, and
What this PR does / why we need it:must contain meaningful content.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/utils.py` at line 1, The PR description is missing required template sections; update the PR description to include the headers "What this PR does / why we need it", "Which issue(s) this PR fixes", "Special notes for reviewer", and "jira-ticket" and provide meaningful content under "What this PR does / why we need it" (summarize the change and rationale), fill "Which issue(s) this PR fixes" with relevant issue IDs, add any review notes under "Special notes for reviewer", and include the ticket identifier under "jira-ticket" so the repo automation and reviewers can validate the PR.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/utils.py`:
- Line 1: The PR description is missing required template sections; update the
PR description to include the headers "What this PR does / why we need it",
"Which issue(s) this PR fixes", "Special notes for reviewer", and "jira-ticket"
and provide meaningful content under "What this PR does / why we need it"
(summarize the change and rationale), fill "Which issue(s) this PR fixes" with
relevant issue IDs, add any review notes under "Special notes for reviewer", and
include the ticket identifier under "jira-ticket" so the repo automation and
reviewers can validate the PR.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 37fd041b-a754-4783-878e-3fe1fa97c387
📒 Files selected for processing (1)
tests/utils.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:pr-4753 published |
|
/verified -k "test_hotplug_cpu and WIN-VM" |
Test execution plan request detailsCRITICAL: You MUST post an inline review comment on the first changed line of the first file. As an expert software testing engineer, analyze all modified files in this PR and create a targeted test execution plan. Analysis Requirements:
Your deliverable: Test Execution Plan
Guidelines:
Hardware-Related Checks (SR-IOV, GPU, DPDK): When PR modifies fixtures for hardware-specific resources:
CRITICAL WORKFLOW COMPLETION RULES: When responding to this test execution plan request, you MUST follow these rules EXACTLY:
ABSOLUTE PROHIBITIONS (violating these creates empty/meaningless reviews):
Remember: The pull request review is visible to users. Additional comments are redundant noise. CRITICAL: Before setting "Run smoke tests: True", you MUST use shell scripts to: |
| def get_os_cpu_count(vm): | ||
| if "windows" in vm.name: | ||
| cmd = shlex.split("echo %NUMBER_OF_PROCESSORS%") | ||
| cmd = shlex.split('powershell.exe -command "[Environment]::ProcessorCount"') |
There was a problem hiding this comment.
Test Execution Plan
Run smoke tests: False
(No dependency path found from any @pytest.mark.smoke test to get_os_cpu_count in tests/utils.py)
Affected Tests
The change only affects the Windows VM branch of get_os_cpu_count. Target Windows CPU hotplug tests specifically:
tests/virt/node/hotplug/test_cpu_memory_hotplug.py— filter with-k "test_hotplug_cpu and WIN"tests/infrastructure/instance_types/test_cpu_memory_hotplug_instancetype.py::TestCPUHotPlugInstanceType::test_hotplug_cpu_instance_typetests/virt/node/migration_and_maintenance/test_post_copy_migration.py— filter with-k "test_hotplug_cpu and WIN"
Note: Linux VMs are unaffected (the
else: nprocbranch is unchanged).
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Unrecoverable error: Failed to clone repository 🏁 Script executed: Length of output: 511 🏁 Script executed: Length of output: 8353 🏁 Script executed: Length of output: 1735 |
|
/wip need one more investigation |
openshift-virtualization-qe-bot
changed the title
5 participants
%NUMBER_OF_PROCESSORS% is an inherited environment variable from the parent process (sshd). After CPU hotplug, sshd retains the stale boot-time value, causing SSH commands to report fewer CPUs than actually available.
Replace with [Environment]::ProcessorCount which queries the kernel directly via GetSystemInfo, reliably reflecting hotplugged CPUs.
This aligns with the Linux scenario that uses nproc (kernel query) on Linux rather than environment variables.
Short description:
More details:
What this PR does / why we need it:
Which issue(s) this PR fixes:
Special notes for reviewer:
jira-ticket:
Summary by CodeRabbit