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

overlay lib: Secure the creation of a disk image when size is < 130 #1103

Merged
merged 1 commit into from
Aug 17, 2023

Conversation

pirat89
Copy link
Member

@pirat89 pirat89 commented Jul 31, 2023

In case the filesystem for which the disk img is going to be created has very small amount of free space (under 130 MiBs), it cannot be formatted by XFS with current params.

This could be hit in several cases:

  • the system partition/volume - in this case, most likely an issue will be hit anyway later by DNF speaking about small amount of free space if a content is installed inside by RPMs as such a small amount of free space is really not expected to see at all
  • it's a data mount point (e.g. iso) or a filesystem type that should by part of the OVERLAY_DO_NOT_MOUNT set, so enlarging the value to 130 MiBs should not affect anything negatively at all
  • in case of /boot, the problem with the free space is covered already in a different actor prior we try to create any disk img, so we are safe here

Based on arguments above, I am considering setting the 130 MiBs as minimal value safe for in-place upgrades. Also it will allow to skip possible problems with specific file systems (like tmpfs, ...) in case we are still missing some in the OVERLAY_DO_NOT_MOUNT - and kind of read only storage (such as iso9660, etc..).

JIRA: OAMG-9463

Orig error

Also updating the error msg when a FS cannot be created as originally it contained just:

2023-07-31 12:31:27.48  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: External command has started: ['/bin/dd', 'if=/dev/zero', 'of=/var/lib/leapp/scratch/diskimages/root_home', 'bs=1M', 'count=0', 'seek=100']
0+0 records in
0+0 records out
0 bytes (0 B) copied, 9.9531e-05 s, 0.0 kB/s
2023-07-31 12:31:27.57  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: External command has finished: ['/bin/dd', 'if=/dev/zero', 'of=/var/lib/leapp/scratch/diskimages/root_home', 'bs=1M', 'count=0', 'seek=100']
2023-07-31 12:31:27.58  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: Creating XFS filesystem in disk image at /var/lib/leapp/scratch/diskimages/root_home
2023-07-31 12:31:27.59  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: External command has started: ['/sbin/mkfs.xfs', '-l', 'size=32m', '-f', '/var/lib/leapp/scratch/diskimages/root_home']
internal log size 8192 too large, must fit in allocation group
Usage: mkfs.xfs
/* blocksize */		[-b log=n|size=num]
/* metadata */		[-m crc=0|1,finobt=0|1,uuid=xxx]
/* data subvol */	[-d agcount=n,agsize=n,file,name=xxx,size=num,
			    (sunit=value,swidth=value|su=num,sw=num|noalign),
			    sectlog=n|sectsize=num
/* force overwrite */	[-f]
/* inode size */	[-i log=n|perblock=n|size=num,maxpct=n,attr=0|1|2,
			    projid32bit=0|1]
/* no discard */	[-K]
/* log subvol */	[-l agnum=n,internal,size=num,logdev=xxx,version=n
			    sunit=value|su=num,sectlog=n|sectsize=num,
			    lazy-count=0|1]
/* label */		[-L label (maximum 12 characters)]
/* naming */		[-n log=n|size=num,version=2|ci,ftype=0|1]
/* no-op info only */	[-N]
/* prototype file */	[-p fname]
/* quiet */		[-q]
/* realtime subvol */	[-r extsize=num,size=num,rtdev=xxx]
/* sectorsize */	[-s log=n|size=num]
/* version */		[-V]
			devicename
<devicename> is required unless -d name=xxx is given.
<num> is xxx (bytes), xxxs (sectors), xxxb (fs blocks), xxxk (xxx KiB),
      xxxm (xxx MiB), xxxg (xxx GiB), xxxt (xxx TiB) or xxxp (xxx PiB).
<value> is xxx (512 byte blocks).
2023-07-31 12:31:27.88  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: Command ['/sbin/mkfs.xfs', '-l', 'size=32m', '-f', '/var/lib/leapp/scratch/diskimages/root_home'] failed with exit code 1.
2023-07-31 12:31:27.90  DEBUG    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: External command has finished: ['/sbin/mkfs.xfs', '-l', 'size=32m', '-f', '/var/lib/leapp/scratch/diskimages/root_home']
2023-07-31 12:31:27.91  ERROR    PID: 15851 leapp.workflow.TargetTransactionFactsCollection.target_userspace_creator: Failed to create XFS filesystem /var/lib/leapp/scratch/diskimages/root_home
Traceback (most recent call last):
  File "/usr/share/leapp-repository/repositories/system_upgrade/common/libraries/overlaygen.py", line 386, in _format_disk_image_xfs
    utils.call_with_oserror_handled(cmd=cmd)
  File "/usr/share/leapp-repository/repositories/system_upgrade/common/libraries/utils.py", line 119, in call_with_oserror_handled
    run(cmd)
  File "/usr/lib/python2.7/site-packages/leapp/libraries/stdlib/__init__.py", line 188, in run
    result=result
CalledProcessError: Command ['/sbin/mkfs.xfs', '-l', 'size=32m', '-f', '/var/lib/leapp/scratch/diskimages/root_home'] failed with exit code 1.
....
============================================================
                           ERRORS                           
============================================================

2023-07-31 12:31:27.096968 [ERROR] Actor: target_userspace_creator
Message: Command ['/sbin/mkfs.xfs', '-l', 'size=32m', '-f', '/var/lib/leapp/scratch/diskimages/root_home'] failed with exit code 1.

============================================================
                       END OF ERRORS                        
============================================================

which has not been saying so much. It should be now better in any way when a FS cannot be created for whatever reason

@pirat89 pirat89 added this to the 8.9/9.3 milestone Jul 31, 2023
@github-actions
Copy link

Thank you for contributing to the Leapp project!

Please note that every PR needs to comply with the Leapp Guidelines and must pass all tests in order to be mergeable.
If you want to request a review or rebuild a package in copr, you can use following commands as a comment:

  • review please @oamg/developers to notify leapp developers of the review request
  • /packit copr-build to submit a public copr build using packit

Packit will automatically schedule regression tests for this PR's build and latest upstream leapp build. If you need a different version of leapp from PR#42, use /packit test oamg/leapp#42

To launch regression testing public members of oamg organization can leave the following comment:

  • /rerun to schedule basic regression tests using this pr build and latest upstream leapp build as artifacts
  • /rerun 42 to schedule basic regression tests using this pr build and leapp*PR42* as artifacts
  • /rerun-sst to schedule sst tests using this pr build and latest upstream leapp build as artifacts
  • /rerun-sst 42 to schedule sst tests using this pr build and leapp*PR42* as artifacts

Please open ticket in case you experience technical problem with the CI. (RH internal only)

Note: In case there are problems with tests not being triggered automatically on new PR/commit or pending for a long time, please contact leapp-infra.

@pirat89 pirat89 requested a review from MichalHe July 31, 2023 09:40
@pirat89
Copy link
Member Author

pirat89 commented Jul 31, 2023

@oamg/developers another aproach would be just to keep the error message. but I started to think about cases that we are still missing and that there could be actually some mountpoints that cannot be enlarged (like iso...not sure why someone would put that in fstab, but...) or mounpoints that we should actually ignore. wdyt about that? In case we do not want to set a minimum limit, we can just update the error msg in a way we inform user what partition/volume does not contain enough free space.

Need to still keep in mind that this whole problem is about corner cases.

@pirat89 pirat89 force-pushed the overlay-fix-fs-creation branch from ed140e2 to a752695 Compare July 31, 2023 09:45
@pirat89
Copy link
Member Author

pirat89 commented Jul 31, 2023

/rerun

@pirat89 pirat89 changed the title overlay lib: Secure the creation of the disk image when size is < 130 overlay lib: Secure the creation of a disk image when size is < 130 Jul 31, 2023
@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6225055

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/6225055 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/6225055 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89 pirat89 requested a review from fernflower July 31, 2023 11:14
@MichalHe MichalHe self-assigned this Aug 16, 2023
@MichalHe
Copy link
Member

/packit build

Copy link
Member

@MichalHe MichalHe left a comment

Choose a reason for hiding this comment

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

I've tested the patch with a 256mb partition that contains 200mb file, and the patch works as expected - preupgrade finished without problems, whereas without this patch the preupgrade fails.

Logs produced by this patch:

2023-08-16 14:45:48.454 WARNING  PID: 15114 leapp.workflow.TargetTransactionCheck.dnf_transaction_check: The apparent size for the disk image representing {path} is too small ({} MiBs) for a formatting. Setting 130 MiBs instead.
2023-08-16 14:45:48.455 DEBUG    PID: 15114 leapp.workflow.TargetTransactionCheck.dnf_transaction_check: Attempting to create disk image at /var/lib/leapp/scratch/diskimages/root_mnt
2023-08-16 14:45:48.456 DEBUG    PID: 15114 leapp.workflow.TargetTransactionCheck.dnf_transaction_check: External command has started: ['/bin/dd', 'if=/dev/zero', 'of=/var/lib/leapp/scratch/diskimages/root_mnt', 'bs=1M', 'count=0', 'seek=130']

@pirat89 please fix the minor issues found and I will approve this no problemo.

repos/system_upgrade/common/libraries/overlaygen.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/libraries/overlaygen.py Outdated Show resolved Hide resolved
repos/system_upgrade/common/libraries/overlaygen.py Outdated Show resolved Hide resolved
In case the filesystem for which the disk img is going to be created
has very small amount of free space (under 130 MiBs), it cannot be
formatted by XFS with current params.

This could be hit in several cases:
 * the system partition/volume - in this case, most likely an issue
   will be hit anyway later by DNF speaking about small amount of
   free space if a content is installed inside by RPMs
   as such a small amount of free space is really not expected
   to see at all
 * it's a data mount point (e.g. iso) or a filesystem type that should
   by part of the OVERLAY_DO_NOT_MOUNT set, so enlarging the value
   to 130 MiBs should not affect anything negatively at all
 * in case of /boot, the problem with the free space is covered
   already in a different actor prior we try to create any disk img,
   so we are safe here

Based on arguments above, I am considering setting the 130 MiBs as
minimal value safe for in-place upgrades. Also it will allow to skip
possible problems with specific file systems (like tmpfs, ...) in case
we are still missing some in the OVERLAY_DO_NOT_MOUNT - and kind of
read only storage (such as iso9660, etc..).

Co-authored-by: Michal Hečko <[email protected]>
@pirat89 pirat89 force-pushed the overlay-fix-fs-creation branch from dc7d836 to d3d8037 Compare August 16, 2023 14:59
@MichalHe
Copy link
Member

/rerun

@github-actions
Copy link

Copr build succeeded: https://copr.fedorainfracloud.org/coprs/build/6311223

@github-actions
Copy link

Testing Farm request for RHEL-8.6-rhui/6311223 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@github-actions
Copy link

Testing Farm request for RHEL-7.9-rhui/6311223 regression testing has been created.
Once finished, results should be available here.
Full pipeline log.

@pirat89
Copy link
Member Author

pirat89 commented Aug 17, 2023

Tests passed. Merging \o

@pirat89 pirat89 added the changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant label Aug 17, 2023
@pirat89 pirat89 merged commit a81ebb0 into oamg:master Aug 17, 2023
@pirat89 pirat89 deleted the overlay-fix-fs-creation branch August 17, 2023 13:51
pirat89 added a commit to pirat89/leapp-repository that referenced this pull request Aug 23, 2023
## Packaging
- Requires leapp-framework 5.0

## Upgrade handling
### Fixes
- Add el8toel9 actor to handle directory -> symlink with ruby IRB. (oamg#1076)
- Do not try to update GRUB core on IBM Z systems (oamg#1117)
- Fix failing upgrades with devtmpfs file systems specified in FSTAB (oamg#1090)
- Fix the calculation of the required free space on each partitions/volume for the upgrade transactions (oamg#1097)
- Fix the generation of the report about hybrid images (oamg#1064)
- Handle correctly the installed certificates to allow upgrades with custom repositories using HTTPs with enabled SSL verification (oamg#1106)
- Minor improvements and fixes of various reports (oamg#1066, oamg#1067, oamg#1085)
- Update error messages about leapp data files to inform user how to obtain valid data files (oamg#1121)
- Update links in various reports (oamg#1062, oamg#1086)
- Update the repomap data to cover changed repoids in RHUI Azure (oamg#1087)
- [IPU 7 -> 8] Fix false positive report about invalid symlinks on RHEL 7 (oamg#1052)
- [IPU 8 -> 9] Inhibit the upgrade when unsupported x86-64 microarchitecture is detected (oamg#1059)

### Enhancements
- Include updated leapp data files in the RPM (oamg#1046, oamg#1092, oamg#1119)
- Update the set of supported upgrade paths (oamg#1077):
  - RHEL with SAP HANA 7.9 -> 8.6, 8.8 (default: 8.6)
  - RHEL with SAP HANA 8.8 -> 9.2
- Introduce new upgrade paths:
  - RHEL 7.9 -> 8.9 (default)
  - RHEL 8.9 -> 9.3
- Correctly update grub2 when /boot resides on multiple devices aggregated in RAID (oamg#1093, oamg#1115)
- Enable upgrades for machines using RHUI on AlibabaCloud (oamg#1088)
- Introduce possibility to add kernel drivers to initramfs (oamg#1081)
- Redesign handling of information about kernel (booted and target) in preparation for new changes in RHEL 9 (oamg#1107)
- Redesign source system overlay to use disk images backed by sparse files to optimize disk space consumption (oamg#1097, oamg#1103)
- Requires leapp-framework 5.0 (oamg#1061, oamg#1116)
- Use new leapp CLI API which provides better report summary output (oamg#1061, oamg#1116)
- [IPU 8 -> 9] Detect and report use of deprecated Xorg drivers (oamg#1078)
- [IPU 8 -> 9] Introduce IPU for systems with FIPS enabled (oamg#1053)

## Additional changes interesting for devels
- Deprecated `GrubInfo.orig_device_name` field in the `GrubInfo` model (replaced by `GrubInfo.orig_devices`) (oamg#1093)
- Deprecated `InstalledTargetKernelVersion` model (replaced by `InstalledTargetKernelInfo`) (oamg#1107)
- Deprecated `leapp.libraries.common.config.version.is_rhel_realtime` (check the type in msg `KernelInfo`, field `type`) (oamg#1107)
- Deprecated `leapp.libraries.common.grub.get_grub_device()` (replaced by `leapp.libraries.common.grub.get_grub_devices()`) (oamg#1093)
- Introduced new devel envar LEAPP_DEVEL_KEEP_DISK_IMGS=1 to skip the removal of the created disk images for OVL. That's sometimes handy for the debugging. (oamg#1097)
@pirat89 pirat89 mentioned this pull request Aug 23, 2023
Rezney pushed a commit that referenced this pull request Aug 23, 2023
## Packaging
- Requires leapp-framework 5.0

## Upgrade handling
### Fixes
- Add el8toel9 actor to handle directory -> symlink with ruby IRB. (#1076)
- Do not try to update GRUB core on IBM Z systems (#1117)
- Fix failing upgrades with devtmpfs file systems specified in FSTAB (#1090)
- Fix the calculation of the required free space on each partitions/volume for the upgrade transactions (#1097)
- Fix the generation of the report about hybrid images (#1064)
- Handle correctly the installed certificates to allow upgrades with custom repositories using HTTPs with enabled SSL verification (#1106)
- Minor improvements and fixes of various reports (#1066, #1067, #1085)
- Update error messages about leapp data files to inform user how to obtain valid data files (#1121)
- Update links in various reports (#1062, #1086)
- Update the repomap data to cover changed repoids in RHUI Azure (#1087)
- [IPU 7 -> 8] Fix false positive report about invalid symlinks on RHEL 7 (#1052)
- [IPU 8 -> 9] Inhibit the upgrade when unsupported x86-64 microarchitecture is detected (#1059)

### Enhancements
- Include updated leapp data files in the RPM (#1046, #1092, #1119)
- Update the set of supported upgrade paths (#1077):
  - RHEL with SAP HANA 7.9 -> 8.6, 8.8 (default: 8.6)
  - RHEL with SAP HANA 8.8 -> 9.2
- Introduce new upgrade paths:
  - RHEL 7.9 -> 8.9 (default)
  - RHEL 8.9 -> 9.3
- Correctly update grub2 when /boot resides on multiple devices aggregated in RAID (#1093, #1115)
- Enable upgrades for machines using RHUI on AlibabaCloud (#1088)
- Introduce possibility to add kernel drivers to initramfs (#1081)
- Redesign handling of information about kernel (booted and target) in preparation for new changes in RHEL 9 (#1107)
- Redesign source system overlay to use disk images backed by sparse files to optimize disk space consumption (#1097, #1103)
- Requires leapp-framework 5.0 (#1061, #1116)
- Use new leapp CLI API which provides better report summary output (#1061, #1116)
- [IPU 8 -> 9] Detect and report use of deprecated Xorg drivers (#1078)
- [IPU 8 -> 9] Introduce IPU for systems with FIPS enabled (#1053)

## Additional changes interesting for devels
- Deprecated `GrubInfo.orig_device_name` field in the `GrubInfo` model (replaced by `GrubInfo.orig_devices`) (#1093)
- Deprecated `InstalledTargetKernelVersion` model (replaced by `InstalledTargetKernelInfo`) (#1107)
- Deprecated `leapp.libraries.common.config.version.is_rhel_realtime` (check the type in msg `KernelInfo`, field `type`) (#1107)
- Deprecated `leapp.libraries.common.grub.get_grub_device()` (replaced by `leapp.libraries.common.grub.get_grub_devices()`) (#1093)
- Introduced new devel envar LEAPP_DEVEL_KEEP_DISK_IMGS=1 to skip the removal of the created disk images for OVL. That's sometimes handy for the debugging. (#1097)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog-checked The merger/reviewer checked the changelog draft document and updated it when relevant
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants