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

…a helpers (#15051) * o/snapstate: move auxinfo functions to snapstate backend with metadata helpers Signed-off-by: Oliver Calder <[email protected]> * o/snapstate: only discard store metadata if no other instances present 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 <[email protected]> * o/snapstate: remove unnecessary check for non-empty snapID before installing 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 <[email protected]> * o/snapstate: make InstallStoreMetadata return an undo callback Signed-off-by: Oliver Calder <[email protected]> * o/snapstate/backend: disambiguate metadata management test cases Signed-off-by: Oliver Calder <[email protected]> * o/snapstate: revise comments around handling of empty snap id in install handler Signed-off-by: Oliver Calder <[email protected]> * o/snapstate: use LinkContext in InstallStoreMetadata instead of individual flags Signed-off-by: Oliver Calder <[email protected]> * o/snapstate: pass aux store info by value, not pointer Signed-off-by: Oliver Calder <[email protected]> * o/snapstate: move store metadata calls to within backend methods 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 <[email protected]> * Revert "o/snapstate: move store metadata calls to within backend methods" 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 <[email protected]> * o/snapstate: pass link context into DiscardStoreMetadata 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 <[email protected]> * o/snapstate{,/backend}: split metadata removal helper into uninstall 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 <[email protected]> * o/snapstate: remove outdated comment Signed-off-by: Oliver Calder <[email protected]> * o/snapstate/backend: DiscardStoreMetadata directly removes metadata Signed-off-by: Oliver Calder <[email protected]> --------- Signed-off-by: Oliver Calder <[email protected]>
canonical · Feb 14, 2025 · 3630eea · 3630eea
1 parent 3680d5d
commit 3630eea
Show file tree

Hide file tree

Showing 14 changed files with 342 additions and 101 deletions.
diff --git a/overlord/snapstate/aux_store_info.go → overlord/snapstate/backend/aux_store_info.go b/overlord/snapstate/aux_store_info.go → overlord/snapstate/backend/aux_store_info.go
@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2018-2022 Canonical Ltd
+ * Copyright (C) 2018-2025 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -17,7 +17,7 @@
  *
  */
 
-package snapstate
+package backend
 
 import (
 	"encoding/json"
@@ -30,27 +30,27 @@ import (
 	"github.com/snapcore/snapd/snap"
 )
 
-// auxStoreInfo is information about a snap (*not* a snap revision), not
+// AuxStoreInfo is information about a snap (*not* a snap revision), not
 // needed in the state, that may be stored to augment the information
 // returned for locally-installed snaps
-type auxStoreInfo struct {
+type AuxStoreInfo struct {
 	Media    snap.MediaInfos `json:"media,omitempty"`
 	StoreURL string          `json:"store-url,omitempty"`
 	// XXX this is now included in snap.SideInfo.EditedLinks but
 	// continue having this to support old snapd
 	Website string `json:"website,omitempty"`
 }
 
-func auxStoreInfoFilename(snapID string) string {
+func AuxStoreInfoFilename(snapID string) string {
 	return filepath.Join(dirs.SnapAuxStoreInfoDir, snapID) + ".json"
 }
 
-// retrieveAuxStoreInfo loads the stored per-snap auxiliary store info into the given *snap.Info
-func retrieveAuxStoreInfo(info *snap.Info) error {
+// RetrieveAuxStoreInfo loads the stored per-snap auxiliary store info into the given *snap.Info
+func RetrieveAuxStoreInfo(info *snap.Info) error {
 	if info.SnapID == "" {
 		return nil
 	}
-	f, err := os.Open(auxStoreInfoFilename(info.SnapID))
+	f, err := os.Open(AuxStoreInfoFilename(info.SnapID))
 	if err != nil {
 		if os.IsNotExist(err) {
 			return nil
@@ -59,7 +59,7 @@ func retrieveAuxStoreInfo(info *snap.Info) error {
 	}
 	defer f.Close()
 
-	var aux auxStoreInfo
+	var aux AuxStoreInfo
 	dec := json.NewDecoder(f)
 	if err := dec.Decode(&aux); err != nil {
 		return fmt.Errorf("cannot decode auxiliary store info for snap %q: %v", info.InstanceName(), err)
@@ -79,15 +79,15 @@ func retrieveAuxStoreInfo(info *snap.Info) error {
 }
 
 // keepAuxStoreInfo saves the given auxiliary store info to disk.
-func keepAuxStoreInfo(snapID string, aux *auxStoreInfo) error {
+func keepAuxStoreInfo(snapID string, aux AuxStoreInfo) error {
 	if snapID == "" {
 		return nil
 	}
 	if err := os.MkdirAll(dirs.SnapAuxStoreInfoDir, 0755); err != nil {
 		return fmt.Errorf("cannot create directory for auxiliary store info: %v", err)
 	}
 
-	af, err := osutil.NewAtomicFile(auxStoreInfoFilename(snapID), 0644, 0, osutil.NoChown, osutil.NoChown)
+	af, err := osutil.NewAtomicFile(AuxStoreInfoFilename(snapID), 0644, 0, osutil.NoChown, osutil.NoChown)
 	if err != nil {
 		return fmt.Errorf("cannot create file for auxiliary store info for snap %s: %v", snapID, err)
 	}
@@ -109,7 +109,7 @@ func discardAuxStoreInfo(snapID string) error {
 	if snapID == "" {
 		return nil
 	}
-	if err := os.Remove(auxStoreInfoFilename(snapID)); err != nil && !os.IsNotExist(err) {
+	if err := os.Remove(AuxStoreInfoFilename(snapID)); err != nil && !os.IsNotExist(err) {
 		return fmt.Errorf("cannot remove auxiliary store info file for snap %s: %v", snapID, err)
 	}
 	return nil

diff --git a/overlord/snapstate/aux_store_info_test.go → .../snapstate/backend/aux_store_info_test.go b/overlord/snapstate/aux_store_info_test.go → .../snapstate/backend/aux_store_info_test.go
@@ -1,7 +1,7 @@
 // -*- Mode: Go; indent-tabs-mode: t -*-
 
 /*
- * Copyright (C) 2018-2022 Canonical Ltd
+ * Copyright (C) 2018-2025 Canonical Ltd
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
@@ -17,7 +17,7 @@
  *
  */
 
-package snapstate_test
+package backend_test
 
 import (
 	"path/filepath"
@@ -26,7 +26,7 @@ import (
 
 	"github.com/snapcore/snapd/dirs"
 	"github.com/snapcore/snapd/osutil"
-	"github.com/snapcore/snapd/overlord/snapstate"
+	"github.com/snapcore/snapd/overlord/snapstate/backend"
 	"github.com/snapcore/snapd/snap"
 )
 
@@ -40,30 +40,30 @@ func (s *auxInfoSuite) SetUpTest(c *check.C) {
 
 func (s *auxInfoSuite) TestAuxStoreInfoFilename(c *check.C) {
 	// precondition check
-	filename := snapstate.AuxStoreInfoFilename("some-snap-id")
+	filename := backend.AuxStoreInfoFilename("some-snap-id")
 	c.Check(filename, check.Equals, filepath.Join(dirs.SnapAuxStoreInfoDir, "some-snap-id.json"))
 }
 
 func (s *auxInfoSuite) TestAuxStoreInfoRoundTrip(c *check.C) {
 	media := snap.MediaInfos{{Type: "1-2-3-testing"}}
 	info := &snap.Info{SuggestedName: "some-snap"}
 	info.SnapID = "some-id"
-	filename := snapstate.AuxStoreInfoFilename(info.SnapID)
+	filename := backend.AuxStoreInfoFilename(info.SnapID)
 	c.Assert(osutil.FileExists(filename), check.Equals, false)
-	c.Check(snapstate.RetrieveAuxStoreInfo(info), check.IsNil)
+	c.Check(backend.RetrieveAuxStoreInfo(info), check.IsNil)
 	c.Check(info.Media, check.HasLen, 0)
 	c.Check(info.Website(), check.Equals, "")
 	c.Check(info.StoreURL, check.Equals, "")
 
-	aux := &snapstate.AuxStoreInfo{
+	aux := backend.AuxStoreInfo{
 		Media:    media,
 		Website:  "http://example.com/some-snap",
 		StoreURL: "https://snapcraft.io/some-snap",
 	}
-	c.Assert(snapstate.KeepAuxStoreInfo(info.SnapID, aux), check.IsNil)
+	c.Assert(backend.KeepAuxStoreInfo(info.SnapID, aux), check.IsNil)
 	c.Check(osutil.FileExists(filename), check.Equals, true)
 
-	c.Assert(snapstate.RetrieveAuxStoreInfo(info), check.IsNil)
+	c.Assert(backend.RetrieveAuxStoreInfo(info), check.IsNil)
 	c.Check(info.Media, check.HasLen, 1)
 	c.Check(info.Media, check.DeepEquals, media)
 	c.Check(info.Website(), check.Equals, "http://example.com/some-snap")
@@ -75,7 +75,7 @@ func (s *auxInfoSuite) TestAuxStoreInfoRoundTrip(c *check.C) {
 	info.EditedLinks = map[string][]string{
 		"website": {"http://newer-website-com"},
 	}
-	c.Assert(snapstate.RetrieveAuxStoreInfo(info), check.IsNil)
+	c.Assert(backend.RetrieveAuxStoreInfo(info), check.IsNil)
 	c.Check(info.Media, check.HasLen, 1)
 	c.Check(info.Media, check.DeepEquals, media)
 	c.Check(info.Website(), check.Equals, "http://newer-website-com")
@@ -86,13 +86,13 @@ func (s *auxInfoSuite) TestAuxStoreInfoRoundTrip(c *check.C) {
 	info.LegacyWebsite = ""
 	info.StoreURL = ""
 
-	c.Assert(snapstate.DiscardAuxStoreInfo(info.SnapID), check.IsNil)
+	c.Assert(backend.DiscardAuxStoreInfo(info.SnapID), check.IsNil)
 	c.Assert(osutil.FileExists(filename), check.Equals, false)
 
-	c.Check(snapstate.RetrieveAuxStoreInfo(info), check.IsNil)
+	c.Check(backend.RetrieveAuxStoreInfo(info), check.IsNil)
 	c.Check(info.Media, check.HasLen, 0)
 	c.Check(info.Website(), check.Equals, "")
 	c.Check(info.StoreURL, check.Equals, "")
 
-	c.Check(snapstate.DiscardAuxStoreInfo(info.SnapID), check.IsNil)
+	c.Check(backend.DiscardAuxStoreInfo(info.SnapID), check.IsNil)
 }
diff --git a/overlord/snapstate/backend/export_test.go b/overlord/snapstate/backend/export_test.go
@@ -36,6 +36,9 @@ var (
 	RemoveIfEmpty      = removeIfEmpty
 	SnapDataDirs       = snapDataDirs
 	SnapCommonDataDirs = snapCommonDataDirs
+
+	KeepAuxStoreInfo    = keepAuxStoreInfo
+	DiscardAuxStoreInfo = discardAuxStoreInfo
 )
 
 func MockWrappersAddSnapdSnapServices(f func(s *snap.Info, opts *wrappers.AddSnapdSnapServicesOptions, inter wrappers.Interacter) (wrappers.SnapdRestart, error)) (restore func()) {

diff --git a/overlord/snapstate/backend/metadata.go b/overlord/snapstate/backend/metadata.go
@@ -0,0 +1,76 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2025 Canonical Ltd
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 3 as
+ * published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ *
+ */
+
+package backend
+
+// InstallStoreMetadata saves revision-agnostic metadata to disk for the snap
+// with the given snap ID. At the moment, this metadata includes auxiliary
+// store information. Returns a closure to undo the function's actions,
+// depending on whether it's a first install or if there are other instances.
+func InstallStoreMetadata(snapID string, aux AuxStoreInfo, linkCtx LinkContext) (undo func(), err error) {
+	if snapID == "" {
+		return func() {}, nil
+	}
+	if err := keepAuxStoreInfo(snapID, aux); err != nil {
+		return nil, err
+	}
+	// TODO: install other types of revision-agnostic metadata
+	return func() {
+		UninstallStoreMetadata(snapID, linkCtx)
+	}, nil
+}
+
+// UninstallStoreMetadata removes revision-agnostic metadata from disk for the
+// snap with the given snap ID. At the moment, this metadata includes auxiliary
+// store information. The given linkCtx governs what metadata is removed and
+// what is preserved.
+func UninstallStoreMetadata(snapID string, linkCtx LinkContext) error {
+	if linkCtx.HasOtherInstances || snapID == "" {
+		return nil
+	}
+	if linkCtx.FirstInstall {
+		// only discard aux store info if there are no other revision of the
+		// snap present, in case we want to roll-back the discard, and need the
+		// auxinfo on disk to re-populate an old snap.Info. This might occur if
+		// e.g. we unlinked the snap and now need to undoUnlinkSnap.
+		if err := discardAuxStoreInfo(snapID); err != nil {
+			return err
+		}
+	}
+	// TODO: discard other types of revision-agnostic metadata
+	return nil
+}
+
+// DiscardStoreMetadata removes revision-agnostic metadata from disk for the
+// snap with the given snap ID, and is intended to be called when the final
+// revision of that snap is being discarded. At the moment, this metadata
+// includes auxiliary store information. If hasOtherInstances is false, this
+// function does nothing, as another instance of the same snap may still
+// require this metadata.
+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 which should be
+	// removed when the final revision of the snap is discarded
+	return nil
+}