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 /usr/lib/bootc/kargs.d support #401

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lukewarmtemp
Copy link
Contributor

@lukewarmtemp lukewarmtemp commented Mar 18, 2024

Fixes #255. Allows users to create files within /usr/lib/bootc/kargs.d with kernel arguments. These arguments can now be applied on a switch, upgrade, or edit.

General process:

  • use ostree commit of fetched container image to return the file tree
  • navigate to /usr/lib/bootc/kargs.d
  • read each file within the directory
  • push the contents of each file (kargs) into a vector containing all the desired kargs
  • pass the kargs to the stage() function

Example Containerfile:

FROM quay.io/fedora/fedora-coreos:stable

RUN mkdir -p /usr/lib/bootc/kargs.d
RUN cat <<EOF >> /usr/lib/bootc/kargs.d/00-console.toml
kargs = ["console=ttyS0,114800n8"]
supported-arch = ["x86_64"]
EOF

RUN cat <<EOF >> /usr/lib/bootc/kargs.d/01-mitigations.toml
kargs = ["mitigations=on", "systemd.unified_cgroup_hierarchy=0"]
supported-arch = ["x86_64", "aarch64"]
EOF

Looking for some input as to how files within /usr/lib/bootc/kargs.d should be formatted (which will also affect how the contents of the file are parsed)

Signed-off-by: Luke Yang luyang@redhat.com

@lukewarmtemp lukewarmtemp changed the title add /usr/lib/bootc/kargs.d support add /usr/lib/bootc/kargs.d support Mar 18, 2024
@lukewarmtemp lukewarmtemp force-pushed the luyang-kargs-support branch 3 times, most recently from 30f8e66 to b5a9e5b Compare March 18, 2024 19:22
Copy link
Collaborator

@cgwalters cgwalters 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 looking at this!

lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/cli.rs Show resolved Hide resolved
@cgwalters cgwalters added enhancement New feature or request area/updates Related to upgrading between versions area/linux-kernel Things related to the Linux Kernel labels Mar 18, 2024
Copy link
Collaborator

@cgwalters cgwalters left a comment

Choose a reason for hiding this comment

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

lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
lib/src/deploy.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

Let's bikeshed some of the file format/semantics.

One angle: I think there are use cases for conditionally-applied kernel arguments. One that very much comes up is the console= kernel argument being different on architectures and platforms - and dealing with that is painful for people deploying across platforms. We could just say "You define your own build process which writes to /usr/lib/bootc/kargs.d" which would be fine...but, something like what systemd does with .unit files or perhaps with .link files could make sense.

IOW

[match]
architecture = x86_64

kargs = ["console=ttyS0,114800n8"]

maybe?

BTW, a whole thing running through all of this is that I think a number of bootc use cases are better off actually generating custom kernel binaries, where you don't need or want this at all; that's something to keep in mind.

@cgwalters
Copy link
Collaborator

@lukewarmtemp
Copy link
Contributor Author

@cgwalters Do you know if there's there a way for Rust to detect what platform your system is running? I'm able to find and match against the arch of the system using std::env::consts::ARCH, but not sure if there's something similar for platform.

@jeckersb
Copy link
Contributor

@cgwalters Do you know if there's there a way for Rust to detect what platform your system is running? I'm able to find and match against the arch of the system using std::env::consts::ARCH, but not sure if there's something similar for platform.

There's a bunch of things you can trigger conditional compilation on, such as target_os, maybe that can cover what you need?

@cgwalters
Copy link
Collaborator

By "platform" I meant e.g. AWS vs GCP vs OpenStack etc. We're not going to build distinct bootc binaries for each of those, and runtime detection is "interesting". What Fedora CoreOS and derivatives do is https://github.com/coreos/fedora-coreos-tracker/blob/main/internals/README-internals.md#ignitionplatformid

At a practical level though I think we will likely end up with distinct container builds per "platform" in this sense, so we probably don't need to do dynamic dispatch here.

That said, for base images that do include ignition they'll need to have ignition.platform.id anyways...so in theory what we could support is a generic matching against an existing kernel argument (which would make this whole thing slightly recursive of course).

@jeckersb
Copy link
Contributor

By "platform" I meant e.g. AWS vs GCP vs OpenStack etc.

Ah gotcha. That's what I get for not reading back far enough for context. Carry on!

@cgwalters
Copy link
Collaborator

It's a bit ugly but we could also consider matching what Cargo does for architecture conditionals... https://doc.rust-lang.org/cargo/reference/specifying-dependencies.html#platform-specific-dependencies

Maybe even lift the code. Dunno.

I think my instinct here is that anything we do with kargs we should do with install configs too, as that's also a made up TOML thing but we may as well try to be consistent.

Specifically it makes sense to me to support "install only" kargs having architecture conditionals too.

@lukewarmtemp lukewarmtemp changed the base branch from main to renovate/all-patch May 8, 2024 18:52
@lukewarmtemp lukewarmtemp changed the base branch from renovate/all-patch to main May 8, 2024 18:52
@lukewarmtemp lukewarmtemp force-pushed the luyang-kargs-support branch 2 times, most recently from 398b24c to fc170bb Compare May 9, 2024 17:23
@cgwalters
Copy link
Collaborator

Thanks so much for working on this! The code is looking good at a very high level. We'll need to add documentation, but we can start on that once we're a bit more certain of the design.

I don't want to unnecessarily delay this, but I just keep honestly waffling on the file formats and location.

We could consider placing these files in e.g. /usr/lib/modules/$kver which is our canonical location for kernel data. Maybe /usr/lib/modules/$kver/kargs.d ? Uncertain...

A lot of things would get simpler if one actually just included the kargs in the kernel image (but, doing so breaks any upstream secure boot signatures, and just in general changes the binary, which is why it's not typically done outside of custom embedded builds).

lib/src/install/config.rs Outdated Show resolved Hide resolved
lib/src/kargs.rs Show resolved Hide resolved
lib/src/kargs.rs Outdated Show resolved Hide resolved
lib/src/kargs.rs Show resolved Hide resolved
lib/src/kargs.rs Show resolved Hide resolved
lib/src/kargs.rs Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

Also, an integration test would really help. But you don't need to block on writing it, I can help

@lukewarmtemp lukewarmtemp force-pushed the luyang-kargs-support branch 2 times, most recently from 4a08f48 to b5e4ead Compare May 22, 2024 15:17
@lukewarmtemp
Copy link
Contributor Author

We could consider placing these files in e.g. /usr/lib/modules/$kver which is our canonical location for kernel data. Maybe /usr/lib/modules/$kver/kargs.d ? Uncertain...

Any new thoughts relating to this? @cgwalters

lib/src/install/config.rs Outdated Show resolved Hide resolved
lib/src/install/config.rs Outdated Show resolved Hide resolved
lib/src/install/config.rs Outdated Show resolved Hide resolved
@cgwalters
Copy link
Collaborator

Any new thoughts relating to this?

I guess a downside of using the modules directory (aka /usr/lib/modules/$kver) is that anyone doing a derived build would need to be more careful to detect the container kernel version as is done for e.g. https://docs.fedoraproject.org/en-US/bootc/initramfs/#_regenerating_the_initrd and drop into that directory vs just a straight COPY 20-mykargs.toml /usr/lib/bootc/kargs.d.

So...I don't have anything better than /usr/lib/bootc/kargs.d. I feel somewhat uncomfortable in inventing a new thing though. One thing I'd like to do is try to standardize some of the bootc stuff in OCI upstream, and it'd be around exactly things like this.

@cgwalters
Copy link
Collaborator

One thing I'd like to do is try to standardize some of the bootc stuff in OCI upstream, and it'd be around exactly things like this.

➡️ https://groups.google.com/a/opencontainers.org/g/dev/c/OtLlhn4jxBg

lukewarmtemp added a commit to lukewarmtemp/bootc that referenced this pull request May 27, 2024
Required for containers#401

Adds a `EnvProperties` struct to allow for conditional merging of kargs
depending on whether the archiecture matches. Future properties such as
`platform.id` can be added in the future.

Signed-off-by: Luke Yang <luyang@redhat.com>
lukewarmtemp added a commit to lukewarmtemp/bootc that referenced this pull request May 27, 2024
Required for containers#401

Adds a `EnvProperties` struct to allow for conditional merging of kargs
depending on whether the archiecture matches. Future properties such as
`platform.id` can be added in the future.

Signed-off-by: Luke Yang <luyang@redhat.com>
lukewarmtemp added a commit to lukewarmtemp/bootc that referenced this pull request May 27, 2024
Required for containers#401

Adds a `EnvProperties` struct to allow for conditional merging of kargs
depending on whether the archiecture matches. Future properties such as
`platform.id` can be added in the future.

Signed-off-by: Luke Yang <luyang@redhat.com>
lukewarmtemp added a commit to lukewarmtemp/bootc that referenced this pull request May 27, 2024
Required for containers#401

Adds a `EnvProperties` struct to allow for conditional merging of kargs
depending on whether the archiecture matches. Future properties such as
`platform.id` can be added in the future.

Signed-off-by: Luke Yang <luyang@redhat.com>
@lukewarmtemp lukewarmtemp force-pushed the luyang-kargs-support branch 5 times, most recently from 34d7cd4 to a862e9e Compare May 29, 2024 15:28
Copy link
Collaborator

@cgwalters cgwalters 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 think this is generally looking good; the big gap I see is around integration testing. I'd hoped to be able to give you better guidance and tooling here by now. We'll continue to track that in #543

Short term, I think we can probably merge, and e.g. @henrywang can look at updating the integration tests there.

We're also going to need changes to the docs.

Can you also take an action item to do a merge request to https://gitlab.com/fedora/bootc/examples say?

lib/src/kargs.rs Outdated Show resolved Hide resolved
Comment on lines +33 to +34
// Get the running kargs of the booted system
if let Some(bootconfig) = ostree::Deployment::bootconfig(booted_deployment) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks correct, though I would say we should probably add a convenience helper to this in ostree.

Fixes containers#255. Allows users to create files within /usr/lib/bootc/kargs.d with kernel arguments. These arguments can now be applied on a switch, upgrade, or edit.

General process:
- use ostree commit of fetched container image to return
the file tree
- navigate to /usr/lib/bootc/kargs.d
- read each file within the directory
- calculate the diff between the booted and fetched kargs in kargs.d
- apply the diff to the kargs currently on the running system
- pass the kargs to the stage() function

Signed-off-by: Luke Yang <luyang@redhat.com>
@lukewarmtemp
Copy link
Contributor Author

the big gap I see is around integration testing

Oh sorry, I was going to look at this but it slipped my mind. I can probably have a crack at it to see if I can get it working. If I can't see to get it, I'll probably reach out to @henrywang.

Can you also take an action item to do a merge request to https://gitlab.com/fedora/bootc/examples say?

Will do.

@cgwalters cgwalters removed the reviewed/needs-rework Needs changes label Jun 4, 2024
@lukewarmtemp
Copy link
Contributor Author

MR updating bootc examples: https://gitlab.com/fedora/bootc/examples/-/merge_requests/48

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/linux-kernel Things related to the Linux Kernel area/updates Related to upgrading between versions enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for e.g. /usr/lib/bootc/kargs.d or equivalent
3 participants