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

Use zfs-import.target in contrib/dracut #6964

Merged
merged 1 commit into from
Jul 31, 2018

Conversation

aerusso
Copy link
Contributor

@aerusso aerusso commented Dec 14, 2017

Description

The new zfs-import.target should be used in place of the zfs-import-*.service units in contrib/dracut.

Motivation and Context

PR #6764 added zfs-import.target to simplify dependency on pool importing. #6822 did some cleanup. The recent #6955 (re: #6953) added RPM support for enabling this units. That bug report has prompted me to grep the code base for zfs-import. The last remaining code section to be updated is under control/dracut/90zfs.

This PR is a work in progress. I don't think dracut users are exposed to any bug presently, because sysroot.mount is still ordered After=zfs-import-*.service

Two files are affected:

  1. zfs-generator.sh.in is straightforwardly modified to order sysroot.mount After=zfs-import.target (instead of each zfs-import-*.service).
  2. module-setup.sh.in is also modified. I need input, because I don't know how precisely dracut works. zfs-import.target (and each zfs-import-*.service) is dracut_install-ed (and unconditionally mark_hostonly-ed). Do we need to build a zfs-import.target.wants directory with zfs-import-*.service links? Or will that be inherited from the host system?

How Has This Been Tested?

This has NOT been tested. This is a place to centralize discussion about these changes.

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:

  • My code follows the ZFS on Linux code style requirements.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • All commit messages are properly formatted and contain Signed-off-by.
  • Change has been approved by a ZFS on Linux member.

@loli10K loli10K added the Status: Work in Progress Not yet ready for general review label Dec 15, 2017
@behlendorf behlendorf added the Component: Dracut dracut integration label Dec 18, 2017
@behlendorf
Copy link
Contributor

Let's get @prometheanfire's and @Rudd-O thoughts on this if possible.

@prometheanfire
Copy link
Contributor

I'm no dracut expert either (I'm having trouble figuring out where/how to enable decryption support). This PR does look straight forward though.

@prometheanfire
Copy link
Contributor

#6982 is the issue I just created for not being able to boot from an ecrypted rootfs on a fully encrypted pool.

@Rudd-O
Copy link
Contributor

Rudd-O commented Dec 20, 2017

You must build the .wants links for sure, aside from adding it in the [Install] section. Remember that all the [Install] section does is create links when you systemctl enable a unit.

@Rudd-O
Copy link
Contributor

Rudd-O commented Dec 20, 2017

Question to everyone involved in this PR: why mark_hostonly?

@aerusso
Copy link
Contributor Author

aerusso commented Dec 20, 2017

@Rudd-O So, you saying that we need to make zfs-import.target.wants/zfs-import-*.service links?

(As an aside, why doesn't Dracut have some mechanism to automate this dependency resolution? This procedure seems really error prone)

####Re: mark_hostonly
It's in the new patch only because it was in the old code base. I also do not understand the conditional logic that I removed.

@behlendorf
Copy link
Contributor

@aerusso how would you like to proceed with this PR?

@aerusso
Copy link
Contributor Author

aerusso commented Jun 5, 2018

@behlendorf I was hoping to move over to a ZFS on root setup on a machine so I could validate this myself, but I haven't had the time. My only hesitation is that lack of testing---and that I'm completely unsure of the significance of type mark_hostonly at this point.

I'm not actually sure anyone has booted with this patch, so I'd really like to get someone to do that first.

@Rudd-O
Copy link
Contributor

Rudd-O commented Jun 5, 2018 via email

@aerusso
Copy link
Contributor Author

aerusso commented Jul 15, 2018

This revision includes @Rudd-O's comment about the wants links needed. This one has been tested booting a Debian VM. It should represent the minimal change to the boot logic to use zfs-import.target.

Re: mark_hostonly

From the Gentoo wiki, hostonly

[should] only include the tools and modules used by the current rootfs file system

But, from man dracut

Boot parameters
       An initramfs generated without the "hostonly" mode, does not contain any
       system configuration files (except for some special exceptions), so the
       configuration has to be done on the kernel command line. With this
       flexibility, you can easily boot from a changed root partition, without

Maybe it's because we are including /etc/zfs/zpool.cache? And then the scan somehow got caught up accidentally (and now zfs-import.target is too)? I'm guessing we should only mark_hostonly the zfs-import-cache.service entries.

Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

I haven't manually tested this change, but functionally it looks correct to me.

@behlendorf
Copy link
Contributor

@aerusso can you rebase this to resolve the new conflict, then let's go ahead and integrate it.

@behlendorf behlendorf added Reviewed and removed Status: Work in Progress Not yet ready for general review labels Jul 30, 2018
The new zfs-import.target should be used in place of the
zfs-import-*.service units.

Signed-off-by: Antonio Russo <antonio.e.russo@gmail.com>
@aerusso aerusso force-pushed the pulls/dracut-zfs-import-target branch from 2e4ffa4 to ec055ae Compare July 31, 2018 02:17
@codecov
Copy link

codecov bot commented Jul 31, 2018

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #6964      +/-   ##
==========================================
- Coverage   78.24%   71.74%   -6.51%     
==========================================
  Files         368      342      -26     
  Lines      112142   110104    -2038     
==========================================
- Hits        87743    78991    -8752     
- Misses      24399    31113    +6714
Flag Coverage Δ
#kernel 58.53% <ø> (-20.23%) ⬇️
#user 63.04% <ø> (-4.25%) ⬇️

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 dae3e9e...ec055ae. Read the comment docs.

@behlendorf behlendorf added the Status: Accepted Ready to integrate (reviewed, tested) label Jul 31, 2018
@behlendorf behlendorf merged commit 9b9d1ad into openzfs:master Jul 31, 2018
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Dracut dracut integration Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants