From c4909c606f3e5c51afb2370561cf00c871e350e2 Mon Sep 17 00:00:00 2001 From: Rushil Chugh Date: Wed, 22 Apr 2015 18:40:18 -0400 Subject: [PATCH] Avoid LUN ID collisions in NetApp iSCSI drivers Previously, the logic to calculate the LUN ID only considered a single attached host. Because multiple hosts can be attached to a LUN, the logic needs to consider multiple hosts. This patchset proposes to choose non-conflicting LUN IDs across the whole array. This patch first checks for an empty LUN ID across the array. If it doesn't find one, then it looks for the least used LUN ID across the whole array. The value of MAX_LUNS_PER_HOST has been changed here as the range function iterates from 0 to n-1 LUN IDs. With Eseries backend, a maximum of 256 LUN_IDS are supported, thus a value of 256 MAX_LUNS_PER_HOST is required. Closes-bug: 1433825 Closes-bug: 1457995 Change-Id: I033368c12c90d7fbbaf81b065f1a1c8c5975006a --- .../volume/drivers/netapp/eseries/fakes.py | 27 ++++ .../netapp/eseries/test_host_mapper.py | 142 ++++++++++++++++-- .../drivers/netapp/eseries/host_mapper.py | 87 ++++++++--- cinder/volume/drivers/netapp/eseries/iscsi.py | 8 +- cinder/volume/drivers/netapp/eseries/utils.py | 2 +- 5 files changed, 229 insertions(+), 37 deletions(-) diff --git a/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py b/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py index 6217cb2c2..37c752bdb 100644 --- a/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py +++ b/cinder/tests/unit/volume/drivers/netapp/eseries/fakes.py @@ -78,6 +78,7 @@ VOLUME = { INITIATOR_NAME = 'iqn.1998-01.com.vmware:localhost-28a58148' INITIATOR_NAME_2 = 'iqn.1998-01.com.vmware:localhost-28a58149' +INITIATOR_NAME_3 = 'iqn.1998-01.com.vmware:localhost-28a58150' HOST = { 'isSAControlled': False, @@ -108,6 +109,22 @@ HOST_2 = { 'label': 'NewStore', 'type': 'iscsi', 'address': INITIATOR_NAME_2}] } +# HOST_3 has all lun_ids in use. +HOST_3 = { + 'isSAControlled': False, + 'confirmLUNMappingCreation': False, + 'label': 'stlrx300s7-55', + 'isLargeBlockFormatHost': False, + 'clusterRef': '8500000060080E500023C73400360351515B78FC', + 'protectionInformationCapableAccessMethod': False, + 'ports': [], + 'hostRef': '8400000060080E501023C73400800381515BFBA5', + 'hostTypeIndex': 6, + 'hostSidePorts': [{ + 'label': 'NewStore', 'type': 'iscsi', + 'address': INITIATOR_NAME_3}], +} + VOLUME_MAPPING = { 'lunMappingRef': '8800000000000000000000000000000000000000', @@ -118,6 +135,16 @@ VOLUME_MAPPING = { 'type': 'all', 'mapRef': HOST['hostRef'] } +# VOLUME_MAPPING_3 corresponding to HOST_3 has all lun_ids in use. +VOLUME_MAPPING_3 = { + 'lunMappingRef': '8800000000000000000000000000000000000000', + 'lun': range(255), + 'ssid': 16384, + 'perms': 15, + 'volumeRef': VOLUME['volumeRef'], + 'type': 'all', + 'mapRef': HOST_3['hostRef'], +} VOLUME_MAPPING_TO_MULTIATTACH_GROUP = copy.deepcopy(VOLUME_MAPPING) VOLUME_MAPPING_TO_MULTIATTACH_GROUP.update( diff --git a/cinder/tests/unit/volume/drivers/netapp/eseries/test_host_mapper.py b/cinder/tests/unit/volume/drivers/netapp/eseries/test_host_mapper.py index d40585837..7561ff589 100644 --- a/cinder/tests/unit/volume/drivers/netapp/eseries/test_host_mapper.py +++ b/cinder/tests/unit/volume/drivers/netapp/eseries/test_host_mapper.py @@ -1,4 +1,4 @@ -# Copyright (c) 2015 Alex Meade. All rights reserved. +# Copyright (c) 2015 Alex Meade. All rights reserved. # All Rights Reserved. # # Licensed under the Apache License, Version 2.0 (the "License"); you may @@ -23,8 +23,8 @@ import six from cinder import exception from cinder import test -from cinder.tests.unit.volume.drivers.netapp.eseries import fakes as \ - eseries_fakes +from cinder.tests.unit.volume.drivers.netapp.eseries \ + import fakes as eseries_fakes from cinder.volume.drivers.netapp.eseries import host_mapper from cinder.volume.drivers.netapp.eseries import utils @@ -41,6 +41,22 @@ def get_fake_volume(): "detached", "status": "available" } +FAKE_MAPPINGS = [{u'lun': 1}] + +FAKE_USED_UP_MAPPINGS = map(lambda n: {u'lun': n}, range(256)) + +FAKE_USED_UP_LUN_ID_DICT = {n: 1 for n in range(256)} + +FAKE_UNUSED_LUN_ID = set([]) + +FAKE_USED_LUN_ID_DICT = ({0: 1, 1: 1}) + +FAKE_USED_LUN_IDS = [1, 2] + +FAKE_SINGLE_USED_LUN_ID = 1 + +FAKE_USED_UP_LUN_IDS = range(256) + class NetAppEseriesHostMapperTestCase(test.TestCase): def setUp(self): @@ -220,7 +236,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): host_mapper.map_volume_to_single_host(self.client, get_fake_volume(), eseries_fakes.VOLUME, eseries_fakes.HOST, - None) + None, + False) self.assertTrue(self.client.create_volume_mapping.called) @@ -234,7 +251,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): get_fake_volume(), eseries_fakes.VOLUME, eseries_fakes.HOST, - eseries_fakes.VOLUME_MAPPING) + eseries_fakes.VOLUME_MAPPING, + False) self.assertFalse(self.client.create_volume_mapping.called) @@ -255,7 +273,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): get_fake_volume(), eseries_fakes.VOLUME, eseries_fakes.HOST, - fake_mapping_to_other_host) + fake_mapping_to_other_host, + False) self.assertTrue(self.client.move_volume_mapping_via_symbol.called) @@ -274,7 +293,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): self.client, fake_volume, eseries_fakes.VOLUME, eseries_fakes.HOST, - fake_mapping_to_other_host) + fake_mapping_to_other_host, + False) self.assertIn('multiattach is disabled', six.text_type(err)) @@ -293,7 +313,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): self.client, fake_volume, eseries_fakes.VOLUME, eseries_fakes.HOST, - fake_mapping_to_other_host) + fake_mapping_to_other_host, + False) self.assertIn('multiattach is disabled', six.text_type(err)) @@ -309,7 +330,8 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): self.client, get_fake_volume(), eseries_fakes.VOLUME, eseries_fakes.HOST, - fake_mapping_to_other_host) + fake_mapping_to_other_host, + False) self.assertIn('multiattach is disabled', six.text_type(err)) @@ -555,3 +577,105 @@ class NetAppEseriesHostMapperTestCase(test.TestCase): fake_volume_mapping) self.assertIn("unsupported host group", six.text_type(err)) + + def test_get_unused_lun_ids(self): + unused_lun_ids = host_mapper._get_unused_lun_ids(FAKE_MAPPINGS) + self.assertEqual(set(range(2, 256)), unused_lun_ids) + + def test_get_unused_lun_id_counter(self): + used_lun_id_count = host_mapper._get_used_lun_id_counter( + FAKE_MAPPINGS) + self.assertEqual(FAKE_USED_LUN_ID_DICT, used_lun_id_count) + + def test_get_unused_lun_ids_used_up_luns(self): + unused_lun_ids = host_mapper._get_unused_lun_ids( + FAKE_USED_UP_MAPPINGS) + self.assertEqual(FAKE_UNUSED_LUN_ID, unused_lun_ids) + + def test_get_lun_id_counter_used_up_luns(self): + used_lun_ids = host_mapper._get_used_lun_id_counter( + FAKE_USED_UP_MAPPINGS) + self.assertEqual(FAKE_USED_UP_LUN_ID_DICT, used_lun_ids) + + def test_host_not_full(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.assertFalse(host_mapper._is_host_full(self.client, fake_host)) + + def test_host_full(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.mock_object(host_mapper, '_get_vol_mapping_for_host_frm_array', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + self.assertTrue(host_mapper._is_host_full(self.client, fake_host)) + + def test_get_free_lun(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + with mock.patch('random.sample') as mock_random: + mock_random.return_value = [3] + lun = host_mapper._get_free_lun(self.client, fake_host, False) + self.assertEqual(3, lun) + + def test_get_free_lun_host_full(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.mock_object(host_mapper, '_is_host_full', + mock.Mock(return_value=True)) + self.assertRaises( + exception.NetAppDriverException, + host_mapper._get_free_lun, + self.client, fake_host, False) + + def test_get_free_lun_no_unused_luns(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.mock_object(self.client, 'get_volume_mappings', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + lun = host_mapper._get_free_lun(self.client, fake_host, False) + self.assertEqual(255, lun) + + def test_get_free_lun_no_unused_luns_host_not_full(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.mock_object(self.client, 'get_volume_mappings', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + self.mock_object(host_mapper, '_is_host_full', + mock.Mock(return_value=False)) + lun = host_mapper._get_free_lun(self.client, fake_host, False) + self.assertEqual(255, lun) + + def test_get_free_lun_no_lun_available(self): + fake_host = copy.deepcopy(eseries_fakes.HOST_3) + self.mock_object(self.client, 'get_volume_mappings', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + self.mock_object(host_mapper, '_get_vol_mapping_for_host_frm_array', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + + self.assertRaises(exception.NetAppDriverException, + host_mapper._get_free_lun, + self.client, fake_host, False) + + def test_get_free_lun_multiattach_enabled_no_unused_ids(self): + fake_host = copy.deepcopy(eseries_fakes.HOST_3) + self.mock_object(self.client, 'get_volume_mappings', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + + self.assertRaises(exception.NetAppDriverException, + host_mapper._get_free_lun, + self.client, fake_host, True) + + def test_get_lun_by_mapping(self): + used_luns = host_mapper._get_used_lun_ids_for_mappings(FAKE_MAPPINGS) + self.assertEqual(set([0, 1]), used_luns) + + def test_get_lun_by_mapping_no_mapping(self): + used_luns = host_mapper._get_used_lun_ids_for_mappings([]) + self.assertEqual(set([0]), used_luns) + + def test_lun_id_available_on_host(self): + fake_host = copy.deepcopy(eseries_fakes.HOST) + self.assertTrue(host_mapper._is_lun_id_available_on_host( + self.client, fake_host, FAKE_UNUSED_LUN_ID)) + + def test_no_lun_id_available_on_host(self): + fake_host = copy.deepcopy(eseries_fakes.HOST_3) + self.mock_object(host_mapper, '_get_vol_mapping_for_host_frm_array', + mock.Mock(return_value=FAKE_USED_UP_MAPPINGS)) + + self.assertFalse(host_mapper._is_lun_id_available_on_host( + self.client, fake_host, FAKE_SINGLE_USED_LUN_ID)) diff --git a/cinder/volume/drivers/netapp/eseries/host_mapper.py b/cinder/volume/drivers/netapp/eseries/host_mapper.py index b6170c30a..8d79efaf8 100644 --- a/cinder/volume/drivers/netapp/eseries/host_mapper.py +++ b/cinder/volume/drivers/netapp/eseries/host_mapper.py @@ -1,4 +1,4 @@ -# Copyright (c) 2015 Alex Meade. All Rights Reserved. +# Copyright (c) 2015 Alex Meade. All Rights Reserved. # # 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 @@ -16,8 +16,10 @@ Groups. """ +import collections +import random + from oslo_log import log as logging -from six.moves import xrange from cinder import exception from cinder.i18n import _ @@ -31,14 +33,13 @@ LOG = logging.getLogger(__name__) @cinder_utils.synchronized('map_es_volume') def map_volume_to_single_host(client, volume, eseries_vol, host, - vol_map): + vol_map, multiattach_enabled): """Maps the e-series volume to host with initiator.""" LOG.debug("Attempting to map volume %s to single host.", volume['id']) # If volume is not mapped on the backend, map directly to host if not vol_map: - mappings = _get_vol_mapping_for_host_frm_array(client, host['hostRef']) - lun = _get_free_lun(client, host, mappings) + lun = _get_free_lun(client, host, multiattach_enabled) return client.create_volume_mapping(eseries_vol['volumeRef'], host['hostRef'], lun) @@ -65,9 +66,7 @@ def map_volume_to_single_host(client, volume, eseries_vol, host, LOG.debug("Volume %(vol)s is not currently attached, moving " "existing mapping to host %(host)s.", {'vol': volume['id'], 'host': host['label']}) - mappings = _get_vol_mapping_for_host_frm_array( - client, host['hostRef']) - lun = _get_free_lun(client, host, mappings) + lun = _get_free_lun(client, host, multiattach_enabled) return client.move_volume_mapping_via_symbol( vol_map.get('mapRef'), host['hostRef'], lun ) @@ -151,20 +150,64 @@ def map_volume_to_multiple_hosts(client, volume, eseries_vol, target_host, return mapping -def _get_free_lun(client, host, maps=None): - """Gets free LUN for given host.""" - ref = host['hostRef'] - luns = maps or _get_vol_mapping_for_host_frm_array(client, ref) - if host['clusterRef'] != utils.NULL_REF: - host_group_maps = _get_vol_mapping_for_host_group_frm_array( - client, host['clusterRef']) - luns.extend(host_group_maps) - used_luns = set(map(lambda lun: int(lun['lun']), luns)) - for lun in xrange(utils.MAX_LUNS_PER_HOST): - if lun not in used_luns: - return lun - msg = _("No free LUNs. Host might have exceeded max number of LUNs.") - raise exception.NetAppDriverException(msg) +def _get_free_lun(client, host, multiattach_enabled): + """Returns least used LUN ID available on the given host.""" + mappings = client.get_volume_mappings() + if not _is_host_full(client, host): + unused_luns = _get_unused_lun_ids(mappings) + if unused_luns: + chosen_lun = random.sample(unused_luns, 1) + return chosen_lun[0] + elif multiattach_enabled: + msg = _("No unused LUN IDs are available on the host; " + "multiattach is enabled which requires that all LUN IDs " + "to be unique across the entire host group.") + raise exception.NetAppDriverException(msg) + used_lun_counts = _get_used_lun_id_counter(mappings) + # most_common returns an arbitrary tuple of members with same frequency + for lun_id, __ in reversed(used_lun_counts.most_common()): + if _is_lun_id_available_on_host(client, host, lun_id): + return lun_id + msg = _("No free LUN IDs left. Maximum number of volumes that can be " + "attached to host (%s) has been exceeded.") + raise exception.NetAppDriverException(msg % utils.MAX_LUNS_PER_HOST) + + +def _get_unused_lun_ids(mappings): + """Returns unused LUN IDs given mappings.""" + used_luns = _get_used_lun_ids_for_mappings(mappings) + + unused_luns = (set(range(utils.MAX_LUNS_PER_HOST)) - set(used_luns)) + return unused_luns + + +def _get_used_lun_id_counter(mapping): + """Returns used LUN IDs with count as a dictionary.""" + used_luns = _get_used_lun_ids_for_mappings(mapping) + used_lun_id_counter = collections.Counter(used_luns) + return used_lun_id_counter + + +def _is_host_full(client, host): + """Checks whether maximum volumes attached to a host have been reached.""" + luns = _get_vol_mapping_for_host_frm_array(client, host['hostRef']) + return len(luns) >= utils.MAX_LUNS_PER_HOST + + +def _is_lun_id_available_on_host(client, host, lun_id): + """Returns a boolean value depending on whether a LUN ID is available.""" + mapping = _get_vol_mapping_for_host_frm_array(client, host['hostRef']) + used_lun_ids = _get_used_lun_ids_for_mappings(mapping) + return lun_id not in used_lun_ids + + +def _get_used_lun_ids_for_mappings(mappings): + """Returns used LUNs when provided with mappings.""" + used_luns = set(map(lambda lun: int(lun['lun']), mappings)) + # E-Series uses LUN ID 0 for special purposes and should not be + # assigned for general use + used_luns.add(0) + return used_luns def _get_vol_mapping_for_host_frm_array(client, host_ref): diff --git a/cinder/volume/drivers/netapp/eseries/iscsi.py b/cinder/volume/drivers/netapp/eseries/iscsi.py index 1937b1c8e..5ebb4c70f 100644 --- a/cinder/volume/drivers/netapp/eseries/iscsi.py +++ b/cinder/volume/drivers/netapp/eseries/iscsi.py @@ -614,11 +614,9 @@ class NetAppEseriesISCSIDriver(driver.ISCSIDriver): host, current_map) else: - mapping = host_mapper.map_volume_to_single_host(self._client, - volume, - eseries_vol, - host, - current_map) + mapping = host_mapper.map_volume_to_single_host( + self._client, volume, eseries_vol, host, current_map, + self.configuration.netapp_enable_multiattach) lun_id = mapping['lun'] msg_fmt = {'id': volume['id'], 'initiator_name': initiator_name} diff --git a/cinder/volume/drivers/netapp/eseries/utils.py b/cinder/volume/drivers/netapp/eseries/utils.py index 6d89444a5..96aa1f40d 100644 --- a/cinder/volume/drivers/netapp/eseries/utils.py +++ b/cinder/volume/drivers/netapp/eseries/utils.py @@ -28,7 +28,7 @@ LOG = logging.getLogger(__name__) MULTI_ATTACH_HOST_GROUP_NAME = 'cinder-multi-attach' NULL_REF = '0000000000000000000000000000000000000000' -MAX_LUNS_PER_HOST = 255 +MAX_LUNS_PER_HOST = 256 MAX_LUNS_PER_HOST_GROUP = 256 -- 2.45.2