Add a btrfs partitioning mode #422
Conversation
Prior this commit, the GenUUID method of Btrfs didn't propagate the generated UUID to a subvolume. Thus, when GetFSSpec was called on the subvolume, an empty UUID was returned. Thus, invalid kernel arguments were generated (for root fs), and the system rendered to be unbootable.
So the code just panicked before. Let's generate a device name in a similar way as we do for a luks container.
This one is a bit weird - the code iterates over all mountable entities, but the code currently doesn't consider the "root" btrfs (i.e. without a subvolume) as a mountable entity. Thus, we need to iterate over all entities, and handle btrfs separately. A test will be added in a subsequent commit.
This commit also adds a test that covers both mkfs.btrfs (see the previous commit) and creating subvolumes.
This commit also adds a semi-fake partition table with a Btrfs setup.
This commits also removes the generic opts (MntOpts) from btrfs subvolumes. They were not used anywhere, and I think it's much better to make all supported mount options explicit, rather than storing them in a generic array.
TODO: needs tests The conversion code from raw partitioning is basically the same as the one for LVM. The main difference is that LVM just embeds an already defined filesystem, but btrfs-ication needs to do some guessing because the code actually creates a new partition: specifically, I just hardcoded the compression to zstd:1. This is not ideal, but it's the Fedora's default, so it's imho fine as the first iteration.
This is weird, because we just hardcode zstd:1, but it's good enough for the starters (and also inspired by Fedora). I also introduced a constant for the default btrfs compression.
|
Maybe use the sum of |
There was a problem hiding this comment.
Thank you for this branch. It looks excellent. I left a lot of comments/suggestion but that is not because there is anything wrong with the branch (the contrary!) but only because a) my default mode is -Wpedantic -Wextra-pedantic b) this is really good stuff so I enjoyed reviewing it c) many are just style/preferences and can just be ignored (or considered)
I only managed to review half because some other review came up but I hope this is still helpful
|
|
||
| "github.com/google/uuid" | ||
| ) | ||
|
|
||
| const DefaultBtrfsCompression = "zstd:1" |
There was a problem hiding this comment.
(nitpick) I wonder if this could not be exported by default, no deep reason though, so fine to leave as is (testdisk/partition.go would have to be modified).
| if name == "/" { | ||
| name = "root" | ||
| } | ||
| subvolumes = append(subvolumes, disk.BtrfsSubvolume{Mountpoint: mntPoint, Name: name, Compress: disk.DefaultBtrfsCompression}) |
There was a problem hiding this comment.
I wonder if Compress unset should just automatically set the defaults, this would mean it does not need to be added explicitly and the user of the API has to think a little less about it :)
| @@ -66,6 +72,11 @@ func NewPartitionTable(basePT *PartitionTable, mountpoints []blueprint.Filesyste | |||
| if err != nil { | |||
| return nil, err | |||
| } | |||
| } else if ensureBtrfs { | |||
There was a problem hiding this comment.
(super nitipck) in go, I really like switch/case for if/else if constructs because they look more "symmetrical" to me, so:
switch {
case ensureLVM:
err := newPT.ensureLVM()
if err != nil {
return nil, err
}
case ensureBtrfs:
err := newPT.ensureBtrfs()
if err != nil {
return nil, err
}
}but that is entirely a style thing and feel free to ignore
| switch mode { | ||
| case LVMPartitioningMode: | ||
| ensureLVM = true | ||
| case RawPartitioningMode: | ||
| ensureLVM = false | ||
| case DefaultPartitioningMode, AutoLVMPartitioningMode: | ||
| ensureLVM = len(newMountpoints) > 0 | ||
| case BtfrsPartitioningMode: | ||
| ensureBtrfs = true |
There was a problem hiding this comment.
(super nitick) given that there is only one case where we need to call ensureBtrfs() maybe we could just call it here directly? it makes things less symmetrical which is a fine reason to ignore/discard this idea but it does not look too bad IMHO (especially if it's a single line if err := ... ; err != nil { pattern)
|
|
||
| if _, ok := parent.(*Btrfs); ok { | ||
| return nil | ||
| } else if part, ok := parent.(*Partition); ok { |
There was a problem hiding this comment.
If I am not misreading we could follow effective go [0] and avoid the not strictly needed nested else here(?) (because the if returns when taken). So something like(?):
@@ -655,7 +651,8 @@ func (pt *PartitionTable) ensureLVM() error {
if _, ok := parent.(*LVMLogicalVolume); ok {
return nil
- } else if part, ok := parent.(*Partition); ok {
+ }
+ if part, ok := parent.(*Partition); ok {
filesystem := part.Payload
vg := &LVMVolumeGroup{|
|
||
| parent := rootPath[1] // NB: entityPath has reversed order | ||
|
|
||
| if _, ok := parent.(*Btrfs); ok { |
There was a problem hiding this comment.
I wonder if this could be a go type switch? i.e.
switch part := parent.(type) {
case *LVMLogicalVolume:
return nil
case *Partition:
filesystem := part.Payload
...
default:
return fmt.Errorf("Unsupported parent for LVM")
}| if pt.Type == "gpt" { | ||
| part.Type = FilesystemDataGUID | ||
| } else { | ||
| part.Type = "83" |
There was a problem hiding this comment.
Would it make sense to have a symbolic name for this constant?
Btrfs needs a minimum size of 16MiB when using |
|
This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days. |
|
This PR was closed because it has been stalled for 30+7 days with no activity. |
This PR has basically three parts:
Weird parts:
min-sizefor extra mountpoints is a weird thing, because the extra mountpoints are translated to subvolumes... Maybe we should just error out if the min size is set with btrfs and depend just on the total size customization... Happy to discuss this!Note that I specifically didn't disable the new mode in EL distributions... My thinking is that users are running these distros on a custom btrfs-enabled kernel, we shouldn't block them.
This doesn't have tests for the actual partitioning mode. There are currently just new tests for the lower level code in the disk package.