Merge changes I8686a1be,I4a9ea40d into stable/havana

* changes:
  GlusterFS: Complete snapshot_delete when info doesn't exist
  GlusterFS: Ensure Cinder can write to shares

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)
 

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()."""

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

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)
+        snap_info = self._read_info_file(info_path, empty_if_missing=True)
-        snapshot_file = snap_info[snapshot['id']]
 
+        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.

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