Robustify writing iscsi target persistence file

With the current code, it's possible to end up with a zero-size
persistence file (or even corruption of the contents) if the
process gets killed or the system takes a power outage.

Switch to a "write to temp file and rename" model for writing the
persistence file.  This will make it more robust against unclean
process termination or unclean system shutdown.

Change-Id: Ic4fdb5a9f6f622b2ab9658f7d4206e4c8ca55046
Closes-Bug: #1499795

diff --git a/cinder/tests/unit/targets/test_tgt_driver.py b/cinder/tests/unit/targets/test_tgt_driver.py
index 71ee09b51becebddcfc10ec6fecd83f17ee2d4ad..58727501751c585b92e2ffcdbe2fcfcb2f9356d9 100644 (file)
--- a/cinder/tests/unit/targets/test_tgt_driver.py
+++ b/cinder/tests/unit/targets/test_tgt_driver.py
@@ -178,20 +178,6 @@ class TestTgtAdmDriver(tf.TargetDriverFixture):
                     self.testvol_path,
                     chap_auth=('chap_foo', 'chap_bar')))
 
-        mock_open.assert_called_once_with(
-            os.path.join(self.fake_volumes_dir, self.test_vol.split(':')[1]),
-            'w+')
-        expected = ('\n<target iqn.2010-10.org.openstack:volume-%(id)s>\n'
-                    '    backing-store %(bspath)s\n'
-                    '    driver iscsi\n'
-                    '    incominguser chap_foo chap_bar\n'
-                    '    bsoflags foo\n'
-                    '    write-cache bar\n'
-                    '</target>\n' % {'id': self.VOLUME_ID,
-                                     'bspath': self.testvol_path})
-        self.assertEqual(expected,
-                         mock_open.return_value.write.call_args[0][0])
-
     def test_create_iscsi_target_already_exists(self):
         def _fake_execute(*args, **kwargs):
             if 'update' in args:

diff --git a/cinder/tests/unit/test_utils.py b/cinder/tests/unit/test_utils.py
index 95ea919f5187fb1e6ec117b4b378ea6a24de9ed2..0bddc52c9dfb13ba126706272f35c9fbad7204ea 100644 (file)
--- a/cinder/tests/unit/test_utils.py
+++ b/cinder/tests/unit/test_utils.py
@@ -191,6 +191,50 @@ class GenericUtilsTestCase(test.TestCase):
                           utils.read_file_as_root,
                           test_filepath)
 
+    @mock.patch('tempfile.NamedTemporaryFile')
+    @mock.patch.object(os, 'open')
+    @mock.patch.object(os, 'fdatasync')
+    @mock.patch.object(os, 'fsync')
+    @mock.patch.object(os, 'rename')
+    @mock.patch.object(os, 'close')
+    @mock.patch.object(os.path, 'isfile')
+    @mock.patch.object(os, 'unlink')
+    def test_write_configfile(self, mock_unlink, mock_isfile, mock_close,
+                              mock_rename, mock_fsync, mock_fdatasync,
+                              mock_open, mock_tmp):
+        filename = 'foo'
+        directory = '/some/random/path'
+        filepath = os.path.join(directory, filename)
+        expected = ('\n<target iqn.2010-10.org.openstack:volume-%(id)s>\n'
+                    '    backing-store %(bspath)s\n'
+                    '    driver iscsi\n'
+                    '    incominguser chap_foo chap_bar\n'
+                    '    bsoflags foo\n'
+                    '    write-cache bar\n'
+                    '</target>\n' % {'id': filename,
+                                     'bspath': filepath})
+
+        # Normal case
+        utils.robust_file_write(directory, filename, expected)
+        mock_open.assert_called_once_with(directory, os.O_DIRECTORY)
+        mock_rename.assert_called_once_with(mock.ANY, filepath)
+        self.assertEqual(
+            expected.encode('utf-8'),
+            mock_tmp.return_value.__enter__.return_value.write.call_args[0][0]
+        )
+
+        # Failure to write persistent file.
+        tempfile = '/some/tempfile'
+        mock_tmp.return_value.__enter__.return_value.name = tempfile
+        mock_rename.side_effect = OSError
+        self.assertRaises(OSError,
+                          utils.robust_file_write,
+                          directory,
+                          filename,
+                          mock.MagicMock())
+        mock_isfile.assert_called_once_with(tempfile)
+        mock_unlink.assert_called_once_with(tempfile)
+
     @mock.patch('oslo_utils.timeutils.utcnow')
     def test_service_is_up(self, mock_utcnow):
         fts_func = datetime.datetime.fromtimestamp

diff --git a/cinder/utils.py b/cinder/utils.py
index 3eef2ac3c0b31843bd53fbd368acb6e0abd642c3..e2865a429ac79b487a3c18fe27a00e7197e3e7c7 100644 (file)
--- a/cinder/utils.py
+++ b/cinder/utils.py
@@ -46,6 +46,7 @@ from oslo_concurrency import processutils
 from oslo_config import cfg
 from oslo_log import log as logging
 from oslo_utils import encodeutils
+from oslo_utils import excutils
 from oslo_utils import importutils
 from oslo_utils import strutils
 from oslo_utils import timeutils
@@ -452,6 +453,49 @@ def read_file_as_root(file_path):
         raise exception.FileNotFound(file_path=file_path)
 
 
+def robust_file_write(directory, filename, data):
+    """Robust file write.
+
+    Use "write to temp file and rename" model for writing the
+    persistence file.
+
+    :param directory: Target directory to create a file.
+    :param filename: File name to store specified data.
+    :param data: String data.
+    """
+    tempname = None
+    dirfd = None
+    try:
+        dirfd = os.open(directory, os.O_DIRECTORY)
+
+        # write data to temporary file
+        with tempfile.NamedTemporaryFile(prefix=filename,
+                                         dir=directory,
+                                         delete=False) as tf:
+            tempname = tf.name
+            tf.write(data.encode('utf-8'))
+            tf.flush()
+            os.fdatasync(tf.fileno())
+            tf.close()
+
+            # Fsync the directory to ensure the fact of the existence of
+            # the temp file hits the disk.
+            os.fsync(dirfd)
+            # If destination file exists, it will be replaced silently.
+            os.rename(tempname, os.path.join(directory, filename))
+            # Fsync the directory to ensure the rename hits the disk.
+            os.fsync(dirfd)
+    except OSError:
+        with excutils.save_and_reraise_exception():
+            LOG.error(_LE("Failed to write persistence file: %(path)s."),
+                      {'path': os.path.join(directory, filename)})
+            if os.path.isfile(tempname):
+                os.unlink(tempname)
+    finally:
+        if dirfd:
+            os.close(dirfd)
+
+
 @contextlib.contextmanager
 def temporary_chown(path, owner_uid=None):
     """Temporarily chown a path.

diff --git a/cinder/volume/targets/cxt.py b/cinder/volume/targets/cxt.py
index c08974f21fd6e26923b07940f504d7e578da14f8..d129f5f38c816470e3b9824e8d3b7842ed1252d5 100644 (file)
--- a/cinder/volume/targets/cxt.py
+++ b/cinder/volume/targets/cxt.py
@@ -130,8 +130,7 @@ class CxtAdm(iscsi.ISCSITarget):
         if os.path.exists(volume_path):
             LOG.warning(_LW('Persistence file already exists for volume, '
                             'found file at: %s'), volume_path)
-        with open(volume_path, 'w+') as f:
-            f.write(volume_conf)
+        utils.robust_file_write(volumes_dir, vol_id, volume_conf)
         LOG.debug('Created volume path %(vp)s,\n'
                   'content: %(vc)s',
                   {'vp': volume_path, 'vc': volume_conf})

diff --git a/cinder/volume/targets/tgt.py b/cinder/volume/targets/tgt.py
index 57c02efd6ad70f1fbe97577b9e5478218b9fc37f..ee3ab9cc2cef65e8df9e878ce694c11f7d1934e2 100644 (file)
--- a/cinder/volume/targets/tgt.py
+++ b/cinder/volume/targets/tgt.py
@@ -165,8 +165,7 @@ class TgtAdm(iscsi.ISCSITarget):
         if os.path.exists(volume_path):
             LOG.warning(_LW('Persistence file already exists for volume, '
                             'found file at: %s'), volume_path)
-        with open(volume_path, 'w+') as f:
-            f.write(volume_conf)
+        utils.robust_file_write(volumes_dir, vol_id, volume_conf)
         LOG.debug(('Created volume path %(vp)s,\n'
                    'content: %(vc)s'),
                   {'vp': volume_path, 'vc': volume_conf})