Skip to content
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

Fix size calculation for containers without sizeable children [COMPOSER-2086] #293

Merged
merged 4 commits into from
Dec 1, 2023

Conversation

achilleas-k
Copy link
Member

The Container size calculation is meant to calculate the size of all the
children of a Container + any metadata in order to:

  1. Ensure the size of the container itself is big enough, if it is a
    Sizeable.
  2. Send the new desired size up the branch for the ancestor Entity
    calculations.

A problem occurs when a Container doesn't have access to the size
information of its descendants. The child size sum is 0, so the
container size is calculated as only the size of any metadata it
requires.

This occurs in the case of a LUKS container with an LVM Volume Group in
it. The LUKS container's child size calculation is 0, the containerSize
is therefore just the MetadataSize() of the LUKS container and so no
decision is made to resize because the containerSize (which is just
MetadataSize) is always smaller than the desired size (which is actually
the size of the children of the VolumeGroup calculated by the previous
recursive execution of the function).

Fix this by detecting when the children size calculation is 0 and
setting the containerSize to the desired size instead.

This changes the logic of the function a bit when it's called directly
on such a container. If we now call
resizeEntityBranch(pathToLUKS, size)
the parent of the LUKS container will be resized to size + 16 MiB (the
LUKS metadata size). This makes sense in our context, because it
ensures that the partition has enough space for 'size' data + the LUKS
metadata.

supakeen
supakeen previously approved these changes Dec 1, 2023
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat. (Container is such an overloaded term).

@achilleas-k
Copy link
Member Author

Neat. (Container is such an overloaded term).

Yeah and naming is HARD!

@say-paul
Copy link
Member

say-paul commented Dec 1, 2023

I tested it against my PR, all green here.

say-paul
say-paul previously approved these changes Dec 1, 2023
thozza
thozza previously approved these changes Dec 1, 2023
Copy link
Member

@thozza thozza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch and great work 👏

I found just one typo, otherwise, this is great 😍

pkg/disk/disk_test.go Outdated Show resolved Hide resolved
Add some comments to make it easier to follow some functions' logic.

COMPOSER-2086
Add LV and VG partition size checks to the LVM custom mountpoint
creation test.

COMPOSER-2086
If the VG is in a LUKS container, then add the LUKS header to the sum of
LVs to verify that the LVs + the header fit on the partition.

COMPOSER-2086
The Container size calculation is meant to calculate the size of all the
children of a Container + any metadata in order to:
1. Ensure the size of the container itself is big enough, if it is a
   Sizeable.
2. Send the new desired size up the branch for the ancestor Entity
   calculations.

A problem occurs when a Container doesn't have access to the size
information of its descendants.  The child size sum is 0, so the
container size is calculated as only the size of any metadata it
requires.

This occurs in the case of a LUKS container with an LVM Volume Group in
it.  The LUKS container's child size calculation is 0, the containerSize
is therefore just the MetadataSize() of the LUKS container and so no
decision is made to resize because the containerSize (which is just
MetadataSize) is always smaller than the desired size (which is actually
the size of the children of the VolumeGroup calculated by the previous
recursive execution of the function).

Fix this by detecting when the children size calculation is 0 and
setting the containerSize to the desired size instead.

This changes the logic of the function a bit when it's called directly
on such a container.  If we now call
  resizeEntityBranch(pathToLUKS, size)
the parent of the LUKS container will be resized to size + 16 MiB (the
LUKS metadata size).  This makes sense in our context, because it
ensures that the partition has enough space for 'size' data + the LUKS
metadata.

COMPOSER-2086
@supakeen supakeen added this pull request to the merge queue Dec 1, 2023
Merged via the queue into osbuild:main with commit 4701f3d Dec 1, 2023
9 checks passed
@achilleas-k achilleas-k deleted the fix/pt-lvm-size branch December 1, 2023 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants