From 9a27434ffc71ce29e86b25a6cd81b54dab68d98c Mon Sep 17 00:00:00 2001 From: Sylvain Bauza Date: Wed, 13 Sep 2023 19:23:01 +0200 Subject: [PATCH] Revert "Limit nodes by ironic shard key" This reverts commit f5a12f511b26858131ee79ae5bce38e3f60b156d. Change-Id: I4a329237231ba741b57b2ef6437fcee226915d40 --- nova/conf/ironic.py | 12 --- .../unit/virt/ironic/test_client_wrapper.py | 6 +- nova/tests/unit/virt/ironic/test_driver.py | 102 +----------------- nova/virt/ironic/client_wrapper.py | 2 +- nova/virt/ironic/driver.py | 37 +++---- .../notes/ironic-shards-5641e4b1ab5bb7aa.yaml | 12 --- 6 files changed, 19 insertions(+), 152 deletions(-) delete mode 100644 releasenotes/notes/ironic-shards-5641e4b1ab5bb7aa.yaml diff --git a/nova/conf/ironic.py b/nova/conf/ironic.py index 81562a782aa1..ca1655dcc24b 100644 --- a/nova/conf/ironic.py +++ b/nova/conf/ironic.py @@ -82,18 +82,6 @@ Related options: 'service. Note that setting this to the empty string (``""``) ' 'will match the default conductor group, and is different than ' 'leaving the option unset.'), - cfg.StrOpt( - 'shard', - default=None, - mutable=True, - max_length=255, - regex=r'^[a-zA-Z0-9_.-]*$', - help='Specify which ironic shard this nova-compute will manage. ' - 'This allows you to shard Ironic nodes between compute ' - 'services across conductors and conductor groups. ' - 'When a shard is set, the peer_list configuraton is ignored. ' - 'We require that there is at most one nova-compute service ' - 'for each shard.'), cfg.ListOpt( 'peer_list', deprecated_for_removal=True, diff --git a/nova/tests/unit/virt/ironic/test_client_wrapper.py b/nova/tests/unit/virt/ironic/test_client_wrapper.py index 9de959264afa..512f1438d694 100644 --- a/nova/tests/unit/virt/ironic/test_client_wrapper.py +++ b/nova/tests/unit/virt/ironic/test_client_wrapper.py @@ -86,7 +86,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': ['1.82', '1.38'], + 'os_ironic_api_version': ['1.46', '1.38'], 'endpoint': self.get_ksa_adapter.return_value.get_endpoint.return_value, 'interface': ['internal', 'public']} @@ -111,7 +111,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': ['1.82', '1.38'], + 'os_ironic_api_version': ['1.46', '1.38'], 'endpoint': None, 'region_name': CONF.ironic.region_name, 'interface': ['internal', 'public']} @@ -132,7 +132,7 @@ class IronicClientWrapperTestCase(test.NoDBTestCase): expected = {'session': 'session', 'max_retries': CONF.ironic.api_max_retries, 'retry_interval': CONF.ironic.api_retry_interval, - 'os_ironic_api_version': ['1.82', '1.38'], + 'os_ironic_api_version': ['1.46', '1.38'], 'endpoint': endpoint, 'interface': ['admin']} mock_ir_cli.assert_called_once_with(1, **expected) diff --git a/nova/tests/unit/virt/ironic/test_driver.py b/nova/tests/unit/virt/ironic/test_driver.py index 111416b318d3..90ea38f6da65 100644 --- a/nova/tests/unit/virt/ironic/test_driver.py +++ b/nova/tests/unit/virt/ironic/test_driver.py @@ -3020,31 +3020,6 @@ class HashRingTestCase(test.NoDBTestCase): uncalled=['host3']) mock_can_send.assert_called_once_with(min_version='1.46') - @mock.patch.object(ironic_driver.IronicDriver, '_can_send_version') - def test__refresh_hash_ring_peer_list_shard(self, mock_can_send): - services = ['host1', 'host2', 'host3'] - expected_hosts = {'host1'} - self.mock_is_up.return_value = True - self.flags(host='host1') - self.flags(shard='shard1', group='ironic') - self._test__refresh_hash_ring(services, expected_hosts, - uncalled=['host2', 'host3']) - mock_can_send.assert_not_called() - - @mock.patch.object(ironic_driver.IronicDriver, '_can_send_version') - def test__refresh_hash_ring_peer_list_shard_and_cg(self, mock_can_send): - services = ['host1', 'host2', 'host3'] - expected_hosts = {'host1'} - self.mock_is_up.return_value = True - self.flags(host='host1') - self.flags(shard='shard1', group='ironic') - self.flags(conductor_group='not-none', group='ironic') - # Note that this is getting ignored, because the shard is set - self.flags(peer_list=['host1', 'host2'], group='ironic') - self._test__refresh_hash_ring(services, expected_hosts, - uncalled=['host2', 'host3']) - mock_can_send.assert_not_called() - @mock.patch.object(ironic_driver.IronicDriver, '_can_send_version') def test__refresh_hash_ring_peer_list_old_api(self, mock_can_send): mock_can_send.side_effect = ( @@ -3119,8 +3094,7 @@ class NodeCacheTestCase(test.NoDBTestCase): def _test__refresh_cache(self, instances, nodes, hosts, mock_instances, mock_nodes, mock_hosts, mock_hash_ring, mock_can_send, partition_key=None, - can_send_146=True, shard=None, - can_send_182=True): + can_send_146=True): mock_instances.return_value = instances mock_nodes.return_value = nodes mock_hosts.side_effect = hosts @@ -3130,18 +3104,12 @@ class NodeCacheTestCase(test.NoDBTestCase): if not can_send_146: mock_can_send.side_effect = ( exception.IronicAPIVersionNotAvailable(version='1.46')) - if not can_send_182: - mock_can_send.side_effect = None, ( - exception.IronicAPIVersionNotAvailable(version='1.82')) - self.driver.node_cache = {} self.driver.node_cache_time = None kwargs = {} if partition_key is not None and can_send_146: kwargs['conductor_group'] = partition_key - if shard and can_send_182: - kwargs["shard"] = shard self.driver._refresh_cache() @@ -3219,74 +3187,6 @@ class NodeCacheTestCase(test.NoDBTestCase): expected_cache = {n.uuid: n for n in nodes} self.assertEqual(expected_cache, self.driver.node_cache) - def test__refresh_cache_shard(self): - # normal operation, one compute service - instances = [] - nodes = [ - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - ] - hosts = [self.host, self.host, self.host] - shard = "shard1" - self.flags(shard=shard, group='ironic') - - self._test__refresh_cache(instances, nodes, hosts, - shard=shard) - - expected_cache = {n.uuid: n for n in nodes} - self.assertEqual(expected_cache, self.driver.node_cache) - - def test__refresh_cache_shard_and_conductor_group(self): - # normal operation, one compute service - instances = [] - nodes = [ - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - ] - hosts = [self.host, self.host, self.host] - shard = "shard1" - self.flags(shard=shard, group='ironic') - partition_key = 'some-group' - self.flags(conductor_group=partition_key, group='ironic') - - self._test__refresh_cache(instances, nodes, hosts, - shard=shard, partition_key=partition_key) - - expected_cache = {n.uuid: n for n in nodes} - self.assertEqual(expected_cache, self.driver.node_cache) - - def test__refresh_cache_shard_and_conductor_group_skip_shard(self): - # normal operation, one compute service - instances = [] - nodes = [ - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - _get_cached_node( - uuid=uuidutils.generate_uuid(), instance_uuid=None), - ] - hosts = [self.host, self.host, self.host] - shard = "shard1" - self.flags(shard=shard, group='ironic') - partition_key = 'some-group' - self.flags(conductor_group=partition_key, group='ironic') - - self._test__refresh_cache(instances, nodes, hosts, - shard=shard, partition_key=partition_key, - can_send_182=False) - - expected_cache = {n.uuid: n for n in nodes} - self.assertEqual(expected_cache, self.driver.node_cache) - def test__refresh_cache_partition_key_old_api(self): # normal operation, one compute service instances = [] diff --git a/nova/virt/ironic/client_wrapper.py b/nova/virt/ironic/client_wrapper.py index 29f2bd68c148..64539c583e2b 100644 --- a/nova/virt/ironic/client_wrapper.py +++ b/nova/virt/ironic/client_wrapper.py @@ -32,7 +32,7 @@ ironic = None IRONIC_GROUP = nova.conf.ironic.ironic_group # The API version required by the Ironic driver -IRONIC_API_VERSION = (1, 82) +IRONIC_API_VERSION = (1, 46) # NOTE(TheJulia): This version should ALWAYS be the _last_ release # supported version of the API version used by nova. If a feature # needs a higher version to be negotiated to operate properly, then the version diff --git a/nova/virt/ironic/driver.py b/nova/virt/ironic/driver.py index 7bd4bf6e73f3..48fc34919196 100644 --- a/nova/virt/ironic/driver.py +++ b/nova/virt/ironic/driver.py @@ -702,17 +702,11 @@ class IronicDriver(virt_driver.ComputeDriver): def _refresh_hash_ring(self, ctxt): peer_list = None - if CONF.ironic.shard is not None: - # When requesting a shard, we assume each compute service is - # targeting a separate shard, so hard code peer_list to - # just this service - peer_list = set([CONF.host]) - # NOTE(jroll) if this is set, we need to limit the set of other # compute services in the hash ring to hosts that are currently up # and specified in the peer_list config option, as there's no way # to check which conductor_group other compute services are using. - if peer_list is None and CONF.ironic.conductor_group is not None: + if CONF.ironic.conductor_group is not None: try: # NOTE(jroll) first we need to make sure the Ironic API can # filter by conductor_group. If it cannot, limiting to @@ -775,24 +769,21 @@ class IronicDriver(virt_driver.ComputeDriver): # attribute. If the API isn't new enough to support conductor groups, # we fall back to managing all nodes. If it is new enough, we can # filter it in the API. - # NOTE(johngarbutt) similarly, if shard is set, we also limit the - # nodes that are returned by the shard key conductor_group = CONF.ironic.conductor_group - shard = CONF.ironic.shard - kwargs = {} - try: - if conductor_group is not None: + if conductor_group is not None: + try: self._can_send_version(min_version='1.46') - kwargs['conductor_group'] = conductor_group - if shard is not None: - self._can_send_version(min_version='1.82') - kwargs['shard'] = shard - nodes = _get_node_list(**kwargs) - except exception.IronicAPIVersionNotAvailable: - LOG.error('Required Ironic API version is not ' - 'available to filter nodes by conductor group ' - 'and shard.') - nodes = _get_node_list(**kwargs) + nodes = _get_node_list(conductor_group=conductor_group) + LOG.debug('Limiting manageable ironic nodes to conductor ' + 'group %s', conductor_group) + except exception.IronicAPIVersionNotAvailable: + LOG.error('Required Ironic API version 1.46 is not ' + 'available to filter nodes by conductor group. ' + 'All nodes will be eligible to be managed by ' + 'this compute service.') + nodes = _get_node_list() + else: + nodes = _get_node_list() # NOTE(saga): As _get_node_list() will take a long # time to return in large clusters we need to call it before diff --git a/releasenotes/notes/ironic-shards-5641e4b1ab5bb7aa.yaml b/releasenotes/notes/ironic-shards-5641e4b1ab5bb7aa.yaml deleted file mode 100644 index 5becd46e9a86..000000000000 --- a/releasenotes/notes/ironic-shards-5641e4b1ab5bb7aa.yaml +++ /dev/null @@ -1,12 +0,0 @@ ---- -features: - - | - Ironic nova-compute services can now target a specific shard of ironic - nodes by setting the config ``[ironic]shard``. - This is particularly useful when using active-passive methods to choose - on which physical host your ironic nova-compute process is running, - while ensuring ``[DEFAULT]host`` stays the same for each shard. - You can use this alongside ``[ironic]conductor_group`` to further limit - which ironic nodes are managed by each nova-compute service. - Note that when you use ``[ironic]shard`` the ``[ironic]peer_list`` - is hard coded to a single nova-compute service.