diff --git a/sahara/conductor/manager.py b/sahara/conductor/manager.py index a9f48708..49532382 100644 --- a/sahara/conductor/manager.py +++ b/sahara/conductor/manager.py @@ -261,7 +261,8 @@ class ConductorManager(db_base.Base): values['tenant_id'] = context.tenant_id values['id'] = id - values['node_groups'] = self._populate_node_groups(context, values) + if 'node_groups' in values: + values['node_groups'] = self._populate_node_groups(context, values) return self.db.cluster_template_update(context, values, ignore_default) diff --git a/sahara/db/sqlalchemy/api.py b/sahara/db/sqlalchemy/api.py index ae8276ec..b829e74e 100644 --- a/sahara/db/sqlalchemy/api.py +++ b/sahara/db/sqlalchemy/api.py @@ -441,7 +441,11 @@ def cluster_template_destroy(context, cluster_template_id, def cluster_template_update(context, values, ignore_default=False): - node_groups = values.pop("node_groups", []) + explicit_node_groups = "node_groups" in values + if explicit_node_groups: + node_groups = values.pop("node_groups") + if node_groups is None: + node_groups = [] session = get_session() cluster_template_id = values['id'] @@ -475,7 +479,7 @@ def cluster_template_update(context, values, ignore_default=False): # If node_groups has not been specified, then we are # keeping the old ones so don't delete! - if node_groups: + if explicit_node_groups: model_query(m.TemplatesRelation, context, session=session).filter_by( cluster_template_id=cluster_template_id).delete() diff --git a/sahara/db/templates/api.py b/sahara/db/templates/api.py index 7398d0f1..efcd6e10 100644 --- a/sahara/db/templates/api.py +++ b/sahara/db/templates/api.py @@ -261,13 +261,7 @@ def substitute_config_values(configs, template, path): for opt, value in six.iteritems(configs): if opt in opt_names and opt in template: if value is None: - # TODO(tmckay): someday if we support 'null' in JSON - # we should replace this value with None. json.load - # will replace 'null' with None, and sqlalchemy will - # accept None as a value for a nullable field. - del template[opt] - LOG.debug("No replacement value specified for {opt} in " - "{path}, removing".format(opt=opt, path=path)) + template[opt] = None else: # Use args to allow for keyword arguments to format args = {opt: value} diff --git a/sahara/service/validations/cluster_template_schema.py b/sahara/service/validations/cluster_template_schema.py index c23e85c1..9eaac461 100644 --- a/sahara/service/validations/cluster_template_schema.py +++ b/sahara/service/validations/cluster_template_schema.py @@ -63,30 +63,30 @@ CLUSTER_TEMPLATE_SCHEMA = { "type": "string", }, "default_image_id": { - "type": "string", + "type": ["string", "null"], "format": "uuid", }, "cluster_configs": { - "type": "configs", + "type": ["configs", "null"], }, "node_groups": { - "type": "array", + "type": ["array", "null"], "items": { "oneOf": [_cluster_tmpl_ng_tmpl_schema, _cluster_tmpl_ng_schema] } }, "anti_affinity": { - "type": "array", + "type": ["array", "null"], "items": { "type": "string", }, }, "description": { - "type": "string", + "type": ["string", "null"], }, "neutron_management_network": { - "type": "string", + "type": ["string", "null"], "format": "uuid" }, }, diff --git a/sahara/service/validations/node_group_template_schema.py b/sahara/service/validations/node_group_template_schema.py index 8758cd33..81d195e6 100644 --- a/sahara/service/validations/node_group_template_schema.py +++ b/sahara/service/validations/node_group_template_schema.py @@ -41,53 +41,51 @@ NODE_GROUP_TEMPLATE_SCHEMA = { "minItems": 1 }, "image_id": { - "type": "string", + "type": ["string", "null"], "format": "uuid", }, "node_configs": { - "type": "configs", + "type": ["configs", "null"], }, "volumes_per_node": { "type": "integer", "minimum": 0, }, "volumes_size": { - "type": "integer", + "type": ["integer", "null"], "minimum": 1, }, "volume_type": { - "type": "string" + "type": ["string", "null"], }, "volumes_availability_zone": { - "type": "string", + "type": ["string", "null"], }, "volume_mount_prefix": { - "type": "string", + "type": ["string", "null"], "format": "posix_path", }, "description": { - "type": "string", + "type": ["string", "null"], }, "floating_ip_pool": { - "type": "string", + "type": ["string", "null"], }, "security_groups": { - "type": "array", - "items": { - "type": "string", - }, + "type": ["array", "null"], + "items": {"type": "string"} }, "auto_security_group": { - "type": "boolean" + "type": ["boolean", "null"], }, "availability_zone": { - "type": "string", + "type": ["string", "null"], }, "is_proxy_gateway": { - "type": "boolean" + "type": ["boolean", "null"], }, "volume_local_to_instance": { - "type": "boolean" + "type": ["boolean", "null"] }, }, "additionalProperties": False, diff --git a/sahara/tests/unit/conductor/manager/test_templates.py b/sahara/tests/unit/conductor/manager/test_templates.py index 8bbd004d..27915f35 100644 --- a/sahara/tests/unit/conductor/manager/test_templates.py +++ b/sahara/tests/unit/conductor/manager/test_templates.py @@ -14,26 +14,28 @@ # limitations under the License. import copy +import uuid +import six from sqlalchemy import exc as sa_ex import testtools from sahara.conductor import manager from sahara import context from sahara import exceptions as ex +from sahara.service.validations import cluster_template_schema as cl_schema +from sahara.service.validations import node_group_template_schema as ngt_schema import sahara.tests.unit.conductor.base as test_base import sahara.tests.unit.conductor.manager.test_clusters as cluster_tests SAMPLE_NGT = { - "plugin_name": "test_plugin", - "flavor_id": "42", - "tenant_id": "tenant_1", - "hadoop_version": "test_version", "name": "ngt_test", + "flavor_id": "42", + "plugin_name": "test_plugin", + "hadoop_version": "test_version", "node_processes": ["p1", "p2"], - "floating_ip_pool": None, - "availability_zone": None, + "image_id": str(uuid.uuid4()), "node_configs": { "service_1": { "config_1": "value_1" @@ -41,14 +43,26 @@ SAMPLE_NGT = { "service_2": { "config_1": "value_1" } - } + }, + "volumes_per_node": 1, + "volumes_size": 1, + "volume_type": "big", + "volumes_availability_zone": "here", + "volume_mount_prefix": "/tmp", + "description": "my template", + "floating_ip_pool": "public", + "security_groups": ["cat", "dog"], + "auto_security_group": False, + "availability_zone": "here", + "is_proxy_gateway": False, + "volume_local_to_instance": False } SAMPLE_CLT = { - "plugin_name": "test_plugin", - "tenant_id": "tenant_1", - "hadoop_version": "test_version", "name": "clt_test", + "plugin_name": "test_plugin", + "hadoop_version": "test_version", + "default_image_id": str(uuid.uuid4()), "cluster_configs": { "service_1": { "config_1": "value_1" @@ -77,7 +91,10 @@ SAMPLE_CLT = { "availability_zone": None, } - ] + ], + "anti_affinity": ["datanode"], + "description": "my template", + "neutron_management_network": str(uuid.uuid4()) } @@ -213,6 +230,28 @@ class NodeGroupTemplates(test_base.ConductorManagerTestCase): ignore_default=True) self.assertEqual(UPDATE_NAME, updated_ngt["name"]) + def test_ngt_update_with_nulls(self): + ctx = context.ctx() + ngt = self.api.node_group_template_create(ctx, SAMPLE_NGT) + ngt_id = ngt["id"] + + updated_values = copy.deepcopy(SAMPLE_NGT) + for prop, value in six.iteritems( + ngt_schema.NODE_GROUP_TEMPLATE_SCHEMA["properties"]): + if type(value["type"]) is list and "null" in value["type"]: + updated_values[prop] = None + + # Prove that we can call update on these fields with null values + # without an exception + self.api.node_group_template_update(ctx, + ngt_id, + updated_values) + + updated_ngt = self.api.node_group_template_get(ctx, ngt_id) + for prop, value in six.iteritems(updated_values): + if value is None: + self.assertIsNone(updated_ngt[prop]) + class ClusterTemplates(test_base.ConductorManagerTestCase): def __init__(self, *args, **kwargs): @@ -383,3 +422,30 @@ class ClusterTemplates(test_base.ConductorManagerTestCase): update_values, ignore_default=True) self.assertEqual(UPDATE_NAME, updated_clt["name"]) + + def test_clt_update_with_nulls(self): + ctx = context.ctx() + clt = self.api.cluster_template_create(ctx, SAMPLE_CLT) + clt_id = clt["id"] + + updated_values = copy.deepcopy(SAMPLE_CLT) + for prop, value in six.iteritems( + cl_schema.CLUSTER_TEMPLATE_SCHEMA["properties"]): + if type(value["type"]) is list and "null" in value["type"]: + updated_values[prop] = None + + # Prove that we can call update on these fields with null values + # without an exception + self.api.cluster_template_update(ctx, + clt_id, + updated_values) + + updated_clt = self.api.cluster_template_get(ctx, clt_id) + for prop, value in six.iteritems(updated_values): + if value is None: + # Conductor populates node groups with [] when + # the value given is null + if prop == "node_groups": + self.assertEqual([], updated_clt[prop]) + else: + self.assertIsNone(updated_clt[prop]) diff --git a/sahara/tests/unit/db/templates/test_update.py b/sahara/tests/unit/db/templates/test_update.py index 26a0c483..33298f6b 100644 --- a/sahara/tests/unit/db/templates/test_update.py +++ b/sahara/tests/unit/db/templates/test_update.py @@ -204,7 +204,7 @@ class TemplateUpdateTestCase(base.ConductorManagerTestCase): "floating_ip_pool": None} template_api.substitute_config_values(configs, ngt, "/path") self.assertEqual("2", ngt["flavor_id"]) - self.assertNotIn("floating_ip_pool", ngt) + self.assertIsNone(ngt["floating_ip_pool"]) def test_substitute_config_values_clt(self): clt = copy.copy(c.SAMPLE_CLT) @@ -216,7 +216,7 @@ class TemplateUpdateTestCase(base.ConductorManagerTestCase): "default_image_id": None} template_api.substitute_config_values(configs, clt, "/path") self.assertEqual(netid, clt["neutron_management_network"]) - self.assertNotIn("default_image_id", clt) + self.assertIsNone(clt["default_image_id"]) def _write_files(self, tempdir, templates): files = [] diff --git a/sahara/tests/unit/service/validation/test_cluster_template_create_validation.py b/sahara/tests/unit/service/validation/test_cluster_template_create_validation.py index 473aa02d..36e2427a 100644 --- a/sahara/tests/unit/service/validation/test_cluster_template_create_validation.py +++ b/sahara/tests/unit/service/validation/test_cluster_template_create_validation.py @@ -13,6 +13,10 @@ # See the License for the specific language governing permissions and # limitations under the License. +import uuid + +import mock + from sahara.service import api from sahara.service.validations import cluster_template_schema as ct_schema from sahara.service.validations import cluster_templates as ct @@ -194,12 +198,52 @@ class TestClusterTemplateCreateValidation(u.ValidationTestCase): u"'hadoop_version' is a required property") ) - def test_cluster_template_create_v_right(self): + @mock.patch("sahara.service.validations.base.check_all_configurations") + @mock.patch("sahara.service.validations.base.check_network_exists") + @mock.patch("sahara.service.validations.base.check_required_image_tags") + @mock.patch("sahara.service.validations.base.check_image_registered") + def test_cluster_template_create_v_right(self, image_reg, image_tags, + net_exists, all_configs): self._assert_create_object_validation( data={ 'name': 'testname', 'plugin_name': 'vanilla', - 'hadoop_version': '1.2.1' + 'hadoop_version': '1.2.1', + 'default_image_id': str(uuid.uuid4()), + 'cluster_configs': { + "service_1": { + "config_1": "value_1" + } + }, + 'node_groups': [ + { + "node_group_template_id": "550e8400-e29b-41d4-a716-" + "446655440000", + "name": "charlie", + 'count': 3 + } + ], + 'anti_affinity': ['datanode'], + 'description': 'my template', + 'neutron_management_network': str(uuid.uuid4()) + }) + + @mock.patch("sahara.service.validations.base.check_network_exists") + @mock.patch("sahara.service.validations.base.check_required_image_tags") + @mock.patch("sahara.service.validations.base.check_image_registered") + def test_cluster_template_create_v_right_nulls(self, image_reg, image_tags, + net_exists): + self._assert_create_object_validation( + data={ + 'name': 'testname', + 'plugin_name': 'vanilla', + 'hadoop_version': '1.2.1', + 'default_image_id': None, + 'cluster_configs': None, + 'node_groups': None, + 'anti_affinity': None, + 'description': None, + 'neutron_management_network': None }) def test_cluster_template_create_v_plugin_name_exists(self): diff --git a/sahara/tests/unit/service/validation/test_ng_template_validation_create.py b/sahara/tests/unit/service/validation/test_ng_template_validation_create.py index d3cf9160..c2d4943a 100644 --- a/sahara/tests/unit/service/validation/test_ng_template_validation_create.py +++ b/sahara/tests/unit/service/validation/test_ng_template_validation_create.py @@ -13,6 +13,8 @@ # See the License for the specific language governing permissions and # limitations under the License. +import mock + from sahara.service import api from sahara.service.validations import node_group_template_schema as ngt_schema from sahara.service.validations import node_group_templates as nt @@ -106,7 +108,14 @@ class TestNGTemplateCreateValidation(u.ValidationTestCase): "['wrong_process']") ) - def test_ng_template_create_v_right(self): + @mock.patch( + "sahara.service.validations.base.check_volume_availability_zone_exist") + @mock.patch("sahara.service.validations.base.check_volume_type_exists") + @mock.patch( + "sahara.service.validations.base.check_availability_zone_exist") + @mock.patch("sahara.service.validations.base.check_security_groups_exist") + def test_ng_template_create_v_right(self, + s_groups, a_zone, v_type, v_a_zone): self._assert_create_object_validation( data={ 'name': 'a', @@ -118,16 +127,53 @@ class TestNGTemplateCreateValidation(u.ValidationTestCase): 'secondarynamenode', 'tasktracker', 'jobtracker'], + 'image_id': '550e8400-e29b-41d4-a716-446655440000', 'node_configs': { 'HDFS': { u'hadoop.tmp.dir': '/temp/' } }, - 'image_id': '550e8400-e29b-41d4-a716-446655440000', 'volumes_per_node': 2, 'volumes_size': 10, - 'description': 'test node template', - 'floating_ip_pool': 'd9a3bebc-f788-4b81-9a93-aa048022c1ca' + 'volume_type': 'fish', + 'volumes_availability_zone': 'ocean', + 'volume_mount_prefix': '/tmp', + 'description': "my node group", + 'floating_ip_pool': 'd9a3bebc-f788-4b81-9a93-aa048022c1ca', + 'security_groups': ['cat', 'dog'], + 'auto_security_group': False, + 'availability_zone': 'here', + 'is_proxy_gateway': False, + 'volume_local_to_instance': False + } + ) + + def test_ng_template_create_v_nulls(self): + self._assert_create_object_validation( + data={ + 'name': 'a', + 'flavor_id': '42', + 'plugin_name': 'vanilla', + 'hadoop_version': '1.2.1', + 'node_processes': ['namenode', + 'datanode', + 'secondarynamenode', + 'tasktracker', + 'jobtracker'], + + 'image_id': None, + 'node_configs': None, + 'volumes_size': None, + 'volume_type': None, + 'volumes_availability_zone': None, + 'volume_mount_prefix': None, + 'description': None, + 'floating_ip_pool': None, + 'security_groups': None, + 'auto_security_group': None, + 'availability_zone': None, + 'is_proxy_gateway': None, + 'volume_local_to_instance': None } ) diff --git a/sahara/tests/unit/service/validation/utils.py b/sahara/tests/unit/service/validation/utils.py index 109be6bb..9b2becf1 100644 --- a/sahara/tests/unit/service/validation/utils.py +++ b/sahara/tests/unit/service/validation/utils.py @@ -30,10 +30,11 @@ from sahara.tests.unit import testutils as tu m = {} _types_checks = { - "string": [1, (), {}], - "integer": ["a", (), {}], - "uuid": ["z550e8400-e29b-41d4-a716-446655440000", 1, "a", (), {}], - "array": [{}, 'a', 1] + "string": [1, (), {}, True], + "integer": ["a", (), {}, True], + "uuid": ["z550e8400-e29b-41d4-a716-446655440000", 1, "a", (), {}, True], + "array": [{}, 'a', 1, True], + "boolean": [1, 'a', (), {}] } @@ -332,26 +333,34 @@ class ValidationTestCase(base.SaharaTestCase): u"'a-!' is not a 'valid_name_hostname'") ) + def _prop_types_str(self, prop_types): + return ", ".join(["'%s'" % prop for prop in prop_types]) + def _assert_types(self, default_data): for p_name in self.scheme['properties']: prop = self.scheme['properties'][p_name] - if prop["type"] in _types_checks: - for type_ex in _types_checks[prop["type"]]: - data = default_data.copy() - value = type_ex - value_str = str(value) - if isinstance(value, str): - value_str = "'%s'" % value_str - data.update({p_name: value}) - message = ("%s is not of type '%s'" % - (value_str, prop["type"])) - if "enum" in prop: - message = [message, "%s is not one of %s" % - (value_str, prop["enum"])] - self._assert_create_object_validation( - data=data, - bad_req_i=(1, 'VALIDATION_ERROR', message) - ) + prop_types = prop["type"] + if type(prop_types) is not list: + prop_types = [prop_types] + for prop_type in prop_types: + if prop_type in _types_checks: + for type_ex in _types_checks[prop_type]: + data = default_data.copy() + value = type_ex + value_str = str(value) + if isinstance(value, str): + value_str = "'%s'" % value_str + data.update({p_name: value}) + message = ("%s is not of type %s" % + (value_str, + self._prop_types_str(prop_types))) + if "enum" in prop: + message = [message, "%s is not one of %s" % + (value_str, prop["enum"])] + self._assert_create_object_validation( + data=data, + bad_req_i=(1, 'VALIDATION_ERROR', message) + ) def _assert_cluster_configs_validation(self, require_image_id=False): data = {