Bug 253445 - bectl: does not function in two-level zfs datasets
Summary: bectl: does not function in two-level zfs datasets
Status: In Progress
Alias: None
Product: Base System
Classification: Unclassified
Component: bin (show other bugs)
Version: 12.2-RELEASE
Hardware: Any Any
: --- Affects Only Me
Assignee: Kyle Evans
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-02-11 21:35 UTC by dougs@dawnsign.com
Modified: 2023-01-02 21:04 UTC (History)
7 users (show)

See Also:


Attachments
diff to allow the pool dataset to be the BE root when specified (1.09 KB, patch)
2021-02-11 23:31 UTC, Wes Maag
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description dougs@dawnsign.com 2021-02-11 21:35:31 UTC
I used mfsbsd to set up default datasets. If using non-raid, by default, mfsbsd formats its zfs datasets for a single disk as follows:

Filesystem 1K-blocks Used Avail Capacity Mounted on
tank/root 302305222 9711156 292594065 3% /
devfs 1 1 0 100% /dev
tank/root/tmp 292594102 37 292594065 0% /tmp
tank/root/var 292924432 330366 292594065 0% /var

When I attempt to list bootable environments, it comes back empty.

[root@squid 11.Feb 12:20pm /usr]# bectl list
[root@squid 11.Feb 12:20pm /usr]#

bectl check does not return anything:

[root@squid 11.Feb 12:20pm /usr]# bectl check
[root@squid 11.Feb 12:20pm /usr]#

I would like to implement boot environments using two level datasets.

Is the three level datasets a requirement for using bectl?

See https://forums.freebsd.org/threads/boot-environments-with-non-standard-dataset-layout-and-bectl-beadm.74925/ for additional details.
Comment 1 Wes Maag 2021-02-11 22:12:05 UTC
(In reply to dougs@dawnsign.com from comment #0)

Out of curiosity, what happens if you specify the root as tank?

i.e bectl -r tank list?
Comment 2 dougs@dawnsign.com 2021-02-11 22:37:27 UTC
[root@squid 11.Feb 12:32pm /]# bectl -r tank list
[root@squid 11.Feb 2:36pm /]#
Comment 3 dougs@dawnsign.com 2021-02-11 22:44:26 UTC
Out of curiosity, I did this:

[root@squid 11.Feb 2:36pm /usr]# bectl -r tank/root list
BE  Active Mountpoint Space Created
tmp -      /tmp       58K   2021-02-06 13:49
var -      /var       336M  2021-02-06 13:49
[root@squid 11.Feb 2:37pm /usr]#
Comment 4 Wes Maag 2021-02-11 23:31:09 UTC
Created attachment 222379 [details]
diff to allow the pool dataset to be the BE root when specified
Comment 5 Wes Maag 2021-02-11 23:33:19 UTC
Yeah, libbe really wants there to be at least two levels. It checks for the existence of a '/' in the root name and errors out if it does not exist.

Attached is a diff that I scratched together which seems to work

works[wes]:/home/wes $ zfs list -r test                                                                                                                                                                                                 [16:27:06]
NAME         USED  AVAIL     REFER  MOUNTPOINT
test         246K   832M       24K  none
test/root1    24K   832M       24K  /tmp/bectl_test_75GY
test/root2    24K   832M       24K  /tmp/bectl_test_75GY

works[wes]:/home/wes $ bectl -r test list                                                                                                                                                                                               [16:16:30]
BE    Active Mountpoint Space Created
root1 -      -          24K   2021-02-11 15:46
root2 -      -          24K   2021-02-11 15:46
Comment 6 Kyle Evans freebsd_committer freebsd_triage 2021-02-12 14:18:00 UTC
We're discussing this a bit out-of-band; in principle Wes's patch is fine, so we'll get this fixed pretty quickly.
Comment 7 dougs@dawnsign.com 2021-02-13 00:30:12 UTC
That's great news! How soon will it be before the changes are rolled out?
Comment 8 dougs@dawnsign.com 2021-05-31 22:21:33 UTC
It will be June 1st tomorrow. How soon can I expect this in the RELEASE version?
Comment 9 Robert Wing freebsd_committer freebsd_triage 2021-06-03 20:00:17 UTC
(In reply to dougs@dawnsign.com from comment #8)

Have you tested the patch provided by Wes? Does it work for you?
Comment 10 dougs@dawnsign.com 2021-06-03 20:05:04 UTC
(In reply to Robert Wing from comment #9)
I'm afraid I do not know how to implement the change. I do not compile my sources/kernel at all-- I use freebsd-update to download/install binary files.

That said, if you could point me in the right direction, I could be persuaded to test! Is there a way I could drop a binary replacement file in place and test?
Comment 11 Wes Maag 2021-06-04 13:10:37 UTC
Hello everyone, 

At the time I replied, I was having issues logging in to reviews.f.o and didn't create a review for this. But that has been resolved and I added a review to hopefully grease the wheels a bit :)

review D30636
Comment 12 Robert Wing freebsd_committer freebsd_triage 2021-06-05 00:08:44 UTC
(In reply to dougs@dawnsign.com from comment #10)

To dougs: Possibly, what version are you on? and/or output of `uname -a`?

To Wes: Thanks for posting a review - I'll give it a look over in the next few days.
Comment 13 dougs@dawnsign.com 2021-06-07 14:30:26 UTC
(In reply to Robert Wing from comment #12)

[root@squid 07.Jun 7:23am ~]# uname -a
FreeBSD squid.dawnsign.com 12.2-RELEASE-p7 FreeBSD 12.2-RELEASE-p7 GENERIC  amd64

Also:

[root@fornax 07.Jun 7:22am ~]# uname -a
FreeBSD fornax.dawnsign.com 13.0-RELEASE-p1 FreeBSD 13.0-RELEASE-p1 #0: Wed May 26 22:15:09 UTC 2021     root@amd64-builder.daemonology.net:/usr/obj/usr/src/amd64.amd64/sys/GENERIC  amd64
Comment 14 Robert Wing freebsd_committer freebsd_triage 2021-06-12 20:06:49 UTC
(In reply to dougs@dawnsign.com from comment #13)

If you get around to it...try the following on your 13.0 machine and let us know if it works:


# curl https://people.freebsd.org/~rew/libbe-D30636.so --output libbe-D30636.so
# env LD_PRELOAD=/absolute/path/to/libbe-D30636.so /sbin/bectl -r tank list

The curl command is downloading a patched version of the libbe library. This patched libbe version uses Wes's patch found at https://reviews.freebsd.org/D30636

You must pass the absolute path of the patched libbe library to LD_PRELOAD.
Comment 15 dougs@dawnsign.com 2021-07-30 23:07:22 UTC
(In reply to Robert Wing from comment #14)
Before I proceed with this step, how do I reverse this step in case I need to?
Comment 16 dougs@dawnsign.com 2021-07-30 23:14:33 UTC
(In reply to dougs@dawnsign.com from comment #15)
Never mind, I just realized that this was a one-time step to test the output.

Here's the output:

[root@fornax 30.Jul 4:10pm ~]# env LD_PRELOAD=/root/libbe-D30636.so /sbin/bectl -r tank list
BE   Active Mountpoint Space Created
root NR     /          19.9G 2019-01-10 09:33
[root@fornax 30.Jul 4:10pm ~]#
Comment 17 Robert Wing freebsd_committer freebsd_triage 2021-08-03 21:37:57 UTC
(In reply to dougs@dawnsign.com from comment #16)

Is this expected output for you?

Did you happen to try and create a boot environment and boot from it using the patched libbe version by any chance?

off hand, it appears to be doing the right thing though..
Comment 18 parv 2022-03-09 22:08:28 UTC
I am having a similar problem with base being installed by installer right onto the "pool" name ("root", not on root/some/other dataset) after choosing to make partitions myself. This was with 14-CURRENT (for Framework laptop) snapshot of Mar 3, 2022.

Should I make a new PR for the case of "bectl' not able to handle the case of base installed right on the pool name?
Comment 19 Jonathan Vasquez 2022-05-09 12:38:18 UTC
I'm also experiencing this on FreeBSD 14-CURRENT:

FreeBSD 14.0-CURRENT #0 main-n255391-c6df2176038: Sun May  8 23:10:06 EDT 2022     root@leslie:/usr/obj/usr/src/amd64.amd64/sys/GENERIC amd64

root@leslie:~ # zfs list
NAME        USED  AVAIL  REFER  MOUNTPOINT
tank       23.3G   845G    96K  none
tank/home  4.81G   845G  4.80G  /usr/home
tank/os    18.4G   845G  9.41G  /

root@leslie:~ # bectl list
libbe_init("") failed.
root@leslie:~ # bectl -r tank list
libbe_init("tank") failed.
Comment 20 Jonathan Vasquez 2022-05-09 13:06:52 UTC
I downloaded Wes's patch and applied to my FreeBSD 14-CURRENT install (as mentioned above). It seems to work. I haven't tested rebooting into a BE or anything but just attempting to display the list.

Once I applied the patch, I did a 'make all install' in '/usr/src/lib/libbe' and that's it:

root@leslie:/usr/src/lib/libbe # bectl list
BE   Active Mountpoint Space Created
home -      /usr/home  4.82G 2022-05-07 23:27
os   NR     /          18.5G 2022-05-07 23:27

root@leslie:/usr/src/lib/libbe # bectl -r tank list
BE   Active Mountpoint Space Created
home -      /usr/home  4.82G 2022-05-07 23:27
os   NR     /          18.5G 2022-05-07 23:27
Comment 21 parv 2022-05-10 01:46:21 UTC
(In reply to Jonathan Vasquez from comment #20)

"home" listed in the list would be correct only if it could be used to boot into it & be able to regular things as if /s{,bin}, /usr/s{,bin} were available from the dataset.
Comment 22 Jonathan Vasquez 2022-05-10 01:48:27 UTC
That's correct. But in this case the error is still the same since I have tank/os as well.
Comment 23 Jonathan Vasquez 2022-05-10 01:53:45 UTC
Unless what you meant was that the patch is working in that, yes, tank/os is being displayed because it is a dataset that could successfully boot (assuming the efi partition and loader are properly installed), but the tank/home dataset should actually be filtered out. I would agree with that, although I don't know if the current implementation goal is to simply be a "dumb" frontend to set the bootfs zfs property, or if it should be smarter about what datasets it allows you to activate.
Comment 24 Kyle Evans freebsd_committer freebsd_triage 2022-05-10 02:02:31 UTC
(In reply to Jonathan Vasquez from comment #23)

With my libbe ballcap on, we should make prop_list_builder_cb() filter out datasets that have a mountpoint property that isn't none or /. I don't know that we can get much smarter than that, unfortunately (e.g., probing contents)
Comment 25 Jonathan Vasquez 2022-05-10 02:18:18 UTC
Haha. Yea that sounds good. The simpler the better honestly.
Comment 26 Andriy Gapon freebsd_committer freebsd_triage 2022-05-10 12:07:39 UTC
(In reply to Kyle Evans from comment #24)
I think that it is perfectly valid to have a default mountpoint value for both "bootable" and "non-bootable" datasets.

Rather than inventing kludges why wouldn't the reporter simply follow the convention (requirement even) of having all BE datasets and only them under a common parent?
That works and does not require any effort from anyone.
Comment 27 Kyle Evans freebsd_committer freebsd_triage 2022-05-10 13:44:29 UTC
(In reply to Andriy Gapon from comment #26)

bectl should not be trying to mandate what's right or wrong here. As long as the kernel and userland rc zfs* scripts are working with this setup otherwise out of the box, I struggle to see why we shouldn't try to accommodate.

I'm otherwise not sure exactly what you mean. The mountpoint is otherwise ignored for roots, what useful values that are neither none or / have you seen?
Comment 28 Jonathan Vasquez 2022-05-10 14:47:09 UTC
In that case then wouldn't the results reported in my list already be correct?

For example in my setup I have:

tank     (none aka common parent / base dataset / pool name as well)
tank/os  (/ dataset)
tank/home (/home dataset)

from what I'm understanding, it would just display 

tank/os and tank/home then since those two datasets are under "tank" and tank is set to "none" as a mountpoint. Wes' patch already yields that result.

What would the ideal output look like in the above scenario ideally?
Comment 29 parv 2022-05-11 02:54:12 UTC
(In reply to Andriy Gapon from comment #26)

In that case I would much prefer un-bootable dataset not be listed in list of "Boot Environment" in boot menu.

I call poppycock on "simply follow the convention (requirement even) of having all BE datasets". Try yourself setting up a boot environment with ZFS on root with custom disk partitioning so that nearly whole disk in not taken by the "default" install by automated set up. See https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=262461 for details on this.
Comment 30 Kyle Evans freebsd_committer freebsd_triage 2022-05-11 03:26:04 UTC
(In reply to Jonathan Vasquez from comment #28)

Ideally, we would only show the direct descendants of tank/ that have a mountpoint of none or / (not paying attention to tank at all here). It's a heuristic that'll generally work, it would filter out tank/home so that we end up with just tank/os in `bectl list` (and thus, `bectl` will only offer to let you activate the `os` BE).

This is basically just an anti-footgun. We could set bootfs to tank/home until the cows come home, but they won't be moo'ing in the morning if we allow that. Of course, that can be overridden at the loader menu (in theory? I'd be curious to see if it works with your setup and what it shows in the BE selector), so it's not a huge deal -- the loader logic wouldn't be touched for any of that.
Comment 31 Andriy Gapon freebsd_committer freebsd_triage 2022-05-11 07:38:07 UTC
(In reply to Kyle Evans from comment #27)
> I'm otherwise not sure exactly what you mean. The mountpoint is otherwise
> ignored for roots, what useful values that are neither none or / have you seen?

The default value. Also, I do not like when a BE's mountpoint is set to /.
I like it when zfs mount <dataset> works safely even for BE datasets.
Comment 32 Andriy Gapon freebsd_committer freebsd_triage 2022-05-11 07:45:23 UTC
(In reply to parv from comment #29)
Bug 262461 looks like a bug which should be fixed, but I do not see why bectl has to be kludged to work around that bug.
Comment 33 Andriy Gapon freebsd_committer freebsd_triage 2022-05-11 07:51:17 UTC
(In reply to Kyle Evans from comment #27)
> bectl should not be trying to mandate what's right or wrong here. As long as the > kernel and userland rc zfs* scripts are working with this setup otherwise out of > the box, I struggle to see why we shouldn't try to accommodate.

I guess that this is a non-technical decision / choice.

My personal approach would be:
- if you want to use a non-conventional layout then you would need to use something other than bectl to manage it
- if you want to use bectl, then please use the conventional layout

You seem to prefer for bectl to support any BE layout.
As an example, how about BEs that are not siblings?
Is that reasonable too?
Comment 34 Jonathan Vasquez 2022-05-11 13:44:46 UTC
@Andriy

I would have to disagree. bectl is a tool, so it shouldn't really need to worry about whether a person used a conventional or non-conventional layout. They are all just zfs datasets after all and bectl is a glorified wrapper around 'zfs snapshot', 'zfs clone', 'zfs promote', and setting the 'bootfs' property (from my understanding at a high level). Choosing to support only "conventional" layouts severely reduces the effectiveness and useful of the tool and makes me feel that the OS is actually working against me and not really respecting my decisions. FreeBSD in general, already delegates a lot of responsibility to the user and it's the users responsibility to understand how things work and not shoot themselves in the foot. With that in mind, I don't see a reason why BEs should have a lot of hand holding in this regard and not just display all the available datasets. I think what @Kyle mentioned makes sense and it's a compromise approach in that we will hold your hand a little bit by just showing you only datasets who's mountpoints are set to either "none" or "/" (which protects the users who might have done a "zfs clone <snapshot> <target>" let's say.. there's a lot of other scenarios where a zfs dataset can have a mountpoint other than / haha. Thus just displaying things for "none" and "/" would cover pretty much all of the use cases, protect most of the users, and still allow the tool to be completely flexible and layout agnostic.
Comment 35 Kyle Evans freebsd_committer freebsd_triage 2022-05-11 14:15:25 UTC
(In reply to Andriy Gapon from comment #31)

Ah, ok. When you referred to the default value earlier, it made much less sense- FreeBSD defaults (installer) zroot/ROOT to none and BEs to /, but you're referring to zfs inferred default and not default inherited. I'm not a fan, but I guess that's a reasonable choice that I also wouldn't want to break.

(In reply to Andriy Gapon from comment #33)

Not any layout, no, but reasonable looking layouts. Your question is awkward because bectl supports arbitrary root roots with the undocumented -r flag, so of course we can handle it- you just have to point it at the other set.
Comment 36 Kyle Evans freebsd_committer freebsd_triage 2022-05-11 15:54:47 UTC
I think the end result of this discussion is that we should land the initial patch to make BEs at the root "work," but we shouldn't really do any filtering or anything like that. This architecture makes a whole lot of sense if you're going to deep BE the whole system, if you see otherwise weird results then there's not a whole lot we can do about that without filtering out perfectly valid BEs on other setups.
Comment 37 Jonathan Vasquez 2022-05-11 16:33:59 UTC
That sounds good to me. Like I said earlier, the simpler the better :D.
Comment 38 Jonathan Vasquez 2022-07-27 18:34:23 UTC
I've converted my existing one layer set up to a two layer one so I now have the option of either using bectl or doing the snapshotting/cloning myself, and still be compatible with the boot environment feature in the loader. I've also updated my guide recommendation accordingly: https://xyinn.org/md/freebsd/zfs_manual_partition_encrypted

previous layout (all 'canmount' options were 'on'):

tank
tank/os <- this would have a mountpoint set to /
tank/home <- /home

new layout

tank
tank/os <- canmount=off, mountpoint=none
tank/os/main <- main / dataset, canmount=noauto, mountpoint=none
tank/home <- /home

I can now either manually do a:

zfs snapshot tank/os/main@test
zfs clone tank/os/main@test tank/os/test

and reboot and select "tank/os/test" in the bootloader's boot environment selection, and if it's good I can do a promotion and a deletion and set 'bootfs' on my pool accordingly if needed

or I can do 'bectl create test' and 'bectl activate test'.

I still think this feature is useful but not as high priority for me anymore.. I do prefer my new slightly modified layout :).