[mic][iso] Added the ability to customize an existing mic-generated iso. by gmileka · Pull Request #8785 · microsoft/azurelinux

gmileka · 2024-04-12T22:23:44Z

Merge Checklist

All boxes should be checked before merging the PR (just tick any boxes which don't apply to this PR)

Summary

This PR allows MIC to take a mic-generated LiveOS iso and customize it.
Customization spans both the squashfs os image as well as the iso file system.

Change Log

[mic][iso] Added the ability to customie an existing mic-generated iso.

Does this affect the toolchain?

NO

Associated issues

10 - [Existing LiveOS ISO] Add support for customizing the ISO file system contents
12 - [Existing LiveOS ISO] Add support for customizing LiveOS root file system (squashfs)

Links to CVEs

none

Test Methodology

Created the following flavors and ensured they all boot successfully and we can log-in with mic/ci credentials:
- vhdx->vhdx
  - os customizations
- vhdx->iso
  - os with ci customizations + iso customizations {additional files + kernel params }
  - os customizations + iso with ci customizations {additional files + kernel params }
    - iso {from previous step} -> iso
      - iso customization
      - os customization
      - both os and iso customization

cwize1 · 2024-04-18T20:09:29Z

+	}()
+
+	// estimate the new disk size
+	safeDiskSizeMB, err := getDiskSizeEstimateInMBs(squashMountDir, 2 /*safety factor*/)


Why is the safety factor so large? Being off by an order of magnitude suggests that something is wrong.

I've changed it to 1.5. It's hard to estimate the size without some more calculations. I'm trying to err on the safe side.

btw:
I've found out that du returns the total size of disk blocks used. It does not just add the exact size of each file. So, du will return 4k for 2 byte file, for example. This makes me think we can lower the safety factor, since choosing a block size that's not 4k is not common. Open to suggestions here...

cwize1 · 2024-04-19T00:25:10Z

+
+	// define a disk layout with a boot partition and a rootfs partition
+	var maxDiskSizeMB imagecustomizerapi.DiskSize
+	maxDiskSizeMB = imagecustomizerapi.DiskSize(safeDiskSizeMB * 1024 * 1024)


Add the ESP partition to the disk's max size?

It's already included since the squashfs has both partitions included.

cwize1 · 2024-04-19T00:45:02Z

+			err := ic.isoBuilder.createWriteableImageFromSquashfs(ic.buildDir, ic.rawImageFile)
+			if err != nil {
+				return fmt.Errorf("failed to create writeable image:\n%w", err)
+			}


Should ic.isoBuilder be deleted here? When customizeOSPartitions is true, ic.isoBuilder isn't used after this point.

It actually needs to survive post this point...
isoBuilder has populated its internal state with all the relevant artifacts. That state can be used to regenerate the iso again without re-building any of these artifacts.
I've moved isoBuild out of the parameter structure so that it's not a side effect anymore. It's now returned from convertInputImageToWriteableFormat and is fed into convertWriteableFormatToOutputImage.

But you create a new copy of the LiveOSIsoBuilder type in the createLiveOSIsoImage function. So, surely the resources being held by ic.isoBuilder aren't needed?

I see you're point. You're right. I think we can make that decision right here.
pending.

I've found a flow where we do need the input iso builder object even when we're re-creating the squashfs. So, I need to keep it around.
Basically, when doing iso->iso creation, even if we re-create the squashfs (boot + rootfs), we still need the original iso state to determine what extra files the user has packed into it (in a previous customization), and copy them unto the new iso as well.

romoh · 2024-05-03T17:59:32Z

-While converting the input full disk image into a LiveOS ISO involves copying
+The Azure Linux Image Customizer can customize an input image and package the
+output as a LiveOS iso image. The input image can be a full disk image
+(vhd/vhdx/qcow2/raw) or another LiveOS iso image. The LiveOS iso output format


does it have to be LiveOS iso only? what will happen if a regular iso is used?

Correct, the input iso has to an image previously generated by mic. If the input iso does not have the same exact names and location of bits as the one generated by mic, customization will fail.

[we need to locate bootloaders, kernel, initrd, rootfs, grub.cfg in specific locations and with specific names]

romoh · 2024-05-03T18:04:47Z


 	switch config.OS.ResetBootLoaderType {
 	case imagecustomizerapi.ResetBootLoaderTypeHard:
+		if config.Storage == nil {


I thought storage is not necessary to resetBootLoaderType but the other way around is mandated (where resetBootLoaderType is a must when storage is specified), is that not the case anymore?

romoh · 2024-05-03T18:18:29Z

+	// the total size of a collection of files is multiplied by the
+	// expansionSafetyFactor to estimate a disk size sufficient to hold those
+	// files.
+	expansionSafetyFactor = 1.5


curious - is this based of anything specific or just a buffered estimate?

not based on anything specific - just a buffered estimate.

I've found out that du returns the total size of disk blocks used. It does not just add the exact size of each file. So, du will return 4k for 2 byte file, for example. This makes me think we can lower the safety factor, since choosing a block size that's not 4k is not common. Open to suggestions here...

cwize1 · 2024-05-03T19:11:58Z

+			err := ic.isoBuilder.createWriteableImageFromSquashfs(ic.buildDir, ic.rawImageFile)
+			if err != nil {
+				return fmt.Errorf("failed to create writeable image:\n%w", err)
+			}


But you create a new copy of the LiveOSIsoBuilder type in the createLiveOSIsoImage function. So, surely the resources being held by ic.isoBuilder aren't needed?

cwize1 · 2024-05-06T18:10:19Z

+
+		inputIsoArtifacts, err := createIsoBuilderFromIsoImage(ic.buildDir, ic.buildDirAbs, ic.inputImageFile)
+		if err != nil {
+			var cleanUpError error


This cleanup should occur in createIsoBuilderFromIsoImage, not in the caller of createIsoBuilderFromIsoImage.

It might be helpful to create a createIsoBuilderFromIsoImageHelper function that handles all the initialization logic. Then have the createIsoBuilderFromIsoImage call the createIsoBuilderFromIsoImageHelper function and call the cleanup logic if createIsoBuilderFromIsoImageHelper fails.

makes sense. I've moved isobuilder.cleanUp() into createIsoBuilderFromIsoImage() and set it such that it gets called only if there's an error. This 'only if' avoid having to add the call to each error handling in the rest of createIsoBuilderFromIsoImage() (and I think we can do without the helper function).

func someFunc() (err error ) { x := &SomeType{} defer func() { if err != nil {x.cleanUp} }() err = anotherFunc() if err != nil { return err } // -> this invokes the clean-up code. return nil // -> this does not invoke the clean-up code. }

cwize1 · 2024-05-06T18:28:48Z

+	// However, there are no customizations, and the input is an iso, we
+	// should just return - but call out any command line switches that might
+	// have requested unsupported changes.
+	if !ic.customizeOSPartitions && ic.inputImageFormat == ".iso" {


Should we also error out if the input is an ISO and the config has a .storage value?

if .storage is defined, that will enable the usual mic flow - i.e.

input is an iso

because one of storage, os, scripts nodes is defined:

input iso is converted to a writeable disk with a boot partition + a rootfs partition

storage configuration is processed as usual

we create an iso from the final state of the writeable disk

So, while it should just work, we can still disable it since the value of repartitioning is not there. All partitions get collapsed into the squashfs anyway (the same argument for vhd(x)/qcow -> iso actually - but users can use storage to move content from other partitions to the boot/rootfs. So, we shouldn't disable this scenario for vhdx(x)/qcow->iso).

I think it makes sense to disable since there's no point of having it.

…so. (microsoft#8785)

gmileka force-pushed the gmileka/iso-customize-iso-2 branch 2 times, most recently from bfaf011 to 973a1a0 Compare April 17, 2024 03:28

gmileka changed the title ~~Gmileka/iso customize iso 2~~ [mic][iso] Added the ability to customie an existing mic-generated iso. Apr 17, 2024

gmileka marked this pull request as ready for review April 18, 2024 00:07

gmileka requested a review from a team as a code owner April 18, 2024 00:07

cwize1 requested changes Apr 18, 2024

View reviewed changes

gmileka force-pushed the gmileka/iso-customize-iso-2 branch 2 times, most recently from a9c121d to 5f4ee0c Compare April 18, 2024 20:58

cwize1 reviewed Apr 19, 2024

View reviewed changes

gmileka changed the title ~~[mic][iso] Added the ability to customie an existing mic-generated iso.~~ [mic][iso] Added the ability to customize an existing mic-generated iso. May 1, 2024