Partition Table, Stages, Mounts, and Devices generator commands for otk #732
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mvo5
previously approved these changes
Jun 7, 2024
| @@ -18,6 +19,44 @@ type Partition struct { | |||
| Payload Entity | |||
| } | |||
|
|
|||
| // XXX: this needs refactoring and tests, it's kinda complicated | |||
There was a problem hiding this comment.
I will work on this in a followup/parallel PR, this is tested indirectly via the otk-make-* tests but of course a direct test is much better and will get added. Note that this is only used by the otk-make-* tooling right now so the risk to merge in the current state is low.
mvo5
previously approved these changes
Jun 7, 2024
ondrejbudai
reviewed
Jun 16, 2024
| "github.com/osbuild/images/pkg/osbuild" | ||
| ) | ||
|
|
||
| type Input struct { |
There was a problem hiding this comment.
Since this basically becomes our public API, I think that all of these have to be documented with docstrings.
ondrejbudai
reviewed
Jun 16, 2024
ondrejbudai
requested changes
Jun 16, 2024
| } | ||
|
|
||
| type InputUEFI struct { | ||
| Size string `json:"size"` |
There was a problem hiding this comment.
seeing UEFI.Size in code isn't very descriptive. What about calling this field esp_size? Do we actually need this under the InputUEFI struct? I think it can live directly under InputProperties.
There was a problem hiding this comment.
I was thinking about this and the BIOSBootPartition and wonder if we should change the structure a little bit as both of those are about automatically creating partitions, maybe something like:
diff --git a/cmd/otk-gen-partition-table/main.go b/cmd/otk-gen-partition-table/main.go
index 496914894..6d78fb2f9 100644
--- a/cmd/otk-gen-partition-table/main.go
+++ b/cmd/otk-gen-partition-table/main.go
@@ -27,17 +27,20 @@ type Input struct {
// InputProperties contains global properties of the partition table
type InputProperties struct {
- UEFI InputUEFI `json:"uefi"`
- BIOS bool `json:"bios"`
Type otkdisk.PartType `json:"type"`
DefaultSize string `json:"default_size"`
UUID string `json:"uuid"`
+ Create InputCreate `json:"create"`
SectorSize uint64 `json:"sector_size"`
}
-// InputUEFI contains the uefi specific bits of the partition input
-type InputUEFI struct {
+// InputAutogenerate contains details what partitions to auto-generate
+type InputCreate struct {
+ BIOSBootPartition bool `json:"bios-boot-partition"`
+ EspPartition bool `json:"esp-partition"`
+ EspPartitionSize string `json:"esp-partition-size"`
+
Size string `json:"size"`
}
@@ -111,7 +114,7 @@ func makePartitionTableFromOtkInput(input *Input) (*disk.PartitionTable, error)
Type: string(input.Properties.Type),
SectorSize: input.Properties.SectorSize,
}
- if input.Properties.BIOS {
+ if input.Properties.Create.BIOSBootPartition {
if len(pt.Partitions) > 0 {
return nil, fmt.Errorf("internal error: bios partition *must* go first")
}
@@ -122,8 +125,13 @@ func makePartitionTableFromOtkInput(input *Input) (*disk.PartitionTable, error)
UUID: disk.BIOSBootPartitionUUID,
})
}
- if input.Properties.UEFI.Size != "" {
- uintSize, err := common.DataSizeToUint64(input.Properties.UEFI.Size)
+ if input.Properties.Create.EspPartition {
+ size := input.Properties.Create.EspPartitionSize
+ if size == "" {
+ // XXX: is this a good default?
+ size = "1 Gb"
+ }
+ uintSize, err := common.DataSizeToUint64(size)
if err != nil {
return nil, err
}alternative names might be InputAutoCreate, InputAutoGenerate or similar.
| } | ||
| }() | ||
|
|
||
| // rhel7 uses PTSgdisk, if we ever need to support this, make this |
There was a problem hiding this comment.
So basically we need to wrap the input in an extra layer, so we can accept more arguments, right?
There was a problem hiding this comment.
AFAICT we are safe here, if we need to support another tool we would do something like:
diff --git a/cmd/otk-make-partition-stages/main.go b/cmd/otk-make-partition-stages/main.go
index 66c25349a..6d6f76bff 100644
--- a/cmd/otk-make-partition-stages/main.go
+++ b/cmd/otk-make-partition-stages/main.go
@@ -20,10 +20,7 @@ func makeImagePrepareStages(inp Input, filename string) (stages []*osbuild.Stage
}
}()
- // rhel7 uses PTSgdisk, if we ever need to support this, make this
- // configurable
- partTool := osbuild.PTSfdisk
- stages = osbuild.GenImagePrepareStages(inp.Const.Internal.PartitionTable, inp.Const.Filename, partTool)
+ stages = osbuild.GenImagePrepareStages(inp.Const.Internal.PartitionTable, inp.Const.Filename, osbuild.PartTool(inp.Const.Internal.PartTool))
return stages, nil
}
diff --git a/internal/otkdisk/partition.go b/internal/otkdisk/partition.go
index 37b649c5c..33c199885 100644
--- a/internal/otkdisk/partition.go
+++ b/internal/otkdisk/partition.go
@@ -50,6 +50,7 @@ type Partition struct {
// "otk-{gen,make}-*" tools for their data exchange.
type Internal struct {
PartitionTable *disk.PartitionTable `json:"partition-table"`
+ PartTool string `json:"part-tool"`
}
// PartType represents a partition typeand from otk-gen-* we would set it (or move it out of internal if it should be something the user should control). Or am I misunderstanding the comment here?
| type partAlias Partition | ||
| var aux struct { | ||
| partAlias | ||
| Payload struct { |
There was a problem hiding this comment.
Payload can be nil. A great example is the bios boot partition. A quick example:
$ cat test.json
{
"partitions": [
{
"fs_mntops": "defaults",
"label": "root",
"mountpoint": "/",
"size": "2 GiB",
"type": "xfs"
}
],
"properties": {
"bios": true,
"default_size": "4 GiB",
"sector_size": 512,
"type": "gpt",
"uefi": {
"size": "512 MiB"
}
}
}
$ ./otk-gen-partition-table <test.json | ./otk-make-partition-stages
error: cannot detect type for {
"Start": 1048576,
"Size": 1048576,
"Type": "21686148-6449-6E6F-744E-656564454649",
"Bootable": true,
"UUID": "FAC7F1FB-3E8D-4137-A512-961DE09A5549",
"Payload": null
}
There was a problem hiding this comment.
Thank you! This part of the code will be replaced with #737 and I added code to handle this case now (it was not doing that before, so a huge thank you for providing this example).
There was a problem hiding this comment.
Might be easiest to review #737 first and then I can just rebase on top of this and drop this initial and slightly naive implementation for the unmarshal (fwiw, I wrote it so I'm entitled to call it "naive" :)
New Partition Table generator command that makes the internal generation code available to external tooling (otk). The command takes as input a base partition table (like the ones defined in pkg/distro/**/partition-tables.go) and a set of options and customizations. These match the arguments to the disk.NewPartitionTable() function. The output is a fully resolved Partition Table (with all UUIDs, start and offset values, and mount options defined), a set of kernel options, and a partition map. The kernel options are useful for some stages that require adding kernel arguments that include the root partition UUID. The partition map is a convenient mapping of names to partitions with predictable names (root and boot) so that otk and other externals can access properties specific to those partitions. The fully resolved Partition Table itself is nested under the "internal" key. This is meant to signify that the representation of the full Partition Table is an internal data structure that shouldn't be considered stable or usable by otk itself. Instead, it is meant to be fed back into other commands that will use it to generate useful data structures (like stage options, mounts, and devices). Co-authored-by: Achilleas Koutsou <achilleas@koutsou.net>
Make run() do the scaffolding like reading from stdin/outputing to stdout and extract a helper `genPartitionTable()` that does the work with real structures.
The struct names for the inputs/outputs of the otk-gen-partition-table are not very consistent. This commit changes this so that there is (a bit) more structure and symmetry. It also drops the Otk prefix since the types aren't really importable or reusable.
This commit adds a new `modifications` field to the input of the
`otk-gen-partition-table` that can be used to tweak the output of the
partition generator.
The inputs follow the inputs from the existing customizations, so
the json input looks like this:
```json
{
...
"modifications": {
"partition_mode": "raw",
"filesystems": [
{"mountpoint": "/var/log", "minsize": 10241024}
]
}
}
```
and the otk snippet would look something like:
```yaml
filesystem:
otk.external.gen_partition_table:
options: ...
partitions: ...
modifications:
partition_mode: ${modifications.partition_mode}
filesystems: ${modfications.filesystems}
```
and the user would override them via:
```
cat > mymod.json <<'EOF'
{
"modifications": {
"partition_mode": "raw",
"filesystems: [ {"mountpoint": "/var/log", "minsize": 10241024 } ]
}
$ otk compile --modifications=mymod.json centos-9-x86_64-ami.otk
```
Also, renames InputOptions ("options") to InputProperties ("properties")
to clarify that these fields are not configuration options that can be
be used to modify partition tables but instead should be properties tied
to the base partition table itself and changes to these should be
handled through the modifications.
Support: - `modification.min_disk_size` - `properties.default_size` during the partitioning
The otk internal package will hold common types and functions for writing otk externals.
New Stages generator for creating the stages that can realise a
filesystem based on a Partition Table.
This external consumes the data from `otk-gen-partition-table` and
generate osbuild stages from it.
The expected usage is something like:
```yaml
otk.define:
modifications:
filesystem:
min_disk_size: 20 GiB
otk.define.defaults:
filesystem:
otk.external.gen_partition_table:
modifications: ${modifications.filesystem}
properties: ...
partitions: ...
stages:
...
otk.external.make_partition_stages:
internal: ${filesystem.internal}
# TODO: add filename here
```
We will need to call this from a new otk external cmd to generate the devices and mounts.
New Mounts and Devices generator for creating the mount and device options that can be attached to stages that need to operate on a partitioned image file. The external consumes the data from `otk-gen-partition-table` and generates osbuild devices and mounts from it.
This commit moves some more things into the new `otkdisk` helper package so that all otk externals use the same data structures.
The otk-make-partition-{stages,mount-devices} externals both need the
filename modification. It seems easiest to just define it in one place
in `otk-gen-partition-table` and then use the outputs from that.
Add a new type otkdisk.PartType with the following consts
otkdisk.PartType{Unset,GPT,DOS} and validation support.
Also adds (extremly basic) validation for the partition table input.
We need to be able to set both the partition and the filesystem UUID. Hence split and rename the existing Partition.UUID (thanks to Ondrej).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a joint PR by @mvo5 and me that adds three new commands:
disk.NewPartitionTable()function.osbuild.GenImagePrepareStages()function.osbuild.GenMountsDevicesFromPT()function.Some tests can still be added and it might be good to test these with otk directly before merging, but they're mostly functionally complete.