CA-387770 Improve error message for readonly shares

This deals with the situation where an NFSSR or SMBSR is being attached,
but that share it uses cannot be written to.
* Make an explicit check that the share can be written to after it is
  mounted, raising a suitable error message if it's not. The check comes
  before the hard-link check, as this also writes to the share, and so
  would fail in this situation.
* If the attach fails (for this, or any other, reason) after the share
  has been mounted, ensure that it's unmounted. This is because if it is
  left mounted then a subsequent attach may succeed, by virtue of the
  checks being skipped.

Signed-off-by: Robin Newton <robin.newton@cloud.com>

drivers/FileSR.py

@@ -1075,6 +1075,20 @@ class SharedFileSR(FileSR):
     FileSR subclass for SRs that use shared network storage
     """
 
+    def _check_writable(self):
+        """
+        Checks that the filesystem being used by the SR can be written to,
+        raising an exception if it can't.
+        """
+        test_name = os.path.join(self.path, str(uuid4()))
+        try:
+            open(test_name, 'ab').close()
+        except OSError as e:
+            util.SMlog("Cannot write to SR file system: %s" % e)
+            raise xs_errors.XenError('SharedFileSystemNoWrite')
+        finally:
+            util.force_unlink(test_name)
+
     def _raise_hardlink_error(self):
         raise OSError(524, "Unknown error 524")
 

drivers/NFSSR.py

@@ -135,10 +135,16 @@ def mount(self, mountpoint, remotepath, timeout=None, retrans=None):
 
     def attach(self, sr_uuid):
         if not self._checkmount():
-            self.validate_remotepath(False)
-            util._testHost(self.dconf['server'], NFSPORT, 'NFSTarget')
-            self.mount_remotepath(sr_uuid)
-            self._check_hardlinks()
+            try:
+                self.validate_remotepath(False)
+                util._testHost(self.dconf['server'], NFSPORT, 'NFSTarget')
+                self.mount_remotepath(sr_uuid)
+                self._check_writable()
+                self._check_hardlinks()
+            except:
+                if self._checkmount():
+                    nfs.unmount(self.path, True)
+                raise
         self.attached = True
 
     def mount_remotepath(self, sr_uuid):

drivers/SMBSR.py

@@ -195,9 +195,17 @@ def attach(self, sr_uuid):
             try:
                 self.mount()
                 os.symlink(self.linkpath, self.path)
+                self._check_writable()
+                self._check_hardlinks()
             except SMBException as exc:
                 raise xs_errors.XenError('SMBMount', opterr=exc.errstr)
-            self._check_hardlinks()
+            except:
+                if util.pathexists(self.path):
+                    os.unlink(self.path)
+                if self.checkmount():
+                    self.unmount(self.mountpoint, True)
+                raise
+
         self.attached = True
 
     def probe(self):

drivers/XE_SR_ERRORCODES.xml

@@ -898,6 +898,12 @@
             <value>460</value>
         </code>
 
+        <code>
+            <name>SharedFileSystemNoWrite</name>
+            <description>The file system for SR cannot be written to.</description>
+            <value>461</value>
+        </code>
+
         <code>
             <name>GenericException</name>
             <description>SM has thrown a generic python exception</description>

tests/test_FileSR.py

@@ -426,6 +426,7 @@ def load(self, sr_uuid):
         self.lock = None
 
     def attach(self, sr_uuid):
+        self._check_writable()
         self._check_hardlinks()
 
     def _read_hardlink_conf(self):
@@ -519,6 +520,20 @@ def test_attach_fist_active(self):
         self.mock_session.xenapi.message.create.assert_called_with(
             'sr_does_not_support_hardlinks', 2, "SR", self.sr_uuid, mock.ANY)
 
+    @testlib.with_context
+    def test_attach_not_writable(self, context):
+        context.setup_error_codes()
+        test_sr = self.create_test_sr()
+
+        with mock.patch('FileSR.open') as mock_open:
+            mock_open.side_effect = OSError
+
+            with self.assertRaises(SR.SROSError) as cm:
+                test_sr.attach(self.sr_uuid)
+
+            self.assertEqual("The file system for SR cannot be written to.",
+                             str(cm.exception))
+
     def test_scan_load_vdis_scan_list_differ(self):
         """
             Load the SR VDIs

tests/test_NFSSR.py

@@ -106,6 +106,7 @@ def test_sr_create_mount_error(
         with self.assertRaises(SR.SROSError):
             nfssr.create(sr_uuid, size)
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('util.makedirs', autospec=True)
     @mock.patch('NFSSR.Lock', autospec=True)
@@ -114,7 +115,8 @@ def test_sr_create_mount_error(
     @mock.patch('nfs.check_server_tcp', autospec=True)
     @mock.patch('nfs.validate_nfsversion', autospec=True)
     def test_attach(self, validate_nfsversion, check_server_tcp, _testhost,
-                    soft_mount, Lock, makedirs, mock_checklinks):
+                    soft_mount, Lock, makedirs, mock_checklinks,
+                    mock_checkwritable):
         validate_nfsversion.return_value = "aNfsversionChanged"
         nfssr = self.create_nfssr(server='aServer', serverpath='/aServerpath',
                                   sr_uuid='UUID', useroptions='options')

tests/test_SMBSR.py

@@ -70,6 +70,7 @@ def test_attach_if_mounted_then_attached(self, mock_lock, mock_checkmount):
         smbsr.attach('asr_uuid')
         self.assertTrue(smbsr.attached)
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -78,7 +79,8 @@ def test_attach_if_mounted_then_attached(self, mock_lock, mock_checkmount):
     @mock.patch('os.symlink', autospec=True)
     @mock.patch('util.listdir', autospec=True)
     def test_attach_vanilla(self, listdir, symlink, pread, mock_lock,
-                            makeMountPoint, mock_checkmount, mock_checklinks):
+                            makeMountPoint, mock_checkmount, mock_checklinks,
+                            mock_checkwritable):
         mock_checkmount.return_value = False
         smbsr = self.create_smbsr()
         makeMountPoint.return_value = "/var/mount"
@@ -87,6 +89,7 @@ def test_attach_vanilla(self, listdir, symlink, pread, mock_lock,
         pread.assert_called_with(['mount.cifs', '\\aServer', "/var/mount", '-o', 'cache=loose,vers=3.0,actimeo=0'],
                                  new_env={'PASSWD': 'aPassword', 'USER': 'aUsername'})
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -96,14 +99,15 @@ def test_attach_vanilla(self, listdir, symlink, pread, mock_lock,
     @mock.patch('util.listdir', autospec=True)
     def test_attach_with_cifs_password(
             self, listdir, symlink, pread, mock_lock, makeMountPoint,
-            mock_checkmount, mock_checklinks):
+            mock_checkmount, mock_checklinks, mock_checkwritable):
         smbsr = self.create_smbsr(dconf_update={"password": "winter2019"})
         mock_checkmount.return_value = False
         makeMountPoint.return_value = "/var/mount"
         smbsr.attach('asr_uuid')
         self.assertTrue(smbsr.attached)
         pread.assert_called_with(['mount.cifs', '\\aServer', "/var/mount", '-o', 'cache=loose,vers=3.0,actimeo=0'], new_env={'PASSWD': 'winter2019', 'USER': 'aUsername'})
 
+    @mock.patch('FileSR.SharedFileSR._check_writable', autospec=True)
     @mock.patch('FileSR.SharedFileSR._check_hardlinks', autospec=True)
     @mock.patch('SMBSR.SMBSR.checkmount', autospec=True)
     @mock.patch('SMBSR.SMBSR.makeMountPoint', autospec=True)
@@ -113,7 +117,7 @@ def test_attach_with_cifs_password(
     @mock.patch('util.listdir', autospec=True)
     def test_attach_with_cifs_password_and_domain(
             self, listdir, symlink, pread, mock_lock, makeMountPoint,
-            mock_checkmount, mock_checklinks):
+            mock_checkmount, mock_checklinks, mock_checkwritable):
         smbsr = self.create_smbsr(username="citrix\jsmith", dconf_update={"password": "winter2019"})
         mock_checkmount.return_value = False
         makeMountPoint.return_value = "/var/mount"