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

Extract recovery kernel #2016

Merged
merged 5 commits into from
Mar 20, 2024
Merged

Extract recovery kernel #2016

merged 5 commits into from
Mar 20, 2024

Conversation

frelon
Copy link
Contributor

@frelon frelon commented Mar 18, 2024

This PR refactors the deployment of the recovery-system in our different code-branches according to the ADR.

  • build-iso
  • build-disk
  • install
  • upgrade-recovery

Fixes #1864 #1930

frelon added 2 commits March 15, 2024 15:53
Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
Refactors build-iso command to use the new DeployRecoverySystem method.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2024

Codecov Report

Attention: Patch coverage is 59.52381% with 51 lines in your changes are missing coverage. Please review.

Project coverage is 72.34%. Comparing base (a2c4f0b) to head (ef40072).

Files Patch % Lines
pkg/elemental/elemental.go 62.02% 22 Missing and 8 partials ⚠️
pkg/action/install.go 30.00% 6 Missing and 1 partial ⚠️
pkg/action/build-disk.go 54.54% 4 Missing and 1 partial ⚠️
pkg/action/upgrade-recovery.go 37.50% 4 Missing and 1 partial ⚠️
pkg/action/build-iso.go 69.23% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2016      +/-   ##
==========================================
- Coverage   72.56%   72.34%   -0.22%     
==========================================
  Files          76       76              
  Lines        8907     8992      +85     
==========================================
+ Hits         6463     6505      +42     
- Misses       1910     1944      +34     
- Partials      534      543       +9     

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

Use DeployRecoverySystem to deploy the recovery system.

Needs some changes to grub.cfg to be fully compatible and also extracts
the bootargs.cfg file into the recovery partition.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon changed the title Extract kernel Extract recovery kernel Mar 18, 2024
Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon linked an issue Mar 19, 2024 that may be closed by this pull request
This commit changes the DeployRecoverySystem method to remove any
conflicting boot artifacts before copying the new files.

Also adds power and squashfs compression flags to the command.

Signed-off-by: Fredrik Lönnegren <fredrik.lonnegren@suse.com>
@frelon frelon marked this pull request as ready for review March 19, 2024 09:47
@frelon frelon requested a review from a team as a code owner March 19, 2024 09:47
target := filepath.Join(bootDir, filepath.Base(file))
if exist, _ := utils.Exists(cfg.Fs, target); exist {
cfg.Logger.Debugf("Removing old file %s", target)
err = cfg.Fs.Remove(target)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is to make sure we deploy the newest files.

On error we could potentially leave the system unbootable because of a mix of kernel/initrd/grub-bootargs.

I think this is safe since we are updating from the active system, but we could consider deploying the artifacts into a temporary boot-dir and then moving it to <recovery>/boot once everything is deployed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes touching bootloader binaries is tricky. In fact this is also a valid concern we currently have for upgrades too when using the bootloader install flag. I'd be happy to brainstorm a little around this topic, if someone faces issues in that area, even it is unlikely, would be quite frustrating. I'd be in favor to apply some more elaborated logic, the question here is which would be the desired behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed! I will create a ticket to stabilize the upgrade! It should probably include some integration tests as well.

Copy link
Contributor

@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.

Nice! Great job 👍

I am wondering if shouldn't we create an issue to discuss bootloader upgrade. Probably we should define some better approach to be more robust in case of failures.

@@ -90,7 +90,7 @@ build-iso:
mkdir -p $(ROOT_DIR)/build
$(DOCKER) run --rm -v $(DOCKER_SOCK):$(DOCKER_SOCK) -v $(ROOT_DIR)/build:/build \
--entrypoint /usr/bin/elemental $(TOOLKIT_REPO):$(VERSION) --debug build-iso --bootloader-in-rootfs -n elemental-$(FLAVOR).$(ARCH) \
--local --platform $(PLATFORM) --squash-no-compression -o /build $(REPO):$(VERSION)
--local --platform $(PLATFORM) -o /build $(REPO):$(VERSION)
Copy link
Contributor

Choose a reason for hiding this comment

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

Will it default to compressed squashfs images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only for our example isos/disks since I noticed we saved ~50MB on the final image which could speed up CI sligthly.

FS: constants.SquashFs,
}

err = elemental.DeployRecoverySystem(b.cfg.Config, image, bootDir)
Copy link
Contributor

Choose a reason for hiding this comment

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

I really like this method 👍

target := filepath.Join(bootDir, filepath.Base(file))
if exist, _ := utils.Exists(cfg.Fs, target); exist {
cfg.Logger.Debugf("Removing old file %s", target)
err = cfg.Fs.Remove(target)
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes touching bootloader binaries is tricky. In fact this is also a valid concern we currently have for upgrades too when using the bootloader install flag. I'd be happy to brainstorm a little around this topic, if someone faces issues in that area, even it is unlikely, would be quite frustrating. I'd be in favor to apply some more elaborated logic, the question here is which would be the desired behavior.

@frelon frelon merged commit d61a24c into rancher:main Mar 20, 2024
17 checks passed
@frelon frelon deleted the extract-kernel branch March 20, 2024 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move recovery kernel and initrd out of recovery image Default to squashfs for recovery partition
3 participants