From e621386cafb6a600ad3b0d0853b6d40014849eb0 Mon Sep 17 00:00:00 2001 From: Achilleas Koutsou Date: Mon, 5 May 2025 15:56:16 +0300 Subject: [PATCH] cloudapi: drop ImageRequest.GetImageOptions() method This method is not particularly useful anymore. Its purpose was to initialise the ImageOptions from an ImageRequest with the appropriate size and partitioning mode. However, the partitioning mode was also being set later using request.GetPartitioningMode(). More importantly, setting the size on the ImageOptions caused issues with the interaction between filesystem and partitioning customizations as well as the image request size (see #4705). The correct thing to do here is to map the ImageRequest.Size directly onto ImageOptions.Size, without taking into account ImageType or the Blueprint Customizations. The rest are considered when generating the manifest in images, either when preparing the Manifest() call or when generating the partition table. This makes it easier to trace and reason about the effect of each option. This kind of decision making in the API layer makes it difficult to maintain the logic, since it requires duplicating the decision making or, as we had now, making certain specific combinations impossible. --- internal/cloudapi/v2/compose.go | 10 ++- internal/cloudapi/v2/imagerequest.go | 23 ------- internal/cloudapi/v2/imagerequest_test.go | 79 ----------------------- 3 files changed, 8 insertions(+), 104 deletions(-) diff --git a/internal/cloudapi/v2/compose.go b/internal/cloudapi/v2/compose.go index ee60039e6..04b0af68f 100644 --- a/internal/cloudapi/v2/compose.go +++ b/internal/cloudapi/v2/compose.go @@ -14,6 +14,7 @@ import ( "github.com/osbuild/images/pkg/customizations/subscription" "github.com/osbuild/images/pkg/datasizes" "github.com/osbuild/images/pkg/disk" + "github.com/osbuild/images/pkg/distro" "github.com/osbuild/images/pkg/distrofactory" "github.com/osbuild/images/pkg/reporegistry" "github.com/osbuild/images/pkg/rhsm/facts" @@ -1251,8 +1252,13 @@ func (request *ComposeRequest) GetImageRequests(distroFactory *distrofactory.Fac repos = append(repos, dr...) } - // Get the initial ImageOptions with image size set - imageOptions := ir.GetImageOptions(imageType, bp) + // Initialise the image options from the image request and + // customizations + imageOptions := distro.ImageOptions{} + + if ir.Size != nil { + imageOptions.Size = *ir.Size + } if request.Koji == nil { imageOptions.Facts = &facts.ImageOptions{ diff --git a/internal/cloudapi/v2/imagerequest.go b/internal/cloudapi/v2/imagerequest.go index fa7a1d7fc..a77ef31bf 100644 --- a/internal/cloudapi/v2/imagerequest.go +++ b/internal/cloudapi/v2/imagerequest.go @@ -7,8 +7,6 @@ import ( ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/google/uuid" - "github.com/osbuild/blueprint/pkg/blueprint" - "github.com/osbuild/images/pkg/disk" "github.com/osbuild/images/pkg/distro" "github.com/osbuild/images/pkg/ostree" "github.com/osbuild/images/pkg/platform" @@ -17,27 +15,6 @@ import ( "github.com/osbuild/osbuild-composer/internal/target" ) -// GetImageOptions returns the initial ImageOptions with Size and PartitioningMode set -// The size is set to the largest of: -// - Default size for the image type -// - Blueprint filesystem customizations -// - Requested size -// -// The partitioning mode is set to AutoLVM which will select LVM if there are additional mountpoints -func (ir *ImageRequest) GetImageOptions(imageType distro.ImageType, bp blueprint.Blueprint) distro.ImageOptions { - // NOTE: The size is in bytes - var size uint64 - minSize := bp.Customizations.GetFilesystemsMinSize() - if ir.Size == nil { - size = imageType.Size(minSize) - } else if bp.Customizations != nil && minSize > 0 && minSize > *ir.Size { - size = imageType.Size(minSize) - } else { - size = imageType.Size(*ir.Size) - } - return distro.ImageOptions{Size: size, PartitioningMode: disk.AutoLVMPartitioningMode} -} - func newAWSTarget(options UploadOptions, imageType distro.ImageType) (*target.Target, error) { var awsUploadOptions AWSEC2UploadOptions jsonUploadOptions, err := json.Marshal(options) diff --git a/internal/cloudapi/v2/imagerequest_test.go b/internal/cloudapi/v2/imagerequest_test.go index d1c9a8368..f2d1582d7 100644 --- a/internal/cloudapi/v2/imagerequest_test.go +++ b/internal/cloudapi/v2/imagerequest_test.go @@ -3,7 +3,6 @@ package v2 import ( "testing" - "github.com/osbuild/blueprint/pkg/blueprint" "github.com/osbuild/images/pkg/arch" "github.com/osbuild/images/pkg/distro/rhel/rhel9" "github.com/osbuild/images/pkg/distro/test_distro" @@ -14,84 +13,6 @@ import ( "github.com/stretchr/testify/require" ) -func TestImageRequestSize(t *testing.T) { - distro := test_distro.DistroFactory(test_distro.TestDistro1Name) - require.NotNil(t, distro) - arch, err := distro.GetArch(test_distro.TestArchName) - if err != nil { - panic(err) - } - imageType, err := arch.GetImageType(test_distro.TestImageTypeName) - if err != nil { - panic(err) - } - - // A blueprint with no filesystem customizations - bp := blueprint.Blueprint{ - Name: "image-request-test", - Description: "Empty Blueprint", - Version: "0.0.1", - } - // #1 With no size request - ir := ImageRequest{ - Architecture: test_distro.TestArchName, - ImageType: test_distro.TestImageTypeName, - Size: nil, - } - imageOptions := ir.GetImageOptions(imageType, bp) - - // The test_distro default size is 1GiB - assert.Equal(t, uint64(1073741824), imageOptions.Size) - - // #2 With size request - ir = ImageRequest{ - Architecture: test_distro.TestArchName, - ImageType: test_distro.TestImageTypeName, - Size: common.ToPtr(uint64(5368709120)), - } - imageOptions = ir.GetImageOptions(imageType, bp) - - // The test_distro default size is actually 5GiB - assert.Equal(t, uint64(5368709120), imageOptions.Size) - - // A blueprint with filesystem customizations - bp = blueprint.Blueprint{ - Name: "image-request-test", - Description: "Customized Filesystem", - Version: "0.0.1", - Customizations: &blueprint.Customizations{ - Filesystem: []blueprint.FilesystemCustomization{ - blueprint.FilesystemCustomization{ - Mountpoint: "/", - MinSize: 2147483648, - }, - }, - }, - } - - // #3 With no size request - ir = ImageRequest{ - Architecture: test_distro.TestArchName, - ImageType: test_distro.TestImageTypeName, - Size: nil, - } - imageOptions = ir.GetImageOptions(imageType, bp) - - // The test_distro default size is actually 2GiB - assert.Equal(t, uint64(2147483648), imageOptions.Size) - - // #4 With size request - ir = ImageRequest{ - Architecture: test_distro.TestArchName, - ImageType: test_distro.TestImageTypeName, - Size: common.ToPtr(uint64(5368709120)), - } - imageOptions = ir.GetImageOptions(imageType, bp) - - // The test_distro default size is actually 5GiB - assert.Equal(t, uint64(5368709120), imageOptions.Size) -} - func TestGetOstreeOptions(t *testing.T) { // No Ostree settings ir := ImageRequest{