diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index 5560ead3fad..575bfd04a53 100644 --- a/cinder/tests/test_glusterfs.py +++ b/cinder/tests/test_glusterfs.py @@ -19,6 +19,7 @@ import errno import json import os +import tempfile import mox as mox_lib from mox import IgnoreArg @@ -34,6 +35,7 @@ from cinder.openstack.common import processutils as putils from cinder import test from cinder.tests.compute import test_nova from cinder import units +from cinder import utils from cinder.volume import configuration as conf from cinder.volume.drivers import glusterfs @@ -311,6 +313,11 @@ class GlusterFsDriverTestCase(test.TestCase): mox = self._mox drv = self._driver + mox.StubOutWithMock(utils, 'get_file_mode') + mox.StubOutWithMock(utils, 'get_file_gid') + mox.StubOutWithMock(drv, '_execute') + mox.StubOutWithMock(drv, '_ensure_share_writable') + mox.StubOutWithMock(drv, '_get_mount_point_for_share') drv._get_mount_point_for_share(self.TEST_EXPORT1).\ AndReturn(self.TEST_MNT_POINT) @@ -319,6 +326,15 @@ class GlusterFsDriverTestCase(test.TestCase): drv._mount_glusterfs(self.TEST_EXPORT1, self.TEST_MNT_POINT, ensure=True) + utils.get_file_gid(self.TEST_MNT_POINT).AndReturn(333333) + + utils.get_file_mode(self.TEST_MNT_POINT).AndReturn(0o777) + + drv._ensure_share_writable(self.TEST_MNT_POINT) + + drv._execute('chgrp', IgnoreArg(), self.TEST_MNT_POINT, + run_as_root=True) + mox.ReplayAll() drv._ensure_share_mounted(self.TEST_EXPORT1) @@ -399,6 +415,52 @@ class GlusterFsDriverTestCase(test.TestCase): mox.VerifyAll() + def _fake_load_shares_config(self, conf): + self._driver.shares = {'127.7.7.7:/gluster1': None} + + def _fake_NamedTemporaryFile(self, prefix=None, dir=None): + raise OSError('Permission denied!') + + def test_setup_set_share_permissions(self): + mox = self._mox + drv = self._driver + + glusterfs.CONF.glusterfs_shares_config = self.TEST_SHARES_CONFIG_FILE + + self.stubs.Set(drv, '_load_shares_config', + self._fake_load_shares_config) + self.stubs.Set(tempfile, 'NamedTemporaryFile', + self._fake_NamedTemporaryFile) + mox.StubOutWithMock(os.path, 'exists') + mox.StubOutWithMock(drv, '_execute') + mox.StubOutWithMock(utils, 'get_file_gid') + mox.StubOutWithMock(utils, 'get_file_mode') + mox.StubOutWithMock(os, 'getegid') + + drv._execute('mount.glusterfs', check_exit_code=False) + + drv._execute('mkdir', '-p', mox_lib.IgnoreArg()) + + os.path.exists(self.TEST_SHARES_CONFIG_FILE).AndReturn(True) + + drv._execute('mount', '-t', 'glusterfs', '127.7.7.7:/gluster1', + mox_lib.IgnoreArg(), run_as_root=True) + + utils.get_file_gid(mox_lib.IgnoreArg()).AndReturn(33333) + # perms not writable + utils.get_file_mode(mox_lib.IgnoreArg()).AndReturn(0o000) + + os.getegid().AndReturn(888) + + drv._execute('chgrp', 888, mox_lib.IgnoreArg(), run_as_root=True) + drv._execute('chmod', 'g+w', mox_lib.IgnoreArg(), run_as_root=True) + + mox.ReplayAll() + + drv.do_setup(IsA(context.RequestContext)) + + mox.VerifyAll() + def test_find_share_should_throw_error_if_there_is_no_mounted_shares(self): """_find_share should throw error if there is no mounted shares.""" drv = self._driver @@ -765,6 +827,7 @@ class GlusterFsDriverTestCase(test.TestCase): (mox, drv) = self._mox, self._driver hashed = drv._get_hash_str(self.TEST_EXPORT1) + volume_dir = os.path.join(self.TEST_MNT_POINT_BASE, hashed) volume_path = '%s/%s/volume-%s' % (self.TEST_MNT_POINT_BASE, hashed, self.VOLUME_UUID) @@ -789,8 +852,11 @@ class GlusterFsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_get_backing_chain_for_path') mox.StubOutWithMock(drv, '_get_matching_backing_file') mox.StubOutWithMock(drv, '_write_info_file') + mox.StubOutWithMock(drv, '_ensure_share_writable') mox.StubOutWithMock(image_utils, 'qemu_img_info') + drv._ensure_share_writable(volume_dir) + img_info = image_utils.QemuImgInfo(qemu_img_info_output) image_utils.qemu_img_info(snap_path_2).AndReturn(img_info) @@ -810,7 +876,8 @@ class GlusterFsDriverTestCase(test.TestCase): snap_path_chain = [{self.SNAP_UUID: snap_file}, {'active': snap_file}] - drv._read_info_file(info_path).AndReturn(info_file_dict) + drv._read_info_file(info_path, empty_if_missing=True).\ + AndReturn(info_file_dict) drv._execute('qemu-img', 'commit', snap_path_2, run_as_root=True) @@ -850,6 +917,7 @@ class GlusterFsDriverTestCase(test.TestCase): hashed = drv._get_hash_str(self.TEST_EXPORT1) volume_file = 'volume-%s' % self.VOLUME_UUID + volume_dir = os.path.join(self.TEST_MNT_POINT_BASE, hashed) volume_path = '%s/%s/%s' % (self.TEST_MNT_POINT_BASE, hashed, volume_file) @@ -887,6 +955,7 @@ class GlusterFsDriverTestCase(test.TestCase): mox.StubOutWithMock(drv, '_write_info_file') mox.StubOutWithMock(drv, '_get_backing_chain_for_path') mox.StubOutWithMock(drv, 'get_active_image_from_info') + mox.StubOutWithMock(drv, '_ensure_share_writable') mox.StubOutWithMock(image_utils, 'qemu_img_info') info_file_dict = {self.SNAP_UUID_2: 'volume-%s.%s' % @@ -894,8 +963,11 @@ class GlusterFsDriverTestCase(test.TestCase): self.SNAP_UUID: 'volume-%s.%s' % (self.VOLUME_UUID, self.SNAP_UUID)} + drv._ensure_share_writable(volume_dir) + info_path = drv._local_path_volume(volume) + '.info' - drv._read_info_file(info_path).AndReturn(info_file_dict) + drv._read_info_file(info_path, empty_if_missing=True).\ + AndReturn(info_file_dict) img_info = image_utils.QemuImgInfo(qemu_img_info_output_snap_1) image_utils.qemu_img_info(snap_path).AndReturn(img_info) @@ -930,6 +1002,44 @@ class GlusterFsDriverTestCase(test.TestCase): mox.VerifyAll() + def test_delete_snapshot_not_in_info(self): + """Snapshot not in info file / info file doesn't exist. + + Snapshot creation failed so nothing is on-disk. Driver + should allow operation to succeed so the manager can + remove the snapshot record. + + (Scenario: Snapshot object created in Cinder db but not + on backing storage.) + + """ + (mox, drv) = self._mox, self._driver + + hashed = drv._get_hash_str(self.TEST_EXPORT1) + volume_dir = os.path.join(self.TEST_MNT_POINT_BASE, hashed) + volume_filename = 'volume-%s' % self.VOLUME_UUID + volume_path = os.path.join(volume_dir, volume_filename) + info_path = '%s%s' % (volume_path, '.info') + + mox.StubOutWithMock(drv, '_read_file') + mox.StubOutWithMock(drv, '_read_info_file') + mox.StubOutWithMock(drv, '_ensure_share_writable') + + snap_ref = {'name': 'test snap', + 'volume_id': self.VOLUME_UUID, + 'volume': self._simple_volume(), + 'id': self.SNAP_UUID_2} + + drv._ensure_share_writable(volume_dir) + + drv._read_info_file(info_path, empty_if_missing=True).AndReturn({}) + + mox.ReplayAll() + + drv.delete_snapshot(snap_ref) + + mox.VerifyAll() + def test_read_info_file(self): (mox, drv) = self._mox, self._driver @@ -1128,6 +1238,7 @@ class GlusterFsDriverTestCase(test.TestCase): hashed = drv._get_hash_str(self.TEST_EXPORT1) volume_file = 'volume-%s' % self.VOLUME_UUID + volume_dir = os.path.join(self.TEST_MNT_POINT_BASE, hashed) volume_path = '%s/%s/%s' % (self.TEST_MNT_POINT_BASE, hashed, volume_file) @@ -1143,11 +1254,15 @@ class GlusterFsDriverTestCase(test.TestCase): mox.StubOutWithMock(os.path, 'exists') mox.StubOutWithMock(db, 'snapshot_get') mox.StubOutWithMock(image_utils, 'qemu_img_info') + mox.StubOutWithMock(drv, '_ensure_share_writable') snap_info = {'active': snap_file, self.SNAP_UUID: snap_file} - drv._read_info_file(info_path).AndReturn(snap_info) + drv._ensure_share_writable(volume_dir) + + drv._read_info_file(info_path, empty_if_missing=True).\ + AndReturn(snap_info) os.path.exists(snap_path).AndReturn(True) @@ -1213,6 +1328,7 @@ class GlusterFsDriverTestCase(test.TestCase): hashed = drv._get_hash_str(self.TEST_EXPORT1) volume_file = 'volume-%s' % self.VOLUME_UUID + volume_dir = os.path.join(self.TEST_MNT_POINT_BASE, hashed) volume_path = '%s/%s/%s' % (self.TEST_MNT_POINT_BASE, hashed, volume_file) @@ -1230,12 +1346,16 @@ class GlusterFsDriverTestCase(test.TestCase): mox.StubOutWithMock(os.path, 'exists') mox.StubOutWithMock(db, 'snapshot_get') mox.StubOutWithMock(image_utils, 'qemu_img_info') + mox.StubOutWithMock(drv, '_ensure_share_writable') snap_info = {'active': snap_file_2, self.SNAP_UUID: snap_file, self.SNAP_UUID_2: snap_file_2} - drv._read_info_file(info_path).AndReturn(snap_info) + drv._ensure_share_writable(volume_dir) + + drv._read_info_file(info_path, empty_if_missing=True).\ + AndReturn(snap_info) os.path.exists(snap_path).AndReturn(True) @@ -1318,7 +1438,8 @@ class GlusterFsDriverTestCase(test.TestCase): snap_info = {'active': snap_file, self.SNAP_UUID: snap_file} - drv._read_info_file(info_path).AndReturn(snap_info) + drv._read_info_file(info_path, empty_if_missing=True).\ + AndReturn(snap_info) os.path.exists(snap_path).AndReturn(True) diff --git a/cinder/tests/test_utils.py b/cinder/tests/test_utils.py index 7d77538eeaa..cde2a96a524 100644 --- a/cinder/tests/test_utils.py +++ b/cinder/tests/test_utils.py @@ -513,6 +513,57 @@ class GenericUtilsTestCase(test.TestCase): self.mox.VerifyAll() + def _make_fake_stat(self, test_file, orig_os_stat): + """Create a fake method to stub out os.stat(). + + Generate a function that will return a particular + stat object for a given file. + + :param: test_file: file to spoof stat() for + :param: orig_os_stat: pointer to original os.stat() + """ + + def fake_stat(path): + if path == test_file: + class stat_result: + st_mode = 0o777 + st_gid = 33333 + return stat_result + else: + return orig_os_stat(path) + + return fake_stat + + def test_get_file_mode(self): + test_file = '/var/tmp/made_up_file' + + orig_os_stat = os.stat + os.stat = self._make_fake_stat(test_file, orig_os_stat) + + self.mox.ReplayAll() + + mode = utils.get_file_mode(test_file) + self.assertEqual(mode, 0o777) + + self.mox.VerifyAll() + + os.stat = orig_os_stat + + def test_get_file_gid(self): + test_file = '/var/tmp/made_up_file' + + orig_os_stat = os.stat + os.stat = self._make_fake_stat(test_file, orig_os_stat) + + self.mox.ReplayAll() + + gid = utils.get_file_gid(test_file) + self.assertEqual(gid, 33333) + + self.mox.VerifyAll() + + os.stat = orig_os_stat + class MonkeyPatchTestCase(test.TestCase): """Unit test for utils.monkey_patch().""" diff --git a/cinder/utils.py b/cinder/utils.py index 694d39434a9..88ff91232f9 100644 --- a/cinder/utils.py +++ b/cinder/utils.py @@ -30,6 +30,7 @@ import pyclbr import random import re import shutil +import stat import sys import tempfile import time @@ -820,3 +821,13 @@ def require_driver_initialized(func): raise exception.DriverNotInitialized(driver=driver_name) return func(self, *args, **kwargs) return wrapper + + +def get_file_mode(path): + """This primarily exists to make unit testing easier.""" + return stat.S_IMODE(os.stat(path).st_mode) + + +def get_file_gid(path): + """This primarily exists to make unit testing easier.""" + return os.stat(path).st_gid diff --git a/cinder/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py index 4f8f8f2ed69..72d5112638f 100644 --- a/cinder/volume/drivers/glusterfs.py +++ b/cinder/volume/drivers/glusterfs.py @@ -20,6 +20,8 @@ import hashlib import json import os import re +import stat +import tempfile import time from oslo.config import cfg @@ -108,6 +110,8 @@ class GlusterfsDriver(nfs.RemoteFsDriver): else: raise + self._ensure_shares_mounted() + def check_for_setup_error(self): """Just to override parent behavior.""" pass @@ -545,6 +549,10 @@ class GlusterfsDriver(nfs.RemoteFsDriver): If volume status is 'in-use', calculate what qcow2 files need to merge, and call to Nova to perform this operation. + :raises: InvalidVolume if status not acceptable + :raises: GlusterfsException(msg) if operation fails + :returns: None + """ LOG.debug(_('deleting snapshot %s') % snapshot['id']) @@ -554,12 +562,24 @@ class GlusterfsDriver(nfs.RemoteFsDriver): msg = _('Volume status must be "available" or "in-use".') raise exception.InvalidVolume(msg) + self._ensure_share_writable( + self._local_volume_dir(snapshot['volume'])) + # Determine the true snapshot file for this snapshot # based on the .info file info_path = self._local_path_volume(snapshot['volume']) + '.info' - snap_info = self._read_info_file(info_path) - snapshot_file = snap_info[snapshot['id']] + snap_info = self._read_info_file(info_path, empty_if_missing=True) + if snapshot['id'] not in snap_info: + # If snapshot info file is present, but snapshot record does not + # exist, do not attempt to delete. + # (This happens, for example, if snapshot_create failed due to lack + # of permission to write to the share.) + LOG.info(_('Snapshot record for %s is not present, allowing ' + 'snapshot_delete to proceed.') % snapshot['id']) + return + + snapshot_file = snap_info[snapshot['id']] LOG.debug(_('snapshot_file for this snap is %s') % snapshot_file) snapshot_path = '%s/%s' % (self._local_volume_dir(snapshot['volume']), @@ -981,6 +1001,26 @@ class GlusterfsDriver(nfs.RemoteFsDriver): LOG.debug(_('Available shares: %s') % str(self._mounted_shares)) + def _ensure_share_writable(self, path): + """Ensure that the Cinder user can write to the share. + + If not, raise an exception. + + :param path: path to test + :raises: GlusterfsException + :returns: None + """ + + prefix = '.cinder-write-test-' + str(os.getpid()) + '-' + + try: + tempfile.NamedTemporaryFile(prefix=prefix, dir=path) + except OSError: + msg = _('GlusterFS share at %(dir)s is not writable by the ' + 'Cinder volume service. Snapshot operations will not be ' + 'supported.') % {'dir': path} + raise exception.GlusterfsException(msg) + def _ensure_share_mounted(self, glusterfs_share): """Mount GlusterFS share. :param glusterfs_share: string @@ -988,6 +1028,21 @@ class GlusterfsDriver(nfs.RemoteFsDriver): mount_path = self._get_mount_point_for_share(glusterfs_share) self._mount_glusterfs(glusterfs_share, mount_path, ensure=True) + # Ensure we can write to this share + group_id = os.getegid() + current_group_id = utils.get_file_gid(mount_path) + current_mode = utils.get_file_mode(mount_path) + + if group_id != current_group_id: + cmd = ['chgrp', group_id, mount_path] + self._execute(*cmd, run_as_root=True) + + if not (current_mode & stat.S_IWGRP): + cmd = ['chmod', 'g+w', mount_path] + self._execute(*cmd, run_as_root=True) + + self._ensure_share_writable(mount_path) + def _find_share(self, volume_size_for): """Choose GlusterFS share among available ones for given volume size. Current implementation looks for greatest capacity. diff --git a/etc/cinder/rootwrap.d/volume.filters b/etc/cinder/rootwrap.d/volume.filters index dd5679671fe..269ba001516 100644 --- a/etc/cinder/rootwrap.d/volume.filters +++ b/etc/cinder/rootwrap.d/volume.filters @@ -67,6 +67,7 @@ find: CommandFilter, find, root # cinder/volume/drivers/glusterfs.py mv: CommandFilter, mv, root +chgrp: CommandFilter, chgrp, root # cinder/volumes/drivers/hds/hds.py: hus-cmd: CommandFilter, hus-cmd, root