summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorRick Bartra <rb560u@att.com>2018-10-29 15:47:56 -0400
committerRick Bartra <rb560u@att.com>2018-10-30 10:23:14 -0400
commit60e82b7bd61fedcc50b46ea7b11963201f5c852f (patch)
treed611a42c11862512d669206fbf53b2949cb38089
parent88fe773cd7615ded05657329f39d2fd79af6ad7e (diff)
Validate additional 'metadata.replacement' scenarios
This patch set adds additional documentation and unit tests to validate further replacement scenarios. In particular this commit adds an additional document check that looks for documents exisitng in different layers that contain the same name and same schema without any of them having `replacement: true` Change-Id: I7c033d32a6755f36e609789a748cbc6d4af06bc2
Notes
Notes (review): Code-Review+2: Felipe Monteiro <felipe.monteiro@att.com> Code-Review+2: Aaron Sheffield <ajs@sheffieldfamily.net> Workflow+1: Aaron Sheffield <ajs@sheffieldfamily.net> Verified+2: Zuul Submitted-by: Zuul Submitted-at: Tue, 30 Oct 2018 21:40:18 +0000 Reviewed-on: https://review.openstack.org/612528 Project: openstack/airship-deckhand Branch: refs/heads/master
-rw-r--r--deckhand/engine/_replacement.py46
-rw-r--r--deckhand/engine/layering.py9
-rw-r--r--deckhand/tests/unit/control/test_errors.py8
-rw-r--r--deckhand/tests/unit/engine/test_document_layering_and_replacement.py204
-rw-r--r--deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py62
-rw-r--r--doc/source/users/replacement.rst18
6 files changed, 264 insertions, 83 deletions
diff --git a/deckhand/engine/_replacement.py b/deckhand/engine/_replacement.py
index 1ac4755..a420aad 100644
--- a/deckhand/engine/_replacement.py
+++ b/deckhand/engine/_replacement.py
@@ -75,8 +75,50 @@ def check_only_one_level_of_replacement(src_ref):
75 # If the document has a replacement, use the replacement as the 75 # If the document has a replacement, use the replacement as the
76 # substitution source instead. 76 # substitution source instead.
77 if src_ref.is_replacement: 77 if src_ref.is_replacement:
78 error_message = ('A replacement document cannot itself' 78 error_message = ('A replacement document cannot itself be replaced by '
79 ' be replaced by another document.') 79 'another document.')
80 raise errors.InvalidDocumentReplacement( 80 raise errors.InvalidDocumentReplacement(
81 schema=src_ref.schema, name=src_ref.name, 81 schema=src_ref.schema, name=src_ref.name,
82 layer=src_ref.layer, reason=error_message) 82 layer=src_ref.layer, reason=error_message)
83
84
85def check_replacement_is_false_uniqueness(
86 document, non_replacement_documents):
87 """Validate uniqueness of ``replacement: false`` for each document
88 identifier.
89
90 This check essentially validates that each raw document (which is uniquely
91 defined by (name, schema, layer)) maps to a unique rendered document
92 (which is uniquely defined by (name, schema)). This can be done by ensuring
93 that each (name, schema) pair only has one occurrence of
94 ``replacement: false``.
95
96 Normally, a ``replacement: true`` document nukes the ``replacement: false``
97 parent. But when > 1 ``replacement: false`` documents with same (name,
98 schema) exist, the raw document unique constraint predominates over the
99 rendered document unique constraint, resulting in a breakdown in the
100 rendering process, as confusion occurs over which document to reference
101 for substitution data and the like.
102
103 :param document: current document in the collection that is being processed
104 :param non_replacement_documents: a set containing tuples of the names and
105 schemas of all the non-replacement documents
106 """
107 if not document.is_control:
108 document_identifier = (
109 document['metadata']['name'],
110 document['metadata']['schema']
111 )
112 if document_identifier in non_replacement_documents:
113 error_message = (
114 'Documents with the same name and schema existing in '
115 'different layers without any of them having '
116 '`replacement = true` cannot exist as Deckhand will '
117 'arbitrarily select any of them for processing and none are '
118 'distinguishable from one another because none are a '
119 'parent or child or a replacement document.')
120 raise errors.InvalidDocumentReplacement(
121 schema=document.schema, name=document.name,
122 layer=document.layer, reason=error_message)
123 else:
124 non_replacement_documents.add(document_identifier)
diff --git a/deckhand/engine/layering.py b/deckhand/engine/layering.py
index c4c0e58..d2719cb 100644
--- a/deckhand/engine/layering.py
+++ b/deckhand/engine/layering.py
@@ -60,6 +60,10 @@ class DocumentLayering(object):
60 def _calc_replacements_and_substitutions( 60 def _calc_replacements_and_substitutions(
61 self, substitution_sources): 61 self, substitution_sources):
62 62
63 # Used to track document names and schemas for documents that are not
64 # replacement documents
65 non_replacement_documents = set()
66
63 for document in self._documents_by_index.values(): 67 for document in self._documents_by_index.values():
64 parent_meta = self._parents.get(document.meta) 68 parent_meta = self._parents.get(document.meta)
65 parent = self._documents_by_index.get(parent_meta) 69 parent = self._documents_by_index.get(parent_meta)
@@ -71,8 +75,13 @@ class DocumentLayering(object):
71 parent, document) 75 parent, document)
72 parent.replaced_by = document 76 parent.replaced_by = document
73 else: 77 else:
78 # Handles case where parent and child have replacement: false
79 # as in this case both documents should not be replacement
80 # documents, requiring them to have different schema/name pair.
74 replacement.check_child_and_parent_different_metadata_name( 81 replacement.check_child_and_parent_different_metadata_name(
75 parent, document) 82 parent, document)
83 replacement.check_replacement_is_false_uniqueness(
84 document, non_replacement_documents)
76 85
77 # Since a substitution source only provides the document's 86 # Since a substitution source only provides the document's
78 # `metadata.name` and `schema`, their tuple acts as the dictionary key. 87 # `metadata.name` and `schema`, their tuple acts as the dictionary key.
diff --git a/deckhand/tests/unit/control/test_errors.py b/deckhand/tests/unit/control/test_errors.py
index 5e9f72a..ed18407 100644
--- a/deckhand/tests/unit/control/test_errors.py
+++ b/deckhand/tests/unit/control/test_errors.py
@@ -185,14 +185,14 @@ class TestValidationMessageFormatting(test_base.BaseControllerTest):
185 headers={'Content-Type': 'application/x-yaml'}, 185 headers={'Content-Type': 'application/x-yaml'},
186 body=payload) 186 body=payload)
187 187
188 with mock.patch('deckhand.control.revision_documents.db_api' 188 with mock.patch('deckhand.control.revision_documents.common'
189 '.revision_documents_get', autospec=True) \ 189 '.get_rendered_docs', autospec=True) \
190 as mock_get_rev_documents: 190 as mock_get_rendered_docs:
191 invalid_document = document_wrapper.DocumentDict( 191 invalid_document = document_wrapper.DocumentDict(
192 yaml.safe_load(payload)) 192 yaml.safe_load(payload))
193 invalid_document.pop('metadata') 193 invalid_document.pop('metadata')
194 mock_get_rendered_docs.return_value = ([invalid_document], False)
194 195
195 mock_get_rev_documents.return_value = [invalid_document]
196 resp = self.app.simulate_get( 196 resp = self.app.simulate_get(
197 '/api/v1.0/revisions/1/rendered-documents', 197 '/api/v1.0/revisions/1/rendered-documents',
198 headers={'Content-Type': 'application/x-yaml'}) 198 headers={'Content-Type': 'application/x-yaml'})
diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement.py
index 057bd58..b202bec 100644
--- a/deckhand/tests/unit/engine/test_document_layering_and_replacement.py
+++ b/deckhand/tests/unit/engine/test_document_layering_and_replacement.py
@@ -12,12 +12,89 @@
12# See the License for the specific language governing permissions and 12# See the License for the specific language governing permissions and
13# limitations under the License. 13# limitations under the License.
14 14
15import inspect
15import itertools 16import itertools
16import os 17import os
17import yaml 18import yaml
18 19
19from deckhand.tests.unit.engine import test_document_layering 20from deckhand.tests.unit.engine import test_document_layering
20 21
22REPLACEMENT_3_TIER_SAMPLE = list(yaml.safe_load_all(inspect.cleandoc(
23 """
24 ---
25 schema: deckhand/LayeringPolicy/v1
26 metadata:
27 schema: metadata/Control/v1
28 name: layering-policy
29 storagePolicy: cleartext
30 data:
31 layerOrder:
32 - global
33 - region
34 - site
35 ---
36 schema: armada/Chart/v1
37 metadata:
38 schema: metadata/Document/v1
39 name: nova-global
40 storagePolicy: cleartext
41 labels:
42 name: nova-global
43 component: nova
44 layeringDefinition:
45 abstract: false
46 layer: global
47 data:
48 values:
49 pod:
50 replicas:
51 server: 16
52 ---
53 schema: armada/Chart/v1
54 metadata:
55 schema: metadata/Document/v1
56 name: nova
57 storagePolicy: cleartext
58 labels:
59 name: nova-5ec
60 component: nova
61 layeringDefinition:
62 abstract: false
63 layer: region
64 parentSelector:
65 name: nova-global
66 actions:
67 - method: merge
68 path: .
69 data: {}
70 ---
71 schema: armada/Chart/v1
72 metadata:
73 schema: metadata/Document/v1
74 replacement: true
75 storagePolicy: cleartext
76 name: nova
77 layeringDefinition:
78 abstract: false
79 layer: site
80 parentSelector:
81 name: nova-5ec
82 actions:
83 - method: merge
84 path: .
85 data:
86 values:
87 pod:
88 replicas:
89 api_metadata: 16
90 placement: 2
91 osapi: 16
92 conductor: 16
93 consoleauth: 2
94 scheduler: 2
95 novncproxy: 2
96 """)))
97
21 98
22class TestDocumentLayeringWithReplacement( 99class TestDocumentLayeringWithReplacement(
23 test_document_layering.TestDocumentLayering): 100 test_document_layering.TestDocumentLayering):
@@ -266,82 +343,8 @@ data:
266 343
267 * Global document called nova-global 344 * Global document called nova-global
268 * Region document called nova (layers with nova-global) 345 * Region document called nova (layers with nova-global)
269 * Site document (replaces nova) 346 * Site document (replaces region document)
270 """ 347 """
271 self.documents = list(yaml.safe_load_all("""
272---
273schema: deckhand/LayeringPolicy/v1
274metadata:
275 schema: metadata/Control/v1
276 name: layering-policy
277 storagePolicy: cleartext
278data:
279 layerOrder:
280 - global
281 - region
282 - site
283---
284schema: armada/Chart/v1
285metadata:
286 schema: metadata/Document/v1
287 name: nova-global
288 storagePolicy: cleartext
289 labels:
290 name: nova-global
291 component: nova
292 layeringDefinition:
293 abstract: false
294 layer: global
295data:
296 values:
297 pod:
298 replicas:
299 server: 16
300---
301schema: armada/Chart/v1
302metadata:
303 schema: metadata/Document/v1
304 name: nova
305 storagePolicy: cleartext
306 labels:
307 name: nova-5ec
308 component: nova
309 layeringDefinition:
310 abstract: false
311 layer: region
312 parentSelector:
313 name: nova-global
314 actions:
315 - method: merge
316 path: .
317data: {}
318---
319schema: armada/Chart/v1
320metadata:
321 schema: metadata/Document/v1
322 replacement: true
323 storagePolicy: cleartext
324 name: nova
325 layeringDefinition:
326 abstract: false
327 layer: site
328 parentSelector:
329 name: nova-5ec
330 actions:
331 - method: merge
332 path: .
333data:
334 values:
335 pod:
336 replicas:
337 api_metadata: 16
338 placement: 2
339 osapi: 16
340 conductor: 16
341 consoleauth: 2
342 scheduler: 2
343 novncproxy: 2
344"""))
345 348
346 site_expected = [ 349 site_expected = [
347 { 350 {
@@ -372,7 +375,56 @@ data:
372 } 375 }
373 } 376 }
374 ] 377 ]
375 self._test_layering(self.documents, 378 self._test_layering(REPLACEMENT_3_TIER_SAMPLE,
376 site_expected=site_expected, 379 site_expected=site_expected,
377 region_expected=None, 380 region_expected=None,
378 global_expected=global_expected) 381 global_expected=global_expected)
382
383 def test_multi_layer_replacement_with_intermediate_replacement(self):
384 """Validate the following scenario:
385
386 * Global document called nova-replace
387 * Region document called nova-replace (layers with nova-replace and
388 replaces it)
389 * Site document (layers with region document)
390 """
391
392 replacement_sample = list(REPLACEMENT_3_TIER_SAMPLE)
393 replacement_sample[1]['metadata']['name'] = 'nova-replace'
394 replacement_sample[2]['metadata']['name'] = 'nova-replace'
395 replacement_sample[2]['metadata']['replacement'] = True
396 replacement_sample[3]['metadata']['replacement'] = False
397
398 site_expected = [
399 {
400 "values": {
401 "pod": {
402 "replicas": {
403 "api_metadata": 16,
404 "placement": 2,
405 "osapi": 16,
406 "conductor": 16,
407 "consoleauth": 2,
408 "scheduler": 2,
409 "novncproxy": 2,
410 "server": 16
411 }
412 }
413 }
414 }
415 ]
416 region_expected = [
417 {
418 "values": {
419 "pod": {
420 "replicas": {
421 "server": 16
422 }
423 }
424 }
425 }
426 ]
427 self._test_layering(replacement_sample,
428 site_expected=site_expected,
429 region_expected=region_expected,
430 global_expected=None)
diff --git a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py
index 192df14..7f73a94 100644
--- a/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py
+++ b/deckhand/tests/unit/engine/test_document_layering_and_replacement_negative.py
@@ -24,6 +24,10 @@ class TestDocumentLayeringReplacementNegative(
24 """Validate that attempting to replace a child with its parent when 24 """Validate that attempting to replace a child with its parent when
25 they don't have the same ``metadata.name`` and ``schema`` results in 25 they don't have the same ``metadata.name`` and ``schema`` results in
26 exception. 26 exception.
27
28 global
29 |
30 site (replacement: true, incompatible with parent)
27 """ 31 """
28 doc_factory = factories.DocumentFactory(2, [1, 1]) 32 doc_factory = factories.DocumentFactory(2, [1, 1])
29 documents = doc_factory.gen_test({}) 33 documents = doc_factory.gen_test({})
@@ -52,6 +56,10 @@ class TestDocumentLayeringReplacementNegative(
52 """Validate that a non-replacement document (i.e. regular document 56 """Validate that a non-replacement document (i.e. regular document
53 without `replacement: true`) cannot have the same schema/name as 57 without `replacement: true`) cannot have the same schema/name as
54 another document. 58 another document.
59
60 global (replacement: false)
61 |
62 site (replacement: false)
55 """ 63 """
56 doc_factory = factories.DocumentFactory(2, [1, 1]) 64 doc_factory = factories.DocumentFactory(2, [1, 1])
57 documents = doc_factory.gen_test({}) 65 documents = doc_factory.gen_test({})
@@ -68,6 +76,10 @@ class TestDocumentLayeringReplacementNegative(
68 def test_replacement_without_parent_raises_exc(self): 76 def test_replacement_without_parent_raises_exc(self):
69 """Validate that attempting to do replacement without a parent document 77 """Validate that attempting to do replacement without a parent document
70 raises an exception. 78 raises an exception.
79
80 None
81 |
82 site (replacement: true)
71 """ 83 """
72 doc_factory = factories.DocumentFactory(2, [1, 1]) 84 doc_factory = factories.DocumentFactory(2, [1, 1])
73 documents = doc_factory.gen_test({}) 85 documents = doc_factory.gen_test({})
@@ -80,9 +92,35 @@ class TestDocumentLayeringReplacementNegative(
80 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, 92 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
81 self._test_layering, documents) 93 self._test_layering, documents)
82 94
95 def test_replacement_with_parent_replace_true_raises_exc(self):
96 """Validate that a parent document with replacement: true necessarily
97 fails if it itself doesn't have a parent.
98
99 None
100 |
101 global (replacement: true)
102 |
103 site
104 """
105 doc_factory = factories.DocumentFactory(2, [1, 1])
106 documents = doc_factory.gen_test({})
107
108 documents[1]['metadata']['replacement'] = True
109
110 error_re = (r'Document replacement requires that the document with '
111 '`replacement: true` have a parent.')
112 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
113 self._test_layering, documents)
114
83 def test_replacement_that_is_replaced_raises_exc(self): 115 def test_replacement_that_is_replaced_raises_exc(self):
84 """Validate that attempting replace a replacement document raises an 116 """Validate that attempting to replace a replacement document raises an
85 exception. 117 exception.
118
119 global
120 |
121 region (replacement: true)
122 |
123 site (replacement: true)
86 """ 124 """
87 doc_factory = factories.DocumentFactory(3, [1, 1, 1]) 125 doc_factory = factories.DocumentFactory(3, [1, 1, 1])
88 documents = doc_factory.gen_test({}, region_abstract=False, 126 documents = doc_factory.gen_test({}, region_abstract=False,
@@ -99,3 +137,25 @@ class TestDocumentLayeringReplacementNegative(
99 'another document.') 137 'another document.')
100 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re, 138 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
101 self._test_layering, documents) 139 self._test_layering, documents)
140
141 def test_replacement_true_with_parent_replacement_true_raises_exc(self):
142 """Validate that when documents have the same `metadata.name` and
143 `metadata.schema` existing in different layers without any of them
144 having `replacement = true` raises an exception
145 """
146 doc_factory = factories.DocumentFactory(2, [1, 1])
147 documents = doc_factory.gen_test({})
148
149 for document in documents[1:]:
150 document['metadata']['name'] = 'foo'
151 document['schema'] = 'example/Kind/v1'
152 document['metadata']['replacement'] = False
153 if 'parentSelector' in document['metadata']['layeringDefinition']:
154 document['metadata']['layeringDefinition'].pop(
155 'parentSelector')
156
157 error_re = (r'Documents with the same name and schema existing in '
158 'different layers without any of them having '
159 '`replacement = true` cannot exist.*')
160 self.assertRaisesRegexp(errors.InvalidDocumentReplacement, error_re,
161 self._test_layering, documents)
diff --git a/doc/source/users/replacement.rst b/doc/source/users/replacement.rst
index 8f86360..60561a0 100644
--- a/doc/source/users/replacement.rst
+++ b/doc/source/users/replacement.rst
@@ -80,6 +80,24 @@ The following result in validation errors:
80* A replacement document cannot itself be replaced. That is, only one level 80* A replacement document cannot itself be replaced. That is, only one level
81 of replacement is allowed. 81 of replacement is allowed.
82 82
83Here are the following possible scenarios regarding child and parent
84replacement values:
85
86+-------------------------------------------------------------+
87| Child | Parent | Status |
88+=============================================================+
89| True | True | Throws InvalidDocumentReplacement exception|
90+-------------------------------------------------------------+
91| False | True | Throws InvalidDocumentReplacement exception|
92+-------------------------------------------------------------+
93| True | False | Valid scenario |
94+-------------------------------------------------------------+
95| False | False | Throws InvalidDocumentReplacement exception|
96+-------------------------------------------------------------+
97
98Examples
99--------
100
83Note that each key in the examples below is *mandatory* and that the 101Note that each key in the examples below is *mandatory* and that the
84``parentSelector`` labels should be able to select the parent to be replaced. 102``parentSelector`` labels should be able to select the parent to be replaced.
85 103