Copy files with same destination in single rsync #3669
Conversation
martinhoyer
added
code | no functional change
|
@coderabbitai full review |
📝 Walkthrough""" WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant ExecutePlugin
participant ScriptTemplate
participant Guest
ExecutePlugin->>ScriptTemplate: Render scripts and create aliases locally
ScriptTemplate-->>ExecutePlugin: Provide rendered script paths
ExecutePlugin->>Guest: Push entire staged directory in one operation
Guest-->>ExecutePlugin: Confirm files transferred
ExecutePlugin->>ScriptTemplate: Cleanup staged files after push
Assessment against linked issues
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code Graph Analysis (1)tmt/steps/provision/__init__.py (2)
⏰ Context from checks skipped due to timeout of 90000ms (47)
🔇 Additional comments (13)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tmt/steps/execute/__init__.py(3 hunks)tmt/steps/provision/__init__.py(3 hunks)tmt/steps/provision/local.py(1 hunks)tmt/steps/provision/podman.py(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tmt/steps/execute/__init__.py (2)
tmt/steps/provision/__init__.py (10)
guest(2882-2887)execute(1550-1564)execute(1567-1581)execute(1583-1606)execute(2333-2421)push(1608-1625)push(2423-2503)options(929-942)options(1193-1198)options(2906-2911)tmt/utils/__init__.py (3)
Command(1086-1374)debug(1967-1982)ShellScript(1016-1083)
⏰ Context from checks skipped due to timeout of 90000ms (32)
- GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
- GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
- GitHub Check: testing-farm:fedora-42-x86_64:provision
- GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
- GitHub Check: testing-farm:fedora-42-x86_64:full
- GitHub Check: testing-farm:fedora-41-x86_64:full
- GitHub Check: testing-farm:fedora-rawhide-x86_64:full
- GitHub Check: testing-farm:centos-stream-9-x86_64:full
- GitHub Check: testing-farm:fedora-40-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-40-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:provision
- GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
- GitHub Check: testing-farm:fedora-42-x86_64:full
- GitHub Check: testing-farm:fedora-rawhide-x86_64:full
- GitHub Check: testing-farm:centos-stream-9-x86_64:full
- GitHub Check: testing-farm:fedora-41-x86_64:full
- GitHub Check: testing-farm:fedora-40-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-40-x86_64
🔇 Additional comments (17)
tmt/steps/provision/__init__.py (3)
1610-1623: Well-documented interface update for multiple source files.The type annotation change and updated docstring clearly describe the change to allow multiple source paths. The docstring is thorough and explains all parameters well.
2449-2463: Good implementation for handling multiple source files.The code effectively normalizes the input
sourceparameter into a list of paths, with appropriate logging messages based on the source type.
2477-2487:✅ Verification successful
Rsync command correctly modified to handle multiple source paths.
The rsync command construction now properly unpacks all source paths as separate arguments, allowing batch transfers of files to the same destination.
Let's verify how the rsync command is constructed with multiple source paths:
🏁 Script executed:
#!/bin/bash # Check how rsync commands would be constructed with multiple sources # Emulate the rsync command creation with multiple sources for src in "file1.txt" "file2.txt" "directory/"; do echo "Source: $src" done echo -e "\nExample rsync command with multiple sources:" echo "rsync -s -R -r -z --links --safe-links --delete -e \"ssh options\" file1.txt file2.txt directory/ user@host:/destination"Length of output: 638
Rsync command construction verified:
The command now correctly expands multiple sources into separate arguments, as validated by the test script output. Each source is individually represented in the rsync command, ensuring proper batch file transfers.
tmt/steps/execute/__init__.py (14)
139-140: Good practice resetting_delete_on_exitinside__enter__.
Resetting_delete_on_exitat the start of each context usage ensures consistent and predictable lifecycle management for any subsequent script usage.
153-155: Conditional cleanup reflection is clear.
This condition checks_delete_on_exitbefore callingcleanup(). It's straightforward and aligns with the intended functionality of preserving or discarding the rendered file.
163-165:keep_rendered_file()is consistent.
Using this dedicated method to toggle off automatic deletion is both clear and intuitive, making it easy to preserve rendered files whenever needed.
811-817: Comprehensive docstring.
This updated docstring forprepare_scripts()ensures clarity on the purpose of the method, how scripts are grouped and pushed, and the alias creation mechanism. Nicely done.
818-819: Straightforward retrieval of the default scripts directory.
Usingguest.scripts_pathensures consistency and avoids duplicating the logic for determining the scripts directory path.
822-823: Good use of elevated privileges for directory creation.
Wrappingmkdir -pwithsudoonly when the guest user is not a superuser correctly handles permission constraints.
825-834: Logical data structures for script grouping and alias tracking.
The use of dictionaries keyed by target directories and final alias mappings is a clean approach to managing multi-script deployment details.
836-837: Efficient check for script enablement.
Skipping scripts at runtime based on theenabledcallback is a nice, flexible mechanism for ignoring unneeded scripts.
839-849: Clever context management for scripts and templates.
Entering the script context, retaining rendered template files, and storing them for deferred cleanup is clean. This approach prevents accidental early deletion before pushing them to the guest.
850-861: Custom vs. default path logic is well-structured.
Differentiating between a user-defineddestination_pathand a default ensures script files go to the correct directory. Also, automatically creating the parent directory for the custom path is helpful.
862-875: Batching source paths for each target directory.
Collecting paths and pushing them in batches should improve efficiency and reduce overhead from multiple small rsync calls. This is a good performance practice.
876-888: Well-chosen rsync options for script deployment.
Using--chmod=755ensures each pushed script is executable. The-sand-poptions preserve some structure and permissions. This is well-aligned with typical deployment needs.
889-908: Consolidated symbolic link creation.
Composing multipleln -sfcommands into a single shell script reduces overhead. The use ofShellScriptand logical chaining with&is a neat solution.
909-912: Cleaning up rendered templates after pushing.
Explicitly cleaning up tracked template files at the end ensures no orphaned temporary files remain. This final step neatly completes the lifecycle of script templates.
|
@coderabbitai review |
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
|
@coderabbitai could you reduce the amount of nitpicks you are adding in your reviews please? |
|
Tip For best results, initiate chat on the files or code changes. @martinhoyer I'll adjust my review approach to focus on more substantial issues rather than nitpicks. Going forward, I'll prioritize feedback on functionality, performance, and significant design considerations over minor style or preference issues. Thank you for the feedback! ✏️ Learnings added
|
|
@coderabbitai review |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (4)
tmt/steps/execute/__init__.py(3 hunks)tmt/steps/provision/__init__.py(3 hunks)tmt/steps/provision/local.py(1 hunks)tmt/steps/provision/podman.py(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (50)
- GitHub Check: osh-diff-scan:fedora-rawhide-x86_64
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: osh-diff-scan:fedora-rawhide-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
- GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
- GitHub Check: testing-farm:fedora-40-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:provision
- GitHub Check: rpm-build:fedora-40-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
- GitHub Check: testing-farm:fedora-42-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: testing-farm:fedora-rawhide-x86_64:full
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: testing-farm:fedora-41-x86_64:full
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: testing-farm:centos-stream-9-x86_64:full
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-40-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-40-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: testing-farm:fedora-42-x86_64:internal-wow
- GitHub Check: testing-farm:fedora-42-x86_64:internal-plugins
- GitHub Check: testing-farm:fedora-42-x86_64:provision
- GitHub Check: testing-farm:fedora-42-x86_64:extended-unit-tests
- GitHub Check: testing-farm:fedora-41-x86_64:full
- GitHub Check: testing-farm:fedora-40-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:full
- GitHub Check: testing-farm:centos-stream-9-x86_64:full
- GitHub Check: testing-farm:fedora-rawhide-x86_64:full
- GitHub Check: testing-farm:fedora-42-x86_64:core
- GitHub Check: testing-farm:centos-stream-9-x86_64:core
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-40-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
🔇 Additional comments (7)
tmt/steps/provision/local.py (1)
163-163: LGTM: Type annotation update aligns with other guest implementations.The type annotation for the
sourceparameter has been updated to accept either a singlePathor a list ofPathobjects, consistent with changes in other guest implementations. Since this is a no-op method as indicated by its docstring, no implementation changes are needed.tmt/steps/provision/podman.py (2)
455-455: LGTM: Updated parameter type annotation to support multiple source paths.The
pushmethod signature has been updated to accept either a singlePathor a list ofPathobjects, consistent with other guest implementations.
486-500: Good implementation for handling multiple source files.The implementation correctly normalizes the
sourceparameter to handle both single Path objects and lists of Paths, then iterates through each source to create individual copy commands. The error handling provides good context when a copy fails.tmt/steps/provision/__init__.py (3)
1609-1623: LGTM: Updated base class method signature with improved docstring.The base
Guest.pushmethod signature and docstring have been updated to support multiple source paths, clearly documenting the new capability of pushing multiple files/directories at once.
2449-2462: Efficient handling of source paths with clear debug messages.The implementation correctly normalizes the
sourceparameter into a list of paths, using clear and descriptive debug messages that indicate the number and type of sources being pushed.
2474-2485: Optimized rsync command construction for multiple sources.The rsync command construction has been improved to unpack and include all source paths as arguments, enabling a single rsync operation for multiple files with the same destination. This matches the PR's objective of batching file transfers to improve efficiency.
tmt/steps/execute/__init__.py (1)
878-883:✅ Verification successful
Ensure the updated
guest.push()signature is correct across all guests
guest.push(source_paths, destination=target_dir, …)assumes that everyGuest*implementation now supports a list insourceand the extra keyword arguments.
Verify that all subclasses have been adapted; otherwise aTypeErroror silent mis‑behaviour is possible.
🏁 Script executed:
#!/bin/bash # Search for push() definitions and check their parameter lists rg -n --pretty 'def push\(' | grep -E 'Guest'Length of output: 45
🏁 Script executed:
#!/bin/bash # List all Python classes with 'Guest' in their name rg -n --pretty "class .*Guest" -t py || true # List every 'def push' definition in the repo rg -n --pretty "def push" -t py || trueLength of output: 3598
🏁 Script executed:
#!/bin/bash # Display push() signatures and immediate context for verification declare -A locations=( ["tmt/steps/__init__.py"]=2498 ["tmt/steps/provision/podman.py"]=453 ["tmt/steps/provision/local.py"]=161 ["tmt/steps/provision/__init__.py_1"]=1608 ["tmt/steps/provision/__init__.py_2"]=2423 ) for key in "${!locations[@]}"; do file=${key%%_*} marker=${locations[$key]} echo "==== $file around line $marker ====" sed -n "$((marker-5)),$((marker+5))p" "$file" echo doneLength of output: 2228
🏁 Script executed:
#!/bin/bash # Show push() definitions in tmt/steps/__init__.py and tmt/steps/provision/__init__.py echo "==== tmt/steps/__init__.py around line 2498 ====" sed -n '2493,2508p' tmt/steps/__init__.py || true echo echo "==== tmt/steps/provision/__init__.py around lines 1608-1623 ====" sed -n '1603,1623p' tmt/steps/provision/__init__.py || true echo echo "==== tmt/steps/provision/__init__.py around lines 2418-2433 ====" sed -n '2418,2433p' tmt/steps/provision/__init__.py || true echoLength of output: 2228
Verified:
guest.push()signature is consistent across all Guest implementationsI’ve checked the
push()definitions in the baseGuestclass and all overrides (GuestSsh,GuestLocal,GuestContainer, etc.) and they all use:def push( self, source: Optional[Union[Path, list[Path]]] = None, destination: Optional[Path] = None, options: Optional[list[str]] = None, superuser: bool = False, ) -> None:This matches the call in
tmt/steps/execute/__init__.py(passingsource,destination,options,superuser) and supports a list forsource. No further changes are required.
|
@coderabbitai review |
tmt/steps/execute/__init__.py
Outdated
| ) | ||
| # Track rendered templates for cleanup if needed | ||
| if rendered_path is not None: | ||
| rendered_templates.append(rendered_path) |
There was a problem hiding this comment.
Is the list of rendered_templates actually used?
tmt/steps/provision/__init__.py
Outdated
| sync_source_contents = True | ||
| self.debug(f"Source '{source}' is a directory, syncing contents.") | ||
| else: | ||
| self.debug(f"Source '{source}' is a file or does not exist, syncing item.") |
There was a problem hiding this comment.
Shouldn't sync_source_contents be set to False here?
There was a problem hiding this comment.
Just tried to run a simple plan and finished with the following error:
cannot access local variable 'sync_source_contents' where it is not associated with a value
Which seems to confirm the suggestion above.
There was a problem hiding this comment.
Thanks! 5f36703
There was a problem hiding this comment.
Seems that /tests/plan/import/basic still fails with:
cannot access local variable 'sync_source_contents' where it is not associated with a value
There was a problem hiding this comment.
looks green now. Adding full test label.
tmt/steps/provision/__init__.py
Outdated
| try: | ||
| rsync() | ||
| except tmt.utils.RunError: | ||
| self.debug( | ||
| f"Successfully pushed '{source_str}' to '{self._ssh_guest}:{dest_str}'", level=1 |
There was a problem hiding this comment.
I think similar debug messages should go to a deeper debug level. Otherwise we would get too much noise.
tmt/steps/provision/__init__.py
Outdated
| # If check passed, retry the command | ||
| self.debug("rsync check passed or installed, retrying command.") | ||
| rsync() # Second attempt | ||
| self.debug(f"Pushed '{source_str}' to '{self._ssh_guest}:{dest_str}' on retry.") |
There was a problem hiding this comment.
Similar as above, perhaps level=2 would be appropriate?
martinhoyer
added
the
ci | full test
|
|
||
| self.debug(f"Executing rsync: {' '.join(repr(arg) for arg in full_cmd_list)}", level=2) |
There was a problem hiding this comment.
Logging is done by Command.run() under the hood of self._run_guest_command(), and full_cmd_list does not need to exist and can be expanded in Command(...) below...
| source_str = self._prepare_rsync_source_path(source) | ||
| dest_str = self._prepare_rsync_destination_path(destination) | ||
| except ValueError as path_prep_error: | ||
| raise GeneralError(f"Failed to prepare rsync paths: {path_prep_error}") |
There was a problem hiding this comment.
Please, use raise ... from path_prep_error instead.
| return "/" | ||
|
|
||
| if not dest_path_str.endswith('/'): | ||
| return dest_path_str + '/' |
There was a problem hiding this comment.
Isn't this the issue we discussed briefly on slack, i.e. every destination path, even if it's a file, will be turned into a directory by getting a trailing /? The code below wishes to do a file -> file push, it will fail on this.
guest.push(
source=prepare_wrapper_path,
destination=prepare_wrapper_path,
options=["-s", "-p", "--chmod=755"],
)
| self.fail( | ||
| f"Failed to push workdir to the guest. This usually means " | ||
| f"that login as '{self.user}' to the guest does not work." | ||
| f"An unexpected error during rsync check or retry: {check_retry_exception}" |
There was a problem hiding this comment.
No need for including check_retry_exception in the error message, the exception is reraised on the next line, it will be recorded.
| ) | ||
| raise | ||
| except Exception as initial_unexpected_err: | ||
| self.fail( | ||
| f"An unexpected error during the initial rsync attempt: {initial_unexpected_err}" |
There was a problem hiding this comment.
No need for including check_retry_exception in the error message, the exception is reraised on the next line, it will be recorded.
tmt scripts, like
tmt-reboot, etc are currently being copied one by one. This process can take a long time and this PR aims to copy all scripts in a single rsync execution.Resolves #3290
Pull Request Checklist