From: Mitsuhiro Tanino Date: Fri, 25 Sep 2015 16:38:23 +0000 (-0600) Subject: Robustify writing iscsi target persistence file X-Git-Url: https://review.fuel-infra.org/gitweb?a=commitdiff_plain;h=0d47c8ec3a07f3311ed9dd3d54201b7c2b1d7292;p=openstack-build%2Fcinder-build.git 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 71ee09b51..587275017 100644 --- 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\n' - ' backing-store %(bspath)s\n' - ' driver iscsi\n' - ' incominguser chap_foo chap_bar\n' - ' bsoflags foo\n' - ' write-cache bar\n' - '\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 95ea919f5..0bddc52c9 100644 --- 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\n' + ' backing-store %(bspath)s\n' + ' driver iscsi\n' + ' incominguser chap_foo chap_bar\n' + ' bsoflags foo\n' + ' write-cache bar\n' + '\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 3eef2ac3c..e2865a429 100644 --- 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 c08974f21..d129f5f38 100644 --- 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 57c02efd6..ee3ab9cc2 100644 --- 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})