-
Notifications
You must be signed in to change notification settings - Fork 259
fix server session unbound error #4295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix server session unbound error #4295
Conversation
This patch can be used to run htx mdts stress_type using htxcmdline tool Additionally, this patch fixes DeprecationWarning of invalid escape sequence 'x' W605 This patch introduces the following changes: 1. Allow running of different stress types in different vms based on stress_type_%s parameter if provided in the cfg file, where %s is the name of the vm. This will allow us to introduce additional types of stress in the future. If the stress_type_%s paramter is not provided, then the default stress_type parameter will be used. 2. define a new function load_htxstress_tool() to run htxcmdline tool in the guest. This function will be called whenever stress_type_vmname = htxcmdline. The user can provide the link to the .rpm/.deb package and the function will handle the installation, followed by the running of the htxcmdline tool by using the htxcmdline_args parameter, all provided by user in the cfg file. Signed-off-by: Misbah Anjum N <misanjum@linux.vnet.ibm.com>
Initialize server_session to None at function start to prevent UnboundLocalError when referenced in finally block. The variable was previously only defined within a conditional block, causing an error when the condition was not met. AI assisted code and commit. Human reviewed. Signed-off-by: hholoubk <hholoubk@redhat.com>
WalkthroughThe changes introduce HTX stress tool support and improve code robustness across two files. In Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Pre-merge checks✅ Passed checks (3 passed)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
virttest/utils_sys.pyvirttest/utils_test/__init__.py
🧰 Additional context used
🪛 GitHub Actions: pre-commit
virttest/utils_test/__init__.py
[error] 1-1: pre-commit hook 'black' reformatted virttest/utils_test/init.py and exited with code 1. Command: 'pre-commit run --show-diff-on-failure --color=always --all-files'. Re-run to apply formatting changes.
🪛 Ruff (0.14.10)
virttest/utils_test/__init__.py
2418-2418: Avoid specifying long messages outside the exception class
(TRY003)
2442-2442: Avoid specifying long messages outside the exception class
(TRY003)
🔇 Additional comments (6)
virttest/utils_sys.py (1)
150-150: LGTM!This correctly prevents the
UnboundLocalErrorby ensuringserver_sessionis always defined before thetryblock. Thefinallyblock's checkif server_session:will safely evaluate toFalsewhenserver_sessionisNone, preventing both the error and unnecessary close attempts.virttest/utils_test/__init__.py (5)
483-486: LGTM!Converting these regex patterns to raw strings is the correct approach to avoid potential issues with backslash escape sequences.
876-876: LGTM!Raw string usage for regex patterns is appropriate.
Also applies to: 896-896
1507-1510: LGTM!Raw string conversions for regex patterns are correct and consistent.
Also applies to: 2066-2066, 2083-2083
2834-2877: Logic looks correct, but verify integration withload_htxstress_tool.The per-VM stress type customization and the branching logic for HTX are correctly implemented. The code appropriately preserves the original
stress_typeindefault_stress_typebefore the loop modifications.However, the reliability of this code path depends on fixing the error handling issues in
load_htxstress_toolmentioned above.
2682-2683: LGTM!The per-VM stress type configuration allows flexibility in running different stress tools on different VMs. The value is properly passed to the parent class where normalization occurs.
| self.stress_wait_for_timeout = int( | ||
| self.params.get("stress_wait_for_timeout", 60) | ||
| ) | ||
| stress_type = stress_type.split('_')[0] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pipeline failure: Black formatting required.
The pre-commit hook indicates this file needs reformatting. The single quotes here may need to be double quotes for consistency with the codebase style.
- stress_type = stress_type.split('_')[0]
+ stress_type = stress_type.split("_")[0]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| stress_type = stress_type.split('_')[0] | |
| stress_type = stress_type.split("_")[0] |
🤖 Prompt for AI Agents
In virttest/utils_test/__init__.py around line 2330, the string literal uses
single quotes which fails the project's Black/formatting/pre-commit checks;
replace single quotes with double quotes for that literal (change 'stress_type'
usage to use double-quoted string) and re-run Black or pre-commit formatting to
ensure the file is properly reformatted.
| LOG.info("Installing htx rpm/deb in the guest") | ||
| self.cmd(remove_old_package) | ||
| self.cmd(get_stress) | ||
| self.cmd(install_stress) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for critical installation steps.
The wget and package installation commands are executed without checking their return status. Since self.cmd uses ignore_status=True / ignore_all_errors=True, failures will be silently ignored. If the download or installation fails, the function will proceed to start a stress tool that wasn't installed, resulting in a confusing failure later.
🔎 Proposed fix with error handling
LOG.info("Installing htx rpm/deb in the guest")
- self.cmd(remove_old_package)
- self.cmd(get_stress)
- self.cmd(install_stress)
+ self.cmd(remove_old_package)
+
+ status, output = self.cmd_status_output(get_stress, timeout=self.stress_shell_timeout)
+ if status != 0:
+ raise exceptions.TestError("Failed to download HTX stress tool: %s" % output)
+
+ status, output = self.cmd_status_output(install_stress, timeout=self.stress_shell_timeout)
+ if status != 0:
+ raise exceptions.TestError("Failed to install HTX stress tool: %s" % output)Committable suggestion skipped: line range outside the PR's diff.
Initialize server_session to None at function start to prevent
UnboundLocalError when referenced in finally block. The variable
was previously only defined within a conditional block, causing
an error when the condition was not met.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.