Merge pull request #5758 from bboozzoo/bboozzoo/parallel-install-hand…

…le-common-snap-dirs

overlord/snapstate, snap: handle shared snap directories when installing/remove snaps with instance key

overlord/snapstate/backend.go

@@ -69,8 +69,10 @@ type managerBackend interface {
 	// remove related
 	UnlinkSnap(info *snap.Info, meter progress.Meter) error
 	RemoveSnapFiles(s snap.PlaceInfo, typ snap.Type, meter progress.Meter) error
+	RemoveSnapDir(s snap.PlaceInfo, hasOtherInstances bool) error
 	RemoveSnapData(info *snap.Info) error
 	RemoveSnapCommonData(info *snap.Info) error
+	RemoveSnapDataDir(info *snap.Info, hasOtherInstances bool) error
 	DiscardSnapNamespace(snapName string) error
 
 	// alias related

overlord/snapstate/backend/copydata.go

@@ -32,6 +32,14 @@ func (b Backend) CopySnapData(newSnap, oldSnap *snap.Info, meter progress.Meter)
 	// deal with the old data or
 	// otherwise just create a empty data dir
 
+	// Make sure the base data directory exists for instance snaps
+	if newSnap.InstanceKey != "" {
+		err := os.MkdirAll(snap.BaseDataDir(newSnap.SnapName()), 0755)
+		if err != nil && !os.IsExist(err) {
+			return err
+		}
+	}
+
 	// Make sure the common data directory exists, even if this isn't a new
 	// install.
 	if err := os.MkdirAll(newSnap.CommonDataDir(), 0755); err != nil {

overlord/snapstate/backend/setup.go

@@ -57,6 +57,13 @@ func (b Backend) SetupSnap(snapFilePath, instanceName string, sideInfo *snap.Sid
 		return snapType, err
 	}
 
+	if s.InstanceKey != "" {
+		err := os.MkdirAll(snap.BaseDir(s.SnapName()), 0755)
+		if err != nil && !os.IsExist(err) {
+			return snapType, err
+		}
+	}
+
 	if err := snapf.Install(s.MountFile(), instdir); err != nil {
 		return snapType, err
 	}
@@ -88,10 +95,6 @@ func (b Backend) RemoveSnapFiles(s snap.PlaceInfo, typ snap.Type, meter progress
 		return err
 	}
 
-	// try to remove parent dir, failure is ok, means some other
-	// revisions are still in there
-	os.Remove(filepath.Dir(mountDir))
-
 	// snapPath may either be a file or a (broken) symlink to a dir
 	snapPath := s.MountFile()
 	if _, err := os.Lstat(snapPath); err == nil {
@@ -111,6 +114,23 @@ func (b Backend) RemoveSnapFiles(s snap.PlaceInfo, typ snap.Type, meter progress
 	return nil
 }
 
+func (b Backend) RemoveSnapDir(s snap.PlaceInfo, hasOtherInstances bool) error {
+	mountDir := s.MountDir()
+
+	snapName, instanceKey := snap.SplitInstanceName(s.InstanceName())
+	if instanceKey != "" {
+		// always ok to remove instance specific one, failure to remove
+		// is ok, there may be other revisions
+		os.Remove(filepath.Dir(mountDir))
+	}
+	if !hasOtherInstances {
+		// remove only if not used by other instances of the same snap,
+		// failure to remove is ok, there may be other revisions
+		os.Remove(snap.BaseDir(snapName))
+	}
+	return nil
+}
+
 // UndoSetupSnap undoes the work of SetupSnap using RemoveSnapFiles.
 func (b Backend) UndoSetupSnap(s snap.PlaceInfo, typ snap.Type, meter progress.Meter) error {
 	return b.RemoveSnapFiles(s, typ, meter)

overlord/snapstate/backend/setup_test.go

@@ -313,3 +313,41 @@ func (s *setupSuite) TestSetupCleanupAfterFail(c *C) {
 	c.Check(osutil.FileExists(minInfo.MountFile()), Equals, false)
 	c.Check(osutil.FileExists(filepath.Join(dirs.SnapBlobDir, "hello_14.snap")), Equals, false)
 }
+
+func (s *setupSuite) TestRemoveSnapFilesDir(c *C) {
+	snapPath := makeTestSnap(c, helloYaml1)
+
+	si := snap.SideInfo{
+		RealName: "hello",
+		Revision: snap.R(14),
+	}
+
+	snapType, err := s.be.SetupSnap(snapPath, "hello_instance", &si, progress.Null)
+	c.Assert(err, IsNil)
+	c.Check(snapType, Equals, snap.TypeApp)
+
+	minInfo := snap.MinimalPlaceInfo("hello_instance", snap.R(14))
+	// mount dir was created
+	c.Assert(osutil.FileExists(minInfo.MountDir()), Equals, true)
+
+	s.be.RemoveSnapFiles(minInfo, snapType, progress.Null)
+	c.Assert(err, IsNil)
+
+	l, _ := filepath.Glob(filepath.Join(dirs.SnapServicesDir, "*.mount"))
+	c.Assert(l, HasLen, 0)
+	c.Assert(osutil.FileExists(minInfo.MountDir()), Equals, false)
+	c.Assert(osutil.FileExists(minInfo.MountFile()), Equals, false)
+	c.Assert(osutil.FileExists(snap.BaseDir(minInfo.InstanceName())), Equals, true)
+	c.Assert(osutil.FileExists(snap.BaseDir(minInfo.SnapName())), Equals, true)
+
+	// /snap/hello is kept as other instances exist
+	err = s.be.RemoveSnapDir(minInfo, true)
+	c.Assert(err, IsNil)
+	c.Assert(osutil.FileExists(snap.BaseDir(minInfo.InstanceName())), Equals, false)
+	c.Assert(osutil.FileExists(snap.BaseDir(minInfo.SnapName())), Equals, true)
+
+	// /snap/hello is removed when there are no more instances
+	err = s.be.RemoveSnapDir(minInfo, false)
+	c.Assert(err, IsNil)
+	c.Assert(osutil.FileExists(snap.BaseDir(minInfo.SnapName())), Equals, false)
+}

overlord/snapstate/backend/snapdata.go

@@ -52,6 +52,26 @@ func (b Backend) RemoveSnapCommonData(snap *snap.Info) error {
 	return removeDirs(dirs)
 }
 
+// RemoveSnapDataDir removes base snap data directory
+func (b Backend) RemoveSnapDataDir(info *snap.Info, hasOtherInstances bool) error {
+	if info.InstanceKey != "" {
+		// data directories of snaps with instance key are never used by
+		// other instances
+		if err := os.Remove(snap.BaseDataDir(info.InstanceName())); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("failed to remove snap %q base directory: %v", info.InstanceName(), err)
+		}
+	}
+	if !hasOtherInstances {
+		// remove the snap base directory only if there are no other
+		// snap instances using it
+		if err := os.Remove(snap.BaseDataDir(info.SnapName())); err != nil && !os.IsNotExist(err) {
+			return fmt.Errorf("failed to remove snap %q base directory: %v", info.SnapName(), err)
+		}
+	}
+
+	return nil
+}
+
 func (b Backend) untrashData(snap *snap.Info) error {
 	dirs, err := snapDataDirs(snap)
 	if err != nil {
@@ -72,9 +92,6 @@ func removeDirs(dirs []string) error {
 		if err := os.RemoveAll(dir); err != nil {
 			return err
 		}
-
-		// Attempt to remove the parent directory as well (ignore any failure)
-		os.Remove(filepath.Dir(dir))
 	}
 
 	return nil

overlord/snapstate/backend/snapdata_test.go

@@ -0,0 +1,120 @@
+// -*- Mode: Go; indent-tabs-mode: t -*-
+
+/*
+ * Copyright (C) 2018 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_test
+
+import (
+	"os"
+	"path/filepath"
+
+	. "gopkg.in/check.v1"
+
+	"github.com/snapcore/snapd/dirs"
+	"github.com/snapcore/snapd/osutil"
+	"github.com/snapcore/snapd/snap"
+	"github.com/snapcore/snapd/snap/snaptest"
+
+	"github.com/snapcore/snapd/overlord/snapstate/backend"
+)
+
+type snapdataSuite struct {
+	be      backend.Backend
+	tempdir string
+}
+
+var _ = Suite(&snapdataSuite{})
+
+func (s *snapdataSuite) SetUpTest(c *C) {
+	s.tempdir = c.MkDir()
+	dirs.SetRootDir(s.tempdir)
+}
+
+func (s *snapdataSuite) TearDownTest(c *C) {
+	dirs.SetRootDir("")
+}
+
+func (s *snapdataSuite) TestRemoveSnapData(c *C) {
+	homedir := filepath.Join(s.tempdir, "home", "user1", "snap")
+	homeData := filepath.Join(homedir, "hello/10")
+	err := os.MkdirAll(homeData, 0755)
+	c.Assert(err, IsNil)
+	varData := filepath.Join(dirs.SnapDataDir, "hello/10")
+	err = os.MkdirAll(varData, 0755)
+	c.Assert(err, IsNil)
+
+	info := snaptest.MockSnap(c, helloYaml1, &snap.SideInfo{Revision: snap.R(10)})
+	err = s.be.RemoveSnapData(info)
+
+	c.Assert(err, IsNil)
+	c.Assert(osutil.FileExists(homeData), Equals, false)
+	c.Assert(osutil.FileExists(filepath.Dir(homeData)), Equals, true)
+	c.Assert(osutil.FileExists(varData), Equals, false)
+	c.Assert(osutil.FileExists(filepath.Dir(varData)), Equals, true)
+}
+
+func (s *snapdataSuite) TestRemoveSnapCommonData(c *C) {
+	homedir := filepath.Join(s.tempdir, "home", "user1", "snap")
+	homeCommonData := filepath.Join(homedir, "hello/common")
+	err := os.MkdirAll(homeCommonData, 0755)
+	c.Assert(err, IsNil)
+	varCommonData := filepath.Join(dirs.SnapDataDir, "hello/common")
+	err = os.MkdirAll(varCommonData, 0755)
+	c.Assert(err, IsNil)
+
+	info := snaptest.MockSnap(c, helloYaml1, &snap.SideInfo{Revision: snap.R(10)})
+
+	err = s.be.RemoveSnapCommonData(info)
+	c.Assert(err, IsNil)
+	c.Assert(osutil.FileExists(homeCommonData), Equals, false)
+	c.Assert(osutil.FileExists(filepath.Dir(homeCommonData)), Equals, true)
+	c.Assert(osutil.FileExists(varCommonData), Equals, false)
+	c.Assert(osutil.FileExists(filepath.Dir(varCommonData)), Equals, true)
+}
+
+func (s *snapdataSuite) TestRemoveSnapDataDir(c *C) {
+	varBaseData := filepath.Join(dirs.SnapDataDir, "hello")
+	err := os.MkdirAll(varBaseData, 0755)
+	c.Assert(err, IsNil)
+	varBaseDataInstance := filepath.Join(dirs.SnapDataDir, "hello_instance")
+	err = os.MkdirAll(varBaseDataInstance, 0755)
+	c.Assert(err, IsNil)
+
+	info := snaptest.MockSnap(c, helloYaml1, &snap.SideInfo{Revision: snap.R(10)})
+
+	err = s.be.RemoveSnapDataDir(info, true)
+	c.Assert(err, IsNil)
+	c.Assert(osutil.FileExists(varBaseData), Equals, true)
+	c.Assert(osutil.FileExists(varBaseDataInstance), Equals, true)
+
+	// now with instance key
+	info.InstanceKey = "instance"
+	err = s.be.RemoveSnapDataDir(info, true)
+	c.Assert(err, IsNil)
+	// instance directory is gone
+	c.Assert(osutil.FileExists(varBaseDataInstance), Equals, false)
+	// but the snap-name one is still around
+	c.Assert(osutil.FileExists(varBaseData), Equals, true)
+
+	// back to no instance key
+	info.InstanceKey = ""
+	err = s.be.RemoveSnapDataDir(info, false)
+	c.Assert(err, IsNil)
+	// the snap-name directory is gone now too
+	c.Assert(osutil.FileExists(varBaseData), Equals, false)
+}

overlord/snapstate/backend_test.go

@@ -61,6 +61,8 @@ type fakeOp struct {
 	rmAliases []*backend.Alias
 
 	userID int
+
+	otherInstances bool
 }
 
 type fakeOps []fakeOp
@@ -626,6 +628,9 @@ func (f *fakeSnappyBackend) ReadInfo(name string, si *snap.SideInfo) (*snap.Info
 	if name == "borken" && si.Revision == snap.R(2) {
 		return nil, errors.New(`cannot read info for "borken" snap`)
 	}
+	if name == "borken-undo-setup" && si.Revision == snap.R(2) {
+		return nil, errors.New(`cannot read info for "borken-undo-setup" snap`)
+	}
 	if name == "not-there" && si.Revision == snap.R(2) {
 		return nil, &snap.NotFoundError{Snap: name, Revision: si.Revision}
 	}
@@ -769,6 +774,9 @@ func (f *fakeSnappyBackend) UndoSetupSnap(s snap.PlaceInfo, typ snap.Type, p pro
 		path:  s.MountDir(),
 		stype: typ,
 	})
+	if s.InstanceName() == "borken-undo-setup" {
+		return errors.New(`cannot undo setup of "borken-undo-setup" snap`)
+	}
 	return nil
 }
 
@@ -821,6 +829,26 @@ func (f *fakeSnappyBackend) RemoveSnapCommonData(info *snap.Info) error {
 	return nil
 }
 
+func (f *fakeSnappyBackend) RemoveSnapDataDir(info *snap.Info, otherInstances bool) error {
+	f.ops = append(f.ops, fakeOp{
+		op:             "remove-snap-data-dir",
+		name:           info.InstanceName(),
+		path:           snap.BaseDataDir(info.SnapName()),
+		otherInstances: otherInstances,
+	})
+	return nil
+}
+
+func (f *fakeSnappyBackend) RemoveSnapDir(s snap.PlaceInfo, otherInstances bool) error {
+	f.ops = append(f.ops, fakeOp{
+		op:             "remove-snap-dir",
+		name:           s.InstanceName(),
+		path:           snap.BaseDir(s.SnapName()),
+		otherInstances: otherInstances,
+	})
+	return nil
+}
+
 func (f *fakeSnappyBackend) DiscardSnapNamespace(snapName string) error {
 	f.appendOp(&fakeOp{
 		op:   "discard-namespace",

overlord/snapstate/export_test.go

@@ -84,6 +84,8 @@ var (
 	ValidateFeatureFlags   = validateFeatureFlags
 
 	DefaultContentPlugProviders = defaultContentPlugProviders
+
+	HasOtherInstances = hasOtherInstances
 )
 
 func PreviousSideInfo(snapst *SnapState) *snap.SideInfo {