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

test: add regression test/fix for gh748 swap space creation #749

Closed
wants to merge 2 commits into from

Conversation

mvo5
Copy link
Collaborator

@mvo5 mvo5 commented Dec 6, 2024

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",
                            }
                        ]
                    }
                ]
            }

and indeed, we test that config in our unit tests (test_manifest_disk_customization_lvm_swa) but we do not run the full osbuild validation on it :(

Closes: #748

@mvo5 mvo5 changed the title test: add regression test for gh748 swap space creation test: add regression test/fix(?) for gh748 swap space creation Dec 6, 2024
@mvo5 mvo5 marked this pull request as ready for review December 6, 2024 09:01
mvo5 added 2 commits December 6, 2024 11:06
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".
@mvo5 mvo5 changed the title test: add regression test/fix(?) for gh748 swap space creation test: add regression test/fix for gh748 swap space creation Dec 6, 2024
Copy link
Member

@ondrejbudai ondrejbudai left a comment

Choose a reason for hiding this comment

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

This is partly my fault. @achilleas-k originally enforced the disk size everywhere, but I told him that I would like if the disk library is stayed with as few hardcoded values as possible, so he extracted it outside.

Maybe the custom pt generation code in images should check this case?

Additionally, should we still enforce the min size of / of 2x the container image size as we do with the old partitioning? On the other hand, I can totally see cases when people don't want it. I actually fully expect some people not to ever run bootc upgrade because they will spin new instances and terminate the old ones. So they might not like the doubling...

@mvo5
Copy link
Collaborator Author

mvo5 commented Dec 6, 2024

This is partly my fault. @achilleas-k originally enforced the disk size everywhere, but I told him that I would like if the disk library is stayed with as few hardcoded values as possible, so he extracted it outside.

Maybe the custom pt generation code in images should check this case?

I think from an API PoV this is sensible, however ideally we would error in the partitioning if we encounter a zero size partition (IMHO). Right now we allow known broken cases in our API without an error which ideally an API woudl prevent.

Additionally, should we still enforce the min size of / of 2x the container image size as we do with the old partitioning? On the other hand, I can totally see cases when people don't want it. I actually fully expect some people not to ever run bootc upgrade because they will spin new instances and terminate the old ones. So they might not like the doubling...

This is a good point, we should probably fix that too, I think the issue is that for LVM the rules are different. IIRC when we create a disk with a minsize we will up the not-used space for "/". However with LVM we expect that (some) people will want to create LVM partitions that have intentional space left so we do not auto-fill. So maybe this fix is actually wrong^Wnot ideal and indead it should put RootfsMinsize here instead of the hardcoded values. But then we have "/" and /"usr" and if we set both to rootfsMinsize (2x container size) we get a disk that is too big for cases where "/" and "/usr" are defined afaict.

@achilleas-k
Copy link
Member

achilleas-k commented Dec 6, 2024

#750 (comment)

Same question.

@ondrejbudai
Copy link
Member

Addressed by #750

@ondrejbudai ondrejbudai closed this Dec 9, 2024
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.

bib failed to generate raw image when config.json contains disk.swap customization
3 participants