Add support for building images from ostree containers and the Fedora CoreOS qcow2 image type #243

achilleas-k · 2023-11-06T23:23:11Z

This PR adds support for the org.osbuild.ostree.deploy.container stage and a new image type that uses it called coreos-qcow2 for Fedora.

I tested the new image type locally with the container we added to the gitlab registry for testing the sample manifest in osbuild.

Some notes and open questions:

I use the same pipeline as the regular ostree deployment with a condition on the type of source. I think it wasn't worth making a whole separate pipeline for this, but we can revisit this decision if the two start diverging.
The new image type is added to Fedora which means that if this is merged and updated in osbuild-composer it will be available to build on-prem. If we want this, we'll have to figure out the command line options for specifying a container source and potentially a target-imgref.
When specifying a container source to deploy, you must specify the ref (otherwise the default will be assumed). This requirement also exists for ostree commits from a repo, but I wonder: Is there a way to determine the container's ostree ref without downloading it?

You can generate the manifest for this image with:

go run ./cmd/gen-manifests --distros fedora-38 --arches x86_64 --images coreos-qcow2 --output .

or build it directly with

go build -o bin/build ./cmd/build
sudo ./bin/build --distro fedora-38 --image coreos-qcow2 --output builds --config ./test/configs/ostree-container-pull.json

You can also add a user to the config file which will create a user after the deployment before finalising the image:

{
  "name": "ostree-container-pull",
  "ostree": {
    "ref": "ostree/1/1/0",
    "container": "registry.gitlab.com/redhat/services/products/image-builder/ci/images/fedora-coreos:stable",
    "tls-verify": true
  },
  "blueprint": {
    "customizations": {
      "user": [
        {
          "name": "<login>",
          "password": "<password>",
          "key": "<pubkey>"
        }
      ]
    }
  }
}

pkg/image/ostree_disk.go

pkg/distro/fedora/images.go

ondrejbudai

I'm very happy with this PR and I haven't found any issues. However, it's a big one, so I would love a third pair of eyes to take a look. @achilleas-k should we ask someone explicitly? Any ideas whom? :)

teg · 2023-11-10T17:07:10Z

pkg/distro/fedora/distro.go

@@ -689,6 +707,7 @@ func newDistro(version int) distro.Distro {
 			UEFIVendor: "fedora",
 		},
 		iotQcow2ImgType,
+		coreosQcow2ImgType,


What about aarch64?

I added it at first but then realised I needed to get the base container in our CI registry to test it and decided to get this open so people can review and I can add it in a follow-up.

teg

I think I noticed an oversight, but would rather fix that in a follow-up than rebasing it tbh, this is plenty useful as it is.

- Take the code from https://github.com/achilleas-k/images/tree/bifrost-image/cmd/osbuild-deploy-container and merge it into this repository, using the code from osbuild/images#243 as a `replace` - Also merge in osbuildbootc

pkg/osbuild/ostree_deploy_container_stage.go

pkg/manifest/ostree_deployment.go

pkg/image/ostree_disk.go

dustymabe · 2023-11-14T19:41:56Z

pkg/ostree/ostree.go

@@ -72,6 +77,9 @@ type ImageOptions struct {
 	// Indicate if the 'org.osbuild.rhsm.consumer' secret should be added when pulling from the
 	// remote.
 	RHSM bool `json:"rhsm"`
+
+	// TLS verification for container sources.
+	TLSVerify *bool `json:"tls-verify"`


since there are multiple things here that are "remote" artifacts it might be a little confusing to have this just apply to one of them and still name it TLSVerify.

One thing we could do here is break out sets of options that go together into subordinate objects.

i.e.

type ContainerImageOptions struct { Container string `json:"container"` TLSVerify *bool `json:"tls-verify"` } ...

There's two things going on here that I don't like and I'm unsure of:

The ostree.ImageOptions and more generally the distro.ImageOptions that exist separately from things like the blueprint customizations. These I want to merge into a build config.

The way that users will be specifying an ostree container for a build when this gets into osbuild-composer.

The way I added the options now sort of reflects things that might happen in composer, but that's just leftover internal structure from the split.

All that said, none of this is considered "stable API" (yet) in images, so I did the quickest thing that felt natural at the time. You're right though, a completely separate object is nicer. Getting the "build config" done soon would be even better.

I'm OK with whatever.

dustymabe · 2023-11-14T19:46:35Z

pkg/distro/fedora/images.go

+	img.Compression = t.compression
+
+	return img, nil
+}


There is a lot going on here. Maybe @achilleas-k we can get together and step through some of it.

Sure, we can do that. For a high level description though: Almost all of this function is essentially creating the img

img := image.NewOSTreeContainerDiskImage(container, ref)

followed by setting options on the image based on image type settings and user customizations.

dustymabe · 2023-11-14T19:49:37Z

pkg/distro/fedora/distro.go

+		buildPipelines:      []string{"build"},
+		payloadPipelines:    []string{"ostree-deployment", "image", "qcow2"},
+		exports:             []string{"qcow2"},
+		basePartitionTables: iotBasePartitionTables,


I probably need to dig into iotBasePartitionTables a little bit more to understand why they are being used here.

I based the image off the iot-raw-image and reused that partition table. I would say, instead of digging, point me to the default partition table you use in coreos images and I'll replicate it here.

this is what I've been using for now.

There is one theme here, though, that I think we keep bumping up into, which is where definitions like this should live. I think it was also touched on in osbuild/bootc-image-builder#4
I

For Image Builder (osbuild-composer and the service), the defaults are defined here. For all the RHEL images we build for example as part of the composes in Brew and for all the images in c.rh.c/ib, this is the source of truth right now.

dustymabe · 2023-11-14T19:53:45Z

pkg/manifest/ostree_deployment.go

@@ -340,7 +340,8 @@ func (p *OSTreeDeployment) serialize() osbuild.Pipeline {
 		}
 	}

-	if !hasRoot {
+	if !hasRoot && p.ostreeSpec != nil {
+		// NOTE: fails on container-based deployments


not sure either.. from my side this feels like something that would be done when composing the ostree commit or oscontainer rather than when composing an image. IOW I feel like this shouldn't be part of Image Builder at all?

Locking the root password during deployment is something that's done for the official Fedora IoT image: https://pagure.io/fedora-kickstarts/blob/main/f/fedora-iot.ks#_9

We were replicating that here.

dustymabe · 2023-11-14T19:54:10Z

pkg/distro/distro_test.go

@@ -46,6 +46,10 @@ func TestImageType_PackageSetsChains(t *testing.T) {
 							URL: "https://example.com", // required by some image types
 						},
 					}
+					if imageType.Name() == "coreos-qcow2" {
+						options.OSTree.Container = "https://registry.example.com/image"


is this supposed to be a container pullspec? those don't often include https:// do they?

It could be any string since the test doesn't really do anything with it, but you're right. Might as well make it look like a real spec.

mvo5

Not sure how helpful my review here is, I did a quick scan to get a feeling for the code and just added tiny coments/ideas inline. As a bit of a meta-comment, would it make sense to split something like ae57912 into it's own PR? It seems generally useful on its own and would make this PR smaller. But there are probably good reasons that I just don't know yet :) Hope this is still useful in some small way.

pkg/osbuild/ostree_deploy_container_stage.go

mvo5 · 2023-11-16T09:53:03Z

pkg/distro/fedora/imagetype.go

@@ -269,7 +269,8 @@ func (t *imageType) checkOptions(bp *blueprint.Blueprint, options distro.ImageOp
 		}
 	}

-	if t.name == "iot-raw-image" || t.name == "iot-qcow2-image" {
+	if t.name == "iot-raw-image" || t.name == "iot-qcow2-image" || t.name == "coreos-qcow2" {


I wonder if if slices.Contains([]string{"iot-raw-image", "iot-qcow2-image", "coreos-qcow2"}, t.name) { might be slightly nicer? It seems we are using this already. But size of the line is almost the same so maybe not a win.

I think that's a good idea. This keeps growing and it'll be nicer to be able to append a single name to a list instead of needing to rewrite the comparison every time. Generally, we should have a nicer way to test these, but that's for another day.

pkg/image/ostree_disk.go

Creates an org.osbuild.ostree.deploy.container stage. Validates the options with a regular expression that matches the schema. Takes a container as input. Validates the length of the input references to be exactly 1. See osbuild/osbuild#1402

The pipeline either way only supported one ostree commit, enforced when adding the resolved commits to the pipeline during serializeStart() and by checking the array length during serialize(). To simplify the requirement and avoid potential confusion when reading the struct itself, replace the array on the pipeline struct definition with a single commit spec (pointer) and keep the len == 1 check in the serializeStart() function that, by its interface, must accept an array. Also document the commit fields for OSTreeDeployment. These are private, so they wont show up in public API docs, but it's still useful to have them documented in the code.

Support a container source for OSTreeDeployment as an alternative to the ostree commit source. Currently, if a container is defined, it fails with an error. The commit source and the container source are now both pointers since they can be unset, but (will) we need to make sure at least one is set when serializing.

Move the ostree pull stage to be right before the ostree deploy stage. This should have no functional impact on the build and will make it easier to replace the two stages together. Old vs new stage order: org.osbuild.ostree.init-fs | org.osbuild.ostree.init-fs org.osbuild.ostree.pull | org.osbuild.ostree.os-init org.osbuild.ostree.os-init | org.osbuild.mkdir org.osbuild.mkdir | org.osbuild.ostree.pull org.osbuild.ostree.deploy | org.osbuild.ostree.deploy

When creating the ostree deployment pipeline, add the appropriate stages only when the ostree commit is specified. The stages are: org.osbuild.pull org.osbuild.ostree.deploy org.osbuild.ostree.remote org.osbuild.ostree.fillvar

Rename the NewOSTreeDeployment() constructor to NewOSTreeCommitDeployment() and add a second one called NewOSTreeContainerDeployment(). Each takes a different kind of source spec (ostree commit or container). The container constructor also requires the ref to be specified since it's not part of the container spec.

Add the org.osbuild.ostree.deploy.container stage when the content source for the deployment is a container.

Support both content source types on the OSTreeDiskImage ImageKind. A second constructor is defined, just like with the deployment pipeline of the image. The original constructor is renamed to NewOSTreeDiskImageFromCommit to differentiate and have a more informative name.

Only required properties should be specified through the constructor to make sure they are not accidentally left empty or nil. Optional properties are added via public fields of the pipeline object. Since the ignition platform is optional, change it to a public field. Also, to simplify the interaction between the two ignition properties, remove the boolean that signifies if ignition should be enabled and use the IgnitionPlatform string instead. Treat the empty string as the disabled value.

A new image functions called coreosImage() that requires a container source and creates a qcow2 image from a CoreOS ostree container. Based on the example manifest added in osbuild/osbuild#1402

Define the new image type using the new coreosImage() function and container deployment pipeline and add it to the x86_64 and aarc64 registries for Fedora.

We always aliased these types so that internal changes wouldn't always require changing our tooling interfaces but it's actually more annoying needing to change them every time something is added or changed in the internal types. These are development and testing tools. It's fine to use the internal types directly and keep them simple.

Use gpgkeys instead of concatenating multiple keys under gpgkey. The singular gpgkey was a leftover from an old format and now we use an array from the internal rpmmd.RepoConfig instead.

Modifying the root user fails when trying to modify a container deployment. Let's disable it for now until we figure out if it's possible.

Use the latest version from the Fedora 38 repo snapshots.

achilleas-k · 2023-11-20T14:10:45Z

Partially addressed comments (and rebased on main). Will resume addressing comments later.

achilleas-k · 2023-11-24T14:27:22Z

Moved most of the commits to #272. I'll rebase this PR so that it only adds the coreos image once that's merged.

supakeen · 2023-11-28T10:49:57Z

@achilleas-k should be able to rebase now as #272 has landed \o/

achilleas-k · 2023-11-28T11:47:18Z

Will rebase and fix conflicts then start addressing the remaining comments.

github-actions · 2023-12-29T04:00:44Z

This PR is stale because it has been open 30 days with no activity. Remove "Stale" label or comment or this will be closed in 7 days.

github-actions · 2024-01-05T04:00:57Z

This PR was closed because it has been stalled for 30+7 days with no activity.

achilleas-k requested review from ondrejbudai, dustymabe and supakeen November 6, 2023 23:23

achilleas-k force-pushed the ostree.deploy.container branch 2 times, most recently from b8b990f to b19437c Compare November 7, 2023 11:08

bcl reviewed Nov 7, 2023

View reviewed changes

pkg/image/ostree_disk.go Outdated Show resolved Hide resolved

ondrejbudai reviewed Nov 8, 2023

View reviewed changes

pkg/distro/fedora/images.go Outdated Show resolved Hide resolved

achilleas-k force-pushed the ostree.deploy.container branch 5 times, most recently from 617ce72 to 25ca0c0 Compare November 10, 2023 10:35

ondrejbudai reviewed Nov 10, 2023

View reviewed changes

achilleas-k mentioned this pull request Nov 10, 2023

Add single-purpose command for building bootable images from an ostree native container #250

Closed

teg reviewed Nov 10, 2023

View reviewed changes

teg previously approved these changes Nov 10, 2023

View reviewed changes

cgwalters mentioned this pull request Nov 13, 2023

WIP: Merge osbuildbootc into here, use osbuild/images PR osbuild/bootc-image-builder#5

Closed

dustymabe reviewed Nov 14, 2023

View reviewed changes

mvo5 reviewed Nov 16, 2023

View reviewed changes

achilleas-k added 10 commits November 20, 2023 14:48

osbuild: add Len() to the ContainersInputReferences interface

6046a59

osbuild: add ostree.deploy.container stage

ec636aa

Creates an org.osbuild.ostree.deploy.container stage. Validates the options with a regular expression that matches the schema. Takes a container as input. Validates the length of the input references to be exactly 1. See osbuild/osbuild#1402

manifest: add ostree deploy stages conditionally

2f90dd1

When creating the ostree deployment pipeline, add the appropriate stages only when the ostree commit is specified. The stages are: org.osbuild.pull org.osbuild.ostree.deploy org.osbuild.ostree.remote org.osbuild.ostree.fillvar

manifest: add ostree.deploy.container stage to ostree deployment

09216f6

Add the org.osbuild.ostree.deploy.container stage when the content source for the deployment is a container.

achilleas-k added 10 commits November 20, 2023 15:07

ostree: add ostree image options for containers

74df68a

distro/fedora: add coreosImage function

e163c4b

A new image functions called coreosImage() that requires a container source and creates a qcow2 image from a CoreOS ostree container. Based on the example manifest added in osbuild/osbuild#1402

distro/fedora: add new image type "coreos-qcow2"

49a0786

Define the new image type using the new coreosImage() function and container deployment pipeline and add it to the x86_64 and aarc64 registries for Fedora.

test: add build config for fedora coreos

160b82b

test: update repository format

f80416f

Use gpgkeys instead of concatenating multiple keys under gpgkey. The singular gpgkey was a leftover from an old format and now we use an array from the internal rpmmd.RepoConfig instead.

manifest: disable root user locking when deploying containers

bbf87f2

Modifying the root user fails when trying to modify a container deployment. Let's disable it for now until we figure out if it's possible.

test: update registry and image type tests

6fc7e74

Schutzfile: unpin osbuild commit sha

02a5d0b

Use the latest version from the Fedora 38 repo snapshots.

test: use Fedora 39 to build images in CI

d3e1734

achilleas-k dismissed teg’s stale review via d3e1734 November 20, 2023 14:10

achilleas-k force-pushed the ostree.deploy.container branch from 25ca0c0 to d3e1734 Compare November 20, 2023 14:10

achilleas-k mentioned this pull request Nov 24, 2023

Add support for building images from ostree containers #272

Merged

github-actions bot added the Stale label Dec 29, 2023

github-actions bot closed this Jan 5, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for building images from ostree containers and the Fedora CoreOS qcow2 image type #243

Add support for building images from ostree containers and the Fedora CoreOS qcow2 image type #243

achilleas-k commented Nov 6, 2023 •

edited

Loading

ondrejbudai left a comment

teg Nov 10, 2023

achilleas-k Nov 10, 2023

teg left a comment

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

dustymabe Nov 14, 2023

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

dustymabe Nov 14, 2023

achilleas-k Nov 14, 2023

mvo5 left a comment

mvo5 Nov 16, 2023

achilleas-k Nov 24, 2023

achilleas-k commented Nov 20, 2023

achilleas-k commented Nov 24, 2023

supakeen commented Nov 28, 2023

achilleas-k commented Nov 28, 2023

github-actions bot commented Dec 29, 2023

github-actions bot commented Jan 5, 2024

Add support for building images from ostree containers and the Fedora CoreOS qcow2 image type #243

Add support for building images from ostree containers and the Fedora CoreOS qcow2 image type #243

Conversation

achilleas-k commented Nov 6, 2023 • edited Loading

ondrejbudai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teg 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

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

mvo5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achilleas-k commented Nov 20, 2023

achilleas-k commented Nov 24, 2023

supakeen commented Nov 28, 2023

achilleas-k commented Nov 28, 2023

github-actions bot commented Dec 29, 2023

github-actions bot commented Jan 5, 2024

achilleas-k commented Nov 6, 2023 •

edited

Loading