fix: move multiprocessing_start_method_fork fixture to shared conftest#4808
Conversation
📝 WalkthroughWalkthroughMoves the ChangesFixture consolidation and application
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsLinked repositories: Your configuration references 1 linked repositories, but your current plan allows 0. Analyzed ``, skipped 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 |
|
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. |
openshift-virtualization-qe-bot-4
requested review from
Ahmad-Hafe and
dalia-frank
openshift-virtualization-qe-bot-4
requested review from
RoniKishner,
acinko-rh,
dshchedr,
jpeimer,
kgoldbla,
kshvaika,
rnetser,
sarahbx and
vsibirsk
![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
🤖 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.
Inline comments:
In `@tests/conftest.py`:
- Around line 231-239: The fixture multiprocessing_start_method_fork currently
sets a process-global start method with scope="module", which can leak side
effects across tests; change its scope to "function" so each test invoking
multiprocessing_start_method_fork uses and restores the start method
independently, and keep the existing behavior of capturing original_start_method
via multiprocessing.get_start_method() and restoring it with
multiprocessing.set_start_method(original_start_method, force=True) in the
teardown to ensure no global state persists.
- Around line 232-239: Add a Google-style docstring to the
multiprocessing_start_method_fork fixture that documents its side effects and
lifecycle: state that it temporarily sets multiprocessing start method to "fork"
to avoid pickling issues, note that it yields control for the test and then
restores the original start method on teardown, and mention any important
caveats (global mutation of multiprocessing behavior and thread-safety
implications). Place this docstring immediately above the
multiprocessing_start_method_fork function definition.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 21d85561-e179-4dc0-86d0-6383ec119d8f
📒 Files selected for processing (3)
tests/chaos/conftest.pytests/conftest.pytests/storage/cdi_upload/test_upload.py
💤 Files with no reviewable changes (1)
- tests/chaos/conftest.py
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
carterpewpew
force-pushed
the
fix/move-multiprocessing-fork-fixture-to-shared-conftest
branch
from
b4b9340 to
97ce5e0
Compare
openshift-virtualization-qe-bot-5
removed
the
changes-requested-coderabbitai[bot]
label
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/conftest.py (1)
231-244:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCRITICAL: Narrow fixture scope to avoid cross-test global state leakage (unresolved).
This fixture mutates process-global multiprocessing configuration with
scope="module", causing all tests in the same module to silently inherit fork mode—even tests that don't request it. Change toscope="function"to constrain the side effect to only the test marked with@pytest.mark.usefixtures("multiprocessing_start_method_fork").🔧 Proposed fix
-@pytest.fixture(scope="module") +@pytest.fixture(scope="function") def multiprocessing_start_method_fork():As per coding guidelines: "NEVER use broader scope if fixture modifies state or creates per-test resources" and "No hidden side effects in functions. Function behavior MUST be controlled via explicit arguments, not hardcoded internally."
🤖 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/conftest.py` around lines 231 - 244, The fixture multiprocessing_start_method_fork currently sets a process-global multiprocessing start method with scope="module", causing cross-test leakage; change its pytest fixture scope to "function" so the side effect is limited to the individual test that uses `@pytest.mark.usefixtures`("multiprocessing_start_method_fork"), and ensure the teardown still restores the original_start_method by using multiprocessing.get_start_method() and multiprocessing.set_start_method(..., force=True) as currently implemented.
🤖 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.
Duplicate comments:
In `@tests/conftest.py`:
- Around line 231-244: The fixture multiprocessing_start_method_fork currently
sets a process-global multiprocessing start method with scope="module", causing
cross-test leakage; change its pytest fixture scope to "function" so the side
effect is limited to the individual test that uses
`@pytest.mark.usefixtures`("multiprocessing_start_method_fork"), and ensure the
teardown still restores the original_start_method by using
multiprocessing.get_start_method() and multiprocessing.set_start_method(...,
force=True) as currently implemented.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c5532712-085a-4862-8004-fc4f1e4bed39
📒 Files selected for processing (3)
tests/chaos/conftest.pytests/conftest.pytests/storage/cdi_upload/test_upload.py
💤 Files with no reviewable changes (1)
- tests/chaos/conftest.py
carterpewpew
force-pushed
the
fix/move-multiprocessing-fork-fixture-to-shared-conftest
branch
from
b16e729 to
fe70b82
Compare
|
/verified #4808 (comment) |
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:
Output rules: Your deliverable: Test Execution Plan
Use these formats:
Real test commands (MANDATORY when changes affect session/runtime code): When the affected code runs at session/collection time (conftest fixtures, pytest plugins,
Example output for a session-startup fix: 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 — Verification rules for smoke and gating decisions: |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Find all usages of multiprocessing_start_method_fork
rg -n 'multiprocessing_start_method_fork' --type py
echo "---SMOKE---"
rg -l '`@pytest.mark.smoke`' tests/chaos/ tests/storage/cdi_upload/ 2>/dev/null
echo "---GATING---"
rg -l '`@pytest.mark.gating`' tests/chaos/ tests/storage/cdi_upload/ 2>/dev/null
echo "---chaos conftest after change---"
cat tests/chaos/conftest.py 2>/dev/null | head -60
echo "---upload test function---"
rg -n 'test_successful_concurrent_uploads' tests/storage/cdi_upload/test_upload.py -A 10Length of output: 3348 🏁 Script executed: #!/bin/bash
# Get PR number and diff to find position of first changed line in tests/chaos/conftest.py
PR_NUMBER=4808
REPO="RedHatQE/openshift-virtualization-tests"
# Get the PR files to find the patch for tests/chaos/conftest.py
gh api repos/${REPO}/pulls/${PR_NUMBER}/files --jq '.[] | select(.filename == "tests/chaos/conftest.py") | {filename: .filename, patch: .patch}'Length of output: 1210 🏁 Script executed: #!/bin/bash
PR_NUMBER=4808
REPO="RedHatQE/openshift-virtualization-tests"
BODY="## Test Execution Plan
**Run smoke tests: False**
No smoke-marked test in \`tests/chaos/\` or \`tests/storage/cdi_upload/\` uses \`multiprocessing_start_method_fork\`. The only smoke-marked file in the affected directories is \`tests/storage/cdi_upload/test_upload_virtctl.py\`, which has no dependency on this fixture.
**Run gating tests: False**
Same reasoning — \`test_upload_virtctl.py\` (gating-marked) has no dependency path through \`multiprocessing_start_method_fork\`.
**Affected tests to run**
| Test | Reason |
|---|---|
| \`tests/storage/cdi_upload/test_upload.py::test_successful_concurrent_uploads\` | New direct consumer via \`@pytest.mark.usefixtures\`; this is the PicklingError fix path |
| \`tests/chaos/standard/test_standard.py\` | Fixture previously resolved from \`tests/chaos/conftest.py\`; now resolves from \`tests/conftest.py\` |
| \`tests/chaos/snapshot/test_snapshot.py\` | Same as above |
| \`tests/chaos/migration/test_migration.py\` | Same as above |
**Real tests (cluster required)**
Error path (the fix — PicklingError regression on Python 3.14+):
\`\`\`
pytest tests/storage/cdi_upload/test_upload.py::test_successful_concurrent_uploads
\`\`\`
Expected: test passes without \`PicklingError\`; fork start method is active when \`multiprocessing.Process\` is spawned.
Happy path (regression — chaos tests still resolve the fixture):
\`\`\`
pytest tests/chaos/standard/test_standard.py tests/chaos/snapshot/test_snapshot.py tests/chaos/migration/test_migration.py --collect-only
\`\`\`
Expected: collection completes without \`fixture 'multiprocessing_start_method_fork' not found\` errors."
gh api repos/${REPO}/pulls/${PR_NUMBER}/reviews \
--method POST \
-f commit_id="$(gh api repos/${REPO}/pulls/${PR_NUMBER} --jq '.head.sha')" \
-f body="" \
-f event="COMMENT" \
-f "comments[][path]=tests/chaos/conftest.py" \
-F "comments[][position]=5" \
-f "comments[][body]=${BODY}"Length of output: 3797 |
|
/retest tox |
|
carterpewpew is not allowed to run retest commands.
|
|
Clean rebase detected — no code changes compared to previous head ( |
|
/lgtm @carterpewpew please add more information about how it was verified - for example, a pytest command you ran. |
|
/lgtm @carterpewpew waiting on your input in #4808 (comment) to approve |
|
Command run (same for both before/after): Before (without this PR) — test crashes immediately: The forkserver start method (Python 3.14 default) tries to pickle the Kubernetes client's connection pool. Workers never start, test hangs. After (with this PR) — multiprocessing fork works, test logic executes: |
On Python 3.14, the default multiprocessing start method changed from fork to forkserver (python/cpython#132898), which requires all Process arguments to be picklable. The unprivileged_client Kubernetes client object contains thread locks and closures that cannot be pickled, causing _pickle.PicklingError in test_successful_concurrent_uploads. Move the multiprocessing_start_method_fork fixture from tests/chaos/conftest.py to tests/conftest.py so it can be reused across test directories, and apply it to the affected upload test. Changes: - Move fixture to tests/conftest.py (shared across all test dirs) - Remove duplicate from tests/chaos/conftest.py - Add @pytest.mark.usefixtures("multiprocessing_start_method_fork") to test_successful_concurrent_uploads in test_upload.py - Existing chaos consumers (test_standard, test_snapshot, test_migration) resolve the fixture from the parent conftest Signed-off-by: Jathavedhan M <jathavedhan.m@ibm.com>
|
Clean rebase detected — no code changes compared to previous head ( |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/conftest.py (1)
231-244:⚠️ Potential issue | 🟠 Major | ⚡ Quick winModule-scoped global start-method mutation leaks across tests.
On Line 231,
scope="module"keeps the process-global start method as"fork"for the rest of that module once activated, so tests that did not request this fixture can silently run under a different multiprocessing mode. This creates order-dependent behavior.Proposed fix
-@pytest.fixture(scope="module") +@pytest.fixture(scope="function") def multiprocessing_start_method_fork(): """Temporarily set multiprocessing start method to ``fork``. Side effects: Changes the process-global multiprocessing start method to ``fork`` - for the module lifetime, then restores the previous method on teardown. + for the test lifetime, then restores the previous method on teardown. """As per coding guidelines, function behavior should avoid hidden side effects and remain explicit to the caller.
🤖 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/conftest.py` around lines 231 - 244, The multiprocessing_start_method_fork fixture uses scope="module", which causes the fork start method to persist globally across all subsequent tests in that module, including those that don't explicitly request this fixture. This creates hidden side effects and order-dependent behavior. Change the fixture scope from "module" to "function" so that the multiprocessing start method is only modified for the specific test function that requests the fixture and is properly restored immediately after that test completes.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@tests/conftest.py`:
- Around line 231-244: The multiprocessing_start_method_fork fixture uses
scope="module", which causes the fork start method to persist globally across
all subsequent tests in that module, including those that don't explicitly
request this fixture. This creates hidden side effects and order-dependent
behavior. Change the fixture scope from "module" to "function" so that the
multiprocessing start method is only modified for the specific test function
that requests the fixture and is properly restored immediately after that test
completes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b2f385fd-e02e-43ec-9e55-90532a611ad5
📒 Files selected for processing (3)
tests/chaos/conftest.pytests/conftest.pytests/storage/cdi_upload/test_upload.py
💤 Files with no reviewable changes (1)
- tests/chaos/conftest.py
|
/approve |
|
/approve |
|
New container for quay.io/openshift-cnv/openshift-virtualization-tests:latest published |
11 participants
Short description:
Move multiprocessing_start_method_fork fixture to shared conftest for reuse across test directories
More details:
Python 3.14 changed the default multiprocessing start method from fork to forkserver, requiring all Process arguments to be picklable. test_successful_concurrent_uploads passes unprivileged_client to child processes, which contains unpicklable thread locks, causing PicklingError.
What this PR does / why we need it:
Which issue(s) this PR fixes:
Fixes PicklingError in test_successful_concurrent_uploads on Python 3.14+
Special notes for reviewer:
The multiprocessing import in tests/chaos/conftest.py is kept — it is still used by chaos_worker_background_process.
jira-ticket:
NONE
Summary by CodeRabbit