Dpkg tool #4237
Dpkg tool #4237
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request introduces support for installing Debian packages (.deb) on nodes by adding a new Dpkg tool and DEBPackageInstallerTransformer. It also refactors the PackageInstaller transformer base class to support wildcard file selection, improved logging, and better error handling.
Changes:
- Adds a new Dpkg tool for managing Debian package installations
- Introduces DEBPackageInstallerTransformer for Debian-based distributions
- Enhances PackageInstaller base class with wildcard file selection, kernel version logging, and error handling
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
| lisa/tools/dpkg.py | New tool implementation for Debian package management with validation and installation capabilities |
| lisa/transformers/package_installer.py | Refactored base class to add wildcard file selection, kernel version logging, improved error handling; added DEBPackageInstallerTransformer |
| lisa/tools/init.py | Added import for the new Dpkg tool |
Key Test Cases:
smoke_test
Impacted LISA Features:
StartStop, Resize
Tested Azure Marketplace Images:
- canonical 0001-com-ubuntu-server-jammy 22_04-lts-gen2 latest
- debian debian-12 12-gen2 latest
- canonical ubuntu-24_04-lts server latest
lisa/tools/dpkg.py
Outdated
| @@ -0,0 +1,40 @@ | |||
| from typing import cast | |||
There was a problem hiding this comment.
The Dpkg class is missing a copyright header and license notice. All tool files in the LISA project should include the standard copyright header as seen in other tool files.
lisa/tools/__init__.py
Outdated
| from .dnsmasq import Dnsmasq | ||
| from .docker import Docker | ||
| from .docker_compose import DockerCompose | ||
| from .dpkg import Dpkg |
There was a problem hiding this comment.
The 'Dpkg' class is imported but not exported in the all list. Following the pattern of other tools in this file (like 'Rpm' at line 279), 'Dpkg' should be added to the all list in alphabetical order between 'DockerCompose' and 'Echo' for consistency and proper module exports.
lisa/tools/dpkg.py
Outdated
| # Install a single deb package | ||
| parameters = f"-i {file}" | ||
| if force: | ||
| parameters = f" --force-all {parameters}" |
There was a problem hiding this comment.
The parameter order in the force flag handling is inconsistent with the RPM implementation. In the Rpm.install_local_package method, the force flag is appended after the base parameters, but here it's prepended with a leading space. The implementation should use consistent ordering: parameters = f"-i {file}" followed by parameters += " --force-all" if force is True, to match the pattern used in rpm.py.
| parameters = f" --force-all {parameters}" | |
| parameters += " --force-all" |
| if self._runbook_files == ["*"]: | ||
| self._runbook_files = [] | ||
| files = node.tools[Ls].list(str(directory)) | ||
| for file in files: | ||
| self._runbook_files.append(PurePath(file).name) |
There was a problem hiding this comment.
The wildcard file listing functionality with the Ls tool is added to the base PackageInstaller class, which will affect both RPM and DEB transformers. This logic should be tested to ensure it works correctly across different distributions. Additionally, using Ls.list() with a wildcard pattern relies on shell expansion which may behave differently across environments.
| try: | ||
| self._install_package(self._node.get_str_path(directory / file)) | ||
| installed_packages.append(file) | ||
| except Exception as e: | ||
| self._log.error(f"Failed to install {file}: {e}") | ||
| raise e |
There was a problem hiding this comment.
The exception handling in the installation loop catches all exceptions but doesn't provide much value since it immediately re-raises the exception. The error logging is redundant because the exception will already propagate with its own context. Consider either removing the try-except block entirely or handling specific exceptions in a meaningful way.
| try: | ||
| self._node.reboot(time_out=900) | ||
| kernel_version = uname.get_linux_information( | ||
| force_run=True | ||
| ).kernel_version_raw | ||
| self._log.info(f"Kernel version after reboot: " f"{kernel_version}") | ||
| except Exception as e: | ||
| self._log.error("Reboot failed") | ||
| raise e |
There was a problem hiding this comment.
Similar to the installation loop, this exception handling in the reboot block catches all exceptions and immediately re-raises them, providing minimal value. The error message "Reboot failed" is also less informative than the actual exception message. Consider removing the try-except or handling specific reboot-related exceptions.
| try: | |
| self._node.reboot(time_out=900) | |
| kernel_version = uname.get_linux_information( | |
| force_run=True | |
| ).kernel_version_raw | |
| self._log.info(f"Kernel version after reboot: " f"{kernel_version}") | |
| except Exception as e: | |
| self._log.error("Reboot failed") | |
| raise e | |
| self._node.reboot(time_out=900) | |
| kernel_version = uname.get_linux_information( | |
| force_run=True | |
| ).kernel_version_raw | |
| self._log.info(f"Kernel version after reboot: {kernel_version}") |
- Make the `_internal_run` method safer to and output kernel versions if rebooted - Add an `_initialise` method to verify file existence and make it call the `_validate` method. - The `_validate` method has added support for `files: ["*"]`. Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
Signed-off-by: Piyush Sachdeva <[email protected]> Signed-off-by: Piyush Sachdeva <[email protected]>
psachdeva-ms
force-pushed
the
dpkg_tool
branch
from
26372e7 to
a382115
Compare
1 participant
This introduces a new DEBPkgInstaller Transformer to install .deb packages on node.
It uses the newly implemented Dpkg tool to do the install.
There is also a slight Re-design of the package_installer transformer to make for a complete solution to support the above functionality. This also helps in simplification: Instead of using the RPMInstaller Transformer, just the RPMPackageInstaller Transformer can do the entire job, making scaling easy in the future if more similar transformers need to be added.