fix: Fix breaking changes in package install #5069
Conversation
| @@ -120,7 +120,9 @@ def handle(name: str, cfg: Config, cloud: Cloud, args: list) -> None: | |||
| try: | |||
| cloud.distro.install_packages(pkglist) | |||
| except Exception as e: | |||
| util.logexc(LOG, "Failed to install packages: %s", pkglist) | |||
| util.logexc( | |||
| LOG, "Failure when attempting to install packages: %s", pkglist | |||
There was a problem hiding this comment.
Not strictly needed, but I made this slightly more passive since the old phrasing sounds like ALL packages failed, whereas that may not always be the case.
Ensure: * Apt can still install packages that contain `-`, `/`, or `=` * If a specified package manager fails we don't retry as a generic package * We do not attempt to invoke a package manager that doesn't exist * Packages that are unable to be installed are tracked and reported appropriately Fixes canonicalGH-5066
| return [ | ||
| pkg | ||
| for pkg in pkglist | ||
| if re.split("/|=|-", pkg)[0] not in self.get_all_packages() |
There was a problem hiding this comment.
The issue you'll have with spitting on hyphen is that many packages contain hyphens, like our own cloud-init :) which will lead to us trying to match cloud instead of cloud-init. It'll have to specifically be an rstrip for the hyphen case... or making sure we only truncate that last character if it's a hyphen.
There was a problem hiding this comment.
I'm +1 on splitting on the / or = as they can't be part of a valid debian package name. But again, the hyphen we need to treat differently once we have that split.
So maybe the following?
| if re.split("/|=|-", pkg)[0] not in self.get_all_packages() | |
| if re.split("/|=", pkg)[0].rstrip("-") not in self.get_all_packages() |
cloudinit/distros/__init__.py
Outdated
| failed = { | ||
| pkg for pkg in uninstalled if pkg not in generic_packages | ||
| failed = manager.install_packages(to_try) | ||
| uninstalled.update(failed) |
There was a problem hiding this comment.
Shouldn't this be the following?
| uninstalled.update(failed) | |
| uninstalled = to_try.difference(failed) |
There was a problem hiding this comment.
I changed the wording from uninstalled to total_failed to hopefully be a little more clear.
Shouldn't this be the following?
No, doing this won't keep track of failures from previous package managers.
Say we have
packages:
- pkg1
- apt:
- pkg2
- snap:
- pkg3If pkg2 failed, it will stay failed regardless of happens during snap install, so overwriting total_failed each time in the loop won't work, because to_try won't contain pkg2 when doing the snap install. On the other hand, if pkg1 fails during apt install, we will still attempt it during snap install, so what happened during apt install should get replaced by what happened during snap. The logic is a bit convoluted, so I'm open to any ideas for simplification.
This is the reason for the new test_failed test in distros/test_init.py
| } | ||
| if failed: | ||
| if unexpected_failed: | ||
| LOG.info(error_message, failed) |
There was a problem hiding this comment.
Should we be logging the unexpected_failed here instead of failed now?
There was a problem hiding this comment.
Hmmm, originally we were logging (at info) all failed packages. I think I want to keep it that way (still requiring a change here) so it shows something didn't install here but may be installed in the future. Let me know if you disagree.
Co-authored-by: Chad Smith <chad.smith@canonical.com>
fix: Fix breaking changes in package install Ensure: * Apt can still install packages that contain `-`, `/`, or `=` * If a specified package manager fails we don't retry as a generic package * We do not attempt to invoke a package manager that doesn't exist * Packages that are unable to be installed are tracked and reported appropriately Fixes canonicalGH-5066
|
Hi ,
but then the packages failed to install . It was working previously with older version of cloud-init that I made in February or early March . So I can easily reproduce it with the following versions :
I got as well the same kind of error |
Proposed Commit Message
Additional Context
#5066
Test Steps
Added additional unit tests
Merge type