Don't load nested stack to get ResourceGroup blacklist

In ResourceGroup, we (unwisely in retrospect) allow blacklisting of
resources by reference ID (i.e. physical resource ID in most cases). In
order to both determine whether a removal policy entry was an existing
resource name and (if not) to find the resource by physical ID, we would
always load the nested stack into memory.

To avoid this, unconditionally create an output in the nested stack that
returns a mapping of all the resource reference IDs and use it for finding
a resource with a matching reference ID. Only if the output is not
available (because the nested stack hasn't been updated since prior to the
output being defined), fall back to the old way of doing it.

Avoid looking up IDs for names that already appear in the blacklist
(because they were blacklisted in a previous update and have been removed
from the stack.)

Since this is more expensive in time (though hopefully cheaper in space),
update the blacklist only once on resource creation and whenever it
actually changes during an update. This is accomplished by moving the
update to a separate function that is called explicitly, rather than making
it a side-effect of getting the current blacklist (which occurs up to 4
times in each create/update).

This also avoids updating the blacklist during a preview, which the
previous code erroneously did.

Change-Id: Ic32d7d99bad06f92b3d2919d58cd1f9128bcfe83
Partial-Bug: #1731349
Closes-Bug: #1749426
(cherry picked from commit 32ec5141de)
This commit is contained in:
Zane Bitter 2018-01-08 17:23:12 -05:00
parent 3e51547f31
commit cbcc15b404
3 changed files with 215 additions and 61 deletions

View File

@ -303,7 +303,7 @@ class ResourceGroup(stack_resource.StackResource):
if not self.get_size():
return
first_name = next(self._resource_names(update_rsrc_data=False))
first_name = next(self._resource_names())
test_tmpl = self._assemble_nested([first_name],
include_all=True)
res_def = next(six.itervalues(test_tmpl.resource_definitions(None)))
@ -330,44 +330,69 @@ class ResourceGroup(stack_resource.StackResource):
else:
return []
def _name_blacklist(self, update_rsrc_data=True):
"""Resolve the remove_policies to names for removal."""
nested = self.nested()
# To avoid reusing names after removal, we store a comma-separated
# blacklist in the resource data - in cases where you want to
# overwrite the stored data, removal_policies_mode: update can be used
current_blacklist = self._current_blacklist()
p_mode = self.properties[self.REMOVAL_POLICIES_MODE]
if p_mode == self.REMOVAL_POLICY_UPDATE:
new_blacklist = []
else:
new_blacklist = current_blacklist
def _get_new_blacklist_entries(self, properties, current_blacklist):
insp = grouputils.GroupInspector.from_parent_resource(self)
# Now we iterate over the removal policies, and update the blacklist
# with any additional names
rsrc_names = set(new_blacklist)
for r in self.properties[self.REMOVAL_POLICIES]:
for r in properties.get(self.REMOVAL_POLICIES, []):
if self.REMOVAL_RSRC_LIST in r:
# Tolerate string or int list values
for n in r[self.REMOVAL_RSRC_LIST]:
str_n = six.text_type(n)
if not nested or str_n in nested:
rsrc_names.add(str_n)
continue
rsrc = nested.resource_by_refid(str_n)
if rsrc:
rsrc_names.add(rsrc.name)
if (str_n in current_blacklist or
self.resource_id is None or
str_n in insp.member_names(include_failed=True)):
yield str_n
elif isinstance(n, six.string_types):
try:
refids = self.get_output(self.REFS_MAP)
except (exception.NotFound,
exception.TemplateOutputError) as op_err:
LOG.debug('Falling back to resource_by_refid() '
' due to %s', op_err)
rsrc = self.nested().resource_by_refid(n)
if rsrc is not None:
yield rsrc.name
else:
if refids is not None:
for name, refid in refids.items():
if refid == n:
yield name
break
# Clear output cache from prior to stack update, so we don't get
# outdated values after stack update.
self._outputs = None
def _update_name_blacklist(self, properties):
"""Resolve the remove_policies to names for removal."""
# To avoid reusing names after removal, we store a comma-separated
# blacklist in the resource data - in cases where you want to
# overwrite the stored data, removal_policies_mode: update can be used
curr_bl = set(self._current_blacklist())
p_mode = properties.get(self.REMOVAL_POLICIES_MODE,
self.REMOVAL_POLICY_APPEND)
if p_mode == self.REMOVAL_POLICY_UPDATE:
init_bl = set()
else:
init_bl = curr_bl
updated_bl = init_bl | set(self._get_new_blacklist_entries(properties,
curr_bl))
# If the blacklist has changed, update the resource data
if update_rsrc_data and rsrc_names != set(current_blacklist):
self.data_set('name_blacklist', ','.join(rsrc_names))
return rsrc_names
if updated_bl != curr_bl:
self.data_set('name_blacklist', ','.join(sorted(updated_bl)))
def _resource_names(self, size=None, update_rsrc_data=True):
name_blacklist = self._name_blacklist(update_rsrc_data)
def _name_blacklist(self):
"""Get the list of resource names to blacklist."""
bl = set(self._current_blacklist())
if self.resource_id is None:
bl |= set(self._get_new_blacklist_entries(self.properties, bl))
return bl
def _resource_names(self, size=None):
name_blacklist = self._name_blacklist()
if size is None:
size = self.get_size()
@ -385,6 +410,7 @@ class ResourceGroup(stack_resource.StackResource):
return len(self._name_blacklist() & set(existing_members))
def handle_create(self):
self._update_name_blacklist(self.properties)
if self.update_policy.get(self.BATCH_CREATE):
batch_create = self.update_policy[self.BATCH_CREATE]
max_batch_size = batch_create[self.MAX_BATCH_SIZE]
@ -443,6 +469,7 @@ class ResourceGroup(stack_resource.StackResource):
checkers = []
self.properties = json_snippet.properties(self.properties_schema,
self.context)
self._update_name_blacklist(self.properties)
if prop_diff and self.res_def_changed(prop_diff):
updaters = self._try_rolling_update()
if updaters:
@ -540,12 +567,13 @@ class ResourceGroup(stack_resource.StackResource):
if key == self.REFS:
value = [get_res_fn(r) for r in resource_names]
elif key == self.REFS_MAP:
value = {r: get_res_fn(r) for r in resource_names}
if value is not None:
yield output.OutputDefinition(output_name, value)
value = {r: get_res_fn(r) for r in resource_names}
yield output.OutputDefinition(self.REFS_MAP, value)
def build_resource_definition(self, res_name, res_defn):
res_def = copy.deepcopy(res_defn)

View File

@ -701,6 +701,9 @@ class SoftwareDeploymentGroup(resource_group.ResourceGroup):
def res_def_changed(self, prop_diff):
return True
def _update_name_blacklist(self, properties):
pass
def _name_blacklist(self):
return set()

View File

@ -174,6 +174,15 @@ class ResourceGroupTest(common.HeatTestCase):
"Foo": "Bar"
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
"2": {"get_resource": "2"},
}
}
}
}
@ -211,6 +220,13 @@ class ResourceGroupTest(common.HeatTestCase):
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
"2": {"get_resource": "2"},
}
},
"foo": {
"value": [
{"get_attr": ["0", "foo"]},
@ -238,6 +254,13 @@ class ResourceGroupTest(common.HeatTestCase):
"type": "OverwrittenFnGetRefIdType",
"properties": {}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
}
}
}
}
self.assertEqual(expect, resg._assemble_nested(['0']).t)
@ -253,6 +276,7 @@ class ResourceGroupTest(common.HeatTestCase):
resg = resource_group.ResourceGroup('test', snip, stack)
expect = {
"heat_template_version": "2015-04-30",
"outputs": {"refs_map": {"value": {}}},
}
self.assertEqual(expect, resg._assemble_nested([]).t)
@ -278,6 +302,13 @@ class ResourceGroupTest(common.HeatTestCase):
'role': 'webserver'
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
}
}
}
}
self.assertEqual(expect, resg._assemble_nested(['0']).t)
@ -298,6 +329,14 @@ class ResourceGroupTest(common.HeatTestCase):
"foo": "baz"
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
}
}
}
}
resource_def = rsrc_defn.ResourceDefinition(
@ -332,6 +371,12 @@ class ResourceGroupTest(common.HeatTestCase):
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
}
},
"bar": {
"value": [
{"get_attr": ["0", "bar"]},
@ -371,6 +416,14 @@ class ResourceGroupTest(common.HeatTestCase):
"foo": "bar"
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
}
}
}
}
@ -404,6 +457,14 @@ class ResourceGroupTest(common.HeatTestCase):
"foo": "bar"
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
}
}
}
}
resource_def = rsrc_defn.ResourceDefinition(
@ -449,6 +510,14 @@ class ResourceGroupTest(common.HeatTestCase):
"type": "OverwrittenFnGetRefIdType",
"properties": {}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
}
}
}
}
self.assertEqual(expected, nested_tmpl.t)
@ -488,6 +557,15 @@ class ResourceGroupTest(common.HeatTestCase):
]
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
"1": {"get_resource": "1"},
"2": {"get_resource": "2"},
}
}
}
}
nested = resg._assemble_nested(['0', '1', '2']).t
@ -514,6 +592,13 @@ class ResourceGroupTest(common.HeatTestCase):
]
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
}
}
}
}
nested = resg._assemble_nested(['0']).t
@ -543,6 +628,13 @@ class ResourceGroupTest(common.HeatTestCase):
]
}
}
},
"outputs": {
"refs_map": {
"value": {
"0": {"get_resource": "0"},
}
}
}
}
nested = resg._assemble_nested(['0']).t
@ -710,7 +802,7 @@ class ResourceGroupTest(common.HeatTestCase):
resgrp._assemble_nested = mock.Mock(return_value='tmpl')
resgrp.properties.data[resgrp.COUNT] = 2
self.patchobject(scheduler.TaskRunner, 'start')
resgrp.handle_update(snip, None, None)
resgrp.handle_update(snip, mock.Mock(), {})
self.assertTrue(resgrp._assemble_nested.called)
def test_handle_delete(self):
@ -728,7 +820,7 @@ class ResourceGroupTest(common.HeatTestCase):
resgrp._assemble_nested = mock.Mock(return_value=None)
resgrp.properties.data[resgrp.COUNT] = 5
self.patchobject(scheduler.TaskRunner, 'start')
resgrp.handle_update(snip, None, None)
resgrp.handle_update(snip, mock.Mock(), {})
self.assertTrue(resgrp._assemble_nested.called)
@ -741,48 +833,63 @@ class ResourceGroupBlackList(common.HeatTestCase):
# 4) resource_list (refid) not in nested()
# 5) resource_list in nested() -> saved
# 6) resource_list (refid) in nested() -> saved
# 7) resource_list (refid) in nested(), update -> saved
# 8) resource_list, update -> saved
# 9) resource_list (refid) in nested(), grouputils fallback -> saved
# A) resource_list (refid) in nested(), update, grouputils -> saved
scenarios = [
('1', dict(data_in=None, rm_list=[],
nested_rsrcs=[], expected=[],
saved=False, rm_mode='append')),
saved=False, fallback=False, rm_mode='append')),
('2', dict(data_in='0,1,2', rm_list=[],
nested_rsrcs=[], expected=['0', '1', '2'],
saved=False, rm_mode='append')),
saved=False, fallback=False, rm_mode='append')),
('3', dict(data_in='1,3', rm_list=['6'],
nested_rsrcs=['0', '1', '3'],
expected=['1', '3'],
saved=False, rm_mode='append')),
saved=False, fallback=False, rm_mode='append')),
('4', dict(data_in='0,1', rm_list=['id-7'],
nested_rsrcs=['0', '1', '3'],
expected=['0', '1'],
saved=False, rm_mode='append')),
saved=False, fallback=False, rm_mode='append')),
('5', dict(data_in='0,1', rm_list=['3'],
nested_rsrcs=['0', '1', '3'],
expected=['0', '1', '3'],
saved=True, rm_mode='append')),
saved=True, fallback=False, rm_mode='append')),
('6', dict(data_in='0,1', rm_list=['id-3'],
nested_rsrcs=['0', '1', '3'],
expected=['0', '1', '3'],
saved=True, rm_mode='append')),
saved=True, fallback=False, rm_mode='append')),
('7', dict(data_in='0,1', rm_list=['id-3'],
nested_rsrcs=['0', '1', '3'],
expected=['3'],
saved=True, rm_mode='update')),
saved=True, fallback=False, rm_mode='update')),
('8', dict(data_in='1', rm_list=[],
nested_rsrcs=['0', '1', '2'],
expected=[],
saved=True, rm_mode='update')),
saved=True, fallback=False, rm_mode='update')),
('9', dict(data_in='0,1', rm_list=['id-3'],
nested_rsrcs=['0', '1', '3'],
expected=['0', '1', '3'],
saved=True, fallback=True, rm_mode='append')),
('A', dict(data_in='0,1', rm_list=['id-3'],
nested_rsrcs=['0', '1', '3'],
expected=['3'],
saved=True, fallback=True, rm_mode='update')),
]
def test_blacklist(self):
stack = utils.parse_stack(template)
resg = stack['group1']
if self.data_in is not None:
resg.resource_id = 'foo'
# mock properties
resg.properties = mock.MagicMock()
properties = mock.MagicMock()
p_data = {'removal_policies': [{'resource_list': self.rm_list}],
'removal_policies_mode': self.rm_mode}
resg.properties.__getitem__.side_effect = p_data.__getitem__
properties.get.side_effect = p_data.get
# mock data get/set
resg.data = mock.Mock()
@ -790,28 +897,41 @@ class ResourceGroupBlackList(common.HeatTestCase):
resg.data_set = mock.Mock()
# mock nested access
def stack_contains(name):
return name in self.nested_rsrcs
mock_inspect = mock.Mock()
self.patchobject(grouputils.GroupInspector, 'from_parent_resource',
return_value=mock_inspect)
mock_inspect.member_names.return_value = self.nested_rsrcs
def by_refid(name):
rid = name.replace('id-', '')
if rid not in self.nested_rsrcs:
return None
res = mock.Mock()
res.name = rid
return res
if not self.fallback:
refs_map = {n: 'id-%s' % n for n in self.nested_rsrcs}
resg.get_output = mock.Mock(return_value=refs_map)
else:
resg.get_output = mock.Mock(side_effect=exception.NotFound)
nested = mock.MagicMock()
nested.__contains__.side_effect = stack_contains
nested.__iter__.side_effect = iter(self.nested_rsrcs)
nested.resource_by_refid.side_effect = by_refid
resg.nested = mock.Mock(return_value=nested)
def stack_contains(name):
return name in self.nested_rsrcs
blacklist = resg._name_blacklist()
self.assertEqual(set(self.expected), blacklist)
def by_refid(name):
rid = name.replace('id-', '')
if rid not in self.nested_rsrcs:
return None
res = mock.Mock()
res.name = rid
return res
nested = mock.MagicMock()
nested.__contains__.side_effect = stack_contains
nested.__iter__.side_effect = iter(self.nested_rsrcs)
nested.resource_by_refid.side_effect = by_refid
resg.nested = mock.Mock(return_value=nested)
resg._update_name_blacklist(properties)
if self.saved:
resg.data_set.assert_called_once_with('name_blacklist',
','.join(blacklist))
','.join(self.expected))
else:
resg.data_set.assert_not_called()
self.assertEqual(set(self.expected), resg._name_blacklist())
class ResourceGroupEmptyParams(common.HeatTestCase):
@ -1261,12 +1381,15 @@ class RollingUpdatePolicyDiffTest(common.HeatTestCase):
tmpl_diff = updated_grp.update_template_diff(
updated_grp_json, current_grp_json)
self.assertTrue(tmpl_diff.update_policy_changed())
prop_diff = current_grp.update_template_diff_properties(
updated_grp.properties,
current_grp.properties)
# test application of the new update policy in handle_update
current_grp._try_rolling_update = mock.Mock()
current_grp._assemble_nested_for_size = mock.Mock()
self.patchobject(scheduler.TaskRunner, 'start')
current_grp.handle_update(updated_grp_json, tmpl_diff, None)
current_grp.handle_update(updated_grp_json, tmpl_diff, prop_diff)
self.assertEqual(updated_grp_json._update_policy or {},
current_grp.update_policy.data)