systemd, tests: RemoveMountUnitFile unmounts even if mount unit file is missing (#17038)

Author: Mohit-Chachada

systemd/emulation.go

@@ -185,11 +185,9 @@ func (s *emulation) EnsureMountUnitFile(unitOptions *MountUnitOptions) (string,
 }
 
 func (s *emulation) RemoveMountUnitFile(mountedDir string) error {
-	unit := MountUnitPath(dirs.StripRootDir(mountedDir))
-	if !osutil.FileExists(unit) {
-		return nil
-	}
-
+	// unmount regardless of whether the unit file exists as
+	// the unit file may have been deleted while the mount is
+	// still active
 	isMounted, err := osutilIsMounted(mountedDir)
 	if err != nil {
 		return err
@@ -201,6 +199,11 @@ func (s *emulation) RemoveMountUnitFile(mountedDir string) error {
 		}
 	}
 
+	unit := MountUnitPath(dirs.StripRootDir(mountedDir))
+	if !osutil.FileExists(unit) {
+		return nil
+	}
+
 	if err := s.DisableNoReload([]string{filepath.Base(unit)}); err != nil {
 		return err
 	}

systemd/systemd.go

@@ -1620,6 +1620,23 @@ func (s *systemd) EnsureMountUnitFile(unitOptions *MountUnitOptions) (string, er
 }
 
 func (s *systemd) RemoveMountUnitFile(mountedDir string) error {
+	// unmount regardless of whether the unit file exists as
+	// the unit file may have been deleted while the mount is
+	// still active
+	isMounted, err := osutilIsMounted(mountedDir)
+	if err != nil {
+		return err
+	}
+	if isMounted {
+		// 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.
+		if output, err := exec.Command("umount", "-d", "-l", mountedDir).CombinedOutput(); err != nil {
+			return osutil.OutputErr(output, err)
+		}
+	}
+
 	daemonReloadLock.Lock()
 	defer daemonReloadLock.Unlock()
 
@@ -1628,20 +1645,8 @@ func (s *systemd) RemoveMountUnitFile(mountedDir string) error {
 		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 := osutilIsMounted(mountedDir)
-	if err != nil {
-		return err
-	}
 	units := []string{filepath.Base(unit)}
 	if isMounted {
-		if output, err := exec.Command("umount", "-d", "-l", mountedDir).CombinedOutput(); err != nil {
-			return osutil.OutputErr(output, err)
-		}
-
 		if err := s.Stop(units); err != nil {
 			return err
 		}

systemd/systemd_test.go

@@ -2088,6 +2088,58 @@ func (s *SystemdTestSuite) TestRemoveMountUnit(c *C) {
 	})
 }
 
+func (s *SystemdTestSuite) TestRemoveMountUnitWhenIsMountedButDeletedUnitFile(c *C) {
+	// When the unit file is missing but the directory is still mounted
+	// (e.g. a user manually deleted the unit file), RemoveMountUnitFile
+	// must still unmount the directory and return nil error
+	rootDir := dirs.GlobalRootDir
+	mountDir := rootDir + "/snap/foo/42"
+
+	restore := MockOsutilIsMounted(func(path string) (bool, error) {
+		c.Check(path, Equals, mountDir)
+		return true, nil
+	})
+	defer restore()
+
+	mockUmountCmd := testutil.MockCommand(c, "umount", "")
+	defer mockUmountCmd.Restore()
+
+	// No unit file on disk
+	err := NewUnderRoot(rootDir, SystemMode, nil).RemoveMountUnitFile(mountDir)
+	c.Assert(err, IsNil)
+
+	// umount was called
+	c.Check(mockUmountCmd.Calls(), HasLen, 1)
+	c.Check(mockUmountCmd.Calls()[0], DeepEquals, []string{"umount", "-d", "-l", mountDir})
+	// no systemctl calls: no unit file to disable or daemon-reload for
+	c.Check(s.argses, HasLen, 0)
+}
+
+func (s *SystemdTestSuite) TestRemoveMountUnitWhenIsNotMountedAndDeletedUnitFile(c *C) {
+	// When the unit file is missing and the directory is not mounted
+	// RemoveMountUnitFile must return nil error without calling umount
+	// or systemctl
+	rootDir := dirs.GlobalRootDir
+	mountDir := rootDir + "/snap/foo/42"
+
+	restore := MockOsutilIsMounted(func(path string) (bool, error) {
+		c.Check(path, Equals, mountDir)
+		return false, nil
+	})
+	defer restore()
+
+	mockUmountCmd := testutil.MockCommand(c, "umount", "")
+	defer mockUmountCmd.Restore()
+
+	// No unit file on disk
+	err := NewUnderRoot(rootDir, SystemMode, nil).RemoveMountUnitFile(mountDir)
+	c.Assert(err, IsNil)
+
+	// nothing to unmount or disable
+	c.Check(mockUmountCmd.Calls(), HasLen, 0)
+	c.Check(s.argses, HasLen, 0)
+}
+
 func (s *SystemdTestSuite) TestDaemonReloadMutex(c *C) {
 	s.testDaemonOpWithMutex(c, Systemd.DaemonReload)
 }
@@ -2684,6 +2736,56 @@ func (s *SystemdTestSuite) TestPreseedModeRemoveMountUnitUnmounted(c *C) {
 	c.Check(mockUmountCmd.Calls(), HasLen, 0)
 }
 
+func (s *SystemdTestSuite) TestPreseedModeRemoveMountUnitMountedButNoUnitFile(c *C) {
+	// When the unit file is missing but the directory is still mounted,
+	// emulation-mode RemoveMountUnitFile must still unmount and return nil
+	// error.
+	mountDir := dirs.GlobalRootDir + "/snap/foo/42"
+
+	restore := MockOsutilIsMounted(func(path string) (bool, error) {
+		c.Check(path, Equals, mountDir)
+		return true, nil
+	})
+	defer restore()
+
+	mockUmountCmd := testutil.MockCommand(c, "umount", "")
+	defer mockUmountCmd.Restore()
+
+	// No unit file on disk
+	sysd := NewEmulationMode(dirs.GlobalRootDir)
+	c.Assert(sysd.RemoveMountUnitFile(mountDir), IsNil)
+
+	// umount was called
+	c.Check(mockUmountCmd.Calls(), HasLen, 1)
+	c.Check(mockUmountCmd.Calls()[0], DeepEquals, []string{"umount", "-d", "-l", mountDir})
+	// no systemctl calls: no unit file to disable
+	c.Check(s.argses, HasLen, 0)
+}
+
+func (s *SystemdTestSuite) TestPreseedModeRemoveMountUnitUnmountedAndNoUnitFile(c *C) {
+	// When the unit file is missing and the directory is not mounted
+	// RemoveMountUnitFile must return nil error without calling umount
+	// or systemctl
+	mountDir := dirs.GlobalRootDir + "/snap/foo/42"
+
+	restore := MockOsutilIsMounted(func(path string) (bool, error) {
+		c.Check(path, Equals, mountDir)
+		return false, nil
+	})
+	defer restore()
+
+	mockUmountCmd := testutil.MockCommand(c, "umount", "")
+	defer mockUmountCmd.Restore()
+
+	// No unit file on disk
+	sysd := NewEmulationMode(dirs.GlobalRootDir)
+	c.Assert(sysd.RemoveMountUnitFile(mountDir), IsNil)
+
+	// nothing to unmount or disable
+	c.Check(mockUmountCmd.Calls(), HasLen, 0)
+	c.Check(s.argses, HasLen, 0)
+}
+
 func (s *SystemdTestSuite) TestPreseedModeBindmountNotSupported(c *C) {
 	sysd := NewEmulationMode(dirs.GlobalRootDir)
 

tests/main/remove-with-missing-mount-unit/task.yaml

@@ -0,0 +1,40 @@
+summary: Snap remove with missing snap mount unit file
+
+details: |
+    The test verifies that snap remove succeeds even if the snap's mount unit file
+    is missing (e.g. manually deleted by a user) while the mount is still active.
+
+environment:
+  SNAP_NAME: test-snapd-tools
+
+prepare: |
+    "$TESTSTOOLS"/snaps-state install-local $SNAP_NAME
+    tests.cleanup defer snap remove --purge $SNAP_NAME
+
+execute: |
+    snap_mount_dir="$(os.paths snap-mount-dir)"
+    revision=$(snap list $SNAP_NAME | awk 'NR==2 {print $3}')
+    mount_dir="$snap_mount_dir/$SNAP_NAME/$revision"
+    mount_unit_name="$(systemd-escape --path "$mount_dir").mount"
+    mount_unit_file="/etc/systemd/system/$mount_unit_name"
+
+    echo "Verify that snap is currently mounted"
+    MATCH "$mount_dir" < /proc/self/mountinfo
+
+    echo "Verify that the mount unit file exists"
+    test -f "$mount_unit_file"
+
+    echo "Simulate user deleting the mount unit file"
+    rm "$mount_unit_file"
+    systemctl daemon-reload
+
+    echo "Verify that the mount unit file is gone but the mount is still active"
+    test ! -f "$mount_unit_file"
+    MATCH "$mount_dir" < /proc/self/mountinfo
+
+    echo "Snap remove must succeed even with the mount unit file missing"
+    snap remove --purge $SNAP_NAME
+
+    echo "Verify that the snap is fully removed"
+    NOMATCH "$mount_dir" < /proc/self/mountinfo
+    not snap list $SNAP_NAME &>/dev/null