Add hooks for deployment steps to allow bootloader swapping #3539
Conversation
|
Hi @Mstrodl. Thanks for your PR. I'm waiting for a github.com member to verify that this patch is reasonable to test. If it is, they should reply with 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. |
Mstrodl
changed the title
![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 adds support for a bootloader-plugin by introducing hooks into the deployment process. The overall logic is sound, with hooks for deploy and swap actions placed at appropriate stages. My main feedback concerns code duplication in the plugin invocation logic, which could be refactored into a helper function for better maintainability. Additionally, the path to the plugin executable is hardcoded, and making it configurable would enhance flexibility for different system layouts.
| const char *const bootplugin_deploy_argv[] = { "/usr/lib/ostree/bootloader-plugin", "deploy", NULL }; | ||
| bootplugin_exists = g_file_test(bootplugin_deploy_argv[0], G_FILE_TEST_EXISTS); | ||
| if (bootplugin_exists) | ||
| { | ||
| if (!g_spawn_sync(NULL, (char **) bootplugin_deploy_argv, NULL, G_SPAWN_CHILD_INHERITS_STDERR, | ||
| NULL, NULL, NULL, NULL, &estatus, error)) { | ||
| return glnx_prefix_error (error, "bootloader-plugin deploy exec failed"); | ||
| } | ||
| if (!g_spawn_check_exit_status (estatus, error)) { | ||
| return glnx_prefix_error (error, "bootloader-plugin deploy failed"); | ||
| } | ||
| } | ||
|
|
||
| if (!full_system_sync (self, cancellable, error)) | ||
| return FALSE; | ||
|
|
||
| if (!swap_bootloader (self, bootloader, self->bootversion, new_bootversion, cancellable, error)) | ||
| return FALSE; | ||
|
|
||
| const char *const bootplugin_swap_argv[] = { bootplugin_deploy_argv[0], "swap", NULL }; | ||
| if (bootplugin_exists) | ||
| { | ||
| if (!g_spawn_sync(NULL, (char **) bootplugin_swap_argv, NULL, G_SPAWN_CHILD_INHERITS_STDERR, | ||
| NULL, NULL, NULL, NULL, &estatus, error)) { | ||
| return glnx_prefix_error (error, "bootloader-plugin swap exec failed"); | ||
| } | ||
| if (!g_spawn_check_exit_status (estatus, error)) { | ||
| return glnx_prefix_error (error, "bootloader-plugin swap failed"); | ||
| } | ||
| } |
There was a problem hiding this comment.
This block of code has a couple of areas for improvement:
-
Code Duplication: The logic for spawning the
bootloader-pluginfor bothdeployandswapactions is nearly identical. This duplicated code should be refactored into a static helper function to improve maintainability and reduce redundancy. The helper could take the command arguments and an action name (e.g., "deploy") to generate contextual error messages. -
Hardcoded Path: The plugin path
"/usr/lib/ostree/bootloader-plugin"is hardcoded withinbootplugin_deploy_argv. This reduces portability. Consider defining this path as aconst char*at the top of the function and making it configurable via the repository'sconfigfile, falling back to the current hardcoded path if not specified.
|
By the way if this is not something you guys want to take, that's fine too. I just figured other people might find it useful too :) |
Can be used to provide hooks into a sysroot deploy for tools like bootupd to perform bootloader swaps. Happy to do this a different way, but this is how we've been doing it on our debian-based system. I understand bootupd is supposed to work a different way. We do it this way because grub-mkconfig will generate a broken config for the older version of grub.
Mstrodl
force-pushed
the
feature/mstrodl/bootplugin
branch
from
56b1834 to
379f414
Compare
|
One important info from #3496 that you forgot to repeat here is that Debian version of grub doesn't support BLS (yet), so you can't use bootupd static config :( |
2 participants
Can be used to provide hooks into a sysroot deploy for tools like bootupd to perform bootloader swaps.
Happy to do this a different way, but this is how we've been doing it on our debian-based system. I understand bootupd is supposed to work a different way. We do it this way because grub-mkconfig will generate a broken config for the older version of grub.
See #3496 for motivation