Add orange flavor btrfs support

[rdesaintleger]

• remove btrfs default subvolume,
• save btrfs device in Btrfs structure,
• rework brtfs state mountpoint detection (uses btrfs device, got rid of rootSubvolID and snapshotsSubvolID constants),
• add 'active' relative symlink to active snapshot (updated in CloseTransaction() and read in getActiveSnapshot()),
• add grub 'active_snap' variable (could it be replaced by dracut resolving the 'active' link ?),
• add grub 'root_subpath' variable which allows to prefix kernel and initrd image with a subpath,
• handle grub modules packaging under /usr/lib/elemental/bootloader
• add RelativizeLink() to recursively make symbolic links relative,
• ensure kernel and initrd are relative links (allow grub to process without mounting subvolume)
• update orange Dockerfile

[kkaempf]

@rdesaintleger thanks for your PR !
Please add some explanation of why this change is needed (a link to an open issue is sufficient). See #2206

[rdesaintleger]

Sorry, my PR is bad for the moment

I found a fix for the RelativizeLink() function which allow build tests to go further (up to snapshotter). Also I didn't notice that snapper did use the btrfs default subvolume feature.

I think that I'll make a new snapshotter without snapper named 'btrfs_nosnapper' with its own regression tests. This will have the advantage of not breaking existing btrfs green flavors and being cross distro when using new snapshotter.

[frelon]

@rdesaintleger really nice to see this implemented! Looking forward to the new snapshotter!

Looking back we should probably have named the current "btrfs" snapshotter as "snapper" instead.. oh well!

[davidcassany]

I think that I'll make a new snapshotter without snapper named 'btrfs_nosnapper' with its own regression tests. This will have the advantage of not breaking existing btrfs green flavors and being cross distro when using new snapshotter.

At a time when I coded this snapshotter in head there were few configuration options for the btrfs snapshotter. Those were related to the required stack. Pure btrfs implementation, without snapper, implementation with snapper and third option using tukit. So my idea was having like three different levels of abstraction. So, even if the implementation could be in different files or structures I imagined all three as single snapshotter for btrfs. The thing is that snapper also support some sort of LVM snapshots which are definitely not supported by Elemental-toolkit and tukit assumes snapper. Hence in all cases we are talking about BTRFS subvolumes.

Just explaining that because most of the parts that are required to make a wrapper for btrfs tool are already implemented in the current snaphsotter hence we should figure out some ways to reuse all that.

See the following manual steps I did at a time to make a PoC before actually implementing the snapshotter for btrfs.

For install (first snaphsot):

# Enable quota management and create subvolumes
btrfs quota enable /mnt
btrfs subvolume create /mnt/@
btrfs subvolume create /mnt/@/.snapshots

mkdir -p /mnt/@/.snapshots/1

btrfs subvolume snapshot /mnt/@ /mnt/@/.snapshots/1/snapshot

# ID should be computed from a "btrfs subvolume list" command
# default set to the equivalent of the STATE partition (probably a /@/state subvolume would make more sense)
volid=$(btrfs subvolume list --sort path /mnt | head -n 1 | cut -d" " -f2)
btrfs subvolume set-default ${volid} /mnt

# Create snapshot 1 info
date=$(date +'%Y-%m-%d %H:%M:%S')
cat << EOF > /mnt/@/.snapshots/1/info.xml
<?xml version="1.0"?>
<snapshot>
  <type>single</type>
  <num>1</num>
  <date>${date}</date>
  <description>first root filesystem</description>
</snapshot>
EOF

# Dump the built image into the first snapshot subvolume
./build/elemental pull-image --local ${osimage} /mnt/@/.snapshots/1/snapshot/


# Create the quota group
btrfs qgroup create 1/0 /mnt
#chroot /mnt/@/.snapshots/1/snapshot snapper --no-dbus set-config QGROUP=1/0

# set first snapshot as readonly
btrfs property set /mnt/@/.snapshots/1/snapshot ro true

All of the above is already coded in the current snapshotter. So most if not all pieces are already there. I'd be happy to help on getting a proper btrfs wrapper that can be used in both cases. With snapper and without. Probably it is even possible to achieve a snapper compatible setup without snapper.

For completeness the rough steps for an upgrade are:

# Get current snapshot
current=$(snapper --csvout list --columns number,active | grep yes | cut -d"," -f1)

# Create new snapshot from current
id=$(snapper create --from ${current} --read-write --print-number --description "Snapshot Update of #${current}" --userdata "update-in-progress=yes")

# Apply image to the new snapshot
elemental pull-image --local ${osimage} /mnt/@/.snapshots/${id}/snapshot/

snapper modify --userdata "update-in-progress=no" $id

# Make snapshot RO
btrfs property set /.snapshots/${id}/snapshot ro true

See here snapper is only used for the creation of a new subvolume.

[rdesaintleger]

Thanks for information. I'll try the following (not sure yet for the grub part)

• modify grub configuration to test (in case of btrfs root) for directory @/.snapshots/${active_snap}/snapshot (if false it's a snapshotter with actual behaviour).
• add a DisableDefaultSubVolume property for BtrfsConfig yaml defaulting to 'false' (false is using snapper)
• Update btrfs.go to handle properly both configuration

If grub is unable to test for snapshot directory I'll search for another solution. The property name comes from the original problem which is that other distro's grub do not handle properly default subvolumes.

[rdesaintleger]

Now able to boot orange on btrfs.

elemental upgrade works in recovery but not in active state due to snapper configuration

[rdesaintleger]

Hi @davidcassany

I'm actually near the end of snapshotter modifications. I've added two booleans for configuration

• disable-snapper : disable snapper and use pure btrfs backend
• disable-default-volume : stick on btrfs top level volume and rely on symlink and new grub variable (disabling is incompatible with snapper).

The real needed change for orange to boot with own bootloader is the grub variable (since 'set btrfs_relative_path="y"' has no effect at all). I've managed to get snapper running with Ubuntu (However sometimes it takes time to enable snapper on first boot, making elemental upgrade fail until fully snapperd is fully bootstraped).

I've done the following changes for snapper bootstrap (it's not actually real shell code) :

# create 'bootstrap' subvolume under state directory 
btrfs subvolume create -i 1/0 ${stateDir}/.bootstrap

# pull the image in the bootstrap directory
elemental pull-image --local ${osimage} ${stateDir}/.bootstrap

# prepare snapper configuration (if .bootstrap/etc/snapper/configs/root does not exists)
# chroot must be prepared with regular directories but no .snapthots
rm -rf ${stateDir}/.bootstrap/.snapshots
chroot ${stateDir}/.bootstrap snapper --no-dbus --config root create-config --fstype btrfs /
# delete the subvolume created by previous call
btrfs subvolume delete -c ${stateDir}/.bootstrap/.snapshots
# create .snapshots directory for future mounts
mkdir ${stateDir}/.bootstrap/.snapshots
# here etc/snapper/configs/root if modified to comply with snapshotter

# prepare second chroot with .snapshots bind mounted
# this call will create .snapshots/1/snapshot (and associated xml file)
chroot ${stateDir}/.bootstrap snapper --no-dbus --config root create --read-write --cleanup-algorithm number

# lastly delete bootstrap subvolume
btrfs subvolume delete -c ${stateDir}/.bootstrap

This process is compatible at least with orange and green flavor. It's heavier than the previous process but I find it more proof to distro specific file path for templates (root snapper config was not created in Ubuntu). What do you think about it ?

Have you got any advice before I work to fix test-cli make target ? I may need help to fix tests which need chrooted calls.

[davidcassany]

This process is compatible at least with orange and green flavor. It's heavier than the previous process but I find it more proof to distro specific file path for templates (root snapper config was not created in Ubuntu).

I see snapper lacking a root config might be challenging but I am not sure I am comfortable with this bootstrap approach, really looks like an overkill and the logic to mimic snapper was almost entirely there.
IMHO snapper needs openSUSE/snapper#944 which is already in the process of being implemented upstream, when done this would not be needed anymore. Hence I'd be more in favor of the former approach to mimic snapper with btrfs. This could also eventually lead us to a position in which the btrfs only snapshotter could be compatible with snapper, so it could be even possible to migrate from btrfs only to snapper at some point as soon as the underlying distro supports the required version of snapper.

Your PR entirely rewrites the btrfs snapshotter, this is a sensitive portion of code. It might take some iterations to get that done. To be frank I am a bit unsure how to proceed. I'd probably try to iterate in smaller steps, I'll try to provide some specific ideas in the following days.

[davidcassany]

I struggle to see the need of this step and the need of RelativizeLink method in general. Why would we delete and recreate a symlink we just created a couple of lines above?

I'd say something like:

relInitrd, err := filepath.Rel(filepath.Dir(constants.InitrdPath), initrd)
if err != nil {
	cfg.Config.Logger.Errorf("could set a relative path from '%s' to '%s': %v", constants.InitrdPath, initrd, err)
	return err
}
err = cfg.Fs.Symlink(relInitrd, constants.InitrdPath)

Should be sufficient and entirely omit the need of RelativizeLink method.

[rdesaintleger]

To be useable in Grub without default subvolumes kernel and initrd links must be recursively relative. I initially set a single link like you suggest with duplicated code on kernel and initrd. However I wanted to ensure that if target of initial link is itself a link, it was a relative link too. Making a general purpose function to relativize recursively a link sounded a better idea since it also simplify the code (don't touch actual code, but adds a call to relativize).

I'll test with requested change (without relativizing) and I'll let you know

[davidcassany]

I don't think this entire method is needed

[rdesaintleger]

Ok, I've restarted from scratch using btrfs.go from main branch.

The following changes have been done outside the main feature:

• rename b.rootDir to b.stateDir (stateDir is needed to maintain active snapshot symlink)
• b.rootDir is now defined only if using active or passive state (otherwise it's an empty string)
• added getSnapshotsDirectory() to return the snapshots directory depending on b.rootDir and b.stateDir definitions
• added '-a' to btrfs subvolume list to ensure that full path is provided for subvol name
• changed isInitiated() subvolumes check from IDs to names (sinces IDs are incorrect agains constants in all tests I've done)
• findStateMount() now returns top level ans state btrfs directory if applicable (to check for subvolumes and active link)
• findStateMount() now extract subvolume names between first '[/' and last ']' before applying regex (since '[]' can be part of subvolume name)
• Added a mechanic to save top subvolume ID (which does not match entries in btrfs_test.go so a constant may not be a good choice)
• added a check to setBtrfsForFirstTime() to ensure that btrfs top level directory is mounted before creating subvolumes
• added runCurrentSnapper() and runSnapshotSnapper() to launch snapper with correct arguments and mounts
• removed snapshots mount/unmount logic (replaced by a bind mount in run*Snapper functions)
• removed currentSnapshotID which is updated but not used anywhere
• removed activeSnapshotID update in snapper snapshot list to have same behavior with or without snapper
• changed order of tests (close transaction failures are tested before close transaction success)
• old snapshots are cleaned just before setBootLoader()

for the cross distro btrfs snapshotter:

• added 'disable-snapper' to disable snapper logic and rely only on btrfs commands
• added 'disable-default-subvolume' to disable btrfs default subvolume and replace current snapshot with relative link in state directory
• moved snapper root config after workdir sync (otherwise snapper complains that workdir is not a subvolume)
• snapper root config is now created using a chroot in the created snapshot
• pure btrfs configuration will create snapper info.xml file (without user data)
• findSubvolumeByPath() has been changed from a subvolume list iteration to an inspect-internal rootid (which directly returns the subvolume id)
• getActiveSnapshot() has now a stateDir parameter to be able to read active symlink during initialization
• activeSnapshotID is now updated in CloseTransaction() before setBootLoader()

[davidcassany]

@rdesaintleger would you mind to hold the implementation for a day or two? Sorry I have been working on it for a while and I think I managed to divide the snapper and pure btrfs snapshotter without touching former logic (aka keep using static volume IDs, current grub and no changes in unit and e2e tests). I will include today eve a draft PR of it. Then if all looks good I wonder if we can start to tacke one by one all the other missing refactors (grub, default volume issues, etc.).

I hope this is OK for you, thanks much for your contributions

[davidcassany]

@rdesaintleger I created this PR #2220 which essentially enables pure btrfs snapshotter but keeping almost 100% of the previous logic when using snapper. The pure btrfs implementation follows the same pattern as done by snapper, I don't have any good argument not to do so and this also potentially enables the possibility of start using snapper later in time.

[rdesaintleger]

Fine, thank you @davidcassany

Just to be clear before starting over. Do you want me to make a PR (with modifications committed one by one) on your new repo before dropping this one ?

Also, I've setup two booleans for two features, do you want me to dropthe active snapshot symlink feature (to avoid using btrfs incompat features) ?

Lastly : I've used 'disable-snapper' to be sure that the default behavior of the snapshotter would be to use snapper to support green flavor downgrades (As I did not find out how to use 'true' as default value). Do I restart with your definition or do I use 'disable-snapper' ?

[davidcassany]

@rdesaintleger hi sorry for the delay, had some time off these days.

Just to be clear before starting over. Do you want me to make a PR (with modifications committed one by one) on your new repo before dropping this one ?

So, my expectation is that now from the current main branch it should be possible to make few little changes to improve the cross distro capabilities. So I think one of the most prominent needed changes is around grub configuration and work around the missing btrfs_relative_path which is not granted on all distros. You had great ideas on that regard, I'd encourage you for a PR for such feature.

Regarding the disable default subvolume feature, am not sure about it, I don't see the need of it. If this is just a grub2 matter then I'd say this needs to be fixed in grub config. Is this causing issues in Ubuntu at runtime? default subvolume concept I don't think is a SUSE specific addition on top of btrfs.

Finally you noted some improvements on some methods, thanks much for that, I'd be happy to review and merge those on small PRs (probably not necessarily one per PR as long as we don't have hundreds of lines together).

[rdesaintleger]

Hi @davidcassany

Changed my branch to include your changes. Basically I did 2 commits. The first one have just needed modifications to enable btrfs snapshotter on orange flavor (I had also booted a debian trixie too but snapshots and state got mounted read only). The second commit fix btrfs state volumes detection (258 is the first snapshot ID which will get deleted and will trigger a bug where btrfs state volume will be seen as uninitialized).

I dropped the active symlink feature as suggested.

The first commit basically do the following:

• ensure that kernel and initrd are accessible by grub using relative links
• maintain an 'active_snap' variable so grub is able to reference the active snapshot directory
• adds a relative path variable in grub (set to empty when not needed)
• allow snapper configuration for orange flavor

I've disabled snapper in orange flavor since it takes time to initialize on boot. It however works. The setBootloader() now takes the commited snapshotID as argument. I guess that this ID could be propagated to other calls but I did not investigate further to keep a minimal impact (could be propagated to getPassiveSnapshots() and an internal version of GetSnapshots()).

The second commit changes the btrfs backend Probe() and findStateMount(). Basically subvolumes are listed with '-a' to ensure that the whole subvolume name is available (<FS_TREE>/ is stripped out from names). A new function getStateSubvolumes() returns root and snapshots subvolumes structure by matching their names (this function is used by Probe()). Lastly the findStateMount() has been modified to extract subvolume name from findmnt command. It's done using substring index/lastIndex to avoid wrong regex matches if subvolumes are named using '[' or ']'. This allow to match state mountpoint using the source subvolume and not the target mount.

Could not figure out how to launch workflow in my repo, sorry if errors are still showing up. Did manual testing on green and orange flavors. I can discard the last commit and make a separate Issue/PR if you wish.

[davidcassany]

Looks good to me 👍 Thanks much for your contribution and patience

[davidcassany]

Good catch, thanks

[davidcassany]

Good catch, I was not aware that snapper on Ubuntu uses different configuration paths

[davidcassany]

Yes, this is a nice improvement, thanks 👍

[frelon]

LGTM