o/devicestate: allow remodeling back to an old snap revision that includes components #15060
Conversation
andrewphelpsj
added
the
Run nested
|
Thu Mar 13 19:17:47 UTC 2025 Failures:Preparing:
Executing:
Restoring:
|
|
As discussed with @andrewphelpsj, it's unlikely this content will make 2.68.2. Keeping tag for the moment, will reevaluate when everything else is ready. |
andrewphelpsj
force-pushed
the
remodel-fix-back-to-old-revision
branch
from
2d8e498 to
a134697
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #15060 +/- ##
==========================================
+ Coverage 78.06% 78.13% +0.06%
==========================================
Files 1183 1183
Lines 158250 158660 +410
==========================================
+ Hits 123541 123963 +422
+ Misses 27029 26990 -39
- Partials 7680 7707 +27
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:
|
|
This PR is not going to make 2.68.2, moving to 2.69. @andrewphelpsj @pedronis |
andrewphelpsj
force-pushed
the
remodel-fix-back-to-old-revision
branch
from
a134697 to
c58aa33
Compare
…ith components involved This change also makes it so we no longer depend on snapstate.Update not reaching out to the store if the snap revision is already installed.
andrewphelpsj
force-pushed
the
remodel-fix-back-to-old-revision
branch
from
c58aa33 to
24ca0f1
Compare
| if wrongRevisionInstalled { | ||
| c.Assert(err, ErrorMatches, `cannot fall back to component "pc-kernel\+kmod" with revision 11, required revision is 12`) | ||
| } else { | ||
| c.Assert(err, ErrorMatches, `cannot find required component in set of already installed components: pc-kernel\+kmod`) | ||
| } | ||
| } |
There was a problem hiding this comment.
the helper could actually return the error and the Assert moved into the tests, no need for the boolean flag
There was a problem hiding this comment.
wrongRevisionInstalled is used earlier in the function as part of the setup as well. I made an attempt at moving the setup into the calling functions, as well as returing the error. My attempt didn't look that great. But I'm happy to try again if you think it'd be better that way.
| } | ||
|
|
||
| return snapstateStoreUpdateGoal(snapstate.StoreUpdate{ | ||
| return snapstatePathUpdateGoal(snapstate.PathSnap{ |
There was a problem hiding this comment.
will this actually move the old sequence point in front or will there be two sequence points?
There was a problem hiding this comment.
This will move the sequence point to the end of the sequence. You can see that in TestUpdateWithComponentsBackToPrevRevision: https://github.com/canonical/snapd/blob/master/overlord/snapstate/snapstate_update_test.go?plain=1#L15370-L15374.
There was a problem hiding this comment.
where do I see that that test is using a PathUpdateGoal?
| updated["pc-kernel"], | ||
| }, []interface{}{ | ||
| updated["pc-kernel+kmod"], | ||
| }) |
There was a problem hiding this comment.
should the test also look at the state of the corresponding SnapState.Sequence after the remodel?
There was a problem hiding this comment.
That is a good idea, and will clear up your other question. These specific tests aren't set up to actually drive the change to completion right now; I'll work on that.
There was a problem hiding this comment.
It looks like that style of test would fit better in managers_test.go, since this change goes across many managers.
… already installed snaps moves around sequence points properly
| c.Assert(snapst.Sequence.Revisions[0], DeepEquals, originalSideState) | ||
| c.Assert(snapst.Sequence.Revisions[1], DeepEquals, expectedSideState) |
There was a problem hiding this comment.
so these are swapped now vs when they were read out in the fixture setup?
There was a problem hiding this comment.
Yes, that is correct.
| // err = os.MkdirAll(filepath.Dir(cpi.MountFile()), 0755) | ||
| // c.Assert(err, check.IsNil) | ||
| // | ||
| // contents := fmt.Sprintf("%s-%s", csi.Component.String(), csi.Revision) | ||
| // err = os.WriteFile(cpi.MountFile(), []byte(contents), 0644) | ||
| // c.Assert(err, check.IsNil) |
There was a problem hiding this comment.
You probably forgot to remove this
This will enable us to remodel back to a snap revision that was already installed in the past (somewhere in the sequence), assuming that the snap has all of the components that are needed already installed as well.