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 an API+CLI to inject metadata for bootable OSTree commits #2296

Merged
merged 1 commit into from
Mar 12, 2021

Conversation

cgwalters
Copy link
Member

I was doing some rpm-ostree work and I wanted to compare two
OSTree commits to see if the kernel has changed. I think
this should be a lot more natural.

Add ostree commit --bootable which calls into a new generic
library API ostree_commit_metadata_for_bootable() that
discovers the kernel version and injects it as an ostree.linux
metadata key. And for extra clarity, add an ostree.bootable
key.

It's interesting because the "core" OSTree layer is all about
generic files, but this is adding special APIs around bootable
OSTree commits (as opposed to e.g. flatpak as well as
things like rpm-ostree's pkgcache refs).

Eventually, I'd like to ensure everyone is using this and
hard require this metadata key for the ostree admin deploy
flow - mainly to prevent accidents.

@cgwalters cgwalters force-pushed the commit-os branch 2 times, most recently from 32b3ec2 to 8556e52 Compare March 11, 2021 20:51
@cgwalters
Copy link
Member Author

OK yeah we need to drop Travis, it's been dead for a while due to the Docker rate limiting. I may look at GH actions for that.
/override continuous-integration/travis-ci/pr

@openshift-ci-robot
Copy link
Collaborator

@cgwalters: Overrode contexts on behalf of cgwalters: continuous-integration/travis-ci/pr

In response to this:

OK yeah we need to drop Travis, it's been dead for a while due to the Docker rate limiting. I may look at GH actions for that.
/override continuous-integration/travis-ci/pr

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

src/libostree/ostree-repo-os.h Outdated Show resolved Hide resolved
src/libostree/ostree-repo-os.c Show resolved Hide resolved
gboolean
ostree_commit_metadata_for_bootable (GFile *root, GVariantDict *dict, GCancellable *cancellable, GError **error)
{
g_autoptr(GFile) modules = g_file_resolve_relative_path (root, "usr/lib/modules");
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah...hard to share logic because using the libostree API abstraction means the content to be committed may not be realized as physical files, and we need to support that here. (For example, this API allows reading from an archive repository directly, and while it's usually better to have a checkout, it can be very convenient to do some operations on an archive repo directly)

Whereas the sysroot logic always operates on a bare repo with real files.

That logic is also trying to handle the legacy cases, which this one is explicitly not supporting.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Missing manpage and bash completion (which yeah, would be nice if that was automatic). Otherwise, LGTM!

I was doing some rpm-ostree work and I wanted to compare two
OSTree commits to see if the kernel has changed.  I think
this should be a lot more natural.

Add `ostree commit --bootable` which calls into a new generic
library API `ostree_commit_metadata_for_bootable()` that
discovers the kernel version and injects it as an `ostree.linux`
metadata key.  And for extra clarity, add an `ostree.bootable`
key.

It's interesting because the "core" OSTree layer is all about
generic files, but this is adding special APIs around bootable
OSTree commits (as opposed to e.g. flatpak as well as
things like rpm-ostree's pkgcache refs).

Eventually, I'd like to ensure everyone is using this and
hard require this metadata key for the `ostree admin deploy`
flow - mainly to prevent accidents.
@cgwalters
Copy link
Member Author

Missing manpage and bash completion (which yeah, would be nice if that was automatic).

Yeah, I was reading clap-rs/clap#552 recently.

Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

LGTM

@jlebon
Copy link
Member

jlebon commented Mar 12, 2021

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit d522f26 into ostreedev:master Mar 12, 2021
@lucab
Copy link
Member

lucab commented Mar 15, 2021

In general, is ostree supposed to handle multiple (inactive) kernel images in a commit?
The second revision of this PR started enforcing that there is a single vmlinuz entry, but I do wonder whether the public API would be more useful if it also takes a (version or path) hint to locate the primary kernel image.

@cgwalters
Copy link
Member Author

In general, is ostree supposed to handle multiple (inactive) kernel images in a commit?

No; we should only have one. OSTree is all about a mechanism to transactionally swap between versioned bootable filesystem trees. We don't have code to install or switch to alternative kernels inside a single ostree commit. Instead, OS builders can ship multiple OSTree commits, and switch between them client or server side. These ostree commits will share storage via hardlinks.

This is very relevant to OpenShift where we want to expose the ability to switch between the mainline kernel and kernel-rt. Today the MCO uses rpm-ostree client side overrides but down the line I'd like to have it be a pre-built ostree commit.

@cgwalters
Copy link
Member Author

Or another way to say this is - handling multiple kernel versions is no different than handling multiple versions of any other (userspace) component of the ostree payload, e.g. systemd - those are distinct commits which one can generate client or server side; it wouldn't make much sense to offer a way to switch systemd versions inside a single commit.

@lucab
Copy link
Member

lucab commented Mar 15, 2021

@cgwalters thanks for the explanation, it clarified all my doubts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants