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

Merged
merged 1 commit into from
Jun 11, 2024

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
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.

@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

@cgwalters
Copy link
Collaborator

I tested this out and hit:

OSTree:ERROR:src/libostree/ostree-repo-file.c:828:ostree_repo_file_tree_query_child: assertion failed: (self->tree_contents)
Bail out! OSTree:ERROR:src/libostree/ostree-repo-file.c:828:ostree_repo_file_tree_query_child: assertion failed: (self->tree_contents)

lib/src/kargs.rs Outdated
.downcast::<ostree::RepoFile>()
.expect("downcast");
if !fetched_tree.query_exists(cancellable) {
return Ok(Default::default());
Copy link
Collaborator

Choose a reason for hiding this comment

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

I also hit on the fact that my test machine didn't come up across an update, and I think this is the cause: this must be return Ok(existing_kargs); right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it might be

kargs.append(&mut existing_kargs);
return Ok(kargs);

Since the kargs vec contains the existing kargs on the running system.

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 lukewarmtemp merged commit 540794d into containers:main Jun 11, 2024
9 checks passed
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