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

Fix missing dkms modules after upgrades (try 2) #8216

Merged
merged 1 commit into from Jan 8, 2019

Conversation

Projects
None yet
4 participants
@tonyhutter
Copy link
Member

commented Dec 19, 2018

Motivation and Context

Make dkms upgrades work correctly when upgrading Fedora. This PR is a refactored version of #8160.

Description

If you were upgrading from say, fc28->fc29, on ZFS version X, the RPMs macros would get called like this:

%post X.fc29
   - This is the step where fc29 gets built by dkms.
     As part of the build, dkms automatically removes the previous
     modules before building the new ones.  It then builds the new
     modules.
%preun X.fc28
   - Right before this step, X.fc29 is be built and installed, but
     since it has the same X, it's files get inadvertently removed
     by fc28's uninstall.
%postun X.fc28

This has the unfortunate side effect that the newly built f29 modules get removed by the f28 %preun step, giving you no modules.

This patch updates %preun X.fc28 to see if we're upgrading or uninstalling. If we're uninstalling, then remove our files. If we're upgrading, check to see if were updating to a new version (0.7.11 -> 0.7.12) or upgrading the same version to a new OS (zfs-dkms.0.7.12.fc28 -> zfs-dkms.0.7.12.fc29). If we're upgrading versions then uninstall the old files. If we're upgrading the OS, then do nothing, since we know our old modules are just going to get overwritten when they're rebuilt for the new OS.

Fixes: #6902

How Has This Been Tested?

Did some unit tests using a reproducer similar to (#8089 (comment)). Also did the following:

  1. Installed pristine Fedora 28.
  2. Installed dkms packages with this fix.
  3. Upgraded to Fedora 29.
  4. Verified that dkms modules recompiled and worked on Fedora 29.

Also:

  1. Installed 0.7.12 dkms packages with this fix on Fedora 29.
  2. Installed newest dkms containing a needed fix (dell/dkms#60).
  3. Upgraded to fake "0.7.13" dkms packages correctly.

Types of changes

  • [1] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation (a change to man pages or other documentation)

Checklist:

Fix missing dkms modules after upgrades
If you were upgrading from say, fc28->fc29, on ZFS version X, the RPMs
macros would get called like this:

%post X.fc29
   - This is the step where fc29 gets built by dkms.
     As part of the build, dkms automatically removes the previous
     modules before building the new ones.  It then builds the new
     modules.
%preun X.fc28
   - Right before this step, X.fc29 is be built and installed, but
     since it has the same X, it's files get inadvertently removed
     by fc28's uninstall.
%postun X.fc28

This patch updates %preun X.fc28 to see if we're upgrading or
uninstalling.  If we're uninstalling, then remove our files. If we're
upgrading then do nothing, since will know dkms will have already
removed our files in %post X.fc29.

Note that since this fixes the %preun step, it's effect isn't going
to be noticed immediately.  It will only be seen when packages
with this fix are upgraded to a newer version.

Signed-off-by: Tony Hutter <hutter2@llnl.gov>
Fixes: #6902
@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

This is more complicated than it needs to be. Just use --rpm_safe_upgrade on dkms add and dkms remove .

Cf. https://www.mankier.com/8/dkms#--rpm_safe_upgrade

@codecov

This comment has been minimized.

Copy link

commented Dec 20, 2018

Codecov Report

Merging #8216 into master will decrease coverage by 0.15%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8216      +/-   ##
==========================================
- Coverage   78.56%   78.41%   -0.16%     
==========================================
  Files         379      378       -1     
  Lines      114924   114759     -165     
==========================================
- Hits        90286    89983     -303     
- Misses      24638    24776     +138
Flag Coverage Δ
#kernel 78.73% <ø> (-0.11%) ⬇️
#user 67.32% <ø> (-0.35%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c87db59...9e347b2. Read the comment docs.

@tonyhutter

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

@Conan-Kudo we already use --rpm_safe_upgrade on remove. We don't use dkms add, but rather /usr/lib/dkms/common.postinst to build the modules, since that's what dkms does in their template spec file (https://github.com/dell/dkms/blob/master/template-dkms-mkrpm.spec).

@Conan-Kudo

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

@tonyhutter So then, this is just logic for dealing with updated releases of the same version? When do we ever do that? Aside from distro upgrades, obviously...

@tonyhutter

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

@Conan-Kudo correct. It's for upgrading the same zfs version between OSs (zfs-dkms-0.7.12.fc28 -> zfs-dkms.0.7.12.fc29).

@behlendorf
Copy link
Member

left a comment

The logic here looks sound to me, though I didn't manually verify this fix.

@behlendorf

This comment has been minimized.

Copy link
Member

commented Dec 28, 2018

@Lalufu would you mind reviewing this fix.

@Lalufu

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2018

The logic looks OK to me, but I believe printing stuff in scripts is frowned upon.

@behlendorf

This comment has been minimized.

Copy link
Member

commented Jan 2, 2019

@Lalufu good point. @tonyhutter let's get this refreshed without the extraneous logging.

@tonyhutter tonyhutter force-pushed the tonyhutter:upgrade-fix branch from 652d891 to 9e347b2 Jan 7, 2019

@behlendorf behlendorf merged commit 21e000a into zfsonlinux:master Jan 8, 2019

23 of 24 checks passed

buildbot/Kernel.org Built-in x86_64 (BUILD) Build done.
Details
buildbot/Amazon 2 x86_64 (BUILD) Build done.
Details
buildbot/Amazon 2 x86_64 Release (TEST) Build done.
Details
buildbot/CentOS 6 x86_64 (BUILD) Build done.
Details
buildbot/CentOS 6 x86_64 (TEST) Build done.
Details
buildbot/CentOS 7 x86_64 (BUILD) Build done.
Details
buildbot/CentOS 7 x86_64 (TEST) Build done.
Details
buildbot/Debian 8 arm (BUILD) Build done.
Details
buildbot/Debian 8 ppc (BUILD) Build done.
Details
buildbot/Debian 8 ppc64 (BUILD) Build done.
Details
buildbot/Debian 9 x86_64 (BUILD) Build done.
Details
buildbot/Debian 9 x86_64 (TEST) Build done.
Details
buildbot/Fedora 29 x86_64 (BUILD) Build done.
Details
buildbot/Fedora 29 x86_64 (TEST) Build done.
Details
buildbot/Ubuntu 16.04 aarch64 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 i386 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 x86_64 (BUILD) Build done.
Details
buildbot/Ubuntu 16.04 x86_64 (TEST) Build done.
Details
buildbot/Ubuntu 17.04 x86_64 Coverage (TEST) Build done.
Details
buildbot/Ubuntu 17.10 x86_64 (STYLE) Build done.
Details
buildbot/Ubuntu 18.04 x86_64 (BUILD) Build done.
Details
buildbot/Ubuntu 18.04 x86_64 (TEST) Build done.
Details
codecov/patch Coverage not affected when comparing c87db59...9e347b2
Details
codecov/project 78.41% (-0.16%) compared to c87db59
Details
@Lalufu

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

FTR, I just updated from F29 (running kernel-5.0.9-200.fc29.x86_64) to F30 (running kernel-5.0.9-301.fc30.x86_64), using spl-dkms-0.7.13-1.fc29.noarch and zfs-dkms-0.7.13-1.fc29.noarch, and ended up without SPL and ZFS modules in the F30 system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.