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

Search and replace GUID in initramfs script #8356

Merged
merged 1 commit into from
Jun 6, 2019

Conversation

ghfields
Copy link
Contributor

@ghfields ghfields commented Jan 29, 2019

Motivation and Context

Fix a mailing list reported regression (http://list.zfsonlinux.org/pipermail/zfs-discuss/2019-January/033049.html) following #8052 for a user who kept root pool data in the root dataset.

Description

After #8052, ZFS_BOOTFS and ZFS_RPOOL values are reconstructed following a sed command that ensures $pool is the pool name, not a GUID. This broke a user's root pool setup, who used the zfs root dataset as their root. After #8052, initramfs attempted to mount rpool/rpool, instead of just rpool. This is corrected by doing a search and replace for only the GUID value within ZFS_BOOTFS and ZFS_RPOOL instead, leaving everything else alone. As mailing responses indicate, this should not be considered a best practice for root pool layout, however it is valid and did work in the past.

How Has This Been Tested?

Installed Ubuntu 18.04.1 to root of dataset
-- Note: Ubuntu's update-grub added a trailing "/" that needed to be manually removed from kernel line
System booted as expected.
Edited the grub.cfg and replaced kernel line pool name with pool GUID
System booted as expected.
Checked for regressions by moving data from root dataset to "rpool/ROOT/ubuntu" and set the kernel line appropriately.
System booted as expected.
Edited the grub.cfg kernel line to "<GUID>/ROOT/ubuntu"
System booted as expected.

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:

@Conan-Kudo
Copy link
Contributor

@ghfields Do we need this change for dracut too? People can and do use dracut on Debian/Ubuntu, and it's the initramfs system used on all other major distributions...

@codecov
Copy link

codecov bot commented Jan 30, 2019

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8356      +/-   ##
==========================================
- Coverage   78.77%   78.65%   -0.13%     
==========================================
  Files         382      380       -2     
  Lines      117770   115731    -2039     
==========================================
- Hits        92775    91028    -1747     
+ Misses      24995    24703     -292
Flag Coverage Δ
#kernel 79.06% <ø> (-0.28%) ⬇️
#user 67.53% <ø> (+0.43%) ⬆️

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 2531ce3...e446497. Read the comment docs.

@ghfields
Copy link
Contributor Author

@ghfields Do we need this change for dracut too? People can and do use dracut on Debian/Ubuntu, and it's the initramfs system used on all other major distributions...

I'm unsure of the Dracut status. Not sure if it imports by GUID or if a root dataset root pool confuses its boot. I've seen initramfs and dracut changes happening in separate pull requests. I'll make sure dracut maintainers are informed about this so they can maintain parity.

@loli10K loli10K added the Status: Code Review Needed Ready for review and testing label Jan 31, 2019
@gmelikov gmelikov self-requested a review February 9, 2019 11:52
@gmelikov
Copy link
Member

gmelikov commented Feb 9, 2019

@Conan-Kudo the initial change with regression was only applied to initramfs script, so I don't think we need to change dracut (but I didn't test it).

@behlendorf behlendorf added Status: Revision Needed Changes are required for the PR to be accepted and removed Status: Code Review Needed Ready for review and testing labels Apr 19, 2019
@behlendorf
Copy link
Contributor

behlendorf commented Apr 19, 2019

Can we get this PR refreshed to address the review feedback.

Signed-off-by: Garrett Fields <ghfields@gmail.com>
Copy link
Member

@gmelikov gmelikov left a comment

Choose a reason for hiding this comment

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

LGTM

@gmelikov gmelikov added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels May 31, 2019
@ghfields
Copy link
Contributor Author

ghfields commented Jun 6, 2019

@rlaager Could you review this one liner bugfix?

Copy link
Member

@rlaager rlaager left a comment

Choose a reason for hiding this comment

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

LGTM

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jun 6, 2019
@behlendorf behlendorf merged commit 627f511 into openzfs:master Jun 6, 2019
behlendorf pushed a commit that referenced this pull request Jun 7, 2019
Reviewed-by: George Melikov <mail@gmelikov.ru>
Reviewed-by: Richard Laager <rlaager@wiktel.com>
Signed-off-by: Garrett Fields <ghfields@gmail.com>
Closes #8356
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants