openstack/cinder
Code Issues Proposed changes

Fix Python 3 issues in ceph and rbd drivers

Browse Source 
* Add cinder.utils.convert_str() function: helper to convert a string to
  a native string: convert to bytes on Python 2, convert to Unicode on
  Python 3. Function needed by ceph and rbd drivers.
* Replace encodeutils.safe_encode() with utils.convert_str()
* test_backup_ceph: get the size of the volume file using os.fstat() to
  fix a test. Before the volume size was a mock, but comparison between
  integers and mocks raise a TypeError on Python 3.
* Fix RBDDriver._update_volume_stats(): replace a/b with a//b to get an integer
  on Python 2 and Python 3 (a/b always returns a float on Python 3)
* Replace map() with a list-comprehension in RBDDriver because a list is
  expected. On Python 3, map() returns an iterator.
* tox.ini: add to following tests to Python 3.4

  - cinder.tests.unit.test_backup_ceph
  - cinder.tests.unit.test_rbd

Blueprint cinder-python3
Change-Id: I7acb0d9eed0d351798133e0c6ef55408fed925dd
This commit is contained in:
Victor Stinner 2015-06-23 11:55:40 +02:00
parent 300d441d51
commit c967b44110
5 changed files with 63 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

37
cinder/backup/drivers/ceph.py
View File

@ -51,7 +51,6 @@ import time
import eventlet import eventlet
from oslo_config import cfg from oslo_config import cfg
from oslo_log import log as logging from oslo_log import log as logging
from oslo_utils import encodeutils
from oslo_utils import excutils from oslo_utils import excutils
from oslo_utils import units from oslo_utils import units
from six.moves import range from six.moves import range
@ -104,7 +103,7 @@ class VolumeMetadataBackup(object):
@property @property
def name(self): def name(self):
return encodeutils.safe_encode("backup.%s.meta" % self._backup_id) return utils.convert_str("backup.%s.meta" % self._backup_id)
@property @property
def exists(self): def exists(self):
@ -181,9 +180,9 @@ class CephBackupDriver(driver.BackupDriver):
self.rbd_stripe_count = 0 self.rbd_stripe_count = 0
self.rbd_stripe_unit = 0 self.rbd_stripe_unit = 0
self._ceph_backup_user = encodeutils.safe_encode(CONF.backup_ceph_user) self._ceph_backup_user = utils.convert_str(CONF.backup_ceph_user)
self._ceph_backup_pool = encodeutils.safe_encode(CONF.backup_ceph_pool) self._ceph_backup_pool = utils.convert_str(CONF.backup_ceph_pool)
self._ceph_backup_conf = encodeutils.safe_encode(CONF.backup_ceph_conf) self._ceph_backup_conf = utils.convert_str(CONF.backup_ceph_conf)
def _validate_string_args(self, *args): def _validate_string_args(self, *args):
"""Ensure all args are non-None and non-empty.""" """Ensure all args are non-None and non-empty."""
@ -239,8 +238,7 @@ class CephBackupDriver(driver.BackupDriver):
conffile=self._ceph_backup_conf) conffile=self._ceph_backup_conf)
try: try:
client.connect() client.connect()
pool_to_open = encodeutils.safe_encode(pool or pool_to_open = utils.convert_str(pool or self._ceph_backup_pool)
self._ceph_backup_pool)
ioctx = client.open_ioctx(pool_to_open) ioctx = client.open_ioctx(pool_to_open)
return client, ioctx return client, ioctx
except self.rados.Error: except self.rados.Error:
@ -263,13 +261,13 @@ class CephBackupDriver(driver.BackupDriver):
""" """
# Ensure no unicode # Ensure no unicode
if diff_format: if diff_format:
return encodeutils.safe_encode("volume-%s.backup.base" % volume_id) return utils.convert_str("volume-%s.backup.base" % volume_id)
else: else:
if backup_id is None: if backup_id is None:
msg = _("Backup id required") msg = _("Backup id required")
raise exception.InvalidParameterValue(msg) raise exception.InvalidParameterValue(msg)
return encodeutils.safe_encode("volume-%s.backup.%s" % return utils.convert_str("volume-%s.backup.%s"
(volume_id, backup_id)) % (volume_id, backup_id))
def _discard_bytes(self, volume, offset, length): def _discard_bytes(self, volume, offset, length):
"""Trim length bytes from offset. """Trim length bytes from offset.
@ -473,7 +471,7 @@ class CephBackupDriver(driver.BackupDriver):
# Since we have deleted the base image we can delete the source # Since we have deleted the base image we can delete the source
# volume backup snapshot. # volume backup snapshot.
src_name = encodeutils.safe_encode(volume_id) src_name = utils.convert_str(volume_id)
if src_name in self.rbd.RBD().list(client.ioctx): if src_name in self.rbd.RBD().list(client.ioctx):
LOG.debug("Deleting source volume snapshot '%(snapshot)s' " LOG.debug("Deleting source volume snapshot '%(snapshot)s' "
"for backup %(basename)s.", "for backup %(basename)s.",
@ -538,15 +536,14 @@ class CephBackupDriver(driver.BackupDriver):
if from_snap is not None: if from_snap is not None:
cmd1.extend(['--from-snap', from_snap]) cmd1.extend(['--from-snap', from_snap])
if src_snap: if src_snap:
path = encodeutils.safe_encode("%s/%s@%s" % path = utils.convert_str("%s/%s@%s"
(src_pool, src_name, src_snap)) % (src_pool, src_name, src_snap))
else: else:
path = encodeutils.safe_encode("%s/%s" % (src_pool, src_name)) path = utils.convert_str("%s/%s" % (src_pool, src_name))
cmd1.extend([path, '-']) cmd1.extend([path, '-'])
cmd2 = ['rbd', 'import-diff'] + dest_ceph_args cmd2 = ['rbd', 'import-diff'] + dest_ceph_args
rbd_path = encodeutils.safe_encode("%s/%s" % (dest_pool, dest_name)) rbd_path = utils.convert_str("%s/%s" % (dest_pool, dest_name))
cmd2.extend(['-', rbd_path]) cmd2.extend(['-', rbd_path])
ret, stderr = self._piped_execute(cmd1, cmd2) ret, stderr = self._piped_execute(cmd1, cmd2)
@ -757,8 +754,8 @@ class CephBackupDriver(driver.BackupDriver):
return backup_snaps return backup_snaps
def _get_new_snap_name(self, backup_id): def _get_new_snap_name(self, backup_id):
return encodeutils.safe_encode("backup.%s.snap.%s" % return utils.convert_str("backup.%s.snap.%s"
(backup_id, time.time())) % (backup_id, time.time()))
def _get_backup_snap_name(self, rbd_image, name, backup_id): def _get_backup_snap_name(self, rbd_image, name, backup_id):
"""Return the name of the snapshot associated with backup_id. """Return the name of the snapshot associated with backup_id.
@ -932,7 +929,7 @@ class CephBackupDriver(driver.BackupDriver):
with rbd_driver.RADOSClient(self, self._ceph_backup_pool) as client: with rbd_driver.RADOSClient(self, self._ceph_backup_pool) as client:
adjust_size = 0 adjust_size = 0
base_image = self.rbd.Image(client.ioctx, base_image = self.rbd.Image(client.ioctx,
encodeutils.safe_encode(backup_base), utils.convert_str(backup_base),
read_only=True) read_only=True)
try: try:
if restore_length != base_image.size(): if restore_length != base_image.size():
@ -942,7 +939,7 @@ class CephBackupDriver(driver.BackupDriver):
if adjust_size: if adjust_size:
with rbd_driver.RADOSClient(self, src_pool) as client: with rbd_driver.RADOSClient(self, src_pool) as client:
restore_vol_encode = encodeutils.safe_encode(restore_vol) restore_vol_encode = utils.convert_str(restore_vol)
dest_image = self.rbd.Image(client.ioctx, restore_vol_encode) dest_image = self.rbd.Image(client.ioctx, restore_vol_encode)
try: try:
LOG.debug("Adjusting restore vol size") LOG.debug("Adjusting restore vol size")

1
cinder/tests/unit/test_backup_ceph.py
View File

@ -307,6 +307,7 @@ class BackupCephTestCase(test.TestCase):
rbd1 = mock.Mock() rbd1 = mock.Mock()
rbd1.read.side_effect = fake_read rbd1.read.side_effect = fake_read
rbd1.size.return_value = os.fstat(self.volume_file.fileno()).st_size
rbd2 = mock.Mock() rbd2 = mock.Mock()
rbd2.write.side_effect = mock_write_data rbd2.write.side_effect = mock_write_data

19
cinder/utils.py
View File

@ -40,6 +40,7 @@ from oslo_concurrency import lockutils
from oslo_concurrency import processutils from oslo_concurrency import processutils
from oslo_config import cfg from oslo_config import cfg
from oslo_log import log as logging from oslo_log import log as logging
from oslo_utils import encodeutils
from oslo_utils import importutils from oslo_utils import importutils
from oslo_utils import timeutils from oslo_utils import timeutils
import retrying import retrying
@ -816,3 +817,21 @@ def convert_version_to_str(version_int):
def convert_version_to_tuple(version_str): def convert_version_to_tuple(version_str):
return tuple(int(part) for part in version_str.split('.')) return tuple(int(part) for part in version_str.split('.'))
def convert_str(text):
"""Convert to native string.
Convert bytes and Unicode strings to native strings:
* convert to bytes on Python 2:
encode Unicode using encodeutils.safe_encode()
* convert to Unicode on Python 3: decode bytes from UTF-8
"""
if six.PY2:
return encodeutils.safe_encode(text)
else:
if isinstance(text, bytes):
return text.decode('utf-8')
else:
return text

48
cinder/volume/drivers/rbd.py
View File

@ -23,7 +23,6 @@ import tempfile
from eventlet import tpool from eventlet import tpool
from oslo_config import cfg from oslo_config import cfg
from oslo_log import log as logging from oslo_log import log as logging
from oslo_utils import encodeutils
from oslo_utils import units from oslo_utils import units
import six import six
from six.moves import urllib from six.moves import urllib
@ -101,9 +100,9 @@ class RBDImageMetadata(object):
"""RBD image metadata to be used with RBDImageIOWrapper.""" """RBD image metadata to be used with RBDImageIOWrapper."""
def __init__(self, image, pool, user, conf): def __init__(self, image, pool, user, conf):
self.image = image self.image = image
self.pool = encodeutils.safe_encode(pool) self.pool = utils.convert_str(pool)
self.user = encodeutils.safe_encode(user) self.user = utils.convert_str(user)
self.conf = encodeutils.safe_encode(conf) self.conf = utils.convert_str(conf)
class RBDImageIOWrapper(io.RawIOBase): class RBDImageIOWrapper(io.RawIOBase):
@ -218,11 +217,11 @@ class RBDVolumeProxy(object):
read_only=False): read_only=False):
client, ioctx = driver._connect_to_rados(pool) client, ioctx = driver._connect_to_rados(pool)
if snapshot is not None: if snapshot is not None:
snapshot = encodeutils.safe_encode(snapshot) snapshot = utils.convert_str(snapshot)
try: try:
self.volume = driver.rbd.Image(ioctx, self.volume = driver.rbd.Image(ioctx,
encodeutils.safe_encode(name), utils.convert_str(name),
snapshot=snapshot, snapshot=snapshot,
read_only=read_only) read_only=read_only)
except driver.rbd.Error: except driver.rbd.Error:
@ -287,7 +286,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
'rbd_ceph_conf', 'rbd_pool']: 'rbd_ceph_conf', 'rbd_pool']:
val = getattr(self.configuration, attr) val = getattr(self.configuration, attr)
if val is not None: if val is not None:
setattr(self.configuration, attr, encodeutils.safe_encode(val)) setattr(self.configuration, attr, utils.convert_str(val))
def check_for_setup_error(self): def check_for_setup_error(self):
"""Returns an error if prerequisites aren't met.""" """Returns an error if prerequisites aren't met."""
@ -330,7 +329,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
clustername=self.configuration.rbd_cluster_name, clustername=self.configuration.rbd_cluster_name,
conffile=self.configuration.rbd_ceph_conf)) conffile=self.configuration.rbd_ceph_conf))
if pool is not None: if pool is not None:
pool = encodeutils.safe_encode(pool) pool = utils.convert_str(pool)
else: else:
pool = self.configuration.rbd_pool pool = self.configuration.rbd_pool
@ -408,8 +407,8 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
pool['name'] == pool['name'] ==
self.configuration.rbd_pool][0]['stats'] self.configuration.rbd_pool][0]['stats']
stats['free_capacity_gb'] = ( stats['free_capacity_gb'] = (
pool_stats['max_avail'] / units.Gi) pool_stats['max_avail'] // units.Gi)
used_capacity_gb = pool_stats['bytes_used'] / units.Gi used_capacity_gb = pool_stats['bytes_used'] // units.Gi
stats['total_capacity_gb'] = (stats['free_capacity_gb'] stats['total_capacity_gb'] = (stats['free_capacity_gb']
+ used_capacity_gb) + used_capacity_gb)
except self.rados.Error: except self.rados.Error:
@ -458,8 +457,8 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
and that clone has rbd_max_clone_depth clones behind it, the source and that clone has rbd_max_clone_depth clones behind it, the source
volume will be flattened. volume will be flattened.
""" """
src_name = encodeutils.safe_encode(src_vref['name']) src_name = utils.convert_str(src_vref['name'])
dest_name = encodeutils.safe_encode(volume['name']) dest_name = utils.convert_str(volume['name'])
flatten_parent = False flatten_parent = False
# Do full copy if requested # Do full copy if requested
@ -544,7 +543,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
with RADOSClient(self) as client: with RADOSClient(self) as client:
self.RBDProxy().create(client.ioctx, self.RBDProxy().create(client.ioctx,
encodeutils.safe_encode(volume['name']), utils.convert_str(volume['name']),
size, size,
order, order,
old_format=False, old_format=False,
@ -563,10 +562,10 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
with RADOSClient(self, src_pool) as src_client: with RADOSClient(self, src_pool) as src_client:
with RADOSClient(self) as dest_client: with RADOSClient(self) as dest_client:
self.RBDProxy().clone(src_client.ioctx, self.RBDProxy().clone(src_client.ioctx,
encodeutils.safe_encode(src_image), utils.convert_str(src_image),
encodeutils.safe_encode(src_snap), utils.convert_str(src_snap),
dest_client.ioctx, dest_client.ioctx,
encodeutils.safe_encode(volume['name']), utils.convert_str(volume['name']),
features=src_client.features) features=src_client.features)
def _resize(self, volume, **kwargs): def _resize(self, volume, **kwargs):
@ -655,7 +654,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
"""Deletes a logical volume.""" """Deletes a logical volume."""
# NOTE(dosaboy): this was broken by commit cbe1d5f. Ensure names are # NOTE(dosaboy): this was broken by commit cbe1d5f. Ensure names are
# utf-8 otherwise librbd will barf. # utf-8 otherwise librbd will barf.
volume_name = encodeutils.safe_encode(volume['name']) volume_name = utils.convert_str(volume['name'])
with RADOSClient(self) as client: with RADOSClient(self) as client:
try: try:
rbd_image = self.rbd.Image(client.ioctx, volume_name) rbd_image = self.rbd.Image(client.ioctx, volume_name)
@ -725,7 +724,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
def create_snapshot(self, snapshot): def create_snapshot(self, snapshot):
"""Creates an rbd snapshot.""" """Creates an rbd snapshot."""
with RBDVolumeProxy(self, snapshot['volume_name']) as volume: with RBDVolumeProxy(self, snapshot['volume_name']) as volume:
snap = encodeutils.safe_encode(snapshot['name']) snap = utils.convert_str(snapshot['name'])
volume.create_snap(snap) volume.create_snap(snap)
volume.protect_snap(snap) volume.protect_snap(snap)
@ -733,8 +732,8 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
"""Deletes an rbd snapshot.""" """Deletes an rbd snapshot."""
# NOTE(dosaboy): this was broken by commit cbe1d5f. Ensure names are # NOTE(dosaboy): this was broken by commit cbe1d5f. Ensure names are
# utf-8 otherwise librbd will barf. # utf-8 otherwise librbd will barf.
volume_name = encodeutils.safe_encode(snapshot['volume_name']) volume_name = utils.convert_str(snapshot['volume_name'])
snap_name = encodeutils.safe_encode(snapshot['name']) snap_name = utils.convert_str(snapshot['name'])
with RBDVolumeProxy(self, volume_name) as volume: with RBDVolumeProxy(self, volume_name) as volume:
try: try:
volume.unprotect_snap(snap_name) volume.unprotect_snap(snap_name)
@ -806,7 +805,8 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
if not location.startswith(prefix): if not location.startswith(prefix):
reason = _('Not stored in rbd') reason = _('Not stored in rbd')
raise exception.ImageUnacceptable(image_id=location, reason=reason) raise exception.ImageUnacceptable(image_id=location, reason=reason)
pieces = map(urllib.parse.unquote, location[len(prefix):].split('/')) pieces = [urllib.parse.unquote(loc)
for loc in location[len(prefix):].split('/')]
if any(map(lambda p: p == '', pieces)): if any(map(lambda p: p == '', pieces)):
reason = _('Blank components') reason = _('Blank components')
raise exception.ImageUnacceptable(image_id=location, reason=reason) raise exception.ImageUnacceptable(image_id=location, reason=reason)
@ -985,8 +985,8 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
with RADOSClient(self) as client: with RADOSClient(self) as client:
rbd_name = existing_ref['source-name'] rbd_name = existing_ref['source-name']
self.RBDProxy().rename(client.ioctx, self.RBDProxy().rename(client.ioctx,
encodeutils.safe_encode(rbd_name), utils.convert_str(rbd_name),
encodeutils.safe_encode(volume['name'])) utils.convert_str(volume['name']))
def manage_existing_get_size(self, volume, existing_ref): def manage_existing_get_size(self, volume, existing_ref):
"""Return size of an existing image for manage_existing. """Return size of an existing image for manage_existing.
@ -1004,7 +1004,7 @@ class RBDDriver(driver.RetypeVD, driver.TransferVD, driver.ExtendVD,
raise exception.ManageExistingInvalidReference( raise exception.ManageExistingInvalidReference(
existing_ref=existing_ref, reason=reason) existing_ref=existing_ref, reason=reason)
rbd_name = encodeutils.safe_encode(existing_ref['source-name']) rbd_name = utils.convert_str(existing_ref['source-name'])
with RADOSClient(self) as client: with RADOSClient(self) as client:
# Raise an exception if we didn't find a suitable rbd image. # Raise an exception if we didn't find a suitable rbd image.

2
tox.ini
View File

@ -31,6 +31,7 @@ commands =
python -m testtools.run \ python -m testtools.run \
cinder.tests.unit.test_api_urlmap \ cinder.tests.unit.test_api_urlmap \
cinder.tests.unit.test_backup \ cinder.tests.unit.test_backup \
cinder.tests.unit.test_backup_ceph \
cinder.tests.unit.test_backup_driver_base \ cinder.tests.unit.test_backup_driver_base \
cinder.tests.unit.test_block_device \ cinder.tests.unit.test_block_device \
cinder.tests.unit.test_cloudbyte \ cinder.tests.unit.test_cloudbyte \
@ -56,6 +57,7 @@ commands =
cinder.tests.unit.test_nimble \ cinder.tests.unit.test_nimble \
cinder.tests.unit.test_openvstorage \ cinder.tests.unit.test_openvstorage \
cinder.tests.unit.test_qos_specs \ cinder.tests.unit.test_qos_specs \
cinder.tests.unit.test_rbd \
cinder.tests.unit.test_remotefs \ cinder.tests.unit.test_remotefs \
cinder.tests.unit.test_replication \ cinder.tests.unit.test_replication \
cinder.tests.unit.test_san \ cinder.tests.unit.test_san \