OSInside / kiwi

KIWI - Appliance Builder Next Generation
https://osinside.github.io/kiwi
GNU General Public License v3.0
300 stars 152 forks source link

Added rd.kiwi.oem.skip_resize_check boot option #2579

Closed schaefi closed 3 months ago

schaefi commented 3 months ago

Specifies to skip the check for unallocated space on a GPT partitioned disk. GPT disks holds a backup table at the end of the disk. Dumping an image to a disk that is larger than the image itself will therefore leave a gab between the backup table and the real end of the disk. kiwi uses this situation to detect if there is free space available for a subsequent resize operation. However, smart firmware implementations can also detect this and sometimes moves the backup table but without any proper resize operation. In such a case rd.kiwi.oem.skip_resize_check will force the resize. This Fixes bsc#1224389

schaefi commented 3 months ago

Just small detail for making sure we double think the expected behavior. Wouldn't it make sense having this skip_resize_check flag set to true by default? I am specially thinking in the case where kiwi_oemresizeonce != "true". I have not looked at it that deep but I'd expect the resize process not to harm the filesystem if re-applied again and I think this the expected behavior if kiwi_oemrisizeonce != "true". I am not aware of current use cases of this kiwi_oemreizeonce flag though, so it can easily be I doing wrong assumptions.

I'm not sure if I can follow you. The use case for resize-once is to resize only once no matter if the disk geometry changes again afterwards. This feature is rarely used and I think it was added for people who wanted to let kiwi do the initial resize of the disk but then use another resize software to handle subsequent disk geometry changes.

but I'd expect the resize process not to harm the filesystem if re-applied again

This is correct, it never harms the filesystem but it is an unneeded block I/O operation without any outcome if there are no extra blocks available. Hence we have this simple check for the location of the backup GPT. If resize-once is active we do not reach this check if the resize already has happened

I currently can't find a path where it does the wrong thing, can you give me more details ?

Thanks

tamara-schmitz commented 3 months ago

As I explained in PR #2576 which gets rid of running sgdisk --verify and that you closed, currently the behaviour of the dracut resizing script this:

it is assumed that a correctly expanded GPT table means that no resizing needs to happen. that is why i opened the PR.

This is correct, it never harms the filesystem but it is an unneeded block I/O operation without any outcome if there are no extra blocks available. Hence we have this simple check for the location of the backup GPT. If resize-once is active we do not reach this check if the resize already has happened

Can you please elaborate more on this unneed block i/o? What is your goal of your optimisation?

I have not checked the source code of sgdisk but according to the manual page, the operation -e is supposed to move the GPT header to the end of the disk. Just like running --verify that requires reading in the existing headers, interpreting them and then if necessary rewriting them. Running the command twice would lead to this reading process to be done twice I would presume? I can collect an strace if wanted.

Another piece of feedback, the option name skip_resize_check is in my opinion too open ended and does not explain the change that the option does. It is supposed to skip checking the GPT table itself right? Then how about skip_gpt_resize_check?

schaefi commented 3 months ago

it is assumed that a correctly expanded GPT table means that no resizing needs to happen. that is why i opened the PR.

Well I'm having a hard time to follow your lines of thought. The PR you opened simply dropped all code to check the location of the backup GPT, which we cannot do, sorry. Instead I created a PR that skips the method you deleted by a new boot option called rd.kiwi.oem.skip_resize_check and also documented it. What's wrong with that, it does the exact same you proposed just a bit less destructive

Can you please elaborate more on this unneed block i/o? What is your goal of your optimisation?

In your and in my PR we will skip the code that actually checks if a resize makes sense. When doing that the resize happens in any case no matter if it's useful or not. The operation to resize covers block I/O to read/write the partition table, call the respective tools to resize the filesystem and all it's associated layers and all these operations will be called and ends a micro second later without any result because the system is already resized. If not needed I would like to avoid this calls and that's what the check on the location of the GPT backup table was handy for. Because as you said the --verify just reads the current table and the real device geometry but does not perform further write or ioctl actions to it. This is also not done for an optimization but for good quality implementations that should not call tools when it doesn't make sense to call them

Another piece of feedback, the option name skip_resize_check is in my opinion too open ended and does not explain the change that the option does. It is supposed to skip checking the GPT table itself right? Then how about skip_gpt_resize_check?

rd.kiwi.oem.skip_resize_check in my opinion does what the names says "it skips the resize check for oem disks". What it does behind the scenes is explained in the documentation to this option. I'm fine with updating those if you think it does not explain things properly ? However, I believe an option name should give an impression what it does on a higher level and has not the responsibility to outline details on the affected technology bits. That part I see as a responsibility of the docs. With the option we want to give users a way to skip the resize check and I would like to use the same option when needed for other partition types, e.g think of s390 and DASD devices, or maybe there will be an opportunity for the old msdos table too, etc etc. In such cases I don't think adding more options for the same would be helpful.

Thoughts ?

schaefi commented 3 months ago

According to the conversation here I have changed the pull request and pushed a commit that change the name and meaning of the proposed boot option. It is now available as:

Forces the disk resize process on an OEM disk image. If set, no sanity
check for unpartitioned/free space is performed and also an eventually
configured `<oem-resize-once>` configuration from the image description
will not be taken into account. The disk resize will be started which
includes re-partition as well as all steps to resize the block layers
up to the filesystem holding the data. As `rd.kiwi.oem.force_resize`
bypasses all sanity checks to detect if such a resize process is
needed or not, it can happen that all program calls of the resize
process ends without any effect if the disk is already properly
resized. It's also important to understand that the partition UUIDs
will change on every resize which might be an unwanted side effect
of a forced resize.

@tamara-schmitz @davidcassany Are you more in favor of this setting and docs to achieve the desired behavior ?

Thanks

schaefi commented 3 months ago

Thanks much David. Then we are only seeking for Tamara's (@tamara-schmitz) ok :smiley: