Only do "booted BIOS with no update" check during actual update#1096
Conversation
|
Hi @alexlarsson. Thanks for your PR. I'm waiting for a coreos member to verify that this patch is reasonable to test. If it is, they should reply with Tip We noticed you've done this a few times! Consider joining the org to skip this step and gain Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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-sigs/prow repository. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new query_requires_update method to the Component trait, with specific implementations for BIOS and EFI components. This method is utilized to validate update requirements when no update metadata is found. A review comment correctly identified a typo in a code comment within the EFI implementation that incorrectly referenced BIOS instead of EFI.
alexlarsson
force-pushed
the
fix-bios-check
branch
from
6684492 to
38b1d4c
Compare
alexlarsson
force-pushed
the
fix-bios-check
branch
from
38b1d4c to
6ae578d
Compare
| fn query_update(&self, sysroot: &openat::Dir) -> Result<Option<ContentMetadata>>; | ||
|
|
||
| /// Used on the client to query if the booted systems requires an update of this component | ||
| fn query_requires_update(&self, sysroot: &openat::Dir) -> Result<()>; |
There was a problem hiding this comment.
imo we should accept Option<ContentMetadata> here so that we only return an error if the metadata is None. Otherwise just looking at the function, it's hard to figure out why we just error our if sys/firmware/efi doesn't exist for BIOS, when it not existing would always be true for BIOS booted systems
There was a problem hiding this comment.
Then the caller is quite weird if it always calls query_update() and then query_requires_update(). Right now it only calls query_requires_update() if metadata is None. If you want to do it like that, then I think having query_requires_update() call query_update() and then do the check is better than having to call both and passing the metadata forward.
There was a problem hiding this comment.
Right now it only calls query_requires_update() if metadata is None
right, and this is a little weird to me as it's not documented. Maybe the best approach is to simply document this in the function comments? Something like This function assumes that metadata for the component was None. It then throws an error if the system was booted with "component"?
There was a problem hiding this comment.
I pushed a new version with the function comment having a bit more detail for when this is called and what it should do.
This fixes coreos#1035 which introduced this check, but did so in every query_update(). However, that is called from non-update places too. In particular from install(), which causes bootc-image-builder to break if used on non-EFI systems. Instead we break out the check to a separate component function query_requires_update() which is only called in the update-with-missing-component case.
alexlarsson
force-pushed
the
fix-bios-check
branch
from
6ae578d to
9b582e1
Compare
|
It feels to me that we are working around the fact that we added this as a quick workaround but what we really need is #1040 instead. |
We need both though right? Or at least we need something like this which removes the dynamic check. |
|
/ok-to-test |
This is a temporary fix for bootupd 0.2.34 until coreos/bootupd#1096 is in the repo. It fixes bootupd failing inside to-disk-image if there is no /sys/firmware/efi on the build machine.
4 participants
This fixes #1035 which introduced this check, but did so in every query_update(). However, that is called from non-update places too. In particular from install(), which causes bootc-image-builder to break if used on non-EFI systems.
Instead we break out the check to a separate component function query_requires_update() which is only called in the update-with-missing-component case.