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

[RFC] 'Fixes' for initramfs creation #8796

Closed
wants to merge 1 commit into from

Conversation

cwedgwood
Copy link
Contributor

@cwedgwood cwedgwood commented May 23, 2019


INCOMPLETE*

This 'kinda' works for me.

I'm pushing this out now in the hope someone can help work out what the right approach is in a few places.


Motivation and Context

initframfs creation on Debian systems does not presently work because a critical file is missing

Partially addresses: #7904

Description

RFC right now, changes are for illustrative purposes. It will however build a functional initramfs.

How Has This Been Tested?

Local build testing, inspection of output and limited VM boot testing.

Types of changes

  • 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:

@codecov
Copy link

codecov bot commented May 23, 2019

Codecov Report

Merging #8796 into master will increase coverage by 0.11%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8796      +/-   ##
==========================================
+ Coverage   78.73%   78.84%   +0.11%     
==========================================
  Files         382      382              
  Lines      117812   117809       -3     
==========================================
+ Hits        92758    92889     +131     
+ Misses      25054    24920     -134
Flag Coverage Δ
#kernel 79.32% <ø> (-0.02%) ⬇️
#user 67.8% <ø> (+0.54%) ⬆️

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 841a7a9...6004973. Read the comment docs.

@behlendorf behlendorf added the Status: Work in Progress Not yet ready for general review label May 24, 2019
@rlaager
Copy link
Member

rlaager commented May 27, 2019

Fixing the missing file seems clearly correct. Adding set -e seems reasonable. (Personally, I use set -eu in every shell script I write.) I'm less certain about the "error if missing" approach. Having zpool.cache be required is wrong.

@cwedgwood
Copy link
Contributor Author

@rlaager thanks for the comments:

  • agree set -eu seems better
  • removed zpool.cache

re: "error if missing" - if we don't do that, we build initramfs which don't work; surely an error at such a time as someone can address/fix it it better than finding out after a reboot it's not working?

force-pushed the update, not sure if github noticed...

@cwedgwood
Copy link
Contributor Author

what i pushed earlier wasn't supposed to work, it was for people to eyeball and comment on

however, in trying to make it work cleanly ... well, it's not clear or working

i'm wondering if instead we should just remove contrib/initramfs entirely?

*** INCOMPLETE ***

This is not entirely correct, it works in some cases for me, though
not reliably, it seems I have to ./autogen.sh twice to get the files
in etc/init.d/ created and put in place for the initramfs Makefile.

zfs-functions is required, copy to /etc/zfs/

The zfs hook that copies files will error on critical files.  The
argument for this is we do not want to create a non-functional
initramfs.

Some files are optional, those we do silently ignore.
@rlaager
Copy link
Member

rlaager commented May 29, 2019

  • agree set -eu seems better

If it works with set -u, that's fine. I wasn't suggesting set -u, just saying I use it personally for everything in addition to set -e. So your idea of using set -e is generally reasonable. (Though it's always possible that the initramfs script, or initramfs-tools generally, won't work with set -e without significant changes.)

  • removed zpool.cache

My point was that zpool.cache should not be required. It should be in the optional list.

re: "error if missing" - if we don't do that, we build initramfs which don't work; surely an error at such a time as someone can address/fix it it better than finding out after a reboot it's not working?

Sure. If whatever problem you're catching is definitely fatal on boot, yes, it's better to catch it earlier.

i'm wondering if instead we should just remove contrib/initramfs entirely?

No. This is the initramfs script used by Debian and Ubuntu, at least.

@dreamcat4
Copy link

Can this one get some higher priority attention? Because otherise the missing file causes a kernel panic during boot. And that leads to broken (not bootable) system. Without any prior warnings / error messages to indicate anything was missing during previous building of initramfs.

@cwedgwood
Copy link
Contributor Author

@dreamcat4 IIRC, I couldn't find a robust way to make sure zfs-functions was always present for packaging so this patch is incomplete (the change to add it work, but the file isn't always generated).

@ghfields ghfields mentioned this pull request Jul 31, 2019
12 tasks
@c0d3z3r0
Copy link
Contributor

c0d3z3r0 commented Aug 1, 2019

@dreamcat4 @cwedgwood could you both test, if #9089 solves this for you, please?

@cwedgwood
Copy link
Contributor Author

I'm going to close this in favor of #9089 which is more appropriate.

@cwedgwood cwedgwood closed this Aug 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Work in Progress Not yet ready for general review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants