overlord/fdestate: run post install checks during auto repair - #16627
Merged
valentindavid merged 1 commit intoJun 3, 2026
Merged
Conversation
valentindavid
added
the
Run nested
github-actions
Bot
added
the
Run only one system
|
Wed Jun 3 09:21:45 UTC 2026 Failures:Executing:
Skipped tests from snapd-testing-skipIf you wish to have any of the below tests run in your PR, in your PR description, add 'unskip:' followed by a copy-and-pasted list (without variants) of the below tests you wish to run (unskip plus test list must be valid yaml)
|
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
3 times, most recently
from
February 20, 2026 10:08
e7fa4b5 to
01afcdd
Compare
github-actions
Bot
added
the
Needs Documentation -auto-
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
April 10, 2026 07:23
01afcdd to
70051ef
Compare
github-actions
Bot
removed
the
Needs Documentation -auto-
valentindavid
marked this pull request as ready for review
April 10, 2026 08:58
valentindavid
removed
the
Run only one system
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
2 times, most recently
from
April 10, 2026 09:36
c01e2a5 to
4975a6c
Compare
Member
Author
|
This will need a spread test. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #16627 +/- ##
==========================================
+ Coverage 79.00% 79.03% +0.03%
==========================================
Files 1376 1367 -9
Lines 192016 192041 +25
Branches 2464 2464
==========================================
+ Hits 151693 151786 +93
+ Misses 31180 31089 -91
- Partials 9143 9166 +23
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
April 14, 2026 09:31
4975a6c to
ff7d44d
Compare
Member
Author
I could not add a spread test. While the spread test do exercise it, they do not do the case where post install check would fail. Unfortunately, most check fails are either disabled on VMs or require interaction with OVMF menu. Instead I manually tested it by disabling secure boot and making sure the state had the right value. |
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
April 14, 2026 09:37
ff7d44d to
70568ab
Compare
| } | ||
|
|
||
| const postInstall = true | ||
| if _, details, err := secbootPreinstallCheck(context.Background(), postInstall, images); len(details) > 0 || err != nil { |
Member
There was a problem hiding this comment.
Is there a reason we are not logging the error details here?
| return nil, err | ||
| } | ||
|
|
||
| for _, runModeBootChain := range runModeBootChains { |
Member
There was a problem hiding this comment.
This loop warrants some comments
Member
Author
|
I have to rebase... |
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
April 28, 2026 11:24
fcf7faa to
db1e994
Compare
ZeyadYasser
reviewed
Apr 28, 2026
| // To support testing, when the system is running in a Virtual Machine, the check | ||
| // configuration is modified to permit this to avoid an error. | ||
| func PreinstallCheck(ctx context.Context, bootImagePaths []string) (*PreinstallCheckContext, []PreinstallErrorDetails, error) { | ||
| func PreinstallCheck(ctx context.Context, postInstall bool, bootImagePaths []bootloader.BootFile) (*PreinstallCheckContext, []PreinstallErrorDetails, error) { |
Contributor
There was a problem hiding this comment.
Might be worth having a separate PostinstallCheck helper, passing a post-install flag to PreinstallCheck is a little confusing even if the underlying secboot calls are like this
Member
Author
There was a problem hiding this comment.
I wonder about the returned "PreinstallCheckContext" "PreinstallErrorDetails". What should they be renamed to?
Contributor
There was a problem hiding this comment.
something like "RunChecksContext" "RunChecksErrorDetails"? but I agree that might be a lot to refactor in this PR so maybe a follow up.
ZeyadYasser
approved these changes
Apr 30, 2026
ZeyadYasser
left a comment
ZeyadYasser
left a comment
Contributor
There was a problem hiding this comment.
LGTM, Thank you! small nitpick comments
| return rs.State, nil | ||
| } | ||
|
|
||
| func getBootChain() ([]bootloader.BootFile, error) { |
Contributor
There was a problem hiding this comment.
does it make sense to move this under boot/seal.go?
Member
Author
There was a problem hiding this comment.
I think it would be nice to factorize with runModeBootChainsWithTrustedAssets. But it is not that easy. I will open a ticket to remember to do that.
Contributor
There was a problem hiding this comment.
this is the run boot chain? maybe the function name should reflect that?
Member
Author
|
I have to rebase again... |
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
May 5, 2026 12:15
95f15bd to
c7b0f00
Compare
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
2 times, most recently
from
May 7, 2026 12:21
89ed402 to
179d439
Compare
ernestl
reviewed
May 19, 2026
| return AutoRepairNotAttempted, err | ||
| } | ||
|
|
||
| if _, details, err := secbootPostinstallCheck(context.Background(), images); len(details) > 0 || err != nil { |
Member
There was a problem hiding this comment.
secbootPreinstallCheck where used in install.go uses context timeout. Considering adding.
| return AutoRepairNotAttempted, err | ||
| } | ||
|
|
||
| if _, details, err := secbootPostinstallCheck(context.Background(), images); len(details) > 0 || err != nil { |
Member
There was a problem hiding this comment.
Should the use if secbootPostinstallCheck be limited to classic hybrid systems >= 25.10? (same as secbootPreinstallCheck)
Member
Author
There was a problem hiding this comment.
That is a very good point.
| break | ||
| } | ||
|
|
||
| // In theory we should only have one hash here. Multiple would be when we are trying |
Member
There was a problem hiding this comment.
Perhaps test and warn if somehow this is not the case?
pedronis
reviewed
May 25, 2026
| // To support testing, when the system is running in a Virtual Machine, the check | ||
| // configuration is modified to permit this to avoid an error. | ||
| func PreinstallCheck(ctx context.Context, bootImagePaths []string) (*PreinstallCheckContext, []PreinstallErrorDetails, error) { | ||
| func preinstallCheck(ctx context.Context, postInstall bool, bootImagePaths []bootloader.BootFile) (*PreinstallCheckContext, []PreinstallErrorDetails, error) { |
Contributor
There was a problem hiding this comment.
maybe Paths should become Files ? here and in the callers
| return rs.State, nil | ||
| } | ||
|
|
||
| func getBootChain() ([]bootloader.BootFile, error) { |
Contributor
There was a problem hiding this comment.
this is the run boot chain? maybe the function name should reflect that?
Member
Author
|
I pushed something, but it is not showing up yet... |
valentindavid
force-pushed
the
valentindavid/post-install-check
branch
from
May 29, 2026 11:57
4dffada to
944d18b
Compare
github-actions
Bot
added
the
Auto rerun spread
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SNAPDENG-36452