Skip to content

OSSN-0094: restrict swap volume to cinder #574

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
grandchild wants to merge 1 commit into
base: stable/2023.2-m3
Choose a base branch
from
Draft

OSSN-0094: restrict swap volume to cinder #574

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions api-ref/source/os-volume-attachments.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,16 +185,16 @@ Update a volume attachment.
.. note:: This action only valid when the server is in ACTIVE, PAUSED and RESIZED state,
or a conflict(409) error will be returned.

.. warning:: When updating volumeId, this API is typically meant to
only be used as part of a larger orchestrated volume
migration operation initiated in the block storage
service via the ``os-retype`` or ``os-migrate_volume``
volume actions. Direct usage of this API to update
volumeId is not recommended and may result in needing to
hard reboot the server to update details within the guest
such as block storage serial IDs. Furthermore, updating
volumeId via this API is only implemented by `certain
compute drivers`_.
.. Important::

When updating volumeId, this API **MUST** only be used
as part of a larger orchestrated volume
migration operation initiated in the block storage
service via the ``os-retype`` or ``os-migrate_volume``
volume actions. Direct usage of this API is not supported
and will be blocked by nova with a 409 conflict.
Furthermore, updating ``volumeId`` via this API is only
implemented by `certain compute drivers`_.

.. _certain compute drivers: https://docs.openstack.org/nova/latest/user/support-matrix.html#operation_swap_volume

Expand Down
6 changes: 6 additions & 0 deletions nova/api/openstack/compute/volumes.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,6 +432,12 @@ def _update_volume_swap(self, req, instance, id, body):
except exception.VolumeNotFound as e:
raise exc.HTTPNotFound(explanation=e.format_message())

if ('migration_status' not in old_volume or
old_volume['migration_status'] in (None, '')):
message = (f"volume {old_volume_id} is not migrating this api "
"should only be called by Cinder")
raise exc.HTTPConflict(explanation=message)

new_volume_id = body['volumeAttachment']['volumeId']
try:
new_volume = self.volume_api.get(context, new_volume_id)
Expand Down
6 changes: 5 additions & 1 deletion nova/tests/fixtures/cinder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,11 +262,15 @@ def fake_get(self, context, volume_id, microversion=None):
'attachment_id': attachment['id'],
'mountpoint': '/dev/vdb',
}

migration_status = (
None if volume_id not in (
self.SWAP_OLD_VOL, self.SWAP_ERR_OLD_VOL)
else "migrating")
volume.update({
'status': 'in-use',
'attach_status': 'attached',
'attachments': attachments,
'migration_status': migration_status
})
# Otherwise mark the volume as avilable and detached
else:
Expand Down
12 changes: 10 additions & 2 deletions nova/tests/functional/notification_sample_tests/test_instance.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1554,8 +1554,14 @@ def _do_setup_server_and_error_flag(self):
def test_volume_swap_server_with_error(self):
server = self._do_setup_server_and_error_flag()

self._volume_swap_server(server, self.cinder.SWAP_ERR_OLD_VOL,
self.cinder.SWAP_ERR_NEW_VOL)
# This is calling swap volume but we are emulating cinder
# volume migrate in the fixture to allow this.
# i.e. this is simulating the workflow where you move a volume
# between cinder backend using a temp volume that cinder internally
# cleans up at the end of the migration.
self._volume_swap_server(
server, self.cinder.SWAP_ERR_OLD_VOL,
self.cinder.SWAP_ERR_NEW_VOL)
self._wait_for_notification('compute.exception')

# Eight versioned notifications are generated.
Expand All @@ -1570,6 +1576,8 @@ def test_volume_swap_server_with_error(self):
self.assertLessEqual(7, len(self.notifier.versioned_notifications),
'Unexpected number of versioned notifications. '
'Got: %s' % self.notifier.versioned_notifications)
# the block device mapping is using SWAP_ERR_OLD_VOL because this is
# the cinder volume migrate workflow.
block_devices = [{
"nova_object.data": {
"boot_index": None,
Expand Down
49 changes: 9 additions & 40 deletions nova/tests/functional/regressions/test_bug_1943431.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@

from nova import context
from nova import objects
from nova.tests.functional.api import client
from nova.tests.functional import integrated_helpers
from nova.tests.functional.libvirt import base
from nova.virt import block_device as driver_block_device
Expand Down Expand Up @@ -46,6 +47,8 @@ def setUp(self):
self.start_compute()

def test_ro_multiattach_swap_volume(self):
# NOTE(sean-k-mooney): This test is emulating calling swap volume
# directly instead of using cinder volume migrate or retype.
server_id = self._create_server(networks='none')['id']
self.api.post_server_volume(
server_id,
Expand All @@ -58,47 +61,13 @@ def test_ro_multiattach_swap_volume(self):
self._wait_for_volume_attach(
server_id, self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL)

# Swap between the old and new volumes
self.api.put_server_volume(
server_id,
self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL,
# NOTE(sean-k-mooney): because of bug 212187 directly using
# swap volume is not supported and should fail.
ex = self.assertRaises(
client.OpenStackApiException, self.api.put_server_volume,
server_id, self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL,
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL)

# Wait until the old volume is detached and new volume is attached
self._wait_for_volume_detach(
server_id, self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL)
self._wait_for_volume_attach(
server_id, self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL)

bdm = objects.BlockDeviceMapping.get_by_volume_and_instance(
context.get_admin_context(),
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
server_id)
connection_info = jsonutils.loads(bdm.connection_info)

# Assert that only the new volume UUID is referenced within the stashed
# connection_info and returned by driver_block_device.get_volume_id
self.assertIn('volume_id', connection_info.get('data'))
self.assertEqual(
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
connection_info['data']['volume_id'])
self.assertIn('volume_id', connection_info)
self.assertEqual(
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
connection_info['volume_id'])
self.assertIn('serial', connection_info)
self.assertEqual(
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
connection_info.get('serial'))
self.assertEqual(
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL,
driver_block_device.get_volume_id(connection_info))

# Assert that the new volume can be detached from the instance
self.api.delete_server_volume(
server_id, self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL)
self._wait_for_volume_detach(
server_id, self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL)
self.assertIn("this api should only be called by Cinder", str(ex))

def test_ro_multiattach_migrate_volume(self):
server_id = self._create_server(networks='none')['id']
Expand Down
67 changes: 67 additions & 0 deletions nova/tests/functional/regressions/test_bug_2112187.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
# Licensed under the Apache License, Version 2.0 (the "License"); you may
# not use this file except in compliance with the License. You may obtain
# a copy of the License at
#
# http://www.apache.org/licenses/LICENSE-2.0
#
# Unless required by applicable law or agreed to in writing, software
# distributed under the License is distributed on an "AS IS" BASIS, WITHOUT
# WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the
# License for the specific language governing permissions and limitations
# under the License.

from nova.tests.functional.api import client
from nova.tests.functional import integrated_helpers
from nova.tests.functional.libvirt import base


class TestDirectSwapVolume(
base.ServersTestBase,
integrated_helpers.InstanceHelperMixin
):
"""Regression test for bug 2112187

During a Cinder orchestrated volume migration nova leaves the
stashed connection_info of the attachment pointing at the original
volume UUID used during the migration because cinder will atomically
revert the UUID of the volume back to the original value.

When swap volume is used directly the uuid should be updated
in the libvirt xml but nova does not support that today.
That results in the uuid in the xml and the uuid in the BDMs
being out of sync.

As a result it is unsafe to allow direct swap volume.
"""

microversion = 'latest'
ADMIN_API = True

def setUp(self):
super().setUp()
self.start_compute()

def test_direct_swap_volume(self):
# NOTE(sean-k-mooney): This test is emulating calling swap volume
# directly instead of using cinder volume migrate or retype.
server_id = self._create_server(networks='none')['id']
# We do not need to use a multiattach volume but any volume
# that does not have a migration state set will work.
self.api.post_server_volume(
server_id,
{
'volumeAttachment': {
'volumeId': self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL
}
}
)
self._wait_for_volume_attach(
server_id, self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL)

# NOTE(sean-k-mooney): because of bug 212187 directly using
# swap volume is not supported and should fail.
ex = self.assertRaises(
client.OpenStackApiException, self.api.put_server_volume,
server_id, self.cinder.MULTIATTACH_RO_SWAP_OLD_VOL,
self.cinder.MULTIATTACH_RO_SWAP_NEW_VOL)
self.assertIn("this api should only be called by Cinder", str(ex))
Loading