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:
parent
3e51547f31
commit
cbcc15b404
|
@ -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)
|
||||
|
||||
|
|
|
@ -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()
|
||||
|
||||
|
|
|
@ -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)
|
||||
|
||||
|
|
Loading…
Reference in New Issue