From 6f8b9f858a152c40966ea1c06099214deef9c521 Mon Sep 17 00:00:00 2001 From: Drew Walters Date: Thu, 4 Oct 2018 15:24:51 +0000 Subject: [PATCH] Refactor validations retrieval for performance The goal of this commit is to reduce the average time spent retrieving validations from Deckhand. Currently, wait times when committing configdocs can be significant due to unnecessary API calls. This change reduces the number of API calls during this process by utilizing the `/revisions/{{revision_id}}/validations/detail` endpoint exposed by Deckhand. During testing, this introduced a 71% decrease in cumulative time for committing configdocs. Note, this commit does not introduce usage of the official Deckhand client, which will be addressed in a future change. Change-Id: I3c86fca6bae1a5a2f74963a87b2198c1705cf3a6 --- .../control/helpers/deckhand_client.py | 104 +++-------------- .../unit/control/test_configdocs_helper.py | 107 +++++++----------- 2 files changed, 55 insertions(+), 156 deletions(-) diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/deckhand_client.py b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/deckhand_client.py index 5771a81a..9c0d4623 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/deckhand_client.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/deckhand_client.py @@ -41,11 +41,7 @@ class DeckhandPaths(enum.Enum): REVISION_LIST = '/revisions' REVISION_TAG_LIST = '/revisions/{}/tags' REVISION_TAG = '/revisions/{}/tags/{}' - REVISION_VALIDATION_LIST = '/revisions/{}/validations' - REVISION_VALIDATION = '/revisions/{}/validations/{}' - REVISION_VALIDATION_ENTRY = ( - '/revisions/{}/validations/{}/entries/{}' - ) + REVISION_VALIDATIONS_LIST = '/revisions/{}/validations/detail' ROLLBACK = '/rollback/{}' @@ -246,98 +242,26 @@ class DeckhandClient(object): self._handle_bad_response(response) return response.text - @staticmethod - def _build_validation_base(dh_validation): - # creates the base structure for validation response - return { - 'count': dh_validation.get('count'), - 'results': [] - } - - @staticmethod - def _build_validation_entry(dh_val_entry): - # creates a validation entry by stripping off the URL - # that the end user can't use anyway. - dh_val_entry.pop('url', None) - return dh_val_entry - def get_all_revision_validations(self, revision_id): """ - Collects and returns the yamls of the validations for a - revision - """ - val_resp = {} - response = self._get_base_validation_resp(revision_id) - # if the base call is no good, stop. - self._handle_bad_response(response) - all_validation_resp = yaml.safe_load(response.text) - if all_validation_resp: - val_resp = DeckhandClient._build_validation_base( - all_validation_resp - ) - validations = all_validation_resp.get('results') - for validation_subset in validations: - subset_name = validation_subset.get('name') - subset_response = self._get_subset_validation_response( - revision_id, - subset_name - ) - # don't fail hard on a single subset not replying - # TODO (bryan-strassner) maybe this should fail hard? - # what if they all fail? - if subset_response.status_code >= 400: - LOG.error( - 'Failed to retrieve %s validations for revision %s', - subset_name, revision_id - ) - val_subset = yaml.safe_load(subset_response.text) - entries = val_subset.get('results') - for entry in entries: - entry_id = entry.get('id') - e_resp = self._get_entry_validation_response(revision_id, - subset_name, - entry_id) - if e_resp.status_code >= 400: - # don't fail hard on a single entry not working - # TODO (bryan-strassner) maybe this should fail hard? - # what if they all fail? - LOG.error( - 'Failed to retrieve entry %s for category %s ' - 'for revision %s', - entry_id, subset_name, revision_id - ) - entry = yaml.safe_load(e_resp.text) - val_resp.get('results').append( - DeckhandClient._build_validation_entry(entry) - ) - return val_resp + Collects a YAML document containing a list of validation results + corresponding to a Deckhand revision ID. - def _get_base_validation_resp(self, revision_id): - # wraps getting the base validation response + :param revision_id: A Deckhand revision ID corresponding to a set of + documents. + :returns: A YAML document containing a results key mapped to a list of + validation results. + """ + # TODO(@drewwalters96): This method should be replaced with usage of + # the official Deckhand client. url = DeckhandClient.get_path( - DeckhandPaths.REVISION_VALIDATION_LIST + DeckhandPaths.REVISION_VALIDATIONS_LIST ).format(revision_id) - return self._get_request(url) + response = self._get_request(url) + self._handle_bad_response(response) - def _get_subset_validation_response(self, revision_id, subset_name): - # wraps getting the subset level of detail of validations - subset_url = DeckhandClient.get_path( - DeckhandPaths.REVISION_VALIDATION - ).format(revision_id, subset_name) - - return self._get_request(subset_url) - - def _get_entry_validation_response(self, - revision_id, - subset_name, - entry_id): - # wraps getting the entry level detail of validation - e_url = DeckhandClient.get_path( - DeckhandPaths.REVISION_VALIDATION_ENTRY - ).format(revision_id, subset_name, entry_id) - - return self._get_request(e_url) + return yaml.safe_load(response.text) @staticmethod def _handle_bad_response(response, threshold=400): diff --git a/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py b/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py index f08b6571..b34b73f1 100644 --- a/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py +++ b/src/bin/shipyard_airflow/tests/unit/control/test_configdocs_helper.py @@ -734,24 +734,7 @@ def test_generate_dh_val_message(): assert generated == expected -FK_VAL_BASE_RESP = FakeResponse( - status_code=200, - text=""" ---- -count: 2 -next: null -prev: null -results: - - name: deckhand-schema-validation - url: https://deckhand/a/url/too/long/for/pep8 - status: success - - name: promenade-site-validation - url: https://deckhand/a/url/too/long/for/pep8 - status: failure -... -""") - -FK_VAL_SUBSET_RESP = FakeResponse( +FK_VAL_RESP = FakeResponse( status_code=200, text=""" --- @@ -759,77 +742,69 @@ count: 1 next: null prev: null results: - - id: 0 - url: https://deckhand/a/url/too/long/for/pep8 + - name: promenade-site-validation + url: https://deckhand/api/v1.0/revisions/4/etc status: failure -... -""") + createdAt: 2017-07-16T02:03Z + expiresAfter: null + expiresAt: null + errors: + - documents: + - schema: promenade/Node/v1 + name: node-document-name + - schema: promenade/Masters/v1 + name: kubernetes-masters + message: This is a message. + - documents: + - schema: promenade/Node/v1 + name: node-document-name + - schema: promenade/Masters/v1 + name: kubernetes-masters + message: This is a message. -FK_VAL_ENTRY_RESP = FakeResponse( - status_code=200, - text=""" ---- -name: promenade-site-validation -url: https://deckhand/a/url/too/long/for/pep8 -status: failure -createdAt: 2017-07-16T02:03Z -expiresAfter: null -expiresAt: null -errors: - - documents: - - schema: promenade/Node/v1 - name: node-document-name - - schema: promenade/Masters/v1 - name: kubernetes-masters - message: Node has master role, but not included in cluster masters list. -... """) -def test__get_deckhand_validation_errors(): +@mock.patch.object(DeckhandClient, 'get_all_revision_validations', + return_value=yaml.safe_load(FK_VAL_RESP.text)) +def test__get_deckhand_validation_errors(mock_client): """ Tets the functionality of processing a response from deckhand """ helper = ConfigdocsHelper(CTX) - helper.deckhand._get_base_validation_resp = ( - lambda revision_id: FK_VAL_BASE_RESP) - helper.deckhand._get_subset_validation_response = ( - lambda reivsion_id, subset_name: FK_VAL_SUBSET_RESP) - helper.deckhand._get_entry_validation_response = ( - lambda reivsion_id, subset_name, entry_id: FK_VAL_ENTRY_RESP) assert len(helper._get_deckhand_validation_errors(5)) == 2 -FK_VAL_ENTRY_RESP_EMPTY = FakeResponse( +FK_VAL_RESP_EMPTY = FakeResponse( status_code=200, text=""" --- -name: promenade-site-validation -url: https://deckhand/a/url/too/long/for/pep8 -status: failure -createdAt: 2017-07-16T02:03Z -expiresAfter: null -expiresAt: null -errors: [] +count: 1 +next: null +prev: null +results: + - name: promenade-site-validation + url: https://deckhand/api/v1.0/revisions/4/etc + status: failure + createdAt: 2017-07-16T02:03Z + expiresAfter: null + expiresAt: null + errors: [] ... """) -def test__get_deckhand_validations_empty_errors(): +@mock.patch.object(DeckhandClient, 'get_all_revision_validations', + return_value=yaml.safe_load(FK_VAL_RESP_EMPTY.text)) +def test__get_deckhand_validations_empty_errors(mock_client): """ Tets the functionality of processing a response from deckhand """ helper = ConfigdocsHelper(CTX) - helper.deckhand._get_base_validation_resp = ( - lambda revision_id: FK_VAL_BASE_RESP) - helper.deckhand._get_subset_validation_response = ( - lambda reivsion_id, subset_name: FK_VAL_SUBSET_RESP) - helper.deckhand._get_entry_validation_response = ( - lambda reivsion_id, subset_name, entry_id: FK_VAL_ENTRY_RESP_EMPTY) assert len(helper._get_deckhand_validation_errors(5)) == 0 -FK_VAL_BASE_RESP_EMPTY = FakeResponse( +FK_VAL_RESP_EMPTY = FakeResponse( status_code=200, text=""" --- @@ -841,13 +816,13 @@ results: [] """) -def test__get_deckhand_validation_errors_empty_results(): +@mock.patch.object(DeckhandClient, 'get_all_revision_validations', + return_value=yaml.safe_load(FK_VAL_RESP_EMPTY.text)) +def test__get_deckhand_validation_errors_empty_results(mock_client): """ Tets the functionality of processing a response from deckhand """ helper = ConfigdocsHelper(CTX) - helper.deckhand._get_base_validation_resp = ( - lambda revision_id: FK_VAL_BASE_RESP_EMPTY) assert len(helper._get_deckhand_validation_errors(5)) == 0