feat: support rhel bib #449

deboer-tim · 2024-05-06T12:48:24Z

What does this PR do?

Adds a new preference that allows you to switch between:

'image' - the default, picks up builder from image label (or use centos if none is defined)
'centos' - always use Centos builder
'rhel' - always use RHEL builder

createBuilderImageOptions() is made async to allow it to check the image and switch the builder if necessary.

Screenshot / video of UI

N/A, only new preference visible.

What issues does this PR fix or reference?

Fixes #428.

How to test this PR?

Tests added for the change, but need to test RHEL bib in general.

cdrage · 2024-05-06T12:59:38Z

packages/backend/src/build-disk-image.ts

@@ -305,8 +305,27 @@ export async function getUnusedName(name: string): Promise<string> {
 }

 // Create builder options for the "bootc-image-builder" container
-export function createBuilderImageOptions(name: string, build: BootcBuildInfo): ContainerCreateOptions {
-  const cmd = [`${build.image}:${build.tag}`, '--output', '/output/', '--local'];
+export async function createBuilderImageOptions(name: string, build: BootcBuildInfo): Promise<ContainerCreateOptions> {


I do not like that we are switching this to async as this is suppose to be just for generating the correct image builder options.

the functionality of selecting the "getting image builder" should be in a separate function outside of createBuilderImageOptions.

that means that we do not need to touch createBuilderImageOptions and that we just need to update the build BootcBuildInfo to build.image = rhel blah blah

You're right, moved the code to determine which builder to use into a separate getBuilder() function, but had to add an optional parameter to createBuilderImageOptions() to specify which builder to use. (build.image is the image you're trying to build, not the one doing the work)

cdrage · 2024-05-06T13:00:48Z

packages/backend/src/build-disk-image.ts

+  } else if (buildProp === 'image') {
+    // or switch to rhel if the preference comes from the image label
+    // AND we detect the rhel label
+    const buildLabel = await containerUtils.getImageBuilderLabel(image);


listImages is very expensive call and I see it's done within getImageBuilderLabel and each time we do it it makes the build "stutter" a bit before building.

i think that we should move this earlier in the call process and use the already done listImages call in the code? so we can avoid calling it twice.

Where do we already call listImages()? I agree we should not call twice if we can avoid it but I didn't see an obvious use already.

cdrage · 2024-05-06T13:03:08Z

packages/backend/src/build-disk-image.ts

+    // AND we detect the rhel label
+    const buildLabel = await containerUtils.getImageBuilderLabel(image);
+    if (buildLabel === 'registry.redhat.io/rhel9/bootc-image-builder') {
+      builder = bootcImageBuilderRHEL;


this part is confusing to me for the if statement.

If buildProp = rhel, we switch to rhel

i feel like we dont need this check statement as it'll just be a:

if rhel = use this image

if centos or default, use centos

But that's it, no need to rhel check?

See CentOS/centos-bootc#453 for some background. The bootc.diskimage-builder label allows an image to say 'I expect to be built with builder X'.

If the default preference (image) is used, then we need to look for this label. For now, we only look for one specific value and change to using the RHEL builder. So there are two cases where we'd use the RHEL builder: the preference is explicitly set to prefer RHEL, or preference is image and we detected an image that has asked for this builder.

We could remove the 'image' option and the image label detection for now if you're worried about the cost; users could still switch the builder (for all builds) through the preference.

for now I think that we should just have it 100% through preferences.

it's also because I just inspected a few of my images and there is no bootc.diskimage-builder for the fedora bootc image...

ex:

"Labels": { "containers.bootc": "1", "io.buildah.version": "1.35.3", "org.opencontainers.image.version": "40.20240503.0", "ostree.bootable": "true", "ostree.commit": "0b148652f58f77457b63129ae2f95d3f742b521b358727e2daca81fe7cd49e25", "ostree.final-diffid": "sha256:12787d84fa137cd5649a9005efe98ec9d05ea46245fdc50aecb7dd007f2035b1", "ostree.linux": "6.8.8-300.fc40.x86_64", "rpmostree.inputhash": "2d27d73eaa1cf6595b3b5e555ffe0b36a6854740298fa498e8bc1e4872f1d401" } },

Ok, label detection code removed and simplified just to preferences for now.

Adds a new preference that allows you to switch between: - 'image' - the default, picks up builder from image label (or use centos if none is defined) - 'Centos' - always use Centos builder - 'RHEL' - always use RHEL builder getBuilder() is added to determine which builder to use, and createBuilderImageOptions() has an optional parameter to specify the builder to use. Signed-off-by: Tim deBoer <[email protected]>

Remove the label detection code for now: doesn't seem to be used by many containers and listImages is a costly call. Signed-off-by: Tim deBoer <[email protected]>

cdrage · 2024-05-06T13:44:31Z

packages/backend/package.json

+        },
+        "bootc.builder": {
+          "type": "string",
+          "default": "Centos",


should be CentOS throughout code + options

cdrage

should we add a test if configuration setting is blank? should pick centos (i'm assuming).

cdrage · 2024-05-06T13:45:37Z

packages/backend/package.json

+            "Centos",
+            "RHEL"
+          ],
+          "description": "Builder image"


Maybe something like "The build image used to create disk images from bootc container inputs" similar to the readme https://github.com/osbuild/bootc-image-builder

Updated. Other prefs don't start with 'the' and I thought the ending was obvious enough (?) so I went with: Builder image used to create disk images.

cdrage · 2024-05-06T13:46:24Z

packages/backend/src/build-disk-image.spec.ts

+});
+
+test('check uses Centos builder', async () => {
+  configurationGetConfigurationMock.mockReturnValue('centos');


should be CentOS

cdrage · 2024-05-06T13:47:13Z

packages/backend/src/build-disk-image.ts

+  }
+
+  // always default to centos bib
+  return bootcImageBuilderCentos;


should we fail at all if the config setting is not centos or rhel? i see in our tests it still passed.

example you can pass in anything 'foobar' as the configuration setting and it'll always pick centos.

I wanted to play it safe, i.e. even if config corrupted => we ignore and use CentOS anyway.

Signed-off-by: Tim deBoer <[email protected]>

cdrage · 2024-05-06T14:13:39Z

packages/backend/src/constants.ts

-export const bootcImageBuilderContainerName = '-bootc-image-builder';
-export const bootcImageBuilderName = 'quay.io/centos-bootc/bootc-image-builder:latest-1714633180';
+export const bootcImageBuilder = 'bootc-image-builder';
+export const bootcImageBuilderCentos = 'quay.io/centos-bootc/bootc-image-builder:latest-1714474808';


you changed the image to latest-1714474808 which is untested

Doh, forgot I was testing that on the side. Fixed.

Signed-off-by: Tim deBoer <[email protected]>

cdrage

Approved and tested! Tested via centos, fedora and rhel and switched between bib images.

HOWEVER there is one bug.

If you select ext4 or xfs with rhel bib selected, it will not build. This is because the rhel bib image does not have --rootfs yet.

We should be aware of this in case users run into that edge case, but the bib image for rhel will be updated shortly.

deboer-tim requested a review from a team as a code owner May 6, 2024 12:48

deboer-tim force-pushed the rhel-bib branch from 59d00bc to f1d98d0 Compare May 6, 2024 12:57

cdrage requested changes May 6, 2024

View reviewed changes

deboer-tim force-pushed the rhel-bib branch from f1d98d0 to b174a70 Compare May 6, 2024 13:24

chore: remove image label detection for now

11a8cc2

Remove the label detection code for now: doesn't seem to be used by many containers and listImages is a costly call. Signed-off-by: Tim deBoer <[email protected]>

cdrage requested changes May 6, 2024

View reviewed changes

chore: fix CentOS, pref description

e7c7101

Signed-off-by: Tim deBoer <[email protected]>

cdrage requested changes May 6, 2024

View reviewed changes

chore: don't switch default bib

099e841

Signed-off-by: Tim deBoer <[email protected]>

cdrage approved these changes May 6, 2024

View reviewed changes

deboer-tim merged commit a64515b into podman-desktop:main May 6, 2024
5 checks passed

deboer-tim deleted the rhel-bib branch May 28, 2024 21:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: support rhel bib #449

feat: support rhel bib #449

deboer-tim commented May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage left a comment

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage May 6, 2024

deboer-tim May 6, 2024

cdrage left a comment

feat: support rhel bib #449

feat: support rhel bib #449

Conversation

deboer-tim commented May 6, 2024

What does this PR do?

Screenshot / video of UI

What issues does this PR fix or reference?

How to test this PR?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage left a comment

Choose a reason for hiding this comment