From cb575889682ab33f0bbe45a3387a5a516ab707f1 Mon Sep 17 00:00:00 2001 From: "anthony.bellino" Date: Tue, 4 Sep 2018 19:34:11 +0000 Subject: [PATCH] Fix for get manifest In some use cases, some site level docs are only included in specific manifests. This is so sites can call out what they want deployed, however currently Armada is checking for all documents to exist and leads to an invalid manifest exception. This PS removes the '.build_charts_deps()' and 'build_chart_groups()' calls in 'get_manifest()' so that only chart documents, and chart group documents are built after finding them within 'build_armada_manfiest()' and 'build_chart_group()'. 'build_armada_manifest()' will now throw the related 'Could not find chart group... exception' for related chart and chart group issues. Additional subclass exceptions were added along with adding traceback to capture the chained exceptions. Change-Id: Idc8a75b290ac0afb1e177203535b012d589b708f --- armada/exceptions/__init__.py | 8 +- armada/exceptions/manifest_exceptions.py | 37 +++++++++ armada/handlers/manifest.py | 81 +++++++------------ armada/tests/unit/api/test_test_controller.py | 5 +- armada/tests/unit/handlers/test_manifest.py | 49 +++++------ .../unit/resources/keystone-manifest.yaml | 34 ++++++++ armada/utils/validate.py | 2 + 7 files changed, 137 insertions(+), 79 deletions(-) diff --git a/armada/exceptions/__init__.py b/armada/exceptions/__init__.py index d2843537..80aeaf50 100644 --- a/armada/exceptions/__init__.py +++ b/armada/exceptions/__init__.py @@ -12,6 +12,12 @@ # See the License for the specific language governing permissions and # limitations under the License. +from armada.exceptions.manifest_exceptions import BuildChartException +from armada.exceptions.manifest_exceptions import BuildChartGroupException +from armada.exceptions.manifest_exceptions import ChartDependencyException from armada.exceptions.manifest_exceptions import ManifestException -__all__ = ['ManifestException'] +__all__ = [ + 'BuildChartException', 'BuildChartGroupException', + 'ChartDependencyException', 'ManifestException' +] diff --git a/armada/exceptions/manifest_exceptions.py b/armada/exceptions/manifest_exceptions.py index 513175ce..0fec4342 100644 --- a/armada/exceptions/manifest_exceptions.py +++ b/armada/exceptions/manifest_exceptions.py @@ -25,3 +25,40 @@ class ManifestException(base.ArmadaBaseException): """ message = 'An error occurred while generating the manifest: %(details)s.' + + +class BuildChartException(ManifestException): + """ + An exception occurred while attempting to build the chart for an + Armada manifest. The exception will return with details as to why. + + **Troubleshoot:** + *Coming Soon* + """ + + message = 'An error occurred while trying to build chart: %(details)s.' + + +class BuildChartGroupException(ManifestException): + """ + An exception occurred while attempting to build the chart group for an + Armada manifest. The exception will return with details as to why. + + **Troubleshoot:** + *Coming Soon* + """ + + message = 'An error occurred while building chart group: %(details)s.' + + +class ChartDependencyException(ManifestException): + """ + An exception occurred while attempting to build a chart dependency for an + Armada manifest. The exception will return with details as to why. + + **Troubleshoot:** + *Coming Soon* + """ + + message = 'An error occurred while building a dependency chart: ' \ + '%(details)s.' diff --git a/armada/handlers/manifest.py b/armada/handlers/manifest.py index 68314832..a030ff67 100644 --- a/armada/handlers/manifest.py +++ b/armada/handlers/manifest.py @@ -112,8 +112,8 @@ class Manifest(object): for chart in self.charts: if chart.get('metadata', {}).get('name') == name: return chart - raise exceptions.ManifestException( - details='Could not find a {} named "{}"'.format( + raise exceptions.BuildChartException( + details='Could not build {} named "{}"'.format( const.DOCUMENT_CHART, name)) def find_chart_group_document(self, name): @@ -128,26 +128,10 @@ class Manifest(object): for group in self.groups: if group.get('metadata', {}).get('name') == name: return group - raise exceptions.ManifestException( - details='Could not find a {} named "{}"'.format( + raise exceptions.BuildChartGroupException( + details='Could not build {} named "{}"'.format( const.DOCUMENT_GROUP, name)) - def build_charts_deps(self): - """Build chart dependencies for every ``chart``. - - :returns: None - """ - for chart in self.charts: - self.build_chart_deps(chart) - - def build_chart_groups(self): - """Build chart dependencies for every ``chart_group``. - - :returns: None - """ - for chart_group in self.groups: - self.build_chart_group(chart_group) - def build_chart_deps(self, chart): """Recursively build chart dependencies for ``chart``. @@ -159,7 +143,6 @@ class Manifest(object): under ``chart['data']['dependencies']`` could not be found. """ try: - dep = None chart_dependencies = chart.get('data', {}).get('dependencies', []) for iter, dep in enumerate(chart_dependencies): if isinstance(dep, dict): @@ -170,9 +153,10 @@ class Manifest(object): 'chart': chart_dep.get('data', {}) } except Exception: - raise exceptions.ManifestException( - details="Could not find dependency chart {} in {}".format( - dep, const.DOCUMENT_CHART)) + raise exceptions.ChartDependencyException( + details="Could not build dependencies for chart {} in {}". + format( + chart.get('metadata').get('name'), const.DOCUMENT_CHART)) else: return chart @@ -193,15 +177,17 @@ class Manifest(object): if isinstance(chart, dict): continue chart_dep = self.find_chart_document(chart) + self.build_chart_deps(chart_dep) chart_group['data']['chart_group'][iter] = { 'chart': chart_dep.get('data', {}) } - except Exception: - raise exceptions.ManifestException( - details="Could not find chart {} in {}".format( - chart, const.DOCUMENT_GROUP)) - else: - return chart_group + except exceptions.ManifestException: + cg_name = chart_group.get('metadata', {}).get('name') + raise exceptions.BuildChartGroupException( + details="Could not build chart group {} in {}".format( + cg_name, const.DOCUMENT_GROUP)) + + return chart_group def build_armada_manifest(self): """Builds the Armada manifest while pulling out data @@ -212,36 +198,27 @@ class Manifest(object): :raises ManifestException: If a chart group's data listed under ``chart_group['data']`` could not be found. """ - try: - group = None - for iter, group in enumerate( - self.manifest.get('data', {}).get('chart_groups', [])): - if isinstance(group, dict): - continue - chart_grp = self.find_chart_group_document(group) + for iter, group in enumerate( + self.manifest.get('data', {}).get('chart_groups', [])): + if isinstance(group, dict): + continue + chart_grp = self.find_chart_group_document(group) + self.build_chart_group(chart_grp) - # Add name to chart group - ch_grp_data = chart_grp.get('data', {}) - ch_grp_data['name'] = chart_grp.get('metadata', {}).get('name') + # Add name to chart group + ch_grp_data = chart_grp.get('data', {}) + ch_grp_data['name'] = chart_grp.get('metadata', {}).get('name') - self.manifest['data']['chart_groups'][iter] = ch_grp_data - except Exception: - raise exceptions.ManifestException( - "Could not find chart group {} in {}".format( - group, const.DOCUMENT_MANIFEST)) - else: - return self.manifest + self.manifest['data']['chart_groups'][iter] = ch_grp_data + + return self.manifest def get_manifest(self): - """Builds all of the documents including the dependencies of the - chart documents, the charts in the chart_groups, and the - Armada manifest + """Builds the Armada manifest :returns: The Armada manifest. :rtype: dict """ - self.build_charts_deps() - self.build_chart_groups() self.build_armada_manifest() return {'armada': self.manifest.get('data', {})} diff --git a/armada/tests/unit/api/test_test_controller.py b/armada/tests/unit/api/test_test_controller.py index 12525a84..739e85d8 100644 --- a/armada/tests/unit/api/test_test_controller.py +++ b/armada/tests/unit/api/test_test_controller.py @@ -143,8 +143,9 @@ class TestReleasesManifestControllerNegativeTest(base.BaseControllerTest): self.assertEqual(1, resp_body['details']['errorCount']) self.assertIn({ 'message': - ('An error occurred while generating the manifest: Could not ' - 'find dependency chart helm-toolkit in armada/Chart/v1.'), + ('An error occurred while building chart group: ' + 'Could not build chart group keystone-infra-services in ' + 'armada/ChartGroup/v1.'), 'error': True, 'kind': diff --git a/armada/tests/unit/handlers/test_manifest.py b/armada/tests/unit/handlers/test_manifest.py index dba5243b..8293bdc9 100644 --- a/armada/tests/unit/handlers/test_manifest.py +++ b/armada/tests/unit/handlers/test_manifest.py @@ -40,12 +40,12 @@ class ManifestTestCase(testtools.TestCase): self.assertIsInstance(armada_manifest.groups, list) self.assertIsNotNone(armada_manifest.manifest) - self.assertEqual(4, len(armada_manifest.charts)) - self.assertEqual(2, len(armada_manifest.groups)) + self.assertEqual(5, len(armada_manifest.charts)) + self.assertEqual(3, len(armada_manifest.groups)) - self.assertEqual([self.documents[x] for x in range(4)], + self.assertEqual([self.documents[x] for x in range(5)], armada_manifest.charts) - self.assertEqual([self.documents[x] for x in range(4, 6)], + self.assertEqual([self.documents[x] for x in range(5, 8)], armada_manifest.groups) self.assertEqual(self.documents[-1], armada_manifest.manifest) @@ -59,12 +59,12 @@ class ManifestTestCase(testtools.TestCase): self.assertIsInstance(armada_manifest.groups, list) self.assertIsNotNone(armada_manifest.manifest) - self.assertEqual(4, len(armada_manifest.charts)) - self.assertEqual(2, len(armada_manifest.groups)) + self.assertEqual(5, len(armada_manifest.charts)) + self.assertEqual(3, len(armada_manifest.groups)) - self.assertEqual([self.documents[x] for x in range(4)], + self.assertEqual([self.documents[x] for x in range(5)], armada_manifest.charts) - self.assertEqual([self.documents[x] for x in range(4, 6)], + self.assertEqual([self.documents[x] for x in range(5, 8)], armada_manifest.groups) self.assertEqual(self.documents[-1], armada_manifest.manifest) self.assertEqual('armada-manifest', @@ -87,12 +87,12 @@ class ManifestTestCase(testtools.TestCase): self.assertIsInstance(armada_manifest.groups, list) self.assertIsNotNone(armada_manifest.manifest) - self.assertEqual(4, len(armada_manifest.charts)) - self.assertEqual(2, len(armada_manifest.groups)) + self.assertEqual(5, len(armada_manifest.charts)) + self.assertEqual(3, len(armada_manifest.groups)) - self.assertEqual([self.documents[x] for x in range(4)], + self.assertEqual([self.documents[x] for x in range(5)], armada_manifest.charts) - self.assertEqual([self.documents[x] for x in range(4, 6)], + self.assertEqual([self.documents[x] for x in range(5, 8)], armada_manifest.groups) self.assertEqual(armada_manifest.manifest, self.documents[-1]) self.assertEqual('armada-manifest', @@ -107,7 +107,8 @@ class ManifestTestCase(testtools.TestCase): armada_manifest.manifest['metadata']['name']) def test_get_manifest(self): - armada_manifest = manifest.Manifest(self.documents) + armada_manifest = manifest.Manifest( + self.documents, target_manifest='armada-manifest') obtained_manifest = armada_manifest.get_manifest() self.assertIsInstance(obtained_manifest, dict) self.assertEqual(obtained_manifest['armada'], @@ -116,7 +117,7 @@ class ManifestTestCase(testtools.TestCase): def test_find_documents(self): armada_manifest = manifest.Manifest(self.documents) chart_documents, chart_groups, manifests = armada_manifest. \ - _find_documents() + _find_documents(target_manifest='armada-manifest') # checking if all the chart documents are present self.assertIsInstance(chart_documents, list) @@ -174,13 +175,13 @@ class ManifestTestCase(testtools.TestCase): ok_chart = armada_manifest. \ find_chart_group_document('openstack-keystone') self.assertIsInstance(ok_chart, dict) - self.assertEqual(self.documents[-2], ok_chart) + self.assertEqual(self.documents[-3], ok_chart) armada_manifest = manifest.Manifest(self.documents) kis_chart = armada_manifest.find_chart_group_document( 'keystone-infra-services') self.assertIsInstance(kis_chart, dict) - self.assertEqual(self.documents[-3], kis_chart) + self.assertEqual(self.documents[-4], kis_chart) def test_verify_build_armada_manifest(self): armada_manifest = manifest.Manifest(self.documents) @@ -375,11 +376,11 @@ class ManifestNegativeTestCase(testtools.TestCase): def test_get_documents_missing_charts(self): # Validates exceptions.ManifestException is thrown if no chart is - # found. Charts are first 4 documents in sample YAML. + # found. Charts are first 5 documents in sample YAML. error_re = ('Documents must be a list of documents with at least one ' 'of each of the following schemas: .*') self.assertRaisesRegexp(exceptions.ManifestException, error_re, - manifest.Manifest, self.documents[4:]) + manifest.Manifest, self.documents[5:]) def test_get_documents_missing_chart_groups(self): # Validates exceptions.ManifestException is thrown if no chart is @@ -392,16 +393,16 @@ class ManifestNegativeTestCase(testtools.TestCase): def test_find_chart_document_negative(self): armada_manifest = manifest.Manifest(self.documents) - error_re = r'Could not find a %s named "%s"' % (const.DOCUMENT_CHART, - 'invalid') - self.assertRaisesRegexp(exceptions.ManifestException, error_re, + error_re = r'Could not build %s named "%s"' % (const.DOCUMENT_CHART, + 'invalid') + self.assertRaisesRegexp(exceptions.BuildChartException, error_re, armada_manifest.find_chart_document, 'invalid') def test_find_group_document_negative(self): armada_manifest = manifest.Manifest(self.documents) - error_re = r'Could not find a %s named "%s"' % (const.DOCUMENT_GROUP, - 'invalid') - self.assertRaisesRegexp(exceptions.ManifestException, error_re, + error_re = r'Could not build %s named "%s"' % (const.DOCUMENT_GROUP, + 'invalid') + self.assertRaisesRegexp(exceptions.BuildChartGroupException, error_re, armada_manifest.find_chart_group_document, 'invalid') diff --git a/armada/tests/unit/resources/keystone-manifest.yaml b/armada/tests/unit/resources/keystone-manifest.yaml index 5d6b5f72..c9c35e82 100644 --- a/armada/tests/unit/resources/keystone-manifest.yaml +++ b/armada/tests/unit/resources/keystone-manifest.yaml @@ -102,6 +102,27 @@ data: dependencies: - helm-toolkit --- +schema: armada/Chart/v1 +metadata: + schema: metadata/Document/v1 + name: ro-global + labels: + name: ro-global + component: ro + layeringDefinition: + abstract: false + layer: global + actions: + - method: merge + path: . + storagePolicy: cleartext +data: + source: {location: '', subpath: charts/ro, type: git} + release: ro-release + chart_name: ro + namespace: openstack + dependencies: [] +--- schema: armada/ChartGroup/v1 metadata: schema: metadata/Document/v1 @@ -123,6 +144,19 @@ data: chart_group: - keystone --- +schema: armada/ChartGroup/v1 +metadata: + schema: metadata/Document/v1 + name: ro-inventory-notifier + layeringDefinition: + abstract: false + layer: global + storagePolicy: cleartext +data: + description: Deploy RO + chart_group: + - ro +--- schema: armada/Manifest/v1 metadata: schema: metadata/Document/v1 diff --git a/armada/utils/validate.py b/armada/utils/validate.py index 7dffb853..98323f35 100644 --- a/armada/utils/validate.py +++ b/armada/utils/validate.py @@ -16,6 +16,7 @@ import jsonschema import os import pkg_resources import requests +import traceback import yaml from oslo_log import log as logging @@ -74,6 +75,7 @@ def _validate_armada_manifest(manifest): except ManifestException as me: vmsg = ValidationMessage( message=str(me), error=True, name='ARM001', level='Error') + LOG.error(traceback.format_exc()) LOG.error('ValidationMessage: %s', vmsg.get_output_json()) details.append(vmsg.get_output()) return False, details