From d39b2d2d808a79280201f6dc00aa93f576d70acd Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Tue, 19 Nov 2013 18:01:55 -0500 Subject: [PATCH 1/2] GlusterFS: Ensure Cinder can write to shares Ensure the Cinder user can write to the GlusterFS share. This is required for snapshot functionality, and means the admin does not have to set this permission manually. Conflicts: cinder/tests/test_glusterfs.py Closes-Bug: #1236966 Change-Id: I4a9ea40df9681ca6931ad6b390aa21b09d6cfec9 (cherry picked from commit 371fa540600b20b97eae389e1f976145866cadae) --- cinder/tests/test_glusterfs.py | 78 ++++++++++++++++++++++++++++ cinder/tests/test_utils.py | 51 ++++++++++++++++++ cinder/utils.py | 11 ++++ cinder/volume/drivers/glusterfs.py | 42 +++++++++++++++ etc/cinder/rootwrap.d/volume.filters | 1 + 5 files changed, 183 insertions(+) diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index 44c78c9b689..bd9c2499c7e 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 @@ -310,6 +312,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) @@ -318,6 +325,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) @@ -398,6 +414,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 @@ -764,6 +826,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) @@ -788,8 +851,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) @@ -849,6 +915,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) @@ -886,6 +953,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' % @@ -893,6 +961,8 @@ 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) @@ -1127,6 +1197,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) @@ -1142,10 +1213,13 @@ 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._ensure_share_writable(volume_dir) + drv._read_info_file(info_path).AndReturn(snap_info) os.path.exists(snap_path).AndReturn(True) @@ -1212,6 +1286,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) @@ -1229,11 +1304,14 @@ 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._ensure_share_writable(volume_dir) + drv._read_info_file(info_path).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 cc75beccd33..a376da67f6a 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 @@ -818,3 +819,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 7f7497c37ed..eac70e106eb 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 @@ -552,6 +556,9 @@ 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' @@ -979,6 +986,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 @@ -986,6 +1013,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 ba7ea698a83..9ca28ad2521 100644 --- a/etc/cinder/rootwrap.d/volume.filters +++ b/etc/cinder/rootwrap.d/volume.filters @@ -64,6 +64,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 From 5122593add816c0d20affe2f3d703a1657ceb0fc Mon Sep 17 00:00:00 2001 From: Eric Harney Date: Tue, 19 Nov 2013 16:25:47 -0500 Subject: [PATCH 2/2] GlusterFS: Complete snapshot_delete when info doesn't exist The snapshot_delete operation will fail if the snapshot info file doesn't contain a record for the snapshot, or does not exist. This happens in cases such as when snapshot_create fails to commit anything to disk. The driver should allow the manager to delete the snapshot in this case, as there is no action required for the driver to delete anything. Closes-Bug: #1252864 (cherry picked from commit d8a11168c908fe6c6a07fbb30a5bc88a6df6e939) Change-Id: I8686a1be09dbb7984072538bff6c026bb84eeb52 --- cinder/tests/test_glusterfs.py | 53 +++++++++++++++++++++++++++--- cinder/volume/drivers/glusterfs.py | 17 ++++++++-- 2 files changed, 63 insertions(+), 7 deletions(-) diff --git a/cinder/tests/test_glusterfs.py b/cinder/tests/test_glusterfs.py index bd9c2499c7e..89f54ba5f5d 100644 --- a/cinder/tests/test_glusterfs.py +++ b/cinder/tests/test_glusterfs.py @@ -875,7 +875,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) @@ -964,7 +965,8 @@ class GlusterFsDriverTestCase(test.TestCase): 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) @@ -999,6 +1001,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 @@ -1220,7 +1260,8 @@ class GlusterFsDriverTestCase(test.TestCase): drv._ensure_share_writable(volume_dir) - 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) @@ -1312,7 +1353,8 @@ class GlusterFsDriverTestCase(test.TestCase): drv._ensure_share_writable(volume_dir) - 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) @@ -1395,7 +1437,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/volume/drivers/glusterfs.py b/cinder/volume/drivers/glusterfs.py index eac70e106eb..a9a86dbc49d 100644 --- a/cinder/volume/drivers/glusterfs.py +++ b/cinder/volume/drivers/glusterfs.py @@ -547,6 +547,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']) @@ -562,9 +566,18 @@ class GlusterfsDriver(nfs.RemoteFsDriver): # 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']),