openstack/openstacksdk
Code Issues Proposed changes

compute: Fix '*volume_attachment' proxy methods

Browse Source 
The majority of these were totally broken and unusable because nova
doesn't allow you to retrieve a volume attachment by its ID: rather,
you need to use a combination of the server and volume IDs. As such,
each method should have been taking a combo of a Server object or ID and
a *Volume* object or ID, not a VolumeAttachment object or ID. These
fixes require rather extensive changes to the method signature, to the
point that we've actually added a release note to call this out. We've
included some hacky workarounds to avoid breaking existing users for
now, giving them time to migrate to the new, improved patterns.

In addition, the 'update_volume_attachment' proxy method never worked
since updates were forbidden on the resource (via 'allow_commit=False').
This is fixed by changing this value to True.

Change-Id: Iaab40ac4cc71f7626063700f476b53f7ffb5f39f
Signed-off-by: Stephen Finucane <stephenfin@redhat.com>
This commit is contained in:
Stephen Finucane 2021-12-14 16:31:38 +00:00
parent 9a17781629
commit ddf2a25049
7 changed files with 358 additions and 84 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
openstack/cloud/_block_storage.py
View File

@ -348,7 +348,8 @@ class BlockStorageCloudMixin:
:raises: OpenStackCloudException on operation error.
"""
self.compute.delete_volume_attachment(
volume['id'], server['id'],
server=server['id'],
volume=volume['id'],
ignore_missing=False,
)
if wait:
@ -396,11 +397,14 @@ class BlockStorageCloudMixin:
% (volume['id'], volume['status'])
)
payload = {'volumeId': volume['id']}
payload = {}
if device:
payload['device'] = device
attachment = self.compute.create_volume_attachment(
server=server['id'], **payload)
server=server['id'],
volume=volume['id'],
**payload,
)
if wait:
if not hasattr(volume, 'fetch'):

234
openstack/compute/v2/_proxy.py
View File

@ -12,6 +12,7 @@
import warnings
from openstack.block_storage.v3 import volume as _volume
from openstack.compute.v2 import aggregate as _aggregate
from openstack.compute.v2 import availability_zone
from openstack.compute.v2 import extension
@ -1559,11 +1560,15 @@ class Proxy(proxy.Proxy):
# ========== Volume Attachments ==========
def create_volume_attachment(self, server, **attrs):
# TODO(stephenfin): Make the volume argument required in 2.0
def create_volume_attachment(self, server, volume=None, **attrs):
"""Create a new volume attachment from attributes
:param server: The server can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server` instance.
:param server: The value can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server` instance that the
volume is attached to.
:param volume: The value can be either the ID of a volume or a
:class:`~openstack.block_storage.v3.volume.Volume` instance.
:param dict attrs: Keyword arguments which will be used to create a
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`,
comprised of the properties on the VolumeAttachment class.
@ -1572,115 +1577,192 @@ class Proxy(proxy.Proxy):
:rtype:
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
"""
# if the user didn't pass the new 'volume' argument, they're probably
# calling things using a legacy parameter
if volume is None:
# there are two ways to pass this legacy parameter: either using
# the openstacksdk alias, 'volume_id', or using the real nova field
# name, 'volumeId'
if 'volume_id' in attrs:
volume_id = attrs.pop('volume_id')
elif 'volumeId' in attrs:
volume_id = attrs.pop('volumeId')
else:
# the user has used neither the new way nor the old way so they
# should start using the new way
# NOTE(stephenfin): we intentionally mimic the behavior of a
# missing positional parameter in stdlib
# https://github.com/python/cpython/blob/v3.10.0/Lib/inspect.py#L1464-L1467
raise TypeError(
'create_volume_attachment() missing 1 required positional '
'argument: volume'
)
# encourage users to the new way so we can eventually remove this
# mess of logic
deprecation_msg = (
'This method was called with a volume_id or volumeId '
'argument. This is legacy behavior that will be removed in '
'a future version. Update callers to use a volume argument.'
)
warnings.warn(deprecation_msg, DeprecationWarning)
else:
volume_id = resource.Resource._get_id(volume)
server_id = resource.Resource._get_id(server)
return self._create(_volume_attachment.VolumeAttachment,
server_id=server_id, **attrs)
return self._create(
_volume_attachment.VolumeAttachment,
server_id=server_id,
volume_id=volume_id,
**attrs,
)
def update_volume_attachment(self, volume_attachment, server,
**attrs):
"""update a volume attachment
def update_volume_attachment(
self, server, volume, volume_id=None, **attrs,
):
"""Update a volume attachment
:param volume_attachment:
The value can be either the ID of a volume attachment or a
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
instance.
:param server: This parameter need to be specified when
VolumeAttachment ID is given as value. It can be
either the ID of a server or a
:class:`~openstack.compute.v2.server.Server`
instance that the attachment belongs to.
:param bool ignore_missing: When set to ``False``
:class:`~openstack.exceptions.ResourceNotFound` will be
raised when the volume attachment does not exist.
When set to ``True``, no exception will be set when
attempting to delete a nonexistent volume attachment.
Note that the underlying API expects a volume ID, not a volume
attachment ID. There is currently no way to update volume attachments
by their own ID.
:param server: The value can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server` instance that the
volume is attached to.
:param volume: The value can be either the ID of a volume or a
:class:`~openstack.block_storage.v3.volume.Volume` instance.
:param volume_id: The ID of a volume to swap to. If this is not
specified, we will default to not swapping the volume.
:param attrs: The attributes to update on the volume attachment
represented by ``volume_attachment``.
:returns: ``None``
"""
server_id = self._get_uri_attribute(volume_attachment, server,
"server_id")
volume_attachment = resource.Resource._get_id(volume_attachment)
new_volume_id = volume_id
return self._update(_volume_attachment.VolumeAttachment,
attachment_id=volume_attachment,
server_id=server_id)
server_id = resource.Resource._get_id(server)
volume_id = resource.Resource._get_id(volume)
def delete_volume_attachment(self, volume_attachment, server,
ignore_missing=True):
if new_volume_id is None:
new_volume_id = volume_id
return self._update(
_volume_attachment.VolumeAttachment,
id=volume_id,
server_id=server_id,
volume_id=new_volume_id,
**attrs,
)
# TODO(stephenfin): Remove this hack in openstacksdk 2.0
def _verify_server_volume_args(self, server, volume):
deprecation_msg = (
'The server and volume arguments to this function appear to '
'be backwards and have been reversed. This is a breaking '
'change introduced in openstacksdk 1.0. This shim will be '
'removed in a future version'
)
# if we have even partial type information and things look as they
# should, we can assume the user did the right thing
if (
isinstance(server, _server.Server)
or isinstance(volume, _volume.Volume)
):
return server, volume
# conversely, if there's type info and things appear off, tell the user
if (
isinstance(server, _volume.Volume)
or isinstance(volume, _server.Server)
):
warnings.warn(deprecation_msg, DeprecationWarning)
return volume, server
# without type info we have to try a find the server corresponding to
# the provided ID and validate it
if self.find_server(server, ignore_missing=True) is not None:
return server, volume
else:
warnings.warn(deprecation_msg, DeprecationWarning)
return volume, server
def delete_volume_attachment(self, server, volume, ignore_missing=True):
"""Delete a volume attachment
:param volume_attachment:
The value can be either the ID of a volume attachment or a
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
instance.
:param server: This parameter need to be specified when
VolumeAttachment ID is given as value. It can be either
the ID of a server or a
:class:`~openstack.compute.v2.server.Server`
instance that the attachment belongs to.
Note that the underlying API expects a volume ID, not a volume
attachment ID. There is currently no way to delete volume attachments
by their own ID.
:param server: The value can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server` instance that the
volume is attached to.
:param volume: The value can be the ID of a volume or a
:class:`~openstack.block_storage.v3.volume.Volume` instance.
:param bool ignore_missing: When set to ``False``
:class:`~openstack.exceptions.ResourceNotFound` will be
raised when the volume attachment does not exist.
When set to ``True``, no exception will be set when
attempting to delete a nonexistent volume attachment.
:class:`~openstack.exceptions.ResourceNotFound` will be raised when
the volume attachment does not exist. When set to ``True``, no
exception will be set when attempting to delete a nonexistent
volume attachment.
:returns: ``None``
"""
server_id = self._get_uri_attribute(volume_attachment, server,
"server_id")
volume_attachment = resource.Resource._get_id(volume_attachment)
server, volume = self._verify_server_volume_args(server, volume)
self._delete(_volume_attachment.VolumeAttachment,
attachment_id=volume_attachment,
server_id=server_id,
ignore_missing=ignore_missing)
server_id = resource.Resource._get_id(server)
volume_id = resource.Resource._get_id(volume)
def get_volume_attachment(self, volume_attachment, server,
ignore_missing=True):
self._delete(
_volume_attachment.VolumeAttachment,
id=volume_id,
server_id=server_id,
ignore_missing=ignore_missing,
)
def get_volume_attachment(self, server, volume):
"""Get a single volume attachment
:param volume_attachment:
The value can be the ID of a volume attachment or a
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
instance.
:param server: This parameter need to be specified when
VolumeAttachment ID is given as value. It can be either
the ID of a server or a
:class:`~openstack.compute.v2.server.Server`
instance that the attachment belongs to.
:param bool ignore_missing: When set to ``False``
:class:`~openstack.exceptions.ResourceNotFound` will be
raised when the volume attachment does not exist.
When set to ``True``, no exception will be set when
attempting to delete a nonexistent volume attachment.
Note that the underlying API expects a volume ID, not a volume
attachment ID. There is currently no way to retrieve volume attachments
by their own ID.
:param server: The value can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server` instance that the
volume is attached to.
:param volume: The value can be the ID of a volume or a
:class:`~openstack.block_storage.v3.volume.Volume` instance.
:returns: One
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
:raises: :class:`~openstack.exceptions.ResourceNotFound`
when no resource can be found.
"""
server_id = self._get_uri_attribute(volume_attachment, server,
"server_id")
volume_attachment = resource.Resource._get_id(volume_attachment)
server_id = resource.Resource._get_id(server)
volume_id = resource.Resource._get_id(volume)
return self._get(_volume_attachment.VolumeAttachment,
server_id=server_id,
attachment_id=volume_attachment,
ignore_missing=ignore_missing)
return self._get(
_volume_attachment.VolumeAttachment,
id=volume_id,
server_id=server_id,
)
def volume_attachments(self, server):
def volume_attachments(self, server, **query):
"""Return a generator of volume attachments
:param server: The server can be either the ID of a server or a
:class:`~openstack.compute.v2.server.Server`.
:params dict query: Query parameters
:returns: A generator of VolumeAttachment objects
:rtype:
:class:`~openstack.compute.v2.volume_attachment.VolumeAttachment`
"""
server_id = resource.Resource._get_id(server)
return self._list(_volume_attachment.VolumeAttachment,
server_id=server_id)
return self._list(
_volume_attachment.VolumeAttachment,
server_id=server_id,
**query,
)
# ========== Server Migrations ==========

6
openstack/compute/v2/volume_attachment.py
View File

@ -21,7 +21,7 @@ class VolumeAttachment(resource.Resource):
# capabilities
allow_create = True
allow_fetch = True
allow_commit = False
allow_commit = True
allow_delete = True
allow_list = True
@ -39,9 +39,9 @@ class VolumeAttachment(resource.Resource):
# #: The UUID of the server
# server_id = resource.Body('server_uuid')
#: The ID of the attached volume.
volume_id = resource.Body('volumeId')
volume_id = resource.Body('volumeId', alternate_id=True)
#: The UUID of the associated volume attachment in Cinder.
attachment_id = resource.Body('attachment_id', alternate_id=True)
attachment_id = resource.Body('attachment_id')
#: The ID of the block device mapping record for the attachment
bdm_id = resource.Body('bdm_uuid')
#: Virtual device tags for the attachment.

20
openstack/tests/unit/cloud/test_volume.py
View File

@ -191,6 +191,11 @@ class TestVolume(base.TestCase):
])
self.register_uris([
self.get_nova_discovery_mock_dict(),
dict(method='GET',
uri=self.get_mock_url(
'compute', 'public',
append=['servers', server['id']]),
json={'server': server}),
dict(method='DELETE',
uri=self.get_mock_url(
'compute', 'public',
@ -207,6 +212,11 @@ class TestVolume(base.TestCase):
])
self.register_uris([
self.get_nova_discovery_mock_dict(),
dict(method='GET',
uri=self.get_mock_url(
'compute', 'public',
append=['servers', server['id']]),
json={'server': server}),
dict(method='DELETE',
uri=self.get_mock_url(
'compute', 'public',
@ -230,6 +240,11 @@ class TestVolume(base.TestCase):
avail_volume = meta.obj_to_munch(fakes.FakeVolume(**vol))
self.register_uris([
self.get_nova_discovery_mock_dict(),
dict(method='GET',
uri=self.get_mock_url(
'compute', 'public',
append=['servers', server['id']]),
json={'server': server}),
dict(method='DELETE',
uri=self.get_mock_url(
'compute', 'public',
@ -254,6 +269,11 @@ class TestVolume(base.TestCase):
errored_volume = meta.obj_to_munch(fakes.FakeVolume(**vol))
self.register_uris([
self.get_nova_discovery_mock_dict(),
dict(method='GET',
uri=self.get_mock_url(
'compute', 'public',
append=['servers', server['id']]),
json={'server': server}),
dict(method='DELETE',
uri=self.get_mock_url(
'compute', 'public',

155
openstack/tests/unit/compute/v2/test_proxy.py
View File

@ -12,7 +12,10 @@
import datetime
from unittest import mock
import uuid
import warnings
from openstack.block_storage.v3 import volume
from openstack.compute.v2 import _proxy
from openstack.compute.v2 import aggregate
from openstack.compute.v2 import availability_zone as az
@ -32,6 +35,7 @@ from openstack.compute.v2 import server_migration
from openstack.compute.v2 import server_remote_console
from openstack.compute.v2 import service
from openstack.compute.v2 import usage
from openstack.compute.v2 import volume_attachment
from openstack import resource
from openstack.tests.unit import test_proxy_base
@ -453,6 +457,157 @@ class TestService(TestComputeProxy):
)
class TestVolumeAttachment(TestComputeProxy):
def test_volume_attachment_create(self):
self.verify_create(
self.proxy.create_volume_attachment,
volume_attachment.VolumeAttachment,
method_kwargs={'server': 'server_id', 'volume': 'volume_id'},
expected_kwargs={
'server_id': 'server_id',
'volume_id': 'volume_id',
},
)
def test_volume_attachment_create__legacy_parameters(self):
with warnings.catch_warnings(record=True) as w:
warnings.simplefilter('always')
self.verify_create(
self.proxy.create_volume_attachment,
volume_attachment.VolumeAttachment,
method_kwargs={'server': 'server_id', 'volumeId': 'volume_id'},
expected_kwargs={
'server_id': 'server_id',
'volume_id': 'volume_id',
},
)
self.assertEqual(1, len(w))
self.assertEqual(w[-1].category, DeprecationWarning)
self.assertIn(
'This method was called with a volume_id or volumeId argument',
str(w[-1]),
)
def test_volume_attachment_create__missing_parameters(self):
exc = self.assertRaises(
TypeError,
self.proxy.create_volume_attachment,
'server_id',
)
self.assertIn(
'create_volume_attachment() missing 1 required positional argument: volume', # noqa: E501
str(exc),
)
def test_volume_attachment_update(self):
self.verify_update(
self.proxy.update_volume_attachment,
volume_attachment.VolumeAttachment,
method_args=[],
method_kwargs={'server': 'server_id', 'volume': 'volume_id'},
expected_kwargs={
'id': 'volume_id',
'server_id': 'server_id',
'volume_id': 'volume_id',
},
)
def test_volume_attachment_delete(self):
# We pass objects to avoid the lookup that's done as part of the
# handling of legacy option order. We test that legacy path separately.
fake_server = server.Server(id=str(uuid.uuid4()))
fake_volume = volume.Volume(id=str(uuid.uuid4()))
self.verify_delete(
self.proxy.delete_volume_attachment,
volume_attachment.VolumeAttachment,
ignore_missing=False,
method_args=[fake_server, fake_volume],
method_kwargs={},
expected_args=[],
expected_kwargs={
'id': fake_volume.id,
'server_id': fake_server.id,
},
)
def test_volume_attachment_delete__ignore(self):
# We pass objects to avoid the lookup that's done as part of the
# handling of legacy option order. We test that legacy path separately.
fake_server = server.Server(id=str(uuid.uuid4()))
fake_volume = volume.Volume(id=str(uuid.uuid4()))
self.verify_delete(
self.proxy.delete_volume_attachment,
volume_attachment.VolumeAttachment,
ignore_missing=True,
method_args=[fake_server, fake_volume],
method_kwargs={},
expected_args=[],
expected_kwargs={
'id': fake_volume.id,
'server_id': fake_server.id,
},
)
def test_volume_attachment_delete__legacy_parameters(self):
fake_server = server.Server(id=str(uuid.uuid4()))
fake_volume = volume.Volume(id=str(uuid.uuid4()))
with mock.patch.object(
self.proxy,
'find_server',
return_value=None,
) as mock_find_server:
# we are calling the method with volume and server ID arguments as
# strings and in the wrong order, which results in a query as we
# attempt to match the server ID to an actual server before we
# switch the argument order once we realize we can't do this
self.verify_delete(
self.proxy.delete_volume_attachment,
volume_attachment.VolumeAttachment,
ignore_missing=False,
method_args=[fake_volume.id, fake_server.id],
method_kwargs={},
expected_args=[],
expected_kwargs={
'id': fake_volume.id,
'server_id': fake_server.id,
},
)
# note that we attempted to call the server with the volume ID but
# this was mocked to return None (as would happen in the real
# world)
mock_find_server.assert_called_once_with(
fake_volume.id,
ignore_missing=True,
)
def test_volume_attachment_get(self):
self.verify_get(
self.proxy.get_volume_attachment,
volume_attachment.VolumeAttachment,
method_args=[],
method_kwargs={'server': 'server_id', 'volume': 'volume_id'},
expected_kwargs={
'id': 'volume_id',
'server_id': 'server_id',
},
)
def test_volume_attachments(self):
self.verify_list(
self.proxy.volume_attachments,
volume_attachment.VolumeAttachment,
method_kwargs={'server': 'server_id'},
expected_kwargs={'server_id': 'server_id'},
)
class TestHypervisor(TestComputeProxy):
def test_hypervisors_not_detailed(self):

4
openstack/tests/unit/compute/v2/test_volume_attachment.py
View File

@ -35,7 +35,7 @@ class TestServerInterface(base.TestCase):
sot.base_path)
self.assertTrue(sot.allow_create)
self.assertTrue(sot.allow_fetch)
self.assertFalse(sot.allow_commit)
self.assertTrue(sot.allow_commit)
self.assertTrue(sot.allow_delete)
self.assertTrue(sot.allow_list)
self.assertDictEqual({"limit": "limit",
@ -45,8 +45,8 @@ class TestServerInterface(base.TestCase):
def test_make_it(self):
sot = volume_attachment.VolumeAttachment(**EXAMPLE)
self.assertEqual(EXAMPLE['volumeId'], sot.id)
self.assertEqual(EXAMPLE['attachment_id'], sot.attachment_id)
self.assertEqual(EXAMPLE['attachment_id'], sot.id)
self.assertEqual(
EXAMPLE['delete_on_termination'], sot.delete_on_termination,
)

13
releasenotes/notes/compute-volume-attachment-proxy-method-rework-dc35fe9ca3af1c16.yaml Normal file
View File

@ -0,0 +1,13 @@
---
upgrade:
- |
The signatures of the various volume attachment-related methods in the
compute API proxy layer have changed. These were previously incomplete and
did not function as expected in many scenarios. Some callers may need to be
reworked. The affected proxy methods are:
- ``create_volume_attachment``
- ``delete_volume_attachment``
- ``update_volume_attachment``
- ``get_volume_attachment``
- ``volume_attachments``