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

many: fix swapping back and forth between kernels with components during remodeling #15046

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

andrewphelpsj
Copy link
Member

@andrewphelpsj andrewphelpsj commented Feb 7, 2025

This fixes and tests the case where we remodel to a kernel snap that is already installed on the system, but is not the current kernel due to a previous remodel.

Note that this PR removes the existing "prepare-kernel-snap" task from the created tasks here, because the task is superfluous in this case. The kernel's drivers tree is already generated from the original installation of the kernel.

@github-actions github-actions bot added the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Feb 7, 2025
@ndyer ndyer added this to the 2.68.1 milestone Feb 7, 2025
Copy link

github-actions bot commented Feb 7, 2025

Tue Feb 18 14:25:45 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13392608203

No spread failures reported

@andrewphelpsj andrewphelpsj force-pushed the remodel-fix-kernel-swapping branch from 88ba090 to f9f7070 Compare February 10, 2025 20:37
@github-actions github-actions bot removed the Needs Documentation -auto- Label automatically added which indicates the change needs documentation label Feb 10, 2025
@andrewphelpsj andrewphelpsj marked this pull request as ready for review February 10, 2025 20:38
@andrewphelpsj andrewphelpsj added the Run nested The PR also runs tests inluded in nested suite label Feb 10, 2025
Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

did a pass

overlord/devicestate/devicestate_remodel_test.go Outdated Show resolved Hide resolved
Comment on lines 3444 to 3448
// this previously created a prepare-kernel-snap task, but this was not
// needed since this function is only used to swap to kernel snaps that
// are still installed on the system (but not in use. this can happen
// because remodeling leaves around old snaps). if the snap is
// installed, then it is expected that the drivers tree is present.
Copy link
Collaborator

Choose a reason for hiding this comment

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

it feels some of this needs to go into the doc comment for (Add)LinkNewBaseOrKernel as it's actually not super clear what their exact usage conditions are

@@ -3492,8 +3503,24 @@ func findSnapSetupTask(tasks []*state.Task) (*state.Task, *SnapSetup, error) {
return nil, nil, nil
}

// AddLinkNewBaseOrKernel creates the same tasks as LinkNewBaseOrKernel but adds
// them to the provided task set.
// LinkNewBaseOrKernel appends tasks to a given task set. This enables swapping
Copy link
Collaborator

Choose a reason for hiding this comment

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

the name is wrong now

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, thanks.

Copy link
Collaborator

@pedronis pedronis left a comment

Choose a reason for hiding this comment

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

thank you

@andrewphelpsj andrewphelpsj force-pushed the remodel-fix-kernel-swapping branch from cf19dea to 0e52fb7 Compare February 18, 2025 14:14
@andrewphelpsj andrewphelpsj force-pushed the remodel-fix-kernel-swapping branch from 0e52fb7 to 82bdecb Compare February 19, 2025 16:00
Copy link

codecov bot commented Feb 19, 2025

Codecov Report

Attention: Patch coverage is 71.01449% with 20 lines in your changes missing coverage. Please review.

Project coverage is 78.12%. Comparing base (a272aac) to head (82bdecb).
Report is 23 commits behind head on master.

Files with missing lines Patch % Lines
overlord/snapstate/snapstate.go 71.01% 13 Missing and 7 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master   #15046      +/-   ##
==========================================
+ Coverage   78.07%   78.12%   +0.05%     
==========================================
  Files        1182     1174       -8     
  Lines      157743   157753      +10     
==========================================
+ Hits       123154   123248      +94     
+ Misses      26943    26857      -86     
- Partials     7646     7648       +2     
Flag Coverage Δ
unittests 78.12% <71.01%> (+0.05%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Run nested The PR also runs tests inluded in nested suite
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants