Conversation
…version Introduce versioning of the organization of the kernel modules & firmware trees. The patch adds a helper to record the version of the layout and another helper to check and compare indicating whether the tree was set up by older version of the code. Both are essential for being able to determine if the tree needs to be rebuilt/updated when updating snapd. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
The KernelInstall idempotency guard only checked directory existence, not completion. Since this path builds directly into destDir (no scratch dir), an interrupted build or a marker-less tree from an older snapd would be wrongly treated as already done. Require DriversTreeNeedsCheck to confirm a current marker before skipping the rebuild, and stop discarding DirExists/DriversTreeNeedsCheck errors. Rebuilding in place on a stale/missing marker is safe: destDir is never live-mounted at either KernelInstall call site. Signed-off-by: Maciej Borzecki <maciej.borzecki@canonical.com>
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
A failed cleanup can leave stale tree content that is subsequently marked current.
Get a fresh assessment by requesting another Copilot review.
Review effort: Balanced
Findings: 1
Open (2)
What changed in this PR
Adds completion/version markers to kernel driver trees so interrupted or legacy builds are rebuilt safely.
Changes:
- Records and validates kernel tree generator metadata.
- Rebuilds stale or markerless installation trees.
- Adds unit and spread coverage.
| File | Description |
|---|---|
kernel/kernel_drivers.go |
Implements metadata and guarded rebuilding. |
kernel/kernel_drivers_test.go |
Tests marker and rebuild behavior. |
kernel/export_test.go |
Exposes test helpers and mocks. |
tests/core/basic20plus/task.yaml |
Verifies metadata on Ubuntu Core. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
455
to
458
| if rmErr := RemoveKernelDriversTree(targetDir); rmErr != nil && | ||
| !errors.Is(err, fs.ErrNotExist) { | ||
| !errors.Is(rmErr, fs.ErrNotExist) { | ||
| logger.Noticef("while removing old kernel tree: %v", rmErr) | ||
| } |
Comment on lines
+1055
to
+1057
| restore := kernel.MockKernelDriversTreeGeneratorVersion(100) | ||
| c.Assert(kernel.WriteDriversTreeMeta(destDir), IsNil) | ||
| restore() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #17701 +/- ##
==========================================
+ Coverage 78.35% 78.39% +0.04%
==========================================
Files 1416 1414 -2
Lines 200464 200666 +202
Branches 2502 2503 +1
==========================================
+ Hits 157070 157320 +250
+ Misses 33972 33911 -61
- Partials 9422 9435 +13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Wed Sep 23 15:34:44 UTC 2026 Test Predictor AnalysisPreparing
Executing
Restoring
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 of the below tests you wish to run (unskip plus test list must be valid yaml)
|
This branch has not been deployed
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
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.
The KernelInstall idempotency guard only checked directory existence,
not completion. Since this path builds directly into destDir (no
scratch dir), an interrupted build or a marker-less tree from an older
snapd would be wrongly treated as already done. Require
DriversTreeNeedsCheck to confirm a current marker before skipping the
rebuild, and stop discarding DirExists/DriversTreeNeedsCheck errors.
Rebuilding in place on a stale/missing marker is safe: destDir is never
live-mounted at either KernelInstall call site.
Cherry picked form #17680
Related: SNAPDENG-37501