Repository navigation
fix: use file lock in virtctl_binary fixture for xdist compatibility - #276
Conversation
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughReplaced per-worker temporary virtctl downloads with a shared, versioned, lock-protected cache coordinated across pytest-xdist workers; fixture signature updated (removed Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant W as Worker (pytest-xdist)
participant L as FileLock (/tmp/.../virtctl-<version>.lock)
participant S as SharedDir (/tmp/.../virtctl-<version>)
participant C as OCP Cluster (download_virtctl_from_cluster)
W->>L: acquire lock (timeout)
alt lock granted
L-->>W: granted
W->>S: check virtctl exists & is executable
alt missing or invalid
W->>C: download & extract virtctl into S
C-->>S: place virtctl binary
end
W->>W: add_to_path(S)
W->>L: release lock
W-->>W: return virtctl path
else timeout
L-->>W: timeout
W-->>W: raise TimeoutError
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
💡 Tips
For more information, please refer to the project documentation or contact the maintainers. |
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
PR Code Suggestions ✨Latest suggestions up to cd353c6
Previous suggestions✅ Suggestions up to commit 93ecb8b
✅ Suggestions up to commit 6390b90
✅ Suggestions up to commit a2bedf8
✅ Suggestions up to commit 288a589
✅ Suggestions up to commit 86991d5
✅ Suggestions up to commit 69690cf
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
…ue-273-virtctl-xdist-lock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 438-443: Add a clarifying comment above the shared_dir definition
explaining that the hard-coded path "/tmp/pytest-shared-virtctl" is
intentionally persistent across test runs for caching and visibility among xdist
workers, and should not be changed to a session-scoped temp directory; reference
the shared_dir variable and related artifacts lock_file and virtctl_path so
future maintainers understand this is deliberate for cross-worker caching and
not a leftover.
- Line 70: Rename the private helper _add_to_path to a public add_to_path:
update the function definition in utilities.virtctl from def _add_to_path(...)
to def add_to_path(...), update all internal references inside utilities.virtctl
to call add_to_path, and change the import in conftest.py from _add_to_path to
add_to_path; ensure any other modules importing the old name are updated and run
tests to confirm there are no remaining references to _add_to_path.
- Around line 446-448: The current existence/executable check around
filelock.FileLock for virtctl_path can reuse a stale binary from a different
cluster version; add version validation so we only accept a binary that matches
the target cluster version. Implement this by querying the cluster version
(using the ocp_admin_client) and comparing it to the virtctl binary version
before returning; either update _check_existing_virtctl to run the
cluster-version vs binary-version comparison and return False on mismatch, or in
conftest.py extract the cluster version and invalidate/delete the binary when it
doesn't match, then call download_virtctl_from_cluster(…) to fetch the correct
version; ensure the comparison uses stable identifiers (e.g., semantic version
or full cluster version string) and consider storing version metadata next to
the binary for quick checks.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
- Use cross-platform tempfile.gettempdir() instead of hardcoded /tmp - Restrict directory permissions with mode=0o700 - Add explicit validation after download to fail fast - Handle filelock.Timeout with descriptive error message
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 428-438: The docstring for the fixture virtctl_binary is missing
the required Args/Returns/Raises sections; update the docstring to include an
Args section describing the ocp_admin_client parameter and its type/role, a
Returns section describing that a Path to the downloaded virtctl binary is
returned, and a Raises section documenting any exceptions that can be raised
(e.g., download or permission errors) during setup; keep the existing
descriptive summary and mention file-lock behavior in the body, using the
function name virtctl_binary and parameter ocp_admin_client to make the contract
explicit.
- Around line 439-445: The shared temp directory creation (shared_dir) can be
hijacked if it already exists with wrong owner or loose permissions; update
conftest.py to after creating shared_dir verify and enforce ownership and
permissions: check that shared_dir.owner/uid matches the current process
(os.getuid()/getpwuid) and fail if not, or chown it to the current uid/gid, and
set mode to 0o700 (use os.chmod) atomically; also ensure lock_file and
virtctl_path are created with safe flags/permissions (open/create with O_EXCL
where possible) and validate their ownership before use; refer to the existing
symbols shared_dir, lock_file, and virtctl_path to locate and modify the logic.
Use PermissionError instead of ValueError for ownership mismatch to better reflect the security nature of the error. Use is_file() instead of exists() to ensure directories don't count as valid executables.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
- Define validation (checking required inputs/config) vs assertions (verifying test outcomes) - Add explicit definition in Validate at Source section - Add cross-reference to Deterministic Tests section in wrong example
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@conftest.py`:
- Around line 456-473: The shared_dir symlink-hijack risk: before calling
shared_dir.stat() or os.chmod(), ensure the path is not a symlink by using a
non-following lstat check (e.g., Path.lstat() or os.lstat(shared_dir)) and fail
if it reports a symlink (or differing inode/owner) so we never follow
attacker-controlled symlinks; keep the existing checks (current_uid, dir_stat)
after confirming no symlink and then proceed to os.chmod(shared_dir, 0o700).
Ensure the guard specifically references shared_dir, uses lstat rather than
stat, and raises a PermissionError if a symlink is detected.
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
💡 Tool usage guide:Overview: The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
See the review usage page for a comprehensive guide on using this tool. |
Add is_symlink() check before stat/chmod operations to prevent symlink hijack attacks on the shared virtctl cache directory. Also change stat() to lstat() for ownership verification to ensure we check the link itself, not the target.
|
Failed to build and push ghcr.io/redhatqe/mtv-api-tests:latest |
User description
Summary
virtctl_binaryfixture to usefilelockwith a shared directoryfilelockdependency topyproject.tomlCloses #273
Test plan
-n auto) and verify no race conditions when downloading virtctlPR Type
Bug fix, Enhancement
Description
Add file locking to
virtctl_binaryfixture for pytest-xdist compatibilityUse shared directory instead of per-worker temporary directories
Prevent race conditions when multiple workers download virtctl simultaneously
Add
filelockdependency to project dependenciesDiagram Walkthrough
File Walkthrough
conftest.py
Implement file locking for virtctl binary downloadconftest.py
filelockmodule for synchronization_add_to_pathutility function from virtctl moduletmp_path_factoryparameter with fixed shared directoryapproach
workers
pyproject.toml
Add filelock dependencypyproject.toml
filelock>=3.16.0as a new project dependencySummary by CodeRabbit
Chores
Bug Fixes
Refactor
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.