Skip to content

Grub Update#2449

Merged
7c6f434c merged 38 commits intoNixOS:masterfrom
wkennington:master.grub
Aug 28, 2014
Merged

Grub Update#2449
7c6f434c merged 38 commits intoNixOS:masterfrom
wkennington:master.grub

Conversation

@wkennington
Copy link
Copy Markdown
Contributor

This patchset brings grub up to the 2.02 beta release. This version of grub now supports most of the features in the zfsonlinux project allowing it to boot directly from root zpools. I also refactored install-grub.pl so that it understands how to search for zpools and btrfs volumes. Additionally a feature was added to allow users to force grub into locating disks by uuid or label.

This still needs extensive testing. I plan to add nixos tests for a variety of different edge case configurations. For now I just wanted to get this out for feedback.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't this only be needed if /boot itself is zfs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes, but when using zfs we cannot tell from the nix expression if /boot is a part of the / dataset or it's own dataset. It was just easier to assume that if they were intending to boot from a root zpool, grub would likely need zfs support. Using grub2 with compiled in zfs support adds no extra dependencies if you already have zfs installed on your machine.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

What's the overhead of zfs support in grub? Can we enable it unconditionally?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It's pretty small as it just needs libzfs. I wouldn't mind enabling this by default but I'll leave it as a configurable option. The biggest issue I suppose is that any breakage to the zfs package (incompatible kernel api) and then you break the ability to build grub. This probably isn't a big deal but something to keep in mind.

shlevy added a commit that referenced this pull request Apr 30, 2014
If /boot is a btrfs subvolume, it will be on a different device than / but not be
at the root from grub's perspective. This should be fixed in a nicer way by #2449,
but that won't make it into 14.04
@jcumming
Copy link
Copy Markdown
Contributor

I have some zfs tests started in NixOS/nixos#246 . Perhaps that could be leveraged.

@wkennington
Copy link
Copy Markdown
Contributor Author

@jcumming Yes, it would be great if we could test full zfs root without separate datasets, /boot and /nix in separate datasets, and then the usual ext2 /boot with a zfs root. Even a zfs dataset /boot with root in a non-top level dataset.

@wkennington
Copy link
Copy Markdown
Contributor Author

As far as btrfs tests are concerned we should have a plain old root with no subvolumes, a root which is set as the default and placed in a subvolume, a separate subvolume for /boot and /nix, and then a combination of default subvolume + /boot on it's own. Again, a separate /boot on ext2 + btrfs root would also be good.

@wkennington wkennington reopened this Apr 30, 2014
shlevy added a commit that referenced this pull request May 1, 2014
If /boot is a btrfs subvolume, it will be on a different device than /
but not be at the root from grub's perspective. This should be fixed in
a nicer way by #2449, but that can't go into 14.04.
shlevy added a commit that referenced this pull request May 1, 2014
If /boot is a btrfs subvolume, it will be on a different device than /
but not be at the root from grub's perspective. This should be fixed in
a nicer way by #2449, but that can't go into 14.04.

(cherry picked from commit e4630c1)
@wkennington
Copy link
Copy Markdown
Contributor Author

I believe this is pretty much in working order. I've added tests for nixos/release.nix within the installer test group. The tests verify that it works in a variety of configurations, using labels, uuids, multiple partition layouts, btrfs plainly, btrfs with subvolumes. I'm only missing zfs tests at this point(maybe related to NixOS/nixos#246). I haven't added these to nixos/release-combined.nix as required dependencies, but this could be added trivially in the future.

@shlevy Could you test if this works for your btrfs setup which relied upon #2456?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think that's a typo.

@7c6f434c
Copy link
Copy Markdown
Member

@shlevy so, do you have any opinion on this?

If the tests still pass, I guess this should be merged after the typo fix…

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Why not just use the backtick operator to run btrfs?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because btrfs doesn't actually return an error string in the cases I need to know it failed, therefore relying on the exit code of the application. In hindsight, I could have used ${^CHILD_ERROR_NATIVE} to grab the return code, I just don't really like the fact that I am retrieving a global variable for the return status.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It was most likely because I didn't want to deal with parsing errors from text vs the real subvolume strings.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Aug 29, 2014

Is this "beta" version considered stable enough to have it as the default grub? I would think it's marked as "beta" for some reason...

EDIT-added: I tend to be careful about GRUB, as it may be difficult to roll back its breakage.

@7c6f434c
Copy link
Copy Markdown
Member

Hm. I used repository-head GRUB for too long, maybe… (without problems)

@wkennington
Copy link
Copy Markdown
Contributor Author

@vcunat Ubuntu 14.04 LTS, Archlinux, and Gentoo all use Grub 2.02-beta2 without issue. I would imagine this is common among almost all distros.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Aug 29, 2014

Hmm, 14.04 has 54 patches on top of that beta [1], which isn't too reassuring, but Arch and Gentoo use it (almost) without patches, so I guess it's probably OK. Latest upstream release is more that two years old.

[1] http://anonscm.debian.org/cgit/pkg-grub/grub.git/tree/debian/patches/series?h=ubuntu/trusty

@lostdj
Copy link
Copy Markdown
Contributor

lostdj commented Aug 30, 2014

Sorry, I'm nixnoob, but how to build this?

# nix-build /opt/nixpkgs/ -A linuxPackages.spl
/nix/store/r5s924wsrq8sb4sgq3g0cdmn1lys4n40-spl-0.6.3-3.12.27

# nix-build /opt/nixpkgs/ -A linuxPackages.zfs
/nix/store/hhmnqymahrjar3q33jn4i3ngz1g998yj-zfs-0.6.3-3.12.27

# nix-build -A grub2_zfs
libgrubkern.a(libgrubkern_a-getroot.o): In function `fini_libzfs':
getroot.c:(.text+0x7dc): undefined reference to `libzfs_fini'
libgrubkern.a(libgrubkern_a-getroot.o): In function `grub_get_libzfs_handle':
getroot.c:(.text+0x7f3): undefined reference to `libzfs_init'
libgrubkern.a(libgrubkern_a-getroot.o): In function `grub_util_find_root_devices_from_poolname':
getroot.c:(.text+0x14d): undefined reference to `zpool_open'
getroot.c:(.text+0x162): undefined reference to `zpool_get_config'
getroot.c:(.text+0x17b): undefined reference to `nvlist_lookup_nvlist'
getroot.c:(.text+0x1b5): undefined reference to `nvlist_lookup_nvlist_array'
getroot.c:(.text+0x234): undefined reference to `nvlist_lookup_nvlist_array'
getroot.c:(.text+0x266): undefined reference to `nvlist_lookup_string'
getroot.c:(.text+0x32f): undefined reference to `zpool_close'
collect2: error: ld returned 1 exit status
make[2]: *** [grub-ofpathname] Error 1
make[2]: Leaving directory `/tmp/nix-build-grub-2.02-beta2.drv-0/grub-2.02~beta2'
make[1]: *** [all-recursive] Error 1
make[1]: Leaving directory `/tmp/nix-build-grub-2.02-beta2.drv-0/grub-2.02~beta2'
make: *** [all] Error 2
builder for `/nix/store/hg7mgy2xmvf5x28j28hhfsjm0lg87rkq-grub-2.02-beta2.drv' failed with exit code 2
error: build of `/nix/store/hg7mgy2xmvf5x28j28hhfsjm0lg87rkq-grub-2.02-beta2.drv' failed

/opt/nixpkgs/ is just a local upstream/master clone, nothing fancy.

@vcunat
Copy link
Copy Markdown
Member

vcunat commented Aug 30, 2014

I reproduce that error in a chrooted build.

@wkennington
Copy link
Copy Markdown
Contributor Author

Yeah, the build is also failing on hydra, let me check it out.

@lostdj
Copy link
Copy Markdown
Contributor

lostdj commented Aug 30, 2014

Also I just found that it's impossible to build grub2 with zfsSupport = false:

environment.systemPackages = with pkgs; [ grub2 ]

->

GRUB2 will be compiled with following components:
With libzfs support: Yes

Now with

nixpkgs.config.packageOverrides = pkgs: { grub2 = pkgs.grub2.override { zfsSupport = false; }; };

it's the same.

But if I modify grub/2.0x.nix like this:

  zfsSupportStr = if zfsSupport then "true" else "false";
  preConfigure =
    ''
       echo "${zfsSupportStr}"


       for i in "tests/util/"*.in
       ... ... ...

Then it builds without ZFS support.

I mean... Wat? :)


Gotta say, it's a very magical package. Been messing with it and even asserts do not work as expected. Or I'm just stupid. Anyhow, placing forced zfsSupport = false in an impl. section solved it.

@aristidb
Copy link
Copy Markdown
Contributor

You clearly didn't think of encrypted volumes when writing this. Sorry if I'm a bit angry, but I'm locked out of my machine because of this.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nope, that is not necessarily true. Searching may fail if it's encrypted, for example!

7c6f434c added a commit that referenced this pull request Aug 31, 2014
This reverts commit 469f22d, reversing
changes made to 0078bc5.

Conflicts:
	nixos/modules/installer/tools/nixos-generate-config.pl
	nixos/modules/system/boot/loader/grub/install-grub.pl
	nixos/release.nix
	nixos/tests/installer.nix

I tried to keep apparently-safe code in conflicts.
@7c6f434c
Copy link
Copy Markdown
Member

Pushed revert

@aristidb
Copy link
Copy Markdown
Contributor

I would recommend simply stripping out the logic that disables the kernel copying if there is a search command. Simply always copy kernels if /boot and / are on a different partition, end of story. The rest of the grub update is probably fine.

@nbp
Copy link
Copy Markdown
Member

nbp commented Aug 31, 2014

copy kernels if /boot and / are on a different partition

You mean /boot, / and /nix/store?

@aristidb
Copy link
Copy Markdown
Contributor

The idea of /nix/store not being on / seems crazy to me:)
Am 31.08.2014 14:24 schrieb "Nicolas B. Pierron" notifications@github.com:

copy kernels if /boot and / are on a different partition

You mean /boot, / and /nix/store?


Reply to this email directly or view it on GitHub
#2449 (comment).

@7c6f434c
Copy link
Copy Markdown
Member

The idea of /nix/store not being on / seems crazy to me:)

Why? My / has /etc/ and /root/, I'd like /nix/ overfull not to affect
my /root.

@aristidb
Copy link
Copy Markdown
Contributor

Isn't the store needed to boot?
Am 31.08.2014 14:46 schrieb "Michael Raskin" notifications@github.com:

The idea of /nix/store not being on / seems crazy to me:)

Why? My / has /etc/ and /root/, I'd like /nix/ overfull not to affect
my /root.


Reply to this email directly or view it on GitHub
#2449 (comment).

@lostdj
Copy link
Copy Markdown
Contributor

lostdj commented Aug 31, 2014

Isn't the store needed to boot?

Not if initrd is copied to /boot.

@7c6f434c
Copy link
Copy Markdown
Member

Isn't the store needed to boot?

So is /tmp and /var/log

I have them all separate and just set neededForBoot=true; in
configuration. Stage 1 mounts them just fine.

@aristidb
Copy link
Copy Markdown
Contributor

Ah, I didn't know about that feature. Nice.

Anyways, the kernel should be copied if it doesn't reside on the same
partition as /boot.

2014-08-31 14:55 GMT+02:00 Michael Raskin notifications@github.com:

Isn't the store needed to boot?

So is /tmp and /var/log

I have them all separate and just set neededForBoot=true; in
configuration. Stage 1 mounts them just fine.


Reply to this email directly or view it on GitHub
#2449 (comment).

lostdj pushed a commit to lostdj/nixpkgs that referenced this pull request Sep 2, 2014
This reverts commit 469f22d, reversing
changes made to 0078bc5.

Conflicts:
	nixos/modules/installer/tools/nixos-generate-config.pl
	nixos/modules/system/boot/loader/grub/install-grub.pl
	nixos/release.nix
	nixos/tests/installer.nix

I tried to keep apparently-safe code in conflicts.

Conflicts:
	nixos/modules/installer/tools/nixos-generate-config.pl
	nixos/modules/system/boot/loader/grub/install-grub.pl
wkennington added a commit to wkennington/nixpkgs that referenced this pull request Sep 2, 2014
….grub""

This reverts commit 94205f5.

Conflicts:
	nixos/modules/system/boot/loader/grub/install-grub.pl
adrianpk added a commit to adrianpk/nixpkgs that referenced this pull request May 31, 2024
If /boot is a btrfs subvolume, it will be on a different device than /
but not be at the root from grub's perspective. This should be fixed in
a nicer way by NixOS#2449, but that can't go into 14.04.

(cherry picked from commit e4630c1)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants