diff --git a/openstack/cloud/_block_storage.py b/openstack/cloud/_block_storage.py index 96c8ba43e..520ac8ca3 100644 --- a/openstack/cloud/_block_storage.py +++ b/openstack/cloud/_block_storage.py @@ -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'): diff --git a/openstack/compute/v2/_proxy.py b/openstack/compute/v2/_proxy.py index 3f0373d1f..d7d8e7baa 100644 --- a/openstack/compute/v2/_proxy.py +++ b/openstack/compute/v2/_proxy.py @@ -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 ========== diff --git a/openstack/compute/v2/volume_attachment.py b/openstack/compute/v2/volume_attachment.py index eee123d77..322c553f7 100644 --- a/openstack/compute/v2/volume_attachment.py +++ b/openstack/compute/v2/volume_attachment.py @@ -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. diff --git a/openstack/tests/unit/cloud/test_volume.py b/openstack/tests/unit/cloud/test_volume.py index 0d5f1baf6..1f6ff2f2e 100644 --- a/openstack/tests/unit/cloud/test_volume.py +++ b/openstack/tests/unit/cloud/test_volume.py @@ -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', diff --git a/openstack/tests/unit/compute/v2/test_proxy.py b/openstack/tests/unit/compute/v2/test_proxy.py index 4f6a4f60c..37cb6786b 100644 --- a/openstack/tests/unit/compute/v2/test_proxy.py +++ b/openstack/tests/unit/compute/v2/test_proxy.py @@ -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): diff --git a/openstack/tests/unit/compute/v2/test_volume_attachment.py b/openstack/tests/unit/compute/v2/test_volume_attachment.py index d623fc523..04218a80b 100644 --- a/openstack/tests/unit/compute/v2/test_volume_attachment.py +++ b/openstack/tests/unit/compute/v2/test_volume_attachment.py @@ -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, ) diff --git a/releasenotes/notes/compute-volume-attachment-proxy-method-rework-dc35fe9ca3af1c16.yaml b/releasenotes/notes/compute-volume-attachment-proxy-method-rework-dc35fe9ca3af1c16.yaml new file mode 100644 index 000000000..5962e6e82 --- /dev/null +++ b/releasenotes/notes/compute-volume-attachment-proxy-method-rework-dc35fe9ca3af1c16.yaml @@ -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``