-
Notifications
You must be signed in to change notification settings - Fork 39
extend-payload-to-esp: extends any src_input_dir to esp #935
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
base: main
Are you sure you want to change the base?
Conversation
I am still trying to build an image with this feature , but with no luck,
After building the image with bib, I don't see the files in /boot/efi. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
src/efi.rs
Outdated
|
||
/// Copy firmware files from /boot/efi to target directory | ||
fn copy_firmware_blobs(&self, src_root: &openat::Dir, dest_root: &str) -> Result<()> { | ||
let src_efi = "boot/efi"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A core design principle that bootupd tries to maintain is that the operating system payload is underneath /usr
as much as possible.
Today in bootc (and prior ostree) we encourage /boot
to be empty in the container image. bootupd takes care at build time of moving the payloads to /usr
.
It also has infrastructure to keep track of the versions of things it installs so it knows when to update.
So the problem domain is just more complex than this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's more discussion in #766 (let's make that the canonical issue).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ran: #936 on my current implementation with an additional step of creating the files in /boot
:
# Build from the current git into a c9s-bootc container image.
# Use e.g. --build-arg=base=quay.io/fedora/fedora-bootc:41 to target
# Fedora or another base image instead.
#
ARG base=quay.io/centos-bootc/centos-bootc:stream9
FROM $base as build
# This installs our package dependencies, and we want to cache it independently of the rest.
# Basically we don't want changing a .rs file to blow out the cache of packages.
RUN <<EORUN
set -xeuo pipefail
dnf -y install cargo git openssl-devel
EORUN
RUN mkdir -p /boot/firmware/efi && \
touch /boot/firmware/efi/dummy.{dtb,dat,efi,elf,bin}
# Now copy the source
COPY . /build
WORKDIR /build
# See https://www.reddit.com/r/rust/comments/126xeyx/exploring_the_problem_of_faster_cargo_docker/
# We aren't using the full recommendations there, just the simple bits.
RUN --mount=type=cache,target=/build/target --mount=type=cache,target=/var/roothome \
make && make install-all DESTDIR=/out
# FROM $base
# # Clean out the default to ensure we're using our updated content
# RUN rpm -e bootupd
# COPY --from=build /out/ /
# # Sanity check this too
# RUN bootc container lint --fatal-warnings
From #755
Content shipped in an RPM with files in /boot that are present in the initial base image build where bootupctl backend generate-update-payload is invoked
The build container has the dummy files:
bash-5.2# ls /boot/firmware/efi/
dummy.bin dummy.dat dummy.dtb dummy.efi dummy.elf
but the final image that is built using the COPY --from=build /out/ /
doesn't have the files, 😕
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this function can be modified into a generic one to copy files, but I am not sure how to trigger it from the container build process
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but the final image that is built using the COPY --from=build /out/ / doesn't have the files, 😕
The build
image is only intended to hold the result of building the bootupd binary itself. If you want to modify the final image for other reasons (e.g. injecting kernel arguments, adding debug tools or other configuration, and as in this case inject arbitrary other content) then you should put it on the last stage.
Or better even - don't modify the dockerfile, make a new one and have it be FROM localhost/bootupd
.
Can you try out #936 ? It has several advantages, but the biggest will be that you can just |
Thank you for working on this. I propose that we continue design discussion (e.g. what the interface should look like) in #766 (comment) and I'll comment on the PR for code review. But a specific downside of what you're doing here is that it would require all tools calling bootupd to be changed (particularly bootc) and to have specific knowledge of what should be installed. Whereas the goal of bootupd is to abstract that from callers (like bootc) - the person building the container extends the payload, and bootc shouldn't need to know it's copying more files to the ESP. Also, crucially, my proposal there would also transparently handle updates, which is an important goal. |
634cf2f
to
9bac770
Compare
It should now stage the updates for any user input path, |
The goal has changed with iterations of the idea and other effort on: #938 , So I updated the PR's main goal on my first comment
Yes, i can keep checks (
Yes
Those are firmware files and uboot binary.
Here's how it plays in with #926 : |
This should now be inline with to what is required by #926 |
We need to teach generate-update-metadata in #938 to read from /firmware/ |
@kfox1111 I can possibly modify upgrade to do things like not to remove the old file if no new file. |
Agree with Colin and Timothee that there are partly duplication with #926, e.g. |
} | ||
|
||
fn extend_payload(&self, sysroot_path: &str, src_input: &str) -> Result<Option<bool>> { | ||
let dest_efidir_base = Path::new(sysroot_path).join("usr/lib/efi").join("firmware"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@HuijingHei do you suggest overloading the function to put files in usr/lib/efi/(firmware|shim|grub)
3781502
to
0412f72
Compare
Anything left? |
b97ea7f
to
cf10fc1
Compare
Nope, just rebased it. |
extend_payload_to_esp <path> will move the all contents of the <path> to /usr This features gives user ability to stage payloads so that install and update can install the contents to esp.It will move all the files and sub dirs under the path provide by user to /usr/lib/efi/firmware/<name>/<version>/EFI/ Incase of an exiting path extend-payload will only keep the latest version of the payload. example usage: `bootupctl backend extend_payload_to_esp /usr/lib/mydata` will move the content of the dir to `/usr/lib/efi/firmware/<pkg-name>/<version>/EFI/` stores metadata at `/usr/lib/efi/firmware/<pkg-name>/<version>/EFI.json`. Added test_extend_payload_to_esp unit test.
Added additional flow in efi::install to read from the path created by extend_payload_to_esp command and install to /boot/efi
efi::update is updated to read from path created by extend_payload_to_esp and update the firmware.
78ed7eb
to
fc199aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for the delay in review and thanks for working on this.
I have some initial comments.
run: | | ||
set -xeuo pipefail | ||
sudo truncate -s 5G myimage-extend.raw | ||
sudo podman run --rm --privileged -v .:/target --pid=host --security-opt label=disable \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not at all a new problem, and I won't say we should block on this but...
This is greatly exacerbating the problem that it's just a pain to run these tests outside of GHA (short of https://github.com/nektos/act etc.)
There's an effort to adapt our tests to TMT, but that gets into larger CI issues.
Anyways again not a blocker, but just noting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually thinking about this a bit more, why can't this test work how we'd expect things to work in production?
Basically if we change this to be a podman build
that installs the files instead that should all Just Work, right?
# Create a fake RPM database for testing | ||
mkdir -p /usr/lib/sysimage/rpm | ||
echo "fake rpm database" > /usr/lib/sysimage/rpm/Packages |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm....this seems highly likely to confuse other code in bootupd at some point.
As is right now, I am pretty sure it will result in bootupd not installing shim and grub, right?
Generate(super::bootupd::GenerateOpts), | ||
#[clap(name = "install", hide = true)] | ||
Install(super::bootupd::InstallOpts), | ||
#[clap(name = "extend-payload-to-esp", hide = true)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should hide = true
this - it's a user visible feature.
panic!("extend_payload returned None - expected success"); | ||
} | ||
Err(e) => { | ||
panic!("extend_payload failed when it should have succeeded: {}", e); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It'd avoid all this nesting to just use .unwrap()
or .expect()
on the result which seems preferable to me.
And we may as well do the same for the Option
|
||
let original_path = std::env::var("PATH").unwrap_or_default(); | ||
let new_path = format!("{}:{}", mock_rpm_dir.path().display(), original_path); | ||
std::env::set_var("PATH", &new_path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fully unsafe
for good reason in Rust 2024: https://doc.rust-lang.org/edition-guide/rust-2024/newly-unsafe-functions.html#stdenvset_var-remove_var
It's more painful but basically if we wanted to mock RPM we'd have to explicitly pass it as a parameter.
anyhow!("RPM query returned an empty or malformed version string") | ||
})?; | ||
|
||
let parts: Vec<&str> = version_string_part.split('-').collect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm @HuijingHei is reworking the version parsing in #977 etc.
format!( | ||
"{}-{}", | ||
parts[parts.len() - 2], | ||
parts[parts.len() - 1] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try using e.g.
match parts.as_slice() {
[name, version, release] => { ... }
_ => anyhow::bail!()
filetree::apply_diff(&updated, &destdir, &diff, None) | ||
.context("applying filesystem changes")?; | ||
// update firmware | ||
let firmware_base_dir_path = Path::new("usr/lib/efi/firmware"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does this path come from? Is it something you made up here?
let mut found_firmware = BTreeMap::new(); | ||
// Scan and install supplemental firmware | ||
let firmware_base_dir_path = Path::new("usr/lib/efi/firmware"); | ||
if src_root.exists(firmware_base_dir_path)? { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code past this is getting really deeply nested, can we factor out the inner part here into a helper function?
We had a chat a while back and this should now be possible with what we have in #938. |
Creates a test variant to evaluate bootupd PR #935's extend-payload-to-esp command as an alternative to the bootupctl shim approach. Key changes: - New pi-pr935 variant using Fedora 43 (Rawhide) - Builds bootupd from say-paul/bootupd firmware-copy branch - Uses bootupctl backend extend-payload-to-esp to stage firmware - No bootupctl shim required - Added task definitions for building container and disk images This will help test and provide feedback on coreos/bootupd#935 and #766 to move the ARM firmware support ecosystem forward. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
should fix: #651
fixes : #766
To support raspberry pi 4, which works well for rpm-ostree, but in bootc.
This PR add command
extend-payload-to-esp
which will enable bootup install to move the files to esp.usage:
what extend-payload-to-esp does:
bootupd install
andbootupd update
is updated to read from the above path and install to esp.rpi4 -usecase: fedora-iot/iot-distro#82 (comment)