diff --git a/api-ref/source/v3/parameters.yaml b/api-ref/source/v3/parameters.yaml index 6ebfcd95911..859c90d0a7a 100644 --- a/api-ref/source/v3/parameters.yaml +++ b/api-ref/source/v3/parameters.yaml @@ -2683,6 +2683,18 @@ shared_targets: required: true type: boolean min_version: 3.48 + max_version: 3.68 +shared_targets_tristate: + description: | + An indicator whether the host connecting the volume should lock for the + whole attach/detach process or not. ``true`` means only is iSCSI initiator + running on host doesn't support manual scans, ``false`` means never use + locks, and ``null`` means to always use locks. Look at os-brick's + ``guard_connection`` context manager. Default=True. + in: body + required: true + type: boolean + min_version: 3.69 size: description: | The size of the volume, in gibibytes (GiB). diff --git a/api-ref/source/v3/samples/versions/version-show-response.json b/api-ref/source/v3/samples/versions/version-show-response.json index 6551cf8bc5e..352098ba049 100644 --- a/api-ref/source/v3/samples/versions/version-show-response.json +++ b/api-ref/source/v3/samples/versions/version-show-response.json @@ -22,7 +22,7 @@ "min_version": "3.0", "status": "CURRENT", "updated": "2022-03-30T00:00:00Z", - "version": "3.68" + "version": "3.69" } ] } diff --git a/api-ref/source/v3/samples/versions/versions-response.json b/api-ref/source/v3/samples/versions/versions-response.json index 2569c9018d0..779a0f1a2c3 100644 --- a/api-ref/source/v3/samples/versions/versions-response.json +++ b/api-ref/source/v3/samples/versions/versions-response.json @@ -22,7 +22,7 @@ "min_version": "3.0", "status": "CURRENT", "updated": "2022-03-30T00:00:00Z", - "version": "3.68" + "version": "3.69" } ] } diff --git a/api-ref/source/v3/samples/volumes/v3.69/volume-create-response.json b/api-ref/source/v3/samples/volumes/v3.69/volume-create-response.json new file mode 100644 index 00000000000..000913c8653 --- /dev/null +++ b/api-ref/source/v3/samples/volumes/v3.69/volume-create-response.json @@ -0,0 +1,41 @@ +{ + "volume": { + "attachments": [], + "availability_zone": "nova", + "bootable": "false", + "consistencygroup_id": null, + "created_at": "2018-11-28T06:21:12.715987", + "description": null, + "encrypted": false, + "id": "2b955850-f177-45f7-9f49-ecb2c256d161", + "links": [ + { + "href": "http://127.0.0.1:33951/v3/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/2b955850-f177-45f7-9f49-ecb2c256d161", + "rel": "self" + }, + { + "href": "http://127.0.0.1:33951/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/2b955850-f177-45f7-9f49-ecb2c256d161", + "rel": "bookmark" + } + ], + "metadata": {}, + "migration_status": null, + "multiattach": false, + "name": null, + "replication_status": null, + "size": 10, + "snapshot_id": null, + "source_volid": null, + "status": "creating", + "updated_at": null, + "user_id": "c853ca26-e8ea-4797-8a52-ee124a013d0e", + "volume_type": "__DEFAULT__", + "group_id": null, + "provider_id": null, + "service_uuid": null, + "shared_targets": null, + "cluster_name": null, + "volume_type_id": "5fed9d7c-401d-46e2-8e80-f30c70cb7e1d", + "consumes_quota": true + } +} diff --git a/api-ref/source/v3/samples/volumes/v3.69/volume-show-response.json b/api-ref/source/v3/samples/volumes/v3.69/volume-show-response.json new file mode 100644 index 00000000000..4f75904eb0c --- /dev/null +++ b/api-ref/source/v3/samples/volumes/v3.69/volume-show-response.json @@ -0,0 +1,45 @@ +{ + "volume": { + "attachments": [], + "availability_zone": "nova", + "bootable": "false", + "consistencygroup_id": null, + "created_at": "2018-11-29T06:50:07.770785", + "description": null, + "encrypted": false, + "id": "f7223234-1afc-4d19-bfa3-d19deb6235ef", + "links": [ + { + "href": "http://127.0.0.1:45839/v3/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/f7223234-1afc-4d19-bfa3-d19deb6235ef", + "rel": "self" + }, + { + "href": "http://127.0.0.1:45839/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/f7223234-1afc-4d19-bfa3-d19deb6235ef", + "rel": "bookmark" + } + ], + "metadata": {}, + "migration_status": null, + "multiattach": false, + "name": null, + "os-vol-host-attr:host": null, + "os-vol-mig-status-attr:migstat": null, + "os-vol-mig-status-attr:name_id": null, + "os-vol-tenant-attr:tenant_id": "89afd400-b646-4bbc-b12b-c0a4d63e5bd3", + "replication_status": null, + "size": 10, + "snapshot_id": null, + "source_volid": null, + "status": "creating", + "updated_at": null, + "user_id": "c853ca26-e8ea-4797-8a52-ee124a013d0e", + "volume_type": "__DEFAULT__", + "provider_id": null, + "group_id": null, + "service_uuid": null, + "shared_targets": null, + "cluster_name": null, + "volume_type_id": "5fed9d7c-401d-46e2-8e80-f30c70cb7e1d", + "consumes_quota": true + } +} diff --git a/api-ref/source/v3/samples/volumes/v3.69/volume-update-response.json b/api-ref/source/v3/samples/volumes/v3.69/volume-update-response.json new file mode 100644 index 00000000000..1bb6b8dc215 --- /dev/null +++ b/api-ref/source/v3/samples/volumes/v3.69/volume-update-response.json @@ -0,0 +1,43 @@ +{ + "volume": { + "attachments": [], + "availability_zone": "nova", + "bootable": "false", + "consistencygroup_id": null, + "created_at": "2018-11-29T06:59:23.679903", + "description": "This is yet, another volume.", + "encrypted": false, + "id": "8b2459d1-0059-4e14-a89f-dfa73a452af6", + "links": [ + { + "href": "http://127.0.0.1:41467/v3/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/8b2459d1-0059-4e14-a89f-dfa73a452af6", + "rel": "self" + }, + { + "href": "http://127.0.0.1:41467/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/8b2459d1-0059-4e14-a89f-dfa73a452af6", + "rel": "bookmark" + } + ], + "metadata": { + "name": "metadata0" + }, + "migration_status": null, + "multiattach": false, + "name": "vol-003", + "replication_status": null, + "size": 10, + "snapshot_id": null, + "source_volid": null, + "status": "creating", + "updated_at": null, + "user_id": "c853ca26-e8ea-4797-8a52-ee124a013d0e", + "volume_type": "__DEFAULT__", + "group_id": null, + "provider_id": null, + "service_uuid": null, + "shared_targets": null, + "cluster_name": null, + "volume_type_id": "5fed9d7c-401d-46e2-8e80-f30c70cb7e1d", + "consumes_quota": true + } +} diff --git a/api-ref/source/v3/samples/volumes/v3.69/volumes-list-detailed-response.json b/api-ref/source/v3/samples/volumes/v3.69/volumes-list-detailed-response.json new file mode 100644 index 00000000000..c3f37c3bcf5 --- /dev/null +++ b/api-ref/source/v3/samples/volumes/v3.69/volumes-list-detailed-response.json @@ -0,0 +1,47 @@ +{ + "volumes": [ + { + "attachments": [], + "availability_zone": "nova", + "bootable": "false", + "consistencygroup_id": null, + "created_at": "2018-11-28T06:25:15.288987", + "description": null, + "encrypted": false, + "id": "cb49b381-9012-40cb-b8ee-80c19a4801b5", + "links": [ + { + "href": "http://127.0.0.1:43543/v3/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/cb49b381-9012-40cb-b8ee-80c19a4801b5", + "rel": "self" + }, + { + "href": "http://127.0.0.1:43543/89afd400-b646-4bbc-b12b-c0a4d63e5bd3/volumes/cb49b381-9012-40cb-b8ee-80c19a4801b5", + "rel": "bookmark" + } + ], + "metadata": {}, + "migration_status": null, + "multiattach": false, + "name": null, + "os-vol-host-attr:host": null, + "os-vol-mig-status-attr:migstat": null, + "os-vol-mig-status-attr:name_id": null, + "os-vol-tenant-attr:tenant_id": "89afd400-b646-4bbc-b12b-c0a4d63e5bd3", + "replication_status": null, + "size": 10, + "snapshot_id": null, + "source_volid": null, + "status": "creating", + "updated_at": null, + "user_id": "c853ca26-e8ea-4797-8a52-ee124a013d0e", + "volume_type": "__DEFAULT__", + "volume_type_id": "5fed9d7c-401d-46e2-8e80-f30c70cb7e1d", + "provider_id": null, + "group_id": null, + "service_uuid": null, + "shared_targets": null, + "cluster_name": null, + "consumes_quota": true + } + ] +} diff --git a/api-ref/source/v3/volumes-v3-volumes.inc b/api-ref/source/v3/volumes-v3-volumes.inc index 86777887e3c..f439577fd58 100644 --- a/api-ref/source/v3/volumes-v3-volumes.inc +++ b/api-ref/source/v3/volumes-v3-volumes.inc @@ -140,6 +140,7 @@ Response Parameters - provider_id: provider_id - service_uuid: service_uuid - shared_targets: shared_targets + - shared_targets: shared_targets_tristate - cluster_name: cluster_name - consumes_quota: consumes_quota - count: count @@ -267,6 +268,7 @@ Response Parameters - provider_id: provider_id - service_uuid: service_uuid - shared_targets: shared_targets + - shared_targets: shared_targets_tristate - cluster_name: cluster_name - consumes_quota: consumes_quota @@ -403,6 +405,7 @@ Response Parameters - volume_type_id: volume_type_id_363 - service_uuid: service_uuid - shared_targets: shared_targets + - shared_targets: shared_targets_tristate - cluster_name: cluster_name - provider_id: provider_id - group_id: group_id_optional @@ -485,6 +488,7 @@ Response Parameters - provider_id: provider_id - service_uuid: service_uuid - shared_targets: shared_targets + - shared_targets: shared_targets_tristate - cluster_name: cluster_name - consumes_quota: consumes_quota diff --git a/cinder/api/microversions.py b/cinder/api/microversions.py index c5900e60cac..425af5d24aa 100644 --- a/cinder/api/microversions.py +++ b/cinder/api/microversions.py @@ -175,6 +175,8 @@ PROJECT_ID_OPTIONAL_IN_URL = '3.67' SUPPORT_REIMAGE_VOLUME = '3.68' +SHARED_TARGETS_TRISTATE = '3.69' + def get_mv_header(version): """Gets a formatted HTTP microversion header. diff --git a/cinder/api/openstack/api_version_request.py b/cinder/api/openstack/api_version_request.py index 65d9697efdf..b9d7864b8eb 100644 --- a/cinder/api/openstack/api_version_request.py +++ b/cinder/api/openstack/api_version_request.py @@ -154,14 +154,15 @@ REST_API_VERSION_HISTORY = """ * 3.66 - Allow snapshotting in-use volumes without force flag. * 3.67 - API URLs no longer need to include a project_id parameter. * 3.68 - Support re-image volume + * 3.69 - Allow null value for shared_targets """ # The minimum and maximum versions of the API supported # The default api version request is defined to be the # minimum version of the API supported. _MIN_API_VERSION = "3.0" -_MAX_API_VERSION = "3.68" -UPDATED = "2021-11-02T00:00:00Z" +_MAX_API_VERSION = "3.69" +UPDATED = "2022-04-20T00:00:00Z" # NOTE(cyeoh): min and max versions declared as functions so we can diff --git a/cinder/api/openstack/rest_api_version_history.rst b/cinder/api/openstack/rest_api_version_history.rst index 9e0da20fdb0..d2c97459875 100644 --- a/cinder/api/openstack/rest_api_version_history.rst +++ b/cinder/api/openstack/rest_api_version_history.rst @@ -518,3 +518,13 @@ not be specified in the API path. ---------------------- Support ability to re-image a volume with a specific image. Specify the ``os-reimage`` action in the request body. + +3.69 +---- +Volume field ``shared_targets`` is a tristate boolean value now, with the +following meanings: + +- ``true``: Do os-brick locking when host iSCSI initiator doesn't support + manual scans. +- ``false``: Never do locking. +- ``null``: Forced locking regardless of the iSCSI initiator. diff --git a/cinder/api/v3/views/volumes.py b/cinder/api/v3/views/volumes.py index ded603a4fc2..a76ac1e120b 100644 --- a/cinder/api/v3/views/volumes.py +++ b/cinder/api/v3/views/volumes.py @@ -57,8 +57,15 @@ class ViewBuilder(views_v2.ViewBuilder): if req_version.matches( mv.VOLUME_SHARED_TARGETS_AND_SERVICE_FIELDS, None): - volume_ref['volume']['shared_targets'] = volume.get( - 'shared_targets', None) + + # For microversion 3.69 or higher it is acceptable to be null + # but for earlier versions we convert None to True + shared = volume.get('shared_targets', False) + if (not req_version.matches(mv.SHARED_TARGETS_TRISTATE, None) + and shared is None): + shared = True + + volume_ref['volume']['shared_targets'] = shared volume_ref['volume']['service_uuid'] = volume.get( 'service_uuid', None) diff --git a/cinder/db/migrations/versions/c92a3e68beed_make_shared_targets_nullable.py b/cinder/db/migrations/versions/c92a3e68beed_make_shared_targets_nullable.py new file mode 100644 index 00000000000..a6e2875aa10 --- /dev/null +++ b/cinder/db/migrations/versions/c92a3e68beed_make_shared_targets_nullable.py @@ -0,0 +1,50 @@ +# 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. + +"""Make shared_targets nullable + +Revision ID: c92a3e68beed +Revises: 921e1a36b076 +Create Date: 2022-03-23 21:30:18.585830 +""" + +from alembic import op +import sqlalchemy as sa + + +# revision identifiers, used by Alembic. +revision = 'c92a3e68beed' +down_revision = '921e1a36b076' +branch_labels = None +depends_on = None + + +def upgrade(): + connection = op.get_bind() + + # Preserve existing type, be it boolean or tinyint treated as boolean + table = sa.Table('volumes', sa.MetaData(), autoload_with=connection) + existing_type = table.c.shared_targets.type + + # SQLite doesn't support altering tables, so we use a workaround + if connection.engine.name == 'sqlite': + with op.batch_alter_table('volumes') as batch_op: + batch_op.alter_column('shared_targets', + existing_type=existing_type, + type_=sa.Boolean(), + nullable=True) + + else: + op.alter_column('volumes', 'shared_targets', + existing_type=existing_type, + type_=sa.Boolean(), + nullable=True) diff --git a/cinder/db/sqlalchemy/models.py b/cinder/db/sqlalchemy/models.py index d5743a813ff..e7ec157ce38 100644 --- a/cinder/db/sqlalchemy/models.py +++ b/cinder/db/sqlalchemy/models.py @@ -402,8 +402,11 @@ class Volume(BASE, CinderBase): foreign_keys=service_uuid, primaryjoin='Volume.service_uuid == Service.uuid', ) + # True => Do locking when iSCSI initiator doesn't support manual scan + # False => Never do locking + # None => Forced locking regardless of the iSCSI initiator # make an FK of service? - shared_targets = sa.Column(sa.Boolean, default=True) + shared_targets = sa.Column(sa.Boolean, nullable=True, default=True) class VolumeMetadata(BASE, CinderBase): diff --git a/cinder/tests/unit/attachments/test_attachments_manager.py b/cinder/tests/unit/attachments/test_attachments_manager.py index 887486370db..37e75eded7e 100644 --- a/cinder/tests/unit/attachments/test_attachments_manager.py +++ b/cinder/tests/unit/attachments/test_attachments_manager.py @@ -36,6 +36,8 @@ class AttachmentManagerTestCase(test.TestCase): """Setup test class.""" super(AttachmentManagerTestCase, self).setUp() self.manager = importutils.import_object(CONF.volume_manager) + self.mock_object(self.manager, '_driver_shares_targets', + return_value=False) self.configuration = mock.Mock(conf.Configuration) self.context = context.get_admin_context() self.context.user_id = fake.USER_ID diff --git a/cinder/tests/unit/db/test_migrations.py b/cinder/tests/unit/db/test_migrations.py index 7f862fbde8c..bf9a1038b66 100644 --- a/cinder/tests/unit/db/test_migrations.py +++ b/cinder/tests/unit/db/test_migrations.py @@ -135,6 +135,7 @@ class MigrationsWalk( # Migrations can take a long time, particularly on underpowered CI nodes. # Give them some breathing room. TIMEOUT_SCALING_FACTOR = 4 + BOOL_TYPE = sqlalchemy.types.BOOLEAN def setUp(self): super().setUp() @@ -198,6 +199,20 @@ class MigrationsWalk( ).get_current_head() self.assertEqual(head, migration.db_version()) + def _pre_upgrade_c92a3e68beed(self, connection): + """Test shared_targets is nullable.""" + table = db_utils.get_table(connection, 'volumes') + self._previous_type = type(table.c.shared_targets.type) + + def _check_c92a3e68beed(self, connection): + """Test shared_targets is nullable.""" + table = db_utils.get_table(connection, 'volumes') + self.assertIn('shared_targets', table.c) + # Type hasn't changed + self.assertIsInstance(table.c.shared_targets.type, self._previous_type) + # But it's nullable + self.assertTrue(table.c.shared_targets.nullable) + class TestMigrationsWalkSQLite( MigrationsWalk, diff --git a/cinder/tests/unit/group/test_groups_manager.py b/cinder/tests/unit/group/test_groups_manager.py index 335af4b8da5..d4d6f07a95c 100644 --- a/cinder/tests/unit/group/test_groups_manager.py +++ b/cinder/tests/unit/group/test_groups_manager.py @@ -46,6 +46,8 @@ class GroupManagerTestCase(test.TestCase): def setUp(self): super(GroupManagerTestCase, self).setUp() self.volume = importutils.import_object(CONF.volume_manager) + self.mock_object(self.volume, '_driver_shares_targets', + return_value=False) self.configuration = mock.Mock(conf.Configuration) self.context = context.get_admin_context() self.context.user_id = fake.USER_ID diff --git a/cinder/tests/unit/volume/__init__.py b/cinder/tests/unit/volume/__init__.py index 174e97b9a55..b65234ed38f 100644 --- a/cinder/tests/unit/volume/__init__.py +++ b/cinder/tests/unit/volume/__init__.py @@ -49,6 +49,8 @@ class BaseVolumeTestCase(test.TestCase): self.flags(volumes_dir=vol_tmpdir) self.addCleanup(self._cleanup) self.volume = importutils.import_object(CONF.volume_manager) + self.mock_object(self.volume, '_driver_shares_targets', + return_value=False) self.volume.message_api = mock.Mock() self.configuration = mock.Mock(conf.Configuration) self.context = context.get_admin_context() diff --git a/cinder/tests/unit/volume/test_driver.py b/cinder/tests/unit/volume/test_driver.py index 8afb5428193..388073b19e3 100644 --- a/cinder/tests/unit/volume/test_driver.py +++ b/cinder/tests/unit/volume/test_driver.py @@ -161,6 +161,8 @@ class BaseDriverTestCase(test.TestCase): self.override_config('volumes_dir', vol_tmpdir, conf.SHARED_CONF_GROUP) self.volume = importutils.import_object(CONF.volume_manager) + self.mock_object(self.volume, '_driver_shares_targets', + return_value=False) self.context = context.get_admin_context() self.output = "" self.configuration = conf.Configuration(None) diff --git a/cinder/tests/unit/volume/test_volume.py b/cinder/tests/unit/volume/test_volume.py index 976079216a6..9de2835877c 100644 --- a/cinder/tests/unit/volume/test_volume.py +++ b/cinder/tests/unit/volume/test_volume.py @@ -355,9 +355,11 @@ class VolumeTestCase(base.BaseVolumeTestCase): @mock.patch('cinder.quota.QUOTAS.rollback', new=mock.Mock()) @mock.patch('cinder.quota.QUOTAS.commit') @mock.patch('cinder.quota.QUOTAS.reserve', return_value=['RESERVATION']) - def test_create_delete_volume(self, use_quota, _mock_reserve, commit_mock, + def test_create_delete_volume(self, boolean, _mock_reserve, commit_mock, mock_notify, mock_clean): """Test volume can be created and deleted.""" + mock_shares = self.mock_object(self.volume, '_driver_shares_targets', + return_value=boolean) volume = tests_utils.create_volume( self.context, availability_zone=CONF.storage_availability_zone, @@ -368,6 +370,8 @@ class VolumeTestCase(base.BaseVolumeTestCase): self.volume.create_volume(self.context, volume) + mock_shares.assert_called_once_with() + self.assertIs(boolean, volume.shared_targets) self.assert_notify_called(mock_notify, (['INFO', 'volume.create.start'], ['INFO', 'volume.create.end']), @@ -376,7 +380,7 @@ class VolumeTestCase(base.BaseVolumeTestCase): self.volume.stats['pools']) # Confirm delete_volume handles use_quota field - volume.use_quota = use_quota + volume.use_quota = boolean volume.save() # Need to save to DB because of the refresh call commit_mock.reset_mock() _mock_reserve.reset_mock() @@ -386,7 +390,7 @@ class VolumeTestCase(base.BaseVolumeTestCase): volume_id) self.assertEqual(vol['status'], 'deleted') - if use_quota: + if boolean: expected_capacity = 0 self.assert_notify_called(mock_notify, (['INFO', 'volume.delete.start'], diff --git a/cinder/tests/unit/volume/test_volume_manager.py b/cinder/tests/unit/volume/test_volume_manager.py index 3b751d2cd10..341e7326fea 100644 --- a/cinder/tests/unit/volume/test_volume_manager.py +++ b/cinder/tests/unit/volume/test_volume_manager.py @@ -16,6 +16,9 @@ from unittest import mock +import ddt + +from cinder.common import constants from cinder import exception from cinder.message import message_field from cinder.tests.unit import fake_constants as fake @@ -24,6 +27,7 @@ from cinder.tests.unit import volume as base from cinder.volume import manager as vol_manager +@ddt.ddt class VolumeManagerTestCase(base.BaseVolumeTestCase): @mock.patch('cinder.message.api.API.create') @@ -287,3 +291,54 @@ class VolumeManagerTestCase(base.BaseVolumeTestCase): manager._parse_connection_options(ctxt, vol, conn_info) self.assertIn('cacheable', conn_info['data']) self.assertIs(conn_info['data']['cacheable'], False) + + @ddt.data(*(constants.ISCSI_VARIANTS + constants.NVMEOF_VARIANTS)) + def test__driver_shares_targets_reported_shared(self, protocol): + """Shared targets must be reported for iSCSI and NVMe-oF.""" + manager = vol_manager.VolumeManager() + fake_driver = mock.MagicMock() + fake_driver.capabilities = {'shared_targets': True, + 'storage_protocol': protocol} + manager.driver = fake_driver + + res = manager._driver_shares_targets() + expected = True if protocol in constants.ISCSI_VARIANTS else None + self.assertIs(expected, res) + + @ddt.data(*(constants.ISCSI_VARIANTS + constants.NVMEOF_VARIANTS)) + def test__driver_shares_targets_reported_nonshared(self, protocol): + """Protocol is irrelevant for drivers that don't share targets.""" + manager = vol_manager.VolumeManager() + fake_driver = mock.MagicMock() + fake_driver.capabilities = {'shared_targets': False, + 'storage_protocol': protocol} + manager.driver = fake_driver + + res = manager._driver_shares_targets() + self.assertFalse(res) + + @ddt.data(*(constants.ISCSI_VARIANTS + constants.NVMEOF_VARIANTS)) + def test__driver_shares_targets_not_reported(self, protocol): + """When driver doesn't report, assume it's shared.""" + manager = vol_manager.VolumeManager() + fake_driver = mock.MagicMock() + fake_driver.capabilities = {'storage_protocol': protocol} + manager.driver = fake_driver + + res = manager._driver_shares_targets() + expected = True if protocol in constants.ISCSI_VARIANTS else None + self.assertIs(expected, res) + + @ddt.data({'storage_protocol': 'NFS'}, + {'shared_targets': True, 'storage_protocol': 'NFS'}, + {'storage_protocol': 'ceph'}, + {'shared_targets': True, 'storage_protocol': 'ceph'}) + def test__driver_shares_targets_other_protocols(self, capabilities): + """Sharing is irrelevant for other protocols.""" + manager = vol_manager.VolumeManager() + fake_driver = mock.MagicMock() + fake_driver.capabilities = capabilities + manager.driver = fake_driver + + res = manager._driver_shares_targets() + self.assertFalse(res) diff --git a/cinder/volume/manager.py b/cinder/volume/manager.py index ded7428b0f9..15df78b6b07 100644 --- a/cinder/volume/manager.py +++ b/cinder/volume/manager.py @@ -844,12 +844,8 @@ class VolumeManager(manager.CleanableManager, self._update_allocated_capacity(volume, decrement=True, host=original_host) - # Shared targets is only relevant for iSCSI connections. - # We default to True to be on the safe side. - capabilities = self.driver.capabilities - volume.shared_targets = ( - capabilities.get('storage_protocol') in constants.ISCSI_VARIANTS - and capabilities.get('shared_targets', True)) + # Shared targets is only relevant for some connections. + volume.shared_targets = self._driver_shares_targets() # TODO(geguileo): service_uuid won't be enough on Active/Active # deployments. There can be 2 services handling volumes from the same # backend. @@ -859,6 +855,50 @@ class VolumeManager(manager.CleanableManager, LOG.info("Created volume successfully.", resource=volume) return volume.id + def _driver_shares_targets(self): + """Report if driver shares targets and needs locking on connecing side. + + This is currently only relevant for iSCSI and for NVMe-oF. + + Relevant for iSCSI, because older iSCSI initiators didn't support + disabling automatic scans, allowing race conditions between os-brick + and cinder. + + In the NVMe-oF case it's even worse, because not only scans are always + automatic, but also in most cases devices/namespaces cannot be removed + from the subsystem, and they are automatically removed when unmapped + via an asynchronous message from the storage system. + + By exposing the shared_targets characteristic in the volume nova (and + other consumers) can use os-brick's specific context manager to prevent + these race conditions. + + Can return 3 possible values:: + + True => iSCSI protocol and shared targets + False => iSCSI protocol and not shared targets + None => Force shared targets locking in os-brick ignoring + ISCSI_SUPPORTS_MANUAL_SCAN. Used by NVMe-oF. + + Drivers can return in their capabilities shared_targets set to ``None`` + to force locking on the host side regarless of the protocol + """ + capabilities = self.driver.capabilities + # We default to True to be on the safe side. + shared = capabilities.get('shared_targets', True) + + # If driver says no shared_targets, honor it + if shared is False: + return False + + protocol = capabilities.get('storage_protocol') + + if protocol in constants.NVMEOF_VARIANTS: + return None # True must be changed to None for NVMe-oF drivers + + # Only iSCSI drivers would need to do locking for shared targets + return protocol in constants.ISCSI_VARIANTS + def _check_is_our_resource(self, resource) -> None: if resource.host: res_backend = volume_utils.extract_host(