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

Grub refactor and other distros #1858

Closed

Conversation

davidcassany
Copy link
Contributor

@davidcassany davidcassany commented Nov 17, 2023

This commit adds a new bootloader interface and provides a grub based implementation for it.

The grub implementation is moved to its own bootloader package and the implementation is refactor to not relay on distro detection anymore, but instead it searches the relevant files (efi image, shim, grub modules, etc.) based on path patterns. The search is done by trying to find a file matching the given pattern, first match wins.

In addition, in order to make the implementation even more flexible, the first path where elemental tries to fine bootloader artifacts is an elemental custom path (currently /usr/lib/elemental/bootloader), this is giving us a chance to set or copy the bootloader artifacts we want into custom path within the Dockerfile to ensure elemental client will later be capable to find them at install time (see Ubuntu example).

Finally the refactor also changes the init command to ensure the initramfs and kernel are set with the appropriate names in init phase. So we are certain init is consistent with the default grub setup.

@davidcassany davidcassany requested a review from a team as a code owner November 17, 2023 20:27
@codecov-commenter
Copy link

codecov-commenter commented Nov 17, 2023

Codecov Report

Attention: 144 lines in your changes are missing coverage. Please review.

Comparison is base (6358862) 75.29% compared to head (8576c3f) 75.97%.
Report is 4 commits behind head on main.

Files Patch % Lines
pkg/bootloader/grub.go 77.93% 50 Missing and 14 partials ⚠️
pkg/mocks/bootloader_mock.go 52.50% 16 Missing and 3 partials ⚠️
pkg/action/build-disk.go 67.74% 7 Missing and 3 partials ⚠️
pkg/action/reset.go 60.00% 8 Missing and 2 partials ⚠️
pkg/utils/common.go 89.89% 7 Missing and 3 partials ⚠️
pkg/action/init.go 75.00% 5 Missing and 2 partials ⚠️
pkg/action/install.go 66.66% 5 Missing and 2 partials ⚠️
pkg/action/build-iso.go 89.28% 4 Missing and 2 partials ⚠️
pkg/action/upgrade.go 80.00% 3 Missing and 1 partial ⚠️
pkg/types/v1/platform.go 0.00% 4 Missing ⚠️
... and 1 more
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1858      +/-   ##
==========================================
+ Coverage   75.29%   75.97%   +0.68%     
==========================================
  Files          67       66       -1     
  Lines        6825     6915      +90     
==========================================
+ Hits         5139     5254     +115     
+ Misses       1315     1290      -25     
  Partials      371      371              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@kkaempf kkaempf left a comment

Choose a reason for hiding this comment

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

I can't (easily) review a PR with 47(!) changed files

cmd/build-iso.go Show resolved Hide resolved
examples/blue/Dockerfile Outdated Show resolved Hide resolved
@davidcassany
Copy link
Contributor Author

@kkaempf alright, orange flavor is finally added too. Now the test matrix is huge and, unfortunately, the macos environment we use is still flaky a bit, hence with such a big matrix is not rare that one or two tests fail. Retrying makes them pass.

On the good side tests are easily reproducible on the local environment, hence if there is a non environment related issue we can easily reproduce it without the need of github actions.

@davidcassany davidcassany requested a review from a team November 21, 2023 12:08
Copy link
Contributor

@frelon frelon left a comment

Choose a reason for hiding this comment

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

LGTM, ship it 🚀

pkg/action/init.go Show resolved Hide resolved
@davidcassany davidcassany force-pushed the grub_refactor_and_other_distros branch from ce0ad86 to 21d62f5 Compare November 22, 2023 16:13
@davidcassany davidcassany added the status/blocked Issue depends on another one label Nov 23, 2023
@davidcassany
Copy link
Contributor Author

Adding blocked label as I think we should sort #1856 first, as the two PR conflict and I do believe it is easier to rebase this PR than the other way round.

@davidcassany davidcassany force-pushed the grub_refactor_and_other_distros branch from 21d62f5 to a27fd26 Compare November 27, 2023 21:42
@davidcassany davidcassany removed the status/blocked Issue depends on another one label Nov 27, 2023
@davidcassany davidcassany force-pushed the grub_refactor_and_other_distros branch 2 times, most recently from 73b41a4 to 8cf2973 Compare November 27, 2023 22:05
@davidcassany davidcassany self-assigned this Nov 28, 2023
This commit moves grub logic into its own bootloader interface.

In addition it adds helper methods to find EFI binaries, kernel and
initrd based on patterns. No longer a distro detection is required.

It also sets an elemental criteria for those bootloader files. In fact
first place to look at is /boot/efi/EFI/elemental, which gives a chance
within the OS Dockerfile to prepare EFI binaries if default distro
paths are not matching any of the default Elemental patterns.

Kernel and initrd symlinks as /boot/vmlinuz and /boot/initrd are also
created within the init command. This gives at build time more
confidence that the kernel and initrd are set consistently with
Elemental expectations.

As part of the refactor BIOS firmware and MSDOS partition tables support
is finally dropped.

Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
…t and efi values respectively

Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany force-pushed the grub_refactor_and_other_distros branch from 8cf2973 to 8576c3f Compare November 30, 2023 08:54
@davidcassany
Copy link
Contributor Author

rebased on top of #1856 and #1859

Copy link
Contributor

@kkaempf kkaempf left a comment

Choose a reason for hiding this comment

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

Unreviewable with 52 changed files :-(

@davidcassany
Copy link
Contributor Author

davidcassany commented Nov 30, 2023

This PR has been split in two, first the refactor itself, and then additional examples of other distros. This way the PR is slightly better to manage and also ends up in more organized and better git history.

Refactor PR #1867
Additional distros #1868

@davidcassany
Copy link
Contributor Author

closing in favor of #1867 and #1868

@davidcassany davidcassany deleted the grub_refactor_and_other_distros branch November 30, 2023 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants