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

Add support for building images from ostree containers #272

Merged
merged 17 commits into from
Nov 28, 2023

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Nov 24, 2023

Cherry-picked commits from #243 without the CoreOS image.

This PR adds support for the org.osbuild.ostree.deploy.container stage and addresses the relevant comments from #243. The other PR will be rebased to only add the CoreOS image after this is merged.

This PR doesn't define a new image type, we will instead instantiate the manifest directly as a separate project, in the new repository https://github.com/osbuild/osbuild-deploy-container/.

Part of HMS-3121.

@achilleas-k
Copy link
Member Author

Needs #278

pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
pkg/distro/fedora/images.go Show resolved Hide resolved
@achilleas-k achilleas-k force-pushed the stages/ostree.deploy.container branch 2 times, most recently from af6af76 to 621df93 Compare November 27, 2023 15:51
@achilleas-k
Copy link
Member Author

Reverted the upgrade of the CI runners to F39. Looked like there might be some misconfiguration with the x86 image. We can investigate separately.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thanks for this PR - I added some comments and nitpicks inline but a lot of it it personal preference and/or comemnts due to my ignorance. I hope it's still useful.

pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Outdated Show resolved Hide resolved
pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
mvo5
mvo5 previously approved these changes Nov 27, 2023
Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Two small followup comments, still approved of course.

pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
@achilleas-k
Copy link
Member Author

@mvo5 I went all in on the switches for you

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.

Also added tests for the validators for options and inputs.

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.
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.
Not all image types with ostree deployments will want to lock the root
user account.  Let's instead make it an option of the image function so
that each image kind can set it itself.

This change has no effect on any of the current image types / manifests.
Not all image types that use ignition want to define the kernel option.
Specifically, the (upcoming) CoreOS images wont want to enable this, so
let's move it into the image function and add it when the ignition
options are added.  It is conditioned on the distro version for now, so
we can't add it to the global image type kernel options.

This change has no effect on any of the current image types / manifests.
achilleas-k and others added 2 commits November 28, 2023 01:38
Handle the two sets of deployment stages in two separate functions to
improve readability.

Co-Authored-By: Michael Vogt <[email protected]>
We need this to get F39 CI runners but we're not switching them just
yet because of a potential small issue.
Copy link
Member

@supakeen supakeen left a comment

Choose a reason for hiding this comment

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

Awesome.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! It looks good to merge, I made a bunch of inline comments with ideas/suggestions/questions but all totally optional.

pkg/osbuild/ostree_deploy_container_stage.go Show resolved Hide resolved
pkg/manifest/ostree_deployment.go Show resolved Hide resolved
cmd/build/main.go Show resolved Hide resolved
test/data/repositories/fedora-39.json Show resolved Hide resolved
@mvo5 mvo5 enabled auto-merge November 28, 2023 10:00
@mvo5 mvo5 added this pull request to the merge queue Nov 28, 2023
Merged via the queue into osbuild:main with commit 8f44d2e Nov 28, 2023
9 checks passed
@achilleas-k achilleas-k deleted the stages/ostree.deploy.container branch November 28, 2023 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants