Make get_provider_by_name public and remove safe_connect
This makes _get_provider_by_name public to expose access for callers outside of SchedulerReportClient. The @safe_connect decorator is removed and the get() is wrapped in a try/except to handle KSA ClientException errors and raise PlacementAPIConnectFailure. The two existing calls for adding/removing providers to/from aggregates are updated to use the public method so they don't need to individually do the safe_connect handling. Change-Id: I504c374d3863a2a956d5c0156a43be2d2a2bc712
This commit is contained in:
Matt Riedemann
parent
18ba6e6e6a
commit
674b70d8c7
|
|
@ -28,6 +28,7 @@ from oslo_middleware import request_id
|
|||
from oslo_utils import excutils
|
||||
from oslo_utils import versionutils
|
||||
import retrying
|
||||
import six
|
||||
|
||||
from nova.compute import provider_tree
|
||||
import nova.conf
|
||||
|
|
@ -2182,8 +2183,7 @@ class SchedulerReportClient(object):
|
|||
# for backward compatibility.
|
||||
pass
|
||||
|
||||
@safe_connect
|
||||
def _get_provider_by_name(self, context, name):
|
||||
def get_provider_by_name(self, context, name):
|
||||
"""Queries the placement API for resource provider information matching
|
||||
a supplied name.
|
||||
|
||||
|
|
@ -2193,9 +2193,17 @@ class SchedulerReportClient(object):
|
|||
provider's UUID and generation
|
||||
:raises: `exception.ResourceProviderNotFound` when no such provider was
|
||||
found
|
||||
:raises: PlacementAPIConnectFailure if there was an issue making the
|
||||
API call to placement.
|
||||
"""
|
||||
resp = self.get("/resource_providers?name=%s" % name,
|
||||
global_request_id=context.global_id)
|
||||
try:
|
||||
resp = self.get("/resource_providers?name=%s" % name,
|
||||
global_request_id=context.global_id)
|
||||
except ks_exc.ClientException as ex:
|
||||
LOG.error('Failed to get resource provider by name: %s. Error: %s',
|
||||
name, six.text_type(ex))
|
||||
raise exception.PlacementAPIConnectFailure()
|
||||
|
||||
if resp.status_code == 200:
|
||||
data = resp.json()
|
||||
records = data['resource_providers']
|
||||
|
|
@ -2244,18 +2252,13 @@ class SchedulerReportClient(object):
|
|||
failure attempting to save the provider aggregates
|
||||
:raises: `exception.ResourceProviderUpdateConflict` if a concurrent
|
||||
update to the provider was detected.
|
||||
:raises: PlacementAPIConnectFailure if there was an issue making an
|
||||
API call to placement.
|
||||
"""
|
||||
if host_name is None and rp_uuid is None:
|
||||
raise ValueError(_("Either host_name or rp_uuid is required"))
|
||||
if rp_uuid is None:
|
||||
rp = self._get_provider_by_name(context, host_name)
|
||||
# NOTE(jaypipes): Unfortunately, due to @safe_connect,
|
||||
# _get_provider_by_name() can return None. If that happens, raise
|
||||
# an error so we can trap for it in the Nova API code and ignore in
|
||||
# Rocky, blow up in Stein.
|
||||
if rp is None:
|
||||
raise exception.PlacementAPIConnectFailure()
|
||||
rp_uuid = rp['uuid']
|
||||
rp_uuid = self.get_provider_by_name(context, host_name)['uuid']
|
||||
|
||||
# Now attempt to add the aggregate to the resource provider. We don't
|
||||
# want to overwrite any other aggregates the provider may be associated
|
||||
|
|
@ -2297,16 +2300,10 @@ class SchedulerReportClient(object):
|
|||
failure attempting to save the provider aggregates
|
||||
:raises: `exception.ResourceProviderUpdateConflict` if a concurrent
|
||||
update to the provider was detected.
|
||||
:raises: PlacementAPIConnectFailure if there was an issue making an
|
||||
API call to placement.
|
||||
"""
|
||||
rp = self._get_provider_by_name(context, host_name)
|
||||
# NOTE(jaypipes): Unfortunately, due to @safe_connect,
|
||||
# _get_provider_by_name() can return None. If that happens, raise an
|
||||
# error so we can trap for it in the Nova API code and ignore in Rocky,
|
||||
# blow up in Stein.
|
||||
if rp is None:
|
||||
raise exception.PlacementAPIConnectFailure()
|
||||
rp_uuid = rp['uuid']
|
||||
|
||||
rp_uuid = self.get_provider_by_name(context, host_name)['uuid']
|
||||
# Now attempt to remove the aggregate from the resource provider. We
|
||||
# don't want to overwrite any other aggregates the provider may be
|
||||
# associated with, however, so we first grab the list of aggregates for
|
||||
|
|
|
|||
|
|
@ -3792,7 +3792,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
}
|
||||
self.mock_get.return_value = get_resp
|
||||
name = 'cn1'
|
||||
res = self.client._get_provider_by_name(self.context, name)
|
||||
res = self.client.get_provider_by_name(self.context, name)
|
||||
|
||||
exp_url = "/resource_providers?name=%s" % name
|
||||
self.mock_get.assert_called_once_with(
|
||||
|
|
@ -3817,7 +3817,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
name = 'cn1'
|
||||
self.assertRaises(
|
||||
exception.ResourceProviderNotFound,
|
||||
self.client._get_provider_by_name, self.context, name)
|
||||
self.client.get_provider_by_name, self.context, name)
|
||||
mock_log.assert_called_once()
|
||||
|
||||
@mock.patch.object(report.LOG, 'warning')
|
||||
|
|
@ -3828,7 +3828,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
name = 'cn1'
|
||||
self.assertRaises(
|
||||
exception.ResourceProviderNotFound,
|
||||
self.client._get_provider_by_name, self.context, name)
|
||||
self.client.get_provider_by_name, self.context, name)
|
||||
mock_log.assert_called_once()
|
||||
|
||||
@mock.patch.object(report.LOG, 'warning')
|
||||
|
|
@ -3839,7 +3839,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
name = 'cn1'
|
||||
self.assertRaises(
|
||||
exception.ResourceProviderNotFound,
|
||||
self.client._get_provider_by_name, self.context, name)
|
||||
self.client.get_provider_by_name, self.context, name)
|
||||
mock_log.assert_not_called()
|
||||
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
|
|
@ -3847,7 +3847,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_add_host_success_no_existing(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -3868,7 +3868,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name', new=mock.NonCallableMock())
|
||||
'get_provider_by_name', new=mock.NonCallableMock())
|
||||
def test_aggregate_add_host_rp_uuid(self, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_aggs.return_value = report.AggInfo(
|
||||
aggregates=set([]), generation=42)
|
||||
|
|
@ -3883,7 +3883,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_add_host_success_already_existing(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -3908,14 +3908,12 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
use_cache=False, generation=43)
|
||||
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name',
|
||||
side_effect=exception.PlacementAPIConnectFailure)
|
||||
def test_aggregate_add_host_no_placement(self, mock_get_by_name):
|
||||
"""In Rocky, we allow nova-api to not be able to communicate with
|
||||
placement, so the @safe_connect decorator will return None. Check that
|
||||
an appropriate exception is raised back to the nova-api code in this
|
||||
case.
|
||||
"""Tests that PlacementAPIConnectFailure will be raised up from
|
||||
aggregate_add_host if get_provider_by_name raises that error.
|
||||
"""
|
||||
mock_get_by_name.return_value = None # emulate @safe_connect...
|
||||
name = 'cn1'
|
||||
agg_uuid = uuids.agg1
|
||||
self.assertRaises(
|
||||
|
|
@ -3929,7 +3927,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_add_host_retry_success(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -3957,7 +3955,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_add_host_retry_raises(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -3984,14 +3982,12 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
self.client.aggregate_add_host, self.context, uuids.agg1)
|
||||
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name',
|
||||
side_effect=exception.PlacementAPIConnectFailure)
|
||||
def test_aggregate_remove_host_no_placement(self, mock_get_by_name):
|
||||
"""In Rocky, we allow nova-api to not be able to communicate with
|
||||
placement, so the @safe_connect decorator will return None. Check that
|
||||
an appropriate exception is raised back to the nova-api code in this
|
||||
case.
|
||||
"""Tests that PlacementAPIConnectFailure will be raised up from
|
||||
aggregate_remove_host if get_provider_by_name raises that error.
|
||||
"""
|
||||
mock_get_by_name.return_value = None # emulate @safe_connect...
|
||||
name = 'cn1'
|
||||
agg_uuid = uuids.agg1
|
||||
self.assertRaises(
|
||||
|
|
@ -4004,7 +4000,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_remove_host_success_already_existing(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -4024,7 +4020,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_remove_host_success_no_existing(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -4053,7 +4049,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_remove_host_retry_success(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
@ -4081,7 +4077,7 @@ class TestAggregateAddRemoveHost(SchedulerReportClientTestCase):
|
|||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_aggregates')
|
||||
@mock.patch('nova.scheduler.client.report.SchedulerReportClient.'
|
||||
'_get_provider_by_name')
|
||||
'get_provider_by_name')
|
||||
def test_aggregate_remove_host_retry_raises(
|
||||
self, mock_get_by_name, mock_get_aggs, mock_set_aggs):
|
||||
mock_get_by_name.return_value = {
|
||||
|
|
|
|||
Loading…
Reference in New Issue