From 7929361a0b399feaa466c51c7257dcf6a5328aec Mon Sep 17 00:00:00 2001 From: Jim Rollenhagen Date: Mon, 2 Jul 2018 15:44:41 +0000 Subject: [PATCH] Add conductor_group field to config, node and conductor objects Adds the fields and bumps the objects versions. Excludes the field from the node API for now. Also adds the conductor_group config option, and populates the field in the conductors table. Also fixes a fundamentally broken test in ironic.tests.unit.db.test_api. Change-Id: Ice2f90f7739b2927712ed45c969865136a216bd6 Story: 2001795 Task: 22640 Task: 22642 --- ironic/api/controllers/v1/node.py | 6 ++ ironic/common/exception.py | 4 + ironic/common/release_mappings.py | 4 +- ironic/common/utils.py | 7 ++ ironic/conductor/base_manager.py | 5 +- ironic/conf/conductor.py | 6 ++ ironic/objects/conductor.py | 12 ++- ironic/objects/node.py | 36 +++++++- ironic/tests/unit/common/test_utils.py | 21 +++++ ironic/tests/unit/db/test_api.py | 6 +- ironic/tests/unit/db/utils.py | 2 + ironic/tests/unit/objects/test_conductor.py | 25 +++++- ironic/tests/unit/objects/test_node.py | 95 +++++++++++++++++++++ ironic/tests/unit/objects/test_objects.py | 4 +- 14 files changed, 222 insertions(+), 11 deletions(-) diff --git a/ironic/api/controllers/v1/node.py b/ironic/api/controllers/v1/node.py index 1a52e2fea9..372dcbc768 100644 --- a/ironic/api/controllers/v1/node.py +++ b/ironic/api/controllers/v1/node.py @@ -172,6 +172,9 @@ def hide_fields_in_newer_versions(obj): if not api_utils.allow_bios_interface(): obj.bios_interface = wsme.Unset + # TODO(jroll) add a microversion here + obj.conductor_group = wsme.Unset + def update_state_in_older_versions(obj): """Change provision state names for API backwards compatibility. @@ -1074,6 +1077,9 @@ class Node(base.APIBase): bios_interface = wsme.wsattr(wtypes.text) """The bios interface to be used for this node""" + conductor_group = wsme.wsattr(wtypes.text) + """The conductor group to manage this node""" + # NOTE(deva): "conductor_affinity" shouldn't be presented on the # API because it's an internal value. Don't add it here. diff --git a/ironic/common/exception.py b/ironic/common/exception.py index 78970d0d61..d5722e36d2 100644 --- a/ironic/common/exception.py +++ b/ironic/common/exception.py @@ -254,6 +254,10 @@ class InvalidName(Invalid): _msg_fmt = _("Expected a logical name but received %(name)s.") +class InvalidConductorGroup(Invalid): + _msg_fmt = _("Expected a conductor group but received %(group)s.") + + class InvalidIdentity(Invalid): _msg_fmt = _("Expected a UUID or int but received %(identity)s.") diff --git a/ironic/common/release_mappings.py b/ironic/common/release_mappings.py index dde8acd47c..c423d36ee1 100644 --- a/ironic/common/release_mappings.py +++ b/ironic/common/release_mappings.py @@ -118,8 +118,8 @@ RELEASE_MAPPING = { 'api': '1.45', 'rpc': '1.46', 'objects': { - 'Node': ['1.26'], - 'Conductor': ['1.2'], + 'Node': ['1.26', '1.27'], + 'Conductor': ['1.3'], 'Chassis': ['1.3'], 'Port': ['1.8'], 'Portgroup': ['1.4'], diff --git a/ironic/common/utils.py b/ironic/common/utils.py index 5b6865481f..fdb4ae48c2 100644 --- a/ironic/common/utils.py +++ b/ironic/common/utils.py @@ -547,3 +547,10 @@ def parse_instance_info_capabilities(node): parse_error() return capabilities + + +def validate_conductor_group(conductor_group): + if not isinstance(conductor_group, six.string_types): + raise exception.InvalidConductorGroup(group=conductor_group) + if not re.match(r'^[a-zA-Z0-9_\-\.]*$', conductor_group): + raise exception.InvalidConductorGroup(group=conductor_group) diff --git a/ironic/conductor/base_manager.py b/ironic/conductor/base_manager.py index ef5de34c83..2fe5217f79 100644 --- a/ironic/conductor/base_manager.py +++ b/ironic/conductor/base_manager.py @@ -158,7 +158,8 @@ class BaseConductorManager(object): try: # Register this conductor with the cluster self.conductor = objects.Conductor.register( - admin_context, self.host, hardware_type_names) + admin_context, self.host, hardware_type_names, + CONF.conductor.conductor_group) except exception.ConductorAlreadyRegistered: # This conductor was already registered and did not shut down # properly, so log a warning and update the record. @@ -167,7 +168,7 @@ class BaseConductorManager(object): {'hostname': self.host}) self.conductor = objects.Conductor.register( admin_context, self.host, hardware_type_names, - update_existing=True) + CONF.conductor.conductor_group, update_existing=True) # register hardware types and interfaces supported by this conductor # and validate them against other conductors diff --git a/ironic/conf/conductor.py b/ironic/conf/conductor.py index 34a41f436e..3b2ae1d570 100644 --- a/ironic/conf/conductor.py +++ b/ironic/conf/conductor.py @@ -177,6 +177,12 @@ opts = [ 'automatically moved out of maintenance mode once its ' 'power state is retrieved successfully. Set to 0 to ' 'disable this check.')), + cfg.StrOpt('conductor_group', + default='', + help=_('Name of the conductor group to join. Can be up to ' + '255 characters and is case insensitive. This ' + 'conductor will only manage nodes with a matching ' + '"conductor_group" field set on the node.')), ] diff --git a/ironic/objects/conductor.py b/ironic/objects/conductor.py index be1154ec74..78c8937a2e 100644 --- a/ironic/objects/conductor.py +++ b/ironic/objects/conductor.py @@ -17,6 +17,7 @@ from oslo_versionedobjects import base as object_base from ironic.common.i18n import _ +from ironic.common import utils from ironic.db import api as db_api from ironic.objects import base from ironic.objects import fields as object_fields @@ -29,7 +30,8 @@ class Conductor(base.IronicObject, object_base.VersionedObjectDictCompat): # to touch() optional. # Version 1.2: Add register_hardware_interfaces() and # unregister_all_hardware_interfaces() - VERSION = '1.2' + # Version 1.3: Add conductor_group field. + VERSION = '1.3' dbapi = db_api.get_instance() @@ -37,6 +39,7 @@ class Conductor(base.IronicObject, object_base.VersionedObjectDictCompat): 'id': object_fields.IntegerField(), 'drivers': object_fields.ListOfStringsField(nullable=True), 'hostname': object_fields.StringField(), + 'conductor_group': object_fields.StringField(), } # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable @@ -95,13 +98,16 @@ class Conductor(base.IronicObject, object_base.VersionedObjectDictCompat): # Implications of calling new remote procedures should be thought through. # @object_base.remotable @classmethod - def register(cls, context, hostname, drivers, update_existing=False): + def register(cls, context, hostname, drivers, conductor_group, + update_existing=False): """Register an active conductor with the cluster. :param cls: the :class:`Conductor` :param context: Security context :param hostname: the hostname on which the conductor will run :param drivers: the list of drivers enabled in the conductor + :param conductor_group: conductor group to join, used for + node:conductor affinity. :param update_existing: When false, registration will raise an exception when a conflicting online record is found. When true, will overwrite the @@ -110,9 +116,11 @@ class Conductor(base.IronicObject, object_base.VersionedObjectDictCompat): :returns: a :class:`Conductor` object. """ + utils.validate_conductor_group(conductor_group) db_cond = cls.dbapi.register_conductor( {'hostname': hostname, 'drivers': drivers, + 'conductor_group': conductor_group.lower(), 'version': cls.get_target_version()}, update_existing=update_existing) return cls._from_db_object(context, cls(), db_cond) diff --git a/ironic/objects/node.py b/ironic/objects/node.py index 8afc57e8d5..47d88fc796 100644 --- a/ironic/objects/node.py +++ b/ironic/objects/node.py @@ -20,6 +20,7 @@ from oslo_versionedobjects import base as object_base from ironic.common import exception from ironic.common.i18n import _ +from ironic.common import utils from ironic.db import api as db_api from ironic import objects from ironic.objects import base @@ -62,7 +63,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # Version 1.24: Add bios_interface field # Version 1.25: Add fault field # Version 1.26: Add deploy_step field - VERSION = '1.26' + # Version 1.27: Add conductor_group field + VERSION = '1.27' dbapi = db_api.get_instance() @@ -98,6 +100,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): # that has most recently performed some action which could require # local state to be maintained (eg, built a PXE config) 'conductor_affinity': object_fields.IntegerField(nullable=True), + 'conductor_group': object_fields.StringField(nullable=True), # One of states.POWER_ON|POWER_OFF|NOSTATE|ERROR 'power_state': object_fields.StringField(nullable=True), @@ -361,6 +364,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): values = self.do_version_changes_for_db() self._validate_property_values(values.get('properties')) self._validate_and_remove_traits(values) + self._validate_and_format_conductor_group(values) db_node = self.dbapi.create_node(values) self._from_db_object(self._context, self, db_node) @@ -408,6 +412,7 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): self.driver_internal_info = {} updates = self.do_version_changes_for_db() self._validate_and_remove_traits(updates) + self._validate_and_format_conductor_group(updates) db_node = self.dbapi.update_node(self.uuid, updates) self._from_db_object(self._context, self, db_node) @@ -431,6 +436,18 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): raise exception.BadRequest() fields.pop('traits') + def _validate_and_format_conductor_group(self, fields): + """Validate conductor_group and format it for our use. + + Currently formatting is just lowercasing it. + + :param fields: a dict of Node fields for create or update. + :raises: InvalidConductorGroup if validation fails. + """ + if 'conductor_group' in fields: + utils.validate_conductor_group(fields['conductor_group']) + fields['conductor_group'] = fields['conductor_group'].lower() + # NOTE(xek): We don't want to enable RPC on this call just yet. Remotable # methods can be used in the future to replace current explicit RPC calls. # Implications of calling new remote procedures should be thought through. @@ -499,6 +516,19 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): elif self.deploy_step: self.deploy_step = {} + def _convert_conductor_group_field(self, target_version, + remove_unavailable_fields=True): + # NOTE(jroll): The default conductor_group is "", not None + is_set = self.obj_attr_is_set('conductor_group') + if target_version >= (1, 27): + if not is_set: + self.conductor_group = '' + elif is_set: + if remove_unavailable_fields: + delattr(self, 'conductor_group') + elif self.conductor_group: + self.conductor_group = '' + def _convert_to_version(self, target_version, remove_unavailable_fields=True): """Convert to the target version. @@ -520,6 +550,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): this, it should be removed. Version 1.26: deploy_step field was added. For versions prior to this, it should be removed. + Version 1.27: conductor_group field was added. For versions prior to + this, it should be removed. :param target_version: the desired version of the object :param remove_unavailable_fields: True to remove fields that are @@ -573,6 +605,8 @@ class Node(base.IronicObject, object_base.VersionedObjectDictCompat): self._convert_fault_field(target_version, remove_unavailable_fields) self._convert_deploy_step_field(target_version, remove_unavailable_fields) + self._convert_conductor_group_field(target_version, + remove_unavailable_fields) @base.IronicObjectRegistry.register diff --git a/ironic/tests/unit/common/test_utils.py b/ironic/tests/unit/common/test_utils.py index 79198dd00e..36af63ed2c 100644 --- a/ironic/tests/unit/common/test_utils.py +++ b/ironic/tests/unit/common/test_utils.py @@ -593,3 +593,24 @@ class JinjaTemplatingTestCase(base.TestCase): utils.render_template(path, self.params)) jinja_fsl_mock.assert_called_once_with('/path/to') + + +class ValidateConductorGroupTestCase(base.TestCase): + def test_validate_conductor_group_success(self): + self.assertIsNone(utils.validate_conductor_group('foo')) + self.assertIsNone(utils.validate_conductor_group('group1')) + self.assertIsNone(utils.validate_conductor_group('group1.with.dot')) + self.assertIsNone(utils.validate_conductor_group('group1_with_under')) + self.assertIsNone(utils.validate_conductor_group('group1-with-dash')) + + def test_validate_conductor_group_fail(self): + self.assertRaises(exception.InvalidConductorGroup, + utils.validate_conductor_group, 'foo:bar') + self.assertRaises(exception.InvalidConductorGroup, + utils.validate_conductor_group, 'foo*bar') + self.assertRaises(exception.InvalidConductorGroup, + utils.validate_conductor_group, 'foo$bar') + self.assertRaises(exception.InvalidConductorGroup, + utils.validate_conductor_group, object()) + self.assertRaises(exception.InvalidConductorGroup, + utils.validate_conductor_group, None) diff --git a/ironic/tests/unit/db/test_api.py b/ironic/tests/unit/db/test_api.py index 2fd8742b39..c454b211eb 100644 --- a/ironic/tests/unit/db/test_api.py +++ b/ironic/tests/unit/db/test_api.py @@ -10,6 +10,8 @@ # License for the specific language governing permissions and limitations # under the License. +import random + import mock from oslo_db.sqlalchemy import utils as db_utils from oslo_utils import uuidutils @@ -61,8 +63,10 @@ class UpgradingTestCase(base.DbTestCase): def test_check_versions_conductor(self): for v in self.object_versions['Conductor']: + # NOTE(jroll) conductor model doesn't have a uuid :( conductor = utils.create_test_conductor( - uuid=uuidutils.generate_uuid(), version=v) + hostname=uuidutils.generate_uuid(), version=v, + id=random.randint(1, 1000000)) conductor = self.dbapi.get_conductor(conductor.hostname) self.assertEqual(v, conductor.version) self.assertTrue(self.dbapi.check_versions()) diff --git a/ironic/tests/unit/db/utils.py b/ironic/tests/unit/db/utils.py index 204b65a0fd..f981c07d8f 100644 --- a/ironic/tests/unit/db/utils.py +++ b/ironic/tests/unit/db/utils.py @@ -182,6 +182,7 @@ def get_test_node(**kw): 'uuid': kw.get('uuid', '1be26c0b-03f2-4d2e-ae87-c02d7f33c123'), 'chassis_id': kw.get('chassis_id', None), 'conductor_affinity': kw.get('conductor_affinity', None), + 'conductor_group': kw.get('conductor_group', ''), 'power_state': kw.get('power_state', states.NOSTATE), 'target_power_state': kw.get('target_power_state', states.NOSTATE), 'provision_state': kw.get('provision_state', states.AVAILABLE), @@ -381,6 +382,7 @@ def get_test_conductor(**kw): 'version': kw.get('version', conductor.Conductor.VERSION), 'hostname': kw.get('hostname', 'test-conductor-node'), 'drivers': kw.get('drivers', ['fake-driver', 'null-driver']), + 'conductor_group': kw.get('conductor_group', ''), 'created_at': kw.get('created_at', timeutils.utcnow()), 'updated_at': kw.get('updated_at', timeutils.utcnow()), } diff --git a/ironic/tests/unit/objects/test_conductor.py b/ironic/tests/unit/objects/test_conductor.py index af65f0deab..a713d5cfc5 100644 --- a/ironic/tests/unit/objects/test_conductor.py +++ b/ironic/tests/unit/objects/test_conductor.py @@ -20,6 +20,7 @@ import types import mock from oslo_utils import timeutils +from ironic.common import exception from ironic import objects from ironic.objects import base from ironic.objects import fields @@ -90,7 +91,8 @@ class TestConductorObject(db_base.DbTestCase): @mock.patch.object(base.IronicObject, 'get_target_version', spec_set=types.FunctionType) - def _test_register(self, mock_target_version, update_existing=False): + def _test_register(self, mock_target_version, update_existing=False, + conductor_group=''): mock_target_version.return_value = '1.5' host = self.fake_conductor['hostname'] drivers = self.fake_conductor['drivers'] @@ -98,12 +100,14 @@ class TestConductorObject(db_base.DbTestCase): autospec=True) as mock_register_cdr: mock_register_cdr.return_value = self.fake_conductor c = objects.Conductor.register(self.context, host, drivers, + conductor_group, update_existing=update_existing) self.assertIsInstance(c, objects.Conductor) mock_register_cdr.assert_called_once_with( {'drivers': drivers, 'hostname': host, + 'conductor_group': conductor_group.lower(), 'version': '1.5'}, update_existing=update_existing) @@ -113,6 +117,25 @@ class TestConductorObject(db_base.DbTestCase): def test_register_update_existing_true(self): self._test_register(update_existing=True) + def test_register_into_group(self): + self._test_register(conductor_group='dc1') + + def test_register_into_group_uppercased(self): + self._test_register(conductor_group='DC1') + + def test_register_into_group_with_update(self): + self._test_register(conductor_group='dc1', update_existing=True) + + @mock.patch.object(base.IronicObject, 'get_target_version', + spec_set=types.FunctionType) + def test_register_with_invalid_group(self, mock_target_version): + mock_target_version.return_value = '1.5' + host = self.fake_conductor['hostname'] + drivers = self.fake_conductor['drivers'] + self.assertRaises(exception.InvalidConductorGroup, + objects.Conductor.register, + self.context, host, drivers, 'invalid:group') + @mock.patch.object(objects.Conductor, 'unregister_all_hardware_interfaces', autospec=True) def test_unregister(self, mock_unreg_ifaces): diff --git a/ironic/tests/unit/objects/test_node.py b/ironic/tests/unit/objects/test_node.py index 29f7d14621..c96d5e4b8c 100644 --- a/ironic/tests/unit/objects/test_node.py +++ b/ironic/tests/unit/objects/test_node.py @@ -216,6 +216,51 @@ class TestNodeObject(db_base.DbTestCase, obj_utils.SchemasTestMixIn): self.assertRaises(exception.BadRequest, n.save) self.assertFalse(mock_update_node.called) + def test_save_with_conductor_group(self): + uuid = self.fake_node['uuid'] + with mock.patch.object(self.dbapi, 'get_node_by_uuid', + autospec=True) as mock_get_node: + mock_get_node.return_value = self.fake_node + with mock.patch.object(self.dbapi, 'update_node', + autospec=True) as mock_update_node: + mock_update_node.return_value = ( + db_utils.get_test_node(conductor_group='group1')) + n = objects.Node.get(self.context, uuid) + n.conductor_group = 'group1' + n.save() + self.assertTrue(mock_update_node.called) + mock_update_node.assert_called_once_with( + uuid, {'conductor_group': 'group1', + 'version': objects.Node.VERSION}) + + def test_save_with_conductor_group_uppercase(self): + uuid = self.fake_node['uuid'] + with mock.patch.object(self.dbapi, 'get_node_by_uuid', + autospec=True) as mock_get_node: + mock_get_node.return_value = self.fake_node + with mock.patch.object(self.dbapi, 'update_node', + autospec=True) as mock_update_node: + mock_update_node.return_value = ( + db_utils.get_test_node(conductor_group='group1')) + n = objects.Node.get(self.context, uuid) + n.conductor_group = 'GROUP1' + n.save() + mock_update_node.assert_called_once_with( + uuid, {'conductor_group': 'group1', + 'version': objects.Node.VERSION}) + + def test_save_with_conductor_group_fail(self): + uuid = self.fake_node['uuid'] + with mock.patch.object(self.dbapi, 'get_node_by_uuid', + autospec=True) as mock_get_node: + mock_get_node.return_value = self.fake_node + with mock.patch.object(self.dbapi, 'update_node', + autospec=True) as mock_update_node: + n = objects.Node.get(self.context, uuid) + n.conductor_group = 'group:1' + self.assertRaises(exception.InvalidConductorGroup, n.save) + self.assertFalse(mock_update_node.called) + def test_refresh(self): uuid = self.fake_node['uuid'] returns = [dict(self.fake_node, properties={"fake": "first"}), @@ -611,6 +656,56 @@ class TestConvertToVersion(db_base.DbTestCase): self.assertIsNone(node.fault) self.assertEqual({'fault': None}, node.obj_get_changes()) + def test_conductor_group_supported_set(self): + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + node.conductor_group = 'group1' + node.obj_reset_changes() + + node._convert_to_version('1.27') + + self.assertEqual('group1', node.conductor_group) + self.assertEqual({}, node.obj_get_changes()) + + def test_conductor_group_supported_unset(self): + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + delattr(node, 'conductor_group') + node.obj_reset_changes() + + node._convert_to_version('1.27') + + self.assertEqual('', node.conductor_group) + self.assertEqual({'conductor_group': ''}, node.obj_get_changes()) + + def test_conductor_group_unsupported_set(self): + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + node.conductor_group = 'group1' + node.obj_reset_changes() + + node._convert_to_version('1.26') + + self.assertNotIn('conductor_group', node) + self.assertEqual({}, node.obj_get_changes()) + + def test_conductor_group_unsupported_unset(self): + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + delattr(node, 'conductor_group') + node.obj_reset_changes() + + node._convert_to_version('1.26') + + self.assertNotIn('conductor_group', node) + self.assertEqual({}, node.obj_get_changes()) + + def test_conductor_group_unsupported_set_no_remove(self): + node = obj_utils.get_test_node(self.ctxt, **self.fake_node) + node.conductor_group = 'group1' + node.obj_reset_changes() + + node._convert_to_version('1.26', remove_unavailable_fields=False) + + self.assertEqual('', node.conductor_group) + self.assertEqual({'conductor_group': ''}, node.obj_get_changes()) + class TestNodePayloads(db_base.DbTestCase): diff --git a/ironic/tests/unit/objects/test_objects.py b/ironic/tests/unit/objects/test_objects.py index ee87a94e68..2102302e5e 100644 --- a/ironic/tests/unit/objects/test_objects.py +++ b/ironic/tests/unit/objects/test_objects.py @@ -664,12 +664,12 @@ class TestObject(_LocalTest, _TestObject): # version bump. It is an MD5 hash of the object fields and remotable methods. # The fingerprint values should only be changed if there is a version bump. expected_object_fingerprints = { - 'Node': '1.26-31732244b5bc3f8c334f77c03449f4c6', + 'Node': '1.27-129323d486c03a99de27053503b2cae3', 'MyObj': '1.5-9459d30d6954bffc7a9afd347a807ca6', 'Chassis': '1.3-d656e039fd8ae9f34efc232ab3980905', 'Port': '1.8-898a47921f4a1f53fcdddd4eeb179e0b', 'Portgroup': '1.4-71923a81a86743b313b190f5c675e258', - 'Conductor': '1.2-5091f249719d4a465062a1b3dc7f860d', + 'Conductor': '1.3-d3f53e853b4d58cae5bfbd9a8341af4a', 'EventType': '1.1-aa2ba1afd38553e3880c267404e8d370', 'NotificationPublisher': '1.0-51a09397d6c0687771fb5be9a999605d', 'NodePayload': '1.9-c0aa5dd602adca3a28f091ca7848a41b',