o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051

olivercalder · 2025-02-08T06:51:20Z

Move actions which are not revision- or instance-specific into dedicated functions within the snapstate backend.

In preparation for the management of snap icons from the store within snapstate handler functions (coming in ~~#15003~~ #15070), move the existing functions related to auxStoreInfo into the backend package, and wrap them in more generic {Install,Discard}StoreMetadata backend helper functions. In the future, functions to install/discard snap icons will be called from within these helpers as well. Thus, the snapstate handlers will only have to worry about calling one function to setup and one function to teardown all snap metadata which is not tied to a specific revision or instance of the snap.

Additionally, make some other drive-by improvements, such as not discarding auxinfo if there are other instances of the snap present.

This work is tracked internally by https://warthogs.atlassian.net/browse/SNAPDENG-34436

…a helpers Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Since store metadata is not specific to any particular revision or instance of a snap, it should not be removed if there are other instances of the snap still present. By making `hasOtherInstances` a parameter to `DiscardStoreMetadata`, callers are encouraged to compute and check `hasOtherInstances` beforehand, thus avoiding accidentally removing metadata which is expected for another instance of the same snap. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

…talling metadata The `keepAuxStoreInfo` helper was already a no-op if `snapID == ""`, so remove the additional check in the calling handler. Additionally, make the backend store metadata helpers explicitly return `nil` early if `snapID` is `""`, rather than relying on all functions called within to guarantee this. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

github-actions · 2025-02-08T06:53:16Z

Thu Feb 13 20:34:34 UTC 2025
The following results are from: https://github.com/canonical/snapd/actions/runs/13314007639

Failures:

Executing:

openstack:fedora-41-64:tests/main/refresh-all
google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-pair-single-reboot-failover:gadget_base
google:ubuntu-20.04-64:tests/unit/go:clang

Restoring:

openstack:fedora-41-64:tests/main/snapctl-from-snap:nobase
openstack:fedora-41-64:tests/main/refresh-all
openstack:fedora-41-64:tests/main/
openstack:fedora-41-64
openstack:fedora-41-64:tests/main/
openstack:fedora-41-64
openstack:fedora-41-64:tests/main/xdg-open-portal
openstack:fedora-41-64:tests/main/
openstack:fedora-41-64
google-core:ubuntu-core-18-64:tests/core/kernel-base-gadget-pair-single-reboot-failover:gadget_base
google-core:ubuntu-core-18-64:tests/core/
google-core:ubuntu-core-18-64

olivercalder · 2025-02-08T07:03:24Z

overlord/snapstate/handlers.go

+	// Write the revision-agnostic store metadata for this snap. If snap ID is
+	// empty, InstallStoreMetadata is a no-op, so no need to check beforehand.
+	// XXX: Previously, err := ... was used and didn't error, indicating that
+	// the err checked in IsErrAndNotWait was the local err (which we know to
+	// be non-nil at time of the check), not the err returned from the
+	// function. Is this analysis correct, and was this intentional?
+	// XXX: Also, why is cand.Snap.SnapID used instead of snapsup.SnapID?
+	// Previously, all of this was inside `if cand.Snap.SnapID != "" {`, and
+	// AFAICT, snapsup.SnapID should never be "".
+	err = backend.InstallStoreMetadata(cand.Snap.SnapID, aux)
+	if err != nil {
+		return err
+	}
+	if len(snapst.Sequence.Revisions) == 1 {
+		defer func() {
+			if IsErrAndNotWait(err) {
+				// the install is getting undone, and there are no more of this snap
+				// try to remove the metadata we just installed
+				backend.DiscardStoreMetadata(cand.Snap.SnapID, otherInstances)
+			}
+		}()


@bboozzoo please advise on this, I'm not sure if there's something special about cand.Snap.SnapID vs simply using snapsup.SnapID, and if/when one would expect either/both to be "".

olivercalder · 2025-02-08T07:11:58Z

overlord/snapstate/backend/metadata.go

+// InstallStoreMetadata saves revision-agnostic metadata to disk for the snap
+// with the given snap ID. At the moment, this metadata includes auxiliary
+// store information.
+func InstallStoreMetadata(snapID string, aux *AuxStoreInfo) error {
+	if snapID == "" {
+		return nil
+	}
+	if err := keepAuxStoreInfo(snapID, aux); err != nil {
+		return err
+	}
+	// TODO: install other types of revision-agnostic metadata
+	return nil
+}
+
+// DiscardStoreMetadata removes revision-agnostic metadata to disk for the snap
+// with the given snap ID. At the moment, this metadata includes auxiliary
+// store information. If hasOtherInstances is true, does nothing.
+func DiscardStoreMetadata(snapID string, hasOtherInstances bool) error {
+	if hasOtherInstances || snapID == "" {
+		return nil
+	}
+	if err := discardAuxStoreInfo(snapID); err != nil {
+		return err
+	}
+	// TODO: discard other types of revision-agnostic metadata
+	return nil
+}


It would be nice if these could be called directly from within other backend methods (e.g. backend.LinkSnap and backend.UnlinkSnap), as that would mean nearly/all of the complexity around saving and reverting auxinfo could be avoided. However, that would require those methods to be able to compute hasOtherInstances, which is not possible at the moment, unless we want to start passing snapstate into the backend.

Notably, the error handling for doLinkSnap after backend.LinkSnap(newInfo, ...) has succeeded already does exactly what we want: if there's another revision, simply call backend.LinkSnap(oldInfo, ...), and if there's no other revision, call backend.UnlinkSnap. If InstallStoreMetadata and DiscardStoreMetadata were called from within these, respectively, the result would be as desired.

There are lots of other backend functions which are called from the handlers though, so I'm not sure if it would be consistent to place calls to these helpers within backend.LinkSnap and backend.UnlinkSnap, for example, else why are so many backend helpers called from within

I'd love to hear your thoughts on this next week @bboozzoo and @pedronis

pedronis · 2025-02-10T10:29:34Z

overlord/snapstate/handlers.go

+	err = backend.InstallStoreMetadata(cand.Snap.SnapID, aux)
+	if err != nil {
+		return err
+	}
+	if len(snapst.Sequence.Revisions) == 1 {
+		defer func() {
+			if IsErrAndNotWait(err) {
+				// the install is getting undone, and there are no more of this snap
+				// try to remove the metadata we just installed
+				backend.DiscardStoreMetadata(cand.Snap.SnapID, otherInstances)
+			}


this still feels a bit messy, it feels like maybe we should pass cand and SnapSetup and an isInstall flag to the backend and get back a something to do inside the if of the defer in any case

We could pass cand in, but I don't think we can pass SnapSetup in, since it's part of the snapstate package and that would cause a circular dependency. Also, if we were to pass SnapSetup in, we'd have all we need from cand, since all we need is SnapID, and SnapSetup has that.

But even if we passed in SnapSetup, we'd also need to pass in State so that we can compute otherInstances. Would we want to do that?

For now, I've added hasOtherInstances and isInstall parameters to InstallStoreMetadata, and made it return an undo callback which takes into account those parameters. Please let me know if this is what you had in mind, or if you'd prefer something else.

Thanks!

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

andrewphelpsj

Thanks, just some small things.

overlord/snapstate/backend/metadata.go

overlord/snapstate/backend/metadata_test.go

overlord/snapstate/handlers.go

andrewphelpsj · 2025-02-10T21:32:36Z

overlord/snapstate/handlers.go

+	// Previously, all of this was inside `if cand.Snap.SnapID != "" {`, and
+	// AFAICT, snapsup.SnapID should never be "".
+	isInstall := len(snapst.Sequence.Revisions) == 1
+	restore, err := backend.InstallStoreMetadata(cand.Snap.SnapID, aux, otherInstances, isInstall)


Is it worth accepting isInstall as a param here, only to use it to change how restore works?

I wonder if this closure is really needed, the indirection is maybe harder to follow than doing things on this side here. I understand for testability, but the task itself probably needs to test those cases anyways.

Eh yeah I'm not sure either way, I understand wanting to have the undo logic right next to where we have the do side of things.

Note that we wouldn't need otherInstances either if we didn't use the closure style undo.

I agree, and I'm also torn. Prior to the previous commit (f7fefa1) the caller was expected to know how to undo things properly, but that undo handling can be rather messy, and Samuele suggested here to just pass in isInstall as a parameter and have the returned undo closure handle the complexity. I think it's nice to have a simple undo closure, but it does push a bit more complexity to the call site of InstallStoreMetadata.

Samuele actually suggested passing something more generic (cand and snapsup) into InstallStoreMetadata, which would ease some of the complexity burden in the handler at the cost of more complexity in the metadata function, but I'm worried about circular dependencies, and I think we'd need to pass all of state in in order to compute otherInstances.

this is not the only backend function that is provided such information though, both sides don't seem overly complicated to me, also in the next step the backend side will take care of icons so the trade off should be a bit more valuable

codecov · 2025-02-10T22:26:47Z

Codecov Report

Attention: Patch coverage is 69.86301% with 22 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@ba9ef8e). Learn more about missing BASE report.
Report is 213 commits behind head on master.

Files with missing lines	Patch %	Lines
overlord/snapstate/backend/metadata.go	66.66%	6 Missing and 4 partials ⚠️
overlord/snapstate/handlers.go	70.58%	6 Missing and 4 partials ⚠️
overlord/snapstate/backend/aux_store_info.go	85.71%	0 Missing and 1 partial ⚠️
overlord/snapstate/snapmgr.go	0.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #15051   +/-   ##
=========================================
  Coverage          ?   78.03%           
=========================================
  Files             ?     1162           
  Lines             ?   156918           
  Branches          ?        0           
=========================================
  Hits              ?   122450           
  Misses            ?    26856           
  Partials          ?     7612

Flag	Coverage Δ
unittests	`78.03% <69.86%> (?)`

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.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

…all handler Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

pedronis

thanks for the changes, did another pass

overlord/snapstate/handlers.go

overlord/snapstate/backend/metadata.go

pedronis · 2025-02-11T17:15:42Z

overlord/snapstate/handlers.go

+	// Previously, all of this was inside `if cand.Snap.SnapID != "" {`, and
+	// AFAICT, snapsup.SnapID should never be "".
+	isInstall := len(snapst.Sequence.Revisions) == 1
+	restore, err := backend.InstallStoreMetadata(cand.Snap.SnapID, aux, otherInstances, isInstall)


this is not the only backend function that is provided such information though, both sides don't seem overly complicated to me, also in the next step the backend side will take care of icons so the trade off should be a bit more valuable

…idual flags Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Move `installStoreMetadata` call from the snapstate handler into the backend `LinkSnap` method call, and move `uninstallStoreMetadata` into `UnlinkSnap`. This reduces the complexity of the snapstate handlers. Since `doDiscardSnap` still needs to discard store metadata if there are no other revisions/instances of the snap present, and it is not inherently associated with a particular install/uninstall operation, there is an additional new metadata helper which, instead of taking a `LinkContext`, simply takes a `hasOtherInstances` bool, and uses it to construct a `LinkContext` and then call `uninstallStoreMetadata`. Auxiliary store info is removed regardless of whether `UnlinkSnap` or `doDiscardSnap` are the callers, but other store metadata (such as cached snap icons) may behave differently, so this wrapper will enable this distinction in the future. One important note: auxinfo for a snap should not be removed unless there are no other revisions of the snap present, because in case the unlink snap is reverted, `undoUnlinkSnap` needs to read the auxinfo from disk in order to populate `oldInfo`, so it can be re-linked. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder · 2025-02-12T00:43:31Z

I've just pushed a commit which moves the calls to {Install,Discard}StoreMetadata into backend.{Link,Unlink}Snap, as @bboozzoo suggested here: #15003 (comment)

This simplifies the snapstate handler code but does require widespread changes to testing, and doDiscardSnap does still need to call a metadata helper directly. What do you think of this change @bboozzoo and @pedronis ? I'm happy to revert it and stick with the handlers calling all the metadata helpers directly, or keep the new approach.

pedronis

@olivercalder given that LinkSnap is about a revision but the metadata is across revisions and as you said you still need to keep things as they are in doDiscardSnap, I think is better to revert the last change

…ods" This reverts commit e590adc. Since `backend.LinkSnap` is about a particular revision but the metadata is revision-agnostic, and the requirement of discarding metadata manually in `doDiscardSnap` means it's probably clearer to leave the metadata calls all within the snapstate handlers, not backend methods. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Some store metadata should only be removed if there are no other revisions of the snap present, but others may need to be removed regardless. By passing a `LinkContext` including both `FirstInstall` and `HasOtherInstances`, the function can decide for itself what metadata should be discarded, and what should be preserved, rather than the caller being responsible for this. As such, moved calls to `DiscardStoreMetadata` out of some checks for the presence of other revisions. This also means that the undo closure returned by `InstallStoreMetadata` can simply call `DiscardStoreMetadata`, without any additional checks. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

pedronis

small new remark

pedronis · 2025-02-13T09:52:06Z

overlord/snapstate/handlers.go

+		// link or unlink, so we can discard store metadata appropriately.
+		linkCtx := backend.LinkContext{
+			// there are no revisions of the snap present, so we're discarding the "first"
+			FirstInstall:      true,


this looks definitely a bit confusing, maybe we should introduce a (Last)Discard flag in LinkContext as well? and DiscardStoreMetadata should treat it accordingly?

or DiscardMetadataContext structure?

doubtful that the clarity win of adding a new structure is enough vs not being able to just pass LinkContext where it makes sense

Aha, this is actually addressed in the follow-up PR I opened yesterday, which adds the icon calls to the metadata helpers: 7643baf#diff-00d6af4a298e5274422799eac8a04e66a2087322bcb3515a1814623ecafe3ab3L3814-R3815

Icons need to be unlinked in undoLink/doLink, but they additionally need to be discarded from the snap icons pool in this place, so we need to split the DiscardStoreMetadata helper into two, with the one here only requiring hasOtherInstances, so we don't need this extra LinkContext here (though one is still used in the metadata helper itself, but I think it's better contained there).

So I can pull in a similar change to this PR now.

my remark was solved

bboozzoo · 2025-02-13T10:34:23Z

overlord/snapstate/handlers.go

+	// XXX: Previously, err := ... was used and didn't error, indicating that
+	// the err checked in IsErrAndNotWait was the local err (which we know to
+	// be non-nil at time of the check), not the err returned from the
+	// function. Is this analysis correct, and was this intentional?


In the previous code err in the scope of the deferred function refers to the named return result from the enclosing function. Even if you shadow the variable names, the values are assigned when returning from the function. See https://go.dev/ref/spec#Defer_statements and https://go.dev/play/p/Zs2RTwh2Yg7

Thanks, that makes sense. I think I ended up in some intermediate situation where the defer call was within the else of that error handling case --- I think what it was was that I'd added something like:

if otherInstances, err := hasOtherInstances(); err == nil && len(snapst.Sequence.Revisions) == 1 { defer func() { if IsErrAndNotWait(err) { // the install is getting undone, and there are no more of this snap // try to remove the aux info we just created discardAuxStoreInfo(cand.Snap.SnapID) } }()

and that err was actually local. This was before moving things into the metadata helpers in the backend, and I was trying to debug why tests weren't covering the discardAuxStoreInfo call within.

Anyway, I don't see how the original code would have resulted in this problem, so I've removed the comment.

Thanks!

bboozzoo · 2025-02-13T10:37:30Z

overlord/snapstate/handlers.go

+		// link or unlink, so we can discard store metadata appropriately.
+		linkCtx := backend.LinkContext{
+			// there are no revisions of the snap present, so we're discarding the "first"
+			FirstInstall:      true,


or DiscardMetadataContext structure?

…and discard When a snap is being unlinked or a link is being undone, the snapstate handler should call `UnlinkStoreMetadata`, which takes a `LinkContext` and checks whether there are other revisions of the snap present. When the final revision of a snap is being discarded, the snapstate handler should call `DiscardStoreMetadata`, which takes a `hasOtherInstances bool` and assumes that there are no other revisions of that instance of the snap present. At the moment, the latter simply calls the former, but in the future, other store metadata which should be handled differently during unlink and discard can be handled appropriately. Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

pedronis

remark

pedronis · 2025-02-13T17:40:48Z

overlord/snapstate/backend/metadata.go

+	linkCtx := LinkContext{
+		// since the final revision is being discarded, we're effectively
+		// removing the "first" install of the snap
+		FirstInstall:      true,
+		HasOtherInstances: hasOtherInstances,
+	}
+	if err := UninstallStoreMetadata(snapID, linkCtx); err != nil {


the reuse here doesn't seem very valuable to me, better to just call discardAuxStoreInfo(snapID)

Sure, good point, probably easier to unconditionally remove auxinfo (and icon in the future) than construct a special LinkContext to get UninstallStoreMetadata to do what we want.

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder requested review from bboozzoo, pedronis and andrewphelpsj February 8, 2025 06:51

olivercalder added 3 commits February 8, 2025 00:52

o/snapstate: move auxinfo functions to snapstate backend with metadat…

2aaf366

…a helpers Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder force-pushed the backend-metadata-management branch from c5b7b15 to c21b939 Compare February 8, 2025 06:52

olivercalder mentioned this pull request Feb 8, 2025

o/snapstate: download/link/unlink/discard snap icons in snapstate handlers #15003

Closed

olivercalder commented Feb 8, 2025

View reviewed changes

pedronis reviewed Feb 10, 2025

View reviewed changes

o/snapstate: make InstallStoreMetadata return an undo callback

f7fefa1

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder requested a review from pedronis February 10, 2025 16:11

andrewphelpsj reviewed Feb 10, 2025

View reviewed changes

olivercalder added 2 commits February 10, 2025 18:19

o/snapstate/backend: disambiguate metadata management test cases

06c89d8

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

o/snapstate: revise comments around handling of empty snap id in inst…

025b681

…all handler Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

pedronis reviewed Feb 11, 2025

View reviewed changes

olivercalder added 3 commits February 11, 2025 13:12

o/snapstate: use LinkContext in InstallStoreMetadata instead of indiv…

2e56a77

…idual flags Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

o/snapstate: pass aux store info by value, not pointer

92b4513

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder requested review from pedronis and andrewphelpsj February 12, 2025 00:49

pedronis previously requested changes Feb 12, 2025

View reviewed changes

olivercalder added 2 commits February 12, 2025 08:27

olivercalder requested a review from pedronis February 12, 2025 15:17

olivercalder mentioned this pull request Feb 13, 2025

o/snapstate: download, link, unlink, and discard snap icon in snapstate handlers #15070

Open

pedronis reviewed Feb 13, 2025

View reviewed changes

bboozzoo reviewed Feb 13, 2025

View reviewed changes

olivercalder added 2 commits February 13, 2025 09:23

o/snapstate: remove outdated comment

bc79190

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder requested review from bboozzoo and pedronis February 13, 2025 15:44

pedronis reviewed Feb 13, 2025

View reviewed changes

o/snapstate/backend: DiscardStoreMetadata directly removes metadata

b16c644

Signed-off-by: Oliver Calder <oliver.calder@canonical.com>

olivercalder requested a review from pedronis February 13, 2025 17:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051

o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051

olivercalder commented Feb 8, 2025 •

edited

Loading

github-actions bot commented Feb 8, 2025 •

edited

Loading

olivercalder Feb 8, 2025

olivercalder Feb 8, 2025

olivercalder Feb 8, 2025

pedronis Feb 10, 2025

olivercalder Feb 10, 2025

andrewphelpsj left a comment

andrewphelpsj Feb 10, 2025

andrewphelpsj Feb 10, 2025

olivercalder Feb 11, 2025

pedronis Feb 11, 2025

codecov bot commented Feb 10, 2025 •

edited

Loading

pedronis left a comment

pedronis Feb 11, 2025

olivercalder commented Feb 12, 2025

pedronis left a comment

pedronis left a comment •

edited

Loading

pedronis Feb 13, 2025

bboozzoo Feb 13, 2025

pedronis Feb 13, 2025 •

edited

Loading

olivercalder Feb 13, 2025

olivercalder Feb 13, 2025

bboozzoo Feb 13, 2025

olivercalder Feb 13, 2025

bboozzoo Feb 13, 2025

pedronis left a comment

pedronis Feb 13, 2025

olivercalder Feb 13, 2025

o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051

Are you sure you want to change the base?

o/snapstate: move auxinfo functions to snapstate backend with metadata helpers #15051

Conversation

olivercalder commented Feb 8, 2025 • edited Loading

github-actions bot commented Feb 8, 2025 • edited Loading

Failures:

Executing:

Restoring:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewphelpsj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 10, 2025 • edited Loading

Codecov Report

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder commented Feb 12, 2025

pedronis left a comment

Choose a reason for hiding this comment

pedronis left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pedronis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

olivercalder commented Feb 8, 2025 •

edited

Loading

github-actions bot commented Feb 8, 2025 •

edited

Loading

codecov bot commented Feb 10, 2025 •

edited

Loading

pedronis left a comment •

edited

Loading

pedronis Feb 13, 2025 •

edited

Loading