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

[RFC] many: make bootstrap container usage a manifest option #1293

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mvo5
Copy link
Contributor

@mvo5 mvo5 commented Mar 6, 2025

[draft as a) it lacks tests b) there are multiple ways of doing this and this outlines the one I like best but that may not match with the others of course]

This PR adds the option to use a bootstrap container when generating the manifest. This allows a seamless experience when using e.g. image-builder:

$ image-builder build --arch=riscv64 minimal-raw --distro fedora-42

(or --arch=aarch64 etc, as long as there is an upstream container with python3 for the given distro)

The approach is to make the distro bootstrap container ref part of the manifest struct just like we set "Distro" there. This is not ideal, it would be much nicer if manifest would hold the full distro.Distro and this way we could simply run something like distro.BootstrapContainerRef() when needed. However because of circular inputs that is not possible.

The other option would be to change the signature of image.ImageKind.InstantiateManifest, e.g.

- InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
+ InstantiateManifest(m *manifest.Manifest, repos newpkg.RepoAndBootrapConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)

here it would be either a new rpmmd.RepoAndBootstrapConfig with a struct that holds []rpmmd.RepoConfig and BootstrapRef - but that feels a bit out of place in rpmmd (because of the container ref that is unreleated to rpm repos). The RepoAndBootstrapConfig cannot be part of manifest,image or distro because of circular imports.

Alternatively:

- InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
+ InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, bootstrapRef string, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)

that just adds a boostrapRef arg. That seems ok but requires a lot of (mechanical) changes.

Note that I think eventually the bootstrap container ref will be part of the "distro.yaml" and the currently (slightly ugly) helper bootstrapContainerFor() will go away.

The matching and small image-builder-cli changes are in https://github.com/osbuild/image-builder-cli/compare/main...mvo5:cross-arch-for-all-2?expand=1 (and the cross-build test in there passes locally but takes ~30min for all 4 tested arches and it lacks a check that inside the generate container image we actually have binaries of the expected architecture)

Note that cross-arch building will not support selinux in the buildroot yet - see the second commit about how we can still archive this in the future.

@mvo5 mvo5 requested a review from ondrejbudai March 6, 2025 11:28
mvo5 added 2 commits March 6, 2025 12:36
This commit adds the option to use a bootstrap container when
generating the manifest. This allows a seamless experience when
using e.g. image-builder:
```console
$ image-builder build --arch=riscv64 minimal-raw --distro fedora-42
```

The approach is to make the distro bootstrap container ref part of
the manifest struct just like we set "Distro" there. This is not
ideal, it would be much nicer if manifest would hold the full
`distro.Distro` and this way we could simply run something like
`distro.BootstrapContainerRef()` when needed. However because
of circular inputs that is not possible.

The other option would be to change the signature of
image.ImageKind.InstantiateManifest, e.g.
```go
- InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
+ InstantiateManifest(m *manifest.Manifest, repos newpkg.RepoAndBootrapConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
```
here it would be either a new `rpmmd.RepoAndBootstrapConfig` with a struct
that holds []rpmmd.RepoConfig and BootstrapRef - but that feels a bit
out of place in rpmmd (because of the container ref that is unreleated to
rpm repos). The RepoAndBootstrapConfig cannot be part of manifest,image
or distro because of circular imports.

Alternatively:
```go
- InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
+ InstantiateManifest(m *manifest.Manifest, repos []rpmmd.RepoConfig, bootstrapRef string, runner runner.Runner, rng *rand.Rand) (*artifact.Artifact, error)
```
that just adds a boostrapRef arg. That seems ok but requires a
lot of (mechanical) changes.

Note that I think eventually the bootstrap container ref will be
part of the "distro.yaml" and the currently (slightly ugly) helper
`bootstrapContainerFor()` will go away.
This commit disables selinux (for now) in the buildroot when doing
a cross-arch build. This is needed because most containers we can
use to bootstrap do not contain `setfiles(8)` from `policycoreutils`.

We have three options:
1. disable selinux (for now)
2. maintain our own containers with setfiles (expensive)
3. teach osbuild to support setfiles via chroot() into the tree

I think we should do (3) eventually but this commit goes with (1)
to unblock us.
@mvo5 mvo5 force-pushed the bootstrap-cnt-2 branch from 397801c to a9ec09c Compare March 6, 2025 11:36
@achilleas-k achilleas-k self-requested a review March 6, 2025 15:11
@achilleas-k
Copy link
Member

I think the bootstrap container ref should be treated like any other source during the manifest instantiation and serialization flow. I don't understand why it needs to be part of the InstantiateManifest() call.

So, right now we have:
(setting aside our plans to simplify this flow for now)

// options contains ostree refs, so ignore the inconsistencies for now
img, err := t.image(w, t, bp, options, staticPackageSets, containerSources, rng)

mf := manifest.New()
img.InstantiateManifest(&mf, repos, runner, rng)

// get source specs
pkgSets := mf.GetPackageSetChains()
containerSources := mf.GetContainers()
commitSources := mf.GetCommits()

// resolve each
packages := depsolve(packages)
containers := resolveContainers(containerSources)
commits := resolveCommits(commitSources)

finalmf := mf.Serialize(packages, containers, commits)

I think the bootstrap container should come from outside this flow, just like we do with repositories. It can be a config file we embed and can override externally. Exactly like we do with repositories.

Unlike repositories though, I like that it goes into the image function like you did in the PR. In fact, maybe repositories should go into the image function too (but, again, let's ignore that for now).

@mvo5 mvo5 closed this Mar 10, 2025
@mvo5 mvo5 reopened this Mar 10, 2025
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.

2 participants