bib: improve the "/" size calculation for LVM #750
Conversation
This commit adds a regression test for the manifest creation
when custom disk are used. When just adding a swap device the
rootlv size is not set/updated correctly which leads to a
manifest like:
```
...
{
"type": "org.osbuild.lvm2.create",
"options": {
"volumes": [
{
"name": "swaplv",
"size": "2147483648B"
},
{
"name": "rootlv",
"size": "0B"
}
]
},
...
```
for a config like:
```
"customizations": {
"disk": {
"partitions": [
{
"type": "lvm",
"minsize": "10 GiB",
"logical_volumes": [
{
"minsize": "2 GiB",
"fs_type": "swap",
}
]
}
]
}
```
When creating custom partitions, include a hardcoded requiredDirectorySizes (taken from images) to get some sensible defaults when the user has no minsizes set for e.g. "/" or "/usr".
I wonder... does |
mvo5
force-pushed
the
tweak-lvm-calculations
branch
from
ffb5f27 to
f639cc6
Compare
mvo5
force-pushed
the
tweak-lvm-calculations
branch
2 times, most recently
from
ba1d513 to
9f71ee5
Compare
mvo5
force-pushed
the
tweak-lvm-calculations
branch
from
9f71ee5 to
367f4ca
Compare
Excellent point, I now remember we disallow /usr in image-mode (c.f. https://github.com/osbuild/bootc-image-builder/blob/main/bib/cmd/bootc-image-builder/image_test.go#L165) so I updated the description and went with making LVM respect minRootSize (I had a bit of an internal debate if the users choice on lvm root for "/" and "minsize" should always trump "rootfsMinSize" but I think it makes most sense to handle this like filesystem customizations and the updateFilesystemSizes there. Sorry for the flip-flopping and the force pushes, I really wanted to get this done before the weekend so maybe was a bit rushing it. |
mvo5
force-pushed
the
tweak-lvm-calculations
branch
2 times, most recently
from
a56043f to
dc1956f
Compare
mvo5
force-pushed
the
tweak-lvm-calculations
branch
from
dc1956f to
32e8a13
Compare
mvo5
force-pushed
the
tweak-lvm-calculations
branch
from
32e8a13 to
3df60d5
Compare
This commit improves the rootfs size calculation for advanced
partitioning. Just like for the tranditional filesystem
customization where we use `updateFilesystemSizes()` to ensure
that the rootfs is at least the two times size of the container
we need to do the same for the advanced disk partitioning.
The disk library will need a `RequiredMinSizes` parameter
(see gh#748) to know what sizes to pick. So far we did not
pass this which can leads to a "0B" root partition.
The naive fix was to just include a hardcoded:
```
requiredDirectorySizes = map[string]uint64{
"/": 1 * datasizes.GiB,
"/usr": 2 * datasizes.GiB,
}
```
but of course that is not ideal because we calculate the
size of disk based on 2x the container size. We set this
for the rootfs so the entire disk is big enough. However
because in advanced partitioning "/" is not automatically
expanded we end up with a potentially too small "/".
This commit tweaks the calculcation so that it sets the
requiredDirectorySizes for "/" to be at least 2x container
size.
Note that a custom "/usr" is not supported in image mode so splitting
rootfsMinSize between / and /usr is not a concern.
mvo5
force-pushed
the
tweak-lvm-calculations
branch
from
3df60d5 to
37a3b8c
Compare
|
The run failed with: it seems we will need osbuild/images#1092 too so that we correctly align when EnsureSize() is called becaue of the requiredMinSizes calculations. |
Mainly to get osbuild/images#1092 to fix the lvm alignment calculation. Co-authored-by: Ondřej Budai <[email protected]
ondrejbudai
force-pushed
the
tweak-lvm-calculations
branch
from
0600441 to
4a6c798
Compare
|
osbuild/images#1092 got released as 0.105.0, so I force-pushed a new go.mod. This should be now ready to go. :) |
Looks good to me as well, thank you! |
This commit improves the rootfs size calculation for advanced
partitioning. Just like for the tranditional filesystem
customization where we use
updateFilesystemSizes()to ensurethat the rootfs is at least the two times size of the container
we need to do the same for the advanced disk partitioning.
The disk library will need a
RequiredMinSizesparameter(see gh#748) to know what sizes to pick. So far we did not
pass this which can leads to a "0B" root partition.
The naive fix was to just include a hardcoded:
but of course that is not ideal because we calculate the
size of disk based on 2x the container size. We set this
for the rootfs so the entire disk is big enough. However
because in advanced partitioning "/" is not automatically
expanded we end up with a potentially too small "/".
This commit tweaks the calculcation so that it sets the
requiredDirectorySizes for "/" to be at least 2x container
size.
Note that a custom "/usr" is not supported in image mode so splitting
rootfsMinSize between / and /usr is not a concern.
Closes: #748