From 10a5a20beaefc9218e84cb129b865d02129ec1ce Mon Sep 17 00:00:00 2001 From: Hemanth Nakkina Date: Tue, 27 Feb 2018 13:45:57 +0530 Subject: [PATCH] Render the documents based on topological order This PS will add layering of documents in addition to existing substitution to create topological order or Document Graph. Render the documents based on sorted topological order. Add test case from https://review.gerrithub.io/#/c/401465/ Change-Id: I06db302aacb9d366d109fa93d81e83eff7fefd4e --- deckhand/engine/layering.py | 148 ++++++------------ .../unit/engine/test_document_layering.py | 87 ++++++++++ 2 files changed, 134 insertions(+), 101 deletions(-) diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py index dbb899c4..62f399dd 100644 --- a/deckhand/engine/layering.py +++ b/deckhand/engine/layering.py @@ -53,16 +53,18 @@ class DocumentLayering(object): # If child has layer N, parent N+1, and current_parent N+2, then swap # parent with current_parent. In other words, if parent's layer is # closer to child's layer than current_parent's layer, then use parent. - current_parent = self._parents.get((child.name, child.schema)) + current_parent_index = self._parents.get((child.schema, child.name)) + current_parent = self._documents_by_index.get( + current_parent_index, None) if current_parent: if (self._layer_order.index(parent.layer) > self._layer_order.index(current_parent.layer)): - self._parents[(child.name, child.schema)] = parent - self._children[ - (current_parent.name, current_parent.schema)].remove(child) + self._parents[(child.schema, child.name)] = \ + (parent.schema, parent.name) all_children[child] -= 1 else: - self._parents.setdefault((child.name, child.schema), parent) + self._parents.setdefault((child.schema, child.name), + (parent.schema, parent.name)) def _is_actual_child_document(self, document, potential_child): if document == potential_child: @@ -112,13 +114,7 @@ class DocumentLayering(object): _potential_children = self._documents_by_labels.get( (label_key, label_val), []) potential_children.extend(_potential_children) - # NOTE(fmontei): The intention here is to preserve the order of all - # the documents that were sorted by `_topologically_sort_documents` - # in order to substitute documents in the right order. But at the same - # time, only unique children should be found. So, this trick below - # maintains the order (unlike set) and guarantees uniqueness. - unique_potential_children = collections.OrderedDict.fromkeys( - potential_children) + unique_potential_children = set(potential_children) for potential_child in unique_potential_children: if self._is_actual_child_document(document, potential_child): @@ -150,7 +146,6 @@ class DocumentLayering(object): all_children = collections.Counter() # Mapping of (doc.name, doc.metadata.name) => children, where children # are the documents whose `parentSelector` references the doc. - self._children = {} self._parents = {} self._parentless_documents = [] @@ -158,7 +153,6 @@ class DocumentLayering(object): documents_in_layer = self._documents_by_layer.get(layer, []) for document in documents_in_layer: children = list(self._calc_document_children(document)) - self._children[(document.name, document.schema)] = children if children: all_children.update(children) @@ -213,8 +207,8 @@ class DocumentLayering(object): return layer_order def _topologically_sort_documents(self, documents): - """Topologically sorts the DAG formed from the documents' substitution - dependency chain. + """Topologically sorts the DAG formed from the documents' layering + and substitution dependency chain. """ documents_by_name = {} result = [] @@ -224,6 +218,11 @@ class DocumentLayering(object): document = document_wrapper.DocumentDict(document) documents_by_name.setdefault((document.schema, document.name), document) + if document.parent_selector: + parent = self._parents.get((document.schema, document.name)) + if parent: + g.add_edge((document.schema, document.name), parent) + for sub in document.substitutions: g.add_edge((document.schema, document.name), (sub['src']['schema'], sub['src']['name'])) @@ -298,10 +297,11 @@ class DocumentLayering(object): :raises IndeterminateDocumentParent: If more than one parent document was found for a document. """ - self._documents_to_layer = [] self._documents_by_layer = {} self._documents_by_labels = {} self._layering_policy = None + self._sorted_documents = {} + self._documents_by_index = {} # TODO(fmontei): Add a hook for post-validation too. if validate: @@ -326,12 +326,10 @@ class DocumentLayering(object): LOG.error(error_msg) raise errors.LayeringPolicyNotFound() - sorted_documents = self._topologically_sort_documents(documents) - - for document in sorted_documents: + for document in documents: document = document_wrapper.DocumentDict(document) - if document.layering_definition: - self._documents_to_layer.append(document) + self._documents_by_index.setdefault( + (document.schema, document.name), document) if document.layer: if document.layer not in self._layering_policy.layer_order: LOG.error('Document layer %s for document [%s] %s not ' @@ -361,6 +359,8 @@ class DocumentLayering(object): self._substitution_sources, fail_on_missing_sub_src=fail_on_missing_sub_src) + self._sorted_documents = self._topologically_sort_documents(documents) + del self._documents_by_layer del self._documents_by_labels @@ -444,21 +444,6 @@ class DocumentLayering(object): return overall_data - def _get_children(self, document): - """Recursively retrieve all children. - - Used in the layering module when calculating children for each - document. - - :returns: List of nested children. - :rtype: Generator[:class:`DocumentDict`] - """ - for child in self._children.get((document.name, document.schema), []): - yield child - grandchildren = self._get_children(child) - for grandchild in grandchildren: - yield grandchild - def render(self): """Perform layering on the list of documents passed to ``__init__``. @@ -476,77 +461,38 @@ class DocumentLayering(object): :raises MissingDocumentKey: If a layering action path isn't found in both the parent and child documents being layered together. """ - # ``rendered_data_by_layer`` tracks the set of changes across all - # actions across each layer for a specific document. - rendered_data_by_layer = document_wrapper.DocumentDict() + for doc in self._sorted_documents: - # NOTE(fmontei): ``global_docs`` represents the topmost documents in - # the system. It should probably be impossible for more than 1 - # top-level doc to exist, but handle multiple for now. - global_docs = [ - doc for doc in self._documents_to_layer - if self._layer_order and doc.layer == self._layer_order[0] - ] - - for doc in global_docs: - layer_idx = self._layer_order.index(doc.layer) - if doc.substitutions: - substituted_data = list( - self.secrets_substitution.substitute_all(doc)) - if substituted_data: - rendered_data_by_layer[layer_idx] = substituted_data[0] - self.secrets_substitution.update_substitution_sources( - doc.schema, doc.name, substituted_data[0].data) - else: - rendered_data_by_layer[layer_idx] = doc - - # Keep iterating as long as a child exists. - for child in self._get_children(doc): - # Retrieve the most up-to-date rendered_data (by referencing - # the child's parent's data). - child_layer_idx = self._layer_order.index(child.layer) - parent = self._parents[child.name, child.schema] - parent_layer_idx = self._layer_order.index(parent.layer) - rendered_data = rendered_data_by_layer[parent_layer_idx] + if doc.parent_selector: + parent_meta = self._parents.get((doc.schema, doc.name)) # Apply each action to the current document. - for action in child.actions: - LOG.debug('Applying action %s to child document with ' - 'name=%s, schema=%s, layer=%s.', action, - child.name, child.schema, child.layer) - rendered_data = self._apply_action( - action, child, rendered_data) + if parent_meta: + parent = self._documents_by_index[parent_meta] + rendered_data = parent + for action in doc.actions: + LOG.debug('Applying action %s to document with ' + 'name=%s, schema=%s, layer=%s.', action, + doc.name, doc.schema, doc.layer) + rendered_data = self._apply_action( + action, doc, rendered_data) + if not doc.is_abstract: + doc.data = rendered_data.data + self.secrets_substitution.update_substitution_sources( + doc.schema, doc.name, rendered_data.data) + self._documents_by_index[(doc.schema, doc.name)] = ( + rendered_data) - # Update the actual document data if concrete. - if not child.is_abstract: - child_index = self._documents_to_layer.index(child) - child.data = rendered_data.data - substituted_data = list( - self.secrets_substitution.substitute_all(child)) - if substituted_data: - rendered_data = substituted_data[0] - self._documents_to_layer[child_index].data = ( - rendered_data.data) - - # Update ``rendered_data_by_layer`` for this layer so that - # children in deeper layers can reference the most up-to-date - # changes. - rendered_data_by_layer[child_layer_idx] = rendered_data - self.secrets_substitution.update_substitution_sources( - child.schema, child.name, rendered_data.data) - - # Handle edge case for parentless documents that require substitution. - # If a document has no parent, then the for loop above doesn't iterate - # over the parentless document, so substitution must be done here for - # parentless documents. - for doc in self._parentless_documents: - if not doc.is_abstract and doc.substitutions: + # Update the actual document data if concrete. + if not doc.is_abstract: substituted_data = list( self.secrets_substitution.substitute_all(doc)) if substituted_data: - doc = substituted_data[0] + rendered_data = substituted_data[0] + doc.data = rendered_data.data self.secrets_substitution.update_substitution_sources( - doc.schema, doc.name, substituted_data[0].data) + doc.schema, doc.name, rendered_data.data) + self._documents_by_index[(doc.schema, doc.name)] = doc # Return only concrete documents. - return [d for d in self._documents_to_layer if d.is_abstract is False] + return [d for d in self._sorted_documents if d.is_abstract is False] diff --git a/deckhand/tests/unit/engine/test_document_layering.py b/deckhand/tests/unit/engine/test_document_layering.py index 7608e2e3..0b42ce1c 100644 --- a/deckhand/tests/unit/engine/test_document_layering.py +++ b/deckhand/tests/unit/engine/test_document_layering.py @@ -200,6 +200,93 @@ data: documents, site_expected, region_expected, global_expected, substitution_sources=[documents[1]]) + def test_layering_verify_that_substitution_dependencies_includes_parents( + self): + """This scenario consists of a layerOrder with global, region, site, + with 1 global documents and 2 sites documents. Below, document 'f' + directly relies on document 'e' for substitutions. However, in order + for document 'e' to have all the data it needs, it must be layered + with its parent, document 'd', first. Hence, document 'f', by extension + has an implicit dependency on document 'd'. This test verifies that + implicit dependencies are factored in. + """ + + payload = """ +--- +schema: aic/Versions/v1 +metadata: + name: d + schema: metadata/Document/v1 + labels: + selector: foo1 + layeringDefinition: + abstract: True + layer: global +data: + conf: + foo: default +--- +schema: aic/Versions/v1 +metadata: + name: e + schema: metadata/Document/v1 + labels: + selector: baz1 + layeringDefinition: + abstract: False + layer: site + parentSelector: + selector: foo1 + actions: + - method: merge + path: . +data: + conf: + bar: override +--- +schema: armada/Chart/v1 +metadata: + name: f + schema: metadata/Document/v1 + layeringDefinition: + abstract: False + layer: global + substitutions: + - src: + schema: aic/Versions/v1 + name: e + path: .conf + dest: + path: .application.conf +data: + application: + conf: {} +--- +schema: deckhand/LayeringPolicy/v1 +metadata: + schema: metadata/Control/v1 + name: layering-policy +data: + layerOrder: + - global + - region + - site +... +""" + + documents = list(yaml.safe_load_all(payload)) + + site_expected = [ + {'conf': {'foo': 'default', 'bar': 'override'}}, + ] + region_expected = None + global_expected = [ + {'application': {'conf': {'bar': 'override', 'foo': 'default'}}} + ] + self._test_layering( + documents, site_expected, region_expected, global_expected, + substitution_sources=[documents[1]]) + class TestDocumentLayering2Layers(TestDocumentLayering):