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

Support osbuild format v2 #1244

Merged
merged 35 commits into from
Mar 17, 2021
Merged

Support osbuild format v2 #1244

merged 35 commits into from
Mar 17, 2021

Conversation

achilleas-k
Copy link
Member

@achilleas-k achilleas-k commented Feb 22, 2021

This pull request includes:

  • adequate testing for the new functionality or fixed issue
  • adequate documentation informing people about the change

Opening as draft for review and CI. Unit tests are covered but needs image type test cases and news item.


OSBuild Manifest Format Version 2!

This PR adds support for the new Manifest schema for OSBuild, introduced as a tech preview in v25. The new packages, osbuild2 contains the new types (Stages, Sources, etc.) which mostly consists of copies of the types from the old osbuild package with small adaptations, and adds new types introduced in OSBuild. The old package is renamed to osbuild1.

New Image Types

A new implementation of the ImageType interface is added in the rhel84 distro which generates Manifests based on the new schema. It adds two new image types (rhel-edge-container image type for x86_64 and aarch64). The target creates a container that, when run, serves an OSTree commit.

General Notes

Image Type Name

I've been using rhel-edge-container while writing it and I see that name is also in @jkozol's draft PR in composer. I'm wondering if that's accurate for the container that's serving the commit. I was thinking about renaming it to rhel-edge-commit-container but that's a bit long, though I'm not sure it matters much one way or another.

Decoding types with interface fields

Types that contain interfaces with multiple implementations implement their own UnmarshalJSON() method. They use a JSON decoder with the DisallowUnknownFields option to catch errors during the deserialization while trying to determine which implementation matches the raw data.

Log output

Log output from OSBuild has a very different format when using the new schema. The osbuild1.Result object now supports unmarshalling the new format and adapting it to the old format. Refer to the corresponding commit message for more information.

Stage packages

The ImageType interface defines two sets of packages: Build packages and image packages. The former is used to set up the build environment (during the Build stage) and the latter is the list of packages that are meant to be installed in the final output image.

With the new Manifest, the idea of an explicit Build stage is gone. Instead we can define multiple Pipelines, each with its own set of stages. Each of these Pipelines might require its own set of packages. For the new image type, for example, we would need three package sets: One for the first pipeline (which in the old format would have been the build stage), one for the commit that will be delivered, and one for the container that serves the commit (which should only be httpd + dependencies). Since we can't define that last package set explicitly, the image type reuses the build packages for the container.

In the (very near) future, we should generalise the ImageType interface to allow defining a separate package set for each Pipeline in the Manifest.

@jkozol
Copy link
Contributor

jkozol commented Feb 22, 2021

I've been using rhel-edge-container while writing it and I see that name is also in @jkozol's draft PR in composer. I'm wondering if that's accurate for the container that's serving the commit. I was thinking about renaming it to rhel-edge-commit-container but that's a bit long, though I'm not sure it matters much one way or another.

I would prefer for it to remain rhel-edge-container since that is what we have specified in the UI.

@achilleas-k achilleas-k changed the title Osbuild schema 2 Support osbuild format v2 Feb 22, 2021
@gicmo
Copy link
Contributor

gicmo commented Feb 22, 2021

I've been using rhel-edge-container while writing it and I see that name is also in @jkozol's draft PR in composer. I'm wondering if that's accurate for the container that's serving the commit. I was thinking about renaming it to rhel-edge-commit-container but that's a bit long, though I'm not sure it matters much one way or another.

I would prefer for it to remain rhel-edge-container since that is what we have specified in the UI.

Not a big deal I think, but it is a good point that an "edge container" is a bit undefined, now that @achilleas-k mentions it.

Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

This is amazing! An impressive amount of work in very challenging circumstances. Love that this sets up the basics for the future, while making the right tradeoffs on temporary solutions. A couple of requests inline about the worker API.

internal/osbuild2/curl_source.go Show resolved Hide resolved
internal/distro/rhel84/distro_v2.go Outdated Show resolved Hide resolved
internal/worker/json.go Show resolved Hide resolved
cmd/osbuild-worker/jobimpl-osbuild.go Outdated Show resolved Hide resolved
@@ -123,3 +126,52 @@ func (cr *Result) Write(writer io.Writer) error {

return nil
}

func (cr *Result) UnmarshalJSON(data []byte) error {
Copy link
Member

Choose a reason for hiding this comment

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

This is obviously completely bonkers, but in a very impressive way. I think it hits the right balance between getting something useful and something we can use now. Let's go with this, and refactor once we have settled on the final format and have the luxury of more time.

Copy link
Member Author

Choose a reason for hiding this comment

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

This definitely needs a bit of discussion. We could define the output schema for osbuild and rely on it, or we could keep it flexible not bother processing it on the receiving end in composer. I think @gicmo told me he'd prefer it wasn't parsed and was assumed completely opaque, but I might be misremembering.

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with keeping this as-is for now, what do you think @gicmo? I agree that keeping it opaque might be nicer and all we really need (we just dump it as logs anyway), but we do anyway parse the result in some cases (koji), so I'm not sure we gain anything by not parsing it?

@achilleas-k achilleas-k force-pushed the osbuild-schema-2 branch 3 times, most recently from a940d54 to 2417ee0 Compare February 23, 2021 16:05
@achilleas-k
Copy link
Member Author

New commits add the --export flag to the osbuild call in the generate-test-cases script and generate a new test case for osbuild-container. For x86_64 I also regenerated the existing test cases to update them with some of the recent changes (kernel metapackage, new sources, new packages).

Marking PR as ready.

@achilleas-k achilleas-k marked this pull request as ready for review February 23, 2021 16:21
Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

This is great! Let's work with @henrywang to adapt the ostree-ng.sh test-case, and then this is good to go I think!
Forgot: had one nit, see inline comment.

@henrywang
Copy link
Member

@achilleas-k, I found worker unit file changed from osbuild-local-worker.service to osbuild-local-worker.socket. Will we change the unit file name forever? If yes, I need update test.
And will this change apply to both Fedora and RHEL 8.3? Thanks.

@henrywang
Copy link
Member

henrywang commented Feb 24, 2021

@achilleas-k, I can import edge container image without any error. But I can't run it.

[cloud-user@rhel-8-4 rhel-edge]$ podman image import 15461c29-8c15-49dc-80ad-1ec90ca751e8-rhel84-container.tar rhel-edge-container:latest
Getting image source signatures
Copying blob 681618047ca9 done
Copying config bb8c7f9749 done
Writing manifest to image destination
Storing signatures
bb8c7f97494a4c3d8ffc3ecf5f3efee8611fa4c798301f51a733bb39bfae838f

[cloud-user@rhel-8-4 rhel-edge]$ podman images
REPOSITORY                               TAG     IMAGE ID      CREATED        SIZE
docker.io/library/rhel-edge-container    latest  bb8c7f97494a  7 minutes ago  832 MB
registry.access.redhat.com/ubi8-minimal  latest  7331d26c1fdf  2 months ago   105 MB

[cloud-user@rhel-8-4 rhel-edge]$ podman run -d --network host docker.io/library/rhel-edge-container:latest
Error: no command or entrypoint provided, and no CMD or ENTRYPOINT from image

[cloud-user@rhel-8-4 rhel-edge]$ podman run -d --network host docker.io/library/rhel-edge-container:latest httpd -D
Error: container_linux.go:370: starting container process caused: exec: "httpd": executable file not found in $PATH: OCI not found

cloud-user@rhel-8-4 rhel-edge]$ podman run -d --network host docker.io/library/rhel-edge-container:latest /usr/sbin/httpd -D FOREGROUND
Error: container_linux.go:370: starting container process caused: exec: "/usr/sbin/httpd": stat /usr/sbin/httpd: no such file or directory: OCI not found

From code, httpd should be installed and it's run by CMD. But looks it does not work in this case.
Could you please take a look if you have time? Thanks.

@gicmo
Copy link
Contributor

gicmo commented Feb 24, 2021

@achilleas-k, I can import edge container image without any error. But I can't run it.

podman import is meant as a counter part of podman export and it is not specified which format is used.

Try one of the following:

  • podman pull oci-archive:15461c29-8c15-49dc-80ad-1ec90ca751e8-rhel84-container.tar
  • cat 15461c29-8c15-49dc-80ad-1ec90ca751e8-rhel84-container.tar | podman load

@achilleas-k
Copy link
Member Author

@henrywang I get the same behaviour when I import the file. @gicmo's suggestion should work.

As for the service/socket files: I don't think anything has changed there recently. The worker service depends on worker socket, so the socket starts automatically when you start the service. I think the test doesn't require any changes there.

@henrywang
Copy link
Member

@henrywang I get the same behaviour when I import the file. @gicmo's suggestion should work.

As for the service/socket files: I don't think anything has changed there recently. The worker service depends on worker socket, so the socket starts automatically when you start the service. I think the test doesn't require any changes there.

Thanks for clearing this.

  1. Test code in this line failed because worker unit file changed from osbuild-local-worker.service to osbuild-local-worker.socket. After I update code to osbuild-local-worker.socket, it works.
  2. @gicmo's comment works for me. But I still get error. Anything I missed? Thanks.
[cloud-user@rhel-8-4 rhel-edge]$ podman pull oci-archive:15461c29-8c15-49dc-80ad-1ec90ca751e8-rhel84-container.tar
Getting image source signatures
Copying blob 3e6464084597 done
Copying config 67607b0bec done
Writing manifest to image destination
Storing signatures
67607b0bece0b407c860dec7d20018c4c8ac396a805d7f3c276d23207f6c6e34

[cloud-user@rhel-8-4 rhel-edge]$ podman images
REPOSITORY                               TAG     IMAGE ID      CREATED       SIZE
<none>                                   <none>  67607b0bece0  6 hours ago   1.34 GB
registry.access.redhat.com/ubi8-minimal  latest  7331d26c1fdf  2 months ago  105 MB

[cloud-user@rhel-8-4 rhel-edge]$ podman run -d --name edge --network host 67607b0bece0
Error: container_linux.go:370: starting container process caused: exec: "httpd": executable file not found in $PATH: OCI not found

[cloud-user@rhel-8-4 rhel-edge]$ podman ps -a
CONTAINER ID  IMAGE         COMMAND               CREATED         STATUS   PORTS   NAMES
d2d3757bd2b7  67607b0bece0  httpd -D FOREGROU...  34 seconds ago  Created          edge

@teg
Copy link
Member

teg commented Feb 24, 2021

rather than unmarshaling the pipeline, could we just always name the last one "assembler"?

@achilleas-k
Copy link
Member Author

rather than unmarshaling the pipeline, could we just always name the last one "assembler"?

I suppose that would make things simpler.

@achilleas-k
Copy link
Member Author

2. @gicmo's comment works for me. But I still get error. Anything I missed? Thanks.

Found the issue there. https isn't added to the container. At some point while finalising the changes, I must have dropped it from the package list for the container. I'm fixing it now.

@achilleas-k
Copy link
Member Author

achilleas-k commented Feb 25, 2021

New commits contain some rebasing with older commits so here's a list of changes:

  • Added httpd to the BuildPackages() method of the new imageTypeS2.
  • Renamed the last pipeline for the new manifests to "assembler" for compatibility with the old format.
  • Changed the packages, excludedPackages, and enabledServices fields of the new image types to reuse the ones defined for the rhel-edge-commit image types since the payloads should be the same.
  • Added comments that mark code that's there for compatibility (or the transition) between v1 and v2 of the manifest format (issue opened for this as well).

teg
teg previously approved these changes Feb 25, 2021
Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

Thanks! I particularly like that you reuse the package sets/services now.

@gicmo
Copy link
Contributor

gicmo commented Feb 25, 2021

* Renamed the last pipeline for the new manifests to "assembler" for compatibility with the old format.

Heh, that seems like a hack. Not strongly against it, but is that meant to stay like that? Or just for now?

achilleas-k and others added 9 commits March 16, 2021 19:27
We need to add the URL to the manifest as an ostree source repo so that
osbuild can pull the commit to embed it in the boot ISO for the new
rhel-edge-installer image type.
New image type that generates a Boot ISO.  The ISO contains a RHEL Edge
commit and an installer.  On Boot, it sets up a new RHEL Edge system
with the commit.

The RHEL Edge commit (ostree commit) is downloaded during build from a
URL that should be supplied with the compose request.  The commit's hash
and URL need to be added to the Sources list in the Manifest.

Unlike other types, the new image type defines its own "build" package
set that is added to the distro and arch build package lists.
The new rhel-edge-installer requires unreleased fixes:
osbuild/osbuild#610
osbuild/osbuild#611
New stage option added in osbuild
osbuild/osbuild#611

System ID is used by osinfo to identify the RHEL boot ISOs, where the
system ID is "LINUX".
The ostree-ng will only be run on RHEL 8.4 because
rhel-edge-container and rhel-edge-installer image type are
supported by RHEL 8.4 only
We only support one combination for now, but let's stay compatible with the old tests.

This fixes the places where these variables are still used.
The public key should be osbuild-composer CI's key, not my local
test one
In the Anaconda pipeline, the kickstart stage should fetch the commit
we're embedding.  It was mistakenly trying to fetch from the URL used to
build the image instead.
@achilleas-k achilleas-k force-pushed the osbuild-schema-2 branch 3 times, most recently from 677a6b6 to 4af5cf6 Compare March 16, 2021 23:06
User home directories don't survive the rpm-ostree stage.  They are
converted to systemd-tmpfiles via rpm-ostree post-process, but the
contents are left behind, so any keys we add to the authorized_keys file
will be gone.

This stage sets up a first-boot service that writes the user's public
key to the file in the home directory during the first system boot.
henrywang and others added 2 commits March 17, 2021 14:14
It's been fixed already by commit 1b0e9e3
Pinning to v27: Released, but not available in repo snapshots yet.

Contains fix for home directories when using ostree:
osbuild/osbuild#613
For the new features, we need a very recent release of Anaconda.
Copy link
Member

@teg teg left a comment

Choose a reason for hiding this comment

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

Thanks to everyone who helped on this massive effort! @achilleas-k @henrywang and @gicmo in particular!

@teg teg merged commit 65ca29b into osbuild:main Mar 17, 2021
@ondrejbudai ondrejbudai mentioned this pull request Mar 17, 2021
@achilleas-k achilleas-k deleted the osbuild-schema-2 branch March 18, 2021 09:13
achilleas-k added a commit to achilleas-k/osbuild-composer that referenced this pull request Mar 18, 2021
Bug fix for changes introduced in osbuild#1244.

The new image types, rhel-edge-container and rhel-edge-installer, would
ignore the user-supplied ostree ref and use the default everywhere.

The default should only be used when a ref is not specified, which the
weldr API takes care of before calling the Manifest() method.
achilleas-k added a commit to achilleas-k/osbuild-composer that referenced this pull request Mar 18, 2021
Bug fix for changes introduced in osbuild#1244.

The new image types, rhel-edge-container and rhel-edge-installer, would
ignore the user-supplied ostree ref and use the default everywhere.

The default should only be used when a ref is not specified, which the
weldr API takes care of before calling the Manifest() method.
ondrejbudai pushed a commit that referenced this pull request Mar 18, 2021
Bug fix for changes introduced in #1244.

The new image types, rhel-edge-container and rhel-edge-installer, would
ignore the user-supplied ostree ref and use the default everywhere.

The default should only be used when a ref is not specified, which the
weldr API takes care of before calling the Manifest() method.
teg pushed a commit that referenced this pull request Mar 18, 2021
Bug fix for changes introduced in #1244.

The new image types, rhel-edge-container and rhel-edge-installer, would
ignore the user-supplied ostree ref and use the default everywhere.

The default should only be used when a ref is not specified, which the
weldr API takes care of before calling the Manifest() method.
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.

6 participants