Skip to content
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

Refresh metadata cache after failed package installation #3348

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

bajertom
Copy link
Collaborator

@bajertom bajertom commented Nov 8, 2024

Fixes #3277

When testing I found out that there is different behavior in dnf5 compared to dnf4, where a mere dnf makecache would be enough, but dnf5 needs the --refresh option as well (or dnf clean metadata or dnf clean expire-cache), see https://bugzilla.redhat.com/show_bug.cgi?id=2324177

Not sure about the Exception being too generic. Should I create a new one, something like InstallationError? And notify the user through logger as to why metadata --refresh is happening?

Pull Request Checklist

  • implement the feature

tmt/steps/prepare/install.py Outdated Show resolved Hide resolved
tmt/steps/prepare/install.py Outdated Show resolved Hide resolved
tmt/steps/prepare/install.py Outdated Show resolved Hide resolved
@bajertom bajertom requested a review from happz November 19, 2024 10:12
@happz happz added area | package managers Changes related to implementations of package managers plugin | install The prepare plugin for installing packages labels Nov 19, 2024
@happz happz modified the milestones: 1.40, 1.39 Nov 19, 2024
except Exception:
# Refresh cache in case of recent but not updated change do repodata
self.guest.package_manager.refresh_metadata()
self._install()
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder whether should capture the first exception as well - this way, the original one is lost, with all its data. How about something like this?

try:
    self._install()

except Exception as exc1:
    # We do not have any special handling for exceptions raised by the following code.
    # Wrapping them with try/except gives us a chance to attach the original exception
    # to whatever the code may raise, and therefore preserve the information attached
    # to the original exception.
    try:
        # Refresh cache in case of recent but not updated change do repodata
        self.guest.package_manager.refresh_metadata()
        self._install()

    except Exception as exc2:
        raise exc2 from ex1

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, that seems like an unnecessary loss of data - better to catch them both. I used your code, if that's alright:)

@bajertom
Copy link
Collaborator Author

/packit test

@bajertom bajertom requested a review from happz November 19, 2024 15:14
@@ -100,6 +100,12 @@ def _extra_options(

return extra_options

def refresh_metadata(self) -> CommandOutput:
script = ShellScript(
f'{self.command.to_script()} update ')
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think there is a command for this in rpm-ostree, you can use dnf update, but rpm-ostree update does not exist

actually you can do rpm-ostree upgrade --check

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just checked on Fedora Silverblue and it does exist:

tom@fedora:~$ rpm -q rpm-ostree
rpm-ostree-2024.8-1.fc41.x86_64
tom@fedora:~$ rpm-ostree update
Receiving metadata objects: 1611/(estimating) 7.2 MB/s 447.6 MB               1614 metadata, 6089 content objects fetched; 446119 KiB transferred in 65 seconds; 883.1 MB content written
Receiving metadata objects: 1611/(estimating) 7.2 MB/s 447.6 MB... done
Staging deployment... done

However it made changes to the system:

Upgraded:
  alsa-lib 1.2.12-2.fc41 -> 1.2.13-3.fc41
  alsa-sof-firmware 2024.09-1.fc41 -> 2024.09.1-1.fc41
....
  vim-data 2:9.1.825-1.fc41 -> 2:9.1.866-1.fc41
  vim-minimal 2:9.1.825-1.fc41 -> 2:9.1.866-1.fc41
Removed:
  perl-Mozilla-CA-20240730-1.fc41.noarch
Added:
  opensc-libs-0.26.0-1.fc41.x86_64
Run "systemctl reboot" to start a reboot

So you're right, rpm-ostree upgrade --check is the way to go.

$ rpm-ostree upgrade --check
1 metadata, 0 content objects fetched; 592 B transferred in 1 seconds; 0 bytes content written
Note: --check and --preview may be unreliable.  See https://github.com/coreos/rpm-ostree/issues/1579
AvailableUpdate:
        Version: 41.20241121.0 (2024-11-21T03:10:35Z)
         Commit: dc7f3ffdfe61b3ff6df6e0f0a69c16a8a783851c1e07cc6d37a25bbb23aae4a7
   GPGSignature: Valid signature by 466CF2D8B60BC3057AA9453ED0622462E99D6AD1
           Diff: 327 upgraded, 10 removed, 1 added

@psss psss modified the milestones: 1.39, 1.40 Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area | package managers Changes related to implementations of package managers plugin | install The prepare plugin for installing packages
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tmt should handle error due to outdated dnf cache
4 participants