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

This commit does removes the assumption of a boot folder (/boot) just for grub setup #1920

Merged
merged 1 commit into from
Jan 30, 2024

Conversation

davidcassany
Copy link
Contributor

With this commit all grub binaries (EFI images and modules) and configurations are installed always in defined prefixes.

Defined prefixes are always appended to the EFI path, which is usually the EFI partition mountpoint at run time.

This causes a duplication of data if multiple prefixes are defined.

In addition this commit also removes unused variables.

@davidcassany davidcassany requested a review from a team as a code owner January 29, 2024 17:22
Copy link
Contributor Author

@davidcassany davidcassany left a comment

Choose a reason for hiding this comment

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

I think the main issue was a missing grub.cfg in EFI partition while creating the ISO, however I did some more changes in order to simplify the setup as was getting a bit confused by unused variables and duplicated variables (bootDir and efiDir were always the same and not really properly distinguished).

In the future we can probably argue if it is needed a more standard setup in EFI that acually looks more like traditional installation:

/EFI/BOOT/<efi binaries and small static grub.cfg>
/EFI/ELEMENTAL/<efi binaries and small static grub.cfg>
/grub2/<the actual grub.cfg and grub modules>
/<kernel and initrd for systemdboot or ISOs>

@@ -187,27 +186,30 @@ func (b *BuildISOAction) ISORun() error {
}

func (b *BuildISOAction) PrepareEFI(rootDir, uefiDir string) error {
return b.bootloader.InstallEFIFallbackBinaries(rootDir, uefiDir, b.spec.Label)
err := b.renderGrubTemplate(uefiDir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

EFI partition was missing the grub.cfg, included in the ISO filesystem but not in the EFI (vfat) filesystem.

}

func (b *BuildISOAction) PrepareISO(rootDir, imageDir string) error {
err := utils.MkdirAll(b.cfg.Fs, filepath.Join(imageDir, grubPrefixDir), constants.DirPerm)
// Include EFI contents in iso root too
return b.PrepareEFI(rootDir, imageDir)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

With this commit PrepareEFI and PrepareISO are just identical, both simply prepare EFI bootloader. One is used while booting as an ISO and the other while booting the ISO as a disk (dd ISO to a raw image and boot it as a disk)

@codecov-commenter
Copy link

codecov-commenter commented Jan 29, 2024

Codecov Report

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

Comparison is base (c2d9965) 73.91% compared to head (91ba5f9) 73.92%.

Files Patch % Lines
pkg/action/build-iso.go 75.00% 2 Missing and 1 partial ⚠️
pkg/mocks/bootloader_mock.go 40.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1920      +/-   ##
==========================================
+ Coverage   73.91%   73.92%   +0.01%     
==========================================
  Files          71       71              
  Lines        7762     7750      -12     
==========================================
- Hits         5737     5729       -8     
+ Misses       1600     1597       -3     
+ Partials      425      424       -1     

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

@@ -17,12 +17,11 @@ limitations under the License.
package v1

type Bootloader interface {
Install(rootDir, bootDir, deviceLabel string) (err error)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Device label was completely unused

InstallConfig(rootDir, bootDir string) error
DoEFIEntries(shimName, efiDir string) error
InstallEFI(rootDir, bootDir, efiDir, deviceLabel string) error
Copy link
Contributor Author

Choose a reason for hiding this comment

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

bootDir and efiDir were set always to the same value and the code is not really prepared to properly use them as different concepts.

With this commit all grub binaries (EFI images and modules)
and configurations are installed always in defined prefixes.

Defined prefixes are always appended to the EFI path, which
is usually the EFI partition mountpoint at run time.

This causes a duplication of data if multiple prefixes are
defined.

Signed-off-by: David Cassany <dcassany@suse.com>
@davidcassany davidcassany changed the title This commit does removes the assumption of a boot folder (/boot) This commit does removes the assumption of a boot folder (/boot) just for grub setup Jan 30, 2024
@davidcassany davidcassany merged commit 24b19bc into rancher:main Jan 30, 2024
14 checks passed
@davidcassany davidcassany deleted the cleanup_bootloader branch January 30, 2024 10:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants