systemd: allow only a single daemon-reload at the same time

This is an RFC PR to see if the "mount protocol error" reported in
systemd/systemd#10872 can be worked around by serializing the mount unit
adding/removal. Proposing to get full spread runs.

This is similar to snapcore#6243 but it goes further by ensuring a single daemon
reload on the systemd go package level. Note that there is still a
chance that the protocol error happens if something else (like dpkg or
the user) runs "systemd daemon-reload" while we write a mount unit.
But the risk should be hughely smaller.

overlord/snapstate/backend/mountunit.go

@@ -20,13 +20,7 @@
 package backend
 
 import (
-	"os"
-	"os/exec"
-	"path/filepath"
-	"time"
-
 	"github.com/snapcore/snapd/dirs"
-	"github.com/snapcore/snapd/osutil"
 	"github.com/snapcore/snapd/progress"
 	"github.com/snapcore/snapd/snap"
 	"github.com/snapcore/snapd/systemd"
@@ -37,57 +31,11 @@ func addMountUnit(s *snap.Info, meter progress.Meter) error {
 	whereDir := dirs.StripRootDir(s.MountDir())
 
 	sysd := systemd.New(dirs.GlobalRootDir, meter)
-	mountUnitName, err := sysd.WriteMountUnitFile(s.InstanceName(), s.Revision.String(), squashfsPath, whereDir, "squashfs")
-	if err != nil {
-		return err
-	}
-
-	// we need to do a daemon-reload here to ensure that systemd really
-	// knows about this new mount unit file
-	if err := sysd.DaemonReload(); err != nil {
-		return err
-	}
-
-	if err := sysd.Enable(mountUnitName); err != nil {
-		return err
-	}
-
-	return sysd.Start(mountUnitName)
+	_, err := sysd.AddMountUnitFile(s.InstanceName(), s.Revision.String(), squashfsPath, whereDir, "squashfs")
+	return err
 }
 
-func removeMountUnit(baseDir string, meter progress.Meter) error {
+func removeMountUnit(mountDir string, meter progress.Meter) error {
 	sysd := systemd.New(dirs.GlobalRootDir, meter)
-	unit := systemd.MountUnitPath(dirs.StripRootDir(baseDir))
-	if osutil.FileExists(unit) {
-		// use umount -d (cleanup loopback devices) -l (lazy) to ensure that even busy mount points
-		// can be unmounted.
-		// note that the long option --lazy is not supported on trusty.
-		// the explicit -d is only needed on trusty.
-		isMounted, err := osutil.IsMounted(baseDir)
-		if err != nil {
-			return err
-		}
-		if isMounted {
-			if output, err := exec.Command("umount", "-d", "-l", baseDir).CombinedOutput(); err != nil {
-				return osutil.OutputErr(output, err)
-			}
-
-			if err := sysd.Stop(filepath.Base(unit), time.Duration(1*time.Second)); err != nil {
-				return err
-			}
-		}
-		if err := sysd.Disable(filepath.Base(unit)); err != nil {
-			return err
-		}
-		if err := os.Remove(unit); err != nil {
-			return err
-		}
-		// daemon-reload to ensure that systemd actually really
-		// forgets about this mount unit
-		if err := sysd.DaemonReload(); err != nil {
-			return err
-		}
-	}
-
-	return nil
+	return sysd.RemoveMountUnitFile(mountDir)
 }

systemd/systemd.go

@@ -23,11 +23,13 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"os/exec"
 	"path/filepath"
 	"regexp"
 	"strconv"
 	"strings"
+	"sync"
 	"time"
 
 	_ "github.com/snapcore/squashfuse"
@@ -46,6 +48,15 @@ var (
 
 	// how much time should Stop wait between notifying the user of the waiting
 	stopNotifyDelay = 20 * time.Second
+
+	// daemonReloadLock is a package level lock to ensure that we
+	// do not run any `systemd daemon-reload` while a
+	// daemon-reload is in progress or a mount unit is
+	// generated/activated.
+	//
+	// See https://github.com/systemd/systemd/issues/10872 for the
+	// upstream systemd bug
+	daemonReloadLock sync.Mutex
 )
 
 // systemctlCmd calls systemctl with the given args, returning its standard output (and wrapped error)
@@ -133,7 +144,8 @@ type Systemd interface {
 	IsEnabled(service string) (bool, error)
 	IsActive(service string) (bool, error)
 	LogReader(services []string, n int, follow bool) (io.ReadCloser, error)
-	WriteMountUnitFile(name, revision, what, where, fstype string) (string, error)
+	AddMountUnitFile(name, revision, what, where, fstype string) (string, error)
+	RemoveMountUnitFile(baseDir string) error
 	Mask(service string) error
 	Unmask(service string) error
 }
@@ -170,7 +182,14 @@ type systemd struct {
 }
 
 // DaemonReload reloads systemd's configuration.
-func (*systemd) DaemonReload() error {
+func (s *systemd) DaemonReload() error {
+	daemonReloadLock.Lock()
+	defer daemonReloadLock.Unlock()
+
+	return s.daemonReloadNoLock()
+}
+
+func (s *systemd) daemonReloadNoLock() error {
 	_, err := systemctlCmd("daemon-reload")
 	return err
 }
@@ -486,7 +505,11 @@ func MountUnitPath(baseDir string) string {
 	return filepath.Join(dirs.SnapServicesDir, escapedPath+".mount")
 }
 
-func (s *systemd) WriteMountUnitFile(name, revision, what, where, fstype string) (string, error) {
+// AddMountUnitFile adds/enables/starts a mount unit.
+func (s *systemd) AddMountUnitFile(snapName, revision, what, where, fstype string) (string, error) {
+	daemonReloadLock.Lock()
+	defer daemonReloadLock.Unlock()
+
 	options := []string{"nodev"}
 	if fstype == "squashfs" {
 		newFsType, newOptions, err := squashfs.FsType()
@@ -513,8 +536,67 @@ Options=%s
 
 [Install]
 WantedBy=multi-user.target
-`, name, revision, what, where, fstype, strings.Join(options, ","))
+`, snapName, revision, what, where, fstype, strings.Join(options, ","))
 
 	mu := MountUnitPath(where)
-	return filepath.Base(mu), osutil.AtomicWriteFile(mu, []byte(c), 0644, 0)
+	mountUnitName, err := filepath.Base(mu), osutil.AtomicWriteFile(mu, []byte(c), 0644, 0)
+	if err != nil {
+		return "", err
+	}
+
+	// we need to do a daemon-reload here to ensure that systemd really
+	// knows about this new mount unit file
+	if err := s.daemonReloadNoLock(); err != nil {
+		return "", err
+	}
+
+	if err := s.Enable(mountUnitName); err != nil {
+		return "", err
+	}
+	if err := s.Start(mountUnitName); err != nil {
+		return "", err
+	}
+
+	return mountUnitName, nil
+}
+
+func (s *systemd) RemoveMountUnitFile(mountedDir string) error {
+	daemonReloadLock.Lock()
+	defer daemonReloadLock.Unlock()
+
+	unit := MountUnitPath(dirs.StripRootDir(mountedDir))
+	if !osutil.FileExists(unit) {
+		return nil
+	}
+
+	// use umount -d (cleanup loopback devices) -l (lazy) to ensure that even busy mount points
+	// can be unmounted.
+	// note that the long option --lazy is not supported on trusty.
+	// the explicit -d is only needed on trusty.
+	isMounted, err := osutil.IsMounted(mountedDir)
+	if err != nil {
+		return err
+	}
+	if isMounted {
+		if output, err := exec.Command("umount", "-d", "-l", mountedDir).CombinedOutput(); err != nil {
+			return osutil.OutputErr(output, err)
+		}
+
+		if err := s.Stop(filepath.Base(unit), time.Duration(1*time.Second)); err != nil {
+			return err
+		}
+	}
+	if err := s.Disable(filepath.Base(unit)); err != nil {
+		return err
+	}
+	if err := os.Remove(unit); err != nil {
+		return err
+	}
+	// daemon-reload to ensure that systemd actually really
+	// forgets about this mount unit
+	if err := s.daemonReloadNoLock(); err != nil {
+		return err
+	}
+
+	return nil
 }

systemd/systemd_test.go

@@ -486,7 +486,7 @@ func (s *SystemdTestSuite) TestMountUnitPath(c *C) {
 	c.Assert(MountUnitPath("/apps/hello/1.1"), Equals, filepath.Join(dirs.SnapServicesDir, "apps-hello-1.1.mount"))
 }
 
-func (s *SystemdTestSuite) TestWriteMountUnit(c *C) {
+func (s *SystemdTestSuite) TestAddMountUnit(c *C) {
 	restore := squashfs.MockUseFuse(false)
 	defer restore()
 
@@ -496,7 +496,7 @@ func (s *SystemdTestSuite) TestWriteMountUnit(c *C) {
 	err = ioutil.WriteFile(mockSnapPath, nil, 0644)
 	c.Assert(err, IsNil)
 
-	mountUnitName, err := New("", nil).WriteMountUnitFile("foo", "42", mockSnapPath, "/snap/snapname/123", "squashfs")
+	mountUnitName, err := New("", nil).AddMountUnitFile("foo", "42", mockSnapPath, "/snap/snapname/123", "squashfs")
 	c.Assert(err, IsNil)
 	defer os.Remove(mountUnitName)
 
@@ -516,13 +516,13 @@ WantedBy=multi-user.target
 `[1:], mockSnapPath))
 }
 
-func (s *SystemdTestSuite) TestWriteMountUnitForDirs(c *C) {
+func (s *SystemdTestSuite) TestAddMountUnitForDirs(c *C) {
 	restore := squashfs.MockUseFuse(false)
 	defer restore()
 
 	// a directory instead of a file produces a different output
 	snapDir := c.MkDir()
-	mountUnitName, err := New("", nil).WriteMountUnitFile("foodir", "x1", snapDir, "/snap/snapname/x1", "squashfs")
+	mountUnitName, err := New("", nil).AddMountUnitFile("foodir", "x1", snapDir, "/snap/snapname/x1", "squashfs")
 	c.Assert(err, IsNil)
 	defer os.Remove(mountUnitName)
 
@@ -564,7 +564,7 @@ exit 0
 	err = ioutil.WriteFile(mockSnapPath, nil, 0644)
 	c.Assert(err, IsNil)
 
-	mountUnitName, err := New("", nil).WriteMountUnitFile("foo", "x1", mockSnapPath, "/snap/snapname/123", "squashfs")
+	mountUnitName, err := New("", nil).AddMountUnitFile("foo", "x1", mockSnapPath, "/snap/snapname/123", "squashfs")
 	c.Assert(err, IsNil)
 	defer os.Remove(mountUnitName)
 
@@ -602,7 +602,7 @@ exit 0
 	err = ioutil.WriteFile(mockSnapPath, nil, 0644)
 	c.Assert(err, IsNil)
 
-	mountUnitName, err := New("", nil).WriteMountUnitFile("foo", "x1", mockSnapPath, "/snap/snapname/123", "squashfs")
+	mountUnitName, err := New("", nil).AddMountUnitFile("foo", "x1", mockSnapPath, "/snap/snapname/123", "squashfs")
 	c.Assert(err, IsNil)
 	defer os.Remove(mountUnitName)
 

wrappers/core18.go

@@ -40,7 +40,7 @@ import (
 var execStartRe = regexp.MustCompile(`(?m)^ExecStart=(/usr/bin/snap\s+.*|/usr/lib/snapd/.*)$`)
 
 func writeSnapdToolingMountUnit(sysd systemd.Systemd, prefix string) error {
-	// Not using WriteMountUnitFile() because we need
+	// Not using AddMountUnitFile() because we need
 	// "RequiredBy=snapd.service"
 	content := []byte(fmt.Sprintf(`[Unit]
 Description=Make the snapd snap tooling available for the system