Enhancement for set_remote_log function in migration base_step.py and… #6784
Conversation
… revoke utils_sys.display_remote_log We need some default values for set_remote_log function to improve the convenicence, and remove the utils_sys.display_remote_log function Signed-off-by: nanli <[email protected]>
WalkthroughThe changes enhance remote libvirtd logging functionality in the migration workflow. The Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![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
🤖 Fix all issues with AI agents
In `@provider/migration/base_steps.py`:
- Around line 390-398: There's a syntax error in the server_ip default: change
self.params.get("server_ip", self.params.get("migrate_dest_host)")) to
self.params.get("server_ip", self.params.get("migrate_dest_host")) and then
propagate defaults back into self.params so subsequent calls use them; e.g.
after computing
server_ip/server_pwd/server_user/remote_file_type/log_level/log_file/log_filters
assign them into self.params (self.params["server_ip"]=server_ip etc.) so
remote.run_remote_cmd and update_remote_file read the resolved values. Ensure
the corrected keys are used where referenced (server_ip, server_pwd,
server_user, remote_file_type) to avoid missing-key behavior.
- Around line 190-191: The code calls the removed function
utils_sys.display_remote_log inside the if-check
self.params.get_boolean("set_remote_libvirtd_log"); remove that call or replace
it with the new pattern using set_remote_log() — locate the conditional in
base_steps.py and either delete the utils_sys.display_remote_log(...) invocation
or invoke utils_sys.set_remote_log(self.params, self.test) (or the appropriate
set_remote_log signature) so the deprecated function is not referenced.
🧹 Nitpick comments (1)
provider/migration/base_steps.py (1)
412-414: Closeremote_sessionduring cleanup to avoid SSH leaks.The session is stored but never closed in
cleanup_default. Please close it there to avoid leaking SSH connections across tests.♻️ Suggested cleanup
self.test.log.debug("Recover test environment") if set_remote_libvirtd_log and self.remote_libvirtd_log: del self.remote_libvirtd_log + remote_session = self.params.pop("remote_session", None) + if remote_session: + remote_session.close()
| if self.params.get_boolean("set_remote_libvirtd_log"): | ||
| utils_sys.display_remote_log(self.params, self.test) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for display_remote_log definition and imports
rg -n "display_remote_log" --type-listRepository: autotest/tp-libvirt
Length of output: 452
🏁 Script executed:
# Find where display_remote_log is defined and used
rg -n "def display_remote_log|display_remote_log" -A 2Repository: autotest/tp-libvirt
Length of output: 240
🏁 Script executed:
# Check the imports and context in base_steps.py around lines 190-191
head -200 provider/migration/base_steps.py | tail -30Repository: autotest/tp-libvirt
Length of output: 1701
🏁 Script executed:
# Also check imports at the top of the file
head -30 provider/migration/base_steps.pyRepository: autotest/tp-libvirt
Length of output: 813
🏁 Script executed:
# Search for the definition of display_remote_log function
rg -n "def display_remote_log"Repository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Check if utils_sys module exists and search for display_remote_log there
find . -name "utils_sys.py" -o -name "utils_sys" -type dRepository: autotest/tp-libvirt
Length of output: 45
🏁 Script executed:
# Search in virttest for display_remote_log
rg -n "display_remote_log" . --type py -B 2 -A 5Repository: autotest/tp-libvirt
Length of output: 667
🏁 Script executed:
# Check git history for references to display_remote_log removal or changes
git log --all --oneline | head -20Repository: autotest/tp-libvirt
Length of output: 1515
🏁 Script executed:
# Search for any comments or TODOs about display_remote_log
rg -n "display_remote_log|libvirtd_log" -B 3 -A 3 --type pyRepository: autotest/tp-libvirt
Length of output: 50375
🏁 Script executed:
# Check if there's a CHANGES file or documentation about API changes
find . -name "CHANGES*" -o -name "NEWS*" -o -name "README*" | head -5Repository: autotest/tp-libvirt
Length of output: 97
Remove the call to the revoked display_remote_log function at line 191.
The commit 7059b4f51 explicitly revoked utils_sys.display_remote_log, yet line 191 still calls this non-existent function. This will cause an AttributeError at runtime. The enhancement introduced set_remote_log() as the replacement mechanism—if remote log display is needed, use that pattern instead or remove this call entirely.
🤖 Prompt for AI Agents
In `@provider/migration/base_steps.py` around lines 190 - 191, The code calls the
removed function utils_sys.display_remote_log inside the if-check
self.params.get_boolean("set_remote_libvirtd_log"); remove that call or replace
it with the new pattern using set_remote_log() — locate the conditional in
base_steps.py and either delete the utils_sys.display_remote_log(...) invocation
or invoke utils_sys.set_remote_log(self.params, self.test) (or the appropriate
set_remote_log signature) so the deprecated function is not referenced.
| log_level = self.params.get("libvirtd_debug_level", "1") | ||
| log_file = self.params.get("libvirtd_debug_file", "/var/log/libvirt/virtqemud.log") | ||
| log_filters = self.params.get("libvirtd_debug_filters", "1:*") | ||
| remote_file_type = self.params.get("remote_file_type", "virtqemud") | ||
| server_ip = self.params.get("server_ip", self.params.get("migrate_dest_host)")) | ||
| server_user = self.params.get("server_user", "root") | ||
| server_pwd = self.params.get("server_pwd", self.params.get("migrate_dest_pwd")) | ||
|
|
||
| self.test.log.debug(f"Start setting {remote_file_type} log on remote host") |
There was a problem hiding this comment.
Fix the server_ip default syntax error and propagate resolved defaults.
Line 394 has a mismatched quote/paren, which is a SyntaxError. Also, remote.run_remote_cmd and update_remote_file read from self.params, so defaults won’t apply if those keys are missing unless you write them back.
🛠️ Proposed fix
- server_ip = self.params.get("server_ip", self.params.get("migrate_dest_host)"))
- server_user = self.params.get("server_user", "root")
- server_pwd = self.params.get("server_pwd", self.params.get("migrate_dest_pwd"))
+ server_ip = self.params.get("server_ip", self.params.get("migrate_dest_host"))
+ server_user = self.params.get("server_user", "root")
+ server_pwd = self.params.get("server_pwd", self.params.get("migrate_dest_pwd"))
+ # Ensure downstream helpers that read params see the resolved defaults
+ self.params.update({
+ "server_ip": server_ip,
+ "server_user": server_user,
+ "server_pwd": server_pwd,
+ })🤖 Prompt for AI Agents
In `@provider/migration/base_steps.py` around lines 390 - 398, There's a syntax
error in the server_ip default: change self.params.get("server_ip",
self.params.get("migrate_dest_host)")) to self.params.get("server_ip",
self.params.get("migrate_dest_host")) and then propagate defaults back into
self.params so subsequent calls use them; e.g. after computing
server_ip/server_pwd/server_user/remote_file_type/log_level/log_file/log_filters
assign them into self.params (self.params["server_ip"]=server_ip etc.) so
remote.run_remote_cmd and update_remote_file read the resolved values. Ensure
the corrected keys are used where referenced (server_ip, server_pwd,
server_user, remote_file_type) to avoid missing-key behavior.
1 participant
revoke utils_sys.display_remote_log
We need some default values for set_remote_log function to improve the convenicence, and remove the utils_sys.display_remote_log function
Signed-off-by: nanli [email protected]
avocado-framework/avocado-vt#4316
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.