From f0fa7312a024266cf6a51638a9ea772128bc3b89 Mon Sep 17 00:00:00 2001 From: Zane Bitter Date: Fri, 22 Jan 2016 12:10:40 -0500 Subject: [PATCH] Don't calculate attributes for metadata request The RPC API call describe_stack_resource is used by two separate ReST API calls and one CFN API call: Resource.show and Resource.metadata in heat-api and StackController.describe_stack_resource. The metadata call in particular throws away all of the results of the describe_stack_resource output except for the metadata itself. Either this call or the cfn one (depending on configuration) is also used very, very often: it's the one that every instance running os-collect-config polls for information about software deployments (so, to a first approximation, every server in every stack will be calling this once every 30s). Since blueprint detailed-resource-show merged in 6d8a5cb35cec81b23a71e8c6fa692f8e7b4dca1c, every call to the RPC describe_stack_resource API has resulted in Heat fetching the attribute values for every single attribute in the resource from its corresponding OpenStack API (including those calls originating from a request for just the metadata, or from the cfn API which similarly discards the attribute values). This is insanely inefficient (we're making OpenStack API calls to fetch data that we don't even return to the user). It's quite possibly a substantial part of the reason why we've had so much difficulty scaling up Heat to deal with large numbers of servers polling it for config. And it tends to leave a lot of annoying warnings in the log messages, not just of Heat but also the other services it is polling, since polling the metadata generally happens using the heat_stack_user's credentials, not those of the actual owner of the resource - it can't actually find the resource and returns a 404. The fix is to change the default "with_attr" value in the RPC client to False, and only pass None (the previous default) or a list of extra attributes to include in the specific case where the ReST API will actually return the attribute values to the user and not just discard them. Since the server previously treated any falsey value for the parameter (e.g. None, False or []) as equivalent, and since nothing was previously passing False, this change is safe to backport to stable branches. The api and engine can be updated in any order without change in behaviour until a new api is talking to a new server (at which point the bug is fixed). This change also updates the ReST API unit tests to accurately reflect the data returned from the engine. Change-Id: Ifffeaa552d3205ca06a79adda09d35a77099a2bf Closes-Bug: #1507568 --- heat/api/openstack/v1/resources.py | 2 ++ heat/engine/api.py | 5 +++-- heat/rpc/client.py | 2 +- heat/tests/api/cfn/test_api_cfn_v1.py | 4 ++-- heat/tests/api/openstack_v1/test_resources.py | 9 ++++++--- heat/tests/test_engine_api_utils.py | 6 ++++++ 6 files changed, 20 insertions(+), 8 deletions(-) diff --git a/heat/api/openstack/v1/resources.py b/heat/api/openstack/v1/resources.py index c2bd060178..99f04e9e23 100644 --- a/heat/api/openstack/v1/resources.py +++ b/heat/api/openstack/v1/resources.py @@ -116,6 +116,8 @@ class ResourceController(object): whitelist = {'with_attr': 'multi'} params = util.get_allowed_params(req.params, whitelist) + if 'with_attr' not in params: + params['with_attr'] = None res = self.rpc_client.describe_stack_resource(req.context, identity, resource_name, diff --git a/heat/engine/api.py b/heat/engine/api.py index 0b2f7907d9..f25ee2e126 100644 --- a/heat/engine/api.py +++ b/heat/engine/api.py @@ -325,8 +325,9 @@ def format_stack_resource(resource, detail=True, with_props=False, if detail: res[rpc_api.RES_DESCRIPTION] = resource.t.description res[rpc_api.RES_METADATA] = resource.metadata_get() - res[rpc_api.RES_SCHEMA_ATTRIBUTES] = format_resource_attributes( - resource, with_attr) + if with_attr is not False: + res[rpc_api.RES_SCHEMA_ATTRIBUTES] = format_resource_attributes( + resource, with_attr) if with_props: res[rpc_api.RES_SCHEMA_PROPERTIES] = format_resource_properties( diff --git a/heat/rpc/client.py b/heat/rpc/client.py index daabc3dde9..fdc73e5f9e 100644 --- a/heat/rpc/client.py +++ b/heat/rpc/client.py @@ -432,7 +432,7 @@ class EngineClient(object): sort_dir=sort_dir)) def describe_stack_resource(self, ctxt, stack_identity, resource_name, - with_attr=None): + with_attr=False): """Get detailed resource information about a particular resource. :param ctxt: RPC context. diff --git a/heat/tests/api/cfn/test_api_cfn_v1.py b/heat/tests/api/cfn/test_api_cfn_v1.py index d7dba3f6e8..a92658445a 100644 --- a/heat/tests/api/cfn/test_api_cfn_v1.py +++ b/heat/tests/api/cfn/test_api_cfn_v1.py @@ -1326,7 +1326,7 @@ class CfnStackControllerTest(common.HeatTestCase): args = { 'stack_identity': identity, 'resource_name': dummy_req.params.get('LogicalResourceId'), - 'with_attr': None, + 'with_attr': False, } rpc_client.EngineClient.call( dummy_req.context, ('describe_stack_resource', args), version='1.2' @@ -1391,7 +1391,7 @@ class CfnStackControllerTest(common.HeatTestCase): args = { 'stack_identity': identity, 'resource_name': dummy_req.params.get('LogicalResourceId'), - 'with_attr': None, + 'with_attr': False, } rpc_client.EngineClient.call( dummy_req.context, ('describe_stack_resource', args), version='1.2' diff --git a/heat/tests/api/openstack_v1/test_resources.py b/heat/tests/api/openstack_v1/test_resources.py index 82c69cade9..97e741b33d 100644 --- a/heat/tests/api/openstack_v1/test_resources.py +++ b/heat/tests/api/openstack_v1/test_resources.py @@ -291,6 +291,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): u'physical_resource_id': u'a3455d8c-9f88-404d-a85b-5315293e67de', u'resource_type': u'AWS::EC2::Instance', + u'attributes': {u'foo': 'bar'}, u'metadata': {u'ensureRunning': u'true'} } self.m.StubOutWithMock(rpc_client.EngineClient, 'call') @@ -323,6 +324,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): u'physical_resource_id': u'a3455d8c-9f88-404d-a85b-5315293e67de', u'resource_type': u'AWS::EC2::Instance', + u'attributes': {u'foo': 'bar'}, } } @@ -354,6 +356,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): u'physical_resource_id': u'a3455d8c-9f88-404d-a85b-5315293e67de', u'resource_type': u'AWS::EC2::Instance', + u'attributes': {u'foo': 'bar'}, u'metadata': {u'ensureRunning': u'true'}, u'nested_stack_id': dict(nested_stack_identity) } @@ -572,7 +575,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): req.context, ('describe_stack_resource', {'stack_identity': stack_identity, 'resource_name': res_name, - 'with_attr': None}), + 'with_attr': False}), version='1.2' ).AndReturn(engine_resp) self.m.ReplayAll() @@ -603,7 +606,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): req.context, ('describe_stack_resource', {'stack_identity': stack_identity, 'resource_name': res_name, - 'with_attr': None}), + 'with_attr': False}), version='1.2' ).AndRaise(tools.to_remote_error(error)) self.m.ReplayAll() @@ -636,7 +639,7 @@ class ResourceControllerTest(tools.ControllerTest, common.HeatTestCase): req.context, ('describe_stack_resource', {'stack_identity': stack_identity, 'resource_name': res_name, - 'with_attr': None}), + 'with_attr': False}), version='1.2' ).AndRaise(tools.to_remote_error(error)) self.m.ReplayAll() diff --git a/heat/tests/test_engine_api_utils.py b/heat/tests/test_engine_api_utils.py index 4c952d2540..ea96d8a695 100644 --- a/heat/tests/test_engine_api_utils.py +++ b/heat/tests/test_engine_api_utils.py @@ -98,6 +98,12 @@ class FormatTest(common.HeatTestCase): formatted[rpc_api.RES_UPDATED_TIME]) self.assertEqual(res.INIT, formatted[rpc_api.RES_ACTION]) + def test_format_stack_resource_no_attrs(self): + res = self.stack['generic1'] + formatted = api.format_stack_resource(res, True, with_attr=False) + self.assertNotIn(rpc_api.RES_SCHEMA_ATTRIBUTES, formatted) + self.assertIn(rpc_api.RES_METADATA, formatted) + def test_format_stack_resource_has_been_deleted(self): # assume the stack and resource have been deleted, # to test the resource's action inherit from stack