-
Notifications
You must be signed in to change notification settings - Fork 50
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
Advanced partitioning customizations (COMPOSER-2355) #926
base: main
Are you sure you want to change the base?
Conversation
c9376dc
to
296fa3e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had a quick look (and then ran out of time) - this looks super nice so far, two tiny comments and I will continue later.
A meta-question - how will this interplay with the otk-gen-partition-table
helper? it seems there is a lot of overlap and we will also need to add lvm/lbtrfs/luks there. Will it be unavoidable that we have two code paths that are doing this full the blueprint and for the otk yaml input or is there a way to share code (or do we do that already because it all maps to the same underlying primitives)?. I will try to understand this question myself a bit better and maybe it's too big to answer in detail in a PR in which case we can just do a OOB brainstorm and then put the summary in here.
} | ||
} | ||
|
||
func mkESP(size uint64) Partition { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(for later) we should make a note that we use this helper in otk-gen-partition-table
somehow, would be nice to consolidate this (similar to mkBIOSBoot)
I don't know, but I'll tell you how I'm thinking about this. The existing filesystem customizations are too simple for the target use cases of otk. Back in the day, the only partition table customization we had was setting the size of the disk (which grew What this PR does is move a lot more of the description of the partition table up from inside the image definitions into the blueprint. I think this move would have been a bit controversial a couple of years ago. We would generally try to keep the blueprints simple, the general philosophy being that the base image definitions are well tested, they're a great starting point for just about anyone, and the limited range of customizations is a feature that offers safety and stability. The general thinking was that the wider we spread the range of customizations the less we can test, the number of "invalid" configurations grow, and the weaker our guarantees of stability become. Note that this might just be me talking, I don't mean to project my ideas of how things were and are onto the entire team, even though I might be presenting it that way. I think we're slowly realising that this has been holding us back, that many (most?) users want more control, and they don't mind the occasional footgun (though I suspect many who claim to want "lots of flexibility, with a chance of footgun" tend to underestimate the pain they're opening themselves up to). What this all means for otk: Otk was conceived as the tool for distro maintainers. osbuild-mpp done right. It's easier to think about the level of customization and flexibility that otk should expose: pretty much anything goes. There's no regard for lack of safety, users of otk "should know what they're doing". My first thought when I read your question was that the level of flexibility exposed by these customizations can be the same for the otk tooling. That seems strange to me though because blueprints should be more abstract/high-level than the distro maintainer tooling. It certainly seems (to me right now) that we could have blueprint customizations for partitioning that are powerful enough for a distro maintainer using otk, but I am worried about the requirements of otk pushing for more advanced features in blueprints. For example, as I see it, there should be no way using blueprints in osbuild-composer to build an image configured for "hybrid boot" without a BIOS boot partition. It should, however, be possible to draw up whatever partition table one wants using otk, because otk shouldn't be that smart. So where does otk fit in the space between blueprint partitioning and make your own partition table to the dot? It's certainly annoying to have to write out a whole PT, but isn't that what otk is for? I'm actually not sure. Examining a few answersThe otk external tooling should work exactly like blueprints (take in
|
296fa3e
to
e7c477f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome work! I love it! I don't see any functional issues, just some minor things that I noticed.
Regarding OTK: The current plan is that osbuild-composer will switch to an OTK backend (through images
) at some point, right? Then, we will need to somehow pass blueprints to OTK: So either we will have to translate both partitioning customizations to a unified format, or omnifests (and related externals) will simply have to understand both.
// | ||
// There should always be one value for each filesystem type supported by | ||
// osbuild stages (stages/org.osbuild.mkfs.*) and the unset/none value. | ||
type FSType uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about making this a string
alias, and use descriptive values? Should make debugging a tad bit simpler. Are there downsides (apart from slightly worse performance)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's not much downside other than making it "easy" to use strings in place of the actual value sometimes, which can make it hard to change or track. Having the enum backed by a uint says, a bit more clearly, "the underlying value doesn't matter", which I find encourages better use of the enum.
But I can go either way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I had the same thought and tried it out and it was the same about the amount of code/complexity, golang just does not have a good way of doing this it feels so I did not comment (maybe I should have).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we have any strong feelings either way? Should I go with strings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally don't have strong feelings. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some quick comments on the first part, did not manage to get further yet, will try later today.
} | ||
|
||
// collect all mountpoints | ||
mountpoints := make([]string, 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super nitpick) this could be just var mountpoints []string
but probably just personal preference :)
@@ -24,6 +162,24 @@ func (fsc *FilesystemCustomization) UnmarshalTOML(data interface{}) error { | |||
return fmt.Errorf("TOML unmarshal: mountpoint must be string, got %v of type %T", d["mountpoint"], d["mountpoint"]) | |||
} | |||
|
|||
switch d["type"].(type) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess we don't test this right now? It's fine if we don't, I would love to have a test for this and also one that checks that the json strings are what we expect but I don't want this PR to become bigger, I can offer to do it as a followup :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added to existing tests.
// | ||
// There should always be one value for each filesystem type supported by | ||
// osbuild stages (stages/org.osbuild.mkfs.*) and the unset/none value. | ||
type FSType uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fwiw, I had the same thought and tried it out and it was the same about the amount of code/complexity, golang just does not have a good way of doing this it feels so I did not comment (maybe I should have).
e7c477f
to
ed0e569
Compare
Fixed up the following from comments:
There are a couple of more comments still open. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you! This is very nice and will be a very useful feature for our users. I look forward seeing it used (in e.g. bib). I added a bunch of comments/ideas etc but all optional.
What is missing to get it out of draft? I also wonder if might make sense to split into smaller chunks as reviewing 2k lines is not easy but then it's hard to split I guess.
Happy to approve fwiw
pkg/disk/partition_table.go
Outdated
switch options.DefaultFSType { | ||
case FS_EXT4, FS_XFS: | ||
bootType = options.DefaultFSType | ||
case FS_NONE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) AFAICT this check is orthogonal to the bootType
selection, right? If so I personally would move the check for FS_NONE earlier as I was a bit confused by the two things happening here that are (seemingly?) othogonal (but maybe it's just me)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, NONE should be allowed. I think we should make it so that options.DefaultFSType == FS_NONE
is only an issue when a non-boot partition is automatically created.
pkg/disk/partition_table.go
Outdated
if _, err := payload.CreateLogicalVolume("", 0, rootfs); err != nil { | ||
return nil, fmt.Errorf("error creating root logical volume: %w", err) | ||
} | ||
break PartitionLoop |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(niptick) moving this into a helper function probably allows to avoid the "break PartitonLoop" here as this would just becomea "return" but I'm not sure how much of a win it would be (have not tried it)
pkg/disk/partition_table.go
Outdated
rootpart := Partition{ | ||
Type: FilesystemDataGUID, | ||
Bootable: false, | ||
Size: 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A small comment like // 0 means auto-adjust
or something might be nice, alternatively we could define a const PART_SIZE_AUTOADJUST 0
or something to allow conveying this without a comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know. 0 means autoadjust only in the case of /
and even then it might stay 0 if there are no required sizes specified. I think a comment is the right way to go instead of a const that could imply more.
pkg/disk/partition_table.go
Outdated
// If no separate requiredSizes are given then we use our defaults | ||
if requiredSizes == nil { | ||
requiredSizes = map[string]uint64{ | ||
"/": 1073741824, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) maybe common.GibiByte
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is gone now and defined on Fedora image types, with GiB.
pkg/disk/partition_table_test.go
Outdated
} | ||
|
||
testCases := map[string]testCase{ | ||
"null": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call this "dos" or "hybrid-dos" here as name or something as a real "null" would leave PartitionTableType/BootMode blank?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed.
func NewCustomPartitionTable(customizations *blueprint.PartitioningCustomization, options *CustomPartitionTableOptions, rng *rand.Rand) (*PartitionTable, error) { | ||
pt := &PartitionTable{} | ||
|
||
if options == nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(super nitpick) many projects (in my small sample that I have seen) put this check as the very first thing as a convention to never accidentally use options before it's initialized. Here it's almost the first and there is no risk so fine to ignore but I wanted to mention it because I like the convention(super nitpick) many projects (in my small sample that I have seen) put this check as the very first thing as a convention to never accidentally use options before it's initialized. Here it's almost the first and there is no risk so fine to ignore but I wanted to mention it because I like the convention
} | ||
|
||
// NewCustomPartitionTable creates a partition table based almost entirely on the partitioning customizations from a blueprint. | ||
func NewCustomPartitionTable(customizations *blueprint.PartitioningCustomization, options *CustomPartitionTableOptions, rng *rand.Rand) (*PartitionTable, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(nitpick) This is a long function. It's okay because it is very linear so it's not a big deal, but I was wondering if it might be split up so that parts can be reused for otk and also if it would be easier to follow/extend with splitting (and test individually). I did a quick experiment in acb3825#diff-ffc14a35d85eb6fd9724427b51786e40bc3e910bc41cbd6762ab977ae9689765R455 which I kinda like but maybe just personal preferences - can of course be a followup
- New structure that's more powerful than the existing FilesystemCustomization. - The general structure starts at 3 top-level objects: - Plain: customizations for plain disk partitioning, meaning partitions with traditional filesystem payloads (xfs, ext4, etc). - LVM: customizations for creating LVM layouts. Supports one or more VolumeGroups. Each VolumeGroup implies the creation of a partition with the appropriate type and payload. - Btrfs: customizations for creating btrfs layouts. Supports one or more volumes and subvolumes for each volume. Each btrfs volume implies the creation of a partition with the appropriate type and payload. - Partitioning.Filesystems is an array of the old FilesystemCustomization struct. - The new FilesystemCustomization struct supports a label and Type. - The LVCustomization (logical volume) embeds the FilesystemCustomization and adds a name for the LV. Unmarshal tests have been updated with new fields.
New function that creates a partition table from scratch based on the new partitioning customizations.
ed0e569
to
5f8d787
Compare
Add one test for each general layout: plain, lvm, and btrfs.
Move the handling of the /boot partition before the plain partition handling so we don't have to find where to insert it (after BIOS boot and ESP, which are going to be added in a later commit, but before plain partitions). Instead, we build the partition list sequentially as needed. Update tests.
Move the BootMode enum from pkg/distro to pkg/platform. We want to import BootMode to the disk package, which would create an import loop with distro.
Add BIOS boot and ESP partitions based on boot mode. The boot mode is passed as an argument to the NewCustomPartitionTable() function. Update tests.
At the end of the partition table's creation, add a root partition if one wasn't created during the processing of the customizations.
When the customizations argument is nil, initialise it with an empty struct and continue processing the function normally so that the root partition is created as well.
Add minsize to the top level of the partitioning customizations. In osbuild-composer, this value comes from a command line option through distro.ImageOptions. The plan is to have that be a blueprint customization that osbuild-composer will resolve when it's defined in multiple places and pass it to images via the customization.
Add a new argument, defaultType, to the NewCustomPartitionTable() function. The purpose of the type is to determine which filesystem will be used for root and boot when they are added automatically. It will also be used to set the filesystem for any mountpoint that has no type defined. The default type should be determined by the distro definition.
When a custom partition or LogicalVolume does not specify a type, use the defaultType.
Add a new enum, FSType, for referring to all supported filesystem types. Use it as the default type argument to the NewCustomPartitionTable() function.
Run a relayout() on the partition table before returning it from NewCustomPartitionTable() to set the start position and size for each partition as well as the total partition table size. Update tests. Partition sizes in tests changed to MiB (from plain bytes) so that the values make more sense when headers are added and values are rounded to the grain size (1 MiB).
5f8d787
to
0084b32
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a tiny documentation issue, but we can resolve it in a follow-up. Otherwise, this LGTM. Thanks a ton! ❤️
Introduce the concept of required sizes from the existing NewPartitionTable() function into the NewCustomPartitionTable(). RequiredSizes is necessary for setting a "sane default" (minimum) size for certain directories. Unlike the old partition table creation function, there is no default value for the custom partition table generator. Instead, image definitions will need to set their own defaults. Without this mechanism, automatically created root filesystems can end up being too small to fit the base OS packages or to have a usable system in the long run. It's also needed when automatically creating root partitions or LVM Logical Volumes. Without a required minimum, the root partition will only be the size of the leftover space on the image (the base image size minus the sum of the rest of the partitions, which can sometimes end up being 0) or LVM VolumeGroup. Add some required sizes to tests.
This is leftover from the repo split. It is implemented and used in osbuild-composer.
This is the equivalent to the CheckMountpointsPolicy() function for the new PartitioningCustomization. It collects all the mountpoints from Plain, LVM, and Btrfs then compares them to the policy.
Preparing the function to use the new PartitioningCustomization when set.
For all image types that don't specify a requiredPartitionSizes, add the defaults that are used in the NewPartitionTable() so that NewCustomPartitionTable() has similar behaviour. The only image type that explicitly doesn't specify minimum sizes is the iot-raw-image, which has a pre-existing empty map.
Add checks for Partitioning customizations in Fedora's checkOptions() function. The function now returns an error if: - Partitioning is defined at the same time as Filesystem - Partitioning is defined for ostree image types (commit and container). - The mountpoints in Partitioning violate the mountpoint policies for the image type.
Add a Validate() method to PartitioningCustomization that returns an error if: 1. Btrfs and LVM are both defined. 2. Any mountpoint is invalid (empty string, relative path, or not "clean"). 3. Any mountpoint is defined more than once in the whole structure. 4. There is more than one LVM Volume Group or more than one btrfs volume. 5. Any LVM Logical Volume name is used more than once in the same VolumeGroup. 6. Any Btrfs subvolume has an empty name. 7. Any Btrfs subvolume name is used more than once in the same Btrfs volume. This function validates the customization itself in the absence of any distro or image-type specific policies. We may relax rule 3 in the future, at least in images if not in osbuild-composer, but for now, let's disallow it while keeping the customization structure flexible. val
The VolumeGroup.CreateLogicalVolume() function assumed that the given name was a mountpoint and would automatically escape it, append 'lv', and append numbers in case of collisions. This made it impossible to use the helper function for LV creation with desired names. Refactor the function and modify all the calls so that now it uses the provided name without validating, escaping, or checking for collisions, and generates a name from the payload's mountpoint when one is not provided. The CreateMountpoint() function's functionality is not affected.
Use the CreateLogicalVolume() helper when creating LVs for the custom partition table. This changes size calculations in tests because each LV is resized to be an integer number of extents, i.e. sizes are rounded up to 4 MiB multiples.
When a volume group's name is not specified in the customizations, the custom partition table creator assigned the names vg00, vg01, etc. A new test is added with multiple volume groups to test this. The scenario of the test is not supported by customizations, the blueprint package does not allow multiple volume groups, but the function can handle it so it's good to have it covered.
One for each variant: - Plain - Plain + btrfs - Plain + lvm
Added extra validation for: - invalid filesystems for specific mountpoints (e.g. xfs on /boot/efi) - /boot or /boot/efi on lvm or btrfs - btrfs as a filesystem on plain Added tests to cover all new validation errors. Added an extra happy test for /boot and /boot/efi with automatic fs type selection (unset, empty strings).
A small function for resolving the filesystem type for a mountpoint with the global default. Will return an error if both are unset.
Split the large NewCustomPartitionTable() function into smaller ones for readability and nicer handling of error conditions (no need to beak on a loop label).
- Legacy boot mode. - UEFI boot mode. - Auto root creation on btrfs.
Change the error message for invalid partition table types to match the other error messages from the NewCustomPartitionTable() function. Add a test.
0084b32
to
6b1f037
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You are fast! 😍
I need one more day to read through user feedback.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lemme go over user feedback that I just got before merging it. :)
@@ -188,6 +188,9 @@ func mkESP(size uint64) Partition { | |||
} | |||
|
|||
type CustomPartitionTableOptions struct { | |||
// PartitionTableType must be either "dos" or "gpt". Defaults to "gpt". | |||
PartitionTableType string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We have an enum-like type for filesystem types, I think it would make sense to be consistent and do the same here instead of strings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, good idea.
// requirements are additive, meaning the minimum size for a mountpoint's | ||
// partition is the sum of all the required directory sizes it will | ||
// contain. | ||
RequiredSizes map[string]uint64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: I'd call it RequiredMinSizes
or maybe reuse FilesystemCustomization
so that it's clear just from the variable name that these are minimums and not an exact size (even though the docstring clearly explains that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RequiredMinSizes
sounds good. I think originally "required" was meant to imply "lower bound"/"minimum requirements" sort of, but adding Min
to the mix makes it clearer.
Wow, great job with this! I only had a couple small comments and one thing I didn't see -- is there anything that catches overrlapping partitions? I didn't notice anything in the validation function or tests. |
The validator in the blueprint normalises mountpoints and checks for duplicates across the whole customization block. It's probably a good idea to have the same checks in the |
This PR adds a new advanced partitioning customization to the blueprint that has the following structure:
The customization is currently enabled only in Fedora. I plan to add support and tests in RHEL and CentOS in a follow-up PR.
When the new customizations are used, the base partition table for the image type is ignored. Instead, a new "custom" partition table is constructed from scratch by iterating through the customizations and creating partitions, volume groups and logical volumes, and btrfs volumes and subvolumes as needed. Boot, EFI, and root partitions are created automatically to make the partition table valid and usable. Parameters such as boot mode and partition table type should be determined by the caller of the function, which should be based on the distro, architecture, and image type. This is, in a way, much simpler than our existing
ensureLVM()
andensureBtrfs()
functions, which needed to modify an existing partition table's structure in a way that preserved some parts and not others.The custom partition table creation function (
NewCustomPartitionTable()
) supports all kinds of valid but perhaps unwanted configurations. For example, it's possible to create a partition table with both LVM volume groups and btrfs volumes, or multiples of each. The customizations however don't allow this, but the structure supports it (see above) in case we want to expand what we support in the future.Customization validation happens in the blueprint. Invalid values such as duplicate mountpoints, empty btrfs subvolume names, etc are caught by a validator in the blueprint code itself.
What's next: