Fix share-service VM restart problem

The /etc/mtab file may have mount information such as 'nfsd'
that if copied to /etc/fstab will cause the share server to
hang when rebooted.

Update /etc/fstab with exactly the newly mounted or unmounted
shares rather than simply overwriting /etc/fstab with /etc/mtab.

Change-Id: I67602bae1f928769d768008deca7bd0f2fef1ac2
Close-Bug: #1639662

etc/manila/rootwrap.d/share.filters

@@ -60,6 +60,9 @@ vgs: CommandFilter, vgs, root
 # manila/share/drivers/lvm.py: 'tune2fs', '-U', 'random', '%volume-snapshot%'
 tune2fs: CommandFilter, tune2fs, root
 
+# manila/share/drivers/generic.py: 'sed', '-i', '\'/%s/d\'', '%s'
+sed: CommandFilter, sed, root
+
 # manila/share/drivers/glusterfs.py: 'mkdir', '%s'
 # manila/share/drivers/ganesha/manager.py: 'mkdir', '-p', '%s'
 mkdir: CommandFilter, mkdir, root

manila/share/drivers/generic.py

@@ -285,16 +285,19 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
                     return True
         return False
 
-    def _sync_mount_temp_and_perm_files(self, server_details):
+    def _add_mount_permanently(self, share_id, server_details):
-        """Sync temporary and permanent files for mounted filesystems."""
+        """Add mount permanently for mounted filesystems."""
         try:
             self._ssh_exec(
                 server_details,
-                ['sudo', 'cp', const.MOUNT_FILE_TEMP, const.MOUNT_FILE],
+                ['grep', share_id, const.MOUNT_FILE_TEMP,
+                 '|', 'sudo', 'tee', '-a', const.MOUNT_FILE],
             )
         except exception.ProcessExecutionError as e:
-            LOG.error("Failed to sync mount files on server '%s'.",
+            LOG.error("Failed to add 'Share-%(share_id)s' mount "
-                      server_details['instance_id'])
+                      "permanently on server '%(instance_id)s'.",
+                      {"share_id": share_id,
+                       "instance_id": server_details['instance_id']})
             raise exception.ShareBackendException(msg=six.text_type(e))
         try:
             # Remount it to avoid postponed point of failure
@@ -302,6 +305,20 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
         except exception.ProcessExecutionError as e:
             LOG.error("Failed to mount all shares on server '%s'.",
                       server_details['instance_id'])
+
+    def _remove_mount_permanently(self, share_id, server_details):
+        """Remove mount permanently from mounted filesystems."""
+        try:
+            self._ssh_exec(
+                server_details,
+                ['sudo', 'sed', '-i', '\'/%s/d\'' % share_id,
+                 const.MOUNT_FILE],
+            )
+        except exception.ProcessExecutionError as e:
+            LOG.error("Failed to remove 'Share-%(share_id)s' mount "
+                      "permanently on server '%(instance_id)s'.",
+                      {"share_id": share_id,
+                       "instance_id": server_details['instance_id']})
             raise exception.ShareBackendException(msg=six.text_type(e))
 
     def _mount_device(self, share, server_details, volume):
@@ -342,9 +359,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
                         '&&', 'sudo', 'mount', device_path, mount_path,
                     )
                     self._ssh_exec(server_details, mount_cmd)
-
+                    self._add_mount_permanently(share.id, server_details)
-                    # Add mount permanently
-                    self._sync_mount_temp_and_perm_files(server_details)
                 else:
                     LOG.warning("Mount point '%(path)s' already exists on "
                                 "server '%(server)s'.", log_data)
@@ -370,8 +385,7 @@ class GenericShareDriver(driver.ExecuteMixin, driver.ShareDriver):
                 unmount_cmd = ['sudo', 'umount', mount_path, '&&', 'sudo',
                                'rmdir', mount_path]
                 self._ssh_exec(server_details, unmount_cmd)
-                # Remove mount permanently
+                self._remove_mount_permanently(share.id, server_details)
-                self._sync_mount_temp_and_perm_files(server_details)
             else:
                 LOG.warning("Mount point '%(path)s' does not exist on "
                             "server '%(server)s'.", log_data)

manila/tests/share/drivers/test_generic.py

@@ -307,7 +307,7 @@ class GenericShareDriverTestCase(test.TestCase):
         volume = {'mountpoint': 'fake_mount_point'}
         self.mock_object(self._driver, '_is_device_mounted',
                          mock.Mock(return_value=False))
-        self.mock_object(self._driver, '_sync_mount_temp_and_perm_files')
+        self.mock_object(self._driver, '_add_mount_permanently')
         self.mock_object(self._driver, '_ssh_exec',
                          mock.Mock(return_value=('', '')))
 
@@ -315,8 +315,8 @@ class GenericShareDriverTestCase(test.TestCase):
 
         self._driver._is_device_mounted.assert_called_once_with(
             mount_path, server, volume)
-        self._driver._sync_mount_temp_and_perm_files.assert_called_once_with(
+        self._driver._add_mount_permanently.assert_called_once_with(
-            server)
+            self.share.id, server)
         self._driver._ssh_exec.assert_called_once_with(
             server, (
                 'sudo', 'mkdir', '-p', mount_path,
@@ -365,7 +365,7 @@ class GenericShareDriverTestCase(test.TestCase):
         mount_path = '/fake/mount/path'
         self.mock_object(self._driver, '_is_device_mounted',
                          mock.Mock(return_value=True))
-        self.mock_object(self._driver, '_sync_mount_temp_and_perm_files')
+        self.mock_object(self._driver, '_remove_mount_permanently')
         self.mock_object(self._driver, '_get_mount_path',
                          mock.Mock(return_value=mount_path))
         self.mock_object(self._driver, '_ssh_exec',
@@ -376,8 +376,8 @@ class GenericShareDriverTestCase(test.TestCase):
         self._driver._get_mount_path.assert_called_once_with(self.share)
         self._driver._is_device_mounted.assert_called_once_with(
             mount_path, self.server)
-        self._driver._sync_mount_temp_and_perm_files.assert_called_once_with(
+        self._driver._remove_mount_permanently.assert_called_once_with(
-            self.server)
+            self.share.id, self.server)
         self._driver._ssh_exec.assert_called_once_with(
             self.server,
             ['sudo', 'umount', mount_path, '&&', 'sudo', 'rmdir', mount_path],
@@ -394,7 +394,7 @@ class GenericShareDriverTestCase(test.TestCase):
         mount_path = '/fake/mount/path'
         self.mock_object(self._driver, '_is_device_mounted',
                          mock.Mock(return_value=True))
-        self.mock_object(self._driver, '_sync_mount_temp_and_perm_files')
+        self.mock_object(self._driver, '_remove_mount_permanently')
         self.mock_object(self._driver, '_get_mount_path',
                          mock.Mock(return_value=mount_path))
         self.mock_object(self._driver, '_ssh_exec',
@@ -408,8 +408,8 @@ class GenericShareDriverTestCase(test.TestCase):
         self.assertEqual([mock.call(mount_path,
                                     self.server) for i in moves.range(2)],
                          self._driver._is_device_mounted.mock_calls)
-        self._driver._sync_mount_temp_and_perm_files.assert_called_once_with(
+        self._driver._remove_mount_permanently.assert_called_once_with(
-            self.server)
+            self.share.id, self.server)
         self.assertEqual(
             [mock.call(self.server, ['sudo', 'umount', mount_path,
                                      '&&', 'sudo', 'rmdir', mount_path])
@@ -488,45 +488,57 @@ class GenericShareDriverTestCase(test.TestCase):
             self.server, ['sudo', 'mount'])
         self.assertFalse(result)
 
-    def test_sync_mount_temp_and_perm_files(self):
+    def test_add_mount_permanently(self):
         self.mock_object(self._driver, '_ssh_exec')
-        self._driver._sync_mount_temp_and_perm_files(self.server)
+        self._driver._add_mount_permanently(self.share.id, self.server)
         self._driver._ssh_exec.has_calls(
             mock.call(
                 self.server,
-                ['sudo', 'cp', const.MOUNT_FILE_TEMP, const.MOUNT_FILE]),
+                ['grep', self.share.id, const.MOUNT_FILE_TEMP,
-            mock.call(self.server, ['sudo', 'mount', '-a']))
+                 '|', 'sudo', 'tee', '-a', const.MOUNT_FILE]),
+            mock.call(self.server, ['sudo', 'mount', '-a'])
+        )
 
-    def test_sync_mount_temp_and_perm_files_raise_error_on_copy(self):
+    def test_add_mount_permanently_raise_error_on_add(self):
         self.mock_object(
             self._driver, '_ssh_exec',
             mock.Mock(side_effect=exception.ProcessExecutionError))
         self.assertRaises(
             exception.ShareBackendException,
-            self._driver._sync_mount_temp_and_perm_files,
+            self._driver._add_mount_permanently,
+            self.share.id,
             self.server
         )
         self._driver._ssh_exec.assert_called_once_with(
             self.server,
-            ['sudo', 'cp', const.MOUNT_FILE_TEMP, const.MOUNT_FILE])
+            ['grep', self.share.id, const.MOUNT_FILE_TEMP,
+             '|', 'sudo', 'tee', '-a', const.MOUNT_FILE],
+        )
 
-    def test_sync_mount_temp_and_perm_files_raise_error_on_mount(self):
+    def test_remove_mount_permanently(self):
-        def raise_error_on_mount(*args, **kwargs):
+        self.mock_object(self._driver, '_ssh_exec')
-            if args[1][1] == 'cp':
+        self._driver._remove_mount_permanently(self.share.id, self.server)
-                raise exception.ProcessExecutionError()
+        self._driver._ssh_exec.assert_called_once_with(
+            self.server,
+            ['sudo', 'sed', '-i', '\'/%s/d\'' % self.share.id,
+             const.MOUNT_FILE],
+        )
 
-        self.mock_object(self._driver, '_ssh_exec',
+    def test_remove_mount_permanently_raise_error_on_remove(self):
-                         mock.Mock(side_effect=raise_error_on_mount))
+        self.mock_object(
+            self._driver, '_ssh_exec',
+            mock.Mock(side_effect=exception.ProcessExecutionError))
         self.assertRaises(
             exception.ShareBackendException,
-            self._driver._sync_mount_temp_and_perm_files,
+            self._driver._remove_mount_permanently,
+            self.share.id,
             self.server
         )
-        self._driver._ssh_exec.has_calls(
+        self._driver._ssh_exec.assert_called_once_with(
-            mock.call(
+            self.server,
-                self.server,
+            ['sudo', 'sed', '-i', '\'/%s/d\'' % self.share.id,
-                ['sudo', 'cp', const.MOUNT_FILE_TEMP, const.MOUNT_FILE]),
+             const.MOUNT_FILE],
-            mock.call(self.server, ['sudo', 'mount', '-a']))
+        )
 
     def test_get_mount_path(self):
         result = self._driver._get_mount_path(self.share)

releasenotes/notes/bug-1639662-fix-share-service-VM-restart-problem-1110f9133cc294e8.yaml

@@ -0,0 +1,6 @@
+---
+fixes:
+  - Changed sync mount permanently logic in the Generic
+    driver to select the newly mounted share from /etc/mtab
+    and insert it into /etc/fstab. Added corresponding
+    remove mount permanently functionality.