From 8e9a347c0e1d5821e43bacbd88ffd239c4274163 Mon Sep 17 00:00:00 2001 From: Gorka Eguileor Date: Tue, 8 Mar 2022 17:18:48 +0100 Subject: [PATCH] nvmeof: Support new connection properties The os-brick nvmeof connector supports 2 different connection properties formats, the original one and a later one that added support to multiple portals as well as replicated devices for a software RAID. Targets that inherit from the nvmeof base target (spdk and nvmet) supported the original connection properties format, but it was decided that new features, such as multipathing, would only be added to os-brick in the new format code path. This patch adds support to the nvmeof target class (and specifically to the nvmet target, though it should work for other as well) for the new connection properties format to enable support for future features. Support for the old connection properties has been maintained, and is still the default, since we need an easy way to exert the old code path in os-brick to ensure that the code still works. Configuration option ``nvmeof_conn_info_version`` is used to select what version of the connection properties the nvmet target should use. With version 1 being the old format and version 2 the new format. Defaults to the old format to preserve existing behavior. Change-Id: If3f7f66a5cd23604cc81a6973304db9f9018fdb3 --- cinder/opts.py | 2 + .../tests/unit/targets/test_nvmeof_driver.py | 41 +++++-- .../tests/unit/targets/test_nvmet_driver.py | 107 ++++++++---------- cinder/tests/unit/volume/drivers/test_spdk.py | 1 + cinder/volume/driver.py | 13 +++ cinder/volume/drivers/lvm.py | 2 +- cinder/volume/targets/nvmeof.py | 60 ++++++++-- cinder/volume/targets/nvmet.py | 39 +++---- ...nvmet-new-conn_props-25320e34d6ca6ac7.yaml | 6 + 9 files changed, 175 insertions(+), 96 deletions(-) create mode 100644 releasenotes/notes/lvm-nvmet-new-conn_props-25320e34d6ca6ac7.yaml diff --git a/cinder/opts.py b/cinder/opts.py index 0cc5df5c65b..49fb4fce384 100644 --- a/cinder/opts.py +++ b/cinder/opts.py @@ -279,6 +279,7 @@ def list_opts(): [cinder_volume_api.az_cache_time_opt], cinder_volume_driver.volume_opts, cinder_volume_driver.iser_opts, + cinder_volume_driver.nvmeof_opts, cinder_volume_driver.nvmet_opts, cinder_volume_driver.scst_opts, cinder_volume_driver.backup_opts, @@ -324,6 +325,7 @@ def list_opts(): itertools.chain( cinder_volume_driver.volume_opts, cinder_volume_driver.iser_opts, + cinder_volume_driver.nvmeof_opts, cinder_volume_driver.nvmet_opts, cinder_volume_driver.scst_opts, cinder_volume_driver.image_opts, diff --git a/cinder/tests/unit/targets/test_nvmeof_driver.py b/cinder/tests/unit/targets/test_nvmeof_driver.py index 62d707c9408..277b75c3cd8 100644 --- a/cinder/tests/unit/targets/test_nvmeof_driver.py +++ b/cinder/tests/unit/targets/test_nvmeof_driver.py @@ -12,6 +12,7 @@ from unittest import mock +import ddt from oslo_utils import timeutils from cinder import context @@ -25,15 +26,11 @@ class FakeNVMeOFDriver(nvmeof.NVMeOF): def __init__(self, *args, **kwargs): super(FakeNVMeOFDriver, self).__init__(*args, **kwargs) - def create_nvmeof_target( - self, target_name, target_ip, target_port, - transport_type, ns_id, volume_path): - pass - def delete_nvmeof_target(self, target_name): pass +@ddt.ddt class TestNVMeOFDriver(tf.TargetDriverFixture): def setUp(self): @@ -75,16 +72,18 @@ class TestNVMeOFDriver(tf.TargetDriverFixture): 'created_at': timeutils.utcnow(), 'host': 'fake_host@lvm#lvm'}) - def test_initialize_connection(self): + @mock.patch.object(nvmeof.NVMeOF, '_get_connection_properties') + def test_initialize_connection(self, mock_get_conn): mock_connector = {'initiator': 'fake_init'} mock_testvol = self.testvol expected_return = { 'driver_volume_type': 'nvmeof', - 'data': self.target._get_connection_properties(mock_testvol) + 'data': mock_get_conn.return_value } self.assertEqual(expected_return, self.target.initialize_connection(mock_testvol, mock_connector)) + mock_get_conn.assert_called_once_with(mock_testvol) @mock.patch.object(FakeNVMeOFDriver, 'create_nvmeof_target') def test_create_export(self, mock_create_nvme_target): @@ -109,7 +108,8 @@ class TestNVMeOFDriver(tf.TargetDriverFixture): self.testvol ) - def test_get_connection_properties(self): + def test__get_connection_properties_old(self): + """Test connection properties with the old NVMe-oF format.""" expected_return = { 'target_portal': self.target_ip, 'target_port': str(self.target_port), @@ -121,6 +121,31 @@ class TestNVMeOFDriver(tf.TargetDriverFixture): self.assertEqual(expected_return, self.target._get_connection_properties(self.testvol)) + @ddt.data(('rdma', 'RoCEv2'), ('tcp', 'tcp')) + @ddt.unpack + @mock.patch.object(nvmeof.NVMeOF, '_get_nvme_uuid') + def test__get_connection_properties_new(self, transport, + expected_transport, mock_uuid): + """Test connection properties with the new NVMe-oF format.""" + nqn = f'ngn.{self.nvmet_subsystem_name}-{self.fake_volume_id}' + vol = self.testvol.copy() + vol['provider_location'] = self.target.get_nvmeof_location( + nqn, self.target_ip, self.target_port, transport, self.nvmet_ns_id) + + self.configuration.nvmeof_conn_info_version = 2 + + expected_return = { + 'target_nqn': nqn, + 'vol_uuid': mock_uuid.return_value, + 'ns_id': str(self.nvmet_ns_id), + 'portals': [(self.target_ip, + str(self.target_port), + expected_transport)], + } + self.assertEqual(expected_return, + self.target._get_connection_properties(vol)) + mock_uuid.assert_called_once_with(vol) + def test_validate_connector(self): mock_connector = {'initiator': 'fake_init'} self.assertTrue(self.target.validate_connector(mock_connector)) diff --git a/cinder/tests/unit/targets/test_nvmet_driver.py b/cinder/tests/unit/targets/test_nvmet_driver.py index 26241653e08..8884ff4fd84 100644 --- a/cinder/tests/unit/targets/test_nvmet_driver.py +++ b/cinder/tests/unit/targets/test_nvmet_driver.py @@ -34,96 +34,79 @@ class TestNVMETDriver(tf.TargetDriverFixture): configuration=self.configuration) fake_nvmet_lib.reset_mock() - @mock.patch.object(nvmet.NVMET, 'create_nvmeof_target') - def test_create_export(self, mock_create_target): - """Test that the nvmeof class calls the nvmet method.""" - res = self.target.create_export(mock.sentinel.ctxt, - self.testvol, - mock.sentinel.volume_path) - self.assertEqual(mock_create_target.return_value, res) - mock_create_target.assert_called_once_with( - self.testvol['id'], - self.target.configuration.target_prefix, - self.target.target_ip, - self.target.target_port, - self.target.nvme_transport_type, - self.target.nvmet_port_id, - self.target.nvmet_ns_id, - mock.sentinel.volume_path) - + @mock.patch.object(nvmet.NVMET, '_get_nvme_uuid') @mock.patch.object(nvmet.NVMET, 'get_nvmeof_location') @mock.patch.object(nvmet.NVMET, '_ensure_port_exports') @mock.patch.object(nvmet.NVMET, '_ensure_subsystem_exists') @mock.patch.object(nvmet.NVMET, '_get_target_nqn') - def test_create_nvmeof_target(self, mock_nqn, mock_subsys, mock_port, - mock_location): + def test_create_export(self, mock_nqn, mock_subsys, mock_port, + mock_location, mock_uuid): """Normal create target execution.""" mock_nqn.return_value = mock.sentinel.nqn + mock_uuid.return_value = mock.sentinel.uuid + vol = mock.Mock() - res = self.target.create_nvmeof_target(mock.sentinel.vol_id, - mock.sentinel.target_prefix, - mock.sentinel.target_ip, - mock.sentinel.target_port, - mock.sentinel.transport_type, - mock.sentinel.port_id, - mock.sentinel.ns_id, - mock.sentinel.volume_path) + res = self.target.create_export(mock.sentinel.context, + vol, + mock.sentinel.volume_path) self.assertEqual({'location': mock_location.return_value, 'auth': ''}, res) - mock_nqn.assert_called_once_with(mock.sentinel.vol_id) + mock_nqn.assert_called_once_with(vol.id) + mock_uuid.assert_called_once_with(vol) mock_subsys.assert_called_once_with(mock.sentinel.nqn, - mock.sentinel.ns_id, - mock.sentinel.volume_path) + self.target.nvmet_ns_id, + mock.sentinel.volume_path, + mock.sentinel.uuid) mock_port.assert_called_once_with(mock.sentinel.nqn, - mock.sentinel.target_ip, - mock.sentinel.target_port, - mock.sentinel.transport_type, - mock.sentinel.port_id) + self.target.target_ip, + self.target.target_port, + self.target.nvme_transport_type, + self.target.nvmet_port_id) mock_location.assert_called_once_with(mock.sentinel.nqn, - mock.sentinel.target_ip, - mock.sentinel.target_port, - mock.sentinel.transport_type, - mock.sentinel.ns_id) + self.target.target_ip, + self.target.target_port, + self.target.nvme_transport_type, + self.target.nvmet_ns_id) @ddt.data((ValueError, None), (None, IndexError)) @ddt.unpack + @mock.patch.object(nvmet.NVMET, '_get_nvme_uuid') @mock.patch.object(nvmet.NVMET, 'get_nvmeof_location') @mock.patch.object(nvmet.NVMET, '_ensure_port_exports') @mock.patch.object(nvmet.NVMET, '_ensure_subsystem_exists') @mock.patch.object(nvmet.NVMET, '_get_target_nqn') - def test_create_nvmeof_target_error(self, subsys_effect, port_effect, - mock_nqn, mock_subsys, mock_port, - mock_location): + def test_create_export_error(self, subsys_effect, port_effect, + mock_nqn, mock_subsys, mock_port, + mock_location, mock_uuid): """Failing create target executing subsystem or port creation.""" mock_subsys.side_effect = subsys_effect mock_port.side_effect = port_effect mock_nqn.return_value = mock.sentinel.nqn + mock_uuid.return_value = mock.sentinel.uuid + vol = mock.Mock() self.assertRaises(nvmet.NVMETTargetAddError, - self.target.create_nvmeof_target, - mock.sentinel.vol_id, - mock.sentinel.target_prefix, - mock.sentinel.target_ip, - mock.sentinel.target_port, - mock.sentinel.transport_type, - mock.sentinel.port_id, - mock.sentinel.ns_id, + self.target.create_export, + mock.sentinel.context, + vol, mock.sentinel.volume_path) - mock_nqn.assert_called_once_with(mock.sentinel.vol_id) + mock_nqn.assert_called_once_with(vol.id) + mock_uuid.assert_called_once_with(vol) mock_subsys.assert_called_once_with(mock.sentinel.nqn, - mock.sentinel.ns_id, - mock.sentinel.volume_path) + self.target.nvmet_ns_id, + mock.sentinel.volume_path, + mock.sentinel.uuid) if subsys_effect: mock_port.assert_not_called() else: mock_port.assert_called_once_with(mock.sentinel.nqn, - mock.sentinel.target_ip, - mock.sentinel.target_port, - mock.sentinel.transport_type, - mock.sentinel.port_id) + self.target.target_ip, + self.target.target_port, + self.target.nvme_transport_type, + self.target.nvmet_port_id) mock_location.assert_not_called() @mock.patch.object(priv_nvmet, 'Subsystem') @@ -131,7 +114,8 @@ class TestNVMETDriver(tf.TargetDriverFixture): """Skip subsystem creation if already exists.""" nqn = 'nqn.nvme-subsystem-1-uuid' self.target._ensure_subsystem_exists(nqn, mock.sentinel.ns_id, - mock.sentinel.vol_path) + mock.sentinel.vol_path, + mock.sentinel.uuid) mock_subsys.assert_called_once_with(nqn) mock_subsys.setup.assert_not_called() @@ -143,12 +127,14 @@ class TestNVMETDriver(tf.TargetDriverFixture): mock_uuid.return_value = 'uuid' nqn = 'nqn.nvme-subsystem-1-uuid' self.target._ensure_subsystem_exists(nqn, mock.sentinel.ns_id, - mock.sentinel.vol_path) + mock.sentinel.vol_path, + mock.sentinel.uuid) mock_subsys.assert_called_once_with(nqn) expected_section = { 'allowed_hosts': [], 'attr': {'allow_any_host': '1'}, 'namespaces': [{'device': {'nguid': 'uuid', + 'uuid': mock.sentinel.uuid, 'path': mock.sentinel.vol_path}, 'enable': 1, 'nsid': mock.sentinel.ns_id}], @@ -265,3 +251,8 @@ class TestNVMETDriver(tf.TargetDriverFixture): def test__get_target_nqn(self): res = self.target._get_target_nqn('volume_id') self.assertEqual('nqn.nvme-subsystem-1-volume_id', res) + + def test__get_nvme_uuid(self): + vol = mock.Mock() + res = self.target._get_nvme_uuid(vol) + self.assertEqual(vol.name_id, res) diff --git a/cinder/tests/unit/volume/drivers/test_spdk.py b/cinder/tests/unit/volume/drivers/test_spdk.py index 5a6fa9df081..2ef5faf9f4d 100644 --- a/cinder/tests/unit/volume/drivers/test_spdk.py +++ b/cinder/tests/unit/volume/drivers/test_spdk.py @@ -504,6 +504,7 @@ class SpdkDriverTestCase(test.TestCase): self.configuration.target_ip_address = "192.168.0.1" self.configuration.target_port = 4420 self.configuration.target_prefix = "nqn.2014-08.io.spdk" + self.configuration.nvmeof_conn_info_version = 1 self.configuration.nvmet_port_id = "1" self.configuration.nvmet_ns_id = "fake_id" self.configuration.nvmet_subsystem_name = "2014-08.io.spdk" diff --git a/cinder/volume/driver.py b/cinder/volume/driver.py index 96c76faed25..0df3126f0ce 100644 --- a/cinder/volume/driver.py +++ b/cinder/volume/driver.py @@ -263,6 +263,16 @@ iser_opts = [ help='The name of the iSER target user-land tool to use'), ] +nvmeof_opts = [ + cfg.IntOpt('nvmeof_conn_info_version', + default=1, + min=1, max=2, + help='NVMe os-brick connector has 2 different connection info ' + 'formats, this allows some NVMe-oF drivers that use the ' + 'original format (version 1), such as spdk and LVM-nvmet, ' + 'to send the newer format.'), +] + nvmet_opts = [ cfg.PortOpt('nvmet_port_id', default=1, @@ -348,11 +358,13 @@ fqdn_opts = [ CONF = cfg.CONF CONF.register_opts(volume_opts, group=configuration.SHARED_CONF_GROUP) CONF.register_opts(iser_opts, group=configuration.SHARED_CONF_GROUP) +CONF.register_opts(nvmeof_opts, group=configuration.SHARED_CONF_GROUP) CONF.register_opts(nvmet_opts, group=configuration.SHARED_CONF_GROUP) CONF.register_opts(scst_opts, group=configuration.SHARED_CONF_GROUP) CONF.register_opts(image_opts, group=configuration.SHARED_CONF_GROUP) CONF.register_opts(volume_opts) CONF.register_opts(iser_opts) +CONF.register_opts(nvmeof_opts) CONF.register_opts(nvmet_opts) CONF.register_opts(scst_opts) CONF.register_opts(backup_opts) @@ -414,6 +426,7 @@ class BaseVD(object, metaclass=abc.ABCMeta): if self.configuration: self.configuration.append_config_values(volume_opts) self.configuration.append_config_values(iser_opts) + self.configuration.append_config_values(nvmeof_opts) self.configuration.append_config_values(nvmet_opts) self.configuration.append_config_values(scst_opts) self.configuration.append_config_values(backup_opts) diff --git a/cinder/volume/drivers/lvm.py b/cinder/volume/drivers/lvm.py index 2e89cd289ee..83ca05e8314 100644 --- a/cinder/volume/drivers/lvm.py +++ b/cinder/volume/drivers/lvm.py @@ -67,7 +67,7 @@ volume_opts = [ cfg.BoolOpt('lvm_suppress_fd_warnings', default=False, help='Suppress leaked file descriptor warnings in LVM ' - 'commands.') + 'commands.'), ] CONF = cfg.CONF diff --git a/cinder/volume/targets/nvmeof.py b/cinder/volume/targets/nvmeof.py index 0992de1c960..9513fa8b488 100644 --- a/cinder/volume/targets/nvmeof.py +++ b/cinder/volume/targets/nvmeof.py @@ -87,28 +87,68 @@ class NVMeOF(driver.Target): def _get_connection_properties(self, volume): """Gets NVMeOF connection configuration. - :return: dictionary of the following keys: - :target_portal: NVMe target IP address - :target_port: NVMe target port - :nqn: NQN of the NVMe target - :transport_type: Network fabric being used for an - NVMe-over-Fabrics network - :ns_id: namespace id associated with the subsystem - """ + For nvmeof_conn_info_version set to 1 (default) the old format will + be sent: + { + 'target_portal': NVMe target IP address + 'target_port': NVMe target port + 'nqn': NQN of the NVMe target + 'transport_type': Network fabric being used for an NVMe-oF network + One of: tcp, rdma + 'ns_id': namespace id associated with the subsystem + } + + For nvmeof_conn_info_version set to 2 the new format will be sent: + { + 'target_nqn': NQN of the NVMe target + 'vol_uuid': NVMe-oF UUID of the volume. May be different than Cinder + volume id and may be None if ns_id is provided. + 'portals': [(target_address, target_port, transport_type) ... ] + 'ns_id': namespace id associated with the subsystem, in case target + doesn't provide the volume_uuid. + } + Unlike the old format the transport_type can be one of RoCEv2 and tcp + + :return: dictionary with the connection properties using one of the 2 + existing formats depending on the nvmeof_conn_info_version + configuration option. + """ location = volume['provider_location'] target_connection, nvme_transport_type, nqn, nvmet_ns_id = ( location.split(' ')) target_portal, target_port = target_connection.split(':') + # NVMe-oF Connection Information Version 2 + if self.configuration.nvmeof_conn_info_version == 2: + uuid = self._get_nvme_uuid(volume) + if nvme_transport_type == 'rdma': + nvme_transport_type = 'RoCEv2' + + return { + 'target_nqn': nqn, + 'vol_uuid': uuid, + 'portals': [(target_portal, target_port, nvme_transport_type)], + 'ns_id': nvmet_ns_id, + } + + # NVMe-oF Connection Information Version 1 return { 'target_portal': target_portal, 'target_port': target_port, 'nqn': nqn, 'transport_type': nvme_transport_type, - 'ns_id': nvmet_ns_id + 'ns_id': nvmet_ns_id, } + def _get_nvme_uuid(self, volume): + """Return the NVMe uuid of a given volume. + + Targets that want to support the nvmeof_conn_info_version=2 option need + to override this method and return the NVMe uuid of the given volume. + """ + return None + def get_nvmeof_location(self, nqn, target_ip, target_port, nvme_transport_type, nvmet_ns_id): """Serializes driver data into single line string.""" @@ -150,7 +190,6 @@ class NVMeOF(driver.Target): missing='initiator') return True - @abc.abstractmethod def create_nvmeof_target(self, volume_id, subsystem_name, @@ -160,6 +199,7 @@ class NVMeOF(driver.Target): nvmet_port_id, ns_id, volume_path): + """Targets that don't override create_export must implement this.""" pass @abc.abstractmethod diff --git a/cinder/volume/targets/nvmet.py b/cinder/volume/targets/nvmet.py index f4e88769c51..63dd046dfad 100644 --- a/cinder/volume/targets/nvmet.py +++ b/cinder/volume/targets/nvmet.py @@ -37,36 +37,33 @@ class NVMET(nvmeof.NVMeOF): self._nvmet_root = nvmet.Root() @utils.synchronized('nvmetcli', external=True) - def create_nvmeof_target(self, - volume_id, - subsystem_name, # Ignoring this, using config - target_ip, - target_port, - transport_type, - nvmet_port_id, - ns_id, - volume_path): + def create_export(self, context, volume, volume_path): # Create NVME subsystem for previously created LV - nqn = self._get_target_nqn(volume_id) + nqn = self._get_target_nqn(volume.id) try: - self._ensure_subsystem_exists(nqn, ns_id, volume_path) - self._ensure_port_exports(nqn, target_ip, target_port, - transport_type, nvmet_port_id) + uuid = self._get_nvme_uuid(volume) + + self._ensure_subsystem_exists(nqn, self.nvmet_ns_id, volume_path, + uuid) + + self._ensure_port_exports(nqn, self.target_ip, self.target_port, + self.nvme_transport_type, + self.nvmet_port_id) except Exception: LOG.error('Failed to add subsystem: %s', nqn) raise NVMETTargetAddError(subsystem=nqn) - LOG.info('Subsystem %s now exported on port %s', nqn, target_port) + LOG.info('Subsystem %s now exported on port %s', nqn, self.target_port) return { 'location': self.get_nvmeof_location( nqn, - target_ip, - target_port, - transport_type, - ns_id), + self.target_ip, + self.target_port, + self.nvme_transport_type, + self.nvmet_ns_id), 'auth': ''} - def _ensure_subsystem_exists(self, nqn, nvmet_ns_id, volume_path): + def _ensure_subsystem_exists(self, nqn, nvmet_ns_id, volume_path, uuid): # Assume if subsystem exists, it has the right configuration try: nvmet.Subsystem(nqn) @@ -84,6 +81,7 @@ class NVMET(nvmeof.NVMeOF): { "device": { "nguid": str(uuidutils.generate_uuid()), + "uuid": uuid, "path": volume_path, }, "enable": 1, @@ -95,6 +93,9 @@ class NVMET(nvmeof.NVMeOF): nvmet.Subsystem.setup(subsystem_section) # privsep LOG.debug('Added subsystem: %s', nqn) + def _get_nvme_uuid(self, volume): + return volume.name_id + def _ensure_port_exports(self, nqn, addr, port, transport_type, port_id): # Assume if port exists, it has the right configuration try: diff --git a/releasenotes/notes/lvm-nvmet-new-conn_props-25320e34d6ca6ac7.yaml b/releasenotes/notes/lvm-nvmet-new-conn_props-25320e34d6ca6ac7.yaml new file mode 100644 index 00000000000..e7004934745 --- /dev/null +++ b/releasenotes/notes/lvm-nvmet-new-conn_props-25320e34d6ca6ac7.yaml @@ -0,0 +1,6 @@ +--- +features: + - | + LVM nvmet target: Added support for new nvmeof connection properties + format (version 2). Controlled with ``nvmeof_conn_info_version`` + configuration option.