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

@@ -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.
 

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.")
 

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'],

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)

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

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.')),
 ]
 
 

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)

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

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)

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())

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()),
     }

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):

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):
 

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',