From 222b074cb193cf409ae64b7d81293b5c449a4d93 Mon Sep 17 00:00:00 2001 From: "Carter, Matthew (mc981n)" Date: Mon, 29 Apr 2019 15:12:08 -0500 Subject: [PATCH] Be configuration driven when referencing document names/schemas Currently, any document name or schema referenced in the Shipyard code base is a hard-coded string. Often times, these strings are repeated throughout the code. This patch set adds a new configuration section to shipyard.conf to define document names and schemas so they can then be referenced in the Shipyard code via the oslo configuration object. This functionality will be important for upcoming Shipyard features which will call for more documents to be validated as well as some new Shipyard-created docs. Change-Id: I34ae8cd578bab730d004c3d176e3817b5a45c89e --- charts/shipyard/values.yaml | 10 +++++ doc/source/_static/shipyard.conf.sample | 21 ++++++++++ doc/source/sampleconf.rst | 1 + doc/source/site-definition-documents.rst | 10 +++++ .../etc/shipyard/shipyard.conf.sample | 21 ++++++++++ .../shipyard_airflow/conf/config.py | 28 +++++++++++++ .../control/helpers/configdocs_helper.py | 2 +- .../validators/validate_deployment_action.py | 6 ++- .../validate_deployment_configuration.py | 5 ++- .../validate_deployment_strategy.py | 5 ++- .../deployment_configuration_operator.py | 39 ++++++++++++------- .../shipyard_airflow/plugins/drydock_nodes.py | 11 ++++-- .../plugins/ucp_base_operator.py | 5 +-- .../test_deployment_configuration_operator.py | 37 ++++++++++++++---- tools/resources/shipyard.conf | 4 ++ 15 files changed, 170 insertions(+), 35 deletions(-) diff --git a/charts/shipyard/values.yaml b/charts/shipyard/values.yaml index 09d06c50..199a54df 100644 --- a/charts/shipyard/values.yaml +++ b/charts/shipyard/values.yaml @@ -415,6 +415,16 @@ conf: # If non-existent rule is used, the request should be denied. The # deny_all rule is hard coded in the policy.py code to allow no access. policy_default_rule: deny_all + document_info: + # The name of the deployment configuration document that Shipyard expects + # and validates + deployment_configuration_name: deployment-configuration + # The schema of the deployment configuration document that Shipyard + # expects and validates + deployment_configuration_schema: shipyard/DeploymentConfiguration/v1 + # The schema of the deployment strategy document that Shipyard expects + # and validates. + deployment_strategy_schema: shipyard/DeploymentStrategy/v1 airflow_config_file: path: /usr/local/airflow/airflow.cfg airflow: diff --git a/doc/source/_static/shipyard.conf.sample b/doc/source/_static/shipyard.conf.sample index 9d455f3e..8c67a399 100644 --- a/doc/source/_static/shipyard.conf.sample +++ b/doc/source/_static/shipyard.conf.sample @@ -84,6 +84,27 @@ #service_type = deckhand +[document_info] + +# +# From shipyard_api +# + +# The name of the deployment-configuration document that Shipyard expects and +# validates (string value) +#deployment_configuration_name = deployment-configuration + +# The schema of the deployment-configuration document that Shipyard expects and +# validates (string value) +#deployment_configuration_schema = shipyard/DeploymentConfiguration/v1 + +# The schema of the deployment strategy document that Shipyard expects and +# validates. Note that the name of this document is not configurable, because +# it is controlled by a field in the deployment configuration document. (string +# value) +#deployment_strategy_schema = shipyard/DeploymentStrategy/v1 + + [drydock] # diff --git a/doc/source/sampleconf.rst b/doc/source/sampleconf.rst index acb6d5c8..529c6bac 100644 --- a/doc/source/sampleconf.rst +++ b/doc/source/sampleconf.rst @@ -12,6 +12,7 @@ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the License for the specific language governing permissions and limitations under the License. +.. _sample-configuration: Sample Configuration File ========================== diff --git a/doc/source/site-definition-documents.rst b/doc/source/site-definition-documents.rst index ecbe9678..65b66eb7 100644 --- a/doc/source/site-definition-documents.rst +++ b/doc/source/site-definition-documents.rst @@ -37,6 +37,11 @@ of the Armada manifest that will be used during the deployment/update. A `sample deployment-configuration`_ shows a completely specified example. +Note that the name and schema Shipyard expects the deployment configuration +document to have is conifgurable via the document_info section in the +:ref:`Shipyard configuration `, but should be left +defaulted in most cases. + `Default configuration values`_ are provided for most values. Supported values @@ -179,6 +184,11 @@ document for the site. Example:: - The success criteria indicates that all nodes must be succssful to consider the group a success. +Note that the schema Shipyard expects the deployment strategy document to have +is conifgurable via the document_info section in the +:ref:`Shipyard configuration `, but should be left +defaulted in most cases. + In short, the default behavior is to deploy everything all at once, and halt if there are any failures. diff --git a/src/bin/shipyard_airflow/etc/shipyard/shipyard.conf.sample b/src/bin/shipyard_airflow/etc/shipyard/shipyard.conf.sample index 9d455f3e..8c67a399 100644 --- a/src/bin/shipyard_airflow/etc/shipyard/shipyard.conf.sample +++ b/src/bin/shipyard_airflow/etc/shipyard/shipyard.conf.sample @@ -84,6 +84,27 @@ #service_type = deckhand +[document_info] + +# +# From shipyard_api +# + +# The name of the deployment-configuration document that Shipyard expects and +# validates (string value) +#deployment_configuration_name = deployment-configuration + +# The schema of the deployment-configuration document that Shipyard expects and +# validates (string value) +#deployment_configuration_schema = shipyard/DeploymentConfiguration/v1 + +# The schema of the deployment strategy document that Shipyard expects and +# validates. Note that the name of this document is not configurable, because +# it is controlled by a field in the deployment configuration document. (string +# value) +#deployment_strategy_schema = shipyard/DeploymentStrategy/v1 + + [drydock] # diff --git a/src/bin/shipyard_airflow/shipyard_airflow/conf/config.py b/src/bin/shipyard_airflow/shipyard_airflow/conf/config.py index 89b12246..fda345b8 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/conf/config.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/conf/config.py @@ -286,6 +286,34 @@ SECTIONS = [ ), ] ), + ConfigSection( + name='document_info', + title=('Information about some of the documents Shipyard needs to ' + 'handle'), + options=[ + cfg.StrOpt( + 'deployment_configuration_name', + default='deployment-configuration', + help=('The name of the deployment-configuration document that ' + 'Shipyard expects and validates') + ), + cfg.StrOpt( + 'deployment_configuration_schema', + default='shipyard/DeploymentConfiguration/v1', + help=('The schema of the deployment-configuration document ' + 'that Shipyard expects and validates') + ), + cfg.StrOpt( + 'deployment_strategy_schema', + default='shipyard/DeploymentStrategy/v1', + help=('The schema of the deployment strategy document that ' + 'Shipyard expects and validates. Note that the name of ' + 'this document is not configurable, because it is ' + 'controlled by a field in the deployment configuration ' + 'document.') + ), + ] + ), ] diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py index 346ecb61..b8b86e2a 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/helpers/configdocs_helper.py @@ -525,7 +525,7 @@ class ConfigdocsHelper(object): service_clients.deckhand_client(), revision_id, [(ValidateDeploymentConfigurationFull, - 'deployment-configuration')] + CONF.document_info.deployment_configuration_name)] ) return sy_val_mgr.validate() except Exception as ex: diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_action.py b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_action.py index 069c1bb6..826c0032 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_action.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_action.py @@ -14,6 +14,7 @@ import logging import falcon +from oslo_config import cfg from .validate_deployment_configuration \ import ValidateDeploymentConfigurationBasic @@ -24,6 +25,7 @@ from shipyard_airflow.common.document_validators.document_validator_manager \ from shipyard_airflow.errors import ApiError LOG = logging.getLogger(__name__) +CONF = cfg.CONF class ValidateDeploymentAction: @@ -40,7 +42,7 @@ class ValidateDeploymentAction: dh_client, self.doc_revision, [(ValidateDeploymentConfigurationFull, - 'deployment-configuration')] + CONF.document_info.deployment_configuration_name)] ) else: # Perform a basic validation only @@ -48,7 +50,7 @@ class ValidateDeploymentAction: dh_client, self.doc_revision, [(ValidateDeploymentConfigurationBasic, - 'deployment-configuration')] + CONF.document_info.deployment_configuration_name)] ) def validate(self): diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_configuration.py b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_configuration.py index 5246e0be..762f8de2 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_configuration.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_configuration.py @@ -18,11 +18,14 @@ is performed by Deckhand on Shipyard's behalf. """ import logging +from oslo_config import cfg + from shipyard_airflow.common.document_validators.document_validator import ( DocumentValidator ) from .validate_deployment_strategy import ValidateDeploymentStrategy +CONF = cfg.CONF LOG = logging.getLogger(__name__) @@ -36,7 +39,7 @@ class ValidateDeploymentConfigurationBasic(DocumentValidator): def __init__(self, **kwargs): super().__init__(**kwargs) - schema = "shipyard/DeploymentConfiguration/v1" + schema = CONF.document_info.deployment_configuration_schema missing_severity = "Error" def do_validate(self): diff --git a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_strategy.py b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_strategy.py index a31a8f39..4326f697 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_strategy.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/control/validators/validate_deployment_strategy.py @@ -18,6 +18,8 @@ is performed by Deckhand on Shipyard's behalf. """ import logging +from oslo_config import cfg + from shipyard_airflow.common.deployment_group.deployment_group_manager import ( DeploymentGroupManager ) @@ -35,6 +37,7 @@ from shipyard_airflow.control.helpers.design_reference_helper import ( DesignRefHelper ) LOG = logging.getLogger(__name__) +CONF = cfg.CONF def _get_node_lookup(revision_id): @@ -56,7 +59,7 @@ class ValidateDeploymentStrategy(DocumentValidator): def __init__(self, **kwargs): super().__init__(**kwargs) - schema = "shipyard/DeploymentStrategy/v1" + schema = CONF.document_info.deployment_strategy_schema missing_severity = "Error" def do_validate(self): diff --git a/src/bin/shipyard_airflow/shipyard_airflow/plugins/deployment_configuration_operator.py b/src/bin/shipyard_airflow/shipyard_airflow/plugins/deployment_configuration_operator.py index c6f38573..277eade0 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/plugins/deployment_configuration_operator.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/plugins/deployment_configuration_operator.py @@ -13,9 +13,10 @@ # limitations under the License. """Deployment Configuration -Retrieves the deployment configuration from Deckhand and places the values +Retrieves the deployment-configuration from Deckhand and places the values retrieved into a dictionary """ +import configparser import logging from airflow.exceptions import AirflowException @@ -34,12 +35,13 @@ except ImportError: from shipyard_airflow.shipyard_const import CustomHeaders LOG = logging.getLogger(__name__) +DOCUMENT_INFO = 'document_info' class DeploymentConfigurationOperator(BaseOperator): """Deployment Configuration Operator - Retrieve the deployment configuration from Deckhand for use throughout + Retrieve the deployment-configuration from Deckhand for use throughout the workflow. Put the configuration into a dictionary. Failures are raised: @@ -89,18 +91,23 @@ class DeploymentConfigurationOperator(BaseOperator): :param main_dag_name: Parent Dag :param shipyard_conf: Location of shipyard.conf """ - super(DeploymentConfigurationOperator, self).__init__(*args, **kwargs) self.main_dag_name = main_dag_name self.shipyard_conf = shipyard_conf self.action_info = {} + def _read_config(self): + """Read in and parse the shipyard config""" + self.config = configparser.ConfigParser() + self.config.read(self.shipyard_conf) + def execute(self, context): """Perform Deployment Configuration extraction""" - + self._read_config() revision_id = self.get_revision_id(context.get('task_instance')) doc = self.get_doc(revision_id) converted = self.map_config_keys(doc) + # return the mapped configuration so that it can be placed on xcom return converted @@ -116,7 +123,7 @@ class DeploymentConfigurationOperator(BaseOperator): revision_id = self.action_info['committed_rev_id'] if revision_id: - LOG.info("Revision is set to: %s for deployment configuration", + LOG.info("Revision is set to: %s for deployment-configuration", revision_id) return revision_id # either revision id was not on xcom, or the task_instance is messed @@ -127,14 +134,16 @@ class DeploymentConfigurationOperator(BaseOperator): def get_doc(self, revision_id): """Get the DeploymentConfiguration document dictionary from Deckhand""" - LOG.info( - "Attempting to retrieve shipyard/DeploymentConfiguration/v1, " - "deployment-configuration from Deckhand" - ) - filters = { - "schema": "shipyard/DeploymentConfiguration/v1", - "metadata.name": "deployment-configuration" - } + schema_fallback = 'shipyard/DeploymentConfiguration/v1' + schema = self.config.get(DOCUMENT_INFO, + 'deployment_configuration_schema', + fallback=schema_fallback) + name = self.config.get(DOCUMENT_INFO, + 'deployment_configuration_name', + fallback='deployment-configuration') + LOG.info("Attempting to retrieve {}, {} from Deckhand".format(schema, + name)) + filters = {"schema": schema, "metadata.name": name} # Create additional headers dict to pass context marker # and end user @@ -160,7 +169,7 @@ class DeploymentConfigurationOperator(BaseOperator): except AttributeError: failed_url = "No URL generated" LOG.exception(ex) - raise AirflowException("Failed to retrieve deployment " + raise AirflowException("Failed to retrieve deployment-" "configuration yaml using url: " "{}".format(failed_url)) @@ -178,7 +187,7 @@ class DeploymentConfigurationOperator(BaseOperator): Converts to a more simple map of key-value pairs """ - LOG.info("Mapping keys from deployment configuration") + LOG.info("Mapping keys from deployment-configuration") return { cfg_key: self.get_cfg_value(cfg_data, cfg_key, cfg_default) for cfg_key, cfg_default in diff --git a/src/bin/shipyard_airflow/shipyard_airflow/plugins/drydock_nodes.py b/src/bin/shipyard_airflow/shipyard_airflow/plugins/drydock_nodes.py index d5f917d4..96fa40c0 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/plugins/drydock_nodes.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/plugins/drydock_nodes.py @@ -53,6 +53,7 @@ except ImportError: ) LOG = logging.getLogger(__name__) +DOCUMENT_INFO = 'document_info' class DrydockNodesOperator(DrydockBaseOperator): @@ -287,10 +288,12 @@ class DrydockNodesOperator(DrydockBaseOperator): strat_name = self.dc['physical_provisioner.deployment_strategy'] if strat_name: # if there is a deployment strategy specified, use it - strategy = self.get_unique_doc( - name=strat_name, - schema="shipyard/DeploymentStrategy/v1" - ) + schema_fallback = 'shipyard/DeploymentStrategy/v1' + schema = self.config.get(DOCUMENT_INFO, + 'deployment_strategy_schema', + fallback=schema_fallback) + + strategy = self.get_unique_doc(name=strat_name, schema=schema) else: # The default behavior is to deploy all nodes, and fail if # any nodes fail to deploy. diff --git a/src/bin/shipyard_airflow/shipyard_airflow/plugins/ucp_base_operator.py b/src/bin/shipyard_airflow/shipyard_airflow/plugins/ucp_base_operator.py index 57ae8185..051cf7c1 100644 --- a/src/bin/shipyard_airflow/shipyard_airflow/plugins/ucp_base_operator.py +++ b/src/bin/shipyard_airflow/shipyard_airflow/plugins/ucp_base_operator.py @@ -253,10 +253,7 @@ class UcpBaseOperator(BaseOperator): if revision_id is None: revision_id = self.revision_id - LOG.info( - "Retrieve shipyard/DeploymentConfiguration/v1, " - "deployment-configuration from Deckhand" - ) + LOG.info("Retrieve {}, {} from Deckhand".format(schema, name)) try: return self.doc_utils.get_unique_doc(revision_id=revision_id, name=name, diff --git a/src/bin/shipyard_airflow/tests/unit/plugins/test_deployment_configuration_operator.py b/src/bin/shipyard_airflow/tests/unit/plugins/test_deployment_configuration_operator.py index c0b51d03..a5b56e94 100644 --- a/src/bin/shipyard_airflow/tests/unit/plugins/test_deployment_configuration_operator.py +++ b/src/bin/shipyard_airflow/tests/unit/plugins/test_deployment_configuration_operator.py @@ -11,6 +11,7 @@ # WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. # See the License for the specific language governing permissions and # limitations under the License. +from configparser import ConfigParser from unittest import mock import pytest @@ -40,11 +41,13 @@ ACTION_INFO_NO_COMMIT = { try: from deployment_configuration_operator import ( - DeploymentConfigurationOperator + DeploymentConfigurationOperator, + DOCUMENT_INFO ) except ImportError: from shipyard_airflow.plugins.deployment_configuration_operator import ( - DeploymentConfigurationOperator + DeploymentConfigurationOperator, + DOCUMENT_INFO ) try: @@ -55,6 +58,21 @@ except ImportError: ) +def make_fake_config(): + """Make/return a fake config using configparser that we can use for testing + + :returns: A fake configuration object + :rtype: ConfigParser + """ + cfg = ConfigParser() + cfg.add_section(DOCUMENT_INFO) + cfg.set(DOCUMENT_INFO, 'deployment_configuration_name', + 'deployment-configuration') + cfg.set(DOCUMENT_INFO, 'deployment_configuration_schema', + 'shipyard/DeploymentConfiguration/v1') + return cfg + + def test_execute_exception(): """Test that execute results in a failure with bad context""" @@ -67,18 +85,21 @@ def test_execute_exception(): assert ("Design_revision is not set. Cannot proceed with retrieval" " of the design configuration") in str(expected_exc) - +@mock.patch.object(DeploymentConfigurationOperator, '_read_config') @mock.patch.object(DeploymentConfigurationOperator, 'get_revision_id', return_value=99) -def test_execute_no_client(*args): +def test_execute_no_client(get_revision_id, read_config): # no keystone authtoken present in configuration dco = DeploymentConfigurationOperator(main_dag_name="main", shipyard_conf="shipyard.conf", task_id="t1") + dco.config = make_fake_config() with pytest.raises(AirflowException) as expected_exc: - dco.execute(context={}) - assert ("Failed to retrieve deployment configuration yaml") in str( + dco.execute(context={'task_instance': 'asdf'}) + assert ("Failed to retrieve deployment-configuration yaml") in str( expected_exc) + get_revision_id.assert_called_once_with('asdf') + read_config.assert_called_once_with() @mock.patch.object(airflow.models.TaskInstance, 'xcom_pull', @@ -113,6 +134,7 @@ def test_get_doc_no_deckhand(): dco = DeploymentConfigurationOperator(main_dag_name="main", shipyard_conf="shipyard.conf", task_id="t1") + dco.config = make_fake_config() with pytest.raises(AirflowException) as expected_exc: dco.get_doc(99) assert "Failed to retrieve deployment" in str(expected_exc) @@ -134,7 +156,7 @@ def test_get_doc_mock_deckhand(*args): dco = DeploymentConfigurationOperator(main_dag_name="main", shipyard_conf="shipyard.conf", task_id="t1") - + dco.config = make_fake_config() doc = dco.get_doc(99) assert doc == 'abcdefg' @@ -146,6 +168,7 @@ def test_get_doc_mock_deckhand_invalid(*args): dco = DeploymentConfigurationOperator(main_dag_name="main", shipyard_conf="shipyard.conf", task_id="t1") + dco.config = make_fake_config() with pytest.raises(AirflowException) as airflow_ex: dco.get_doc(99) diff --git a/tools/resources/shipyard.conf b/tools/resources/shipyard.conf index fcd4d813..dee194e3 100644 --- a/tools/resources/shipyard.conf +++ b/tools/resources/shipyard.conf @@ -45,3 +45,7 @@ service_type = shipyard [oslo_policy] policy_file = /etc/shipyard/policy.yaml policy_default_rule = deny_all +[document_info] +deployment_configuration_name = deployment-configuration +deployment_configuration_schema = shipyard/DeploymentConfiguration/v1 +deployment_strategy_schema = shipyard/DeploymentStrategy/v1 \ No newline at end of file