fix: move multiprocessing_start_method_fork fixture to shared conftest#4808
Open
carterpewpew wants to merge 1 commit into
Open
Conversation
Contributor
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughMoves the multiprocessing start-method fixture from ChangesFixture consolidation and application
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 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)
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}
Contributor
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
Contributor
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
d5a40b7 to
e0cb3b4
Compare
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:running
openshift-virtualization-qe-bot
added
tox:verify-tc-requirement-polarion:passed
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4934. Overlapping filestests/conftest.py |
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>
carterpewpew
force-pushed
the
fix/move-multiprocessing-fork-fixture-to-shared-conftest
branch
from
e0cb3b4 to
b16e729
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/conftest.py (1)
231-244:⚠️ Potential issue | 🟠 MajorHIGH: Scope this multiprocessing fixture to the requesting test only.
multiprocessing.set_start_method()mutates interpreter-global state. Withscope="module", one test opting into this fixture keeps the whole module on"fork"until module teardown, so unrelated tests in that module can become order-dependent.Proposed fix
-@pytest.fixture(scope="module") +@pytest.fixture(scope="function") def multiprocessing_start_method_fork():As per coding guidelines: "No hidden side effects. 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, Change the multiprocessing_start_method_fork fixture to be per-test instead of module-wide: update the `@pytest.fixture` decorator on multiprocessing_start_method_fork to scope="function" (or remove the scope param) so multiprocessing.get_start_method() / multiprocessing.set_start_method("fork", force=True) only apply for the requesting test, and keep the existing yield and teardown that calls multiprocessing.set_start_method(original_start_method, force=True) to restore the prior method; ensure the fixture still references multiprocessing.get_start_method and multiprocessing.set_start_method by name so the original method is captured and restored.
🤖 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 465-478: The new cpu_arch filter can produce an empty schedulable
list and downstream fixtures (e.g., worker_node1/worker_node2/worker_node3)
index into it; update the fixture that builds schedulable (the code using
cpu_arch and variable schedulable in tests/conftest.py) to fail-fast: after
computing schedulable, if cpu_arch is set and schedulable is empty, raise a
clear RuntimeError (or pytest.Skip/Fail per project convention) that includes
the requested cpu_arch value and the list of available node architectures/names
so callers immediately see why no nodes matched instead of hitting later
IndexErrors. Ensure the exception is raised before yielding.
---
Duplicate comments:
In `@tests/conftest.py`:
- Around line 231-244: Change the multiprocessing_start_method_fork fixture to
be per-test instead of module-wide: update the `@pytest.fixture` decorator on
multiprocessing_start_method_fork to scope="function" (or remove the scope
param) so multiprocessing.get_start_method() /
multiprocessing.set_start_method("fork", force=True) only apply for the
requesting test, and keep the existing yield and teardown that calls
multiprocessing.set_start_method(original_start_method, force=True) to restore
the prior method; ensure the fixture still references
multiprocessing.get_start_method and multiprocessing.set_start_method by name so
the original method is captured and restored.
🪄 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: 2af42e3d-fbb6-4a8b-87ca-4473d639e382
📒 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
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
♻️ Duplicate comments (1)
tests/conftest.py (1)
231-244:⚠️ Potential issue | 🟠 MajorHIGH: Scope this multiprocessing fixture to the requesting test only.
multiprocessing.set_start_method()mutates interpreter-global state. Withscope="module", one test opting into this fixture keeps the whole module on"fork"until module teardown, so unrelated tests in that module can become order-dependent.Proposed fix
-@pytest.fixture(scope="module") +@pytest.fixture(scope="function") def multiprocessing_start_method_fork():As per coding guidelines: "No hidden side effects. 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, Change the multiprocessing_start_method_fork fixture to be per-test instead of module-wide: update the `@pytest.fixture` decorator on multiprocessing_start_method_fork to scope="function" (or remove the scope param) so multiprocessing.get_start_method() / multiprocessing.set_start_method("fork", force=True) only apply for the requesting test, and keep the existing yield and teardown that calls multiprocessing.set_start_method(original_start_method, force=True) to restore the prior method; ensure the fixture still references multiprocessing.get_start_method and multiprocessing.set_start_method by name so the original method is captured and restored.
🤖 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 465-478: The new cpu_arch filter can produce an empty schedulable
list and downstream fixtures (e.g., worker_node1/worker_node2/worker_node3)
index into it; update the fixture that builds schedulable (the code using
cpu_arch and variable schedulable in tests/conftest.py) to fail-fast: after
computing schedulable, if cpu_arch is set and schedulable is empty, raise a
clear RuntimeError (or pytest.Skip/Fail per project convention) that includes
the requested cpu_arch value and the list of available node architectures/names
so callers immediately see why no nodes matched instead of hitting later
IndexErrors. Ensure the exception is raised before yielding.
---
Duplicate comments:
In `@tests/conftest.py`:
- Around line 231-244: Change the multiprocessing_start_method_fork fixture to
be per-test instead of module-wide: update the `@pytest.fixture` decorator on
multiprocessing_start_method_fork to scope="function" (or remove the scope
param) so multiprocessing.get_start_method() /
multiprocessing.set_start_method("fork", force=True) only apply for the
requesting test, and keep the existing yield and teardown that calls
multiprocessing.set_start_method(original_start_method, force=True) to restore
the prior method; ensure the fixture still references
multiprocessing.get_start_method and multiprocessing.set_start_method by name so
the original method is captured and restored.
🪄 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: 2af42e3d-fbb6-4a8b-87ca-4473d639e382
📒 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
🛑 Comments failed to post (1)
tests/conftest.py (1)
465-478:
⚠️ Potential issue | 🟠 Major | ⚡ Quick winHIGH: Fail fast when the new
cpu_archfilter removes every schedulable node.This fixture now silently returns
[]when no node matchescpu_arch, but downstream fixtures likeworker_node1,worker_node2, andworker_node3index into this list directly. That turns an architecture-selection problem into laterIndexErrors with no clue about the real cause.Proposed fix
schedulable = [ node for node in nodes if schedulable_label in node.labels.keys() and node.labels[schedulable_label] == "true" and not node.instance.spec.unschedulable and not kubernetes_taint_exists(node) and node.kubelet_ready and (not cpu_arch or node.labels.get(KUBERNETES_ARCH_LABEL) == cpu_arch) ] + + if cpu_arch and not schedulable: + pytest.exit(f"No schedulable nodes found for requested cpu_arch={cpu_arch}")As per coding guidelines: "No defensive programming. Fail-fast and don't hide bugs with fake defaults."
🧰 Tools
🪛 Ruff (0.15.15)
[warning] 469-469: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
[warning] 477-477: Logging statement uses f-string
(G004)
[warning] 478-478: No teardown in fixture
schedulable_nodes, usereturninstead ofyieldReplace
yieldwithreturn(PT022)
🤖 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 465 - 478, The new cpu_arch filter can produce an empty schedulable list and downstream fixtures (e.g., worker_node1/worker_node2/worker_node3) index into it; update the fixture that builds schedulable (the code using cpu_arch and variable schedulable in tests/conftest.py) to fail-fast: after computing schedulable, if cpu_arch is set and schedulable is empty, raise a clear RuntimeError (or pytest.Skip/Fail per project convention) that includes the requested cpu_arch value and the list of available node architectures/names so callers immediately see why no nodes matched instead of hitting later IndexErrors. Ensure the exception is raised before yielding.
Contributor
|
/approve |
geetikakay
reviewed
Jun 2, 2026
|
|
||
|
|
||
| @pytest.fixture(scope="module") | ||
| def multiprocessing_start_method_fork(): |
Contributor
There was a problem hiding this comment.
should we consider moving to threads , something similar to create_pod_deleting_thread
Contributor
|
/retest all Auto-triggered: Files in this PR were modified by merged PR #4985. Overlapping filestests/conftest.py |
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
9 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.
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